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

From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Sep 26, 2025, 15:53 UTC
Message-ID: <87zfahmcqi.fsf@collabora.com>
In-Reply-To: <f408e46c-650a-4632-9628-cf817e393e7f@gmail.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.

Show 8 quoted lines

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

Show 22 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 6 quoted lines

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

Show 30 quoted lines

>
> 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");