[PATCH v2 0/8] hook API: connect hooks to the TTY again, fixes a v2.36.0 regression

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

A re-roll of v1[1]. I believe this addresses all comments on the v1
(but perhaps I missed something). Changes:

 * The run_processes_parallel() now takes only one argument, the new
   "opts" struct which has options, callbacks etc. This will also make
   the subsequent config-based hooks topic less churny (it needs new
   callbacks).

   As a result the whole internal *_tr2() wrapper/static function are
   gone.

 * Replaced checks of "ungroup" with whether we have a NULL or not
   (e.g. for pp->pfd), also for free().

 * Typo/grammar fixes in commit messages.

 * Hopefully the 8/8 is less confusing vis-a-vis
   https://lore.kernel.org/git/xmqqfslva3mx.fsf@gitster.g/; I.e. now
   we only add "stdout_to_stderr".

 * The 01/08 and 04/08 are new: Splitting those out made subsequent
   diffs smaller.

 * Tweaked 5/8 a bit to make the diff smaller.

 * Used "err" and "out", not "actual" and "out" in tests, per Junio's
   suggestion.

Passing CI for this series at: https://github.com/avar/git/actions/runs/2346571047

1. https://lore.kernel.org/git/cover-0.6-00000000000-20220421T122108Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (8):
  run-command tests: change if/if/... to if/else if/else
  run-command API: use "opts" struct for run_processes_parallel{,_tr2}()
  run-command tests: test stdout of run_command_parallel()
  run-command.c: add an initializer for "struct parallel_processes"
  run-command: add an "ungroup" option to run_process_parallel()
  hook tests: fix redirection logic error in 96e7225b310
  hook API: don't redundantly re-set "no_stdin" and "stdout_to_stderr"
  hook API: fix v2.36.0 regression: hooks should be connected to a TTY

 builtin/fetch.c             |  18 +++--
 builtin/submodule--helper.c |  15 ++--
 hook.c                      |  28 +++++---
 run-command.c               | 132 +++++++++++++++++++++++-------------
 run-command.h               |  66 ++++++++++++++----
 submodule.c                 |  18 +++--
 t/helper/test-run-command.c |  65 ++++++++++++------
 t/t0061-run-command.sh      |  55 ++++++++++++---
 t/t1800-hook.sh             |  39 ++++++++++-
 9 files changed, 316 insertions(+), 120 deletions(-)

Range-diff against v1:
-:  ----------- > 1:  26a81eff267 run-command tests: change if/if/... to if/else if/else
1:  8bf71ce63dd ! 2:  5f0a6e9925f run-command API: replace run_processes_parallel_tr2() with opts struct
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    run-command API: replace run_processes_parallel_tr2() with opts struct
    +    run-command API: use "opts" struct for run_processes_parallel{,_tr2}()
     
    -    Add a new "struct run_process_parallel_opts" to cover the trace2
    -    use-case added in ee4512ed481 (trace2: create new combined trace
    -    facility, 2019-02-22). A subsequent commit will add more options, and
    -    having a proliferation of new functions or extra parameters would
    -    result in needless churn.
    +    Add a new "struct run_process_parallel_opts" to replace the growing
    +    run_processes_parallel() and run_processes_parallel_tr2() argument
    +    lists. This refactoring makes it easier to add new options and
    +    parameters easier.
     
    -    It makes for a smaller change to make run_processes_parallel() and
    -    run_processes_parallel_tr2() wrapper functions for the new "static"
    -    run_processes_parallel_1(), which contains the main logic. We pass
    -    down "opts" to the *_1() function even though it isn't used there
    -    yet (only in the *_tr2() function), a subsequent commit will make more
    -    use of it.
    +    The *_tr2() variant of the function was added in ee4512ed481 (trace2:
    +    create new combined trace facility, 2019-02-22), and has subsequently
    +    been used by every caller except t/helper/test-run-command.c.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ builtin/fetch.c: static int fetch_multiple(struct string_list *list, int max_chi
     +		struct run_process_parallel_opts run_opts = {
     +			.tr2_category = "fetch",
     +			.tr2_label = "parallel/fetch",
    ++
    ++			.jobs = max_children,
    ++
    ++			.get_next_task = &fetch_next_remote,
    ++			.start_failure = &fetch_failed_to_start,
    ++			.task_finished = &fetch_finished,
    ++			.data = &state,
     +		};
      
      		strvec_push(&argv, "--end-of-options");
    @@ builtin/fetch.c: static int fetch_multiple(struct string_list *list, int max_chi
     -						    &fetch_finished,
     -						    &state,
     -						    "fetch", "parallel/fetch");
    -+		result = run_processes_parallel(max_children,
    -+						&fetch_next_remote,
    -+						&fetch_failed_to_start,
    -+						&fetch_finished, &state,
    -+						&run_opts);
    ++		result = run_processes_parallel(&run_opts);
      
      		if (!result)
      			result = state.result;
    @@ builtin/submodule--helper.c: static int update_submodules(struct update_data *up
     +	struct run_process_parallel_opts run_opts = {
     +		.tr2_category = "submodule",
     +		.tr2_label = "parallel/update",
    ++
    ++		.get_next_task = update_clone_get_next_task,
    ++		.start_failure = update_clone_start_failure,
    ++		.task_finished = update_clone_task_finished,
    ++		.data = &suc,
     +	};
      
      	suc.update_data = update_data;
    @@ builtin/submodule--helper.c: static int update_submodules(struct update_data *up
     -				   update_clone_start_failure,
     -				   update_clone_task_finished, &suc, "submodule",
     -				   "parallel/update");
    -+	run_processes_parallel(suc.update_data->max_jobs,
    -+			       update_clone_get_next_task,
    -+			       update_clone_start_failure,
    -+			       update_clone_task_finished, &suc, &run_opts);
    ++	run_opts.jobs = suc.update_data->max_jobs;
    ++	run_processes_parallel(&run_opts);
      
      	/*
      	 * We saved the output and put it out all at once now.
     
      ## hook.c ##
     @@ hook.c: int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
    + 		.options = options,
    + 	};
      	const char *const hook_path = find_hook(hook_name);
    - 	int jobs = 1;
    +-	int jobs = 1;
    ++	const int jobs = 1;
      	int ret = 0;
     +	struct run_process_parallel_opts run_opts = {
     +		.tr2_category = "hook",
     +		.tr2_label = hook_name,
    ++
    ++		.jobs = jobs,
    ++
    ++		.get_next_task = pick_next_hook,
    ++		.start_failure = notify_start_failure,
    ++		.task_finished = notify_hook_finished,
    ++		.data = &cb_data,
     +	};
      
      	if (!options)
    @@ hook.c: int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
     -				   &cb_data,
     -				   "hook",
     -				   hook_name);
    -+	run_processes_parallel(jobs, pick_next_hook, notify_start_failure,
    -+			       notify_hook_finished, &cb_data, &run_opts);
    ++	run_processes_parallel(&run_opts);
      	ret = cb_data.rc;
      cleanup:
      	strbuf_release(&abs_path);
     
      ## run-command.c ##
    +@@ run-command.c: static void handle_children_on_signal(int signo)
    + }
    + 
    + static void pp_init(struct parallel_processes *pp,
    +-		    int n,
    +-		    get_next_task_fn get_next_task,
    +-		    start_failure_fn start_failure,
    +-		    task_finished_fn task_finished,
    +-		    void *data)
    ++		    struct run_process_parallel_opts *opts)
    + {
    + 	int i;
    ++	int n = opts->jobs;
    ++	void *data = opts->data;
    ++	get_next_task_fn get_next_task = opts->get_next_task;
    ++	start_failure_fn start_failure = opts->start_failure;
    ++	task_finished_fn task_finished = opts->task_finished;
    + 
    + 	if (n < 1)
    + 		n = online_cpus();
     @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp)
      	return result;
      }
    @@ run-command.c: static int pp_collect_finished(struct parallel_processes *pp)
     -			   start_failure_fn start_failure,
     -			   task_finished_fn task_finished,
     -			   void *pp_cb)
    -+static int run_processes_parallel_1(int n, get_next_task_fn get_next_task,
    -+				    start_failure_fn start_failure,
    -+				    task_finished_fn task_finished,
    -+				    void *pp_cb,
    -+				    struct run_process_parallel_opts *opts)
    ++int run_processes_parallel(struct run_process_parallel_opts *opts)
      {
      	int i, code;
      	int output_timeout = 100;
    + 	int spawn_cap = 4;
    + 	struct parallel_processes pp;
    ++	const char *tr2_category = opts->tr2_category;
    ++	const char *tr2_label = opts->tr2_label;
    ++	const int do_trace2 = tr2_category && tr2_label;
    ++	const int n = opts->jobs;
    + 
    +-	pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb);
    ++	if (do_trace2)
    ++		trace2_region_enter_printf(tr2_category, tr2_label, NULL,
    ++					   "max:%d", ((n < 1) ? online_cpus()
    ++						      : n));
    ++
    ++	pp_init(&pp, opts);
    + 	while (1) {
    + 		for (i = 0;
    + 		    i < spawn_cap && !pp.shutdown &&
     @@ run-command.c: int run_processes_parallel(int n,
    - 	return 0;
    - }
    + 	}
      
    + 	pp_cleanup(&pp);
    +-	return 0;
    +-}
    +-
     -int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task,
     -			       start_failure_fn start_failure,
     -			       task_finished_fn task_finished, void *pp_cb,
     -			       const char *tr2_category, const char *tr2_label)
    -+static int run_processes_parallel_tr2(int n, get_next_task_fn get_next_task,
    -+				      start_failure_fn start_failure,
    -+				      task_finished_fn task_finished,
    -+				      void *pp_cb,
    -+				      struct run_process_parallel_opts *opts)
    - {
    -+	const char *tr2_category = opts->tr2_category;
    -+	const char *tr2_label = opts->tr2_label;
    - 	int result;
    +-{
    +-	int result;
      
    - 	trace2_region_enter_printf(tr2_category, tr2_label, NULL, "max:%d",
    - 				   ((n < 1) ? online_cpus() : n));
    +-	trace2_region_enter_printf(tr2_category, tr2_label, NULL, "max:%d",
    +-				   ((n < 1) ? online_cpus() : n));
    ++	if (do_trace2)
    ++		trace2_region_leave(tr2_category, tr2_label, NULL);
      
     -	result = run_processes_parallel(n, get_next_task, start_failure,
     -					task_finished, pp_cb);
    -+	result = run_processes_parallel_1(n, get_next_task, start_failure,
    -+					  task_finished, pp_cb, opts);
    - 
    - 	trace2_region_leave(tr2_category, tr2_label, NULL);
    - 
    - 	return result;
    +-
    +-	trace2_region_leave(tr2_category, tr2_label, NULL);
    +-
    +-	return result;
    ++	return 0;
      }
      
    -+int run_processes_parallel(int n, get_next_task_fn get_next_task,
    -+			   start_failure_fn start_failure,
    -+			   task_finished_fn task_finished, void *pp_cb,
    -+			   struct run_process_parallel_opts *opts)
    -+{
    -+	if (opts->tr2_category && opts->tr2_label)
    -+		return run_processes_parallel_tr2(n, get_next_task,
    -+						  start_failure, task_finished,
    -+						  pp_cb, opts);
    -+
    -+	return run_processes_parallel_1(n, get_next_task, start_failure,
    -+					task_finished, pp_cb, opts);
    -+}
    -+
    -+
      int run_auto_maintenance(int quiet)
    - {
    - 	int enabled;
     
      ## run-command.h ##
     @@ run-command.h: typedef int (*task_finished_fn)(int result,
    - 				void *pp_cb,
      				void *pp_task_cb);
      
    -+/**
    + /**
     + * Options to pass to run_processes_parallel(), { 0 }-initialized
     + * means no options. Fields:
     + *
     + * tr2_category & tr2_label: sets the trace2 category and label for
     + * logging. These must either be unset, or both of them must be set.
    ++ *
    ++ * jobs: see 'n' in run_processes_parallel() below.
    ++ *
    ++ * *_fn & data: see run_processes_parallel() below.
     + */
     +struct run_process_parallel_opts
     +{
     +	const char *tr2_category;
     +	const char *tr2_label;
    ++
    ++	int jobs;
    ++
    ++	get_next_task_fn get_next_task;
    ++	start_failure_fn start_failure;
    ++	task_finished_fn task_finished;
    ++	void *data;
     +};
     +
    - /**
    ++/**
    ++ * Options are passed via the "struct run_process_parallel_opts" above.
    ++
       * Runs up to n processes at the same time. Whenever a process can be
       * started, the callback get_next_task_fn is called to obtain the data
    +  * required to start another child process.
     @@ run-command.h: typedef int (*task_finished_fn)(int result,
    -  *
       * start_failure_fn and task_finished_fn can be NULL to omit any
       * special handling.
    -+ *
    -+ * Options are passed via a "struct run_process_parallel_opts".
       */
     -int run_processes_parallel(int n,
     -			   get_next_task_fn,
    @@ run-command.h: typedef int (*task_finished_fn)(int result,
     -int run_processes_parallel_tr2(int n, get_next_task_fn, start_failure_fn,
     -			       task_finished_fn, void *pp_cb,
     -			       const char *tr2_category, const char *tr2_label);
    -+int run_processes_parallel(int n, get_next_task_fn, start_failure_fn,
    -+			   task_finished_fn, void *pp_cb,
    -+			   struct run_process_parallel_opts *opts);
    ++int run_processes_parallel(struct run_process_parallel_opts *opts);
      
      /**
       * Convenience function which prepares env_array for a command to be run in a
    @@ submodule.c: int fetch_submodules(struct repository *r,
     +	struct run_process_parallel_opts run_opts = {
     +		.tr2_category = "submodule",
     +		.tr2_label = "parallel/fetch",
    ++
    ++		.jobs = max_parallel_jobs,
    ++
    ++		.get_next_task = get_next_submodule,
    ++		.start_failure = fetch_start_failure,
    ++		.task_finished = fetch_finish,
    ++		.data = &spf,
     +	};
      
      	spf.r = r;
    @@ submodule.c: int fetch_submodules(struct repository *r,
     -				   fetch_finish,
     -				   &spf,
     -				   "submodule", "parallel/fetch");
    -+	run_processes_parallel(max_parallel_jobs, get_next_submodule,
    -+			       fetch_start_failure, fetch_finish, &spf,
    -+			       &run_opts);
    ++	run_processes_parallel(&run_opts);
      
      	if (spf.submodules_with_errors.len > 0)
      		fprintf(stderr, _("Errors during submodule fetch:\n%s"),
     
      ## t/helper/test-run-command.c ##
     @@ t/helper/test-run-command.c: static int testsuite(int argc, const char **argv)
    + 			 "write JUnit-style XML files"),
    + 		OPT_END()
    + 	};
    ++	struct run_process_parallel_opts run_opts = {
    ++		.get_next_task = next_test,
    ++		.start_failure = test_failed,
    ++		.task_finished = test_finished,
    ++		.data = &suite,
    ++	};
    + 
    + 	argc = parse_options(argc, argv, NULL, options,
    + 			testsuite_usage, PARSE_OPT_STOP_AT_NON_OPTION);
    +@@ t/helper/test-run-command.c: static int testsuite(int argc, const char **argv)
    + 
    + 	fprintf(stderr, "Running %"PRIuMAX" tests (%d at a time)\n",
      		(uintmax_t)suite.tests.nr, max_jobs);
    ++	run_opts.jobs = max_jobs;
      
    - 	ret = run_processes_parallel(max_jobs, next_test, test_failed,
    +-	ret = run_processes_parallel(max_jobs, next_test, test_failed,
     -				     test_finished, &suite);
    -+				     test_finished, &suite, NULL);
    ++	ret = run_processes_parallel(&run_opts);
      
      	if (suite.failed.nr > 0) {
      		ret = 1;
     @@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
    - {
    - 	struct child_process proc = CHILD_PROCESS_INIT;
      	int jobs;
    + 	get_next_task_fn next_fn = NULL;
    + 	task_finished_fn finished_fn = NULL;
     +	struct run_process_parallel_opts opts = { 0 };
      
      	if (argc > 1 && !strcmp(argv[1], "testsuite"))
      		exit(testsuite(argc - 1, argv + 1));
     @@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
    + 	jobs = atoi(argv[2]);
    + 	strvec_clear(&proc.args);
    + 	strvec_pushv(&proc.args, (const char **)argv + 3);
    ++	opts.jobs = jobs;
    ++	opts.data = &proc;
      
    - 	if (!strcmp(argv[1], "run-command-parallel"))
    - 		exit(run_processes_parallel(jobs, parallel_next,
    --					    NULL, NULL, &proc));
    -+					    NULL, NULL, &proc, &opts));
    - 
    - 	if (!strcmp(argv[1], "run-command-abort"))
    --		exit(run_processes_parallel(jobs, parallel_next,
    --					    NULL, task_finished, &proc));
    -+		exit(run_processes_parallel(jobs, parallel_next, NULL,
    -+					    task_finished, &proc, &opts));
    - 
    - 	if (!strcmp(argv[1], "run-command-no-jobs"))
    --		exit(run_processes_parallel(jobs, no_job,
    --					    NULL, task_finished, &proc));
    -+		exit(run_processes_parallel(jobs, no_job, NULL, task_finished,
    -+					    &proc, &opts));
    + 	if (!strcmp(argv[1], "run-command-parallel")) {
    + 		next_fn = parallel_next;
    +@@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
    + 		return 1;
    + 	}
      
    - 	fprintf(stderr, "check usage\n");
    - 	return 1;
    +-	exit(run_processes_parallel(jobs, next_fn, NULL, finished_fn, &proc));
    ++	opts.get_next_task = next_fn;
    ++	opts.task_finished = finished_fn;
    ++	exit(run_processes_parallel(&opts));
    + }
2:  d9c9b158130 ! 3:  a8e1fc07b65 run-command tests: test stdout of run_command_parallel()
    @@ t/t0061-run-command.sh: World
      
      test_expect_success 'run_command runs in parallel with more jobs available than tasks' '
     -	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    -+	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
    +-	test_cmp expect actual
    ++	test-tool run-command run-command-parallel 5 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
     +	test_must_be_empty out &&
    - 	test_cmp expect actual
    ++	test_cmp expect 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-tool run-command run-command-parallel 4 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
    +-	test_cmp expect actual
    ++	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 actual
    ++	test_cmp expect 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-tool run-command run-command-parallel 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
    +-	test_cmp expect actual
    ++	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 actual
    ++	test_cmp expect err
      '
      
    + cat >expect <<-EOF
     @@ t/t0061-run-command.sh: asking for a quick stop
      EOF
      
      test_expect_success 'run_command is asked to abort gracefully' '
     -	test-tool run-command run-command-abort 3 false 2>actual &&
    -+	test-tool run-command run-command-abort 3 false >out 2>actual &&
    +-	test_cmp expect actual
    ++	test-tool run-command run-command-abort 3 false >out 2>err &&
     +	test_must_be_empty out &&
    - 	test_cmp expect actual
    ++	test_cmp expect err
      '
      
    + cat >expect <<-EOF
     @@ t/t0061-run-command.sh: no further jobs available
      EOF
      
      test_expect_success 'run_command outputs ' '
     -	test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" 2>actual &&
    -+	test-tool run-command run-command-no-jobs 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>actual &&
    +-	test_cmp expect actual
    ++	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 actual
    ++	test_cmp expect err
      '
      
    + test_trace () {
-:  ----------- > 4:  663936fb4ad run-command.c: add an initializer for "struct parallel_processes"
3:  d76f63c2948 ! 5:  c2e015ed840 run-command: add an "ungroup" option to run_process_parallel()
    @@ run-command.c: struct parallel_processes {
      	struct pollfd *pfd;
      
      	unsigned shutdown : 1;
    -+	unsigned ungroup:1;
    ++	unsigned ungroup : 1;
      
      	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, struct run_process_parallel_opts *opts)
    - {
    -+	const int ungroup = opts->ungroup;
    - 	int i;
    - 
    - 	if (n < 1)
    -@@ run-command.c: static void pp_init(struct parallel_processes *pp,
    - 	pp->start_failure = start_failure ? start_failure : default_start_failure;
    - 	pp->task_finished = task_finished ? task_finished : default_task_finished;
    - 
    -+	pp->ungroup = ungroup;
    -+
      	pp->nr_processes = 0;
      	pp->output_owner = 0;
      	pp->shutdown = 0;
    ++	pp->ungroup = opts->ungroup;
      	CALLOC_ARRAY(pp->children, n);
     -	CALLOC_ARRAY(pp->pfd, n);
    -+	if (!ungroup)
    ++	if (!pp->ungroup)
     +		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);
    -+		if (ungroup)
    ++		if (!pp->pfd)
     +			continue;
      		pp->pfd[i].events = POLLIN | POLLHUP;
      		pp->pfd[i].fd = -1;
      	}
    -@@ run-command.c: static void pp_init(struct parallel_processes *pp,
    - 
    - static void pp_cleanup(struct parallel_processes *pp)
    - {
    -+	const int ungroup = pp->ungroup;
    - 	int i;
    - 
    - 	trace_printf("run_processes_parallel: done");
     @@ run-command.c: static void pp_cleanup(struct parallel_processes *pp)
    - 	}
    - 
    - 	free(pp->children);
    --	free(pp->pfd);
    -+	if (!ungroup)
    -+		free(pp->pfd);
    - 
    - 	/*
      	 * When get_next_task added messages to the buffer in its last
      	 * iteration, the buffered output is non empty.
      	 */
     -	strbuf_write(&pp->buffered_output, stderr);
    --	strbuf_release(&pp->buffered_output);
    -+	if (!ungroup) {
    ++	if (!pp->ungroup)
     +		strbuf_write(&pp->buffered_output, stderr);
    -+		strbuf_release(&pp->buffered_output);
    -+	}
    + 	strbuf_release(&pp->buffered_output);
      
      	sigchain_pop_common();
    - }
     @@ run-command.c: static void pp_cleanup(struct parallel_processes *pp)
       */
      static int pp_start_one(struct parallel_processes *pp)
    @@ run-command.c: static int pp_start_one(struct parallel_processes *pp)
      	}
     -	pp->children[i].process.err = -1;
     -	pp->children[i].process.stdout_to_stderr = 1;
    --	pp->children[i].process.no_stdin = 1;
    -+
     +	if (!ungroup) {
     +		pp->children[i].process.err = -1;
     +		pp->children[i].process.stdout_to_stderr = 1;
    -+		pp->children[i].process.no_stdin = 1;
     +	}
    + 	pp->children[i].process.no_stdin = 1;
      
      	if (start_command(&pp->children[i].process)) {
     -		code = pp->start_failure(&pp->children[i].err,
    @@ run-command.c: 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 (!ungroup)
    ++	if (pp->pfd)
     +		pp->pfd[i].fd = pp->children[i].process.err;
      	return 0;
      }
    @@ run-command.c: 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 (!ungroup)
    ++		if (pp->pfd)
     +			pp->pfd[i].fd = -1;
      		child_process_init(&pp->children[i].process);
      
     -		if (i != pp->output_owner) {
     +		if (ungroup) {
    -+			/* no strbuf_*() work to do here */
    ++			; /* 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 {
    -@@ run-command.c: static int run_processes_parallel_1(int n, get_next_task_fn get_next_task,
    - 				    void *pp_cb,
    - 				    struct run_process_parallel_opts *opts)
    - {
    -+	const int ungroup = opts->ungroup;
    - 	int i, code;
    - 	int output_timeout = 100;
    - 	int spawn_cap = 4;
    - 	struct parallel_processes pp;
    - 
    --	pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb);
    -+	pp_init(&pp, n, get_next_task, start_failure, task_finished, pp_cb,
    -+		opts);
    - 	while (1) {
    - 		for (i = 0;
    - 		    i < spawn_cap && !pp.shutdown &&
    -@@ run-command.c: static int run_processes_parallel_1(int n, get_next_task_fn get_next_task,
    +@@ run-command.c: int run_processes_parallel(struct run_process_parallel_opts *opts)
      		}
      		if (!pp.nr_processes)
      			break;
     -		pp_buffer_stderr(&pp, output_timeout);
     -		pp_output(&pp);
    -+		if (ungroup) {
    ++		if (opts->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_task_cb is the callback cookie as passed into get_next_task_fn.
     @@ run-command.h: typedef int (*task_finished_fn)(int result,
       *
    -  * tr2_category & tr2_label: sets the trace2 category and label for
    -  * logging. These must either be unset, or both of them must be set.
    -+ *
    +  * 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
    - {
    - 	const char *tr2_category;
    +@@ 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
    @@ run-command.h: struct run_process_parallel_opts
       *
       * start_failure_fn and task_finished_fn can be NULL to omit any
       * special handling.
    -  *
    -- * Options are passed via a "struct run_process_parallel_opts".
    -+ * Options are passed via a "struct run_process_parallel_opts". If the
    -+ * "ungroup" option isn't specified the callbacks will get a pointer
    -+ * to a "struct strbuf *out", and must not write to stdout or stderr
    -+ * as such output will mess up the output of the other parallel
    ++ *
    ++ * If the "ungroup" option isn't specified the callbacks will get a
    ++ * pointer to a "struct strbuf *out", and 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.
       */
    - int run_processes_parallel(int n, get_next_task_fn, start_failure_fn,
    - 			   task_finished_fn, void *pp_cb,
    + int run_processes_parallel(struct run_process_parallel_opts *opts);
    + 
     
      ## 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)
    - 	strvec_clear(&proc.args);
    - 	strvec_pushv(&proc.args, (const char **)argv + 3);
    + 	opts.jobs = jobs;
    + 	opts.data = &proc;
      
    --	if (!strcmp(argv[1], "run-command-parallel"))
    +-	if (!strcmp(argv[1], "run-command-parallel")) {
     +	if (!strcmp(argv[1], "run-command-parallel") ||
     +	    !strcmp(argv[1], "run-command-parallel-ungroup")) {
    -+		opts.ungroup = !strcmp(argv[1], "run-command-parallel-ungroup");
    - 		exit(run_processes_parallel(jobs, parallel_next,
    - 					    NULL, NULL, &proc, &opts));
    -+	}
    - 
    --	if (!strcmp(argv[1], "run-command-abort"))
    -+	if (!strcmp(argv[1], "run-command-abort") ||
    -+	    !strcmp(argv[1], "run-command-abort-ungroup")) {
    -+		opts.ungroup = !strcmp(argv[1], "run-command-abort-ungroup");
    - 		exit(run_processes_parallel(jobs, parallel_next, NULL,
    - 					    task_finished, &proc, &opts));
    -+	}
    - 
    --	if (!strcmp(argv[1], "run-command-no-jobs"))
    -+	if (!strcmp(argv[1], "run-command-no-jobs") ||
    -+	    !strcmp(argv[1], "run-command-no-jobs-ungroup")) {
    -+		opts.ungroup = !strcmp(argv[1], "run-command-no-jobs-ungroup");
    - 		exit(run_processes_parallel(jobs, no_job, NULL, task_finished,
    - 					    &proc, &opts));
    -+	}
    + 		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;
    + 	}
      
    - 	fprintf(stderr, "check usage\n");
    - 	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));
     
      ## 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 actual
    + 	test_cmp expect err
      '
      
     +test_expect_success 'run_command runs ungrouped in parallel with more jobs available than tasks' '
    @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m
     +'
     +
      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>actual &&
    + 	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 actual
    + 	test_cmp expect err
      '
      
     +test_expect_success 'run_command runs ungrouped in parallel with as many jobs as tasks' '
    @@ t/t0061-run-command.sh: test_expect_success 'run_command runs in parallel with m
     +'
     +
      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>actual &&
    + 	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 actual
    + 	test_cmp expect err
      '
      
     +test_expect_success 'run_command runs ungrouped in parallel with more tasks than jobs available' '
    @@ 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 actual
    + 	test_cmp expect err
      '
      
     +test_expect_success 'run_command is asked to abort gracefully (ungroup)' '
    @@ 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 actual
    + 	test_cmp expect err
      '
      
     +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>actual &&
    ++	test-tool run-command run-command-no-jobs-ungroup 3 sh -c "printf \"%s\n%s\n\" Hello World" >out 2>err &&
     +	test_must_be_empty out &&
    -+	test_cmp expect actual
    ++	test_cmp expect err
     +'
     +
      test_trace () {
4:  cf62569b2e0 = 6:  84e92c6f7c7 hook tests: fix redirection logic error in 96e7225b310
5:  98c26c9917b ! 7:  bf7d871565f hook API: don't redundantly re-set "no_stdin" and "stdout_to_stderr"
    @@ Commit message
         hook API: don't redundantly re-set "no_stdin" and "stdout_to_stderr"
     
         Amend code added in 96e7225b310 (hook: add 'run' subcommand,
    -    2021-12-22) top stop setting these two flags. We use the
    +    2021-12-22) to stop setting these two flags. We use the
         run_process_parallel() API added in c553c72eed6 (run-command: add an
         asynchronous parallel child processor, 2015-12-15), which always sets
         these in pp_start_one() (in addition to setting .err = -1).
6:  de3664f6d2b ! 8:  238155fcb9d 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 "no_stdin=1" and
    -    "stdout_to_stderr=1" assignments. This change brings them back as with
    -    ".ungroup=1" the run_process_parallel() function doesn't provide them
    -    for us implicitly.
    +    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
    @@ Commit message
     
      ## hook.c ##
     @@ hook.c: static int pick_next_hook(struct child_process *cp,
    - 	if (!hook_path)
      		return 0;
      
    -+	cp->no_stdin = 1;
      	strvec_pushv(&cp->env_array, hook_cb->options->env.v);
    -+	cp->stdout_to_stderr = 1;
    ++	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)
    - 		.options = options,
    - 	};
    - 	const char *const hook_path = find_hook(hook_name);
    --	int jobs = 1;
    -+	const int jobs = 1;
    - 	int ret = 0;
    - 	struct run_process_parallel_opts run_opts = {
    - 		.tr2_category = "hook",
      		.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");
      
-- 
2.36.1.952.g0ae626f6cd7