{"thread":{"id":"54010","subject":"[PATCH] test_cmp: diagnose incorrect arguments","startedAt":"2020-08-09T06:09:07Z","lastAt":"2020-10-17T06:07:08Z","messageCount":20,"participants":["Eric Sunshine","Shourya Shukla","Junio C Hamano","Taylor Blau","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"403230","messageId":"20200809060810.31370-1-sunshine@sunshineco.com","threadId":"54010","inReplyTo":null,"subject":"[PATCH] test_cmp: diagnose incorrect arguments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-09T06:08:10Z","receivedAt":"2020-08-09T06:09:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Under normal circumstances, if a test author misspells a filename passed\nto test_cmp(), the error is quickly discovered when the test fails\nunexpectedly due to test_cmp() being unable to find the file. However,\nif the test is expected to fail, as with test_expect_failure(), a\nmisspelled filename as argument to test_cmp() will go unnoticed since\nthe test will indeed fail, but for the wrong reason. Make it easier for\ntest authors to discover such problems early by sanity-checking the\narguments to test_cmp(). To avoid penalizing all clients of test_cmp()\nin the general case, only check for missing files if the comparison\nfails.\n\nWhile at it, make test_cmp_bin() sanity-check its arguments, as well.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n\nNotes:\n    This change was motivated by seeing Elijah's patch[1] which fixed\n    several cases of bogus test_cmp() invocations which perhaps could\n    have been discovered earlier had test_cmp() done some\n    sanity-checking of its arguments. It turns out that the tests in\n    question[1] fail before test_cmp() is ever called, so this patch\n    would not have helped catch those mistakes after all. It also only\n    helps catch mistakes in test_expect_failure() cases, which are\n    relatively rare compared with test_expect_success() cases, thus may\n    not have a lot of value. Nevertheless, it's a relatively small and\n    simple change which only kicks in when test_cmp() fails, thus should\n    not penalize the typical case; there are a handful of `! test_cmp`\n    cases, however, which will trigger the additional argument sanity\n    checking but that cost is probably small enough to go unnoticed.\n    \n    [1]: https://lore.kernel.org/git/7f408b7d4069403b969d334f4940ebf87f1dc797.1596906081.git.gitgitgadget@gmail.com/\n\n t/test-lib-functions.sh | 16 ++++++++++++++--\n 1 file changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex b791933ffd..8d77deebd2 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -952,7 +952,13 @@ test_expect_code () {\n # - not all diff versions understand \"-u\"\n \n test_cmp() {\n-\teval \"$GIT_TEST_CMP\" '\"$@\"'\n+\ttest $# -eq 2 || BUG \"test_cmp requires two arguments\"\n+\tif ! eval \"$GIT_TEST_CMP\" '\"$@\"'\n+\tthen\n+\t\ttest -e \"$1\" || BUG \"test_cmp 'expect' file missing\"\n+\t\ttest -e \"$2\" || BUG \"test_cmp 'actual' file missing\"\n+\t\treturn 1\n+\tfi\n }\n \n # Check that the given config key has the expected value.\n@@ -981,7 +987,13 @@ test_cmp_config() {\n # test_cmp_bin - helper to compare binary files\n \n test_cmp_bin() {\n-\tcmp \"$@\"\n+\ttest $# -eq 2 || BUG \"test_cmp_bin requires two arguments\"\n+\tif ! cmp \"$@\"\n+\tthen\n+\t\ttest -e \"$1\" || BUG \"test_cmp_bin 'expect' file missing\"\n+\t\ttest -e \"$2\" || BUG \"test_cmp_bin 'actual' file missing\"\n+\t\treturn 1\n+\tfi\n }\n \n # Use this instead of test_cmp to compare files that contain expected and\n-- \n2.28.0.213.gdd9653da77\n\n"},{"id":"403231","messageId":"20200809083227.GA11219@konoha","threadId":"54010","inReplyTo":"20200809060810.31370-1-sunshine@sunshineco.com","subject":"Re: [PATCH] test_cmp: diagnose incorrect arguments","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2020-08-09T08:32:27Z","receivedAt":"2020-08-09T08:32:51Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Hello Eric,\n\n>  test_cmp() {\n> -\teval \"$GIT_TEST_CMP\" '\"$@\"'\n> +\ttest $# -eq 2 || BUG \"test_cmp requires two arguments\"\n> +\tif ! eval \"$GIT_TEST_CMP\" '\"$@\"'\n> +\tthen\n> +\t\ttest -e \"$1\" || BUG \"test_cmp 'expect' file missing\"\n> +\t\ttest -e \"$2\" || BUG \"test_cmp 'actual' file missing\"\n> +\t\treturn 1\n> +\tfi\n>  }\n\nI reckon we could be just a little bit more precise here by bugging out\nwith the exact filename which is missing instead of 'expect' or 'actual'\nso that the user has more idea as to what happened. What do you think?\n\nBTW, I looked up the 'test_i18ncmp' function as well and if we have\nthis small loophole you mentioned above, I think maybe we could make a\nsimilar fix for it too. What I mean is that in case of absence of the\nrequired locale, it should error out kind of like what we did above\n\n    BUG \"locale missing\"\n\n\nso that the user it is clear to the user what was the failure point.\nThough I will be honest that I have not really encountered a locale related\nerror or seen what the error looks like, so maybe we can ignore this\nsuggestion.\n\nRegards,\nShourya Shukla\n\n"},{"id":"403232","messageId":"CAPig+cT3Mwy6gJE3=5P78xwmFSp+gJYazboZtrkpFy7v8X7K2A@mail.gmail.com","threadId":"54010","inReplyTo":"20200809083227.GA11219@konoha","subject":"Re: [PATCH] test_cmp: diagnose incorrect arguments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-09T08:49:09Z","receivedAt":"2020-08-09T08:49:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 9, 2020 at 4:32 AM Shourya Shukla\n<shouryashukla.oo@gmail.com> wrote:\n> > +       test -e \"$1\" || BUG \"test_cmp 'expect' file missing\"\n> > +       test -e \"$2\" || BUG \"test_cmp 'actual' file missing\"\n>\n> I reckon we could be just a little bit more precise here by bugging out\n> with the exact filename which is missing instead of 'expect' or 'actual'\n> so that the user has more idea as to what happened. What do you think?\n\nGood idea. I'm planning on re-rolling anyhow since Junio pointed out\nprivately that some callers use \"-\" (meaning standard input) as one of\nthe arguments to test_cmp(), so that case ought to be taken into\naccount, as well.\n\n> BTW, I looked up the 'test_i18ncmp' function as well and if we have\n> this small loophole you mentioned above, I think maybe we could make a\n> similar fix for it too. What I mean is that in case of absence of the\n> required locale, it should error out kind of like what we did above\n>\n>   BUG \"locale missing\"\n>\n> so that the user it is clear to the user what was the failure point.\n\nSorry, I'm not really sure what you are suggesting, but I'm guessing\nthat you're misunderstanding of the purpose of test_i18ncmp:\n\n    test_i18ncmp () {\n        ! test_have_prereq C_LOCALE_OUTPUT || test_cmp \"$@\"\n    }\n\nwhich says that test_cmp() should only be called if the current locale\nis \"C\"; in all other cases, we specifically do not want test_cmp() to\nbe called and instead simply pretend that the test passed.\n"},{"id":"403234","messageId":"20200809174209.15466-1-sunshine@sunshineco.com","threadId":"54010","inReplyTo":"20200809060810.31370-1-sunshine@sunshineco.com","subject":"[PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-09T17:42:09Z","receivedAt":"2020-08-09T17:43:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Under normal circumstances, if a test author misspells a filename passed\nto test_cmp(), the error is quickly discovered when the test fails\nunexpectedly due to test_cmp() being unable to find the file. However,\nif the test is expected to fail, as with test_expect_failure(), a\nmisspelled filename as argument to test_cmp() will go unnoticed since\nthe test will indeed fail, but for the wrong reason. Make it easier for\ntest authors to discover such problems early by sanity-checking the\narguments to test_cmp(). To avoid penalizing all clients of test_cmp()\nin the general case, only check for missing files if the comparison\nfails.\n\nWhile at it, make test_cmp_bin() sanity-check its arguments, as well.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n\nNotes:\n    This is a re-roll of [1] which was motivated by seeing Elijah fix[2]\n    several cases of bogus test_cmp() calls which perhaps could have\n    been detected earlier.\n    \n    Changes since v1:\n    \n    * take into account that some callers pass \"-\" (meaning standard\n      input) as an argument to test_cmp() (pointed out privately by\n      Junio)\n    \n    * show the name of the missing file rather than a placeholder\n      (Shourya[3])\n    \n    [1]: https://lore.kernel.org/git/20200809060810.31370-1-sunshine@sunshineco.com/\n    [2]: https://lore.kernel.org/git/7f408b7d4069403b969d334f4940ebf87f1dc797.1596906081.git.gitgitgadget@gmail.com/\n    [3]: https://lore.kernel.org/git/20200809083227.GA11219@konoha/\n\nInterdiff against v1:\n  diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n  index 8d77deebd2..a12d6a3fc9 100644\n  --- a/t/test-lib-functions.sh\n  +++ b/t/test-lib-functions.sh\n  @@ -955,8 +955,8 @@ test_cmp() {\n   \ttest $# -eq 2 || BUG \"test_cmp requires two arguments\"\n   \tif ! eval \"$GIT_TEST_CMP\" '\"$@\"'\n   \tthen\n  -\t\ttest -e \"$1\" || BUG \"test_cmp 'expect' file missing\"\n  -\t\ttest -e \"$2\" || BUG \"test_cmp 'actual' file missing\"\n  +\t\ttest \"x$1\" = x- || test -e \"$1\" || BUG \"test_cmp '$1' missing\"\n  +\t\ttest \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp '$2' missing\"\n   \t\treturn 1\n   \tfi\n   }\n  @@ -990,8 +990,8 @@ test_cmp_bin() {\n   \ttest $# -eq 2 || BUG \"test_cmp_bin requires two arguments\"\n   \tif ! cmp \"$@\"\n   \tthen\n  -\t\ttest -e \"$1\" || BUG \"test_cmp_bin 'expect' file missing\"\n  -\t\ttest -e \"$2\" || BUG \"test_cmp_bin 'actual' file missing\"\n  +\t\ttest \"x$1\" = x- || test -e \"$1\" || BUG \"test_cmp_bin '$1' missing\"\n  +\t\ttest \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp_bin '$2' missing\"\n   \t\treturn 1\n   \tfi\n   }\n\n t/test-lib-functions.sh | 16 ++++++++++++++--\n 1 file changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex b791933ffd..a12d6a3fc9 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -952,7 +952,13 @@ test_expect_code () {\n # - not all diff versions understand \"-u\"\n \n test_cmp() {\n-\teval \"$GIT_TEST_CMP\" '\"$@\"'\n+\ttest $# -eq 2 || BUG \"test_cmp requires two arguments\"\n+\tif ! eval \"$GIT_TEST_CMP\" '\"$@\"'\n+\tthen\n+\t\ttest \"x$1\" = x- || test -e \"$1\" || BUG \"test_cmp '$1' missing\"\n+\t\ttest \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp '$2' missing\"\n+\t\treturn 1\n+\tfi\n }\n \n # Check that the given config key has the expected value.\n@@ -981,7 +987,13 @@ test_cmp_config() {\n # test_cmp_bin - helper to compare binary files\n \n test_cmp_bin() {\n-\tcmp \"$@\"\n+\ttest $# -eq 2 || BUG \"test_cmp_bin requires two arguments\"\n+\tif ! cmp \"$@\"\n+\tthen\n+\t\ttest \"x$1\" = x- || test -e \"$1\" || BUG \"test_cmp_bin '$1' missing\"\n+\t\ttest \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp_bin '$2' missing\"\n+\t\treturn 1\n+\tfi\n }\n \n # Use this instead of test_cmp to compare files that contain expected and\n-- \n2.28.0.220.ged08abb693\n\n"},{"id":"403238","messageId":"xmqq7du7eiqi.fsf@gitster.c.googlers.com","threadId":"54010","inReplyTo":"20200809174209.15466-1-sunshine@sunshineco.com","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-09T19:12:37Z","receivedAt":"2020-08-09T19:12:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> Under normal circumstances, if a test author misspells a filename passed\n> to test_cmp(), the error is quickly discovered when the test fails\n> unexpectedly due to test_cmp() being unable to find the file. However,\n> if the test is expected to fail, as with test_expect_failure(), a\n> misspelled filename as argument to test_cmp() will go unnoticed since\n> the test will indeed fail, but for the wrong reason. Make it easier for\n> test authors to discover such problems early by sanity-checking the\n> arguments to test_cmp(). To avoid penalizing all clients of test_cmp()\n> in the general case, only check for missing files if the comparison\n> fails.\n>\n> While at it, make test_cmp_bin() sanity-check its arguments, as well.\n>\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> ---\n\nThe motivation makes quite a lot of sense.\n\nI suspect that it is a bug to use \"-\", i.e. \"read from the standard\ninput\", for \"$2\", because\n\n (1) if it is used for the expected output, we got the comparison in\n     the wrong direction;\n\n (2) if it is used for the actual output, we have a command whose\n     output we are interested in on the upstream side of a pipe to\n     discard its exit status.\n\nbut I'll let it pass.\n\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index b791933ffd..a12d6a3fc9 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -952,7 +952,13 @@ test_expect_code () {\n>  # - not all diff versions understand \"-u\"\n>  \n>  test_cmp() {\n> -\teval \"$GIT_TEST_CMP\" '\"$@\"'\n> +\ttest $# -eq 2 || BUG \"test_cmp requires two arguments\"\n> +\tif ! eval \"$GIT_TEST_CMP\" '\"$@\"'\n> +\tthen\n> +\t\ttest \"x$1\" = x- || test -e \"$1\" || BUG \"test_cmp '$1' missing\"\n> +\t\ttest \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp '$2' missing\"\n> +\t\treturn 1\n> +\tfi\n>  }\n>  \n>  # Check that the given config key has the expected value.\n> @@ -981,7 +987,13 @@ test_cmp_config() {\n>  # test_cmp_bin - helper to compare binary files\n>  \n>  test_cmp_bin() {\n> -\tcmp \"$@\"\n> +\ttest $# -eq 2 || BUG \"test_cmp_bin requires two arguments\"\n> +\tif ! cmp \"$@\"\n> +\tthen\n> +\t\ttest \"x$1\" = x- || test -e \"$1\" || BUG \"test_cmp_bin '$1' missing\"\n> +\t\ttest \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp_bin '$2' missing\"\n> +\t\treturn 1\n> +\tfi\n>  }\n>  \n>  # Use this instead of test_cmp to compare files that contain expected and\n"},{"id":"403239","messageId":"CAPig+cTA_BVz-bGcGY++g2RV-Zd3Gv_yTGjv=VeMeqhP4eZZrw@mail.gmail.com","threadId":"54010","inReplyTo":"xmqq7du7eiqi.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-09T19:34:57Z","receivedAt":"2020-08-09T19:35:11Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 9, 2020 at 3:12 PM Junio C Hamano <gitster@pobox.com> wrote:\n> The motivation makes quite a lot of sense.\n>\n> I suspect that it is a bug to use \"-\", i.e. \"read from the standard\n> input\", for \"$2\", because\n>\n>  (1) if it is used for the expected output, we got the comparison in\n>      the wrong direction;\n\nI had the same concern that it didn't necessarily make sense to allow\n\"-\" for $2, but eventually thought of a case such as:\n\n    sed '...' actual | test_cmp expect - &&\n\nwhich massages 'actual' before test_cmp() sees it. True, we don't\nactually have such callers, but it theoretically is legitimate.\n\n>  (2) if it is used for the actual output, we have a command whose\n>      output we are interested in on the upstream side of a pipe to\n>      discard its exit status.\n\nIf the command upstream is a Git command, that could indeed be a\nreason for concern, but, as illustrated above, it could just be a\nsystem command.\n"},{"id":"403269","messageId":"xmqqtuxacyp8.fsf@gitster.c.googlers.com","threadId":"54010","inReplyTo":"CAPig+cTA_BVz-bGcGY++g2RV-Zd3Gv_yTGjv=VeMeqhP4eZZrw@mail.gmail.com","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-08-10T15:22:59Z","receivedAt":"2020-08-10T15:41:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> I had the same concern that it didn't necessarily make sense to allow\n> \"-\" for $2, but eventually thought of a case such as:\n>\n>     sed '...' actual | test_cmp expect - &&\n>\n> which massages 'actual' before test_cmp() sees it. True, we don't\n> actually have such callers, but it theoretically is legitimate.\n\nYup, that looks a bit too stretching [*] to me, but that was what I\nhad in mind when I said \"I'll let it pass\".\n\n    Side note.  Presumably that 'actual' was written by Git, to\n    avoid losing its exit code, e.g.\n\n\tgit frotz >actual &&\n\tsed '...' actual | test_cmp expect -\n\n    but then it becomes more natural to write the second one like\n    so:\n\n\tgit frotz >raw &&\n\tsed '...' raw >actual &&\n\ttest_cmp expect actual\n\n"},{"id":"403372","messageId":"20200811183258.GB33865@syl.lan","threadId":"54010","inReplyTo":"20200809174209.15466-1-sunshine@sunshineco.com","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-11T18:32:58Z","receivedAt":"2020-08-11T18:33:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Aug 09, 2020 at 01:42:09PM -0400, Eric Sunshine wrote:\n> Under normal circumstances, if a test author misspells a filename passed\n> to test_cmp(), the error is quickly discovered when the test fails\n> unexpectedly due to test_cmp() being unable to find the file. However,\n> if the test is expected to fail, as with test_expect_failure(), a\n> misspelled filename as argument to test_cmp() will go unnoticed since\n> the test will indeed fail, but for the wrong reason. Make it easier for\n> test authors to discover such problems early by sanity-checking the\n> arguments to test_cmp(). To avoid penalizing all clients of test_cmp()\n> in the general case, only check for missing files if the comparison\n> fails.\n>\n> While at it, make test_cmp_bin() sanity-check its arguments, as well.\n>\n> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>\n> ---\n>\n> Notes:\n>     This is a re-roll of [1] which was motivated by seeing Elijah fix[2]\n>     several cases of bogus test_cmp() calls which perhaps could have\n>     been detected earlier.\n>\n>     Changes since v1:\n>\n>     * take into account that some callers pass \"-\" (meaning standard\n>       input) as an argument to test_cmp() (pointed out privately by\n>       Junio)\n>\n>     * show the name of the missing file rather than a placeholder\n>       (Shourya[3])\n>\n>     [1]: https://lore.kernel.org/git/20200809060810.31370-1-sunshine@sunshineco.com/\n>     [2]: https://lore.kernel.org/git/7f408b7d4069403b969d334f4940ebf87f1dc797.1596906081.git.gitgitgadget@gmail.com/\n>     [3]: https://lore.kernel.org/git/20200809083227.GA11219@konoha/\n>\n> Interdiff against v1:\n>   diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n>   index 8d77deebd2..a12d6a3fc9 100644\n>   --- a/t/test-lib-functions.sh\n>   +++ b/t/test-lib-functions.sh\n>   @@ -955,8 +955,8 @@ test_cmp() {\n>    \ttest $# -eq 2 || BUG \"test_cmp requires two arguments\"\n>    \tif ! eval \"$GIT_TEST_CMP\" '\"$@\"'\n>    \tthen\n>   -\t\ttest -e \"$1\" || BUG \"test_cmp 'expect' file missing\"\n>   -\t\ttest -e \"$2\" || BUG \"test_cmp 'actual' file missing\"\n>   +\t\ttest \"x$1\" = x- || test -e \"$1\" || BUG \"test_cmp '$1' missing\"\n>   +\t\ttest \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp '$2' missing\"\n>    \t\treturn 1\n>    \tfi\n>    }\n>   @@ -990,8 +990,8 @@ test_cmp_bin() {\n>    \ttest $# -eq 2 || BUG \"test_cmp_bin requires two arguments\"\n>    \tif ! cmp \"$@\"\n>    \tthen\n>   -\t\ttest -e \"$1\" || BUG \"test_cmp_bin 'expect' file missing\"\n>   -\t\ttest -e \"$2\" || BUG \"test_cmp_bin 'actual' file missing\"\n>   +\t\ttest \"x$1\" = x- || test -e \"$1\" || BUG \"test_cmp_bin '$1' missing\"\n>   +\t\ttest \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp_bin '$2' missing\"\n>    \t\treturn 1\n>    \tfi\n>    }\n>\n>  t/test-lib-functions.sh | 16 ++++++++++++++--\n>  1 file changed, 14 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index b791933ffd..a12d6a3fc9 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -952,7 +952,13 @@ test_expect_code () {\n>  # - not all diff versions understand \"-u\"\n>\n>  test_cmp() {\n> -\teval \"$GIT_TEST_CMP\" '\"$@\"'\n> +\ttest $# -eq 2 || BUG \"test_cmp requires two arguments\"\n> +\tif ! eval \"$GIT_TEST_CMP\" '\"$@\"'\n> +\tthen\n> +\t\ttest \"x$1\" = x- || test -e \"$1\" || BUG \"test_cmp '$1' missing\"\n> +\t\ttest \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp '$2' missing\"\n\nNot related to your patch, but I've seen this style of \"x$1\" in a few\nplaces in test-lib-functions.sh. Why can't this be written as 'test \"$1\"\n= -'?\n\n> +\t\treturn 1\n> +\tfi\n>  }\n>\n>  # Check that the given config key has the expected value.\n> @@ -981,7 +987,13 @@ test_cmp_config() {\n>  # test_cmp_bin - helper to compare binary files\n>\n>  test_cmp_bin() {\n> -\tcmp \"$@\"\n> +\ttest $# -eq 2 || BUG \"test_cmp_bin requires two arguments\"\n> +\tif ! cmp \"$@\"\n> +\tthen\n> +\t\ttest \"x$1\" = x- || test -e \"$1\" || BUG \"test_cmp_bin '$1' missing\"\n> +\t\ttest \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp_bin '$2' missing\"\n> +\t\treturn 1\n> +\tfi\n>  }\n>\n>  # Use this instead of test_cmp to compare files that contain expected and\n> --\n> 2.28.0.220.ged08abb693\n>\nThanks,\nTaylor\n"},{"id":"403384","messageId":"CAPig+cSroWZEoOL78COmPS4rkvKLE-yCiqh6Part+5gUgVon+A@mail.gmail.com","threadId":"54010","inReplyTo":"20200811183258.GB33865@syl.lan","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-11T19:25:03Z","receivedAt":"2020-08-11T19:25:18Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Aug 11, 2020 at 2:33 PM Taylor Blau <me@ttaylorr.com> wrote:\n> On Sun, Aug 09, 2020 at 01:42:09PM -0400, Eric Sunshine wrote:\n> > +             test \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp '$2' missing\"\n>\n> Not related to your patch, but I've seen this style of \"x$1\" in a few\n> places in test-lib-functions.sh. Why can't this be written as 'test \"$1\"\n> = -'?\n\nShort answer: To prevent 'test' from thinking that the argument is a switch.\n\nLonger answer:\n\n'test' can accept both switches (i.e. \"-e\") and non-switch arguments.\nKeep in mind, too, that all the quoting is stripped by the shell\n_before_ 'test' ever sees its arguments. Let's say that the caller has\na filename whose name actually is \"-e\" and passes that in as $1. So,\nwhat does 'test' see?\n\n    test -e = -\n\nRather than comparing literal string \"-e\" to literal string \"-\", it's\ninstead (almost) asking if the file named \"=\" exists; I say \"almost\"\nbecause it's actually an error since switch -e only accepts one\nargument, but it's being given two arguments, \"=\" and \"-\".\n\nYou might say that having a file named \"-e\" (or similar) is unlikely,\nhowever, what is not unlikely is a caller passing \"-\" for\nstandard-input as $1. In this case, 'test' sees:\n\n    test - = -\n\nwhich may or may not be an error in a particular implementation of\n'test'. Some implementations may understand that \"-\" is not a valid\nswitch, thus infer that you're actually asking for an equality\ncomparison between arguments, but other implementations may complain\neither that there is no switch named \"-\" or that those arguments\nsimply make no sense.\n\nThis is why it's a very common idiom in shell programming with 'test'\nto see \"x\" prepended, thus ensuring that the argument can't be\nconfused with a switch.\n"},{"id":"403405","messageId":"20200811210337.GA34786@syl.lan","threadId":"54010","inReplyTo":"CAPig+cSroWZEoOL78COmPS4rkvKLE-yCiqh6Part+5gUgVon+A@mail.gmail.com","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-08-11T21:03:37Z","receivedAt":"2020-08-11T21:03:42Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Aug 11, 2020 at 03:25:03PM -0400, Eric Sunshine wrote:\n> On Tue, Aug 11, 2020 at 2:33 PM Taylor Blau <me@ttaylorr.com> wrote:\n> > On Sun, Aug 09, 2020 at 01:42:09PM -0400, Eric Sunshine wrote:\n> > > +             test \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp '$2' missing\"\n> >\n> > Not related to your patch, but I've seen this style of \"x$1\" in a few\n> > places in test-lib-functions.sh. Why can't this be written as 'test \"$1\"\n> > = -'?\n>\n> Short answer: To prevent 'test' from thinking that the argument is a switch.\n\nMakes sense. Now I feel silly for asking :).\n\n> Longer answer:\n>\n> 'test' can accept both switches (i.e. \"-e\") and non-switch arguments.\n> Keep in mind, too, that all the quoting is stripped by the shell\n> _before_ 'test' ever sees its arguments. Let's say that the caller has\n> a filename whose name actually is \"-e\" and passes that in as $1. So,\n> what does 'test' see?\n>\n>     test -e = -\n>\n> Rather than comparing literal string \"-e\" to literal string \"-\", it's\n> instead (almost) asking if the file named \"=\" exists; I say \"almost\"\n> because it's actually an error since switch -e only accepts one\n> argument, but it's being given two arguments, \"=\" and \"-\".\n>\n> You might say that having a file named \"-e\" (or similar) is unlikely,\n> however, what is not unlikely is a caller passing \"-\" for\n> standard-input as $1. In this case, 'test' sees:\n>\n>     test - = -\n>\n> which may or may not be an error in a particular implementation of\n> 'test'. Some implementations may understand that \"-\" is not a valid\n> switch, thus infer that you're actually asking for an equality\n> comparison between arguments, but other implementations may complain\n> either that there is no switch named \"-\" or that those arguments\n> simply make no sense.\n>\n> This is why it's a very common idiom in shell programming with 'test'\n> to see \"x\" prepended, thus ensuring that the argument can't be\n> confused with a switch.\n\nThanks for a careful and helpful explanation, as always :-). Makes sense\nto me.\n\nThanks,\nTaylor\n"},{"id":"403463","messageId":"20200812153705.GC33189@coredump.intra.peff.net","threadId":"54010","inReplyTo":"CAPig+cSroWZEoOL78COmPS4rkvKLE-yCiqh6Part+5gUgVon+A@mail.gmail.com","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-12T15:37:05Z","receivedAt":"2020-08-12T15:37:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 11, 2020 at 03:25:03PM -0400, Eric Sunshine wrote:\n\n> 'test' can accept both switches (i.e. \"-e\") and non-switch arguments.\n> Keep in mind, too, that all the quoting is stripped by the shell\n> _before_ 'test' ever sees its arguments. Let's say that the caller has\n> a filename whose name actually is \"-e\" and passes that in as $1. So,\n> what does 'test' see?\n> \n>     test -e = -\n> \n> Rather than comparing literal string \"-e\" to literal string \"-\", it's\n> instead (almost) asking if the file named \"=\" exists; I say \"almost\"\n> because it's actually an error since switch -e only accepts one\n> argument, but it's being given two arguments, \"=\" and \"-\".\n\nI don't think this is an error. The program can tell which you meant by\nthe number of arguments. POSIX lays out some rules here (from \"man\n1posix test\" on my system, but I'm sure you can find it online):\n\n  3 arguments:\n    *  If $2 is a binary primary, perform the binary test of $1 and $3.\n\n    *  If $1 is '!', negate the two-argument test of $2 and $3.\n\n    *  If $1 is '(' and $3 is ')', perform the unary test of $2.  On\n       systems that do not support the XSI option, the results are\n       unspecified if $1 is '(' and $3 is ')'.\n\n    *  Otherwise, produce unspecified results.\n\nSo we'd see that \"=\" is a binary primary (the complete set is defined\nearlier). Likewise \"! -e -\" would hit the second rule. We wouldn't get\nfooled by trying to compare the string \"!\" because it knows that \"=\" is\na binary operator and \"-e\" is a unary operator.\n\nIt gets weird with \"-a\" joining expressions. There's some discussion in\nthe Rationale section of the posix page, and it even warns explicitly\nagainst \"-a\" (in favor of \"test expr1 && test expr1\").\n\n> which may or may not be an error in a particular implementation of\n> 'test'. Some implementations may understand that \"-\" is not a valid\n> switch, thus infer that you're actually asking for an equality\n> comparison between arguments, but other implementations may complain\n> either that there is no switch named \"-\" or that those arguments\n> simply make no sense.\n\nYeah, historically I think there were shells that were not quite so\nclever. I have no idea which ones, or whether any are still around. I\ndon't think we've generally been that concerned with this case in Git,\nand nobody has complained. I'd be totally unsurprised to hear that SunOS\n/bin/sh doesn't get this right, but we've already written it off as\nunusable (it doesn't even support $() expansion).\n\n-Peff\n"},{"id":"403467","messageId":"CAPig+cQNvJ02fm82oBtyyxHUqfNk3oAZJW__Lu2Kn0Qt3VaDWw@mail.gmail.com","threadId":"54010","inReplyTo":"20200812153705.GC33189@coredump.intra.peff.net","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-12T16:15:32Z","receivedAt":"2020-08-12T16:15:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 12, 2020 at 11:37 AM Jeff King <peff@peff.net> wrote:\n> On Tue, Aug 11, 2020 at 03:25:03PM -0400, Eric Sunshine wrote:\n> >     test -e = -\n> > Rather than comparing literal string \"-e\" to literal string \"-\", it's\n> > instead (almost) asking if the file named \"=\" exists; I say \"almost\"\n> > because it's actually an error since switch -e only accepts one\n> > argument, but it's being given two arguments, \"=\" and \"-\".\n>\n> I don't think this is an error. The program can tell which you meant by\n> the number of arguments. POSIX lays out some rules here (from \"man\n> 1posix test\" on my system, but I'm sure you can find it online):\n\nI intentionally didn't focus on or mention POSIX in my response\nbecause I wanted to represent the Real World reason why \"x$var\" is\nsuch a common idiom. As a person who regularly uses ancient operating\nsystems and old computers (it's common for me to be stuck on computers\n10-25 years old; my most up-to-date computer is 9.5 years old), Real\nWorld is something I focus on more than POSIX. Okay, I may be an\noutlier, but still...\n\n> > which may or may not be an error in a particular implementation of\n> > 'test'. Some implementations may understand that \"-\" is not a valid\n> > switch, thus infer that you're actually asking for an equality\n> > comparison between arguments, but other implementations may complain\n> > either that there is no switch named \"-\" or that those arguments\n> > simply make no sense.\n>\n> Yeah, historically I think there were shells that were not quite so\n> clever. I have no idea which ones, or whether any are still around. I\n> don't think we've generally been that concerned with this case in Git,\n> and nobody has complained. I'd be totally unsurprised to hear that SunOS\n> /bin/sh doesn't get this right, but we've already written it off as\n> unusable (it doesn't even support $() expansion).\n\nI'm probably showing my age, but having had to deal with the many\ninconsistencies of implementation over the years, it's hard to _not_\nworry about such things even now. Adding the \"x\" to \"$1\" or to making\nother minor portability concessions is reflex at this point. These\nportability concerns are also always on mind when dealing with Mac OS\nsince so many of its utilities are ancient and retain the old\nbehaviors.\n"},{"id":"403469","messageId":"CAPig+cSzLcVdzjoacm6=7mDAhUn8j06Z0LXo7r1LMC7Dx_2JZA@mail.gmail.com","threadId":"54010","inReplyTo":"CAPig+cQNvJ02fm82oBtyyxHUqfNk3oAZJW__Lu2Kn0Qt3VaDWw@mail.gmail.com","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-08-12T16:39:14Z","receivedAt":"2020-08-12T16:39:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 12, 2020 at 12:15 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, Aug 12, 2020 at 11:37 AM Jeff King <peff@peff.net> wrote:\n> > I don't think this is an error. The program can tell which you meant by\n> > the number of arguments. POSIX lays out some rules here (from \"man\n> > 1posix test\" on my system, but I'm sure you can find it online):\n>\n> I intentionally didn't focus on or mention POSIX in my response\n> because I wanted to represent the Real World reason why \"x$var\" is\n> such a common idiom. [...]\n\nI probably should have done a better job in my original response to\nTaylor to make it clear that I was talking about Real World (even if\nold) rather than POSIX.\n"},{"id":"403476","messageId":"20200812171038.GB43566@coredump.intra.peff.net","threadId":"54010","inReplyTo":"CAPig+cSzLcVdzjoacm6=7mDAhUn8j06Z0LXo7r1LMC7Dx_2JZA@mail.gmail.com","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-08-12T17:10:38Z","receivedAt":"2020-08-12T17:10:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 12, 2020 at 12:39:14PM -0400, Eric Sunshine wrote:\n\n> On Wed, Aug 12, 2020 at 12:15 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n> > On Wed, Aug 12, 2020 at 11:37 AM Jeff King <peff@peff.net> wrote:\n> > > I don't think this is an error. The program can tell which you meant by\n> > > the number of arguments. POSIX lays out some rules here (from \"man\n> > > 1posix test\" on my system, but I'm sure you can find it online):\n> >\n> > I intentionally didn't focus on or mention POSIX in my response\n> > because I wanted to represent the Real World reason why \"x$var\" is\n> > such a common idiom. [...]\n> \n> I probably should have done a better job in my original response to\n> Taylor to make it clear that I was talking about Real World (even if\n> old) rather than POSIX.\n\nYeah. I guess I'm questioning how current that Real World view is. It\nhasn't bitten us yet, though we do seem to do the \"x\" thing in some\nplaces. And most of our shell code is in the test suite, which sees\npretty tame filenames.\n\n-Peff\n"},{"id":"407691","messageId":"20201016001704.GA2937048@coredump.intra.peff.net","threadId":"54010","inReplyTo":"20200809174209.15466-1-sunshine@sunshineco.com","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-10-16T00:17:04Z","receivedAt":"2020-10-16T00:17:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 09, 2020 at 01:42:09PM -0400, Eric Sunshine wrote:\n\n> Under normal circumstances, if a test author misspells a filename passed\n> to test_cmp(), the error is quickly discovered when the test fails\n> unexpectedly due to test_cmp() being unable to find the file. However,\n> if the test is expected to fail, as with test_expect_failure(), a\n> misspelled filename as argument to test_cmp() will go unnoticed since\n> the test will indeed fail, but for the wrong reason. Make it easier for\n> test authors to discover such problems early by sanity-checking the\n> arguments to test_cmp(). To avoid penalizing all clients of test_cmp()\n> in the general case, only check for missing files if the comparison\n> fails.\n\nThis patch caused some interesting confusion for me today.\n\nI was looking at the patch from [1] which causes a test failure (and I\nwanted to see where it failed, etc). And I got:\n\n  $ ./t5601-clone.sh\n  ok 1 - setup\n  ok 2 - clone with excess parameters (1)\n  ok 3 - clone with excess parameters (2)\n  ok 4 - output from clone\n  ok 5 - clone does not keep pack\n  ok 6 - clone checks out files\n  ok 7 - clone respects GIT_WORK_TREE\n  error: bug in the test script: test_cmp 'r2/.git/HEAD' missing\n\nwhich was somewhat unhelpful (or at least less helpful than a regular\ntest failure). The test in question does this:\n\n\ttest_cmp r0/.git/HEAD r2/.git/HEAD &&\n\nand expects to fail if an earlier step didn't correctly create r2. Is it\na bug or misuse of test_cmp for it to do so? I could see an argument\nthat it is, but I'm also not sure if there's a convenient alternative.\nThe best I could come up with is:\n\n  test_path_is_file r2/.git/HEAD &&\n  test_cmp r0/.git/HEAD r2/.git/HEAD\n\nwhich isn't that great.\n\n-Peff\n\n[1] https://lore.kernel.org/git/20200130102933.GE840531@coredump.intra.peff.net/\n"},{"id":"407700","messageId":"CAPig+cSU=1GcQuqZab+0Vff_A-JmD59wEc3RMr3wDojpgRYUuw@mail.gmail.com","threadId":"54010","inReplyTo":"20201016001704.GA2937048@coredump.intra.peff.net","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-10-16T02:18:03Z","receivedAt":"2020-10-16T02:18:17Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Oct 15, 2020 at 8:17 PM Jeff King <peff@peff.net> wrote:\n> On Sun, Aug 09, 2020 at 01:42:09PM -0400, Eric Sunshine wrote:\n> > [...] Make it easier for test authors to discover such problems early\n> > by sanity-checking the arguments to test_cmp(). [...]\n>\n> This patch caused some interesting confusion for me today.\n>   error: bug in the test script: test_cmp 'r2/.git/HEAD' missing\n> which was somewhat unhelpful (or at least less helpful than a regular\n> test failure). The test in question does this:\n>         test_cmp r0/.git/HEAD r2/.git/HEAD &&\n> and expects to fail if an earlier step didn't correctly create r2. Is it\n> a bug or misuse of test_cmp for it to do so? I could see an argument\n> that it is, but I'm also not sure if there's a convenient alternative.\n\nI can see the argument going both ways as to whether it's a misuse of\n'test_cmp'.\n\n> The best I could come up with is:\n>\n>   test_path_is_file r2/.git/HEAD &&\n>   test_cmp r0/.git/HEAD r2/.git/HEAD\n>\n> which isn't that great.\n\nÆvar ran into the same issue recently[1] and came up with the same\nworkaround. Despite its good intention (trying to catch bugs in\n'test_expect_failure' tests), this change[2] doesn't seem to have\ncaught any genuine bugs (it wouldn't even have caught the bug which\nserved as its inspiration[3]), but has nevertheless caused a couple\nhiccups already. As such, I would not be opposed to seeing the change\nreverted.\n\n[1]: https://lore.kernel.org/git/20200921104000.2304-15-avarab@gmail.com/\n[2]: https://lore.kernel.org/git/20200809060810.31370-1-sunshine@sunshineco.com/\n[3]: https://lore.kernel.org/git/7f408b7d4069403b969d334f4940ebf87f1dc797.1596906081.git.gitgitgadget@gmail.com/\n"},{"id":"407737","messageId":"xmqqzh4maugq.fsf@gitster.c.googlers.com","threadId":"54010","inReplyTo":"CAPig+cSU=1GcQuqZab+0Vff_A-JmD59wEc3RMr3wDojpgRYUuw@mail.gmail.com","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-16T18:36:53Z","receivedAt":"2020-10-16T18:37:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> On Thu, Oct 15, 2020 at 8:17 PM Jeff King <peff@peff.net> wrote:\n>> On Sun, Aug 09, 2020 at 01:42:09PM -0400, Eric Sunshine wrote:\n>> > [...] Make it easier for test authors to discover such problems early\n>> > by sanity-checking the arguments to test_cmp(). [...]\n>>\n>> This patch caused some interesting confusion for me today.\n>>   error: bug in the test script: test_cmp 'r2/.git/HEAD' missing\n>> which was somewhat unhelpful (or at least less helpful than a regular\n>> test failure). The test in question does this:\n>>         test_cmp r0/.git/HEAD r2/.git/HEAD &&\n>> and expects to fail if an earlier step didn't correctly create r2. Is it\n>> a bug or misuse of test_cmp for it to do so? I could see an argument\n>> that it is, but I'm also not sure if there's a convenient alternative.\n>\n> I can see the argument going both ways as to whether it's a misuse of\n> 'test_cmp'.\n>\n>> The best I could come up with is:\n>>\n>>   test_path_is_file r2/.git/HEAD &&\n>>   test_cmp r0/.git/HEAD r2/.git/HEAD\n>>\n>> which isn't that great.\n\nHmph, I agree that the \"both must be file\" is a bit too eager and\nignores that \"they must match, but the possible reasons they may not\ninclude one of them may be missing\" use case.\n\n> Ævar ran into the same issue recently[1] and came up with the same\n> workaround. Despite its good intention (trying to catch bugs in\n> 'test_expect_failure' tests), this change[2] doesn't seem to have\n> caught any genuine bugs (it wouldn't even have caught the bug which\n> served as its inspiration[3]), but has nevertheless caused a couple\n> hiccups already. As such, I would not be opposed to seeing the change\n> reverted.\n\nSounds good.  Anybody wants to do the honors?\n"},{"id":"407760","messageId":"xmqqv9f9ao0v.fsf@gitster.c.googlers.com","threadId":"54010","inReplyTo":"xmqqzh4maugq.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-16T20:56:00Z","receivedAt":"2020-10-16T20:56:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Hmph, I agree that the \"both must be file\" is a bit too eager and\n> ignores that \"they must match, but the possible reasons they may not\n> include one of them may be missing\" use case.\n>\n>> Ævar ran into the same issue recently[1] and came up with the same\n>> workaround. Despite its good intention (trying to catch bugs in\n>> 'test_expect_failure' tests), this change[2] doesn't seem to have\n>> caught any genuine bugs (it wouldn't even have caught the bug which\n>> served as its inspiration[3]), but has nevertheless caused a couple\n>> hiccups already. As such, I would not be opposed to seeing the change\n>> reverted.\n>\n> Sounds good.  Anybody wants to do the honors?\n\nFor now I've queued this directly on top of the offending change and\nmerged the result in 'seen', perhaps to be advanced to 'next' and\n'master' after the 2.29 final unless a better version comes.\n\nThanks.\n\n-- >8 --\ncommit b0b2d8b4e00fd34c0031b6dbcd67b4bcf22d864c\nAuthor: Junio C Hamano <gitster@pobox.com>\nDate:   Fri Oct 16 13:51:04 2020 -0700\n\n    Revert \"test_cmp: diagnose incorrect arguments\"\n    \n    This reverts commit d572f52a64c6a69990f72ad6a09504b9b615d2e4; the\n    idea to detect that \"test_cmp expect actual\" was fed a misspelt\n    filename meant well, but when the version of Git tested exhibits a\n    bug, the reason why these two files do not match may be because one\n    of them did not get created as expected, in which case missing file\n    is not a sign of misspelt filename but is a genuine test failure.\n---\n t/test-lib-functions.sh | 16 ++--------------\n 1 file changed, 2 insertions(+), 14 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 21225330c2..3103be8a32 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -905,13 +905,7 @@ test_expect_code () {\n # - not all diff versions understand \"-u\"\n \n test_cmp() {\n-\ttest $# -eq 2 || BUG \"test_cmp requires two arguments\"\n-\tif ! eval \"$GIT_TEST_CMP\" '\"$@\"'\n-\tthen\n-\t\ttest \"x$1\" = x- || test -e \"$1\" || BUG \"test_cmp '$1' missing\"\n-\t\ttest \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp '$2' missing\"\n-\t\treturn 1\n-\tfi\n+\teval \"$GIT_TEST_CMP\" '\"$@\"'\n }\n \n # Check that the given config key has the expected value.\n@@ -940,13 +934,7 @@ test_cmp_config() {\n # test_cmp_bin - helper to compare binary files\n \n test_cmp_bin() {\n-\ttest $# -eq 2 || BUG \"test_cmp_bin requires two arguments\"\n-\tif ! cmp \"$@\"\n-\tthen\n-\t\ttest \"x$1\" = x- || test -e \"$1\" || BUG \"test_cmp_bin '$1' missing\"\n-\t\ttest \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp_bin '$2' missing\"\n-\t\treturn 1\n-\tfi\n+\tcmp \"$@\"\n }\n \n # Use this instead of test_cmp to compare files that contain expected and\n"},{"id":"407777","messageId":"xmqqy2k5931k.fsf@gitster.c.googlers.com","threadId":"54010","inReplyTo":"xmqqv9f9ao0v.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-16T23:14:31Z","receivedAt":"2020-10-16T23:14:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>  test_cmp() {\n> -\ttest $# -eq 2 || BUG \"test_cmp requires two arguments\"\n> -\tif ! eval \"$GIT_TEST_CMP\" '\"$@\"'\n> -\tthen\n> -\t\ttest \"x$1\" = x- || test -e \"$1\" || BUG \"test_cmp '$1' missing\"\n> -\t\ttest \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp '$2' missing\"\n> -\t\treturn 1\n> -\tfi\n> +\teval \"$GIT_TEST_CMP\" '\"$@\"'\n>  }\n>  \n>  # Check that the given config key has the expected value.\n> @@ -940,13 +934,7 @@ test_cmp_config() {\n>  # test_cmp_bin - helper to compare binary files\n>  \n>  test_cmp_bin() {\n> -\ttest $# -eq 2 || BUG \"test_cmp_bin requires two arguments\"\n> -\tif ! cmp \"$@\"\n> -\tthen\n> -\t\ttest \"x$1\" = x- || test -e \"$1\" || BUG \"test_cmp_bin '$1' missing\"\n> -\t\ttest \"x$2\" = x- || test -e \"$2\" || BUG \"test_cmp_bin '$2' missing\"\n> -\t\treturn 1\n> -\tfi\n> +\tcmp \"$@\"\n>  }\n\nLooking at this again, I think we could keep the \"we should have two\narguments, no more than, no less than, but exactly two\".  But I think\nthose who write new tests are working to eventually make them pass,\nso hopefully they'll notice and investigate test_cmp that yields false\nanyway, I guess.\n\n"},{"id":"407786","messageId":"CAPig+cRqnRKUdmwX3K+hbYs8+T0CCpatkPi4T-bu8ofH6jTnNg@mail.gmail.com","threadId":"54010","inReplyTo":"xmqqy2k5931k.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2] test_cmp: diagnose incorrect arguments","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-10-17T06:06:46Z","receivedAt":"2020-10-17T06:07:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Oct 16, 2020 at 7:14 PM Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> >  test_cmp() {\n> > -     test $# -eq 2 || BUG \"test_cmp requires two arguments\"\n> > -     if ! eval \"$GIT_TEST_CMP\" '\"$@\"'\n>\n> Looking at this again, I think we could keep the \"we should have two\n> arguments, no more than, no less than, but exactly two\".  But I think\n> those who write new tests are working to eventually make them pass,\n> so hopefully they'll notice and investigate test_cmp that yields false\n> anyway, I guess.\n\nThe purpose of the change in question[1] was specifically to help\ncatch bugs in the test_expect_failure() case, which is quite rare in\nthe Git test suite (and hopefully becomes rarer over time). So, I\nthink it's fine to revert the change fully, including the 2-argument\ncheck since, as you say, test writers are normally trying to make\ntheir tests pass, and the 2-argument check doesn't provide much, if\nany, value for the test_expect_success() case.\n\n[1]: d572f52a64 (test_cmp: diagnose incorrect arguments, 2020-08-09)\n"}]}