| Message ID | b51f97f6ae37e4b69b9651cbd60a480e5db3e72d.1585115341.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t: replace incorrect test_must_fail usage (part 3) | expand |
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' '
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.
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' '
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(-)