threads / patch / 54010

patchtest_cmp: diagnose incorrect arguments

Subject: [PATCH] test_cmp: diagnose incorrect arguments

## tl;dr

20 messages between Aug 9, 2020 and Oct 17, 2020. Diffs are folded; open one to read it.

replies: 19people: 5as markdown or json

Eric Sunshine· Aug 9, 2020, 06:08 UTC · lore

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 change was motivated by seeing Elijah's patch[1] which fixed
    several cases of bogus test_cmp() invocations which perhaps could
    have been discovered earlier had test_cmp() done some
    sanity-checking of its arguments. It turns out that the tests in
    question[1] fail before test_cmp() is ever called, so this patch
    would not have helped catch those mistakes after all. It also only
    helps catch mistakes in test_expect_failure() cases, which are
    relatively rare compared with test_expect_success() cases, thus may
    not have a lot of value. Nevertheless, it's a relatively small and
    simple change which only kicks in when test_cmp() fails, thus should
    not penalize the typical case; there are a handful of `! test_cmp`
    cases, however, which will trigger the additional argument sanity
    checking but that cost is probably small enough to go unnoticed.
    
    [1]: https://lore.kernel.org/git/7f408b7d4069403b969d334f4940ebf87f1dc797.1596906081.git.gitgitgadget@gmail.com/
 t/test-lib-functions.sh | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)
Show changes to t/test-lib-functions.sh +14 −2
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index b791933ffd..8d77deebd2 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 -e "$1" || BUG "test_cmp 'expect' file missing"
+		test -e "$2" || BUG "test_cmp 'actual' file 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 -e "$1" || BUG "test_cmp_bin 'expect' file missing"
+		test -e "$2" || BUG "test_cmp_bin 'actual' file missing"
+		return 1
+	fi
 }
 
 # Use this instead of test_cmp to compare files that contain expected and

-- 
2.28.0.213.gdd9653da77
Shourya Shukla· Aug 9, 2020, 08:32 UTC · re: Eric Sunshine · lore

Re: [PATCH] test_cmp: diagnose incorrect arguments

Hello Eric,
Show 10 quoted lines
>  test_cmp() {
> -	eval "$GIT_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"
> +		return 1
> +	fi
>  }

I reckon we could be just a little bit more precise here by bugging out with the exact filename which is missing instead of 'expect' or 'actual' so that the user has more idea as to what happened. What do you think?

BTW, I looked up the 'test_i18ncmp' function as well and if we have this small loophole you mentioned above, I think maybe we could make a similar fix for it too. What I mean is that in case of absence of the required locale, it should error out kind of like what we did above

    BUG "locale missing"

so that the user it is clear to the user what was the failure point. Though I will be honest that I have not really encountered a locale related error or seen what the error looks like, so maybe we can ignore this suggestion.

Regards, Shourya Shukla

Eric Sunshine· Aug 9, 2020, 08:49 UTC · re: Shourya Shukla · lore

Re: [PATCH] test_cmp: diagnose incorrect arguments

On Sun, Aug 9, 2020 at 4:32 AM Shourya Shukla <shouryashukla.oo@gmail.com> wrote:

Show 6 quoted lines
> > +       test -e "$1" || BUG "test_cmp 'expect' file missing"
> > +       test -e "$2" || BUG "test_cmp 'actual' file missing"
>
> I reckon we could be just a little bit more precise here by bugging out
> with the exact filename which is missing instead of 'expect' or 'actual'
> so that the user has more idea as to what happened. What do you think?

Good idea. I'm planning on re-rolling anyhow since Junio pointed out privately that some callers use "-" (meaning standard input) as one of the arguments to test_cmp(), so that case ought to be taken into account, as well.

Show 8 quoted lines
> BTW, I looked up the 'test_i18ncmp' function as well and if we have
> this small loophole you mentioned above, I think maybe we could make a
> similar fix for it too. What I mean is that in case of absence of the
> required locale, it should error out kind of like what we did above
>
>   BUG "locale missing"
>
> so that the user it is clear to the user what was the failure point.

Sorry, I'm not really sure what you are suggesting, but I'm guessing that you're misunderstanding of the purpose of test_i18ncmp:

    test_i18ncmp () {
        ! test_have_prereq C_LOCALE_OUTPUT || test_cmp "$@"
    }

which says that test_cmp() should only be called if the current locale is "C"; in all other cases, we specifically do not want test_cmp() to be called and instead simply pretend that the test passed.

Eric Sunshine· Aug 9, 2020, 17:42 UTC · re: Eric Sunshine · lore

[PATCH v2] test_cmp: diagnose incorrect arguments

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(-)
Show changes to t/test-lib-functions.sh +14 −2
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
Junio C Hamano· Aug 9, 2020, 19:12 UTC · re: Eric Sunshine · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 15 quoted lines
> 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>
> ---
The motivation makes quite a lot of sense.

I suspect that it is a bug to use "-", i.e. "read from the standard input", for "$2", because

 (1) if it is used for the expected output, we got the comparison in
     the wrong direction;
 (2) if it is used for the actual output, we have a command whose
     output we are interested in on the upstream side of a pipe to
     discard its exit status.
but I'll let it pass.
Show 34 quoted lines
> 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
Eric Sunshine· Aug 9, 2020, 19:34 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

On Sun, Aug 9, 2020 at 3:12 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
> The motivation makes quite a lot of sense.
>
> I suspect that it is a bug to use "-", i.e. "read from the standard
> input", for "$2", because
>
>  (1) if it is used for the expected output, we got the comparison in
>      the wrong direction;

I had the same concern that it didn't necessarily make sense to allow "-" for $2, but eventually thought of a case such as:

    sed '...' actual | test_cmp expect - &&

which massages 'actual' before test_cmp() sees it. True, we don't actually have such callers, but it theoretically is legitimate.

>  (2) if it is used for the actual output, we have a command whose
>      output we are interested in on the upstream side of a pipe to
>      discard its exit status.

If the command upstream is a Git command, that could indeed be a reason for concern, but, as illustrated above, it could just be a system command.

Junio C Hamano· Aug 10, 2020, 15:22 UTC · re: Eric Sunshine · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 7 quoted lines
> I had the same concern that it didn't necessarily make sense to allow
> "-" for $2, but eventually thought of a case such as:
>
>     sed '...' actual | test_cmp expect - &&
>
> which massages 'actual' before test_cmp() sees it. True, we don't
> actually have such callers, but it theoretically is legitimate.

Yup, that looks a bit too stretching [*] to me, but that was what I had in mind when I said "I'll let it pass".

    Side note.  Presumably that 'actual' was written by Git, to
    avoid losing its exit code, e.g.
	git frotz >actual &&
	sed '...' actual | test_cmp expect -
    but then it becomes more natural to write the second one like
    so:
	git frotz >raw &&
	sed '...' raw >actual &&
	test_cmp expect actual
Taylor Blau· Aug 11, 2020, 18:32 UTC · re: Eric Sunshine · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

On Sun, Aug 09, 2020 at 01:42:09PM -0400, Eric Sunshine wrote:
Show 79 quoted lines
> 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"

Not related to your patch, but I've seen this style of "x$1" in a few places in test-lib-functions.sh. Why can't this be written as 'test "$1" = -'?

Show 23 quoted lines
> +		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
>

Thanks, Taylor

Eric Sunshine· Aug 11, 2020, 19:25 UTC · re: Taylor Blau · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

On Tue, Aug 11, 2020 at 2:33 PM Taylor Blau <me@ttaylorr.com> wrote:
Show 6 quoted lines
> On Sun, Aug 09, 2020 at 01:42:09PM -0400, Eric Sunshine wrote:
> > +             test "x$2" = x- || test -e "$2" || BUG "test_cmp '$2' missing"
>
> Not related to your patch, but I've seen this style of "x$1" in a few
> places in test-lib-functions.sh. Why can't this be written as 'test "$1"
> = -'?
Short answer: To prevent 'test' from thinking that the argument is a switch.
Longer answer:

'test' can accept both switches (i.e. "-e") and non-switch arguments. Keep in mind, too, that all the quoting is stripped by the shell _before_ 'test' ever sees its arguments. Let's say that the caller has a filename whose name actually is "-e" and passes that in as $1. So, what does 'test' see?

    test -e = -

Rather than comparing literal string "-e" to literal string "-", it's instead (almost) asking if the file named "=" exists; I say "almost" because it's actually an error since switch -e only accepts one argument, but it's being given two arguments, "=" and "-".

You might say that having a file named "-e" (or similar) is unlikely, however, what is not unlikely is a caller passing "-" for standard-input as $1. In this case, 'test' sees:

    test - = -

which may or may not be an error in a particular implementation of 'test'. Some implementations may understand that "-" is not a valid switch, thus infer that you're actually asking for an equality comparison between arguments, but other implementations may complain either that there is no switch named "-" or that those arguments simply make no sense.

This is why it's a very common idiom in shell programming with 'test' to see "x" prepended, thus ensuring that the argument can't be confused with a switch.

Taylor Blau· Aug 11, 2020, 21:03 UTC · re: Eric Sunshine · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

On Tue, Aug 11, 2020 at 03:25:03PM -0400, Eric Sunshine wrote:
Show 9 quoted lines
> On Tue, Aug 11, 2020 at 2:33 PM Taylor Blau <me@ttaylorr.com> wrote:
> > On Sun, Aug 09, 2020 at 01:42:09PM -0400, Eric Sunshine wrote:
> > > +             test "x$2" = x- || test -e "$2" || BUG "test_cmp '$2' missing"
> >
> > Not related to your patch, but I've seen this style of "x$1" in a few
> > places in test-lib-functions.sh. Why can't this be written as 'test "$1"
> > = -'?
>
> Short answer: To prevent 'test' from thinking that the argument is a switch.
Makes sense. Now I feel silly for asking :).
Show 31 quoted lines
> Longer answer:
>
> 'test' can accept both switches (i.e. "-e") and non-switch arguments.
> Keep in mind, too, that all the quoting is stripped by the shell
> _before_ 'test' ever sees its arguments. Let's say that the caller has
> a filename whose name actually is "-e" and passes that in as $1. So,
> what does 'test' see?
>
>     test -e = -
>
> Rather than comparing literal string "-e" to literal string "-", it's
> instead (almost) asking if the file named "=" exists; I say "almost"
> because it's actually an error since switch -e only accepts one
> argument, but it's being given two arguments, "=" and "-".
>
> You might say that having a file named "-e" (or similar) is unlikely,
> however, what is not unlikely is a caller passing "-" for
> standard-input as $1. In this case, 'test' sees:
>
>     test - = -
>
> which may or may not be an error in a particular implementation of
> 'test'. Some implementations may understand that "-" is not a valid
> switch, thus infer that you're actually asking for an equality
> comparison between arguments, but other implementations may complain
> either that there is no switch named "-" or that those arguments
> simply make no sense.
>
> This is why it's a very common idiom in shell programming with 'test'
> to see "x" prepended, thus ensuring that the argument can't be
> confused with a switch.

Thanks for a careful and helpful explanation, as always :-). Makes sense to me.

Thanks, Taylor

Jeff King· Aug 12, 2020, 15:37 UTC · re: Eric Sunshine · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

On Tue, Aug 11, 2020 at 03:25:03PM -0400, Eric Sunshine wrote:
Show 12 quoted lines
> 'test' can accept both switches (i.e. "-e") and non-switch arguments.
> Keep in mind, too, that all the quoting is stripped by the shell
> _before_ 'test' ever sees its arguments. Let's say that the caller has
> a filename whose name actually is "-e" and passes that in as $1. So,
> what does 'test' see?
> 
>     test -e = -
> 
> Rather than comparing literal string "-e" to literal string "-", it's
> instead (almost) asking if the file named "=" exists; I say "almost"
> because it's actually an error since switch -e only accepts one
> argument, but it's being given two arguments, "=" and "-".

I don't think this is an error. The program can tell which you meant by the number of arguments. POSIX lays out some rules here (from "man 1posix test" on my system, but I'm sure you can find it online):

  3 arguments:
    *  If $2 is a binary primary, perform the binary test of $1 and $3.
    *  If $1 is '!', negate the two-argument test of $2 and $3.
    *  If $1 is '(' and $3 is ')', perform the unary test of $2.  On
       systems that do not support the XSI option, the results are
       unspecified if $1 is '(' and $3 is ')'.
    *  Otherwise, produce unspecified results.

So we'd see that "=" is a binary primary (the complete set is defined earlier). Likewise "! -e -" would hit the second rule. We wouldn't get fooled by trying to compare the string "!" because it knows that "=" is a binary operator and "-e" is a unary operator.

It gets weird with "-a" joining expressions. There's some discussion in the Rationale section of the posix page, and it even warns explicitly against "-a" (in favor of "test expr1 && test expr1").

Show 6 quoted lines
> which may or may not be an error in a particular implementation of
> 'test'. Some implementations may understand that "-" is not a valid
> switch, thus infer that you're actually asking for an equality
> comparison between arguments, but other implementations may complain
> either that there is no switch named "-" or that those arguments
> simply make no sense.

Yeah, historically I think there were shells that were not quite so clever. I have no idea which ones, or whether any are still around. I don't think we've generally been that concerned with this case in Git, and nobody has complained. I'd be totally unsurprised to hear that SunOS /bin/sh doesn't get this right, but we've already written it off as unusable (it doesn't even support $() expansion).

-Peff
Eric Sunshine· Aug 12, 2020, 16:15 UTC · re: Jeff King · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

On Wed, Aug 12, 2020 at 11:37 AM Jeff King <peff@peff.net> wrote:
Show 10 quoted lines
> On Tue, Aug 11, 2020 at 03:25:03PM -0400, Eric Sunshine wrote:
> >     test -e = -
> > Rather than comparing literal string "-e" to literal string "-", it's
> > instead (almost) asking if the file named "=" exists; I say "almost"
> > because it's actually an error since switch -e only accepts one
> > argument, but it's being given two arguments, "=" and "-".
>
> I don't think this is an error. The program can tell which you meant by
> the number of arguments. POSIX lays out some rules here (from "man
> 1posix test" on my system, but I'm sure you can find it online):

I intentionally didn't focus on or mention POSIX in my response because I wanted to represent the Real World reason why "x$var" is such a common idiom. As a person who regularly uses ancient operating systems and old computers (it's common for me to be stuck on computers 10-25 years old; my most up-to-date computer is 9.5 years old), Real World is something I focus on more than POSIX. Okay, I may be an outlier, but still...

Show 13 quoted lines
> > which may or may not be an error in a particular implementation of
> > 'test'. Some implementations may understand that "-" is not a valid
> > switch, thus infer that you're actually asking for an equality
> > comparison between arguments, but other implementations may complain
> > either that there is no switch named "-" or that those arguments
> > simply make no sense.
>
> Yeah, historically I think there were shells that were not quite so
> clever. I have no idea which ones, or whether any are still around. I
> don't think we've generally been that concerned with this case in Git,
> and nobody has complained. I'd be totally unsurprised to hear that SunOS
> /bin/sh doesn't get this right, but we've already written it off as
> unusable (it doesn't even support $() expansion).

I'm probably showing my age, but having had to deal with the many inconsistencies of implementation over the years, it's hard to _not_ worry about such things even now. Adding the "x" to "$1" or to making other minor portability concessions is reflex at this point. These portability concerns are also always on mind when dealing with Mac OS since so many of its utilities are ancient and retain the old behaviors.

Eric Sunshine· Aug 12, 2020, 16:39 UTC · re: Eric Sunshine · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

On Wed, Aug 12, 2020 at 12:15 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 8 quoted lines
> On Wed, Aug 12, 2020 at 11:37 AM Jeff King <peff@peff.net> wrote:
> > I don't think this is an error. The program can tell which you meant by
> > the number of arguments. POSIX lays out some rules here (from "man
> > 1posix test" on my system, but I'm sure you can find it online):
>
> I intentionally didn't focus on or mention POSIX in my response
> because I wanted to represent the Real World reason why "x$var" is
> such a common idiom. [...]

I probably should have done a better job in my original response to Taylor to make it clear that I was talking about Real World (even if old) rather than POSIX.

Jeff King· Aug 12, 2020, 17:10 UTC · re: Eric Sunshine · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

On Wed, Aug 12, 2020 at 12:39:14PM -0400, Eric Sunshine wrote:
Show 13 quoted lines
> On Wed, Aug 12, 2020 at 12:15 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > On Wed, Aug 12, 2020 at 11:37 AM Jeff King <peff@peff.net> wrote:
> > > I don't think this is an error. The program can tell which you meant by
> > > the number of arguments. POSIX lays out some rules here (from "man
> > > 1posix test" on my system, but I'm sure you can find it online):
> >
> > I intentionally didn't focus on or mention POSIX in my response
> > because I wanted to represent the Real World reason why "x$var" is
> > such a common idiom. [...]
> 
> I probably should have done a better job in my original response to
> Taylor to make it clear that I was talking about Real World (even if
> old) rather than POSIX.

Yeah. I guess I'm questioning how current that Real World view is. It hasn't bitten us yet, though we do seem to do the "x" thing in some places. And most of our shell code is in the test suite, which sees pretty tame filenames.

-Peff
Jeff King· Oct 16, 2020, 00:17 UTC · re: Eric Sunshine · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

On Sun, Aug 09, 2020 at 01:42:09PM -0400, Eric Sunshine wrote:
Show 10 quoted lines
> 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.
This patch caused some interesting confusion for me today.

I was looking at the patch from [1] which causes a test failure (and I wanted to see where it failed, etc). And I got:

  $ ./t5601-clone.sh
  ok 1 - setup
  ok 2 - clone with excess parameters (1)
  ok 3 - clone with excess parameters (2)
  ok 4 - output from clone
  ok 5 - clone does not keep pack
  ok 6 - clone checks out files
  ok 7 - clone respects GIT_WORK_TREE
  error: bug in the test script: test_cmp 'r2/.git/HEAD' missing

which was somewhat unhelpful (or at least less helpful than a regular test failure). The test in question does this:

	test_cmp r0/.git/HEAD r2/.git/HEAD &&

and expects to fail if an earlier step didn't correctly create r2. Is it a bug or misuse of test_cmp for it to do so? I could see an argument that it is, but I'm also not sure if there's a convenient alternative. The best I could come up with is:

  test_path_is_file r2/.git/HEAD &&
  test_cmp r0/.git/HEAD r2/.git/HEAD
which isn't that great.
-Peff
[1] https://lore.kernel.org/git/20200130102933.GE840531@coredump.intra.peff.net/
Eric Sunshine· Oct 16, 2020, 02:18 UTC · re: Jeff King · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

On Thu, Oct 15, 2020 at 8:17 PM Jeff King <peff@peff.net> wrote:
Show 12 quoted lines
> On Sun, Aug 09, 2020 at 01:42:09PM -0400, Eric Sunshine wrote:
> > [...] Make it easier for test authors to discover such problems early
> > by sanity-checking the arguments to test_cmp(). [...]
>
> This patch caused some interesting confusion for me today.
>   error: bug in the test script: test_cmp 'r2/.git/HEAD' missing
> which was somewhat unhelpful (or at least less helpful than a regular
> test failure). The test in question does this:
>         test_cmp r0/.git/HEAD r2/.git/HEAD &&
> and expects to fail if an earlier step didn't correctly create r2. Is it
> a bug or misuse of test_cmp for it to do so? I could see an argument
> that it is, but I'm also not sure if there's a convenient alternative.

I can see the argument going both ways as to whether it's a misuse of 'test_cmp'.

Show 6 quoted lines
> The best I could come up with is:
>
>   test_path_is_file r2/.git/HEAD &&
>   test_cmp r0/.git/HEAD r2/.git/HEAD
>
> which isn't that great.

Æ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.

[1]: https://lore.kernel.org/git/20200921104000.2304-15-avarab@gmail.com/ [2]: https://lore.kernel.org/git/20200809060810.31370-1-sunshine@sunshineco.com/ [3]: https://lore.kernel.org/git/7f408b7d4069403b969d334f4940ebf87f1dc797.1596906081.git.gitgitgadget@gmail.com/

Junio C Hamano· Oct 16, 2020, 18:36 UTC · re: Eric Sunshine · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 23 quoted lines
> On Thu, Oct 15, 2020 at 8:17 PM Jeff King <peff@peff.net> wrote:
>> On Sun, Aug 09, 2020 at 01:42:09PM -0400, Eric Sunshine wrote:
>> > [...] Make it easier for test authors to discover such problems early
>> > by sanity-checking the arguments to test_cmp(). [...]
>>
>> This patch caused some interesting confusion for me today.
>>   error: bug in the test script: test_cmp 'r2/.git/HEAD' missing
>> which was somewhat unhelpful (or at least less helpful than a regular
>> test failure). The test in question does this:
>>         test_cmp r0/.git/HEAD r2/.git/HEAD &&
>> and expects to fail if an earlier step didn't correctly create r2. Is it
>> a bug or misuse of test_cmp for it to do so? I could see an argument
>> that it is, but I'm also not sure if there's a convenient alternative.
>
> I can see the argument going both ways as to whether it's a misuse of
> 'test_cmp'.
>
>> The best I could come up with is:
>>
>>   test_path_is_file r2/.git/HEAD &&
>>   test_cmp r0/.git/HEAD r2/.git/HEAD
>>
>> which isn't that great.

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.

Show 7 quoted lines
> Æ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?
Junio C Hamano· Oct 16, 2020, 20:56 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

Junio C Hamano <gitster@pobox.com> writes:
Show 13 quoted lines
> 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?

For now I've queued this directly on top of the offending change and merged the result in 'seen', perhaps to be advanced to 'next' and 'master' after the 2.29 final unless a better version comes.

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(-)
Show changes to t/test-lib-functions.sh +2 −14
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
Junio C Hamano· Oct 16, 2020, 23:14 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

Junio C Hamano <gitster@pobox.com> writes:
Show 25 quoted lines
>  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 "$@"
>  }

Looking at this again, I think we could keep the "we should have two arguments, no more than, no less than, but exactly two". But I think those who write new tests are working to eventually make them pass, so hopefully they'll notice and investigate test_cmp that yields false anyway, I guess.

Eric Sunshine· Oct 17, 2020, 06:06 UTC · re: Junio C Hamano · lore

Re: [PATCH v2] test_cmp: diagnose incorrect arguments

On Fri, Oct 16, 2020 at 7:14 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> >  test_cmp() {
> > -     test $# -eq 2 || BUG "test_cmp requires two arguments"
> > -     if ! eval "$GIT_TEST_CMP" '"$@"'
>
> Looking at this again, I think we could keep the "we should have two
> arguments, no more than, no less than, but exactly two".  But I think
> those who write new tests are working to eventually make them pass,
> so hopefully they'll notice and investigate test_cmp that yields false
> anyway, I guess.

The purpose of the change in question[1] was specifically to help catch bugs in the test_expect_failure() case, which is quite rare in the Git test suite (and hopefully becomes rarer over time). So, I think it's fine to revert the change fully, including the 2-argument check since, as you say, test writers are normally trying to make their tests pass, and the 2-argument check doesn't provide much, if any, value for the test_expect_success() case.

[1]: d572f52a64 (test_cmp: diagnose incorrect arguments, 2020-08-09)

← back to recent threads