diff options
author | Junio C Hamano <gitster@pobox.com> | 2022-06-14 01:53:41 +0300 |
---|---|---|
committer | Junio C Hamano <gitster@pobox.com> | 2022-06-14 01:53:41 +0300 |
commit | 1a7f6be5b17f572fc68ff2a2e0c079d50c671c74 (patch) | |
tree | 1968c2539b29085e584648a5b8079551ec61b752 | |
parent | 66c2948ffd6e376447252a3a72f7ce78eeef3e52 (diff) | |
parent | a082345372e14ecd30a84a790eda0c38154e4602 (diff) |
Merge branch 'ab/hooks-regression-fix'
In Git 2.36 we revamped the way how hooks are invoked. One change
that is end-user visible is that the output of a hook is no longer
directly connected to the standard output of "git" that spawns the
hook, which was noticed post release. This is getting corrected.
* ab/hooks-regression-fix:
hook API: fix v2.36.0 regression: hooks should be connected to a TTY
run-command: add an "ungroup" option to run_process_parallel()
-rw-r--r-- | hook.c | 1 | ||||
-rw-r--r-- | run-command.c | 70 | ||||
-rw-r--r-- | run-command.h | 30 | ||||
-rw-r--r-- | t/helper/test-run-command.c | 22 | ||||
-rwxr-xr-x | t/t0061-run-command.sh | 30 | ||||
-rwxr-xr-x | t/t1800-hook.sh | 31 |
6 files changed, 155 insertions, 29 deletions
@@ -144,6 +144,7 @@ int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options) cb_data.hook_path = abs_path.buf; } + run_processes_parallel_ungroup = 1; run_processes_parallel_tr2(jobs, pick_next_hook, notify_start_failure, diff --git a/run-command.c b/run-command.c index 789eaa5975..8e36b8ee74 100644 --- a/run-command.c +++ b/run-command.c @@ -1472,6 +1472,7 @@ enum child_state { GIT_CP_WAIT_CLEANUP, }; +int run_processes_parallel_ungroup; struct parallel_processes { void *data; @@ -1495,6 +1496,7 @@ struct parallel_processes { struct pollfd *pfd; unsigned shutdown : 1; + unsigned ungroup : 1; int output_owner; struct strbuf buffered_output; /* of finished children */ @@ -1538,7 +1540,7 @@ static void pp_init(struct parallel_processes *pp, get_next_task_fn get_next_task, start_failure_fn start_failure, task_finished_fn task_finished, - void *data) + void *data, int ungroup) { int i; @@ -1560,15 +1562,21 @@ static void pp_init(struct parallel_processes *pp, pp->nr_processes = 0; pp->output_owner = 0; pp->shutdown = 0; + pp->ungroup = ungroup; CALLOC_ARRAY(pp->children, n); - CALLOC_ARRAY(pp->pfd, n); + if (pp->ungroup) + pp->pfd = NULL; + else + CALLOC_ARRAY(pp->pfd, n); strbuf_init(&pp->buffered_output, 0); for (i = 0; i < n; i++) { strbuf_init(&pp->children[i].err, 0); child_process_init(&pp->children[i].process); - pp->pfd[i].events = POLLIN | POLLHUP; - pp->pfd[i].fd = -1; + if (pp->pfd) { + pp->pfd[i].events = POLLIN | POLLHUP; + pp->pfd[i].fd = -1; + } } pp_for_signal = pp; @@ -1616,24 +1624,31 @@ static int pp_start_one(struct parallel_processes *pp) BUG("bookkeeping is hard"); code = pp->get_next_task(&pp->children[i].process, - &pp->children[i].err, + pp->ungroup ? NULL : &pp->children[i].err, pp->data, &pp->children[i].data); if (!code) { - strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); - strbuf_reset(&pp->children[i].err); + if (!pp->ungroup) { + strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); + strbuf_reset(&pp->children[i].err); + } return 1; } - pp->children[i].process.err = -1; - pp->children[i].process.stdout_to_stderr = 1; + if (!pp->ungroup) { + pp->children[i].process.err = -1; + pp->children[i].process.stdout_to_stderr = 1; + } pp->children[i].process.no_stdin = 1; if (start_command(&pp->children[i].process)) { - code = pp->start_failure(&pp->children[i].err, + code = pp->start_failure(pp->ungroup ? NULL : + &pp->children[i].err, pp->data, pp->children[i].data); - strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); - strbuf_reset(&pp->children[i].err); + if (!pp->ungroup) { + strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); + strbuf_reset(&pp->children[i].err); + } if (code) pp->shutdown = 1; return code; @@ -1641,7 +1656,8 @@ static int pp_start_one(struct parallel_processes *pp) pp->nr_processes++; pp->children[i].state = GIT_CP_WORKING; - pp->pfd[i].fd = pp->children[i].process.err; + if (pp->pfd) + pp->pfd[i].fd = pp->children[i].process.err; return 0; } @@ -1675,6 +1691,7 @@ static void pp_buffer_stderr(struct parallel_processes *pp, int output_timeout) static void pp_output(struct parallel_processes *pp) { int i = pp->output_owner; + if (pp->children[i].state == GIT_CP_WORKING && pp->children[i].err.len) { strbuf_write(&pp->children[i].err, stderr); @@ -1697,7 +1714,7 @@ static int pp_collect_finished(struct parallel_processes *pp) code = finish_command(&pp->children[i].process); - code = pp->task_finished(code, + code = pp->task_finished(code, pp->ungroup ? NULL : &pp->children[i].err, pp->data, pp->children[i].data); @@ -1708,10 +1725,13 @@ static int pp_collect_finished(struct parallel_processes *pp) pp->nr_processes--; pp->children[i].state = GIT_CP_FREE; - pp->pfd[i].fd = -1; + if (pp->pfd) + pp->pfd[i].fd = -1; child_process_init(&pp->children[i].process); - if (i != pp->output_owner) { + if (pp->ungroup) { + ; /* no strbuf_*() work to do here */ + } else if (i != pp->output_owner) { strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); strbuf_reset(&pp->children[i].err); } else { @@ -1748,9 +1768,14 @@ int run_processes_parallel(int n, int i, code; int output_timeout = 100; int spawn_cap = 4; + int ungroup = run_processes_parallel_ungroup; struct parallel_processes pp; - pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb); + /* unset for the next API user */ + run_processes_parallel_ungroup = 0; + + pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb, + ungroup); while (1) { for (i = 0; i < spawn_cap && !pp.shutdown && @@ -1767,8 +1792,15 @@ int run_processes_parallel(int n, } if (!pp.nr_processes) break; - pp_buffer_stderr(&pp, output_timeout); - pp_output(&pp); + if (ungroup) { + int i; + + for (i = 0; i < pp.max_processes; i++) + pp.children[i].state = GIT_CP_WAIT_CLEANUP; + } else { + pp_buffer_stderr(&pp, output_timeout); + pp_output(&pp); + } code = pp_collect_finished(&pp); if (code) { pp.shutdown = 1; diff --git a/run-command.h b/run-command.h index 8a700da099..0e85e5846a 100644 --- a/run-command.h +++ b/run-command.h @@ -405,6 +405,9 @@ void check_pipe(int err); * pp_cb is the callback cookie as passed to run_processes_parallel. * You can store a child process specific callback cookie in pp_task_cb. * + * See run_processes_parallel() below for a discussion of the "struct + * strbuf *out" parameter. + * * Even after returning 0 to indicate that there are no more processes, * this function will be called again until there are no more running * child processes. @@ -423,9 +426,8 @@ typedef int (*get_next_task_fn)(struct child_process *cp, * This callback is called whenever there are problems starting * a new process. * - * You must not write to stdout or stderr in this function. Add your - * message to the strbuf out instead, which will be printed without - * messing up the output of the other parallel processes. + * See run_processes_parallel() below for a discussion of the "struct + * strbuf *out" parameter. * * pp_cb is the callback cookie as passed into run_processes_parallel, * pp_task_cb is the callback cookie as passed into get_next_task_fn. @@ -441,9 +443,8 @@ typedef int (*start_failure_fn)(struct strbuf *out, /** * This callback is called on every child process that finished processing. * - * You must not write to stdout or stderr in this function. Add your - * message to the strbuf out instead, which will be printed without - * messing up the output of the other parallel processes. + * See run_processes_parallel() below for a discussion of the "struct + * strbuf *out" parameter. * * pp_cb is the callback cookie as passed into run_processes_parallel, * pp_task_cb is the callback cookie as passed into get_next_task_fn. @@ -464,11 +465,26 @@ typedef int (*task_finished_fn)(int result, * * The children started via this function run in parallel. Their output * (both stdout and stderr) is routed to stderr in a manner that output - * from different tasks does not interleave. + * from different tasks does not interleave (but see "ungroup" below). * * start_failure_fn and task_finished_fn can be NULL to omit any * special handling. + * + * If the "ungroup" option isn't specified, the API will set the + * "stdout_to_stderr" parameter in "struct child_process" and provide + * the callbacks with a "struct strbuf *out" parameter to write output + * to. In this case the callbacks must not write to stdout or + * stderr as such output will mess up the output of the other parallel + * processes. If "ungroup" option is specified callbacks will get a + * NULL "struct strbuf *out" parameter, and are responsible for + * emitting their own output, including dealing with any race + * conditions due to writing in parallel to stdout and stderr. + * The "ungroup" option can be enabled by setting the global + * "run_processes_parallel_ungroup" to "1" before invoking + * run_processes_parallel(), it will be set back to "0" as soon as the + * API reads that setting. */ +extern int run_processes_parallel_ungroup; int run_processes_parallel(int n, get_next_task_fn, start_failure_fn, diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index 9050e3113c..c9283b47af 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -31,7 +31,11 @@ static int parallel_next(struct child_process *cp, return 0; strvec_pushv(&cp->args, d->args.v); - strbuf_addstr(err, "preloaded output of a child\n"); + if (err) + strbuf_addstr(err, "preloaded output of a child\n"); + else + fprintf(stderr, "preloaded output of a child\n"); + number_callbacks++; return 1; } @@ -41,7 +45,10 @@ static int no_job(struct child_process *cp, void *cb, void **task_cb) { - strbuf_addstr(err, "no further jobs available\n"); + if (err) + strbuf_addstr(err, "no further jobs available\n"); + else + fprintf(stderr, "no further jobs available\n"); return 0; } @@ -50,7 +57,10 @@ static int task_finished(int result, void *pp_cb, void *pp_task_cb) { - strbuf_addstr(err, "asking for a quick stop\n"); + if (err) + strbuf_addstr(err, "asking for a quick stop\n"); + else + fprintf(stderr, "asking for a quick stop\n"); return 1; } @@ -407,6 +417,12 @@ int cmd__run_command(int argc, const char **argv) if (!strcmp(argv[1], "run-command")) exit(run_command(&proc)); + if (!strcmp(argv[1], "--ungroup")) { + argv += 1; + argc -= 1; + run_processes_parallel_ungroup = 1; + } + jobs = atoi(argv[2]); strvec_clear(&proc.args); strvec_pushv(&proc.args, (const char **)argv + 3); diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh index ee281909bc..7b5423eebd 100755 --- a/t/t0061-run-command.sh +++ b/t/t0061-run-command.sh @@ -134,16 +134,34 @@ test_expect_success 'run_command runs in parallel with more jobs available than test_cmp expect actual ' +test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' ' + test-tool run-command --ungroup run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err && + test_line_count = 8 out && + test_line_count = 4 err +' + test_expect_success 'run_command runs in parallel with as many jobs as tasks' ' test-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual && test_cmp expect actual ' +test_expect_success 'run_command runs ungrouped in parallel with as many jobs as tasks' ' + test-tool run-command --ungroup run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err && + test_line_count = 8 out && + test_line_count = 4 err +' + test_expect_success 'run_command runs in parallel with more tasks than jobs available' ' test-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual && test_cmp expect actual ' +test_expect_success 'run_command runs ungrouped in parallel with more tasks than jobs available' ' + test-tool run-command --ungroup run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err && + test_line_count = 8 out && + test_line_count = 4 err +' + cat >expect <<-EOF preloaded output of a child asking for a quick stop @@ -158,6 +176,12 @@ test_expect_success 'run_command is asked to abort gracefully' ' test_cmp expect actual ' +test_expect_success 'run_command is asked to abort gracefully (ungroup)' ' + test-tool run-command --ungroup run-command-abort 3 false >out 2>err && + test_must_be_empty out && + test_line_count = 6 err +' + cat >expect <<-EOF no further jobs available EOF @@ -167,6 +191,12 @@ test_expect_success 'run_command outputs ' ' test_cmp expect actual ' +test_expect_success 'run_command outputs (ungroup) ' ' + test-tool run-command --ungroup run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err && + test_must_be_empty out && + test_cmp expect err +' + test_trace () { expect="$1" shift diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 26ed5e11bc..210f429887 100755 --- a/t/t1800-hook.sh +++ b/t/t1800-hook.sh @@ -4,6 +4,7 @@ test_description='git-hook command' TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-terminal.sh test_expect_success 'git hook usage' ' test_expect_code 129 git hook && @@ -120,4 +121,34 @@ test_expect_success 'git -c core.hooksPath=<PATH> hook run' ' test_cmp expect actual ' +test_hook_tty () { + cat >expect <<-\EOF + STDOUT TTY + STDERR TTY + EOF + + test_when_finished "rm -rf repo" && + git init repo && + + test_commit -C repo A && + test_commit -C repo B && + git -C repo reset --soft HEAD^ && + + test_hook -C repo pre-commit <<-EOF && + test -t 1 && echo STDOUT TTY >>actual || echo STDOUT NO TTY >>actual && + test -t 2 && echo STDERR TTY >>actual || echo STDERR NO TTY >>actual + EOF + + test_terminal git -C repo "$@" && + test_cmp expect repo/actual +} + +test_expect_success TTY 'git hook run: stdout and stderr are connected to a TTY' ' + test_hook_tty hook run pre-commit +' + +test_expect_success TTY 'git commit: stdout and stderr are connected to a TTY' ' + test_hook_tty commit -m"B.new" +' + test_done |