mbox series

[v6,0/2] hook API: connect hooks to the TTY again, fixes a v2.36.0 regression

Message ID cover-v6-0.2-00000000000-20220606T170356Z-avarab@gmail.com (mailing list archive)
Headers show
Series hook API: connect hooks to the TTY again, fixes a v2.36.0 regression | expand

Message

Ævar Arnfjörð Bjarmason June 7, 2022, 8:48 a.m. UTC
This series fixes a v2.36.0 regression[1]. See [2] for the v5. The
reasons for why a regression needs this relatively large change to
move forward is discussed in past rounds, e.g. around [3]. CI at
https://github.com/avar/git/actions/runs/2448496389

Changes since v5:

 * Make the hook run test more meaningful, we now test with "-t" in
   the hook, instead of redirecting one of STDOUT or STDERR.

 * Add a test for both "git hook run" and "git commit", to showh that
   the "git hook run" command and one "real" user of it agree.

1. https://lore.kernel.org/git/cover-v5-0.2-00000000000-20220602T131858Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (2):
  run-command: add an "ungroup" option to run_process_parallel()
  hook API: fix v2.36.0 regression: hooks should be connected to a TTY

Ævar Arnfjörð Bjarmason (2):
  run-command: add an "ungroup" option to run_process_parallel()
  hook API: fix v2.36.0 regression: hooks should be connected to a TTY

 hook.c                      |  1 +
 run-command.c               | 70 +++++++++++++++++++++++++++----------
 run-command.h               | 30 ++++++++++++----
 t/helper/test-run-command.c | 22 ++++++++++--
 t/t0061-run-command.sh      | 30 ++++++++++++++++
 t/t1800-hook.sh             | 31 ++++++++++++++++
 6 files changed, 155 insertions(+), 29 deletions(-)

Range-diff against v5:
1:  d018b7c4441 = 1:  45248c786d7 run-command: add an "ungroup" option to run_process_parallel()
2:  b0f0dc7492a ! 2:  503ef241a52 hook API: fix v2.36.0 regression: hooks should be connected to a TTY
    @@ t/t1800-hook.sh: test_expect_success 'git -c core.hooksPath=<PATH> hook run' '
      '
      
     +test_hook_tty() {
    -+	local fd="$1" &&
    -+
    -+	cat >expect &&
    ++	cat >expect <<-\EOF
    ++	STDOUT TTY
    ++	STDERR TTY
    ++	EOF
     +
     +	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_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: 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' '
    ++	test_hook_tty -C repo hook run pre-commit
     +'
     +
    -+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_expect_success TTY 'git commit: stdout and stderr are connected to a TTY' '
    ++	test_hook_tty -C repo commit -m"B.new"
     +'
     +
      test_done

Comments

Junio C Hamano June 7, 2022, 5:02 p.m. UTC | #1
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> This series fixes a v2.36.0 regression[1]. See [2] for the v5. The
> reasons for why a regression needs this relatively large change to
> move forward is discussed in past rounds, e.g. around [3]. CI at
> https://github.com/avar/git/actions/runs/2448496389
>
> Changes since v5:
>
>  * Make the hook run test more meaningful, we now test with "-t" in
>    the hook, instead of redirecting one of STDOUT or STDERR.
>
>  * Add a test for both "git hook run" and "git commit", to showh that
>    the "git hook run" command and one "real" user of it agree.

Thanks for a careful review and a timely response.

>
> 1. https://lore.kernel.org/git/cover-v5-0.2-00000000000-20220602T131858Z-avarab@gmail.com/
>
> Ævar Arnfjörð Bjarmason (2):
>   run-command: add an "ungroup" option to run_process_parallel()
>   hook API: fix v2.36.0 regression: hooks should be connected to a TTY
>
> Ævar Arnfjörð Bjarmason (2):
>   run-command: add an "ungroup" option to run_process_parallel()
>   hook API: fix v2.36.0 regression: hooks should be connected to a TTY

Puzzling.  Perhaps copy-and-paste mistake we can ignore.

>  hook.c                      |  1 +
>  run-command.c               | 70 +++++++++++++++++++++++++++----------
>  run-command.h               | 30 ++++++++++++----
>  t/helper/test-run-command.c | 22 ++++++++++--
>  t/t0061-run-command.sh      | 30 ++++++++++++++++
>  t/t1800-hook.sh             | 31 ++++++++++++++++
>  6 files changed, 155 insertions(+), 29 deletions(-)