From: Adrian Ratiu Date: Mon, 29 Sep 2025 07:33:23 GMT Subject: Re: [PATCH 03/10] hook: convert 'post-rewrite' hook in sequencer.c to hook.h Message-ID: <873485d870.fsf@gentoo.mail-host-address-is-not-set> In-Reply-To: On Fri, 26 Sep 2025, Junio C Hamano wrote: > Phillip Wood 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.