{"thread":{"id":"64791","subject":"[PATCH] hook: make stdout_to_stderr optional","startedAt":"2026-01-13T11:57:53Z","lastAt":"2026-01-18T08:45:15Z","messageCount":30,"participants":["Adrian Ratiu","Patrick Steinhardt","Junio C Hamano","Jeff King","Kristoffer Haugsbakk"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"533731","messageId":"20260113115633.230479-1-adrian.ratiu@collabora.com","threadId":"64791","inReplyTo":null,"subject":"[PATCH] hook: make stdout_to_stderr optional","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-13T11:56:33Z","receivedAt":"2026-01-13T11:57:53Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"The last batch of hooks converted to the hook.[ch] API introduced\na regression because pick_next_hook() always sets stdout_to_stderr\nfor its child processes.\n\nPre-push is the only hook API user which requires stdout_to_stderr\nto be 0, so it can be argued that pre-push needs fixing, however\nthis will likely break many pre-push hooks, so it's better to allow\nit to be 0, i.e. to match the previous behavior.\n\nWe can introduce an extension for the breaking change of all hooks\nsending stdout to stderr, however this just fixes the regression.\n\nReported-by: Chris Darroch \nSuggested-by: brian m. carlson \nSigned-off-by: Adrian Ratiu \n---\nThis is based on the latest master branch.\nPushed to GitHub: https://github.com/10ne1/git/tree/dev/aratiu/make-hook-stdout_to_stderr-optional-v1\nSuccesful CI run: https://github.com/10ne1/git/actions/runs/20954859587\n---\n hook.c | 2 +-\n hook.h | 6 ++++++\n transport.c | 1 +\n 3 files changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/hook.c b/hook.c\nindex 35211e5ed7..ebd9d9e26e 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -81,7 +81,7 @@ static int pick_next_hook(struct child_process *cp,\n \t\tcp->in = -1;\n \t}\n \n-\tcp->stdout_to_stderr = 1;\n+\tcp->stdout_to_stderr = hook_cb->options->stdout_to_stderr;\n \tcp->trace2_hook_name = hook_cb->hook_name;\n \tcp->dir = hook_cb->options->dir;\n \ndiff --git a/hook.h b/hook.h\nindex ae502178b9..2488db7133 100644\n--- a/hook.h\n+++ b/hook.h\n@@ -39,6 +39,11 @@ struct run_hooks_opt\n \t */\n \tunsigned int ungroup:1;\n \n+\t/**\n+\t * Send the hook's stdout to stderr.\n+\t */\n+\tunsigned int stdout_to_stderr:1;\n+\n \t/**\n \t * Path to file which should be piped to stdin for each hook.\n \t */\n@@ -93,6 +98,7 @@ struct run_hooks_opt\n #define RUN_HOOKS_OPT_INIT { \\\n \t.env = STRVEC_INIT, \\\n \t.args = STRVEC_INIT, \\\n+\t.stdout_to_stderr = 1, \\\n }\n \n struct hook_cb_data {\ndiff --git a/transport.c b/transport.c\nindex 6d0f02be5d..8f0e5987ab 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1372,6 +1372,7 @@ static int run_pre_push_hook(struct transport *transport,\n \n \topt.feed_pipe = pre_push_hook_feed_stdin;\n \topt.feed_pipe_cb_data = &data;\n+\topt.stdout_to_stderr = 0;\n \n \tret = run_hooks_opt(the_repository, \"pre-push\", &opt);\n \n-- \n2.52.0\n\n"},{"id":"533739","messageId":"aWZKYAxhavFc1ZaH@pks.im","threadId":"64791","inReplyTo":"20260113115633.230479-1-adrian.ratiu@collabora.com","subject":"Re: [PATCH] hook: make stdout_to_stderr optional","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-13T13:36:32Z","receivedAt":"2026-01-13T13:36:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 13, 2026 at 01:56:33PM +0200, Adrian Ratiu wrote:\n> The last batch of hooks converted to the hook.[ch] API introduced\n> a regression because pick_next_hook() always sets stdout_to_stderr\n> for its child processes.\n> \n> Pre-push is the only hook API user which requires stdout_to_stderr\n> to be 0, so it can be argued that pre-push needs fixing, however\n> this will likely break many pre-push hooks, so it's better to allow\n> it to be 0, i.e. to match the previous behavior.\n\nOkay. Do you happen to know whether we've got test coverage for those\nother hooks? Would be great to verify whether changing\n`stodut_to_stderr` to default-disabled causes at least one test to fail\nfor every hook we've got.\n\n> We can introduce an extension for the breaking change of all hooks\n> sending stdout to stderr, however this just fixes the regression.\n\nIs it really necessary to change this though? I wouldn't really want to\ngo there without a good reason.\n\n> diff --git a/transport.c b/transport.c\n> index 6d0f02be5d..8f0e5987ab 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1372,6 +1372,7 @@ static int run_pre_push_hook(struct transport *transport,\n> \n> \topt.feed_pipe = pre_push_hook_feed_stdin;\n> \topt.feed_pipe_cb_data = &data;\n> +\topt.stdout_to_stderr = 0;\n> \n> \tret = run_hooks_opt(the_repository, \"pre-push\", &opt);\n\nThe fact that this was able to sneak in without anybody noticing shows\nthat we have a test gap. Can we maybe have a test that verifies that the\nhook output goes to the correct standard stream?\n\nThanks!\n\nPatrick\n"},{"id":"533741","messageId":"87ms2hipma.fsf@collabora.com","threadId":"64791","inReplyTo":"aWZKYAxhavFc1ZaH@pks.im","subject":"Re: [PATCH] hook: make stdout_to_stderr optional","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-13T13:55:25Z","receivedAt":"2026-01-13T13:55:46Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Tue, 13 Jan 2026, Patrick Steinhardt wrote:\n> On Tue, Jan 13, 2026 at 01:56:33PM +0200, Adrian Ratiu wrote:\n>> The last batch of hooks converted to the hook.[ch] API introduced\n>> a regression because pick_next_hook() always sets stdout_to_stderr\n>> for its child processes.\n>> \n>> Pre-push is the only hook API user which requires stdout_to_stderr\n>> to be 0, so it can be argued that pre-push needs fixing, however\n>> this will likely break many pre-push hooks, so it's better to allow\n>> it to be 0, i.e. to match the previous behavior.\n>\n> Okay. Do you happen to know whether we've got test coverage for those\n> other hooks? Would be great to verify whether changing\n> `stodut_to_stderr` to default-disabled causes at least one test to fail\n> for every hook we've got.\n\nNo, we do not have test coverage in this area and this is also the\nreason why this went unnoticed.\n\n>> We can introduce an extension for the breaking change of all hooks\n>> sending stdout to stderr, however this just fixes the regression.\n>\n> Is it really necessary to change this though? I wouldn't really want to\n> go there without a good reason.\n\nI'm still running tests on the full patch series, however the answer up\nto now is no, I do not think we need to change this.\n\nThis means we can keep the existing behavior as-is and just introduce\nsome tests to detect when/if stdout/stderr output expectation regresses.\n\nNo breakage/changes to existing hooks.\n\n>> diff --git a/transport.c b/transport.c\n>> index 6d0f02be5d..8f0e5987ab 100644\n>> --- a/transport.c\n>> +++ b/transport.c\n>> @@ -1372,6 +1372,7 @@ static int run_pre_push_hook(struct transport *transport,\n>> \n>> \topt.feed_pipe = pre_push_hook_feed_stdin;\n>> \topt.feed_pipe_cb_data = &data;\n>> +\topt.stdout_to_stderr = 0;\n>> \n>> \tret = run_hooks_opt(the_repository, \"pre-push\", &opt);\n>\n> The fact that this was able to sneak in without anybody noticing shows\n> that we have a test gap. Can we maybe have a test that verifies that the\n> hook output goes to the correct standard stream?\n\nAgreed.\n\nI'll send a v2 containing a stdout/stderr expectation test for each\nhook and remove the extension commment from the commit message.\n"},{"id":"533742","messageId":"xmqq7btlliip.fsf@gitster.g","threadId":"64791","inReplyTo":"20260113115633.230479-1-adrian.ratiu@collabora.com","subject":"Re: [PATCH] hook: make stdout_to_stderr optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-13T14:00:30Z","receivedAt":"2026-01-13T14:00:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adrian Ratiu writes:\n\n> diff --git a/hook.c b/hook.c\n> index 35211e5ed7..ebd9d9e26e 100644\n> --- a/hook.c\n> +++ b/hook.c\n> @@ -81,7 +81,7 @@ static int pick_next_hook(struct child_process *cp,\n> \t\tcp->in = -1;\n> \t}\n> \n> -\tcp->stdout_to_stderr = 1;\n> +\tcp->stdout_to_stderr = hook_cb->options->stdout_to_stderr;\n> \tcp->trace2_hook_name = hook_cb->hook_name;\n> \tcp->dir = hook_cb->options->dir;\n\nSo stdout_to_stderr used to get always forced to 1, but now the\nvalue comes from the hook option, which ...\n\n> diff --git a/hook.h b/hook.h\n> index ae502178b9..2488db7133 100644\n> --- a/hook.h\n> +++ b/hook.h\n> @@ -39,6 +39,11 @@ struct run_hooks_opt\n> \t */\n> \tunsigned int ungroup:1;\n> \n> +\t/**\n> +\t * Send the hook's stdout to stderr.\n> +\t */\n> +\tunsigned int stdout_to_stderr:1;\n> +\n> \t/**\n> \t * Path to file which should be piped to stdin for each hook.\n> \t */\n> @@ -93,6 +98,7 @@ struct run_hooks_opt\n> #define RUN_HOOKS_OPT_INIT { \\\n> \t.env = STRVEC_INIT, \\\n> \t.args = STRVEC_INIT, \\\n> +\t.stdout_to_stderr = 1, \\\n> }\n\n... defaults to 1 for everybody, unless a specific caller opts out\nby ...\n\n> struct hook_cb_data {\n> diff --git a/transport.c b/transport.c\n> index 6d0f02be5d..8f0e5987ab 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1372,6 +1372,7 @@ static int run_pre_push_hook(struct transport *transport,\n> \n> \topt.feed_pipe = pre_push_hook_feed_stdin;\n> \topt.feed_pipe_cb_data = &data;\n> +\topt.stdout_to_stderr = 0;\n\n... setting it to 0 in their hook option.\n\n> The last batch of hooks converted to the hook.[ch] API introduced\n> a regression because pick_next_hook() always sets stdout_to_stderr\n> for its child processes.\n>\n> Pre-push is the only hook API user which requires stdout_to_stderr\n> to be 0, so it can be argued that pre-push needs fixing, however\n> this will likely break many pre-push hooks, so it's better to allow\n> it to be 0, i.e. to match the previous behavior.\n\nWhat was the previous behaviour of code paths that ran other hooks?\nWas pre-push the only one that didn't divert standard output to\nstandard error? This patch does look like a proper regression fix\nin that case. I browsed \"git log -p 1627809eef..c65f26fca4\" (i.e.,\nthe change for \"Merge branch 'ar/run-command-hook'\") and random\nsampling (like run_receive_hook() that used run_and_feed_hook(),\nwhich set stdout_to_stderr to 1) seems to indicate that it is the\ncase.\n\n> We can introduce an extension for the breaking change of all hooks\n> sending stdout to stderr, however this just fixes the regression.\n\nThanks.\n"},{"id":"533743","messageId":"xmqqzf6hk3ox.fsf@gitster.g","threadId":"64791","inReplyTo":"xmqq7btlliip.fsf@gitster.g","subject":"Re: [PATCH] hook: make stdout_to_stderr optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-13T14:06:06Z","receivedAt":"2026-01-13T14:06:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> What was the previous behaviour of code paths that ran other hooks?\n> Was pre-push the only one that didn't divert standard output to\n> standard error? This patch does look like a proper regression fix\n> in that case. I browsed \"git log -p 1627809eef..c65f26fca4\" (i.e.,\n> the change for \"Merge branch 'ar/run-command-hook'\") and random\n> sampling (like run_receive_hook() that used run_and_feed_hook(),\n> which set stdout_to_stderr to 1) seems to indicate that it is the\n> case.\n\nBy the way, if stdout_to_stderr is by default set to true, but tnis\nregression fix allows specific callers to opt out of it, then the\ntitle \"make stdout_to_stderr optional\" is a bit misleaing. It makes\nit sound as if it is false by default and optionally turned on.\n\nPerhaps like \"hook: allow stdout_to_stderr optionally off\" or\nsomething?\n\nThanks.\n"},{"id":"533744","messageId":"87jyxlioup.fsf@collabora.com","threadId":"64791","inReplyTo":"xmqq7btlliip.fsf@gitster.g","subject":"Re: [PATCH] hook: make stdout_to_stderr optional","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-13T14:11:58Z","receivedAt":"2026-01-13T14:12:16Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Tue, 13 Jan 2026, Junio C Hamano wrote:\n> Adrian Ratiu writes:\n>\n>> diff --git a/hook.c b/hook.c\n>> index 35211e5ed7..ebd9d9e26e 100644\n>> --- a/hook.c\n>> +++ b/hook.c\n>> @@ -81,7 +81,7 @@ static int pick_next_hook(struct child_process *cp,\n>> \t\tcp->in = -1;\n>> \t}\n>> \n>> -\tcp->stdout_to_stderr = 1;\n>> +\tcp->stdout_to_stderr = hook_cb->options->stdout_to_stderr;\n>> \tcp->trace2_hook_name = hook_cb->hook_name;\n>> \tcp->dir = hook_cb->options->dir;\n>\n> So stdout_to_stderr used to get always forced to 1, but now the\n> value comes from the hook option, which ...\n>\n>> diff --git a/hook.h b/hook.h\n>> index ae502178b9..2488db7133 100644\n>> --- a/hook.h\n>> +++ b/hook.h\n>> @@ -39,6 +39,11 @@ struct run_hooks_opt\n>> \t */\n>> \tunsigned int ungroup:1;\n>> \n>> +\t/**\n>> +\t * Send the hook's stdout to stderr.\n>> +\t */\n>> +\tunsigned int stdout_to_stderr:1;\n>> +\n>> \t/**\n>> \t * Path to file which should be piped to stdin for each hook.\n>> \t */\n>> @@ -93,6 +98,7 @@ struct run_hooks_opt\n>> #define RUN_HOOKS_OPT_INIT { \\\n>> \t.env = STRVEC_INIT, \\\n>> \t.args = STRVEC_INIT, \\\n>> +\t.stdout_to_stderr = 1, \\\n>> }\n>\n> ... defaults to 1 for everybody, unless a specific caller opts out\n> by ...\n>\n>> struct hook_cb_data {\n>> diff --git a/transport.c b/transport.c\n>> index 6d0f02be5d..8f0e5987ab 100644\n>> --- a/transport.c\n>> +++ b/transport.c\n>> @@ -1372,6 +1372,7 @@ static int run_pre_push_hook(struct transport *transport,\n>> \n>> \topt.feed_pipe = pre_push_hook_feed_stdin;\n>> \topt.feed_pipe_cb_data = &data;\n>> +\topt.stdout_to_stderr = 0;\n>\n> ... setting it to 0 in their hook option.\n>\n>> The last batch of hooks converted to the hook.[ch] API introduced\n>> a regression because pick_next_hook() always sets stdout_to_stderr\n>> for its child processes.\n>>\n>> Pre-push is the only hook API user which requires stdout_to_stderr\n>> to be 0, so it can be argued that pre-push needs fixing, however\n>> this will likely break many pre-push hooks, so it's better to allow\n>> it to be 0, i.e. to match the previous behavior.\n>\n> What was the previous behaviour of code paths that ran other hooks?\n> Was pre-push the only one that didn't divert standard output to\n> standard error? This patch does look like a proper regression fix\n> in that case. I browsed \"git log -p 1627809eef..c65f26fca4\" (i.e.,\n> the change for \"Merge branch 'ar/run-command-hook'\") and random\n> sampling (like run_receive_hook() that used run_and_feed_hook(),\n> which set stdout_to_stderr to 1) seems to indicate that it is the\n> case.\n\nYes, your understanding is correct in all the above.\n\nEach hook used to set the child process stdout_to_stderr to 1 with the\nexception of pre-push, which regressed.\n\nAs Patrick noted, we are also missing some test cases to verify hooks\nwrite to stderr and pre-push to stdout.\n\nI'll add these tests and send v2, unless you want to land this as-is to\nfix the regression ASAP, then I'll add the tests in a separate commit.\n"},{"id":"533745","messageId":"87h5spimno.fsf@collabora.com","threadId":"64791","inReplyTo":"xmqqzf6hk3ox.fsf@gitster.g","subject":"Re: [PATCH] hook: make stdout_to_stderr optional","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-13T14:59:23Z","receivedAt":"2026-01-13T14:59:43Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Tue, 13 Jan 2026, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> What was the previous behaviour of code paths that ran other hooks?\n>> Was pre-push the only one that didn't divert standard output to\n>> standard error? This patch does look like a proper regression fix\n>> in that case. I browsed \"git log -p 1627809eef..c65f26fca4\" (i.e.,\n>> the change for \"Merge branch 'ar/run-command-hook'\") and random\n>> sampling (like run_receive_hook() that used run_and_feed_hook(),\n>> which set stdout_to_stderr to 1) seems to indicate that it is the\n>> case.\n>\n> By the way, if stdout_to_stderr is by default set to true, but tnis\n> regression fix allows specific callers to opt out of it, then the\n> title \"make stdout_to_stderr optional\" is a bit misleaing. It makes\n> it sound as if it is false by default and optionally turned on.\n>\n> Perhaps like \"hook: allow stdout_to_stderr optionally off\" or\n> something?\n\nAck. Will rename in v2.\n\nPlease wait for v2 because, while writing the tests, I noticed pre-push\nneeds 1 additional line (ungroup output) to function as before.\n"},{"id":"533746","messageId":"xmqqv7h5k05v.fsf@gitster.g","threadId":"64791","inReplyTo":"87h5spimno.fsf@collabora.com","subject":"Re: [PATCH] hook: make stdout_to_stderr optional","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-13T15:22:20Z","receivedAt":"2026-01-13T15:22:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adrian Ratiu writes:\n\n> On Tue, 13 Jan 2026, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>\n>>> What was the previous behaviour of code paths that ran other hooks?\n>>> Was pre-push the only one that didn't divert standard output to\n>>> standard error? This patch does look like a proper regression fix\n>>> in that case. I browsed \"git log -p 1627809eef..c65f26fca4\" (i.e.,\n>>> the change for \"Merge branch 'ar/run-command-hook'\") and random\n>>> sampling (like run_receive_hook() that used run_and_feed_hook(),\n>>> which set stdout_to_stderr to 1) seems to indicate that it is the\n>>> case.\n>>\n>> By the way, if stdout_to_stderr is by default set to true, but tnis\n>> regression fix allows specific callers to opt out of it, then the\n>> title \"make stdout_to_stderr optional\" is a bit misleaing. It makes\n>> it sound as if it is false by default and optionally turned on.\n>>\n>> Perhaps like \"hook: allow stdout_to_stderr optionally off\" or\n>> something?\n>\n> Ack. Will rename in v2.\n>\n> Please wait for v2 because, while writing the tests, I noticed pre-push\n> needs 1 additional line (ungroup output) to function as before.\n\nUnderstood. Thanks.\n\nWriting these tests would take particular care, I imagine. Apply\nthe test to the tip of the 'master' before ar/run-commmand-hook was\nmerged, to verify that the tests expect the behaviour before these\nseries, and then merge the result up in more recent 'master' to see\nthat the changes in ar/run-commmand-hook did not negatively change\nthe behaviour, or something like that?\n\nThanks for working on the fix and the tests.\n"},{"id":"533748","messageId":"87ecntikvc.fsf@collabora.com","threadId":"64791","inReplyTo":"xmqqv7h5k05v.fsf@gitster.g","subject":"Re: [PATCH] hook: make stdout_to_stderr optional","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-13T15:37:59Z","receivedAt":"2026-01-13T15:38:20Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Tue, 13 Jan 2026, Junio C Hamano wrote:\n> Adrian Ratiu writes:\n>\n>> On Tue, 13 Jan 2026, Junio C Hamano wrote:\n>>> Junio C Hamano writes:\n>>>\n>>>> What was the previous behaviour of code paths that ran other hooks?\n>>>> Was pre-push the only one that didn't divert standard output to\n>>>> standard error? This patch does look like a proper regression fix\n>>>> in that case. I browsed \"git log -p 1627809eef..c65f26fca4\" (i.e.,\n>>>> the change for \"Merge branch 'ar/run-command-hook'\") and random\n>>>> sampling (like run_receive_hook() that used run_and_feed_hook(),\n>>>> which set stdout_to_stderr to 1) seems to indicate that it is the\n>>>> case.\n>>>\n>>> By the way, if stdout_to_stderr is by default set to true, but tnis\n>>> regression fix allows specific callers to opt out of it, then the\n>>> title \"make stdout_to_stderr optional\" is a bit misleaing. It makes\n>>> it sound as if it is false by default and optionally turned on.\n>>>\n>>> Perhaps like \"hook: allow stdout_to_stderr optionally off\" or\n>>> something?\n>>\n>> Ack. Will rename in v2.\n>>\n>> Please wait for v2 because, while writing the tests, I noticed pre-push\n>> needs 1 additional line (ungroup output) to function as before.\n>\n> Understood. Thanks.\n>\n> Writing these tests would take particular care, I imagine. Apply\n> the test to the tip of the 'master' before ar/run-commmand-hook was\n> merged, to verify that the tests expect the behaviour before these\n> series, and then merge the result up in more recent 'master' to see\n> that the changes in ar/run-commmand-hook did not negatively change\n> the behaviour, or something like that?\n\nYes, that is an excellent idea. The tests should work the same before\nand after the conversion. Will do.\n"},{"id":"533781","messageId":"20260113234528.1749921-1-adrian.ratiu@collabora.com","threadId":"64791","inReplyTo":"20260113115633.230479-1-adrian.ratiu@collabora.com","subject":"[PATCH v2] hook: allow hooks to disable stdout_to_stderr","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-13T23:45:28Z","receivedAt":"2026-01-13T23:47:14Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"The last batch of hooks converted to the hook.[ch] API introduced\na regression because pick_next_hook() always sets stdout_to_stderr\nfor its child processes.\n\nPre-push is the only hook API user which requires stdout_to_stderr\nto be 0, so it can be argued that pre-push needs fixing, however\nthis will likely break many pre-push hooks, so it's better to allow\nit to be 0, i.e. to match the previous behavior.\n\nTo prevent such regressions in the future, extend the hook tests to\nverify hooks write to the expected stdout vs stderr streams and\nmaintain backward compatibility with the hooks output assumptions.\n\nThe tests are independent of the actual hook implementations: I've\ntested they work the same before and after the hook.[ch] conversion\nand will continue to work after we eventually introduce parallel\nhook execution and config-based hooks.\n\nReported-by: Chris Darroch \nSuggested-by: brian m. carlson \nSigned-off-by: Adrian Ratiu \n---\nThis is based on the latest master branch.\n\nChanges in v2:\n* Extended hook test coverage to detect future regressions (Junio, Patrick)\n* Reworded commit message and added explanatory comment (Junio, Patrick)\n* Set ungroup = 1 because grouping overrides stdout_to_stderr (Adrian)\n\nPushed to GitHub: https://github.com/10ne1/git/tree/dev/aratiu/make-hook-stdout_to_stderr-optional-v2\nSuccesful CI run: https://github.com/10ne1/git/actions/runs/20975732134\n---\n hook.c | 2 +-\n hook.h | 6 +++\n t/t1800-hook.sh | 127 ++++++++++++++++++++++++++++++++++++++++++++++++\n transport.c | 9 ++++\n 4 files changed, 143 insertions(+), 1 deletion(-)\n\ndiff --git a/hook.c b/hook.c\nindex 35211e5ed7..ebd9d9e26e 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -81,7 +81,7 @@ static int pick_next_hook(struct child_process *cp,\n \t\tcp->in = -1;\n \t}\n \n-\tcp->stdout_to_stderr = 1;\n+\tcp->stdout_to_stderr = hook_cb->options->stdout_to_stderr;\n \tcp->trace2_hook_name = hook_cb->hook_name;\n \tcp->dir = hook_cb->options->dir;\n \ndiff --git a/hook.h b/hook.h\nindex ae502178b9..2488db7133 100644\n--- a/hook.h\n+++ b/hook.h\n@@ -39,6 +39,11 @@ struct run_hooks_opt\n \t */\n \tunsigned int ungroup:1;\n \n+\t/**\n+\t * Send the hook's stdout to stderr.\n+\t */\n+\tunsigned int stdout_to_stderr:1;\n+\n \t/**\n \t * Path to file which should be piped to stdin for each hook.\n \t */\n@@ -93,6 +98,7 @@ struct run_hooks_opt\n #define RUN_HOOKS_OPT_INIT { \\\n \t.env = STRVEC_INIT, \\\n \t.args = STRVEC_INIT, \\\n+\t.stdout_to_stderr = 1, \\\n }\n \n struct hook_cb_data {\ndiff --git a/t/t1800-hook.sh b/t/t1800-hook.sh\nindex 4feaf0d7be..0e4f93fb31 100755\n--- a/t/t1800-hook.sh\n+++ b/t/t1800-hook.sh\n@@ -184,4 +184,131 @@ test_expect_success 'stdin to hooks' '\n \ttest_cmp expect actual\n '\n \n+check_stdout_separate_from_stderr () {\n+\tfor hook in \"$@\"\n+\tdo\n+\t\ttest_grep ! \"Hook $hook stdout\" stderr.actual &&\n+\t\ttest_grep ! \"Hook $hook stderr\" stdout.actual &&\n+\t\ttest_grep \"Hook $hook stderr\" stderr.actual &&\n+\t\ttest_grep \"Hook $hook stdout\" stdout.actual || return 1\n+\tdone\n+}\n+\n+check_stdout_merged_to_stderr () {\n+\ttest_grep ! \"Hook .* stdout\" stdout.actual &&\n+\ttest_grep ! \"Hook .* stderr\" stdout.actual &&\n+\tfor hook in \"$@\"\n+\tdo\n+\t\ttest_grep \"Hook $hook stdout\" stderr.actual &&\n+\t\ttest_grep \"Hook $hook stderr\" stderr.actual || return 1\n+\tdone\n+}\n+\n+test_expect_success 'client pre-push hook expects separate stdout and stderr' '\n+\ttest_when_finished \"rm -f stdout.actual stderr.actual\" &&\n+\tgit init --bare remote &&\n+\tgit remote add origin remote &&\n+\ttest_commit A &&\n+\n+\thook=pre-push &&\n+\ttest_hook $hook <<-EOF &&\n+\techo >&1 Hook $hook stdout\n+\techo >&2 Hook $hook stderr\n+\tEOF\n+\n+\tgit push origin HEAD:main >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_separate_from_stderr pre-push\n+'\n+\n+test_expect_success 'client hooks expect stdout redirected to stderr' '\n+\ttest_when_finished \"rm -f stdout.actual stderr.actual\" &&\n+\tfor hook in pre-commit post-commit post-checkout pre-merge-commit \\\n+\t\tprepare-commit-msg commit-msg post-merge post-rewrite reference-transaction \\\n+\t\tapplypatch-msg pre-applypatch post-applypatch pre-rebase post-index-change\n+\tdo\n+\t\ttest_hook $hook <<-EOF || return 1\n+\t\techo >&1 Hook $hook stdout\n+\t\techo >&2 Hook $hook stderr\n+\t\tEOF\n+\tdone &&\n+\n+\tgit checkout -B main &&\n+\tgit checkout -b branch-a &&\n+\ttest_commit commit-on-branch-a &&\n+\n+\t# Trigger pre-commit, prepare-commit-msg, commit-msg, post-commit, reference-transaction\n+\tgit commit --allow-empty -m \"Test\" >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr pre-commit prepare-commit-msg commit-msg post-commit reference-transaction &&\n+\n+\t# Trigger post-checkout, reference-transaction\n+\tgit checkout -b new-branch main >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr post-checkout reference-transaction &&\n+\n+\t# Trigger pre-merge-commit, post-merge, reference-transaction\n+\ttest_commit new-branch-commit &&\n+\tgit merge --no-ff branch-a >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr pre-merge-commit post-merge reference-transaction &&\n+\n+\t# Trigger post-rewrite, reference-transaction\n+\tgit commit --amend --allow-empty --no-edit >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr post-rewrite reference-transaction &&\n+\n+\t# Trigger applypatch-msg, pre-applypatch, post-applypatch\n+\tgit checkout -b branch-b main &&\n+\ttest_commit branch-b &&\n+\tgit format-patch -1 --stdout >patch &&\n+\tgit checkout -b branch-c main &&\n+\tgit am patch >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr applypatch-msg pre-applypatch post-applypatch &&\n+\n+\t# Trigger pre-rebase\n+\tgit checkout -b branch-d main &&\n+\ttest_commit branch-d &&\n+\tgit checkout main &&\n+\ttest_commit diverge-main &&\n+\tgit checkout branch-d &&\n+\tgit rebase main >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr pre-rebase &&\n+\n+\t# Trigger post-index-change\n+\toid=$(git hash-object -w --stdin stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr post-index-change\n+'\n+\n+test_expect_success 'server hooks expect stdout redirected to stderr' '\n+\ttest_when_finished \"rm -f stdout.actual stderr.actual\" &&\n+\tgit init --bare remote-server &&\n+\tgit remote add origin-server remote-server &&\n+\n+\tfor hook in pre-receive update post-receive post-update\n+\tdo\n+\t\twrite_script remote-server/hooks/$hook <<-EOF || return 1\n+\t\techo >&1 Hook $hook stdout\n+\t\techo >&2 Hook $hook stderr\n+\t\tEOF\n+\tdone &&\n+\n+\t# Trigger pre-receive update post-receive post-update\n+\tgit push origin-server HEAD:new-branch >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr pre-receive update post-receive post-update\n+'\n+\n+test_expect_success 'server push-to-checkout hook expects stdout redirected to stderr' '\n+\ttest_when_finished \"rm -f stdout.actual stderr.actual\" &&\n+\tgit init server &&\n+\tgit -C server checkout -b main &&\n+\ttest_config -C server receive.denyCurrentBranch updateInstead &&\n+\tgit remote add origin-server-2 server &&\n+\n+\twrite_script server/.git/hooks/push-to-checkout <<-EOF &&\n+\techo >&1 Hook push-to-checkout stdout\n+\techo >&2 Hook push-to-checkout stderr\n+\tEOF\n+\n+\t# Trigger push-to-checkout\n+\tgit push origin-server-2 HEAD:main >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr push-to-checkout\n+'\n+\n test_done\ndiff --git a/transport.c b/transport.c\nindex 6d0f02be5d..5aa39626da 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1373,6 +1373,15 @@ static int run_pre_push_hook(struct transport *transport,\n \topt.feed_pipe = pre_push_hook_feed_stdin;\n \topt.feed_pipe_cb_data = &data;\n \n+\t/*\n+\t * pre-push hooks expect stdout & stderr to be separate, so don't merge\n+\t * them to keep backwards compatibility with existing hooks.\n+\t * run_process_parallel(), called via run_hooks_opt() below, will buffer\n+\t * and merge the streams when output is grouped, so also set ungroup = 1.\n+\t */\n+\topt.stdout_to_stderr = 0;\n+\topt.ungroup = 1;\n+\n \tret = run_hooks_opt(the_repository, \"pre-push\", &opt);\n \n \tstrbuf_release(&data.buf);\n-- \n2.52.0.732.gb351b5166d.dirty\n\n"},{"id":"533791","messageId":"20260114031257.GA858646@coredump.intra.peff.net","threadId":"64791","inReplyTo":"20260113234528.1749921-1-adrian.ratiu@collabora.com","subject":"Re: [PATCH v2] hook: allow hooks to disable stdout_to_stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-14T03:12:57Z","receivedAt":"2026-01-14T03:12:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 14, 2026 at 01:45:28AM +0200, Adrian Ratiu wrote:\n\n> Changes in v2:\n> * Extended hook test coverage to detect future regressions (Junio, Patrick)\n> * Reworded commit message and added explanatory comment (Junio, Patrick)\n> * Set ungroup = 1 because grouping overrides stdout_to_stderr (Adrian)\n\nI have not really been following this topic, but I did read (and\nreproduce) Kristoffer's earlier report about reading stdin. The fix here\nwas not quite what I expected.\n\nIn particular...\n\n> @@ -93,6 +98,7 @@ struct run_hooks_opt\n> #define RUN_HOOKS_OPT_INIT { \\\n> \t.env = STRVEC_INIT, \\\n> \t.args = STRVEC_INIT, \\\n> +\t.stdout_to_stderr = 1, \\\n> }\n\n...I expected to see:\n\n .ungroup = 1, \\\n\nhere. The stdin issue goes back to 857f047e40 (hook: allow overriding\nthe ungroup option, 2025-12-26), where the \"ungroup\" field was added,\nand various code paths set it to \"1\" to match the previous behavior. But\nany paths that were missed, including run_pre_push_hook(), would see a\nchange of behavior (and in this case, a bug).\n\nMy reading of 857f047e40 is that it meant to give callers the _option_\nto switch the ungroup behavior, but not actually change anything. So\nwouldn't we want to leave the default as it was by initializing it to\n\"1\"?\n\n> @@ -1373,6 +1373,15 @@ static int run_pre_push_hook(struct transport *transport,\n> \topt.feed_pipe = pre_push_hook_feed_stdin;\n> \topt.feed_pipe_cb_data = &data;\n> \n> +\t/*\n> +\t * pre-push hooks expect stdout & stderr to be separate, so don't merge\n> +\t * them to keep backwards compatibility with existing hooks.\n> +\t * run_process_parallel(), called via run_hooks_opt() below, will buffer\n> +\t * and merge the streams when output is grouped, so also set ungroup = 1.\n> +\t */\n> +\topt.stdout_to_stderr = 0;\n> +\topt.ungroup = 1;\n\nThe other unexpected thing is that these two fixes are grouped at all.\nAFAICT, setting ungroup to 1 will fix Kristoffer's stdin problem without\nchanging stdout_to_stderr at all.\n\nBut I'm still not entirely sure I understand why the ungroup setting,\nwhich supposedly only affects stderr handling, causes the hook to fail\nto read stdin. Poking at it in a debugger and via strace, it looks like\nwe are in a poll loop while feeding stdin, even though we are not\nchecking whether the child can read! If we instrument like this:\n\ndiff --git a/transport.c b/transport.c\nindex 6d0f02be5d..7381450123 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1342,6 +1342,7 @@ static int pre_push_hook_feed_stdin(int hook_stdin_fd, void *pp_cb UNUSED, void\n \t\tbreak;\n \t}\n \n+\twarning(\"called pre_push_hook_feed_stdin for %s\", r->name);\n \tif (!r->peer_ref)\n \t\treturn 0;\n \n\nand then run the push from Kristoffer's recipe under strace, I see:\n\n poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout)\n write(2, \"warning: called pre_push_hook_feed_stdin for refs/tags/gitgui-0.6.3\\n\", 68) = 68\n poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout)\n write(2, \"warning: called pre_push_hook_feed_stdin for refs/tags/gitgui-0.6.4\\n\", 68) = 68\n poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout)\n write(2, \"warning: called pre_push_hook_feed_stdin for refs/tags/gitgui-0.6.5\\n\", 68) = 68\n poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout)\n write(2, \"warning: called pre_push_hook_feed_stdin for refs/tags/gitgui-0.7.0\\n\", 68) = 68\n poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout)\n write(2, \"warning: called pre_push_hook_feed_stdin for refs/tags/gitgui-0.7.0-rc1\\n\", 72) = 72\n poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout)\n\nSo we are hitting the poll timeout for each ref we consider, and it\ntakes forever to actually write the whole input stream. Which seems like\na bug in using feed_pipe without ungroup. Either:\n\n 1. We should write everything to the child as quickly as possible,\n assuming that we do not have to worry about reading back from it to\n avoid deadlock.\n\n 2. We should add the child's input pipes to our poll() call so that we\n can tell it is ready for more input (without hitting the timeout).\n\nSetting ungroup=1 saves us from this because it means that we'll skip\nthe poll() call entirely in pp_handle_child_IO(). So we end up\neffectively doing (1), which is OK because ungroup means we are not\nreading stdout or stderr from the child at all.\n\nBut it feels like this is papering over a bug, or at least providing a\ndangerous interface. AFAICT you _must_ set ungroup if you are going to\nuse the feed_pipe callback. And it does not really have anything to do\nwith the stdout_to_stderr flag at all.\n\nIt looks like feed_pipe feature is new-ish in your series. Maybe it\nshould just be a BUG() to use it without ungroup?\n\n-Peff\n"},{"id":"533793","messageId":"6746acf0-4538-41fc-8699-5acae6ec936e@app.fastmail.com","threadId":"64791","inReplyTo":"20260113234528.1749921-1-adrian.ratiu@collabora.com","subject":"Re: [PATCH v2] hook: allow hooks to disable stdout_to_stderr","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2026-01-14T06:13:43Z","receivedAt":"2026-01-14T06:14:06Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Wed, Jan 14, 2026, at 00:45, Adrian Ratiu wrote:\n> The last batch of hooks converted to the hook.[ch] API introduced\n> a regression because pick_next_hook() always sets stdout_to_stderr\n> for its child processes.\n>\n> Pre-push is the only hook API user which requires stdout_to_stderr\n> to be 0, so it can be argued that pre-push needs fixing, however\n> this will likely break many pre-push hooks, so it's better to allow\n> it to be 0, i.e. to match the previous behavior.\n>\n> To prevent such regressions in the future, extend the hook tests to\n> verify hooks write to the expected stdout vs stderr streams and\n> maintain backward compatibility with the hooks output assumptions.\n>\n> The tests are independent of the actual hook implementations: I've\n> tested they work the same before and after the hook.[ch] conversion\n> and will continue to work after we eventually introduce parallel\n> hook execution and config-based hooks.\n>\n> Reported-by: Chris Darroch \n> Suggested-by: brian m. carlson \n> Signed-off-by: Adrian Ratiu \n> ---\n> This is based on the latest master branch.\n>\n> Changes in v2:\n> * Extended hook test coverage to detect future regressions (Junio, Patrick)\n> * Reworded commit message and added explanatory comment (Junio, Patrick)\n> * Set ungroup = 1 because grouping overrides stdout_to_stderr (Adrian)\n>[snip]\n\nThis fixes the issue reported here: https://lore.kernel.org/git/249f08d1-4457-4a41-8dbe-9725c0c392de@app.fastmail.com/\n\n(Subject: [BUG] push: pre-push hook that waits for stdin is slow)\n\nVia: https://lore.kernel.org/git/87ecntqd9f.fsf@gentoo.mail-host-address-is-not-set/\n"},{"id":"533809","messageId":"878qe0zimo.fsf@gentoo.mail-host-address-is-not-set","threadId":"64791","inReplyTo":"20260114031257.GA858646@coredump.intra.peff.net","subject":"Re: [PATCH v2] hook: allow hooks to disable stdout_to_stderr","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-14T08:46:39Z","receivedAt":"2026-01-14T08:47:16Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Tue, 13 Jan 2026, Jeff King wrote:\n> On Wed, Jan 14, 2026 at 01:45:28AM +0200, Adrian Ratiu wrote:\n>\n>> Changes in v2:\n>> * Extended hook test coverage to detect future regressions (Junio, Patrick)\n>> * Reworded commit message and added explanatory comment (Junio, Patrick)\n>> * Set ungroup = 1 because grouping overrides stdout_to_stderr (Adrian)\n>\n> I have not really been following this topic, but I did read (and\n> reproduce) Kristoffer's earlier report about reading stdin. The fix here\n> was not quite what I expected.\n>\n> In particular...\n>\n>> @@ -93,6 +98,7 @@ struct run_hooks_opt\n>> #define RUN_HOOKS_OPT_INIT { \\\n>> \t.env = STRVEC_INIT, \\\n>> \t.args = STRVEC_INIT, \\\n>> +\t.stdout_to_stderr = 1, \\\n>> }\n>\n> ...I expected to see:\n>\n> .ungroup = 1, \\\n\nGood catch. I actually missed this in v2.\n\nI will drop ungroup from this patch in v3 and add another patch fixing\nKristoffer's issue (rationale below).\n\n>\n> here. The stdin issue goes back to 857f047e40 (hook: allow overriding\n> the ungroup option, 2025-12-26), where the \"ungroup\" field was added,\n> and various code paths set it to \"1\" to match the previous behavior. But\n> any paths that were missed, including run_pre_push_hook(), would see a\n> change of behavior (and in this case, a bug).\n>\n> My reading of 857f047e40 is that it meant to give callers the _option_\n> to switch the ungroup behavior, but not actually change anything. So\n> wouldn't we want to leave the default as it was by initializing it to\n> \"1\"?\n\nThat is correct: my mistake in v2 was assuming Kristoffer and Chris\nreported the same bug, when in fact there are 2 separate bugs requiring\nseparate fixes, so I will create 2 separate commits in v3 for each.\n\n>\n>> @@ -1373,6 +1373,15 @@ static int run_pre_push_hook(struct transport *transport,\n>> \topt.feed_pipe = pre_push_hook_feed_stdin;\n>> \topt.feed_pipe_cb_data = &data;\n>> \n>> +\t/*\n>> +\t * pre-push hooks expect stdout & stderr to be separate, so don't merge\n>> +\t * them to keep backwards compatibility with existing hooks.\n>> +\t * run_process_parallel(), called via run_hooks_opt() below, will buffer\n>> +\t * and merge the streams when output is grouped, so also set ungroup = 1.\n>> +\t */\n>> +\topt.stdout_to_stderr = 0;\n>> +\topt.ungroup = 1;\n>\n> The other unexpected thing is that these two fixes are grouped at all.\n> AFAICT, setting ungroup to 1 will fix Kristoffer's stdin problem without\n> changing stdout_to_stderr at all.\n>\n> But I'm still not entirely sure I understand why the ungroup setting,\n> which supposedly only affects stderr handling, causes the hook to fail\n> to read stdin. Poking at it in a debugger and via strace, it looks like\n> we are in a poll loop while feeding stdin, even though we are not\n> checking whether the child can read! If we instrument like this:\n>\n> diff --git a/transport.c b/transport.c\n> index 6d0f02be5d..7381450123 100644\n> --- a/transport.c\n> +++ b/transport.c\n> @@ -1342,6 +1342,7 @@ static int pre_push_hook_feed_stdin(int hook_stdin_fd, void *pp_cb UNUSED, void\n> \t\tbreak;\n> \t}\n> \n> +\twarning(\"called pre_push_hook_feed_stdin for %s\", r->name);\n> \tif (!r->peer_ref)\n> \t\treturn 0;\n> \n>\n> and then run the push from Kristoffer's recipe under strace, I see:\n>\n> poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout)\n> write(2, \"warning: called pre_push_hook_feed_stdin for refs/tags/gitgui-0.6.3\\n\", 68) = 68\n> poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout)\n> write(2, \"warning: called pre_push_hook_feed_stdin for refs/tags/gitgui-0.6.4\\n\", 68) = 68\n> poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout)\n> write(2, \"warning: called pre_push_hook_feed_stdin for refs/tags/gitgui-0.6.5\\n\", 68) = 68\n> poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout)\n> write(2, \"warning: called pre_push_hook_feed_stdin for refs/tags/gitgui-0.7.0\\n\", 68) = 68\n> poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout)\n> write(2, \"warning: called pre_push_hook_feed_stdin for refs/tags/gitgui-0.7.0-rc1\\n\", 72) = 72\n> poll([{fd=7, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout)\n>\n> So we are hitting the poll timeout for each ref we consider, and it\n> takes forever to actually write the whole input stream. Which seems like\n> a bug in using feed_pipe without ungroup. Either:\n>\n> 1. We should write everything to the child as quickly as possible,\n> assuming that we do not have to worry about reading back from it to\n> avoid deadlock.\n>\n> 2. We should add the child's input pipes to our poll() call so that we\n> can tell it is ready for more input (without hitting the timeout).\n>\n> Setting ungroup=1 saves us from this because it means that we'll skip\n> the poll() call entirely in pp_handle_child_IO(). So we end up\n> effectively doing (1), which is OK because ungroup means we are not\n> reading stdout or stderr from the child at all.\n>\n> But it feels like this is papering over a bug, or at least providing a\n> dangerous interface. AFAICT you _must_ set ungroup if you are going to\n> use the feed_pipe callback. And it does not really have anything to do\n> with the stdout_to_stderr flag at all.\n>\n> It looks like feed_pipe feature is new-ish in your series. Maybe it\n> should just be a BUG() to use it without ungroup?\n\nThis is all very useful and it proves there are 2 separate bugs here,\nrequiring two separate fixes for both Chris and Kristoffer.\n\nThe logic in v1 (without ungroup) is enough to fix Chris' issue with\nstdin and for Kristoffer I will do a smarter fix which implements your\n(1) suggestion: batch more than a single stdin fd write in each poll\ncall so we achieve comparable throughtput (no added poll latency).\n\nWe already do this for the receive hook in feed_receive_hook_cb(). In\nthis case we just need the callback to process more than just 1 ref at a\ntime.\n\nI will send v3 addressing your feedback, it is very much appreciated,\nAdrian\n"},{"id":"533810","messageId":"875x94zi0x.fsf@collabora.com","threadId":"64791","inReplyTo":"878qe0zimo.fsf@gentoo.mail-host-address-is-not-set","subject":"Re: [PATCH v2] hook: allow hooks to disable stdout_to_stderr","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-14T08:59:42Z","receivedAt":"2026-01-14T09:00:10Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Wed, 14 Jan 2026, Adrian Ratiu wrote:\n> On Tue, 13 Jan 2026, Jeff King wrote:\n>> On Wed, Jan 14, 2026 at 01:45:28AM +0200, Adrian Ratiu wrote:\n>>\n>>> Changes in v2:\n>>> * Extended hook test coverage to detect future regressions (Junio, Patrick)\n>>> * Reworded commit message and added explanatory comment (Junio, Patrick)\n>>> * Set ungroup = 1 because grouping overrides stdout_to_stderr (Adrian)\n>>\n>> I have not really been following this topic, but I did read (and\n>> reproduce) Kristoffer's earlier report about reading stdin. The fix here\n>> was not quite what I expected.\n>>\n>> In particular...\n>>\n>>> @@ -93,6 +98,7 @@ struct run_hooks_opt\n>>> #define RUN_HOOKS_OPT_INIT { \\\n>>> \t.env = STRVEC_INIT, \\\n>>> \t.args = STRVEC_INIT, \\\n>>> +\t.stdout_to_stderr = 1, \\\n>>> }\n>>\n>> ...I expected to see:\n>>\n>> .ungroup = 1, \\\n>\n> Good catch. I actually missed this in v2.\n>\n> I will drop ungroup from this patch in v3 and add another patch fixing\n> Kristoffer's issue (rationale below).\n>\n>>\n>> here. The stdin issue goes back to 857f047e40 (hook: allow overriding\n>> the ungroup option, 2025-12-26), where the \"ungroup\" field was added,\n>> and various code paths set it to \"1\" to match the previous behavior. But\n>> any paths that were missed, including run_pre_push_hook(), would see a\n>> change of behavior (and in this case, a bug).\n>>\n>> My reading of 857f047e40 is that it meant to give callers the _option_\n>> to switch the ungroup behavior, but not actually change anything. So\n>> wouldn't we want to leave the default as it was by initializing it to\n>> \"1\"?\n>\n> That is correct: my mistake in v2 was assuming Kristoffer and Chris\n> reported the same bug, when in fact there are 2 separate bugs requiring\n> separate fixes, so I will create 2 separate commits in v3 for each.\n\nMinor correction: I think we need 3 commits for 3 separate bugs we\nuncovered (ungroup should have its own commit). :)\n\nPlease wait for v3, I will code, test and send it ASAP.\n"},{"id":"533811","messageId":"0ab59443-0c46-481e-9e77-9cbc182e9920@app.fastmail.com","threadId":"64791","inReplyTo":"878qe0zimo.fsf@gentoo.mail-host-address-is-not-set","subject":"Re: [PATCH v2] hook: allow hooks to disable stdout_to_stderr","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2026-01-14T09:36:38Z","receivedAt":"2026-01-14T09:37:00Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Wed, Jan 14, 2026, at 09:46, Adrian Ratiu wrote:\n>[snip]\n> That is correct: my mistake in v2 was assuming Kristoffer and Chris\n> reported the same bug, when in fact there are 2 separate bugs requiring\n> separate fixes, so I will create 2 separate commits in v3 for each.\n\nYeah I tested your v1 before I sent the report and it didn’t work.\n\nThanks Adrian and Peff for explaining what is going on in the code. That\n`cat` would hang and time out was beyond my understanding. :)\n\n>[snip]\n"},{"id":"533844","messageId":"20260114170849.GB885771@coredump.intra.peff.net","threadId":"64791","inReplyTo":"878qe0zimo.fsf@gentoo.mail-host-address-is-not-set","subject":"Re: [PATCH v2] hook: allow hooks to disable stdout_to_stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-14T17:08:49Z","receivedAt":"2026-01-14T17:08:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 14, 2026 at 10:46:39AM +0200, Adrian Ratiu wrote:\n\n> > So we are hitting the poll timeout for each ref we consider, and it\n> > takes forever to actually write the whole input stream. Which seems like\n> > a bug in using feed_pipe without ungroup. Either:\n> >\n> > 1. We should write everything to the child as quickly as possible,\n> > assuming that we do not have to worry about reading back from it to\n> > avoid deadlock.\n> >\n> > 2. We should add the child's input pipes to our poll() call so that we\n> > can tell it is ready for more input (without hitting the timeout).\n> >\n> > Setting ungroup=1 saves us from this because it means that we'll skip\n> > the poll() call entirely in pp_handle_child_IO(). So we end up\n> > effectively doing (1), which is OK because ungroup means we are not\n> > reading stdout or stderr from the child at all.\n> >\n> > But it feels like this is papering over a bug, or at least providing a\n> > dangerous interface. AFAICT you _must_ set ungroup if you are going to\n> > use the feed_pipe callback. And it does not really have anything to do\n> > with the stdout_to_stderr flag at all.\n> >\n> > It looks like feed_pipe feature is new-ish in your series. Maybe it\n> > should just be a BUG() to use it without ungroup?\n> \n> This is all very useful and it proves there are 2 separate bugs here,\n> requiring two separate fixes for both Chris and Kristoffer.\n> \n> The logic in v1 (without ungroup) is enough to fix Chris' issue with\n> stdin and for Kristoffer I will do a smarter fix which implements your\n> (1) suggestion: batch more than a single stdin fd write in each poll\n> call so we achieve comparable throughtput (no added poll latency).\n> \n> We already do this for the receive hook in feed_receive_hook_cb(). In\n> this case we just need the callback to process more than just 1 ref at a\n> time.\n\nI looked at what feed_receive_hook_cb() is doing and...it's kind of\nhorrifying. It arbitrarily sends 500 lines, and then yields to the\ncaller to pump stderr (assuming ungroup=0). So:\n\n 1. It is assuming that 500 lines of input won't fill up the pipe\n buffer and block. Even if we compute the size of 500 lines we're\n sending, we don't know if the caller has cleared anything from the\n pipe in the last call. There might be zero bytes available!\n\n 2. After 500 lines we'll go back to the caller, which will then\n poll(). But if there's nothing to read on stderr, it will wait for\n the 100ms timeout. So if you have, say, 501 lines to send, then\n there will be a pointless 100ms pause in the middle.\n\nSo here's an example hook setup that will deadlock due to (1):\n\n-- >8 --\n#!/bin/sh\n\n# make two repos: one to push from, and one to push into\nrm -rf repo\ngit init repo\ncd repo\ngit init --bare dst.git\n\n# And here's our pre-receive hook that will cause problems.\ncat >dst.git/hooks/pre-receive <<\\EOF\n#!/bin/sh\n\n# Imagine we write a lot of output to stderr. For example, progress\n# reporting for some kind of setup procedure (but it could be anything).\n# The key thing is that it is enough to fill up the pipe buffer going\n# back to git.\nfor i in $(seq 10000); do printf \"\\rprocessing $i...\"; done\necho done\n\n# and now we are ready to read the input from the caller. A real hook\n# would do something useful with the input, but we'll just read it\n# and discard.\ncat >/dev/null\nEOF\nchmod +x dst.git/hooks/pre-receive\n\n# And now do a big push. 1000 ref updates seems to be enough to fill up\n# the pipe buffer (each one is 2 oids plus the ref name plus whitespace,\n# which is 100+ bytes each).\ngit commit --allow-empty -m foo\nseq --format='create refs/heads/branch-%g HEAD' 1000 |\ngit update-ref --stdin\ngit push -q --all dst.git\n-- >8 --\n\nThis will deadlock when run using the ar/run-command-hook topic. What\nhappens is this:\n\n 1. Git writes out the first 500 lines to the hook. This partially\n fills the pipe buffer going to the hook.\n\n 2. The hook writes to stderr, filling up the pipe buffer back to Git\n and blocking.\n\n 3. Git does its poll() and sees that there is data to read on stderr.\n It reads some of it (8k, I think, due to strbuf_read_once).\n\n 4. The hook sees more room in the pipe, so it writes another 8k. But\n it blocks again, still not having read any of its stdin.\n\n 5. Git, having done one round of poll(), goes back to trying to write\n to the hook's stdin, and tries for another 500 lines. But since the\n hook did not read anything from stdin, this fills up the pipe\n buffer.\n\n 6. Now we are deadlocked. Git is blocked trying to write to the hook's\n stdin, but the hook is blocked trying to write to stderr.\n\nTo solve this we must either:\n\n a. Make sure ungroup=1 is set, which means that Git does not read back\n stderr. In which case the 500-line batching is pointless. We can\n just write everything! But I assume you do not want to do this, as I'd\n guess the point of the series is that we want to buffer the stderr\n of each hook so that multiple hooks can be run in parallel without\n stomping on each other's output.\n\n b. Do a real poll() loop that checks both for incoming data on stderr\n from the hook, but also for the ability to write to the hook's\n stdin. Look at how pipe_command() and pump_io() do this, for\n example. You'd want something like that, but extended across\n multiple sub-processes running at once.\n\n-Peff\n\nPS If the goal of the series is to buffer stderr, that has another side\n effect: hooks can no longer produce real-time progress updates. Maybe\n losing that ability is a good tradeoff to keep the stderr output from\n multiple hooks from stomping on each other. But for a single hook,\n should we retain the existing behavior?\n"},{"id":"533845","messageId":"20260114171929.GC885771@coredump.intra.peff.net","threadId":"64791","inReplyTo":"20260114170849.GB885771@coredump.intra.peff.net","subject":"Re: [PATCH v2] hook: allow hooks to disable stdout_to_stderr","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-14T17:19:29Z","receivedAt":"2026-01-14T17:19:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 14, 2026 at 12:08:49PM -0500, Jeff King wrote:\n\n> I looked at what feed_receive_hook_cb() is doing and...it's kind of\n> horrifying. It arbitrarily sends 500 lines, and then yields to the\n> caller to pump stderr (assuming ungroup=0). So:\n> \n> 1. It is assuming that 500 lines of input won't fill up the pipe\n> buffer and block. Even if we compute the size of 500 lines we're\n> sending, we don't know if the caller has cleared anything from the\n> pipe in the last call. There might be zero bytes available!\n> \n> 2. After 500 lines we'll go back to the caller, which will then\n> poll(). But if there's nothing to read on stderr, it will wait for\n> the 100ms timeout. So if you have, say, 501 lines to send, then\n> there will be a pointless 100ms pause in the middle.\n> \n> So here's an example hook setup that will deadlock due to (1):\n\nAnd just for fun, here's an example that shows problem (2):\n\n-- >8 --\nrm -rf repo\ngit init repo\ncd repo\ngit commit --allow-empty -m foo\ngit init --bare dst.git\n\ncat >dst.git/hooks/pre-receive <<\\EOF\n#!/bin/sh\n# We don't even need to do anything interesting here! Git\n# will send us 500 lines, then block waiting for stderr which\n# we'll never send, and then send us another batch of 500.\ncat >/dev/null\nEOF\nchmod +x dst.git/hooks/pre-receive\n\n# Now do a moderate push of 500 branches.\nseq --format='create refs/heads/small-%g HEAD' 500 |\ngit update-ref --stdin\ntime git push -q dst.git refs/heads/small-*\n\n# And compare with one that sends just one more.\nseq --format='create refs/heads/large-%g HEAD' 501 |\ngit update-ref --stdin\ntime git push -q dst.git refs/heads/large-*\n-- >8 --\n\nThe second push always takes 100ms more! If we run the server side under\nstrace by replacing the final line with this:\n\n git push -q --receive-pack='strace -T git-receive-pack' dst.git refs/heads/large-*\n\nwe can see the stall here as we write to the hook:\n\n write(4, \"00000000000000000000000000000000\"..., 51393) = 51393 <0.000011>\n poll([{fd=5, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout) <0.100506>\n write(4, \"00000000000000000000000000000000\"..., 102) = 102 <0.000057>\n\nThat would likewise be solved by using ungroup=1 (in which case we do\nnot poll, but just call the feed function immediately again) or by using\na real poll() loop (which would see immediately that the hook is ready\nfor more input, rather than hitting the 100ms timeout).\n\n-Peff\n"},{"id":"533858","messageId":"87tswokri0.fsf@gentoo.mail-host-address-is-not-set","threadId":"64791","inReplyTo":"20260114171929.GC885771@coredump.intra.peff.net","subject":"Re: [PATCH v2] hook: allow hooks to disable stdout_to_stderr","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-14T17:56:23Z","receivedAt":"2026-01-14T17:56:56Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Wed, 14 Jan 2026, Jeff King wrote:\n> On Wed, Jan 14, 2026 at 12:08:49PM -0500, Jeff King wrote:\n>\n>> I looked at what feed_receive_hook_cb() is doing and...it's kind of\n>> horrifying. It arbitrarily sends 500 lines, and then yields to the\n>> caller to pump stderr (assuming ungroup=0). So:\n>> \n>> 1. It is assuming that 500 lines of input won't fill up the pipe\n>> buffer and block. Even if we compute the size of 500 lines we're\n>> sending, we don't know if the caller has cleared anything from the\n>> pipe in the last call. There might be zero bytes available!\n>> \n>> 2. After 500 lines we'll go back to the caller, which will then\n>> poll(). But if there's nothing to read on stderr, it will wait for\n>> the 100ms timeout. So if you have, say, 501 lines to send, then\n>> there will be a pointless 100ms pause in the middle.\n>> \n>> So here's an example hook setup that will deadlock due to (1):\n>\n> And just for fun, here's an example that shows problem (2):\n>\n> -- >8 --\n> rm -rf repo\n> git init repo\n> cd repo\n> git commit --allow-empty -m foo\n> git init --bare dst.git\n>\n> cat >dst.git/hooks/pre-receive <<\\EOF\n> #!/bin/sh\n> # We don't even need to do anything interesting here! Git\n> # will send us 500 lines, then block waiting for stderr which\n> # we'll never send, and then send us another batch of 500.\n> cat >/dev/null\n> EOF\n> chmod +x dst.git/hooks/pre-receive\n>\n> # Now do a moderate push of 500 branches.\n> seq --format='create refs/heads/small-%g HEAD' 500 |\n> git update-ref --stdin\n> time git push -q dst.git refs/heads/small-*\n>\n> # And compare with one that sends just one more.\n> seq --format='create refs/heads/large-%g HEAD' 501 |\n> git update-ref --stdin\n> time git push -q dst.git refs/heads/large-*\n> -- >8 --\n>\n> The second push always takes 100ms more! If we run the server side under\n> strace by replacing the final line with this:\n>\n> git push -q --receive-pack='strace -T git-receive-pack' dst.git refs/heads/large-*\n>\n> we can see the stall here as we write to the hook:\n>\n> write(4, \"00000000000000000000000000000000\"..., 51393) = 51393 <0.000011>\n> poll([{fd=5, events=POLLIN|POLLHUP}], 1, 100) = 0 (Timeout) <0.100506>\n> write(4, \"00000000000000000000000000000000\"..., 102) = 102 <0.000057>\n>\n> That would likewise be solved by using ungroup=1 (in which case we do\n> not poll, but just call the feed function immediately again) or by using\n> a real poll() loop (which would see immediately that the hook is ready\n> for more input, rather than hitting the 100ms timeout).\n\nThanks for the detailed examples.\n\nFor the server-side hooks, I think the way forward is to implement the\npoll loop as you suggested so we can buffer stderr and for a single\n(non-parallel) hook, we can keep the existing behavior (still need to\ntest this). I'll do that in a separate patch and drop the batching.\n\nFor the client side hooks, I'll send v3 of this series which fixes the\ntwo regressions reported by Chris and Kristoffer.\n"},{"id":"533863","messageId":"20260114185731.2381550-1-adrian.ratiu@collabora.com","threadId":"64791","inReplyTo":"20260113115633.230479-1-adrian.ratiu@collabora.com","subject":"[PATCH v3 0/2] Fix two hook conversion regressions","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-14T18:57:29Z","receivedAt":"2026-01-14T18:58:55Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"Hello everyone,\n\nThis series fixes 2 regressions reported by Chris and Kristoffer,\nintroduced by the 'ar/run-command-hook' merge into master.\n\nBased on a discussion with Peff on v2, I do plan to revisit and\nrework the server-side hook I/O polling & batching logic, however\nthat will be a separate patch unrelated to these two regressions.\n\nMany thanks to everyone who helped debug & fix these!\n\nThis series is based on the master branch.\n\nPushed to GitHub: https://github.com/10ne1/git/tree/dev/aratiu/make-hook-stdout_to_stderr-optional-v3\nSuccessful CI run: https://github.com/10ne1/git/actions/runs/21004980299\n\nChanges in v3:\n* New commit to make hook opts.ungroup = 1 default (Peff)\n* Dropped the ungroup fix from the first commit because it's now\n handled by the more comprehensive second commit (Peff, Adrian)\n* Added fixes tags to commits (Adrian)\n\nRange-diff between v2 -> v3:\n1: 898a21ddd0 ! 1: 77db7035c5 hook: allow hooks to disable stdout_to_stderr\n @@ Commit message\n and will continue to work after we eventually introduce parallel\n hook execution and config-based hooks.\n \n + Fixes: 3e2836a742d8 (\"transport: convert pre-push to hook API\")\n Reported-by: Chris Darroch \n Suggested-by: brian m. carlson \n Signed-off-by: Adrian Ratiu \n @@ transport.c: static int run_pre_push_hook(struct transport *transport,\n +\t/*\n +\t * pre-push hooks expect stdout & stderr to be separate, so don't merge\n +\t * them to keep backwards compatibility with existing hooks.\n -+\t * run_process_parallel(), called via run_hooks_opt() below, will buffer\n -+\t * and merge the streams when output is grouped, so also set ungroup = 1.\n +\t */\n +\topt.stdout_to_stderr = 0;\n -+\topt.ungroup = 1;\n +\n \tret = run_hooks_opt(the_repository, \"pre-push\", &opt);\n \n-: ---------- > 2: de3001f063 hook: make ungroup opt-out instead of opt-in\n\nAdrian Ratiu (2):\n hook: allow hooks to disable stdout_to_stderr\n hook: make ungroup opt-out instead of opt-in\n\n builtin/hook.c | 6 --\n builtin/receive-pack.c | 12 ++-\n commit.c | 3 -\n hook.c | 5 +-\n hook.h | 7 ++\n t/t1800-hook.sh | 176 +++++++++++++++++++++++++++++++++++++++++\n transport.c | 6 ++\n 7 files changed, 199 insertions(+), 16 deletions(-)\n\n-- \n2.52.0.732.gb351b5166d.dirty\n\n"},{"id":"533864","messageId":"20260114185731.2381550-3-adrian.ratiu@collabora.com","threadId":"64791","inReplyTo":"20260114185731.2381550-1-adrian.ratiu@collabora.com","subject":"[PATCH v3 2/2] hook: make ungroup opt-out instead of opt-in","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-14T18:57:31Z","receivedAt":"2026-01-14T18:58:57Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"In 857f047e40 (hook: allow overriding the ungroup option, 2025-12-26),\nI accidentally made the ungroup option opt-in instead of opt-out and\ndespite my best efforts to set it for all API users, I missed a case\nwhich requires it to be set: the pre-push hook which regressed.\n\nThe only thing I needed in that commit was a way to change the default,\nto convert the remaining receive-pack hooks which require ungroup == 0\nfor sideband output, so it doesn't matter if it's on or off by default.\n\nBring back the original behavior by setting it for all hooks in the\nstruct run_hooks_opt initializer, which nicely allows changing the\ndefault value only where needed, in receive-pack.c.\n\nWhile at it add a few hook tests which exercise receive-pack sideband\noutput since they are the only ungroup=0 exceptions and there are no\nother tests exercising this functionality.\n\nFixes: 857f047e40f7 (\"hook: allow overriding the ungroup option\")\nReported-by: Kristoffer Haugsbakk \nSuggested-by: Jeff King \nSigned-off-by: Adrian Ratiu \n---\n builtin/hook.c | 6 ------\n builtin/receive-pack.c | 12 ++++++++---\n commit.c | 3 ---\n hook.c | 3 ---\n hook.h | 1 +\n t/t1800-hook.sh | 49 ++++++++++++++++++++++++++++++++++++++++++\n 6 files changed, 59 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/hook.c b/builtin/hook.c\nindex 73e7b8c2e8..7afec380d2 100644\n--- a/builtin/hook.c\n+++ b/builtin/hook.c\n@@ -43,12 +43,6 @@ static int run(int argc, const char **argv, const char *prefix,\n \tif (!argc)\n \t\tgoto usage;\n \n-\t/*\n-\t * All current \"hook run\" use-cases require ungrouped child output.\n-\t * If this changes, a hook run argument can be added to toggle it.\n-\t */\n-\topt.ungroup = 1;\n-\n \t/*\n \t * Having a -- for \"run\" when providing is\n \t * mandatory.\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex ef1f77be8c..2d1a94f3a9 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -905,8 +905,10 @@ static int run_receive_hook(struct command *commands,\n \tprepare_push_cert_sha1(&opt);\n \n \t/* set up sideband printer */\n-\tif (use_sideband)\n+\tif (use_sideband) {\n \t\topt.consume_output = hook_output_to_sideband;\n+\t\topt.ungroup = 0; /* mandatory for sideband output */\n+\t}\n \n \t/* set up stdin callback */\n \tfeed_state.cmd = commands;\n@@ -933,8 +935,10 @@ static int run_update_hook(struct command *cmd)\n \t\t oid_to_hex(&cmd->new_oid),\n \t\t NULL);\n \n-\tif (use_sideband)\n+\tif (use_sideband) {\n \t\topt.consume_output = hook_output_to_sideband;\n+\t\topt.ungroup = 0; /* mandatory for sideband output */\n+\t}\n \n \treturn run_hooks_opt(the_repository, \"update\", &opt);\n }\n@@ -1623,8 +1627,10 @@ static void run_update_post_hook(struct command *commands)\n \tif (!opt.args.nr)\n \t\treturn;\n \n-\tif (use_sideband)\n+\tif (use_sideband) {\n \t\topt.consume_output = hook_output_to_sideband;\n+\t\topt.ungroup = 0; /* mandatory for sideband output */\n+\t}\n \n \trun_hooks_opt(the_repository, \"post-update\", &opt);\n }\ndiff --git a/commit.c b/commit.c\nindex efd0c02683..28bb5ce029 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1978,9 +1978,6 @@ int run_commit_hook(int editor_is_used, const char *index_file,\n \t\tstrvec_push(&opt.args, arg);\n \tva_end(args);\n \n-\t/* All commit hook use-cases require ungrouping child output. */\n-\topt.ungroup = 1;\n-\n \topt.invoked_hook = invoked_hook;\n \treturn run_hooks_opt(the_repository, name, &opt);\n }\ndiff --git a/hook.c b/hook.c\nindex ebd9d9e26e..4e7631132a 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -199,9 +199,6 @@ int run_hooks(struct repository *r, const char *hook_name)\n {\n \tstruct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;\n \n-\t/* All use-cases of this API require ungrouping. */\n-\topt.ungroup = 1;\n-\n \treturn run_hooks_opt(r, hook_name, &opt);\n }\n \ndiff --git a/hook.h b/hook.h\nindex 2488db7133..b2b4db2b3d 100644\n--- a/hook.h\n+++ b/hook.h\n@@ -99,6 +99,7 @@ struct run_hooks_opt\n \t.env = STRVEC_INIT, \\\n \t.args = STRVEC_INIT, \\\n \t.stdout_to_stderr = 1, \\\n+\t.ungroup = 1, \\\n }\n \n struct hook_cb_data {\ndiff --git a/t/t1800-hook.sh b/t/t1800-hook.sh\nindex 0e4f93fb31..0555a1fd42 100755\n--- a/t/t1800-hook.sh\n+++ b/t/t1800-hook.sh\n@@ -311,4 +311,53 @@ test_expect_success 'server push-to-checkout hook expects stdout redirected to s\n \tcheck_stdout_merged_to_stderr push-to-checkout\n '\n \n+test_expect_success 'receive-pack hooks sideband output works' '\n+\tgit init --bare target-sideband.git &&\n+\ttest_commit sideband-a &&\n+\tgit remote add origin-sideband ./target-sideband.git &&\n+\n+\t# pre-receive hook\n+\ttest_hook -C target-sideband.git pre-receive <<-\\EOF &&\n+\techo \"stdout pre-receive\"\n+\techo \"stderr pre-receive\" >&2\n+\tEOF\n+\n+\tgit push origin-sideband HEAD:refs/heads/pre-receive 2>actual &&\n+\ttest_grep \"remote: stdout pre-receive\" actual &&\n+\ttest_grep \"remote: stderr pre-receive\" actual &&\n+\n+\t# update hook\n+\ttest_hook -C target-sideband.git update <<-\\EOF &&\n+\techo \"stdout update\"\n+\techo \"stderr update\" >&2\n+\tEOF\n+\n+\ttest_commit sideband-b &&\n+\tgit push origin-sideband HEAD:refs/heads/update 2>actual &&\n+\ttest_grep \"remote: stdout update\" actual &&\n+\ttest_grep \"remote: stderr update\" actual &&\n+\n+\t# post-receive hook\n+\ttest_hook -C target-sideband.git post-receive <<-\\EOF &&\n+\techo >&1 \"stdout post-receive\"\n+\techo >&2 \"stderr post-receive\"\n+\tEOF\n+\n+\ttest_commit sideband-c &&\n+\tgit push origin-sideband HEAD:refs/heads/post-receive 2>actual &&\n+\ttest_grep \"remote: stdout post-receive\" actual &&\n+\ttest_grep \"remote: stderr post-receive\" actual &&\n+\n+\t# post-update hook\n+\ttest_hook -C target-sideband.git post-update <<-\\EOF &&\n+\techo >&1 \"stdout post-update\"\n+\techo >&2 \"stderr post-update\"\n+\tEOF\n+\n+\ttest_commit sideband-d &&\n+\tgit push origin-sideband HEAD:refs/heads/post-update 2>actual &&\n+\ttest_grep \"remote: stdout post-update\" actual &&\n+\ttest_grep \"remote: stderr post-update\" actual\n+'\n+\n test_done\n-- \n2.52.0.732.gb351b5166d.dirty\n\n"},{"id":"533865","messageId":"20260114185731.2381550-2-adrian.ratiu@collabora.com","threadId":"64791","inReplyTo":"20260114185731.2381550-1-adrian.ratiu@collabora.com","subject":"[PATCH v3 1/2] hook: allow hooks to disable stdout_to_stderr","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-14T18:57:30Z","receivedAt":"2026-01-14T18:59:01Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"The last batch of hooks converted to the hook.[ch] API introduced\na regression because pick_next_hook() always sets stdout_to_stderr\nfor its child processes.\n\nPre-push is the only hook API user which requires stdout_to_stderr\nto be 0, so it can be argued that pre-push needs fixing, however\nthis will likely break many pre-push hooks, so it's better to allow\nit to be 0, i.e. to match the previous behavior.\n\nTo prevent such regressions in the future, extend the hook tests to\nverify hooks write to the expected stdout vs stderr streams and\nmaintain backward compatibility with the hooks output assumptions.\n\nThe tests are independent of the actual hook implementations: I've\ntested they work the same before and after the hook.[ch] conversion\nand will continue to work after we eventually introduce parallel\nhook execution and config-based hooks.\n\nFixes: 3e2836a742d8 (\"transport: convert pre-push to hook API\")\nReported-by: Chris Darroch \nSuggested-by: brian m. carlson \nSigned-off-by: Adrian Ratiu \n---\n hook.c | 2 +-\n hook.h | 6 +++\n t/t1800-hook.sh | 127 ++++++++++++++++++++++++++++++++++++++++++++++++\n transport.c | 6 +++\n 4 files changed, 140 insertions(+), 1 deletion(-)\n\ndiff --git a/hook.c b/hook.c\nindex 35211e5ed7..ebd9d9e26e 100644\n--- a/hook.c\n+++ b/hook.c\n@@ -81,7 +81,7 @@ static int pick_next_hook(struct child_process *cp,\n \t\tcp->in = -1;\n \t}\n \n-\tcp->stdout_to_stderr = 1;\n+\tcp->stdout_to_stderr = hook_cb->options->stdout_to_stderr;\n \tcp->trace2_hook_name = hook_cb->hook_name;\n \tcp->dir = hook_cb->options->dir;\n \ndiff --git a/hook.h b/hook.h\nindex ae502178b9..2488db7133 100644\n--- a/hook.h\n+++ b/hook.h\n@@ -39,6 +39,11 @@ struct run_hooks_opt\n \t */\n \tunsigned int ungroup:1;\n \n+\t/**\n+\t * Send the hook's stdout to stderr.\n+\t */\n+\tunsigned int stdout_to_stderr:1;\n+\n \t/**\n \t * Path to file which should be piped to stdin for each hook.\n \t */\n@@ -93,6 +98,7 @@ struct run_hooks_opt\n #define RUN_HOOKS_OPT_INIT { \\\n \t.env = STRVEC_INIT, \\\n \t.args = STRVEC_INIT, \\\n+\t.stdout_to_stderr = 1, \\\n }\n \n struct hook_cb_data {\ndiff --git a/t/t1800-hook.sh b/t/t1800-hook.sh\nindex 4feaf0d7be..0e4f93fb31 100755\n--- a/t/t1800-hook.sh\n+++ b/t/t1800-hook.sh\n@@ -184,4 +184,131 @@ test_expect_success 'stdin to hooks' '\n \ttest_cmp expect actual\n '\n \n+check_stdout_separate_from_stderr () {\n+\tfor hook in \"$@\"\n+\tdo\n+\t\ttest_grep ! \"Hook $hook stdout\" stderr.actual &&\n+\t\ttest_grep ! \"Hook $hook stderr\" stdout.actual &&\n+\t\ttest_grep \"Hook $hook stderr\" stderr.actual &&\n+\t\ttest_grep \"Hook $hook stdout\" stdout.actual || return 1\n+\tdone\n+}\n+\n+check_stdout_merged_to_stderr () {\n+\ttest_grep ! \"Hook .* stdout\" stdout.actual &&\n+\ttest_grep ! \"Hook .* stderr\" stdout.actual &&\n+\tfor hook in \"$@\"\n+\tdo\n+\t\ttest_grep \"Hook $hook stdout\" stderr.actual &&\n+\t\ttest_grep \"Hook $hook stderr\" stderr.actual || return 1\n+\tdone\n+}\n+\n+test_expect_success 'client pre-push hook expects separate stdout and stderr' '\n+\ttest_when_finished \"rm -f stdout.actual stderr.actual\" &&\n+\tgit init --bare remote &&\n+\tgit remote add origin remote &&\n+\ttest_commit A &&\n+\n+\thook=pre-push &&\n+\ttest_hook $hook <<-EOF &&\n+\techo >&1 Hook $hook stdout\n+\techo >&2 Hook $hook stderr\n+\tEOF\n+\n+\tgit push origin HEAD:main >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_separate_from_stderr pre-push\n+'\n+\n+test_expect_success 'client hooks expect stdout redirected to stderr' '\n+\ttest_when_finished \"rm -f stdout.actual stderr.actual\" &&\n+\tfor hook in pre-commit post-commit post-checkout pre-merge-commit \\\n+\t\tprepare-commit-msg commit-msg post-merge post-rewrite reference-transaction \\\n+\t\tapplypatch-msg pre-applypatch post-applypatch pre-rebase post-index-change\n+\tdo\n+\t\ttest_hook $hook <<-EOF || return 1\n+\t\techo >&1 Hook $hook stdout\n+\t\techo >&2 Hook $hook stderr\n+\t\tEOF\n+\tdone &&\n+\n+\tgit checkout -B main &&\n+\tgit checkout -b branch-a &&\n+\ttest_commit commit-on-branch-a &&\n+\n+\t# Trigger pre-commit, prepare-commit-msg, commit-msg, post-commit, reference-transaction\n+\tgit commit --allow-empty -m \"Test\" >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr pre-commit prepare-commit-msg commit-msg post-commit reference-transaction &&\n+\n+\t# Trigger post-checkout, reference-transaction\n+\tgit checkout -b new-branch main >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr post-checkout reference-transaction &&\n+\n+\t# Trigger pre-merge-commit, post-merge, reference-transaction\n+\ttest_commit new-branch-commit &&\n+\tgit merge --no-ff branch-a >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr pre-merge-commit post-merge reference-transaction &&\n+\n+\t# Trigger post-rewrite, reference-transaction\n+\tgit commit --amend --allow-empty --no-edit >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr post-rewrite reference-transaction &&\n+\n+\t# Trigger applypatch-msg, pre-applypatch, post-applypatch\n+\tgit checkout -b branch-b main &&\n+\ttest_commit branch-b &&\n+\tgit format-patch -1 --stdout >patch &&\n+\tgit checkout -b branch-c main &&\n+\tgit am patch >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr applypatch-msg pre-applypatch post-applypatch &&\n+\n+\t# Trigger pre-rebase\n+\tgit checkout -b branch-d main &&\n+\ttest_commit branch-d &&\n+\tgit checkout main &&\n+\ttest_commit diverge-main &&\n+\tgit checkout branch-d &&\n+\tgit rebase main >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr pre-rebase &&\n+\n+\t# Trigger post-index-change\n+\toid=$(git hash-object -w --stdin stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr post-index-change\n+'\n+\n+test_expect_success 'server hooks expect stdout redirected to stderr' '\n+\ttest_when_finished \"rm -f stdout.actual stderr.actual\" &&\n+\tgit init --bare remote-server &&\n+\tgit remote add origin-server remote-server &&\n+\n+\tfor hook in pre-receive update post-receive post-update\n+\tdo\n+\t\twrite_script remote-server/hooks/$hook <<-EOF || return 1\n+\t\techo >&1 Hook $hook stdout\n+\t\techo >&2 Hook $hook stderr\n+\t\tEOF\n+\tdone &&\n+\n+\t# Trigger pre-receive update post-receive post-update\n+\tgit push origin-server HEAD:new-branch >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr pre-receive update post-receive post-update\n+'\n+\n+test_expect_success 'server push-to-checkout hook expects stdout redirected to stderr' '\n+\ttest_when_finished \"rm -f stdout.actual stderr.actual\" &&\n+\tgit init server &&\n+\tgit -C server checkout -b main &&\n+\ttest_config -C server receive.denyCurrentBranch updateInstead &&\n+\tgit remote add origin-server-2 server &&\n+\n+\twrite_script server/.git/hooks/push-to-checkout <<-EOF &&\n+\techo >&1 Hook push-to-checkout stdout\n+\techo >&2 Hook push-to-checkout stderr\n+\tEOF\n+\n+\t# Trigger push-to-checkout\n+\tgit push origin-server-2 HEAD:main >stdout.actual 2>stderr.actual &&\n+\tcheck_stdout_merged_to_stderr push-to-checkout\n+'\n+\n test_done\ndiff --git a/transport.c b/transport.c\nindex 6d0f02be5d..e876cc9189 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1373,6 +1373,12 @@ static int run_pre_push_hook(struct transport *transport,\n \topt.feed_pipe = pre_push_hook_feed_stdin;\n \topt.feed_pipe_cb_data = &data;\n \n+\t/*\n+\t * pre-push hooks expect stdout & stderr to be separate, so don't merge\n+\t * them to keep backwards compatibility with existing hooks.\n+\t */\n+\topt.stdout_to_stderr = 0;\n+\n \tret = run_hooks_opt(the_repository, \"pre-push\", &opt);\n \n \tstrbuf_release(&data.buf);\n-- \n2.52.0.732.gb351b5166d.dirty\n\n"},{"id":"533894","messageId":"20260114212718.GB1010080@coredump.intra.peff.net","threadId":"64791","inReplyTo":"20260114185731.2381550-3-adrian.ratiu@collabora.com","subject":"Re: [PATCH v3 2/2] hook: make ungroup opt-out instead of opt-in","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-01-14T21:27:18Z","receivedAt":"2026-01-14T21:27:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 14, 2026 at 08:57:31PM +0200, Adrian Ratiu wrote:\n\n> In 857f047e40 (hook: allow overriding the ungroup option, 2025-12-26),\n> I accidentally made the ungroup option opt-in instead of opt-out and\n> despite my best efforts to set it for all API users, I missed a case\n> which requires it to be set: the pre-push hook which regressed.\n> \n> The only thing I needed in that commit was a way to change the default,\n> to convert the remaining receive-pack hooks which require ungroup == 0\n> for sideband output, so it doesn't matter if it's on or off by default.\n> \n> Bring back the original behavior by setting it for all hooks in the\n> struct run_hooks_opt initializer, which nicely allows changing the\n> default value only where needed, in receive-pack.c.\n\nI think this is an improvement overall to what's currently in 'seen',\nand the patch looks as I'd expect.\n\nI have doubts in general about the approach taken by c65f26fca4\n(receive-pack: convert receive hooks to hook API, 2025-12-26). We used\nto use an async muxer thread, and now we are buffering hook stderr,\nwhich to my mind is a regression (both in terms of real-time output, but\nalso the deadlock issues mentioned earlier).\n\nI'd rather see us continue to set up a muxer thread, and then direct the\nhook API to attach the stderr of the hook processes to that descriptor.\nThen receive-pack would just work as before, without having to fiddle\nwith the ungroup flag at all.\n\nYou can take that with the appropriate size grain of salt from an\nobserver who has not been following the series (and is not really\ninterested in it, beyond making sure we do not introduce regressions).\nBut it is also an observer who has dealt with many I/O deadlocks in Git. ;)\n\n-Peff\n"},{"id":"533910","messageId":"87qzrrlspa.fsf@collabora.com","threadId":"64791","inReplyTo":"20260114212718.GB1010080@coredump.intra.peff.net","subject":"Re: [PATCH v3 2/2] hook: make ungroup opt-out instead of opt-in","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-14T22:45:05Z","receivedAt":"2026-01-14T22:45:28Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Wed, 14 Jan 2026, Jeff King wrote:\n> On Wed, Jan 14, 2026 at 08:57:31PM +0200, Adrian Ratiu wrote:\n>\n>> In 857f047e40 (hook: allow overriding the ungroup option, 2025-12-26),\n>> I accidentally made the ungroup option opt-in instead of opt-out and\n>> despite my best efforts to set it for all API users, I missed a case\n>> which requires it to be set: the pre-push hook which regressed.\n>> \n>> The only thing I needed in that commit was a way to change the default,\n>> to convert the remaining receive-pack hooks which require ungroup == 0\n>> for sideband output, so it doesn't matter if it's on or off by default.\n>> \n>> Bring back the original behavior by setting it for all hooks in the\n>> struct run_hooks_opt initializer, which nicely allows changing the\n>> default value only where needed, in receive-pack.c.\n>\n> I think this is an improvement overall to what's currently in 'seen',\n> and the patch looks as I'd expect.\n\nThanks. :)\n\nMy intention for this series is to fix the two regressions reported by\nChris and Kristoffer ASAP and not touch receive-pack (yet!) because it's\na separate topic which deserves its own patch & review / discussion.\n\n>\n> I have doubts in general about the approach taken by c65f26fca4\n> (receive-pack: convert receive hooks to hook API, 2025-12-26). We used\n> to use an async muxer thread, and now we are buffering hook stderr,\n> which to my mind is a regression (both in terms of real-time output, but\n> also the deadlock issues mentioned earlier).\n>\n> I'd rather see us continue to set up a muxer thread, and then direct the\n> hook API to attach the stderr of the hook processes to that descriptor.\n> Then receive-pack would just work as before, without having to fiddle\n> with the ungroup flag at all.\n\nI am certainly open to try this other design, especially if we can\neliminate the risk of deadlocks. I will code something along these\nlines then send it for you to review.\n\n>\n> You can take that with the appropriate size grain of salt from an\n> observer who has not been following the series (and is not really\n> interested in it, beyond making sure we do not introduce regressions).\n> But it is also an observer who has dealt with many I/O deadlocks in Git. ;)\n\nNo worries, all feedback is welcome. \n\nI genuinely appreciate your ideas and suggestions. :)\n\nExpect a receive-pack patch from me soon, in a separate topic.\n"},{"id":"533959","messageId":"xmqqpl7bc68b.fsf@gitster.g","threadId":"64791","inReplyTo":"20260114185731.2381550-1-adrian.ratiu@collabora.com","subject":"Re: [PATCH v3 0/2] Fix two hook conversion regressions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-15T14:15:16Z","receivedAt":"2026-01-15T14:15:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adrian Ratiu writes:\n\n> Hello everyone,\n>\n> This series fixes 2 regressions reported by Chris and Kristoffer,\n> introduced by the 'ar/run-command-hook' merge into master.\n>\n> Based on a discussion with Peff on v2, I do plan to revisit and\n> rework the server-side hook I/O polling & batching logic, however\n> that will be a separate patch unrelated to these two regressions.\n\nI've read these two over once again, and am inclined to say that we\nshould merge these in upcoming 2.53 release. Opinions?\n\nThanks.\n"},{"id":"533973","messageId":"87o6mulrnq.fsf@collabora.com","threadId":"64791","inReplyTo":"xmqqpl7bc68b.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] Fix two hook conversion regressions","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-15T17:19:53Z","receivedAt":"2026-01-15T17:20:16Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Thu, 15 Jan 2026, Junio C Hamano wrote:\n> Adrian Ratiu writes:\n>\n>> Hello everyone,\n>>\n>> This series fixes 2 regressions reported by Chris and Kristoffer,\n>> introduced by the 'ar/run-command-hook' merge into master.\n>>\n>> Based on a discussion with Peff on v2, I do plan to revisit and\n>> rework the server-side hook I/O polling & batching logic, however\n>> that will be a separate patch unrelated to these two regressions.\n>\n> I've read these two over once again, and am inclined to say that we\n> should merge these in upcoming 2.53 release. Opinions?\n\nI agree with this.\n\nWe can't let these two regressions enter a release, so we have two\nreal chices:\n\n1. Merge both fixes to 1.53 or\n2. Revert the 'ar/run-command-hook' topic merge.\n\nThe only remaining known open issue is the potential deadlocks in\nserver-side hooks highlighted by Peff, however that is less severe than\nthese two (I'd actually be surprised if anyone hits in practice without\na well crafted use case, having access to those hooks).\n\nSo I'm inclined for option 1, to land the fixes.\n\n(OFC I'm working on the deadlock issue in parallel, just addressed the\nuser bug reports first).\n\nThanks,\nAdrian\n"},{"id":"533974","messageId":"xmqq4iomdbn0.fsf@gitster.g","threadId":"64791","inReplyTo":"87o6mulrnq.fsf@collabora.com","subject":"Re: [PATCH v3 0/2] Fix two hook conversion regressions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-15T17:33:07Z","receivedAt":"2026-01-15T17:33:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adrian Ratiu writes:\n\n> I agree with this.\n>\n> We can't let these two regressions enter a release, so we have two\n> real chices:\n>\n> 1. Merge both fixes to 1.53 or\n> 2. Revert the 'ar/run-command-hook' topic merge.\n\nHmph, at this early point in the late release cycle before -rc1\n(yes, rc0 is scheduled for this morning, but that is not really a\nrelease candidate that counts as anything), it is tempting to take\n#2, actually. I just do not know how much damage such a revert\nwould cause to the tree. I'll experiment after I finish cutting the\n-rc0 preview release.\n"},{"id":"533976","messageId":"87ldhylq4e.fsf@collabora.com","threadId":"64791","inReplyTo":"xmqq4iomdbn0.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] Fix two hook conversion regressions","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-15T17:53:05Z","receivedAt":"2026-01-15T17:53:25Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Thu, 15 Jan 2026, Junio C Hamano wrote:\n> Adrian Ratiu writes:\n>\n>> I agree with this.\n>>\n>> We can't let these two regressions enter a release, so we have two\n>> real chices:\n>>\n>> 1. Merge both fixes to 1.53 or\n>> 2. Revert the 'ar/run-command-hook' topic merge.\n>\n> Hmph, at this early point in the late release cycle before -rc1\n> (yes, rc0 is scheduled for this morning, but that is not really a\n> release candidate that counts as anything), it is tempting to take\n> #2, actually. I just do not know how much damage such a revert\n> would cause to the tree. I'll experiment after I finish cutting the\n> -rc0 preview release.\n\nI do not expect any conflicts and, if there any, they should be trivial.\n\nLet me know if you need any help.\n"},{"id":"533983","messageId":"xmqqfr86bp0n.fsf@gitster.g","threadId":"64791","inReplyTo":"87ldhylq4e.fsf@collabora.com","subject":"Re: [PATCH v3 0/2] Fix two hook conversion regressions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-15T20:27:04Z","receivedAt":"2026-01-15T20:27:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adrian Ratiu writes:\n\n> On Thu, 15 Jan 2026, Junio C Hamano wrote:\n>> Adrian Ratiu writes:\n>>\n>>> I agree with this.\n>>>\n>>> We can't let these two regressions enter a release, so we have two\n>>> real chices:\n>>>\n>>> 1. Merge both fixes to 1.53 or\n>>> 2. Revert the 'ar/run-command-hook' topic merge.\n>>\n>> Hmph, at this early point in the late release cycle before -rc1\n>> (yes, rc0 is scheduled for this morning, but that is not really a\n>> release candidate that counts as anything), it is tempting to take\n>> #2, actually. I just do not know how much damage such a revert\n>> would cause to the tree. I'll experiment after I finish cutting the\n>> -rc0 preview release.\n>\n> I do not expect any conflicts and, if there any, they should be trivial.\n>\n> Let me know if you need any help.\n\nThanks. I think I got\n\n - revert of ar/run-command-hook directly on top of 2.53-rc0, which\n would become the tip of 'master' tomorrow.\n\n - rebuild of ar/run-command-hook + two fix-up topics on top of it,\n called ar/run-command-hook-take-2\n\nas the \"take-2\" topic is totally outside 'next', we can rebuild the\nentire topic and get it right the first time, instead of\nincrementally fixing them on top.\n\n\n"},{"id":"533992","messageId":"87h5smlgbu.fsf@collabora.com","threadId":"64791","inReplyTo":"xmqqfr86bp0n.fsf@gitster.g","subject":"Re: [PATCH v3 0/2] Fix two hook conversion regressions","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-15T21:24:37Z","receivedAt":"2026-01-15T21:25:04Z","isPatch":true,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Thu, 15 Jan 2026, Junio C Hamano wrote:\n> Adrian Ratiu writes:\n>\n>> On Thu, 15 Jan 2026, Junio C Hamano wrote:\n>>> Adrian Ratiu writes:\n>>>\n>>>> I agree with this.\n>>>>\n>>>> We can't let these two regressions enter a release, so we have two\n>>>> real chices:\n>>>>\n>>>> 1. Merge both fixes to 1.53 or\n>>>> 2. Revert the 'ar/run-command-hook' topic merge.\n>>>\n>>> Hmph, at this early point in the late release cycle before -rc1\n>>> (yes, rc0 is scheduled for this morning, but that is not really a\n>>> release candidate that counts as anything), it is tempting to take\n>>> #2, actually. I just do not know how much damage such a revert\n>>> would cause to the tree. I'll experiment after I finish cutting the\n>>> -rc0 preview release.\n>>\n>> I do not expect any conflicts and, if there any, they should be trivial.\n>>\n>> Let me know if you need any help.\n>\n> Thanks. I think I got\n>\n> - revert of ar/run-command-hook directly on top of 2.53-rc0, which\n> would become the tip of 'master' tomorrow.\n>\n> - rebuild of ar/run-command-hook + two fix-up topics on top of it,\n> called ar/run-command-hook-take-2\n>\n> as the \"take-2\" topic is totally outside 'next', we can rebuild the\n> entire topic and get it right the first time, instead of\n> incrementally fixing them on top.\n\nCool. I'll integrate the fixes into the series and send v7 continuing\nwhere we left off.\n\nThough I'll send the new test separately, in advance: there's no use\nblocking the new regression tests after the hooks conversions.\n"},{"id":"534137","messageId":"a2408c8c-db6e-4632-8fd8-7ac888bd3fa2@app.fastmail.com","threadId":"64791","inReplyTo":"20260114185731.2381550-3-adrian.ratiu@collabora.com","subject":"Re: [PATCH v3 2/2] hook: make ungroup opt-out instead of opt-in","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2026-01-18T08:44:53Z","receivedAt":"2026-01-18T08:45:15Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Wed, Jan 14, 2026, at 19:57, Adrian Ratiu wrote:\n> In 857f047e40 (hook: allow overriding the ungroup option, 2025-12-26),\n> I accidentally made the ungroup option opt-in instead of opt-out and\n> despite my best efforts to set it for all API users, I missed a case\n> which requires it to be set: the pre-push hook which regressed.\n>\n> The only thing I needed in that commit was a way to change the default,\n> to convert the remaining receive-pack hooks which require ungroup == 0\n> for sideband output, so it doesn't matter if it's on or off by default.\n>\n> Bring back the original behavior by setting it for all hooks in the\n> struct run_hooks_opt initializer, which nicely allows changing the\n> default value only where needed, in receive-pack.c.\n>\n> While at it add a few hook tests which exercise receive-pack sideband\n> output since they are the only ungroup=0 exceptions and there are no\n> other tests exercising this functionality.\n\nThis description looks okay given that the regression was never\nreleased. Only those who go out of their way build on top of `master`\n(for some reason) could have observed it. However if this was a bug in\nsome release then the description is very technical and play-by-play. As\na Git user reading this isolation, I see nothing that links these\nconcrete code discussions back to something that might have been weird\nin my hook scripts.\n\nThis would be especially relevant given that this bug is so weird. It’s\nnot a crash with some error text that can be googled; everything works\n(eventually) the same as before, only that *if* you read from standard\ninput you’ll have to wait a minute or so for something to time out and\nunstuck the whole process.\n\nAgain. I think this is okay since it was never a bug in any release.\n\n>\n> Fixes: 857f047e40f7 (\"hook: allow overriding the ungroup option\")\n> Reported-by: Kristoffer Haugsbakk \n> Suggested-by: Jeff King \n> Signed-off-by: Adrian Ratiu \n> ---\n>[snip]\n"}]}