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, Junio C Hamano <gitster@pobox.com> wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes: 
> 
>>> +	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. 
> 
> Ouch. 

I replied separately to this, because I do not think it is 
correct. :)

Please see my other reply and if there are further questions or 
concerns we can continue the discussion there, happy to address 
them.
 
>>> +	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. 
>> 
>>> +		return ret; +	} + +	/* Reset the input buffer 
>>> to avoid sending it again */ +	strbuf_reset(to_pipe); 
>> 
>> Shouldn't the return value do that? 
> 
> Sorry, I do not understand this comment, but did you mean 
> to_pipe strbuf is left with some buffered bytes when we take the 
> early-return path when we got an error above?

I described in the other reply how this function works. I messed 
up by making it more complicated than necessary. I'll simplify it 
in v2.

Basically I assumed this write works recursively and the reset 
stops the loop, so the next call exits early to stops loop.

Since we write just 1 strbuf once, there is no need for 
recursiveness and we can just return based on the return value, as 
Phillip suggested.
 
> 
> This part of the new code makes me wonder what the lifetime 
> rules for the to_pipe message are? 
> 
> In the original code before this rewrite, it was clear that the 
> caller of this function was responsible to allocate the strbuf, 
> to feed it to the subprocess, and to release the resources it 
> held.  Now, what is the rule?  The caller still prepares the 
> strbuf, but the called machinery using the hook API will release 
> the resources? 

It's the same as before: the caller owns the strbuf and releases 
it (see the strbuf_release(&sb); in this patch's context, I did 
not modify that).

I'll document this in addition to simplifying the function logic in v2.