From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Tue, 13 Jan 2026 14:11:58 GMT
Subject: Re: [PATCH] hook: make stdout_to_stderr optional
Message-ID: <87jyxlioup.fsf@collabora.com>
In-Reply-To: <xmqq7btlliip.fsf@gitster.g>

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

Yes, your understanding is correct in all the above.

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

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

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