From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 25 Sep 2025 21:52:27 GMT
Subject: Re: [PATCH 06/10] run-command: allow capturing of collated output
Message-ID: <xmqqplbedwtg.fsf@gitster.g>
In-Reply-To: <20250925125352.1728840-7-adrian.ratiu@collabora.com>

Adrian Ratiu <adrian.ratiu@collabora.com> writes:

> From: Emily Shaffer <emilyshaffer@google.com>
>
> Some callers, for example server-side hooks which wish to relay hook
> output to clients across a transport, want to capture what would
> normally print to stderr and do something else with it. Allow that via a
> callback.
>
> By calling the callback regardless of whether there's output available,
> we allow clients to send e.g. a keepalive if necessary.
>
> Because we expose a strbuf, not a fd or FILE*, there's no need to create
> a temporary pipe or similar - we can just skip the print to stderr and
> instead hand it to the caller.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
>  builtin/fetch.c             |  2 +-
>  builtin/submodule--helper.c |  2 +-
>  hook.c                      |  2 +-
>  run-command.c               | 33 ++++++++++++++++++++++++---------
>  run-command.h               | 22 +++++++++++++++++++++-
>  submodule.c                 |  2 +-
>  t/helper/test-run-command.c | 15 +++++++++++++++
>  t/t0061-run-command.sh      |  7 +++++++
>  8 files changed, 71 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 24645c4653..53bd5552c4 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -2129,7 +2129,7 @@ static int fetch_multiple(struct string_list *list, int max_children,
>  
>  	if (max_children != 1 && list->nr != 1) {
>  		struct parallel_fetch_state state = { argv.v, list, 0, 0, config };
> -		const struct run_process_parallel_opts opts = {
> +		struct run_process_parallel_opts opts = {
>  			.tr2_category = "fetch",
>  			.tr2_label = "parallel/fetch",

This ...

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 07a1935cbe..76cae9f015 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2700,7 +2700,7 @@ static int update_submodules(struct update_data *update_data)
>  {
>  	int i, ret = 0;
>  	struct submodule_update_clone suc = SUBMODULE_UPDATE_CLONE_INIT;
> -	const struct run_process_parallel_opts opts = {
> +	struct run_process_parallel_opts opts = {
>  		.tr2_category = "submodule",
>  		.tr2_label = "parallel/update",

... and this ...

>  
> diff --git a/hook.c b/hook.c
> index 54568d5bc0..199c210b97 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -135,7 +135,7 @@ int run_hooks_opt(struct repository *r, const char *hook_name,
>  	};
>  	const char *const hook_path = find_hook(r, hook_name);
>  	int ret = 0;
> -	const struct run_process_parallel_opts opts = {
> +	struct run_process_parallel_opts opts = {
>  		.tr2_category = "hook",
>  		.tr2_label = hook_name,

... and this are curious changes that are not explained in the
proposed log message.

> @@ -1841,6 +1852,10 @@ void run_processes_parallel(const struct run_process_parallel_opts *opts)
>  					   "max:%"PRIuMAX,
>  					   (uintmax_t)opts->processes);
>  
> +	/* ungroup and reading sideband are mutualy exclusive, so disable ungroup */
> +	If (opts->ungroup && opts->consume_sideband)
> +		opts->ungroup = 0;

Make it a BUG(""), which may help avoid unintended bugs, especially ...

> diff --git a/run-command.h b/run-command.h
> index 4679987c8e..ad0bab14b0 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -436,6 +436,20 @@ typedef int (*feed_pipe_fn)(int child_in,
>  				void *pp_cb,
>  				void *pp_task_cb);
>  
> +/**
> + * If this callback is provided, instead of collating process output to stderr,
> + * they will be collated into a new pipe. consume_sideband_fn will be called
> + * repeatedly. When output is available on that pipe, it will be contained in
> + * 'output'. But it will be called with an empty 'output' too, to allow for
> + * keepalives or similar operations if necessary.
> + *
> + * pp_cb is the callback cookie as passed into run_processes_parallel.
> + *
> + * Since this callback is provided with the collated output, no task cookie is
> + * provided.
> + */
> +typedef void (*consume_sideband_fn)(struct strbuf *output, void *pp_cb);

> +
>  /**
>   * This callback is called on every child process that finished processing.
>   *
> @@ -495,6 +509,12 @@ struct run_process_parallel_opts
>  	 */
>  	feed_pipe_fn feed_pipe;
>  
> +	/*
> +	 * consume_sideband: see consume_sideband_fn() above. This can be NULL
> +	 * to omit any special handling.
> +	 */
> +	consume_sideband_fn consume_sideband;

... because which one between this and ungroup gets precedence.
Document that they are mutually exclusive, and help the callers
with a BUG("") message when both are set.

> @@ -529,7 +549,7 @@ struct run_process_parallel_opts
>   * emitting their own output, including dealing with any race
>   * conditions due to writing in parallel to stdout and stderr.
>   */
> -void run_processes_parallel(const struct run_process_parallel_opts *opts);
> +void run_processes_parallel(struct run_process_parallel_opts *opts);

This is the same unexplained curiousity I touched earlier.

Thanks.