Thread (2 messages) 2 messages, 2 authors, 2025-09-26

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 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 ... 
quoted
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 ... 
quoted
  
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.
quoted
@@ -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
 
quoted
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.
 
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 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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help