Re: [PATCH 6/6] hook API: fix v2.36.0 regression: hooks should be connected to a TTY

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> In the preceding commit we removed the "no_stdin=1" and
> "stdout_to_stderr=1" assignments. This change brings them back as with
> ".ungroup=1" the run_process_parallel() function doesn't provide them
> for us implicitly.

Wait.

No hunk in this step updates how pick_next_hook() is called
(presumably "with .ungroup=1"), and there is no change to the code
that uses the cp structure prepared by pick_next_hook() further, so
why does the change in the previous step need to be reverted in this
step?  Does it mean if we apply patches 1-5 without this step, because
of step 5, the contents of cp structure returned by pick_next_hook()
is broken?  But we clearly saw a claim that these assignments are
redundant and unnecessary.

So I am confused as to what change in this step makes these
assignments necessary again?  There is no removal of assignments to
these two members that we used to have in this patch.  There is no
addition of a new caller that calls pick_next_hook and uses it
differently (i.e. the other existing caller(s) made assignments to
these two members, which allowed 5/6 to remove the assignments, but
if this step adds a different caller that uses the struct without
making these assignments, then we do need to add them back).

Either I am reading a wrong patch, or the steps 5 & 6 confused me
beyond repair, or perhaps a bit of both?

If [5/6] were not there, and [6/6] was added because [5/6] broke it
by forgetting that some caller that already exists after applying
[5/6] did not make assignments to these two members (iow, the
assignment removed by that step were not redundant and [5/6] was
buggy in removing them), then I would understand it, but that does
not seem to be the case...

Puzzled and utterly confused I am.

> @@ -53,7 +53,9 @@ static int pick_next_hook(struct child_process *cp,
>  	if (!hook_path)
>  		return 0;
>  
> +	cp->no_stdin = 1;
>  	strvec_pushv(&cp->env_array, hook_cb->options->env.v);
> +	cp->stdout_to_stderr = 1;
>  	cp->trace2_hook_name = hook_cb->hook_name;
>  	cp->dir = hook_cb->options->dir;
>  
> @@ -119,16 +121,20 @@ int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options)
>  		.options = options,
>  	};
>  	const char *const hook_path = find_hook(hook_name);
> -	int jobs = 1;
> +	const int jobs = 1;
>  	int ret = 0;
>  	struct run_process_parallel_opts run_opts = {
>  		.tr2_category = "hook",
>  		.tr2_label = hook_name,
> +		.ungroup = jobs == 1,
>  	};
>  
>  	if (!options)
>  		BUG("a struct run_hooks_opt must be provided to run_hooks");
>  
> +	if (jobs != 1 || !run_opts.ungroup)
> +		BUG("TODO: think about & document order & interleaving of parallel hook output");
> +
>  	if (options->invoked_hook)
>  		*options->invoked_hook = 0;
>  
> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
> index 1e4adc3d53e..f22754deccc 100755
> --- a/t/t1800-hook.sh
> +++ b/t/t1800-hook.sh
> @@ -4,6 +4,7 @@ test_description='git-hook command'
>  
>  TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-terminal.sh
>  
>  test_expect_success 'git hook usage' '
>  	test_expect_code 129 git hook &&
> @@ -120,4 +121,40 @@ test_expect_success 'git -c core.hooksPath=<PATH> hook run' '
>  	test_cmp expect actual
>  '
>  
> +test_hook_tty() {
> +	local fd="$1" &&
> +
> +	cat >expect &&
> +
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +
> +	test_hook -C repo pre-commit <<-EOF &&
> +	{
> +		test -t 1 && echo >&$fd STDOUT TTY || echo >&$fd STDOUT NO TTY &&
> +		test -t 2 && echo >&$fd STDERR TTY || echo >&$fd STDERR NO TTY
> +	} $fd>actual
> +	EOF
> +
> +	test_commit -C repo A &&
> +	test_commit -C repo B &&
> +	git -C repo reset --soft HEAD^ &&
> +	test_terminal git -C repo commit -m"B.new" &&
> +	test_cmp expect repo/actual
> +}
> +
> +test_expect_success TTY 'git hook run: stdout and stderr are connected to a TTY: STDOUT redirect' '
> +	test_hook_tty 1 <<-\EOF
> +	STDOUT NO TTY
> +	STDERR TTY
> +	EOF
> +'
> +
> +test_expect_success TTY 'git hook run: stdout and stderr are connected to a TTY: STDERR redirect' '
> +	test_hook_tty 2 <<-\EOF
> +	STDOUT TTY
> +	STDERR NO TTY
> +	EOF
> +'
> +
>  test_done