From: Adrian Ratiu Date: Tue, 13 Jan 2026 13:55:25 GMT Subject: Re: [PATCH] hook: make stdout_to_stderr optional Message-ID: <87ms2hipma.fsf@collabora.com> In-Reply-To: On Tue, 13 Jan 2026, Patrick Steinhardt wrote: > On Tue, Jan 13, 2026 at 01:56:33PM +0200, Adrian Ratiu wrote: >> The last batch of hooks converted to the hook.[ch] API introduced >> a regression because pick_next_hook() always sets stdout_to_stderr >> for its child processes. >> >> Pre-push is the only hook API user which requires stdout_to_stderr >> to be 0, so it can be argued that pre-push needs fixing, however >> this will likely break many pre-push hooks, so it's better to allow >> it to be 0, i.e. to match the previous behavior. > > Okay. Do you happen to know whether we've got test coverage for those > other hooks? Would be great to verify whether changing > `stodut_to_stderr` to default-disabled causes at least one test to fail > for every hook we've got. No, we do not have test coverage in this area and this is also the reason why this went unnoticed. >> We can introduce an extension for the breaking change of all hooks >> sending stdout to stderr, however this just fixes the regression. > > Is it really necessary to change this though? I wouldn't really want to > go there without a good reason. I'm still running tests on the full patch series, however the answer up to now is no, I do not think we need to change this. This means we can keep the existing behavior as-is and just introduce some tests to detect when/if stdout/stderr output expectation regresses. No breakage/changes to existing hooks. >> diff --git a/transport.c b/transport.c >> index 6d0f02be5d..8f0e5987ab 100644 >> --- a/transport.c >> +++ b/transport.c >> @@ -1372,6 +1372,7 @@ static int run_pre_push_hook(struct transport *transport, >> >> opt.feed_pipe = pre_push_hook_feed_stdin; >> opt.feed_pipe_cb_data = &data; >> + opt.stdout_to_stderr = 0; >> >> ret = run_hooks_opt(the_repository, "pre-push", &opt); > > The fact that this was able to sneak in without anybody noticing shows > that we have a test gap. Can we maybe have a test that verifies that the > hook output goes to the correct standard stream? Agreed. I'll send a v2 containing a stdout/stderr expectation test for each hook and remove the extension commment from the commit message.