[8/8] t5801: teach compare_refs() to accept !
diff mbox series

Message ID b51f97f6ae37e4b69b9651cbd60a480e5db3e72d.1585115341.git.liu.denton@gmail.com
State New
Headers show
Series
  • t: replace incorrect test_must_fail usage (part 3)
Related show

Commit Message

Denton Liu March 25, 2020, 5:54 a.m. UTC
Before, testing if two refs weren't equal with compare_refs() was done
with `test_must_fail compare_refs`. This was wrong for two reasons.
First, test_must_fail should only be used on git commands. Second,
negating the error code is a little heavy-handed since in the case where
one of the git invocations within compare_refs() fails, we will report
success, even though it failed at an unexpected point.

Teach compare_refs() to accept `!` as the first argument which would
_only_ negate the test_cmp()'s return code.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t5801-remote-helpers.sh | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Junio C Hamano March 25, 2020, 6:31 a.m. UTC | #1
Denton Liu <liu.denton@gmail.com> writes:

>  compare_refs() {
> +	fail= &&
> +	if test "x$1" = 'x!'
> +	then
> +		fail='!' &&
> +		shift
> +	fi &&
>  	git --git-dir="$1/.git" rev-parse --verify $2 >expect &&
>  	git --git-dir="$3/.git" rev-parse --verify $4 >actual &&
> -	test_cmp expect actual
> +	eval $fail test_cmp expect actual
>  }
>  
>  test_expect_success 'setup repository' '
> @@ -189,7 +195,7 @@ test_expect_success GPG 'push signed tag' '
>  	git push origin signed-tag
>  	) &&
>  	compare_refs local signed-tag^{} server signed-tag^{} &&
> -	test_must_fail compare_refs local signed-tag server signed-tag
> +	compare_refs ! local signed-tag server signed-tag

While this is not wrong per-se, I do not know why we cannot just use

	! compare_refs local signed-tag server signed-tag

i.e. "we expect these two repositories have different tags"?

It is totally plausible that I am missing something obvious, as it
is getting late and I no longer am a night person.

Thanks.

>  '
>  
>  test_expect_success GPG 'push signed tag with signed-tags capability' '
Eric Sunshine March 25, 2020, 6:36 a.m. UTC | #2
On Wed, Mar 25, 2020 at 2:31 AM Junio C Hamano <gitster@pobox.com> wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> >  compare_refs() {
> > +     fail= &&
> > +     if test "x$1" = 'x!'
> > +     then
> > +             fail='!' &&
> > +             shift
> > +     fi &&
> >       git --git-dir="$1/.git" rev-parse --verify $2 >expect &&
> >       git --git-dir="$3/.git" rev-parse --verify $4 >actual &&
> > -     test_cmp expect actual
> > +     eval $fail test_cmp expect actual
> >  }
> > -     test_must_fail compare_refs local signed-tag server signed-tag
> > +     compare_refs ! local signed-tag server signed-tag
>
> While this is not wrong per-se, I do not know why we cannot just use
>
>         ! compare_refs local signed-tag server signed-tag
>
> i.e. "we expect these two repositories have different tags"?

As mentioned in the commit message, if one of the git-rev-parse
invocations fails unexpectedly, then compare_refs() would return early
with a failure code, but the "!" would then (undesirably) turn that
failure into a success. We don't want to lose a failure code from
git-rev-parse, so the simple use of "! compare_refs ..." is avoided.

Patch
diff mbox series

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index 121e5c6edb..0f04b6cddb 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -11,9 +11,15 @@  test_description='Test remote-helper import and export commands'
 PATH="$TEST_DIRECTORY/t5801:$PATH"
 
 compare_refs() {
+	fail= &&
+	if test "x$1" = 'x!'
+	then
+		fail='!' &&
+		shift
+	fi &&
 	git --git-dir="$1/.git" rev-parse --verify $2 >expect &&
 	git --git-dir="$3/.git" rev-parse --verify $4 >actual &&
-	test_cmp expect actual
+	eval $fail test_cmp expect actual
 }
 
 test_expect_success 'setup repository' '
@@ -189,7 +195,7 @@  test_expect_success GPG 'push signed tag' '
 	git push origin signed-tag
 	) &&
 	compare_refs local signed-tag^{} server signed-tag^{} &&
-	test_must_fail compare_refs local signed-tag server signed-tag
+	compare_refs ! local signed-tag server signed-tag
 '
 
 test_expect_success GPG 'push signed tag with signed-tags capability' '