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

[Junio C Hamano <gitster@pobox.com>]

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.

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