From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 13 Jan 2026 13:36:32 GMT
Subject: Re: [PATCH] hook: make stdout_to_stderr optional
Message-ID: <aWZKYAxhavFc1ZaH@pks.im>
In-Reply-To: <20260113115633.230479-1-adrian.ratiu@collabora.com>

On Tue, Jan 13, 2026 at 01:56:33PM +0200, Adrian Ratiu wrote:
> The last batch of hooks converted to the hook.[ch] API introduced
> a regression because pick_next_hook() always sets stdout_to_stderr
> for its child processes.
> 
> Pre-push is the only hook API user which requires stdout_to_stderr
> to be 0, so it can be argued that pre-push needs fixing, however
> this will likely break many pre-push hooks, so it's better to allow
> it to be 0, i.e. to match the previous behavior.

Okay. Do you happen to know whether we've got test coverage for those
other hooks? Would be great to verify whether changing
`stodut_to_stderr` to default-disabled causes at least one test to fail
for every hook we've got.

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

Is it really necessary to change this though? I wouldn't really want to
go there without a good reason.

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

The fact that this was able to sneak in without anybody noticing shows
that we have a test gap. Can we maybe have a test that verifies that the
hook output goes to the correct standard stream?

Thanks!

Patrick