From: Adrian Ratiu Date: Wed, 14 Jan 2026 08:46:39 GMT Subject: Re: [PATCH v2] hook: allow hooks to disable stdout_to_stderr Message-ID: <878qe0zimo.fsf@gentoo.mail-host-address-is-not-set> In-Reply-To: <20260114031257.GA858646@coredump.intra.peff.net> On Tue, 13 Jan 2026, Jeff King wrote: > On Wed, Jan 14, 2026 at 01:45:28AM +0200, Adrian Ratiu wrote: > >> Changes in v2: >> * Extended hook test coverage to detect future regressions (Junio, Patrick) >> * Reworded commit message and added explanatory comment (Junio, Patrick) >> * Set ungroup = 1 because grouping overrides stdout_to_stderr (Adrian) > > I have not really been following this topic, but I did read (and > reproduce) Kristoffer's earlier report about reading stdin. The fix here > was not quite what I expected. > > In particular... > >> @@ -93,6 +98,7 @@ struct run_hooks_opt >> #define RUN_HOOKS_OPT_INIT { \ >> .env = STRVEC_INIT, \ >> .args = STRVEC_INIT, \ >> + .stdout_to_stderr = 1, \ >> } > > ...I expected to see: > > .ungroup = 1, \ Good catch. I actually missed this in v2. I will drop ungroup from this patch in v3 and add another patch fixing Kristoffer's issue (rationale below). > > here. The stdin issue goes back to 857f047e40 (hook: allow overriding > the ungroup option, 2025-12-26), where the "ungroup" field was added, > and various code paths set it to "1" to match the previous behavior. But > any paths that were missed, including run_pre_push_hook(), would see a > change of behavior (and in this case, a bug). > > My reading of 857f047e40 is that it meant to give callers the _option_ > to switch the ungroup behavior, but not actually change anything. So > wouldn't we want to leave the default as it was by initializing it to > "1"? That is correct: my mistake in v2 was assuming Kristoffer and Chris reported the same bug, when in fact there are 2 separate bugs requiring separate fixes, so I will create 2 separate commits in v3 for each. > >> @@ -1373,6 +1373,15 @@ static int run_pre_push_hook(struct transport *transport, >> opt.feed_pipe = pre_push_hook_feed_stdin; >> opt.feed_pipe_cb_data = &data; >> >> + /* >> + * pre-push hooks expect stdout & stderr to be separate, so don't merge >> + * them to keep backwards compatibility with existing hooks. >> + * run_process_parallel(), called via run_hooks_opt() below, will buffer >> + * and merge the streams when output is grouped, so also set ungroup = 1. >> + */ >> + opt.stdout_to_stderr = 0; >> + opt.ungroup = 1; > > The other unexpected thing is that these two fixes are grouped at all. > AFAICT, setting ungroup to 1 will fix Kristoffer's stdin problem without > changing stdout_to_stderr at all. > > But I'm still not entirely sure I understand why the ungroup setting, > which supposedly only affects stderr handling, causes the hook to fail > to read stdin. Poking at it in a debugger and via strace, it looks like > we are in a poll loop while feeding stdin, even though we are not > checking whether the child can read! If we instrument like this: > > diff --git a/transport.c b/transport.c > index 6d0f02be5d..7381450123 100644 > --- a/transport.c > +++ b/transport.c > @@ -1342,6 +1342,7 @@ static int pre_push_hook_feed_stdin(int hook_stdin_fd, void *pp_cb UNUSED, void > break; > } > > + warning("called pre_push_hook_feed_stdin for %s", r->name); > if (!r->peer_ref) > return 0; > > > and then run the push from Kristoffer's recipe under strace, I see: > > poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout) > write(2, "warning: called pre_push_hook_feed_stdin for refs/tags/gitgui-0.6.3\n", 68) = 68 > poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout) > write(2, "warning: called pre_push_hook_feed_stdin for refs/tags/gitgui-0.6.4\n", 68) = 68 > poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout) > write(2, "warning: called pre_push_hook_feed_stdin for refs/tags/gitgui-0.6.5\n", 68) = 68 > poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout) > write(2, "warning: called pre_push_hook_feed_stdin for refs/tags/gitgui-0.7.0\n", 68) = 68 > poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout) > write(2, "warning: called pre_push_hook_feed_stdin for refs/tags/gitgui-0.7.0-rc1\n", 72) = 72 > poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout) > > So we are hitting the poll timeout for each ref we consider, and it > takes forever to actually write the whole input stream. Which seems like > a bug in using feed_pipe without ungroup. Either: > > 1. We should write everything to the child as quickly as possible, > assuming that we do not have to worry about reading back from it to > avoid deadlock. > > 2. We should add the child's input pipes to our poll() call so that we > can tell it is ready for more input (without hitting the timeout). > > Setting ungroup=1 saves us from this because it means that we'll skip > the poll() call entirely in pp_handle_child_IO(). So we end up > effectively doing (1), which is OK because ungroup means we are not > reading stdout or stderr from the child at all. > > But it feels like this is papering over a bug, or at least providing a > dangerous interface. AFAICT you _must_ set ungroup if you are going to > use the feed_pipe callback. And it does not really have anything to do > with the stdout_to_stderr flag at all. > > It looks like feed_pipe feature is new-ish in your series. Maybe it > should just be a BUG() to use it without ungroup? This is all very useful and it proves there are 2 separate bugs here, requiring two separate fixes for both Chris and Kristoffer. The logic in v1 (without ungroup) is enough to fix Chris' issue with stdin and for Kristoffer I will do a smarter fix which implements your (1) suggestion: batch more than a single stdin fd write in each poll call so we achieve comparable throughtput (no added poll latency). We already do this for the receive hook in feed_receive_hook_cb(). In this case we just need the callback to process more than just 1 ref at a time. I will send v3 addressing your feedback, it is very much appreciated, Adrian