Re: [PATCH 6/6] hook API: fix v2.36.0 regression: hooks should be connected to a TTY
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 29, 2022, 23:09 UTC
- Message-ID
- <xmqqfslva3mx.fsf@gitster.g>
- In-Reply-To
- <patch-6.6-de3664f6d2b-20220421T122108Z-avarab@gmail.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.
Show 85 quoted lines
> @@ -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