From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 20 Apr 2022 09:00:45 GMT
Subject: Re: git 2.36.0 regression: pre-commit hooks no longer have stdout/stderr as tty
Message-ID: <6aabbcd6-f6c2-fe97-eb73-593bcf2e9e75@gmail.com>
In-Reply-To: <Yl9Hn0C0TwalASC0@google.com>

Hi Emily

On 20/04/2022 00:37, Emily Shaffer wrote:
> On Tue, Apr 19, 2022 at 02:59:36PM -0400, Anthony Sottile wrote:
>> [...]
> 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?

Exactly, stderr is redirected to a pipe so that we can buffer the output 
from each process and then write it to the real stdout when the process 
has finished to avoid the output from different processes getting mixed 
together. Ideally in this case we'd see that stdout is a tty and create 
a pty rather than a pipe when buffering the output from the process.

Best Wishes

Phillip