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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 26, 2025, 17:52 UTC
Message-ID: <xmqqikh5ayoy.fsf@gitster.g>
In-Reply-To: <f408e46c-650a-4632-9628-cf817e393e7f@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

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

Ouch.

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?

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?