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

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

[Adam Zethraeus]

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.

```
> git commit -m "-" 1>/dev/null
stdout
stderr
> git commit -m "-" 2>/dev/null
> git commit -m "-" --no-verify
> git push 1>/dev/null
stderr
error: failed to push some refs to 'github.com:org/repo.git'
> git push 2>/dev/null
stdout
```

What's different between what you expected and what actually happened?

There was inconsistency in the behavior when I expected consistency.

Anything else you want to add:

```
> /bin/bash --version
GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin21)
```

Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.37.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
feature: fsmonitor--daemon
uname: Darwin 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:22
PDT 2022; root:xnu-8020.121.3~4/RELEASE_X86_64 x86_64
compiler info: clang: 13.1.6 (clang-1316.0.21.2.5)
libc info: no libc information available
$SHELL (typically, interactive shell): /bin/zsh


[Enabled Hooks]
pre-commit
pre-push

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

[Junio C Hamano]

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.

>
> ```
>> git commit -m "-" 1>/dev/null
> stdout
> stderr
>> git commit -m "-" 2>/dev/null
>> git commit -m "-" --no-verify
>> git push 1>/dev/null
> stderr
> error: failed to push some refs to 'github.com:org/repo.git'
>> git push 2>/dev/null
> stdout
> ```
>
> What's different between what you expected and what actually happened?
>
> There was inconsistency in the behavior when I expected consistency.
>
> Anything else you want to add:
>
> ```
>> /bin/bash --version
> GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin21)
> ```
>
> Please review the rest of the bug report below.
> You can delete any lines you don't wish to share.
>
>
> [System Info]
> git version:
> git version 2.37.0
> cpu: x86_64
> no commit associated with this build
> sizeof-long: 8
> sizeof-size_t: 8
> shell-path: /bin/sh
> feature: fsmonitor--daemon
> uname: Darwin 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:22
> PDT 2022; root:xnu-8020.121.3~4/RELEASE_X86_64 x86_64
> compiler info: clang: 13.1.6 (clang-1316.0.21.2.5)
> libc info: no libc information available
> $SHELL (typically, interactive shell): /bin/zsh
>
>
> [Enabled Hooks]
> pre-commit
> pre-push

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

[Ævar Arnfjörð Bjarmason]

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.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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).

"behavior change" meaning a regression of a particular hook, or
"behavior difference" between hooks, each of which never changed the
behavior?

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

I guess you meant the latter.  If so, the inconsistency may be
unfortunate, but I agree that it is not cut-and-dried that it is a
good idea to change pre-push to spew its output to standard error
stream.

> 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.

Absolutely.

> 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.

We'd probably need an inventory of them all anyway before we can
push the hook.c API forward.  If the oddball ones are very small
minority, it may be worth having a transition period and make
backward incompatible change to unify where the output goes.  If
they are random mixture, on the other hand, the hook.c API may have
to gain a bit for the caller to tell where the output of the hook
should go.

Thanks.

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

[Emily Shaffer]

On Thu, Jul 7, 2022 at 9:57 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
> > 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).
>
> "behavior change" meaning a regression of a particular hook, or
> "behavior difference" between hooks, each of which never changed the
> behavior?
>
> > I tested this with a local v2.30.0, and the behavior was the same.
>
> I guess you meant the latter.  If so, the inconsistency may be
> unfortunate, but I agree that it is not cut-and-dried that it is a
> good idea to change pre-push to spew its output to standard error
> stream.
>
> > 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.
>
> Absolutely.

On the one hand, we just had a particularly frustrating regression
around hook output direction, and it's likely none of us are
interested in repeating such pain anytime soon.

On the other hand, I wonder how worried we actually should be about
changing the behavior to normalize pre-push and other "oddball" hooks.
If the pipe used changes, what changes for the hook author? What about
for the person running the hook on their repository?

For the hook author, there is no change, in that the hook author
cannot control which pipe we decide to send their output to, and
should have written their hook with that in mind in the first place. I
found one example[1] of a hook which manually sends its only output to
stderr, everything else I saw just 'echo'es with abandon. I did notice
many hooks relying on colored output, but we've fixed that bug ;)

For the hook runner, the main difference has to do with parsing output
for scripts. That is, if I'm just looking with my human eyes, I don't
care which pipe is used, as long as it comes to the terminal. Of
course for scripting that is a different story, and if output I was
expecting on a certain pipe suddenly disappears (or vice versa) that
is a pain. However, I don't see much evidence that hook output can be
used in that way. In fact, I think that is the reason we shunt most
hook output to stderr; Git typically says "stdout is what you should
parse with your script, and stderr is what humans should look at with
their eyeballs." Through the very scientific method of looking at the
first 10 pages of GitHub search results[2], I didn't find any evidence
that pre-push hooks are being written with the intent for their output
to be parsed. That makes sense to me - I'd need to carefully
synchronize the output from my pre-push hook *and* the parser in my
script, and at that point it makes more sense to just teach the
pre-push hook to take whatever action it's trying to communicate to
the parser and cut out the interprocess communication, right?

Anyway, tl;dr - I think it would actually be a good and reasonable
thing to normalize the pre-push hook, either now or at the point where
it's converted to hook.c.

As for proc-receive, I think that one does something weird - the
hook's output is intended to be parsed by Git itself, and isn't shown
to the user. proc-receive is in fact such a weird and different hook
that it is completely exempt from the migration to hook.c. It does
bidirectional communication with the Git parent process and the parent
process decides how to act based on that communication, which means
that even if run_processes_parallel could support that (it doesn't) it
wouldn't make sense to configure more than one proc-receive hook
anyways. So proc-receive is intended to continue being an oddball and
not use the hook.c library (other than to check for the existence of 0
or 1 proc-receive hooks).

 - Emily

1: https://github.com/Kaliraj-hesabe/hesabe-reactnative-app/blob/3526fbe3fed8f08dadf10483c1292b8cd8f7d4ed/node_modules/realm/vendor/realm-core/tools/pre-push
2: https://github.com/search?l=&p=9&q=filename%3Apre-push+-filename%3A%2A.sample&type=Code