On Fri, 26 Sep 2025, Junio C Hamano [off-list ref] wrote:

Phillip Wood [off-list ref] writes: 

quoted

quoted

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

quoted

quoted

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

quoted

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