Re: [PATCH 06/10] run-command: allow capturing of collated output
From: Adrian Ratiu <hidden>
Date: 2025-09-26 14:14:56
On Thu, 25 Sep 2025, Junio C Hamano [off-list ref] wrote:
quoted
diff --git a/builtin/fetch.c b/builtin/fetch.c index24645c4653..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 ...quoted
diff --git a/builtin/submodule--helper.cb/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 ...quoted
diff --git a/hook.c b/hook.c index 54568d5bc0..199c210b97100644 --- 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.
quoted
@@ -1841,6 +1852,10 @@ void run_processes_parallel(const structrun_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
quoted
diff --git a/run-command.h b/run-command.h index4679987c8e..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.
quoted
@@ -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 structrun_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.