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

> 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?