Subject: Re: [PATCH v4 28/36] hook tests: test for exact "pre-push" hook input

From: Emily Shaffer <[email-masked]>
Date: Thu, 19 Aug 2021 17:16:30 -0700

On Tue, Aug 03, 2021 at 09:38:54PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> Extend the tests added in ec55559f937 (push: Add support for pre-push
> hooks, 2013-01-13) to exhaustively test for the exact input we're
> expecting. This helps a parallel series that's refactoring how the
> hook is called, to e.g. make sure that we don't miss a trailing
> newline

The reference to "a parallel series" I think didn't belong in the
commit-msg in the first place, and doesn't make sense now (because this
is in said series, right?).

> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <[email-masked]>
> ---
>  t/t5571-pre-push-hook.sh | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
> index ad8d5804f7b..d2857a6fbc0 100755
> --- a/t/t5571-pre-push-hook.sh
> +++ b/t/t5571-pre-push-hook.sh
> @@ -11,7 +11,7 @@ HOOKDIR="$(git rev-parse --git-dir)/hooks"
>  HOOK="$HOOKDIR/pre-push"
>  mkdir -p "$HOOKDIR"
>  write_script "$HOOK" <<EOF
> -cat >/dev/null
> +cat >actual
>  exit 0
>  EOF
>  
> @@ -20,10 +20,16 @@ test_expect_success 'setup' '
>  	git init --bare repo1 &&
>  	git remote add parent1 repo1 &&
>  	test_commit one &&
> -	git push parent1 HEAD:foreign
> +	cat >expect <<-EOF &&
> +	HEAD $(git rev-parse HEAD) refs/heads/foreign $(test_oid zero)
> +	EOF
> +
> +	test_when_finished "rm actual" &&
> +	git push parent1 HEAD:foreign &&
> +	test_cmp expect actual
>  '
>  write_script "$HOOK" <<EOF
> -cat >/dev/null
> +cat >actual

Hm, I am not sure I like this. It upsets the usual convention of what
'actual' means ("output I captured from a 'git' command"). It is
nitpicky, but I would be happier to see it cat to some other filename,
like 'received-input'.

>  exit 1
>  EOF
>  
> @@ -32,11 +38,18 @@ export COMMIT1
>  
>  test_expect_success 'push with failing hook' '
>  	test_commit two &&
> -	test_must_fail git push parent1 HEAD
> +	cat >expect <<-EOF &&
> +	HEAD $(git rev-parse HEAD) refs/heads/main $(test_oid zero)
> +	EOF
> +
> +	test_when_finished "rm actual" &&
> +	test_must_fail git push parent1 HEAD &&
> +	test_cmp expect actual
>  '
>  
>  test_expect_success '--no-verify bypasses hook' '
> -	git push --no-verify parent1 HEAD
> +	git push --no-verify parent1 HEAD &&
> +	test_path_is_missing actual

Other than the naming nit, though, the test changes look reasonable to
me to ensure we don't goof up the stdin support in hook.c. Thanks.

With (or, I guess, without) changes,
Reviewed-by: Emily Shaffer <[email-masked]>

>  '
>  
>  COMMIT2="$(git rev-parse HEAD)"
> -- 
> 2.33.0.rc0.595.ge31e012651d
>