From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Fri, 26 Sep 2025 14:14:39 GMT
Subject: Re: [PATCH 06/10] run-command: allow capturing of collated output
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: 
>> 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.

> 
>> @@ -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
 
>> 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.
 
> 
>> @@ -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.