| Message ID | patch-v6-2.2-503ef241a52-20220606T170356Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hook API: connect hooks to the TTY again, fixes a v2.36.0 regression | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > @@ -120,4 +121,34 @@ test_expect_success 'git -c core.hooksPath=<PATH> hook run' ' > test_cmp expect actual > ' > > +test_hook_tty() { Style. > + cat >expect <<-\EOF > + STDOUT TTY > + STDERR TTY > + EOF > + > + test_when_finished "rm -rf repo" && > + git init repo && > + > + test_commit -C repo A && > + test_commit -C repo B && > + git -C repo reset --soft HEAD^ && > + > + test_hook -C repo pre-commit <<-EOF && > + test -t 1 && echo STDOUT TTY >>actual || echo STDOUT NO TTY >>actual && > + test -t 2 && echo STDERR TTY >>actual || echo STDERR NO TTY >>actual > + EOF So, when this hook is run, we'd see if STDOUT and STDERR are connected to a tty in the "actual" file. > + test_terminal git "$@" && And we run the test and see > + test_cmp expect repo/actual what happens. The test_cmp knows that the git command runs in "repo" by hardcoding repo/actual, and this helper is full of the same knowledge, so it would be easier to see what is going on if you removed "-C repo" from the two callers (below) and instead added it to where you run "git" under test_terminal (above). > +} > + > +test_expect_success TTY 'git hook run: stdout and stderr are connected to a TTY' ' > + test_hook_tty -C repo hook run pre-commit > +' > + > +test_expect_success TTY 'git commit: stdout and stderr are connected to a TTY' ' > + test_hook_tty -C repo commit -m"B.new" > +' > + > test_done Other than that, looking good. Thanks.
diff --git a/hook.c b/hook.c index 1d51be3b77a..7451205657a 100644 --- a/hook.c +++ b/hook.c @@ -144,6 +144,7 @@ int run_hooks_opt(const char *hook_name, struct run_hooks_opt *options) cb_data.hook_path = abs_path.buf; } + run_processes_parallel_ungroup = 1; run_processes_parallel_tr2(jobs, pick_next_hook, notify_start_failure, diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh index 26ed5e11bc8..0175a0664da 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,34 @@ test_expect_success 'git -c core.hooksPath=<PATH> hook run' ' test_cmp expect actual ' +test_hook_tty() { + cat >expect <<-\EOF + STDOUT TTY + STDERR TTY + EOF + + test_when_finished "rm -rf repo" && + git init repo && + + test_commit -C repo A && + test_commit -C repo B && + git -C repo reset --soft HEAD^ && + + test_hook -C repo pre-commit <<-EOF && + test -t 1 && echo STDOUT TTY >>actual || echo STDOUT NO TTY >>actual && + test -t 2 && echo STDERR TTY >>actual || echo STDERR NO TTY >>actual + EOF + + test_terminal git "$@" && + test_cmp expect repo/actual +} + +test_expect_success TTY 'git hook run: stdout and stderr are connected to a TTY' ' + test_hook_tty -C repo hook run pre-commit +' + +test_expect_success TTY 'git commit: stdout and stderr are connected to a TTY' ' + test_hook_tty -C repo commit -m"B.new" +' + test_done
Fix a regression reported[1] against f443246b9f2 (commit: convert {pre-commit,prepare-commit-msg} hook to hook.h, 2021-12-22): Due to using the run_process_parallel() API in the earlier 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) we'd capture the hook's stderr and stdout, and thus lose the connection to the TTY in the case of e.g. the "pre-commit" hook. As a preceding commit notes GNU parallel's similar --ungroup option also has it emit output faster. While we're unlikely to have hooks that emit truly massive amounts of output (or where the performance thereof matters) it's still informative to measure the overhead. In a similar "seq" test we're now ~30% faster: $ cat .git/hooks/seq-hook; git hyperfine -L rev origin/master,HEAD~0 -s 'make CFLAGS=-O3' './git hook run seq-hook' #!/bin/sh seq 100000000 Benchmark 1: ./git hook run seq-hook' in 'origin/master Time (mean ± σ): 787.1 ms ± 13.6 ms [User: 701.6 ms, System: 534.4 ms] Range (min … max): 773.2 ms … 806.3 ms 10 runs Benchmark 2: ./git hook run seq-hook' in 'HEAD~0 Time (mean ± σ): 603.4 ms ± 1.6 ms [User: 573.1 ms, System: 30.3 ms] Range (min … max): 601.0 ms … 606.2 ms 10 runs Summary './git hook run seq-hook' in 'HEAD~0' ran 1.30 ± 0.02 times faster than './git hook run seq-hook' in 'origin/master' 1. https://lore.kernel.org/git/CA+dzEBn108QoMA28f0nC8K21XT+Afua0V2Qv8XkR8rAeqUCCZw@mail.gmail.com/ Reported-by: Anthony Sottile <asottile@umich.edu> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- hook.c | 1 + t/t1800-hook.sh | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+)