A re-roll of [1] which aims to address the concerns about the previous 8-part series being too large to fix a release regression. "If it isn't bolted down, throw it overboard!". The main change here is: * The new "ungroup" parameter is now passed via an "extern" parameter. * Tests for existing run-command.c behavior (not narrowly needed for the regression fix) are gone. * Adding an INIT macro is gone, instead we explicitly initialize to NULL. * Stray bugfix for existing hook test is gone. etc. I think all of those still make sense, but they're something I can rebase on this topic once it (hopefully) lands. In the meantime the updated commit messages for the remaining two (see start of the range-diff below) argue for this being a a safe API change, even if the interface is a bit nasty. 1. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20220518T195858Z-avarab@xxxxxxxxx Ævar Arnfjörð Bjarmason (2): run-command: add an "ungroup" option to run_process_parallel() hook API: fix v2.36.0 regression: hooks should be connected to a TTY hook.c | 1 + run-command.c | 88 ++++++++++++++++++++++++++++--------- run-command.h | 31 ++++++++++--- t/helper/test-run-command.c | 19 ++++++-- t/t0061-run-command.sh | 35 +++++++++++++++ t/t1800-hook.sh | 37 ++++++++++++++++ 6 files changed, 181 insertions(+), 30 deletions(-) Range-diff against v2: 1: 26a81eff267 < -: ----------- run-command tests: change if/if/... to if/else if/else 2: 5f0a6e9925f < -: ----------- run-command API: use "opts" struct for run_processes_parallel{,_tr2}() 3: a8e1fc07b65 < -: ----------- run-command tests: test stdout of run_command_parallel() 4: 663936fb4ad < -: ----------- run-command.c: add an initializer for "struct parallel_processes" 5: c2e015ed840 ! 1: aabd99de680 run-command: add an "ungroup" option to run_process_parallel() @@ Commit message user, i.e. they'd typically use one mode or the other, and would know whether they'd provided "ungroup" or not. + We could also avoid the strbuf_init() for "buffered_output" by having + "struct parallel_processes" use a static PARALLEL_PROCESSES_INIT + initializer, but let's leave that cleanup for later. + + Using a global "run_processes_parallel_ungroup" variable to enable + this option is rather nasty, but is being done here to produce as + minimal of a change as possible for a subsequent regression fix. This + change is extracted from a larger initial version[1] which ends up + with a better end-state for the API, but in doing so needed to modify + all existing callers of the API. Let's defer that for now, and + narrowly focus on what we need for fixing the regression in the + subsequent commit. + + It's safe to do this with a global variable because: + + A) hook.c is the only user of it that sets it to non-zero, and before + we'll get any other API users we'll refactor away this method of + passing in the option, i.e. re-roll [1]. + + B) Even if hook.c wasn't the only user we don't have callers of this + API that concurrently invoke this parallel process starting API + itself in parallel. + + As noted above "A" && "B" are rather nasty, and we don't want to live + with those caveats long-term, but for now they should be an acceptable + compromise. + + 1. https://lore.kernel.org/git/cover-v2-0.8-00000000000-20220518T195858Z-avarab@xxxxxxxxx/ + Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## run-command.c ## @@ run-command.c: int pipe_command(struct child_process *cmd, + GIT_CP_WAIT_CLEANUP, /* only for !ungroup */ }; ++int run_processes_parallel_ungroup; struct parallel_processes { + void *data; + @@ run-command.c: struct parallel_processes { struct pollfd *pfd; @@ run-command.c: struct parallel_processes { int output_owner; struct strbuf buffered_output; /* of finished children */ +@@ run-command.c: 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, const int ungroup) + { + int i; + @@ run-command.c: static void pp_init(struct parallel_processes *pp, pp->nr_processes = 0; pp->output_owner = 0; pp->shutdown = 0; -+ pp->ungroup = opts->ungroup; ++ pp->ungroup = ungroup; CALLOC_ARRAY(pp->children, n); - CALLOC_ARRAY(pp->pfd, n); -+ if (!pp->ungroup) ++ 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); @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp) strbuf_addbuf(&pp->buffered_output, &pp->children[i].err); strbuf_reset(&pp->children[i].err); } else { -@@ run-command.c: int run_processes_parallel(struct run_process_parallel_opts *opts) +@@ run-command.c: int run_processes_parallel(int n, + int output_timeout = 100; + int spawn_cap = 4; + struct parallel_processes pp; ++ const int ungroup = run_processes_parallel_ungroup; + +- 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 && +@@ run-command.c: int run_processes_parallel(int n, } if (!pp.nr_processes) break; - pp_buffer_stderr(&pp, output_timeout); - pp_output(&pp); -+ if (opts->ungroup) { ++ if (ungroup) { + pp_mark_working_for_cleanup(&pp); + } else { + pp_buffer_stderr(&pp, output_timeout); @@ run-command.h: typedef int (*start_failure_fn)(struct strbuf *out, * 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. @@ run-command.h: typedef int (*task_finished_fn)(int result, - * - * jobs: see 'n' in run_processes_parallel() below. - * -+ * ungroup: Ungroup output. Output is printed as soon as possible and -+ * bypasses run-command's internal processing. This may cause output -+ * from different commands to be mixed. -+ * - * *_fn & data: see run_processes_parallel() below. - */ - struct run_process_parallel_opts -@@ run-command.h: struct run_process_parallel_opts - const char *tr2_label; - - int jobs; -+ unsigned int ungroup:1; - - get_next_task_fn get_next_task; - start_failure_fn start_failure; -@@ run-command.h: struct run_process_parallel_opts * * The children started via this function run in parallel. Their output * (both stdout and stderr) is routed to stderr in a manner that output @@ run-command.h: struct run_process_parallel_opts + * 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. */ - int run_processes_parallel(struct run_process_parallel_opts *opts); - ++extern int run_processes_parallel_ungroup; + int run_processes_parallel(int n, + get_next_task_fn, + start_failure_fn, ## t/helper/test-run-command.c ## @@ t/helper/test-run-command.c: static int parallel_next(struct child_process *cp, @@ t/helper/test-run-command.c: static int task_finished(int result, } @@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv) - opts.jobs = jobs; - opts.data = &proc; + strvec_clear(&proc.args); + strvec_pushv(&proc.args, (const char **)argv + 3); -- if (!strcmp(argv[1], "run-command-parallel")) { -+ if (!strcmp(argv[1], "run-command-parallel") || -+ !strcmp(argv[1], "run-command-parallel-ungroup")) { - next_fn = parallel_next; -- } else if (!strcmp(argv[1], "run-command-abort")) { -+ } else if (!strcmp(argv[1], "run-command-abort") || -+ !strcmp(argv[1], "run-command-abort-ungroup")) { - next_fn = parallel_next; - finished_fn = task_finished; -- } else if (!strcmp(argv[1], "run-command-no-jobs")) { -+ } else if (!strcmp(argv[1], "run-command-no-jobs") || -+ !strcmp(argv[1], "run-command-no-jobs-ungroup")) { - next_fn = no_job; - finished_fn = task_finished; - } else { -@@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv) - return 1; - } - -+ opts.ungroup = ends_with(argv[1], "-ungroup"); - opts.get_next_task = next_fn; - opts.task_finished = finished_fn; - exit(run_processes_parallel(&opts)); ++ if (getenv("RUN_PROCESSES_PARALLEL_UNGROUP")) ++ run_processes_parallel_ungroup = 1; ++ + if (!strcmp(argv[1], "run-command-parallel")) + exit(run_processes_parallel(jobs, parallel_next, + NULL, NULL, &proc)); ## t/t0061-run-command.sh ## @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with more jobs available than - test_cmp expect err + test_cmp expect actual ' +test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' ' -+ test-tool run-command run-command-parallel-ungroup 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err && ++ RUN_PROCESSES_PARALLEL_UNGROUP=1 \ ++ test-tool run-command 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" >out 2>err && - test_must_be_empty out && - test_cmp expect err + 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 run-command-parallel-ungroup 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err && ++ RUN_PROCESSES_PARALLEL_UNGROUP=1 \ ++ test-tool run-command 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" >out 2>err && - test_must_be_empty out && - test_cmp expect err + 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 run-command-parallel-ungroup 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err && ++ RUN_PROCESSES_PARALLEL_UNGROUP=1 \ ++ test-tool run-command 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 +' @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m preloaded output of a child asking for a quick stop @@ t/t0061-run-command.sh: test_expect_success 'run_command is asked to abort gracefully' ' - test_cmp expect err + test_cmp expect actual ' +test_expect_success 'run_command is asked to abort gracefully (ungroup)' ' -+ test-tool run-command run-command-abort-ungroup 3 false >out 2>err && ++ RUN_PROCESSES_PARALLEL_UNGROUP=1 \ ++ test-tool run-command run-command-abort 3 false >out 2>err && + test_must_be_empty out && + test_line_count = 6 err +' @@ t/t0061-run-command.sh: test_expect_success 'run_command is asked to abort grace no further jobs available EOF @@ t/t0061-run-command.sh: test_expect_success 'run_command outputs ' ' - test_cmp expect err + test_cmp expect actual ' +test_expect_success 'run_command outputs (ungroup) ' ' -+ test-tool run-command run-command-no-jobs-ungroup 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err && ++ RUN_PROCESSES_PARALLEL_UNGROUP=1 \ ++ test-tool run-command 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 +' 6: 84e92c6f7c7 < -: ----------- hook tests: fix redirection logic error in 96e7225b310 7: bf7d871565f < -: ----------- hook API: don't redundantly re-set "no_stdin" and "stdout_to_stderr" 8: 238155fcb9d ! 2: ec27e3906e1 hook API: fix v2.36.0 regression: hooks should be connected to a TTY @@ Commit message './git hook run seq-hook' in 'HEAD~0' ran 1.30 ± 0.02 times faster than './git hook run seq-hook' in 'origin/master' - In the preceding commit we removed the "stdout_to_stderr=1" assignment - as being redundant. This change brings it back as with ".ungroup=1" - the run_process_parallel() function doesn't provide them for us - implicitly. - - As an aside omitting the stdout_to_stderr=1 here would have all tests - pass, except those that test "git hook run" itself in - t1800-hook.sh. But our tests passing is the result of another test - blind spot, as was the case with the regression being fixed here. The - "stdout_to_stderr=1" for hooks is long-standing behavior, see - e.g. 1d9e8b56fe3 (Split back out update_hook handling in receive-pack, - 2007-03-10) and other follow-up commits (running "git log" with - "--reverse -p -Gstdout_to_stderr" is a good start). - 1. https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@xxxxxxxxxxxxxx/ Reported-by: Anthony Sottile <asottile@xxxxxxxxx> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> ## hook.c ## -@@ hook.c: static int pick_next_hook(struct child_process *cp, - return 0; - - strvec_pushv(&cp->env_array, hook_cb->options->env.v); -+ cp->stdout_to_stderr = 1; /* because of .ungroup = 1 */ - cp->trace2_hook_name = hook_cb->hook_name; - cp->dir = hook_cb->options->dir; - @@ hook.c: int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options) - .tr2_label = hook_name, - - .jobs = jobs, -+ .ungroup = jobs == 1, - - .get_next_task = pick_next_hook, - .start_failure = notify_start_failure, -@@ hook.c: int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options) - if (!options) - BUG("a struct run_hooks_opt must be provided to run_hooks"); - -+ if (jobs != 1 || !run_opts.ungroup) -+ BUG("TODO: think about & document order & interleaving of parallel hook output"); -+ - if (options->invoked_hook) - *options->invoked_hook = 0; + cb_data.hook_path = abs_path.buf; + } ++ run_processes_parallel_ungroup = 1; + run_processes_parallel_tr2(jobs, + pick_next_hook, + notify_start_failure, ## t/t1800-hook.sh ## @@ t/t1800-hook.sh: test_description='git-hook command' -- 2.36.1.1046.g586767a6996
