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 <avarab@xxxxxxxxx>
> ---
> 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 <emilyshaffer@xxxxxxxxxx>
> '
>
> COMMIT2="$(git rev-parse HEAD)"
> --
> 2.33.0.rc0.595.ge31e012651d
>
