Re: git 2.36.0 regression: pre-commit hooks no longer have stdout/stderr as tty

[Emily Shaffer <emilyshaffer@google.com>]

On Wed, Apr 20, 2022 at 10:25 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Emily Shaffer <emilyshaffer@google.com> writes:
>
> >> In the longer term, there are multiple possible action items.
> >> ...
> >>
> >>  * We should teach hooks API to make it _optional_ to use the
> >>    parallel subprocess API.  If we are not spawning hooks in
> >>    parallel today, there is no reason to incur this regression by
> >>    using the parallel subprocess API---this was a needress bug, and
> >>    I am angry.
> >
> > To counter, I think that having hooks invoked via two different
> > mechanisms depending on how many are provided or whether they are
> > parallelized is a mess to debug and maintain. I still stand by the
> > decision to use the parallel subprocess API, which I think was
> > reasonable to expect to do the same thing when jobs=1, and I think we
> > should continue to do so. It simplifies the hook code significantly.
>
> A simple code that does not behave as it should and causes end-user
> regression is not a code worth defending.  Admitting it was a bad
> move we made in the past is the first step to make it better.

I am also sorry that this use case was broken. However, I don't see
that it's documented in 'git help githooks' or elsewhere that we
guarantee isatty() (or similar) of hooks matches that of the parent
process. I think it is an accident that this worked before, and not
something that was guaranteed by Git documentation - for example, we
also do not have regression tests ensuring that behavior for hooks
today, either, or else we would not be having this conversation. (If I
simply missed the documentation promising that behavior, then I am
sorry, and please point me to it.)

>
> The use of the parallel subprocess API in the hooks was prematurely
> done, before we had clear use cases for running multiple hooks in
> parallel, and due to the lack of use cases, we didn't have chance to
> think about the issues that need to be addressed before we can start
> using the parallel subprocess API.  The message you are responding to
> was written with an explicit purpose of starting to list them.
>
> >>  * the hooks API should learn a mechanism for multiple hooks to
> >>    coordinate their executions.  Perhaps they indicate their
> >>    preference if they are OK to be run in parallel, and those that
> >>    want isolation will be run one-at-a-time before or after others
> >>    run in parallel, or something.
> >
> > There is such a mechanism for hooks overall, but not yet for
> > individual hooks. I know we discussed it at length[1] before, and
>
> This...
>
> > decided it would be okay to figure this out later on. I suppose "later
> > on" may have come :)
>
> Yes, besides patching up this regression for short term, I listed it
> as a possible ation item for the longer term.
>
> >>  * The hooks API should learn a mechanism for us to tell what
> >>    execution environment they are in.  Ideally, the hooks, if it is
> >>    sane to run under the parallel subprocess API, shouldn't have
> >>    been learning if they are talking to an interactive human user by
> >>    looking at isatty(), but we should have been explicitly telling
> >>    them that they are, perhaps by exporting an environment
> >>    variable.  There may probably be more clue hooks writers want
> >>    other than "am I talking to human user?" that we would want to
> >>    enumerate before going this route.
> >
> > Hm. I was going to mention that Ævar and I discussed the possibility
> > of setting an environment variable for hook child processes, telling
>
> That...
>
> > them which hook they are being run as - e.g.
> > "GIT_HOOK=prepare-commit-msg" - but I suppose that relying on that
> > alone doesn't tell us anything about whether the parent is being run
> > in tty. I agree it could be very useful to simply pass
> > GIT_PARENT_ISATTY to hooks (and I suppose other child processes).
> > Could we simply do that from start_command() or something else deep in
> > run-command.h machinery? Then Anthony's use case becomes
> >
> > if [-t 1|| GIT_PARENT_ISATTY]
> >  ...
> >
> > and no need to examine Git version.
>
> But DO NOT call it ISATTY.  "Are we showing the output to human
> end-users" is the question it is answering to, and isatty() happens
> to be an implementation detail on POSIXy system.
>
> "This" and "That" above make it smell like discussion was done, but
> everybody got tired of discussing and the topic was shipped without
> necessary polishment?  That sounds like a process failure, which we
> may want to address in the new development cycle, not limited to this
> particular topic.

I think, rather, during discussion we said "without knowing how real
users want to use hooks, it's not possible for us to make a good
design for individual hooks to state whether they need to be
parallelized or not." Perhaps that means this body of work should have
stayed in 'next' longer, rather than making it to a release?

For what it's worth, Google internally has been using multiple hooks
via config for something like a year, with this design, from a
combination of 'next' and pending hooks patches. But we haven't
imagined the need to color hook output for users and check isatty() or
similar. I think there are not many other consumers of 'next' besides
the Google internal release. So I'm not sure that longer time in
'next' would have allowed us to see this issue, either.

 - Emily