Re: [PATCH 03/10] hook: convert 'post-rewrite' hook in sequencer.c to hook.h

[Adrian Ratiu <adrian.ratiu@collabora.com>]

On Fri, 26 Sep 2025, Phillip Wood <phillip.wood123@gmail.com> 
wrote:
> Hi Adrian 
> 
> Thanks for working on this, it would be really good to be able 
> to run  hooks in parallel. 

Hi Phillip and thank you for your feedback! It is very valuable 
and appreciated on all my patch series.

> 
> On 25/09/2025 13:53, Adrian Ratiu wrote: 
>> From: Emily Shaffer <emilyshaffer@google.com>  By using 
>> 'hook.h' for 'post-rewrite', we simplify hook invocations by 
>> not needing to put together our own 'struct child_process'. 
> 
> Right so instead we use the new api to feed an strbuf into the 
> hook's  stdin, sounds reasonable. 

That is the high level idea yes, maybe I can improve the commit 
msg a bit in v2 to make it clearer (those are not actually my 
words :).

Slightly unrelated:

I actually thought about putting pipe_from_strbuf() into hook.c or 
someplace similar because it's a generic utility function, however 
this is the only hook which needs it, so I've left it in 
sequencer.c.
 
>> The signal handling that's being removed by this commit now 
>> takes place in run-command.h:run_processes_parallel(), so it is 
>> OK to remove them here.   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> --- 
>>   sequencer.c | 62 
>>   ++++++++++++++++++++++++++++++++--------------------- 1 file 
>>   changed, 38 insertions(+), 24 deletions(-) 
>>  diff --git a/sequencer.c b/sequencer.c index 
>> 9ae40a91b2..93cd6ab1f2 100644 --- a/sequencer.c +++ 
>> b/sequencer.c @@ -1298,32 +1298,46 @@ int 
>> update_head_with_reflog(const struct commit *old_head, 
>>   	return ret; }  
>> +static int pipe_from_strbuf(int hook_stdin_fd, void *pp_cb, 
>> void *pp_task_cb UNUSED) +{ +	struct hook_cb_data 
>> *hook_cb = pp_cb; +	struct strbuf *to_pipe = 
>> hook_cb->options->feed_pipe_ctx; +	int ret; + +	if 
>> (!to_pipe || !to_pipe->len) +		return 1; /* 
>> nothing to feed */ 
> 
> Why are we running the hook if there is nothing to pass to it? 

run-commands has a ppoll loop which calls the stdin feed callback 
(pipe_from_stdbuf in this case) repeatedly until it signals 
reading is finished by return 1;

Now that I look again at this code, I made the mistake of assuming 
it needs to work recursively:
    1. write the strbuf
    2. reset the strbuf (return 0)
    3. next callback sees the strbuf is empty and stops the loop 
    (return 1)

The line you're asking asking here is actually step 3. :)

This all can be simplified by writing just once and returning 1 
immediately since it's just a simple strbuf to write to stdin.

We still need to keep the pointer null check though, just in case.
 
>> +	ret = write_in_full(hook_stdin_fd, to_pipe->buf, 
>> to_pipe->len); 
> 
> This will block until the hook has read all of the input. Unless 
> the  hook drains and closes stdin before it does anything else 
> it will block  the parallel execution of other hooks. 

I double checked the write_in_full / xwrite implementations. :)

Sorry for the wall of text btw, I try to explain as best I can.

I think it blocks only if/when the stdin fd pipe is full, which 
can't happen in this specific instance because the strbuf data is 
small, so the write_in_full() call just writes all it has, then 
returns.

In other words, the most important aspect I think is how much data 
we are writing to the pipe in every single callback call.

The idea of these callbacks is to write small chunks of data at a 
time, then switch context: it's usually the child hook processes 
which end up blocking for their stdin input which is fed by the 
parent which multiplexes between the parallel processes.

Of course, a balance needs to be found: I've noticed, in other 
hooks, that if we write too small chunks of data in each callback, 
we unnecessarily increase wait times for hooks.

This can be seen especially in hooks like post-receive which can 
get a lot of data: if we feed one line at a time and let's say 
run-command's ppoll loop adds 100ms delay between callbacks, then 
from 7-800 ms we end up with something like 90 seconds!

Of course that is a pathological case and the solution there is to 
batch more data in a single callback write. I've actually batched 
500 lines in every write for those update hooks (we can do more or 
less).

None of this is set in stone and can be changed. What I tried is 
to get similar performance with what we had before these 
callbacks.

If you notice any specific degradation or unnecessary blocking, 
please raise it up and we can address it.
 
>> +	if (ret < 0) { +		if (errno == EPIPE) { + 
>> return 1; /* child closed pipe, nothing more to feed */ + 
>> } 
> 
> Style: we don't use braces for single statement bodies. 

Ack, will fix in v2.
 
>> +		return ret; +	} + +	/* Reset the input buffer 
>> to avoid sending it again */ +	strbuf_reset(to_pipe); 
> 
> Shouldn't the return value do that?

Yes, as explained above with my 1 2 3 recursive steps above we 
don't actually need this and the function can be simplified.

I'll do that in v2.
 
>> +	return ret; +} 
> 
> The changes to run_rewrite_hook() look fine. I'm not sure whats 
> happening in commit_post_rewrite() below though - am I missing 
> something  or have you just renamed "child" -> "notes_cp". If so 
> I don't see what  that has to do with using the new api.

Thanks for spotting this!

It's actually a leftover from Emily and Aevar's string_list API 
implementation which I've rewritten / removed and this hunk fell 
through the cracks. :)

Will drop it in v2.

>
> Thanks
>
> Phillip
>
>>   void commit_post_rewrite(struct repository *r,
>> @@ -5140,16 +5154,16 @@ static int pick_commits(struct repository *r,
>>   		flush_rewritten_pending();
>>   		if (!stat(rebase_path_rewritten_list(), &st) &&
>>   				st.st_size > 0) {
>> -			struct child_process child = CHILD_PROCESS_INIT;
>> +			struct child_process notes_cp = CHILD_PROCESS_INIT;
>>   			struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT;
>>   
>> -			child.in = open(rebase_path_rewritten_list(), O_RDONLY);
>> -			child.git_cmd = 1;
>> -			strvec_push(&child.args, "notes");
>> -			strvec_push(&child.args, "copy");
>> -			strvec_push(&child.args, "--for-rewrite=rebase");
>> +			notes_cp.in = open(rebase_path_rewritten_list(), O_RDONLY);
>> +			notes_cp.git_cmd = 1;
>> +			strvec_push(&notes_cp.args, "notes");
>> +			strvec_push(&notes_cp.args, "copy");
>> +			strvec_push(&notes_cp.args, "--for-rewrite=rebase");
>>   			/* we don't care if this copying failed */
>> -			run_command(&child);
>> +			run_command(&notes_cp);
>>   
>>   			hook_opt.path_to_stdin = rebase_path_rewritten_list();
>>   			strvec_push(&hook_opt.args, "rebase");