Re: bug report: pre-commit & pre-push hook output is redirected differently

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Jul 06 2022, Junio C Hamano wrote:

> Adam Zethraeus <adam.zethraeus@includedhealth.com> writes:
>
>> What did you do before the bug happened? (Steps to reproduce your issue)
>>
>> Installed identical pre-commit and pre-push hooks:
>>
>> ```
>> #!/bin/bash
>>
>>>&1 echo "stdout"
>>>&2 echo "stderr"
>> exit 1
>> ```
>>
>> What did you expect to happen? (Expected behavior)
>>
>> `git push` and `git commit` should have the same hook behavior.
>>
>> What happened instead? (Actual behavior)
>>
>> The pre-commit hook was run with stdout redirected to stderr but the
>> pre-push hook's output was unaltered.
>
> Without looking into it very much, the output of hooks is an area
> with known regression at 2.36, so let me redirect it to those who
> are likely to know it ;-)
>
> Thanks for a report.

I may be missing something, but I think this report has nothing to do
with any recent changes or regressions, but is merely noting a behavior
change between pre-push and some other hooks that we've had since 1.8.2,
or since the "pre-push" hook was added in ec55559f937 (push: Add support
for pre-push hooks, 2013-01-13).

I tested this with a local v2.30.0, and the behavior was the same.

This will "fix" it:
	
	diff --git a/transport.c b/transport.c
	index 52db7a3cb09..0cc7d05e0da 100644
	--- a/transport.c
	+++ b/transport.c
	@@ -1225,6 +1225,7 @@ static int run_pre_push_hook(struct transport *transport,
	 	strvec_push(&proc.args, transport->url);
	 
	 	proc.in = -1;
	+	proc.stdout_to_stderr = 1;
	 	proc.trace2_hook_name = "pre-push";
	 
	 	if (start_command(&proc)) {

But whether that's a fix or not depends on whether we think we should
make this behavior consistent. I tend to think so, but it would be a
behavior change to long-established behavior in pre-push.

It *is* something we need to be careful of when converting the rest of
the hooks to the hooks API, i.e. we need tests for how stderr/stdout is
handled for each one.

But this being different is just because some hook use the hook.c API
(and before that the helper in run-command.c), and others use "struct
child_process" or whatever explicitly (such as "pre-push").

Since it's up to each callsite to set up the "proc" (or equivalent) some
supply "stdout_to_stderr", some don't.

From some quick grepping it seems the odd ones out are pre-push and
proc-receive, but I only skimmed a "git grep" to find the second one,
and may have missed others.