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

[Junio C Hamano <gitster@pobox.com>]

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.

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.

Thanks.