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
