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

[Emily Shaffer <emilyshaffer@google.com>]

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