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

From: Jeff King <peff@peff.net>
Date: Jan 14, 2026, 03:12 UTC
Message-ID: <20260114031257.GA858646@coredump.intra.peff.net>
In-Reply-To: <20260113234528.1749921-1-adrian.ratiu@collabora.com>

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

Show 6 quoted lines

> @@ -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, \

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

Show 12 quoted lines

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

-Peff