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

[Anthony Sottile <asottile@umich.edu>]

On Tue, Apr 19, 2022 at 7:37 PM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> 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 :)

ah, most of the repro script was just so I could bisect -- you can
ignore pretty much all of it except for the `pre-commit` file:

#!/usr/bin/env bash
if [ -t 1 ]; then
   echo GOOD
fi

this is doing "isatty" against fd 1 which is stdout (it could also try
the same against fd 2 which was also a tty previously)

Anthony

>
>  - Emily