[PATCH v2] test_cmp: diagnose incorrect arguments

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 9, 2020, 17:42 UTC
Message-ID: <20200809174209.15466-1-sunshine@sunshineco.com>
In-Reply-To: <20200809060810.31370-1-sunshine@sunshineco.com>

Under normal circumstances, if a test author misspells a filename passed to test_cmp(), the error is quickly discovered when the test fails unexpectedly due to test_cmp() being unable to find the file. However, if the test is expected to fail, as with test_expect_failure(), a misspelled filename as argument to test_cmp() will go unnoticed since the test will indeed fail, but for the wrong reason. Make it easier for test authors to discover such problems early by sanity-checking the arguments to test_cmp(). To avoid penalizing all clients of test_cmp() in the general case, only check for missing files if the comparison fails.

While at it, make test_cmp_bin() sanity-check its arguments, as well.

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---

Notes:
    This is a re-roll of [1] which was motivated by seeing Elijah fix[2]
    several cases of bogus test_cmp() calls which perhaps could have
    been detected earlier.
    
    Changes since v1:
    
    * take into account that some callers pass "-" (meaning standard
      input) as an argument to test_cmp() (pointed out privately by
      Junio)
    
    * show the name of the missing file rather than a placeholder
      (Shourya[3])
    
    [1]: https://lore.kernel.org/git/20200809060810.31370-1-sunshine@sunshineco.com/
    [2]: https://lore.kernel.org/git/7f408b7d4069403b969d334f4940ebf87f1dc797.1596906081.git.gitgitgadget@gmail.com/
    [3]: https://lore.kernel.org/git/20200809083227.GA11219@konoha/

Interdiff against v1:
  diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
  index 8d77deebd2..a12d6a3fc9 100644
  --- a/t/test-lib-functions.sh
  +++ b/t/test-lib-functions.sh
  @@ -955,8 +955,8 @@ test_cmp() {
   	test $# -eq 2 || BUG "test_cmp requires two arguments"
   	if ! eval "$GIT_TEST_CMP" '"$@"'
   	then
  -		test -e "$1" || BUG "test_cmp 'expect' file missing"
  -		test -e "$2" || BUG "test_cmp 'actual' file missing"
  +		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
   }
  @@ -990,8 +990,8 @@ test_cmp_bin() {
   	test $# -eq 2 || BUG "test_cmp_bin requires two arguments"
   	if ! cmp "$@"
   	then
  -		test -e "$1" || BUG "test_cmp_bin 'expect' file missing"
  -		test -e "$2" || BUG "test_cmp_bin 'actual' file missing"
  +		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
   }

 t/test-lib-functions.sh | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index b791933ffd..a12d6a3fc9 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -952,7 +952,13 @@ test_expect_code () {
 # - not all diff versions understand "-u"
 
 test_cmp() {
-	eval "$GIT_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
 }
 
 # Check that the given config key has the expected value.
@@ -981,7 +987,13 @@ test_cmp_config() {
 # test_cmp_bin - helper to compare binary files
 
 test_cmp_bin() {
-	cmp "$@"
+	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
 }
 
 # Use this instead of test_cmp to compare files that contain expected and

-- 
2.28.0.220.ged08abb693