Re: [PATCH] hook: make stdout_to_stderr optional

From: Adrian Ratiu <adrian.ratiu@collabora.com>
Date: Jan 13, 2026, 13:55 UTC
Message-ID: <87ms2hipma.fsf@collabora.com>
In-Reply-To: <aWZKYAxhavFc1ZaH@pks.im>

On Tue, 13 Jan 2026, Patrick Steinhardt <ps@pks.im> wrote:

Show 14 quoted lines

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

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

Show 5 quoted lines

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

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

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

No breakage/changes to existing hooks.

Show 15 quoted lines

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

Agreed.

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