Re: [PATCH 06/10] run-command: allow capturing of collated output
- From
Adrian Ratiu <adrian.ratiu@collabora.com>
- Date
- Sep 26, 2025, 14:14 UTC
- Message-ID
- <873489nvw0.fsf@collabora.com>
- In-Reply-To
- <xmqqplbedwtg.fsf@gitster.g>
On Thu, 25 Sep 2025, Junio C Hamano <gitster@pobox.com> wrote:
Show 39 quoted lines
>> 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. Yes and sorry for not explaining these better. The only reason I had to remove the const is to be able to set opts->ungroup = 0 below.
If I can find a way to do what you propose, then 100% I will drop all these hunks. I agree that is the best way forward in v2.
Show 11 quoted lines
>
>> @@ -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 ... I did exactly this and got test failures because some tests end up setting both ungroup and consume_sideband. Since the original code I got from Emily and Aevar just defaulted to setting ungroup = 0 and removing the const I went with that instead of actually fixing the tests, assuming the tests actually need to do that. :)
Now I know better and for v2 I will fix the tests and add a BUG() here, with proper reasoning for the test modification.
An example of test which fails because it sets both is: t1416-ref-transaction-hooks.sh not ok 7 - interleaving hook calls succeed
Show 28 quoted lines
>> 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.Agreed, will do in v2.
Show 10 quoted lines
> >> @@ -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.
Yes, I promise I'll drop all these in v2.