Re: [PATCH v2] hook: allow hooks to disable stdout_to_stderr

[Adrian Ratiu <adrian.ratiu@collabora.com>]

On Tue, 13 Jan 2026, Jeff King <peff@peff.net> 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