patchhook: make stdout_to_stderr optional

> The last batch of hooks converted to the hook.[ch] API introduced
> a regression because pick_next_hook() always sets stdout_to_stderr
> for its child processes.
> 
> Pre-push is the only hook API user which requires stdout_to_stderr
> to be 0, so it can be argued that pre-push needs fixing, however
> this will likely break many pre-push hooks, so it's better to allow
> it to be 0, i.e. to match the previous behavior.

> diff --git a/transport.c b/transport.c
> index 6d0f02be5d..8f0e5987ab 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1372,6 +1372,7 @@ static int run_pre_push_hook(struct transport *transport,
>  
>  	opt.feed_pipe = pre_push_hook_feed_stdin;
>  	opt.feed_pipe_cb_data = &data;
> +	opt.stdout_to_stderr = 0;
>  
>  	ret = run_hooks_opt(the_repository, "pre-push", &opt);

> On Tue, Jan 13, 2026 at 01:56:33PM +0200, Adrian Ratiu wrote:
>> The last batch of hooks converted to the hook.[ch] API introduced
>> a regression because pick_next_hook() always sets stdout_to_stderr
>> for its child processes.
>> 
>> Pre-push is the only hook API user which requires stdout_to_stderr
>> to be 0, so it can be argued that pre-push needs fixing, however
>> this will likely break many pre-push hooks, so it's better to allow
>> it to be 0, i.e. to match the previous behavior.
>
> Okay. Do you happen to know whether we've got test coverage for those
> other hooks? Would be great to verify whether changing
> `stodut_to_stderr` to default-disabled causes at least one test to fail
> for every hook we've got.

>> We can introduce an extension for the breaking change of all hooks
>> sending stdout to stderr, however this just fixes the regression.
>
> Is it really necessary to change this though? I wouldn't really want to
> go there without a good reason.

>> diff --git a/transport.c b/transport.c
>> index 6d0f02be5d..8f0e5987ab 100644
>> --- a/transport.c
>> +++ b/transport.c
>> @@ -1372,6 +1372,7 @@ static int run_pre_push_hook(struct transport *transport,
>>  
>>  	opt.feed_pipe = pre_push_hook_feed_stdin;
>>  	opt.feed_pipe_cb_data = &data;
>> +	opt.stdout_to_stderr = 0;
>>  
>>  	ret = run_hooks_opt(the_repository, "pre-push", &opt);
>
> The fact that this was able to sneak in without anybody noticing shows
> that we have a test gap. Can we maybe have a test that verifies that the
> hook output goes to the correct standard stream?

> diff --git a/hook.c b/hook.c
> index 35211e5ed7..ebd9d9e26e 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -81,7 +81,7 @@ static int pick_next_hook(struct child_process *cp,
>  		cp->in = -1;
>  	}
>  
> -	cp->stdout_to_stderr = 1;
> +	cp->stdout_to_stderr = hook_cb->options->stdout_to_stderr;
>  	cp->trace2_hook_name = hook_cb->hook_name;
>  	cp->dir = hook_cb->options->dir;

> diff --git a/hook.h b/hook.h
> index ae502178b9..2488db7133 100644
> --- a/hook.h
> +++ b/hook.h
> @@ -39,6 +39,11 @@ struct run_hooks_opt
>  	 */
>  	unsigned int ungroup:1;
>  
> +	/**
> +	 * Send the hook's stdout to stderr.
> +	 */
> +	unsigned int stdout_to_stderr:1;
> +
>  	/**
>  	 * Path to file which should be piped to stdin for each hook.
>  	 */
> @@ -93,6 +98,7 @@ struct run_hooks_opt
>  #define RUN_HOOKS_OPT_INIT { \
>  	.env = STRVEC_INIT, \
>  	.args = STRVEC_INIT, \
> +	.stdout_to_stderr = 1, \
>  }

>  struct hook_cb_data {
> diff --git a/transport.c b/transport.c
> index 6d0f02be5d..8f0e5987ab 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -1372,6 +1372,7 @@ static int run_pre_push_hook(struct transport *transport,
>  
>  	opt.feed_pipe = pre_push_hook_feed_stdin;
>  	opt.feed_pipe_cb_data = &data;
> +	opt.stdout_to_stderr = 0;

> The last batch of hooks converted to the hook.[ch] API introduced
> a regression because pick_next_hook() always sets stdout_to_stderr
> for its child processes.
>
> Pre-push is the only hook API user which requires stdout_to_stderr
> to be 0, so it can be argued that pre-push needs fixing, however
> this will likely break many pre-push hooks, so it's better to allow
> it to be 0, i.e. to match the previous behavior.

> What was the previous behaviour of code paths that ran other hooks?
> Was pre-push the only one that didn't divert standard output to
> standard error?  This patch does look like a proper regression fix
> in that case.  I browsed "git log -p 1627809eef..c65f26fca4" (i.e.,
> the change for "Merge branch 'ar/run-command-hook'") and random
> sampling (like run_receive_hook() that used run_and_feed_hook(),
> which set stdout_to_stderr to 1) seems to indicate that it is the
> case.

> Junio C Hamano <gitster@pobox.com> writes:
>
>> What was the previous behaviour of code paths that ran other hooks?
>> Was pre-push the only one that didn't divert standard output to
>> standard error?  This patch does look like a proper regression fix
>> in that case.  I browsed "git log -p 1627809eef..c65f26fca4" (i.e.,
>> the change for "Merge branch 'ar/run-command-hook'") and random
>> sampling (like run_receive_hook() that used run_and_feed_hook(),
>> which set stdout_to_stderr to 1) seems to indicate that it is the
>> case.
>
> By the way, if stdout_to_stderr is by default set to true, but tnis
> regression fix allows specific callers to opt out of it, then the
> title "make stdout_to_stderr optional" is a bit misleaing.  It makes
> it sound as if it is false by default and optionally turned on.
>
> Perhaps like "hook: allow stdout_to_stderr optionally off" or
> something?

> On Tue, 13 Jan 2026, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> What was the previous behaviour of code paths that ran other hooks?
>>> Was pre-push the only one that didn't divert standard output to
>>> standard error?  This patch does look like a proper regression fix
>>> in that case.  I browsed "git log -p 1627809eef..c65f26fca4" (i.e.,
>>> the change for "Merge branch 'ar/run-command-hook'") and random
>>> sampling (like run_receive_hook() that used run_and_feed_hook(),
>>> which set stdout_to_stderr to 1) seems to indicate that it is the
>>> case.
>>
>> By the way, if stdout_to_stderr is by default set to true, but tnis
>> regression fix allows specific callers to opt out of it, then the
>> title "make stdout_to_stderr optional" is a bit misleaing.  It makes
>> it sound as if it is false by default and optionally turned on.
>>
>> Perhaps like "hook: allow stdout_to_stderr optionally off" or
>> something?
>
> Ack. Will rename in v2.
>
> Please wait for v2 because, while writing the tests, I noticed pre-push
> needs 1 additional line (ungroup output) to function as before.

> Adrian Ratiu <adrian.ratiu@collabora.com> writes:
>
>> On Tue, 13 Jan 2026, Junio C Hamano <gitster@pobox.com> wrote:
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>> What was the previous behaviour of code paths that ran other hooks?
>>>> Was pre-push the only one that didn't divert standard output to
>>>> standard error?  This patch does look like a proper regression fix
>>>> in that case.  I browsed "git log -p 1627809eef..c65f26fca4" (i.e.,
>>>> the change for "Merge branch 'ar/run-command-hook'") and random
>>>> sampling (like run_receive_hook() that used run_and_feed_hook(),
>>>> which set stdout_to_stderr to 1) seems to indicate that it is the
>>>> case.
>>>
>>> By the way, if stdout_to_stderr is by default set to true, but tnis
>>> regression fix allows specific callers to opt out of it, then the
>>> title "make stdout_to_stderr optional" is a bit misleaing.  It makes
>>> it sound as if it is false by default and optionally turned on.
>>>
>>> Perhaps like "hook: allow stdout_to_stderr optionally off" or
>>> something?
>>
>> Ack. Will rename in v2.
>>
>> Please wait for v2 because, while writing the tests, I noticed pre-push
>> needs 1 additional line (ungroup output) to function as before.
>
> Understood.  Thanks.
>
> Writing these tests would take particular care, I imagine.  Apply
> the test to the tip of the 'master' before ar/run-commmand-hook was
> merged, to verify that the tests expect the behaviour before these
> series, and then merge the result up in more recent 'master' to see
> that the changes in ar/run-commmand-hook did not negatively change
> the behaviour, or something like that?

> Adrian Ratiu <adrian.ratiu@collabora.com> writes:
>
>> diff --git a/hook.c b/hook.c
>> index 35211e5ed7..ebd9d9e26e 100644
>> --- a/hook.c
>> +++ b/hook.c
>> @@ -81,7 +81,7 @@ static int pick_next_hook(struct child_process *cp,
>>  		cp->in = -1;
>>  	}
>>  
>> -	cp->stdout_to_stderr = 1;
>> +	cp->stdout_to_stderr = hook_cb->options->stdout_to_stderr;
>>  	cp->trace2_hook_name = hook_cb->hook_name;
>>  	cp->dir = hook_cb->options->dir;
>
> So stdout_to_stderr used to get always forced to 1, but now the
> value comes from the hook option, which ...
>
>> diff --git a/hook.h b/hook.h
>> index ae502178b9..2488db7133 100644
>> --- a/hook.h
>> +++ b/hook.h
>> @@ -39,6 +39,11 @@ struct run_hooks_opt
>>  	 */
>>  	unsigned int ungroup:1;
>>  
>> +	/**
>> +	 * Send the hook's stdout to stderr.
>> +	 */
>> +	unsigned int stdout_to_stderr:1;
>> +
>>  	/**
>>  	 * Path to file which should be piped to stdin for each hook.
>>  	 */
>> @@ -93,6 +98,7 @@ struct run_hooks_opt
>>  #define RUN_HOOKS_OPT_INIT { \
>>  	.env = STRVEC_INIT, \
>>  	.args = STRVEC_INIT, \
>> +	.stdout_to_stderr = 1, \
>>  }
>
> ... defaults to 1 for everybody, unless a specific caller opts out
> by ...
>
>>  struct hook_cb_data {
>> diff --git a/transport.c b/transport.c
>> index 6d0f02be5d..8f0e5987ab 100644
>> --- a/transport.c
>> +++ b/transport.c
>> @@ -1372,6 +1372,7 @@ static int run_pre_push_hook(struct transport *transport,
>>  
>>  	opt.feed_pipe = pre_push_hook_feed_stdin;
>>  	opt.feed_pipe_cb_data = &data;
>> +	opt.stdout_to_stderr = 0;
>
> ... setting it to 0 in their hook option.
>
>> The last batch of hooks converted to the hook.[ch] API introduced
>> a regression because pick_next_hook() always sets stdout_to_stderr
>> for its child processes.
>>
>> Pre-push is the only hook API user which requires stdout_to_stderr
>> to be 0, so it can be argued that pre-push needs fixing, however
>> this will likely break many pre-push hooks, so it's better to allow
>> it to be 0, i.e. to match the previous behavior.
>
> What was the previous behaviour of code paths that ran other hooks?
> Was pre-push the only one that didn't divert standard output to
> standard error?  This patch does look like a proper regression fix
> in that case.  I browsed "git log -p 1627809eef..c65f26fca4" (i.e.,
> the change for "Merge branch 'ar/run-command-hook'") and random
> sampling (like run_receive_hook() that used run_and_feed_hook(),
> which set stdout_to_stderr to 1) seems to indicate that it is the
> case.

> @@ -93,6 +98,7 @@ struct run_hooks_opt
>  #define RUN_HOOKS_OPT_INIT { \
>  	.env = STRVEC_INIT, \
>  	.args = STRVEC_INIT, \
> +	.stdout_to_stderr = 1, \
>  }

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

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

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

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

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

> > 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 looked at what feed_receive_hook_cb() is doing and...it's kind of
> horrifying. It arbitrarily sends 500 lines, and then yields to the
> caller to pump stderr (assuming ungroup=0). So:
> 
>   1. It is assuming that 500 lines of input won't fill up the pipe
>      buffer and block. Even if we compute the size of 500 lines we're
>      sending, we don't know if the caller has cleared anything from the
>      pipe in the last call. There might be zero bytes available!
> 
>   2. After 500 lines we'll go back to the caller, which will then
>      poll(). But if there's nothing to read on stderr, it will wait for
>      the 100ms timeout. So if you have, say, 501 lines to send, then
>      there will be a pointless 100ms pause in the middle.
> 
> So here's an example hook setup that will deadlock due to (1):

> On Wed, Jan 14, 2026 at 12:08:49PM -0500, Jeff King wrote:
>
>> I looked at what feed_receive_hook_cb() is doing and...it's kind of
>> horrifying. It arbitrarily sends 500 lines, and then yields to the
>> caller to pump stderr (assuming ungroup=0). So:
>> 
>>   1. It is assuming that 500 lines of input won't fill up the pipe
>>      buffer and block. Even if we compute the size of 500 lines we're
>>      sending, we don't know if the caller has cleared anything from the
>>      pipe in the last call. There might be zero bytes available!
>> 
>>   2. After 500 lines we'll go back to the caller, which will then
>>      poll(). But if there's nothing to read on stderr, it will wait for
>>      the 100ms timeout. So if you have, say, 501 lines to send, then
>>      there will be a pointless 100ms pause in the middle.
>> 
>> So here's an example hook setup that will deadlock due to (1):
>
> And just for fun, here's an example that shows problem (2):
>
> -- >8 --
> rm -rf repo
> git init repo
> cd repo
> git commit --allow-empty -m foo
> git init --bare dst.git
>
> cat >dst.git/hooks/pre-receive <<\EOF
> #!/bin/sh
> # We don't even need to do anything interesting here! Git
> # will send us 500 lines, then block waiting for stderr which
> # we'll never send, and then send us another batch of 500.
> cat >/dev/null
> EOF
> chmod +x dst.git/hooks/pre-receive
>
> # Now do a moderate push of 500 branches.
> seq --format='create refs/heads/small-%g HEAD' 500 |
> git update-ref --stdin
> time git push -q dst.git refs/heads/small-*
>
> # And compare with one that sends just one more.
> seq --format='create refs/heads/large-%g HEAD' 501 |
> git update-ref --stdin
> time git push -q dst.git refs/heads/large-*
> -- >8 --
>
> The second push always takes 100ms more! If we run the server side under
> strace by replacing the final line with this:
>
>   git push -q --receive-pack='strace -T git-receive-pack' dst.git refs/heads/large-*
>
> we can see the stall here as we write to the hook:
>
>   write(4, "00000000000000000000000000000000"..., 51393) = 51393 <0.000011>
>   poll([{fd=5, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout) <0.100506>
>   write(4, "00000000000000000000000000000000"..., 102) = 102 <0.000057>
>
> That would likewise be solved by using ungroup=1 (in which case we do
> not poll, but just call the feed function immediately again) or by using
> a real poll() loop (which would see immediately that the hook is ready
> for more input, rather than hitting the 100ms timeout).

> The last batch of hooks converted to the hook.[ch] API introduced
> a regression because pick_next_hook() always sets stdout_to_stderr
> for its child processes.
>
> Pre-push is the only hook API user which requires stdout_to_stderr
> to be 0, so it can be argued that pre-push needs fixing, however
> this will likely break many pre-push hooks, so it's better to allow
> it to be 0, i.e. to match the previous behavior.
>
> To prevent such regressions in the future, extend the hook tests to
> verify hooks write to the expected stdout vs stderr streams and
> maintain backward compatibility with the hooks output assumptions.
>
> The tests are independent of the actual hook implementations: I've
> tested they work the same before and after the hook.[ch] conversion
> and will continue to work after we eventually introduce parallel
> hook execution and config-based hooks.
>
> Reported-by: Chris Darroch <chrisd@apache.org>
> Suggested-by: brian m. carlson <sandals@crustytoothpaste.net>
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
> This is based on the latest master branch.
>
> 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)
>[snip]

> In 857f047e40 (hook: allow overriding the ungroup option, 2025-12-26),
> I accidentally made the ungroup option opt-in instead of opt-out and
> despite my best efforts to set it for all API users, I missed a case
> which requires it to be set: the pre-push hook which regressed.
> 
> The only thing I needed in that commit was a way to change the default,
> to convert the remaining receive-pack hooks which require ungroup == 0
> for sideband output, so it doesn't matter if it's on or off by default.
> 
> Bring back the original behavior by setting it for all hooks in the
> struct run_hooks_opt initializer, which nicely allows changing the
> default value only where needed, in receive-pack.c.

> On Wed, Jan 14, 2026 at 08:57:31PM +0200, Adrian Ratiu wrote:
>
>> In 857f047e40 (hook: allow overriding the ungroup option, 2025-12-26),
>> I accidentally made the ungroup option opt-in instead of opt-out and
>> despite my best efforts to set it for all API users, I missed a case
>> which requires it to be set: the pre-push hook which regressed.
>> 
>> The only thing I needed in that commit was a way to change the default,
>> to convert the remaining receive-pack hooks which require ungroup == 0
>> for sideband output, so it doesn't matter if it's on or off by default.
>> 
>> Bring back the original behavior by setting it for all hooks in the
>> struct run_hooks_opt initializer, which nicely allows changing the
>> default value only where needed, in receive-pack.c.
>
> I think this is an improvement overall to what's currently in 'seen',
> and the patch looks as I'd expect.

>
> I have doubts in general about the approach taken by c65f26fca4
> (receive-pack: convert receive hooks to hook API, 2025-12-26). We used
> to use an async muxer thread, and now we are buffering hook stderr,
> which to my mind is a regression (both in terms of real-time output, but
> also the deadlock issues mentioned earlier).
>
> I'd rather see us continue to set up a muxer thread, and then direct the
> hook API to attach the stderr of the hook processes to that descriptor.
> Then receive-pack would just work as before, without having to fiddle
> with the ungroup flag at all.

>
> You can take that with the appropriate size grain of salt from an
> observer who has not been following the series (and is not really
> interested in it, beyond making sure we do not introduce regressions).
> But it is also an observer who has dealt with many I/O deadlocks in Git. ;)

> In 857f047e40 (hook: allow overriding the ungroup option, 2025-12-26),
> I accidentally made the ungroup option opt-in instead of opt-out and
> despite my best efforts to set it for all API users, I missed a case
> which requires it to be set: the pre-push hook which regressed.
>
> The only thing I needed in that commit was a way to change the default,
> to convert the remaining receive-pack hooks which require ungroup == 0
> for sideband output, so it doesn't matter if it's on or off by default.
>
> Bring back the original behavior by setting it for all hooks in the
> struct run_hooks_opt initializer, which nicely allows changing the
> default value only where needed, in receive-pack.c.
>
> While at it add a few hook tests which exercise receive-pack sideband
> output since they are the only ungroup=0 exceptions and there are no
> other tests exercising this functionality.

>
> Fixes: 857f047e40f7 ("hook: allow overriding the ungroup option")
> Reported-by: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
> Suggested-by: Jeff King <peff@peff.net>
> Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
> ---
>[snip]

> Hello everyone,
>
> This series fixes 2 regressions reported by Chris and Kristoffer,
> introduced by the 'ar/run-command-hook' merge into master.
>
> Based on a discussion with Peff on v2, I do plan to revisit and
> rework the server-side hook I/O polling & batching logic, however
> that will be a separate patch unrelated to these two regressions.

> Adrian Ratiu <adrian.ratiu@collabora.com> writes:
>
>> Hello everyone,
>>
>> This series fixes 2 regressions reported by Chris and Kristoffer,
>> introduced by the 'ar/run-command-hook' merge into master.
>>
>> Based on a discussion with Peff on v2, I do plan to revisit and
>> rework the server-side hook I/O polling & batching logic, however
>> that will be a separate patch unrelated to these two regressions.
>
> I've read these two over once again, and am inclined to say that we
> should merge these in upcoming 2.53 release.  Opinions?

> I agree with this.
>
> We can't let these two regressions enter a release, so we have two
> real chices:
>
> 1. Merge both fixes to 1.53 or
> 2. Revert the 'ar/run-command-hook' topic merge.

> Adrian Ratiu <adrian.ratiu@collabora.com> writes:
>
>> I agree with this.
>>
>> We can't let these two regressions enter a release, so we have two
>> real chices:
>>
>> 1. Merge both fixes to 1.53 or
>> 2. Revert the 'ar/run-command-hook' topic merge.
>
> Hmph, at this early point in the late release cycle before -rc1
> (yes, rc0 is scheduled for this morning, but that is not really a
> release candidate that counts as anything), it is tempting to take
> #2, actually.  I just do not know how much damage such a revert
> would cause to the tree.  I'll experiment after I finish cutting the
> -rc0 preview release.

> On Thu, 15 Jan 2026, Junio C Hamano <gitster@pobox.com> wrote:
>> Adrian Ratiu <adrian.ratiu@collabora.com> writes:
>>
>>> I agree with this.
>>>
>>> We can't let these two regressions enter a release, so we have two
>>> real chices:
>>>
>>> 1. Merge both fixes to 1.53 or
>>> 2. Revert the 'ar/run-command-hook' topic merge.
>>
>> Hmph, at this early point in the late release cycle before -rc1
>> (yes, rc0 is scheduled for this morning, but that is not really a
>> release candidate that counts as anything), it is tempting to take
>> #2, actually.  I just do not know how much damage such a revert
>> would cause to the tree.  I'll experiment after I finish cutting the
>> -rc0 preview release.
>
> I do not expect any conflicts and, if there any, they should be trivial.
>
> Let me know if you need any help.

> Adrian Ratiu <adrian.ratiu@collabora.com> writes:
>
>> On Thu, 15 Jan 2026, Junio C Hamano <gitster@pobox.com> wrote:
>>> Adrian Ratiu <adrian.ratiu@collabora.com> writes:
>>>
>>>> I agree with this.
>>>>
>>>> We can't let these two regressions enter a release, so we have two
>>>> real chices:
>>>>
>>>> 1. Merge both fixes to 1.53 or
>>>> 2. Revert the 'ar/run-command-hook' topic merge.
>>>
>>> Hmph, at this early point in the late release cycle before -rc1
>>> (yes, rc0 is scheduled for this morning, but that is not really a
>>> release candidate that counts as anything), it is tempting to take
>>> #2, actually.  I just do not know how much damage such a revert
>>> would cause to the tree.  I'll experiment after I finish cutting the
>>> -rc0 preview release.
>>
>> I do not expect any conflicts and, if there any, they should be trivial.
>>
>> Let me know if you need any help.
>
> Thanks.  I think I got
>
>  - revert of ar/run-command-hook directly on top of 2.53-rc0, which
>    would become the tip of 'master' tomorrow.
>
>  - rebuild of ar/run-command-hook + two fix-up topics on top of it,
>    called ar/run-command-hook-take-2
>
> as the "take-2" topic is totally outside 'next', we can rebuild the
> entire topic and get it right the first time, instead of
> incrementally fixing them on top.