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

[Emily Shaffer <emilyshaffer@google.com>]

On Tue, Apr 19, 2022 at 02:59:36PM -0400, Anthony Sottile wrote:
> 
> here's the shortest reproduction --
> 
> ```console
> $ cat ../testrepo/.git/hooks/pre-commit
> #!/usr/bin/env bash
> if [ -t 1 ]; then
>     echo GOOD
> fi
> ```
> 
> in previous git versions:
> 
> ```
> $ git commit -q --allow-empty -m foo
> GOOD
> $
> ```
> 
> with git 2.36.0:
> 
> ````
> $ git commit -q --allow-empty -m foo
> $
> ```
> 
> why I care: I maintain a git hooks framework which uses `isatty` to
> detect whether it's appropriate to color the output.  many tools
> utilize the same check.  in 2.36.0+ isatty is false for stdout and
> stderr causing coloring to be turned off.
> 
> I bisected this (it was a little complicated, needed to force a pty):
> 
> `../testrepo`: a git repo set up with the hook above
> 
> `../bisect.sh`:
> 
> ```bash
> #!/usr/bin/env bash
> set -eux
> git clean -fxfd >& /dev/null
> make -j6 prefix="$PWD/prefix" NO_GETTEXT=1 NO_TCLTK=1 install >& /dev/null
> export PATH="$PWD/prefix/bin:$PATH"
> cd ../testrepo
> (../pty git commit -q --allow-empty -m foo || true) | grep GOOD
> ```
> 
> `../pty`:
> 
> ```python
> #!/usr/bin/env python3
> import errno
> import os
> import subprocess
> import sys
> 
> x: int = 'nope'
> 
> 
> class Pty(object):
>     def __init__(self):
>         self.r = self.w = None
> 
>     def __enter__(self):
>         self.r, self.w = os.openpty()
> 
>         return self
> 
>     def close_w(self):
>         if self.w is not None:
>             os.close(self.w)
>             self.w = None
> 
>     def close_r(self):
>         assert self.r is not None
>         os.close(self.r)
>         self.r = None
> 
>     def __exit__(self, exc_type, exc_value, traceback):
>         self.close_w()
>         self.close_r()
> 
> 
> def cmd_output_p(*cmd, **kwargs):
>     with open(os.devnull) as devnull, Pty() as pty:
>         kwargs = {'stdin': devnull, 'stdout': pty.w, 'stderr': pty.w}
>         proc = subprocess.Popen(cmd, **kwargs)
>         pty.close_w()
> 
>         buf = b''
>         while True:
>             try:
>                 bts = os.read(pty.r, 4096)
>             except OSError as e:
>                 if e.errno == errno.EIO:
>                     bts = b''
>                 else:
>                     raise
>             else:
>                 buf += bts
>             if not bts:
>                 break
> 
>     return proc.wait(), buf, None
> 
> 
> if __name__ == '__main__':
>     _, buf, _ = cmd_output_p(*sys.argv[1:])
>     sys.stdout.buffer.write(buf)
> ```
> 
> the first commit it points out:
> 
> ```
> f443246b9f29b815f0b98a07bb2d425628ae6522 is the first bad commit
> commit f443246b9f29b815f0b98a07bb2d425628ae6522
> Author: Emily Shaffer <emilyshaffer@google.com>
> 
>     commit: convert {pre-commit,prepare-commit-msg} hook to hook.h
> 
>     Move these hooks hook away from run-command.h to and over to the new
>     hook.h library.
> 
>     Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
>     Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>     Acked-by: Emily Shaffer <emilyshaffer@google.com>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
>  commit.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> bisect run success
> ```

Interesting. I'm surprised to see the tty-ness of hooks changing with
this patch, as the way the hook is called is pretty much the same:

run_hook_ve() ("the old way") sets no_stdin, stdout_to_stderr, args,
envvars, and some trace variables, and then runs 'run_command()';
run_command() invokes start_command().

run_hooks_opt ("the new way") ultimately kicks off the hook with a
callback that sets up a child_process with no_stdin, stdout_to_stderr,
args, envvars, and some trace variables (hook.c:pick_next_hook); the
task queue manager also sets .err to -1 on that child_process; then it
calls start_command() directly (run-command.c:pp_start_one()).

I'm not sure I see why the tty-ness would change between the two. If I'm
being honest, I'm actually slightly surprised that `isatty` returned
true for your hook before - since the hook process is a child of Git and
its output is, presumably, being consumed by Git first rather than by an
interactive user shell.

I suppose that with stdout_to_stderr being set, the tty-ness of the main
process's stderr would then apply to the child process's stdout (we do
that by calling `dup(2)`). But that's being set in both "the old way"
and "the new way", so I'm pretty surprised to see a change here.

It *is* true that run-command.c:pp_start_one() sets child_process:err=-1
for the child and run-command.c:run_hook_ve() didn't do that; that -1
means that start_command() will create a new fd for the child's stderr.
Since run_hook_ve() didn't care about the child's stderr before, I
wonder if that is why? Could it be that now that we're processing the
child's stderr, the child no longer thinks stderr is in tty, because the
parent is consuming its output?

I think if that's the case, a fix would involve
run-command.c:pp_start_one() not setting .err, .stdout_to_stderr, or
.no_stdin at all on its own, and relying on the 'get_next_task' callback
to set those things. It's a little more painful than I initially thought
because the run_processes_parallel() library depends on that err capture
to run pp_buffer_stderr() unconditionally; I guess it needs a tiny bit
of shim logic to deal with callers who don't care to see their
children's stderr.

All that said.... I'd expect that the dup() from the child's stdout to
the parent's stderr would still result in a happy isatty(1). So I'm not
convinced this is actually the right solution.... From your repro
script, I can't quite tell which fd the isatty call is against (to be
honest, I can't find the isatty call, either). So maybe I'm going the
wrong direction :)

 - Emily