[PATCH] hook: make stdout_to_stderr optional

[PATCH] hook: make stdout_to_stderr optional

[Adrian Ratiu]

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.

We can introduce an extension for the breaking change of all hooks
sending stdout to stderr, however this just fixes the regression.

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.
Pushed to GitHub: https://github.com/10ne1/git/tree/dev/aratiu/make-hook-stdout_to_stderr-optional-v1
Succesful CI run: https://github.com/10ne1/git/actions/runs/20954859587
---
 hook.c      | 2 +-
 hook.h      | 6 ++++++
 transport.c | 1 +
 3 files changed, 8 insertions(+), 1 deletion(-)

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;
 
 	ret = run_hooks_opt(the_repository, "pre-push", &opt);
 
-- 
2.52.0

Re: [PATCH] hook: make stdout_to_stderr optional

[Patrick Steinhardt]

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?

Thanks!

Patrick

Re: [PATCH] hook: make stdout_to_stderr optional

[Adrian Ratiu]

On Tue, 13 Jan 2026, Patrick Steinhardt <ps@pks.im> wrote:
> 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.

No, we do not have test coverage in this area and this is also the
reason why this went unnoticed.

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

I'm still running tests on the full patch series, however the answer up
to now is no, I do not think we need to change this.

This means we can keep the existing behavior as-is and just introduce
some tests to detect when/if stdout/stderr output expectation regresses.

No breakage/changes to existing hooks.

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

Agreed.

I'll send a v2 containing a stdout/stderr expectation test for each
hook and remove the extension commment from the commit message.

Re: [PATCH] hook: make stdout_to_stderr optional

[Junio C Hamano]

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.

> We can introduce an extension for the breaking change of all hooks
> sending stdout to stderr, however this just fixes the regression.

Thanks.

Re: [PATCH] hook: make stdout_to_stderr optional

[Junio C Hamano]

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?

Thanks.

Re: [PATCH] hook: make stdout_to_stderr optional

[Adrian Ratiu]

On Tue, 13 Jan 2026, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

Yes, your understanding is correct in all the above.

Each hook used to set the child process stdout_to_stderr to 1 with the
exception of pre-push, which regressed.

As Patrick noted, we are also missing some test cases to verify hooks
write to stderr and pre-push to stdout.

I'll add these tests and send v2, unless you want to land this as-is to
fix the regression ASAP, then I'll add the tests in a separate commit.

Re: [PATCH] hook: make stdout_to_stderr optional

[Adrian Ratiu]

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.

Re: [PATCH] hook: make stdout_to_stderr optional

[Junio C Hamano]

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?

Thanks for working on the fix and the tests.

Re: [PATCH] hook: make stdout_to_stderr optional

[Adrian Ratiu]

On Tue, 13 Jan 2026, Junio C Hamano <gitster@pobox.com> wrote:
> 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?

Yes, that is an excellent idea. The tests should work the same before
and after the conversion. Will do.

[PATCH v2] hook: allow hooks to disable stdout_to_stderr

[Adrian Ratiu]

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)

Pushed to GitHub: https://github.com/10ne1/git/tree/dev/aratiu/make-hook-stdout_to_stderr-optional-v2
Succesful CI run: https://github.com/10ne1/git/actions/runs/20975732134
---
 hook.c          |   2 +-
 hook.h          |   6 +++
 t/t1800-hook.sh | 127 ++++++++++++++++++++++++++++++++++++++++++++++++
 transport.c     |   9 ++++
 4 files changed, 143 insertions(+), 1 deletion(-)

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/t/t1800-hook.sh b/t/t1800-hook.sh
index 4feaf0d7be..0e4f93fb31 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -184,4 +184,131 @@ test_expect_success 'stdin to hooks' '
 	test_cmp expect actual
 '
 
+check_stdout_separate_from_stderr () {
+	for hook in "$@"
+	do
+		test_grep ! "Hook $hook stdout" stderr.actual &&
+		test_grep ! "Hook $hook stderr" stdout.actual &&
+		test_grep "Hook $hook stderr" stderr.actual &&
+		test_grep "Hook $hook stdout" stdout.actual || return 1
+	done
+}
+
+check_stdout_merged_to_stderr () {
+	test_grep ! "Hook .* stdout" stdout.actual &&
+	test_grep ! "Hook .* stderr" stdout.actual &&
+	for hook in "$@"
+	do
+		test_grep "Hook $hook stdout" stderr.actual &&
+		test_grep "Hook $hook stderr" stderr.actual || return 1
+	done
+}
+
+test_expect_success 'client pre-push hook expects separate stdout and stderr' '
+	test_when_finished "rm -f stdout.actual stderr.actual" &&
+	git init --bare remote &&
+	git remote add origin remote &&
+	test_commit A &&
+
+	hook=pre-push &&
+	test_hook $hook <<-EOF &&
+	echo >&1 Hook $hook stdout
+	echo >&2 Hook $hook stderr
+	EOF
+
+	git push origin HEAD:main >stdout.actual 2>stderr.actual &&
+	check_stdout_separate_from_stderr pre-push
+'
+
+test_expect_success 'client hooks expect stdout redirected to stderr' '
+	test_when_finished "rm -f stdout.actual stderr.actual" &&
+	for hook in pre-commit post-commit post-checkout pre-merge-commit \
+		prepare-commit-msg commit-msg post-merge post-rewrite reference-transaction \
+		applypatch-msg pre-applypatch post-applypatch pre-rebase post-index-change
+	do
+		test_hook $hook <<-EOF || return 1
+		echo >&1 Hook $hook stdout
+		echo >&2 Hook $hook stderr
+		EOF
+	done &&
+
+	git checkout -B main &&
+	git checkout -b branch-a &&
+	test_commit commit-on-branch-a &&
+
+	# Trigger pre-commit, prepare-commit-msg, commit-msg, post-commit, reference-transaction
+	git commit --allow-empty -m "Test" >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr pre-commit prepare-commit-msg commit-msg post-commit reference-transaction &&
+
+	# Trigger post-checkout, reference-transaction
+	git checkout -b new-branch main >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr post-checkout reference-transaction &&
+
+	# Trigger pre-merge-commit, post-merge, reference-transaction
+	test_commit new-branch-commit &&
+	git merge --no-ff branch-a >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr pre-merge-commit post-merge reference-transaction &&
+
+	# Trigger post-rewrite, reference-transaction
+	git commit --amend --allow-empty --no-edit >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr post-rewrite reference-transaction &&
+
+	# Trigger applypatch-msg, pre-applypatch, post-applypatch
+	git checkout -b branch-b main &&
+	test_commit branch-b &&
+	git format-patch -1 --stdout >patch &&
+	git checkout -b branch-c main &&
+	git am patch >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr applypatch-msg pre-applypatch post-applypatch &&
+
+	# Trigger pre-rebase
+	git checkout -b branch-d main &&
+	test_commit branch-d &&
+	git checkout main &&
+	test_commit diverge-main &&
+	git checkout branch-d &&
+	git rebase main >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr pre-rebase &&
+
+	# Trigger post-index-change
+	oid=$(git hash-object -w --stdin </dev/null) &&
+	git update-index --add --cacheinfo 100644 $oid new-file >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr post-index-change
+'
+
+test_expect_success 'server hooks expect stdout redirected to stderr' '
+	test_when_finished "rm -f stdout.actual stderr.actual" &&
+	git init --bare remote-server &&
+	git remote add origin-server remote-server &&
+
+	for hook in pre-receive update post-receive post-update
+	do
+		write_script remote-server/hooks/$hook <<-EOF || return 1
+		echo >&1 Hook $hook stdout
+		echo >&2 Hook $hook stderr
+		EOF
+	done &&
+
+	# Trigger pre-receive update post-receive post-update
+	git push origin-server HEAD:new-branch >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr pre-receive update post-receive post-update
+'
+
+test_expect_success 'server push-to-checkout hook expects stdout redirected to stderr' '
+	test_when_finished "rm -f stdout.actual stderr.actual" &&
+	git init server &&
+	git -C server checkout -b main &&
+	test_config -C server receive.denyCurrentBranch updateInstead &&
+	git remote add origin-server-2 server &&
+
+	write_script server/.git/hooks/push-to-checkout <<-EOF &&
+	echo >&1 Hook push-to-checkout stdout
+	echo >&2 Hook push-to-checkout stderr
+	EOF
+
+	# Trigger push-to-checkout
+	git push origin-server-2 HEAD:main >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr push-to-checkout
+'
+
 test_done
diff --git a/transport.c b/transport.c
index 6d0f02be5d..5aa39626da 100644
--- a/transport.c
+++ b/transport.c
@@ -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;
+
 	ret = run_hooks_opt(the_repository, "pre-push", &opt);
 
 	strbuf_release(&data.buf);
-- 
2.52.0.732.gb351b5166d.dirty

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

[Jeff King]

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?

-Peff

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

[Kristoffer Haugsbakk]

On Wed, Jan 14, 2026, at 00:45, 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.
>
> 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]

This fixes the issue reported here: https://lore.kernel.org/git/249f08d1-4457-4a41-8dbe-9725c0c392de@app.fastmail.com/

(Subject: [BUG] push: pre-push hook that waits for stdin is slow)

Via: https://lore.kernel.org/git/87ecntqd9f.fsf@gentoo.mail-host-address-is-not-set/

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

[Adrian Ratiu]

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

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

[Adrian Ratiu]

On Wed, 14 Jan 2026, Adrian Ratiu <adrian.ratiu@collabora.com> wrote:
> 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.

Minor correction: I think we need 3 commits for 3 separate bugs we
uncovered (ungroup should have its own commit). :)

Please wait for v3, I will code, test and send it ASAP.

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

[Kristoffer Haugsbakk]

On Wed, Jan 14, 2026, at 09:46, Adrian Ratiu wrote:
>[snip]
> 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.

Yeah I tested your v1 before I sent the report and it didn’t work.

Thanks Adrian and Peff for explaining what is going on in the code. That
`cat` would hang and time out was beyond my understanding. :)

>[snip]

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

[Jeff King]

On Wed, Jan 14, 2026 at 10:46:39AM +0200, Adrian Ratiu wrote:

> > 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):

-- >8 --
#!/bin/sh

# make two repos: one to push from, and one to push into
rm -rf repo
git init repo
cd repo
git init --bare dst.git

# And here's our pre-receive hook that will cause problems.
cat >dst.git/hooks/pre-receive <<\EOF
#!/bin/sh

# Imagine we write a lot of output to stderr. For example, progress
# reporting for some kind of setup procedure (but it could be anything).
# The key thing is that it is enough to fill up the pipe buffer going
# back to git.
for i in $(seq 10000); do printf "\rprocessing $i..."; done
echo done

# and now we are ready to read the input from the caller. A real hook
# would do something useful with the input, but we'll just read it
# and discard.
cat >/dev/null
EOF
chmod +x dst.git/hooks/pre-receive

# And now do a big push. 1000 ref updates seems to be enough to fill up
# the pipe buffer (each one is 2 oids plus the ref name plus whitespace,
# which is 100+ bytes each).
git commit --allow-empty -m foo
seq --format='create refs/heads/branch-%g HEAD' 1000 |
git update-ref --stdin
git push -q --all dst.git
-- >8 --

This will deadlock when run using the ar/run-command-hook topic. What
happens is this:

  1. Git writes out the first 500 lines to the hook. This partially
     fills the pipe buffer going to the hook.

  2. The hook writes to stderr, filling up the pipe buffer back to Git
     and blocking.

  3. Git does its poll() and sees that there is data to read on stderr.
     It reads some of it (8k, I think, due to strbuf_read_once).

  4. The hook sees more room in the pipe, so it writes another 8k. But
     it blocks again, still not having read any of its stdin.

  5. Git, having done one round of poll(), goes back to trying to write
     to the hook's stdin, and tries for another 500 lines. But since the
     hook did not read anything from stdin, this fills up the pipe
     buffer.

  6. Now we are deadlocked. Git is blocked trying to write to the hook's
     stdin, but the hook is blocked trying to write to stderr.

To solve this we must either:

  a. Make sure ungroup=1 is set, which means that Git does not read back
     stderr. In which case the 500-line batching is pointless. We can
     just write everything! But I assume you do not want to do this, as I'd
     guess the point of the series is that we want to buffer the stderr
     of each hook so that multiple hooks can be run in parallel without
     stomping on each other's output.

  b. Do a real poll() loop that checks both for incoming data on stderr
     from the hook, but also for the ability to write to the hook's
     stdin. Look at how pipe_command() and pump_io() do this, for
     example. You'd want something like that, but extended across
     multiple sub-processes running at once.

-Peff

PS If the goal of the series is to buffer stderr, that has another side
   effect: hooks can no longer produce real-time progress updates. Maybe
   losing that ability is a good tradeoff to keep the stderr output from
   multiple hooks from stomping on each other. But for a single hook,
   should we retain the existing behavior?

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

[Jeff King]

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

-Peff

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

[Adrian Ratiu]

On Wed, 14 Jan 2026, Jeff King <peff@peff.net> wrote:
> 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).

Thanks for the detailed examples.

For the server-side hooks, I think the way forward is to implement the
poll loop as you suggested so we can buffer stderr and for a single
(non-parallel) hook, we can keep the existing behavior (still need to
test this). I'll do that in a separate patch and drop the batching.

For the client side hooks, I'll send v3 of this series which fixes the
two regressions reported by Chris and Kristoffer.

[PATCH v3 0/2] Fix two hook conversion regressions

[Adrian Ratiu]

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.

Many thanks to everyone who helped debug & fix these!

This series is based on the master branch.

Pushed to GitHub: https://github.com/10ne1/git/tree/dev/aratiu/make-hook-stdout_to_stderr-optional-v3
Successful CI run: https://github.com/10ne1/git/actions/runs/21004980299

Changes in v3:
* New commit to make hook opts.ungroup = 1 default (Peff)
* Dropped the ungroup fix from the first commit because it's now
  handled by the more comprehensive second commit (Peff, Adrian)
* Added fixes tags to commits (Adrian)

Range-diff between v2 -> v3:
1:  898a21ddd0 ! 1:  77db7035c5 hook: allow hooks to disable stdout_to_stderr
    @@ Commit message
         and will continue to work after we eventually introduce parallel
         hook execution and config-based hooks.
     
    +    Fixes: 3e2836a742d8 ("transport: convert pre-push to hook API")
         Reported-by: Chris Darroch <chrisd@apache.org>
         Suggested-by: brian m. carlson <sandals@crustytoothpaste.net>
         Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
    @@ transport.c: static int run_pre_push_hook(struct transport *transport,
     +	/*
     +	 * 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;
     +
      	ret = run_hooks_opt(the_repository, "pre-push", &opt);
      
-:  ---------- > 2:  de3001f063 hook: make ungroup opt-out instead of opt-in

Adrian Ratiu (2):
  hook: allow hooks to disable stdout_to_stderr
  hook: make ungroup opt-out instead of opt-in

 builtin/hook.c         |   6 --
 builtin/receive-pack.c |  12 ++-
 commit.c               |   3 -
 hook.c                 |   5 +-
 hook.h                 |   7 ++
 t/t1800-hook.sh        | 176 +++++++++++++++++++++++++++++++++++++++++
 transport.c            |   6 ++
 7 files changed, 199 insertions(+), 16 deletions(-)

-- 
2.52.0.732.gb351b5166d.dirty

[PATCH v3 1/2] hook: allow hooks to disable stdout_to_stderr

[Adrian Ratiu]

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.

Fixes: 3e2836a742d8 ("transport: convert pre-push to hook API")
Reported-by: Chris Darroch <chrisd@apache.org>
Suggested-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Adrian Ratiu <adrian.ratiu@collabora.com>
---
 hook.c          |   2 +-
 hook.h          |   6 +++
 t/t1800-hook.sh | 127 ++++++++++++++++++++++++++++++++++++++++++++++++
 transport.c     |   6 +++
 4 files changed, 140 insertions(+), 1 deletion(-)

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/t/t1800-hook.sh b/t/t1800-hook.sh
index 4feaf0d7be..0e4f93fb31 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -184,4 +184,131 @@ test_expect_success 'stdin to hooks' '
 	test_cmp expect actual
 '
 
+check_stdout_separate_from_stderr () {
+	for hook in "$@"
+	do
+		test_grep ! "Hook $hook stdout" stderr.actual &&
+		test_grep ! "Hook $hook stderr" stdout.actual &&
+		test_grep "Hook $hook stderr" stderr.actual &&
+		test_grep "Hook $hook stdout" stdout.actual || return 1
+	done
+}
+
+check_stdout_merged_to_stderr () {
+	test_grep ! "Hook .* stdout" stdout.actual &&
+	test_grep ! "Hook .* stderr" stdout.actual &&
+	for hook in "$@"
+	do
+		test_grep "Hook $hook stdout" stderr.actual &&
+		test_grep "Hook $hook stderr" stderr.actual || return 1
+	done
+}
+
+test_expect_success 'client pre-push hook expects separate stdout and stderr' '
+	test_when_finished "rm -f stdout.actual stderr.actual" &&
+	git init --bare remote &&
+	git remote add origin remote &&
+	test_commit A &&
+
+	hook=pre-push &&
+	test_hook $hook <<-EOF &&
+	echo >&1 Hook $hook stdout
+	echo >&2 Hook $hook stderr
+	EOF
+
+	git push origin HEAD:main >stdout.actual 2>stderr.actual &&
+	check_stdout_separate_from_stderr pre-push
+'
+
+test_expect_success 'client hooks expect stdout redirected to stderr' '
+	test_when_finished "rm -f stdout.actual stderr.actual" &&
+	for hook in pre-commit post-commit post-checkout pre-merge-commit \
+		prepare-commit-msg commit-msg post-merge post-rewrite reference-transaction \
+		applypatch-msg pre-applypatch post-applypatch pre-rebase post-index-change
+	do
+		test_hook $hook <<-EOF || return 1
+		echo >&1 Hook $hook stdout
+		echo >&2 Hook $hook stderr
+		EOF
+	done &&
+
+	git checkout -B main &&
+	git checkout -b branch-a &&
+	test_commit commit-on-branch-a &&
+
+	# Trigger pre-commit, prepare-commit-msg, commit-msg, post-commit, reference-transaction
+	git commit --allow-empty -m "Test" >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr pre-commit prepare-commit-msg commit-msg post-commit reference-transaction &&
+
+	# Trigger post-checkout, reference-transaction
+	git checkout -b new-branch main >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr post-checkout reference-transaction &&
+
+	# Trigger pre-merge-commit, post-merge, reference-transaction
+	test_commit new-branch-commit &&
+	git merge --no-ff branch-a >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr pre-merge-commit post-merge reference-transaction &&
+
+	# Trigger post-rewrite, reference-transaction
+	git commit --amend --allow-empty --no-edit >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr post-rewrite reference-transaction &&
+
+	# Trigger applypatch-msg, pre-applypatch, post-applypatch
+	git checkout -b branch-b main &&
+	test_commit branch-b &&
+	git format-patch -1 --stdout >patch &&
+	git checkout -b branch-c main &&
+	git am patch >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr applypatch-msg pre-applypatch post-applypatch &&
+
+	# Trigger pre-rebase
+	git checkout -b branch-d main &&
+	test_commit branch-d &&
+	git checkout main &&
+	test_commit diverge-main &&
+	git checkout branch-d &&
+	git rebase main >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr pre-rebase &&
+
+	# Trigger post-index-change
+	oid=$(git hash-object -w --stdin </dev/null) &&
+	git update-index --add --cacheinfo 100644 $oid new-file >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr post-index-change
+'
+
+test_expect_success 'server hooks expect stdout redirected to stderr' '
+	test_when_finished "rm -f stdout.actual stderr.actual" &&
+	git init --bare remote-server &&
+	git remote add origin-server remote-server &&
+
+	for hook in pre-receive update post-receive post-update
+	do
+		write_script remote-server/hooks/$hook <<-EOF || return 1
+		echo >&1 Hook $hook stdout
+		echo >&2 Hook $hook stderr
+		EOF
+	done &&
+
+	# Trigger pre-receive update post-receive post-update
+	git push origin-server HEAD:new-branch >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr pre-receive update post-receive post-update
+'
+
+test_expect_success 'server push-to-checkout hook expects stdout redirected to stderr' '
+	test_when_finished "rm -f stdout.actual stderr.actual" &&
+	git init server &&
+	git -C server checkout -b main &&
+	test_config -C server receive.denyCurrentBranch updateInstead &&
+	git remote add origin-server-2 server &&
+
+	write_script server/.git/hooks/push-to-checkout <<-EOF &&
+	echo >&1 Hook push-to-checkout stdout
+	echo >&2 Hook push-to-checkout stderr
+	EOF
+
+	# Trigger push-to-checkout
+	git push origin-server-2 HEAD:main >stdout.actual 2>stderr.actual &&
+	check_stdout_merged_to_stderr push-to-checkout
+'
+
 test_done
diff --git a/transport.c b/transport.c
index 6d0f02be5d..e876cc9189 100644
--- a/transport.c
+++ b/transport.c
@@ -1373,6 +1373,12 @@ 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.
+	 */
+	opt.stdout_to_stderr = 0;
+
 	ret = run_hooks_opt(the_repository, "pre-push", &opt);
 
 	strbuf_release(&data.buf);
-- 
2.52.0.732.gb351b5166d.dirty

[PATCH v3 2/2] hook: make ungroup opt-out instead of opt-in

[Adrian Ratiu]

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>
---
 builtin/hook.c         |  6 ------
 builtin/receive-pack.c | 12 ++++++++---
 commit.c               |  3 ---
 hook.c                 |  3 ---
 hook.h                 |  1 +
 t/t1800-hook.sh        | 49 ++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 59 insertions(+), 15 deletions(-)

diff --git a/builtin/hook.c b/builtin/hook.c
index 73e7b8c2e8..7afec380d2 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -43,12 +43,6 @@ static int run(int argc, const char **argv, const char *prefix,
 	if (!argc)
 		goto usage;
 
-	/*
-	 * All current "hook run" use-cases require ungrouped child output.
-	 * If this changes, a hook run argument can be added to toggle it.
-	 */
-	opt.ungroup = 1;
-
 	/*
 	 * Having a -- for "run" when providing <hook-args> is
 	 * mandatory.
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index ef1f77be8c..2d1a94f3a9 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -905,8 +905,10 @@ static int run_receive_hook(struct command *commands,
 	prepare_push_cert_sha1(&opt);
 
 	/* set up sideband printer */
-	if (use_sideband)
+	if (use_sideband) {
 		opt.consume_output = hook_output_to_sideband;
+		opt.ungroup = 0; /* mandatory for sideband output */
+	}
 
 	/* set up stdin callback */
 	feed_state.cmd = commands;
@@ -933,8 +935,10 @@ static int run_update_hook(struct command *cmd)
 		     oid_to_hex(&cmd->new_oid),
 		     NULL);
 
-	if (use_sideband)
+	if (use_sideband) {
 		opt.consume_output = hook_output_to_sideband;
+		opt.ungroup = 0; /* mandatory for sideband output */
+	}
 
 	return run_hooks_opt(the_repository, "update", &opt);
 }
@@ -1623,8 +1627,10 @@ static void run_update_post_hook(struct command *commands)
 	if (!opt.args.nr)
 		return;
 
-	if (use_sideband)
+	if (use_sideband) {
 		opt.consume_output = hook_output_to_sideband;
+		opt.ungroup = 0; /* mandatory for sideband output */
+	}
 
 	run_hooks_opt(the_repository, "post-update", &opt);
 }
diff --git a/commit.c b/commit.c
index efd0c02683..28bb5ce029 100644
--- a/commit.c
+++ b/commit.c
@@ -1978,9 +1978,6 @@ int run_commit_hook(int editor_is_used, const char *index_file,
 		strvec_push(&opt.args, arg);
 	va_end(args);
 
-	/* All commit hook use-cases require ungrouping child output. */
-	opt.ungroup = 1;
-
 	opt.invoked_hook = invoked_hook;
 	return run_hooks_opt(the_repository, name, &opt);
 }
diff --git a/hook.c b/hook.c
index ebd9d9e26e..4e7631132a 100644
--- a/hook.c
+++ b/hook.c
@@ -199,9 +199,6 @@ int run_hooks(struct repository *r, const char *hook_name)
 {
 	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
 
-	/* All use-cases of this API require ungrouping. */
-	opt.ungroup = 1;
-
 	return run_hooks_opt(r, hook_name, &opt);
 }
 
diff --git a/hook.h b/hook.h
index 2488db7133..b2b4db2b3d 100644
--- a/hook.h
+++ b/hook.h
@@ -99,6 +99,7 @@ struct run_hooks_opt
 	.env = STRVEC_INIT, \
 	.args = STRVEC_INIT, \
 	.stdout_to_stderr = 1, \
+	.ungroup = 1, \
 }
 
 struct hook_cb_data {
diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 0e4f93fb31..0555a1fd42 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -311,4 +311,53 @@ test_expect_success 'server push-to-checkout hook expects stdout redirected to s
 	check_stdout_merged_to_stderr push-to-checkout
 '
 
+test_expect_success 'receive-pack hooks sideband output works' '
+	git init --bare target-sideband.git &&
+	test_commit sideband-a &&
+	git remote add origin-sideband ./target-sideband.git &&
+
+	# pre-receive hook
+	test_hook -C target-sideband.git pre-receive <<-\EOF &&
+	echo "stdout pre-receive"
+	echo "stderr pre-receive" >&2
+	EOF
+
+	git push origin-sideband HEAD:refs/heads/pre-receive 2>actual &&
+	test_grep "remote: stdout pre-receive" actual &&
+	test_grep "remote: stderr pre-receive" actual &&
+
+	# update hook
+	test_hook -C target-sideband.git update <<-\EOF &&
+	echo "stdout update"
+	echo "stderr update" >&2
+	EOF
+
+	test_commit sideband-b &&
+	git push origin-sideband HEAD:refs/heads/update 2>actual &&
+	test_grep "remote: stdout update" actual &&
+	test_grep "remote: stderr update" actual &&
+
+	# post-receive hook
+	test_hook -C target-sideband.git post-receive <<-\EOF &&
+	echo >&1 "stdout post-receive"
+	echo >&2 "stderr post-receive"
+	EOF
+
+	test_commit sideband-c &&
+	git push origin-sideband HEAD:refs/heads/post-receive 2>actual &&
+	test_grep "remote: stdout post-receive" actual &&
+	test_grep "remote: stderr post-receive" actual &&
+
+	# post-update hook
+	test_hook -C target-sideband.git post-update <<-\EOF &&
+	echo >&1 "stdout post-update"
+	echo >&2 "stderr post-update"
+	EOF
+
+	test_commit sideband-d &&
+	git push origin-sideband HEAD:refs/heads/post-update 2>actual &&
+	test_grep "remote: stdout post-update" actual &&
+	test_grep "remote: stderr post-update" actual
+'
+
 test_done
-- 
2.52.0.732.gb351b5166d.dirty

Re: [PATCH v3 2/2] hook: make ungroup opt-out instead of opt-in

[Jeff King]

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

-Peff

Re: [PATCH v3 2/2] hook: make ungroup opt-out instead of opt-in

[Adrian Ratiu]

On Wed, 14 Jan 2026, Jeff King <peff@peff.net> wrote:
> 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.

Thanks. :)

My intention for this series is to fix the two regressions reported by
Chris and Kristoffer ASAP and not touch receive-pack (yet!) because it's
a separate topic which deserves its own patch & review / discussion.

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

I am certainly open to try this other design, especially if we can
eliminate the risk of deadlocks. I will code something along these
lines then send it for you to review.

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

No worries, all feedback is welcome. 

I genuinely appreciate your ideas and suggestions. :)

Expect a receive-pack patch from me soon, in a separate topic.

Re: [PATCH v3 0/2] Fix two hook conversion regressions

[Junio C Hamano]

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?

Thanks.

Re: [PATCH v3 0/2] Fix two hook conversion regressions

[Adrian Ratiu]

On Thu, 15 Jan 2026, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

The only remaining known open issue is the potential deadlocks in
server-side hooks highlighted by Peff, however that is less severe than
these two (I'd actually be surprised if anyone hits in practice without
a well crafted use case, having access to those hooks).

So I'm inclined for option 1, to land the fixes.

(OFC I'm working on the deadlock issue in parallel, just addressed the
user bug reports first).

Thanks,
Adrian

Re: [PATCH v3 0/2] Fix two hook conversion regressions

[Junio C Hamano]

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.

Re: [PATCH v3 0/2] Fix two hook conversion regressions

[Adrian Ratiu]

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.

Re: [PATCH v3 0/2] Fix two hook conversion regressions

[Junio C Hamano]

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.

Re: [PATCH v3 0/2] Fix two hook conversion regressions

[Adrian Ratiu]

On Thu, 15 Jan 2026, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

Cool. I'll integrate the fixes into the series and send v7 continuing
where we left off.

Though I'll send the new test separately, in advance: there's no use
blocking the new regression tests after the hooks conversions.

Re: [PATCH v3 2/2] hook: make ungroup opt-out instead of opt-in

[Kristoffer Haugsbakk]

On Wed, Jan 14, 2026, at 19:57, 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.
>
> 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.

This description looks okay given that the regression was never
released. Only those who go out of their way build on top of `master`
(for some reason) could have observed it. However if this was a bug in
some release then the description is very technical and play-by-play. As
a Git user reading this isolation, I see nothing that links these
concrete code discussions back to something that might have been weird
in my hook scripts.

This would be especially relevant given that this bug is so weird. It’s
not a crash with some error text that can be googled; everything works
(eventually) the same as before, only that *if* you read from standard
input you’ll have to wait a minute or so for something to time out and
unstuck the whole process.

Again. I think this is okay since it was never a bug in any release.

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