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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Adrian

On 26/09/2025 16:53, Adrian Ratiu wrote:
> On Fri, 26 Sep 2025, Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 25/09/2025 13:53, Adrian Ratiu wrote:
> 
> 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.

Yes, I was surprised that was the only place we ended up needing that 
function. I think it makes sense to leave it where it is if that's the 
only place that needs it.

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

We should make that a BUG() I think

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

Oh sorry, I'd forgotten this was just used on a short buffer, and not 
used when we're rebasing. Good point, it should be fine.

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

That's great, getting a good balance between not blocking for too long 
and doing a reasonable amount of work in each call to the callback 
sounds like a good plan.

Thanks

Phillip