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

From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Sep 29, 2025, 07:33 UTC
Message-ID: <873485d870.fsf@gentoo.mail-host-address-is-not-set>
In-Reply-To: <xmqqikh5ayoy.fsf@gitster.g>

On Fri, 26 Sep 2025, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

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

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

Show 10 quoted lines

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