Re: [PATCH] test_cmp: diagnose incorrect arguments more precisely

[Junio C Hamano <gitster@pobox.com>]

Eric Sunshine <sunshine@sunshineco.com> writes:

> d572f52a64 (test_cmp: diagnose incorrect arguments, 2020-08-09) taught
> test_cmp() and test_cmp_bin() to diagnose a missing input source. Even
> though the arguments to test_cmp() must name regular files (or standard
> input), it only diagnoses whether a source is missing, which makes the
> check a bit loose. Teach the check to be more precise by diagnosing, not
> only a missing source, but also if the source is not a regular file.
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---
>
>   On Wed, Sep 16, 2020 at 5:14 PM Junio C Hamano <gitster@pobox.com> wrote:
>   > Eric Sunshine <sunshine@sunshineco.com> writes:
>   > > [...] I ask because test_cmp() was updated not long ago to
>   > > provide better diagnostics when one of the files is missing.
>   > > [1]: d572f52a64 (test_cmp: diagnose incorrect arguments, 2020-08-09)
>   >
>   > Yes, you did this with the commit,
>   >         test "x$1" = x- || test -e "$1" || BUG "test_cmp '$1' missing"
>   >         test "x$2" = x- || test -e "$2" || BUG "test_cmp '$2' missing"
>   > and I do not immediately see why "test -e" shouldn't be "test -f".
>   > It should ideally be "stdin is OK, otherwise it must be a readable
>   > regular file".
>
>   Perhaps the present patch suitably address your concern?

Certainly s/-e/-f/ is better to reject directories, but if we are
doing more checks, it would probably be more maintainable to have
a helper and catch unreadable files.

Note that

	mkdir one two &&
	test_cmp one two

would likely succeed, so I am not sure how much this matters in
practice, even though we would catch

	mkdir one >two &&
	test_cmp one two

as a mistake.

 t/test-lib-functions.sh | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8d59b90348..ddaa14275a 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -938,6 +938,20 @@ test_expect_code () {
 	return 1
 } 7>&2 2>&4
 
+# helper for test_cmp and test_cmp_bin to diagnose programmer errors
+# usage: test_cmp_missing "$funcname" "$filename"
+test_cmp_missing_check () {
+	if test "x$2" = x-
+	then
+		:; # standard input is OK
+	elif test -f "$2" && test -r "$2"
+	then
+		:; # readable file is OK
+	else
+		BUG "$1 '$2' not a readable file"
+	fi
+}
+
 # test_cmp is a helper function to compare actual and expected output.
 # You can use it like:
 #
@@ -955,9 +969,9 @@ 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
+		test_cmp_missing_check test_cmp "$1"
+		test_cmp_missing_check test_cmp "$2"
+		return 1;
 	fi
 }
 
@@ -990,8 +1004,8 @@ 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"
+		test_cmp_missing_check test_cmp_bin "$1"
+		test_cmp_missing_check test_cmp_bin "$2"
 		return 1
 	fi
 }