Re: [PATCH v2] test_cmp: diagnose incorrect arguments

From

Junio C Hamano <gitster@pobox.com>

Date

Oct 16, 2020, 20:56 UTC

Message-ID

<xmqqv9f9ao0v.fsf@gitster.c.googlers.com>

In-Reply-To

<xmqqzh4maugq.fsf@gitster.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

> Hmph, I agree that the "both must be file" is a bit too eager and
> ignores that "they must match, but the possible reasons they may not
> include one of them may be missing" use case.
>
>> Ævar ran into the same issue recently[1] and came up with the same
>> workaround. Despite its good intention (trying to catch bugs in
>> 'test_expect_failure' tests), this change[2] doesn't seem to have
>> caught any genuine bugs (it wouldn't even have caught the bug which
>> served as its inspiration[3]), but has nevertheless caused a couple
>> hiccups already. As such, I would not be opposed to seeing the change
>> reverted.
>
> Sounds good.  Anybody wants to do the honors?

Thanks.

-- >8 --
commit b0b2d8b4e00fd34c0031b6dbcd67b4bcf22d864c
Author: Junio C Hamano <gitster@pobox.com>
Date:   Fri Oct 16 13:51:04 2020 -0700

    Revert "test_cmp: diagnose incorrect arguments"
    
    This reverts commit d572f52a64c6a69990f72ad6a09504b9b615d2e4; the
    idea to detect that "test_cmp expect actual" was fed a misspelt
    filename meant well, but when the version of Git tested exhibits a
    bug, the reason why these two files do not match may be because one
    of them did not get created as expected, in which case missing file
    is not a sign of misspelt filename but is a genuine test failure.
---
 t/test-lib-functions.sh | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 21225330c2..3103be8a32 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -905,13 +905,7 @@ test_expect_code () {
 # - not all diff versions understand "-u"
 
 test_cmp() {
-	test $# -eq 2 || BUG "test_cmp requires two arguments"
-	if ! eval "$GIT_TEST_CMP" '"$@"'
-	then
-		test "x$1" = x- || test -e "$1" || BUG "test_cmp '$1' missing"
-		test "x$2" = x- || test -e "$2" || BUG "test_cmp '$2' missing"
-		return 1
-	fi
+	eval "$GIT_TEST_CMP" '"$@"'
 }
 
 # Check that the given config key has the expected value.
@@ -940,13 +934,7 @@ test_cmp_config() {
 # test_cmp_bin - helper to compare binary files
 
 test_cmp_bin() {
-	test $# -eq 2 || BUG "test_cmp_bin requires two arguments"
-	if ! cmp "$@"
-	then
-		test "x$1" = x- || test -e "$1" || BUG "test_cmp_bin '$1' missing"
-		test "x$2" = x- || test -e "$2" || BUG "test_cmp_bin '$2' missing"
-		return 1
-	fi
+	cmp "$@"
 }
 
 # Use this instead of test_cmp to compare files that contain expected and