{"thread":{"id":"49122","subject":"[PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","startedAt":"2018-08-14T11:48:33Z","lastAt":"2018-08-28T06:52:32Z","messageCount":17,"participants":["SZEDER Gábor","Jeff King","Andrei Rybak","Junio C Hamano","Matthew DeVore","Kirill Smelkov"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"355535","messageId":"20180814114721.25577-1-szeder.dev@gmail.com","threadId":"49122","inReplyTo":null,"subject":"[PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-08-14T11:47:21Z","receivedAt":"2018-08-14T11:48:33Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The test 'pack-objects to file can use bitmap' added in 645c432d61\n(pack-objects: use reachability bitmap index when generating\nnon-stdout pack, 2016-09-10) is silently buggy and doesn't check what\nit's supposed to.\n\nIn 't5310-pack-bitmaps.sh', the 'list_packed_objects' helper function\ndoes what its name implies by running:\n\n  git show-index <\"$1\" | cut -d' ' -f2\n\nThe test in question invokes this function like this:\n\n  list_packed_objects <packa-$packasha1.idx >packa.objects &&\n  list_packed_objects <packb-$packbsha1.idx >packb.objects &&\n  test_cmp packa.objects packb.objects\n\nNote how these two callsites don't specify the name of the pack index\nfile as the function's parameter, but redirect the function's standard\ninput from it.  This triggers an error message from the shell, as it\nhas no filename to redirect from in the function, but this error is\nignored, because it happens upstream of a pipe.  Consequently, both\ninvocations produce empty 'pack{a,b}.objects' files, and the\nsubsequent 'test_cmp' happily finds those two empty files identical.\n\nFix these two 'list_packed_objects' invocations by specifying the pack\nindex files as parameters.  Furthermore, eliminate the pipe in that\nfunction by replacing it with an &&-chained pair of commands using an\nintermediate file, so a failure of 'git show-index' or the shell\nredirection will fail the test.\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n t/t5310-pack-bitmaps.sh | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\nindex 6ee4d3f2d9..557bd0d0c0 100755\n--- a/t/t5310-pack-bitmaps.sh\n+++ b/t/t5310-pack-bitmaps.sh\n@@ -9,7 +9,8 @@ objpath () {\n \n # show objects present in pack ($1 should be associated *.idx)\n list_packed_objects () {\n-\tgit show-index <\"$1\" | cut -d' ' -f2\n+\tgit show-index <\"$1\" >object-list &&\n+\tcut -d' ' -f2 object-list\n }\n \n # has_any pattern-file content-file\n@@ -204,8 +205,8 @@ test_expect_success 'pack-objects to file can use bitmap' '\n \t# verify equivalent packs are generated with/without using bitmap index\n \tpackasha1=$(git pack-objects --no-use-bitmap-index --all packa </dev/null) &&\n \tpackbsha1=$(git pack-objects --use-bitmap-index --all packb </dev/null) &&\n-\tlist_packed_objects <packa-$packasha1.idx >packa.objects &&\n-\tlist_packed_objects <packb-$packbsha1.idx >packb.objects &&\n+\tlist_packed_objects packa-$packasha1.idx >packa.objects &&\n+\tlist_packed_objects packb-$packbsha1.idx >packb.objects &&\n \ttest_cmp packa.objects packb.objects\n '\n \n-- \n2.18.0.720.g1f300496fc\n\n"},{"id":"355631","messageId":"20180814214942.GB667@sigill.intra.peff.net","threadId":"49122","inReplyTo":"20180814114721.25577-1-szeder.dev@gmail.com","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-14T21:49:42Z","receivedAt":"2018-08-14T21:49:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 14, 2018 at 01:47:21PM +0200, SZEDER Gábor wrote:\n\n> The test 'pack-objects to file can use bitmap' added in 645c432d61\n> (pack-objects: use reachability bitmap index when generating\n> non-stdout pack, 2016-09-10) is silently buggy and doesn't check what\n> it's supposed to.\n> \n> In 't5310-pack-bitmaps.sh', the 'list_packed_objects' helper function\n> does what its name implies by running:\n> \n>   git show-index <\"$1\" | cut -d' ' -f2\n> \n> The test in question invokes this function like this:\n> \n>   list_packed_objects <packa-$packasha1.idx >packa.objects &&\n>   list_packed_objects <packb-$packbsha1.idx >packb.objects &&\n>   test_cmp packa.objects packb.objects\n> \n> Note how these two callsites don't specify the name of the pack index\n> file as the function's parameter, but redirect the function's standard\n> input from it.  This triggers an error message from the shell, as it\n> has no filename to redirect from in the function, but this error is\n> ignored, because it happens upstream of a pipe.  Consequently, both\n> invocations produce empty 'pack{a,b}.objects' files, and the\n> subsequent 'test_cmp' happily finds those two empty files identical.\n> \n> Fix these two 'list_packed_objects' invocations by specifying the pack\n> index files as parameters.  Furthermore, eliminate the pipe in that\n> function by replacing it with an &&-chained pair of commands using an\n> intermediate file, so a failure of 'git show-index' or the shell\n> redirection will fail the test.\n\nGood catch, and nicely explained. The patch itself looks obviously\ncorrect.\n\n-Peff\n"},{"id":"355879","messageId":"4d6ffc81-a1e8-a60f-d53f-2ec159160fcd@gmail.com","threadId":"49122","inReplyTo":"20180814114721.25577-1-szeder.dev@gmail.com","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2018-08-16T20:51:43Z","receivedAt":"2018-08-16T20:51:49Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 14/08/18 13:47, SZEDER Gábor wrote:\n> ... both\n> invocations produce empty 'pack{a,b}.objects' files, and the\n> subsequent 'test_cmp' happily finds those two empty files identical.\n\nIs test_cmp ever used for empty files? Would it make sense for\ntest_cmp to issue warning when an empty file is being compared?\n\n> ---\n>  t/t5310-pack-bitmaps.sh | 7 ++++---\n>  1 file changed, 4 insertions(+), 3 deletions(-)\n> \n> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\n> index 6ee4d3f2d9..557bd0d0c0 100755\n> --- a/t/t5310-pack-bitmaps.sh\n> +++ b/t/t5310-pack-bitmaps.sh\n> @@ -9,7 +9,8 @@ objpath () {\n>  \n>  # show objects present in pack ($1 should be associated *.idx)\n>  list_packed_objects () {\n> -\tgit show-index <\"$1\" | cut -d' ' -f2\n> +\tgit show-index <\"$1\" >object-list &&\n> +\tcut -d' ' -f2 object-list\n>  }\n>  \n>  # has_any pattern-file content-file\n> @@ -204,8 +205,8 @@ test_expect_success 'pack-objects to file can use bitmap' '\n>  \t# verify equivalent packs are generated with/without using bitmap index\n>  \tpackasha1=$(git pack-objects --no-use-bitmap-index --all packa </dev/null) &&\n>  \tpackbsha1=$(git pack-objects --use-bitmap-index --all packb </dev/null) &&\n> -\tlist_packed_objects <packa-$packasha1.idx >packa.objects &&\n> -\tlist_packed_objects <packb-$packbsha1.idx >packb.objects &&\n> +\tlist_packed_objects packa-$packasha1.idx >packa.objects &&\n> +\tlist_packed_objects packb-$packbsha1.idx >packb.objects &&\n>  \ttest_cmp packa.objects packb.objects\n>  '\n>  \n> \n\n"},{"id":"355886","messageId":"xmqqr2iyc526.fsf@gitster-ct.c.googlers.com","threadId":"49122","inReplyTo":"4d6ffc81-a1e8-a60f-d53f-2ec159160fcd@gmail.com","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-16T22:36:17Z","receivedAt":"2018-08-16T22:36:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak <rybak.a.v@gmail.com> writes:\n\n> On 14/08/18 13:47, SZEDER Gábor wrote:\n>> ... both\n>> invocations produce empty 'pack{a,b}.objects' files, and the\n>> subsequent 'test_cmp' happily finds those two empty files identical.\n>\n> Is test_cmp ever used for empty files? Would it make sense for\n> test_cmp to issue warning when an empty file is being compared?\n\nTypically test_cmp is used to compare the actual output from a\ndubious command being tested with the expected output from a\nprocedure that is known not to be broken (e.g. a run of 'echo', or a\n'cat' of here-doc), so at least one side would not be empty.\n\nThe test done here is an odd case---it compares output from two\nequally dubious processes that are being tested and sees if their\nresults match.\n\nThat said, since we have test_must_be_empty, we could forbid feeding\nempty files to test_cmp, after telling everybody that a test that\nexpects an empty output must use test_must_be_empty.  I do not think\nit is a terrible idea.\n"},{"id":"355925","messageId":"CAM0VKjkT7fBJRie_3f4B13BHT9hp9MxRhuX5r1sogh2x7KQzbg@mail.gmail.com","threadId":"49122","inReplyTo":"xmqqr2iyc526.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-08-17T17:39:01Z","receivedAt":"2018-08-17T17:39:16Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Aug 17, 2018 at 12:36 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Andrei Rybak <rybak.a.v@gmail.com> writes:\n>\n> > On 14/08/18 13:47, SZEDER Gábor wrote:\n> >> ... both\n> >> invocations produce empty 'pack{a,b}.objects' files, and the\n> >> subsequent 'test_cmp' happily finds those two empty files identical.\n> >\n> > Is test_cmp ever used for empty files? Would it make sense for\n> > test_cmp to issue warning when an empty file is being compared?\n>\n> Typically test_cmp is used to compare the actual output from a\n> dubious command being tested with the expected output from a\n> procedure that is known not to be broken (e.g. a run of 'echo', or a\n> 'cat' of here-doc), so at least one side would not be empty.\n>\n> The test done here is an odd case---it compares output from two\n> equally dubious processes that are being tested and sees if their\n> results match.\n>\n> That said, since we have test_must_be_empty, we could forbid feeding\n> empty files to test_cmp, after telling everybody that a test that\n> expects an empty output must use test_must_be_empty.  I do not think\n> it is a terrible idea.\n\nI thought so as well, and I've looked into it; in fact this patch was\none of the skeletons that fell out of our test suite \"while at it\".\nHowever, I had to change my mind about it, and now I don't think we\nshould go all the way and forbid that.\n\nSee, we have quite a few tests that extract repetitive common tasks\ninto helper functions, which sometimes includes preparing the expected\nresults and running 'test_cmp', e.g. something like this\n(oversimplified) example:\n\n  check_cmd () {\n        git cmd $1 >actual &&\n        echo \"$2\" >expect &&\n        test_cmp expect actual\n  }\n\n  check_cmd --foo    FOO\n  check_cmd --no-foo \"\"\n\nCompletely forbidding feeding empty files to 'test_cmp' would require\nus to add a bit of logic to cases like this to call 'test_cmp' or\n'test_must_be_empty' based on the content of the second parameter.\nArguably it's only a tiny bit of logic, as only a single if statement\nis needed, but following our coding style it would take 7 lines\ninstead of only 2:\n\n  if test -n \"$2\"\n  then\n        echo \"$2\" >expect &&\n        test_cmp expect actual\n  else\n        test_must_be_empty actual\n  fi\n\nI don't think it's worth it in cases like this.\n"},{"id":"355933","messageId":"eeb04c94-50c1-13ee-880b-ea380031a685@gmail.com","threadId":"49122","inReplyTo":"CAM0VKjkT7fBJRie_3f4B13BHT9hp9MxRhuX5r1sogh2x7KQzbg@mail.gmail.com","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2018-08-17T19:27:22Z","receivedAt":"2018-08-17T19:27:28Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 17/08/18 19:39, SZEDER Gábor wrote:\n> \n> See, we have quite a few tests that extract repetitive common tasks\n> into helper functions, which sometimes includes preparing the expected\n> results and running 'test_cmp', e.g. something like this\n> (oversimplified) example:\n> \n>   check_cmd () {\n>         git cmd $1 >actual &&\n>         echo \"$2\" >expect &&\n>         test_cmp expect actual\n>   }\n> \n>   check_cmd --foo    FOO\n>   check_cmd --no-foo \"\"\n\nI've only had time to look into this from t0001 up to t0008-ignores.sh, where\ntest_check_ignore does this. If these helper functions need to allow comparing\nempty files -- how about adding special variation of cmp functions for cases\nlike this: test_cmp_allow_empty and test_i18ncmp_allow_empty?\n\nI think it would be a good trade-off to allow these helper functions to skip\nchecking emptiness of arguments for test_cmp. Such patch will require only\ns/test_cmp/&_allow_empty/ for these helper functions and it will help catch\ncases as bogus test in t5310.\n\nI'll try something like the following on the weekend:\n\n\ttest_cmp() {\n\t\tif test \"$1\" != - && ! test -s \"$1\"\n\t\tthen\n\t\t\techo >&4 \"error: trying to compare empty file '$1'\"\n\t\t\treturn 1\n\t\tfi\n\t\tif test \"$2\" != - && ! test -s \"$2\"\n\t\tthen\n\t\t\techo >&4 \"error: trying to compare empty file '$2'\"\n\t\t\treturn 1\n\t\tfi\n\t\ttest_cmp_allow_empty \"$@\"\n\t}\n\t\n\ttest_cmp_allow_empty() {\n\t\t$GIT_TEST_CMP \"$@\"\n\t}\n\n(I'm not sure about redirections in test lib functions. The two if's would\nprobably be in a separate function to be re-used by test_i18ncmp.)\n"},{"id":"355936","messageId":"xmqq4lfsbvru.fsf@gitster-ct.c.googlers.com","threadId":"49122","inReplyTo":"eeb04c94-50c1-13ee-880b-ea380031a685@gmail.com","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-17T20:09:09Z","receivedAt":"2018-08-17T20:09:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak <rybak.a.v@gmail.com> writes:\n\n> I think it would be a good trade-off to allow these helper functions to skip\n> checking emptiness of arguments for test_cmp. Such patch will require only\n> s/test_cmp/&_allow_empty/ for these helper functions and it will help catch\n> cases as bogus test in t5310.\n>\n> I'll try something like the following on the weekend:\n>\n> \ttest_cmp() {\n\nStyle: SP before and after ().\n\n> \t\tif test \"$1\" != - && ! test -s \"$1\"\n> \t\tthen\n> \t\t\techo >&4 \"error: trying to compare empty file '$1'\"\n> \t\t\treturn 1\n> \t\tfi\n> \t\tif test \"$2\" != - && ! test -s \"$2\"\n> \t\tthen\n> \t\t\techo >&4 \"error: trying to compare empty file '$2'\"\n> \t\t\treturn 1\n> \t\tfi\n> \t\ttest_cmp_allow_empty \"$@\"\n> \t}\n\nI actually think the above gives way too confusing output, when the\nactual output is empty and we are expecting some output.  I.e.\n\n\t: >expect &&\n\tgit cmd >actual &&\n\ttest_cmp expect actual\n\nThe tester wants to hear from test_cmp \"your 'git cmd' produced some\noutput when we are expecting none\" as the primary message.  We are\ntrying to find bugs in \"git\" under development, and diagnosing iffy\ntests is secondary.  But with your change, the first thing that is\nchecked is if 'expect' is an empty file and that is what we get\ncomplaints about, without even looking at what is in 'actual'.\n\nIt's the same story, and it is even worse than the above, when we\nare expecting some output but the command produced no output, i.e.\n\n\n\techo Everything up-to-date. >expect &&\n\tgit cmd >actual &&\n\ttest_cmp expect actual\n\nWe should get a complaint from test_cmp that 'actual' does not match\nthe string we were expecting (and even better, we show how they are\ndifferent by running them thru \"diff -u\"), not that 'actual' is an\nempty file.\n\n\n> \ttest_cmp_allow_empty() {\n> \t\t$GIT_TEST_CMP \"$@\"\n> \t}\n>\n> (I'm not sure about redirections in test lib functions. The two if's would\n> probably be in a separate function to be re-used by test_i18ncmp.)\n"},{"id":"355937","messageId":"CAM0VKj=rw14MbLbXvuJiiwf05Txo1tu6A8xzy+_3RHOytX8mXA@mail.gmail.com","threadId":"49122","inReplyTo":"eeb04c94-50c1-13ee-880b-ea380031a685@gmail.com","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-08-17T20:15:19Z","receivedAt":"2018-08-17T20:15:34Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Aug 17, 2018 at 9:27 PM Andrei Rybak <rybak.a.v@gmail.com> wrote:\n>\n> On 17/08/18 19:39, SZEDER Gábor wrote:\n> >\n> > See, we have quite a few tests that extract repetitive common tasks\n> > into helper functions, which sometimes includes preparing the expected\n> > results and running 'test_cmp', e.g. something like this\n> > (oversimplified) example:\n> >\n> >   check_cmd () {\n> >         git cmd $1 >actual &&\n> >         echo \"$2\" >expect &&\n> >         test_cmp expect actual\n> >   }\n> >\n> >   check_cmd --foo    FOO\n> >   check_cmd --no-foo \"\"\n>\n> I've only had time to look into this from t0001 up to t0008-ignores.sh, where\n> test_check_ignore does this. If these helper functions need to allow comparing\n> empty files -- how about adding special variation of cmp functions for cases\n> like this: test_cmp_allow_empty and test_i18ncmp_allow_empty?\n>\n> I think it would be a good trade-off to allow these helper functions to skip\n> checking emptiness of arguments for test_cmp. Such patch will require only\n> s/test_cmp/&_allow_empty/ for these helper functions and it will help catch\n> cases as bogus test in t5310.\n>\n> I'll try something like the following on the weekend:\n>\n>         test_cmp() {\n>                 if test \"$1\" != - && ! test -s \"$1\"\n>                 then\n>                         echo >&4 \"error: trying to compare empty file '$1'\"\n>                         return 1\n>                 fi\n>                 if test \"$2\" != - && ! test -s \"$2\"\n>                 then\n>                         echo >&4 \"error: trying to compare empty file '$2'\"\n>                         return 1\n>                 fi\n\nYeah, these conditions are what I instrumented 'test_cmp' with (except\nI used 'error \"bug in test script ...\"') to find callsites that could\nbe converted to 'test_must_be_empty', that's how I found the bug fixed\nin this patch.  However, it resulted in a lot of errors from the cases\nmentioned in my previous email.  Then I reached out to Bash and tried\nthis:\n\n  test_cmp() {\n         if test -n \"$BASH_VERSION\" &&\n            test \"${FUNCNAME[1]}\" = \"test_eval_inner_\" &&\n            test \"$1\" != \"-\" &&\n            test ! -s \"$1\"\n         then\n                 error \"bug in test script: using test_cmp to check\nempty file; use test_must_be_empty instead\"\n         fi\n         $GIT_TEST_CMP \"$@\"\n  }\n\ni.e. to limit the check callsites where 'test_cmp' is called directly\nfrom within a test_expect_{success,failure} block.  This is better,\nalmost all errors could be converted to 'test_must_be_empty' stright\naway; I have the patches almost ready.  There are, however, a few\nparametric cases that choke on this: where we run 'test_cmp' in a\nloop, e.g. 'cvs update (-p)' in t9400, and I think there was a case\nwhere the 'test_expect_success' block is within a function.\n\n\n>                 test_cmp_allow_empty \"$@\"\n>         }\n>\n>         test_cmp_allow_empty() {\n>                 $GIT_TEST_CMP \"$@\"\n>         }\n>\n> (I'm not sure about redirections in test lib functions. The two if's would\n> probably be in a separate function to be re-used by test_i18ncmp.)\n"},{"id":"356037","messageId":"73346b91-6d19-651a-c361-1666a39681f0@gmail.com","threadId":"49122","inReplyTo":"xmqq4lfsbvru.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2018-08-19T17:50:42Z","receivedAt":"2018-08-19T17:51:02Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 17/08/18 22:09, Junio C Hamano wrote:\n> Andrei Rybak <rybak.a.v@gmail.com> writes:\n>>\n>> I'll try something like the following on the weekend:\n>>\n>> \ttest_cmp () {\n>> \t\tif test \"$1\" != - && ! test -s \"$1\"\n>> \t\tthen\n>> \t\t\techo >&4 \"error: trying to compare empty file '$1'\"\n>> \t\t\treturn 1\n>> \t\tfi\n>> \t\tif test \"$2\" != - && ! test -s \"$2\"\n>> \t\tthen\n>> \t\t\techo >&4 \"error: trying to compare empty file '$2'\"\n>> \t\t\treturn 1\n>> \t\tfi\n>> \t\ttest_cmp_allow_empty \"$@\"\n>> \t}\n> \n> I actually think the above gives way too confusing output, when the\n> actual output is empty and we are expecting some output.\n> \n> The tester wants to hear from test_cmp \"your 'git cmd' produced some\n> output when we are expecting none\" as the primary message.  We are\n> trying to find bugs in \"git\" under development, and diagnosing iffy\n> tests is secondary.  But with your change, the first thing that is\n> checked is if 'expect' is an empty file and that is what we get\n> complaints about, without even looking at what is in 'actual'.\n\nI came up with two solutions for this issue:\n\n  1. Check both files at the same time (combination with Gábor's\n     function):\n\n\ttest_cmp () {\n\t\tif test \"$1\" != - &&\n\t\t   test \"$2\" != - &&\n\t\t   ! test -s \"$1\" && \n\t\t   ! test -s \"$2\"\n\t\tthen\n\t\t\terror \"bug in test script: using test_cmp to check empty file; use test_must_be_empty instead\"\n\t\tfi\n\t\ttest_cmp_allow_empty \"$@\"\n\t}\n\n     This will still be reporting to the developer clearly, but\n     will only catch cases exactly like the bogus test in t5310.\n\n  2. Enable this check via variable, smth like EMPTY_CMP_LINT=1\n"},{"id":"356055","messageId":"20180819203253.GA5789@sigill.intra.peff.net","threadId":"49122","inReplyTo":"73346b91-6d19-651a-c361-1666a39681f0@gmail.com","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-19T20:32:53Z","receivedAt":"2018-08-19T20:32:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 19, 2018 at 07:50:42PM +0200, Andrei Rybak wrote:\n\n> > I actually think the above gives way too confusing output, when the\n> > actual output is empty and we are expecting some output.\n> > \n> > The tester wants to hear from test_cmp \"your 'git cmd' produced some\n> > output when we are expecting none\" as the primary message.  We are\n> > trying to find bugs in \"git\" under development, and diagnosing iffy\n> > tests is secondary.  But with your change, the first thing that is\n> > checked is if 'expect' is an empty file and that is what we get\n> > complaints about, without even looking at what is in 'actual'.\n> \n> I came up with two solutions for this issue:\n> \n>   1. Check both files at the same time (combination with Gábor's\n>      function):\n> \n> \ttest_cmp () {\n> \t\tif test \"$1\" != - &&\n> \t\t   test \"$2\" != - &&\n> \t\t   ! test -s \"$1\" && \n> \t\t   ! test -s \"$2\"\n> \t\tthen\n> \t\t\terror \"bug in test script: using test_cmp to check empty file; use test_must_be_empty instead\"\n> \t\tfi\n> \t\ttest_cmp_allow_empty \"$@\"\n> \t}\n> \n>      This will still be reporting to the developer clearly, but\n>      will only catch cases exactly like the bogus test in t5310.\n\nDoesn't that have the opposite issue? If we expect non-empty output but\nthe command produces empty output, we'd say \"bug in the test script\".\nBut that is not true at all; it's a failed test.\n\nIf we assume that \"expect\" is first (which is our convention but not\nnecessarily guaranteed), then I think the best logic is something like:\n\n  if $1 is empty; then\n    bug in the test script\n  elif test_cmp_allow_empty \"$@\"\n    test failure\n\nWe do not need to check $2 at all. An empty one is either irrelevant (if\nthe expectation is empty), or a test failure (because it would not match\nthe non-empty $1).\n\nIf we go that route, we should make sure that test_cmp's documentation\nis updated to mention that the two sides are not symmetric (and possibly\nupdate some callers, though I'd be OK leaving ones that aren't broken by\nthis, and just clean them up over time).\n\n-Peff\n"},{"id":"356057","messageId":"aedb7f33-65cc-5d63-4e7e-dd5f37831e19@gmail.com","threadId":"49122","inReplyTo":"20180819203253.GA5789@sigill.intra.peff.net","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2018-08-19T21:37:59Z","receivedAt":"2018-08-19T21:38:07Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 19/08/18 22:32, Jeff King wrote:\n> On Sun, Aug 19, 2018 at 07:50:42PM +0200, Andrei Rybak wrote:\n> \n>>   1. Check both files at the same time (combination with Gábor's\n>>      function):\n>>\n>> \ttest_cmp () {\n>> \t\tif test \"$1\" != - &&\n>> \t\t   test \"$2\" != - &&\n>> \t\t   ! test -s \"$1\" && \n>> \t\t   ! test -s \"$2\"\n>> \t\tthen\n>> \t\t\terror \"bug in test script: using test_cmp to check empty file; use test_must_be_empty instead\"\n>> \t\tfi\n>> \t\ttest_cmp_allow_empty \"$@\"\n>> \t}\n>>\n>>      This will still be reporting to the developer clearly, but\n>>      will only catch cases exactly like the bogus test in t5310.\n> \n> Doesn't that have the opposite issue? If we expect non-empty output but\n> the command produces empty output, we'd say \"bug in the test script\".\n> But that is not true at all; it's a failed test.\n\nNo. Only when both \"$1\" and \"$2\" are empty files will the function above\nreport \"bug in test script\". From patch's commit message:\n\n  ... both invocations produce empty 'pack{a,b}.objects' files, and the\n  subsequent 'test_cmp' happily finds those two empty files identical.\n\nThat's what I meant by \"will only catch cases exactly like the bogus\ntest in t5310\".\n\nHowever ...\n\n> If we assume that \"expect\" is first (which is our convention but not\n> necessarily guaranteed), then I think the best logic is something like:\n> \n>   if $1 is empty; then\n>     bug in the test script\n>   elif test_cmp_allow_empty \"$@\"\n>     test failure\n> \n> We do not need to check $2 at all. An empty one is either irrelevant (if\n> the expectation is empty), or a test failure (because it would not match\n> the non-empty $1).\n\n... this is indeed a better solution. I written out the cases for\nupdated test_cmp to straighten out my thinking:\n\n * both $1 and $2 are empty:\n     bogus test:\n       needs either fixing generation of both expect and actual\n       or switching to test_must_be_empty\n   OR\n     bogus helper function, as Gábor described above:\n       needs to switch to test_cmp_allow_empty\n \n * $1 is non-empty && $2 is empty\n   proceeding with test\n   test failure from GIT_TEST_CMP\n \n * $1 is empty && $2 is non-empty\n   bogus test - needs either switching to test_must_be_empty\n   (and after that test_must_be_empty will report failure)\n   or fixing generation of expect (and after that test result\n   depends on contents).\n \n * both $1 and $2 are non-empty\n   proceeding with test\n   result depends on contents\n"},{"id":"356058","messageId":"20180819214334.GA2626@sigill.intra.peff.net","threadId":"49122","inReplyTo":"aedb7f33-65cc-5d63-4e7e-dd5f37831e19@gmail.com","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-19T21:43:34Z","receivedAt":"2018-08-19T21:43:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 19, 2018 at 11:37:59PM +0200, Andrei Rybak wrote:\n\n> On 19/08/18 22:32, Jeff King wrote:\n> > On Sun, Aug 19, 2018 at 07:50:42PM +0200, Andrei Rybak wrote:\n> > \n> >>   1. Check both files at the same time (combination with Gábor's\n> >>      function):\n> >>\n> >> \ttest_cmp () {\n> >> \t\tif test \"$1\" != - &&\n> >> \t\t   test \"$2\" != - &&\n> >> \t\t   ! test -s \"$1\" && \n> >> \t\t   ! test -s \"$2\"\n> >> \t\tthen\n> >> \t\t\terror \"bug in test script: using test_cmp to check empty file; use test_must_be_empty instead\"\n> >> \t\tfi\n> >> \t\ttest_cmp_allow_empty \"$@\"\n> >> \t}\n> >>\n> >>      This will still be reporting to the developer clearly, but\n> >>      will only catch cases exactly like the bogus test in t5310.\n> > \n> > Doesn't that have the opposite issue? If we expect non-empty output but\n> > the command produces empty output, we'd say \"bug in the test script\".\n> > But that is not true at all; it's a failed test.\n> \n> No. Only when both \"$1\" and \"$2\" are empty files will the function above\n> report \"bug in test script\". From patch's commit message:\n\nOh, you're right. Somewhere between the screen and my brain the \"&&\"\nbecame an \"||\".\n\nI agree that works, and has the advantage that the arguments are treated\nsymmetrically. We _might_ say \"test failure\" instead of \"bug in test\"\nwhen the expectation is empty and the generated output is not (because\nwe do not know which is which), but I think that would be uncommon (and\nthe most important thing is that we do not silently consider it a pass).\n\n> > If we assume that \"expect\" is first (which is our convention but not\n> > necessarily guaranteed), then I think the best logic is something like:\n> > \n> >   if $1 is empty; then\n> >     bug in the test script\n> >   elif test_cmp_allow_empty \"$@\"\n> >     test failure\n> > \n> > We do not need to check $2 at all. An empty one is either irrelevant (if\n> > the expectation is empty), or a test failure (because it would not match\n> > the non-empty $1).\n> \n> ... this is indeed a better solution. I written out the cases for\n> updated test_cmp to straighten out my thinking:\n\nI'd be OK pursuing either this line, or what you showed originally.\n\n-Peff\n"},{"id":"356224","messageId":"xmqqh8jn75g2.fsf@gitster-ct.c.googlers.com","threadId":"49122","inReplyTo":"20180819214334.GA2626@sigill.intra.peff.net","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-08-21T21:52:45Z","receivedAt":"2018-08-21T21:52:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> > If we assume that \"expect\" is first (which is our convention but not\n>> > necessarily guaranteed), then I think the best logic is something like:\n>> > \n>> >   if $1 is empty; then\n>> >     bug in the test script\n>> >   elif test_cmp_allow_empty \"$@\"\n>> >     test failure\n>> > \n>> > We do not need to check $2 at all. An empty one is either irrelevant (if\n>> > the expectation is empty), or a test failure (because it would not match\n>> > the non-empty $1).\n>> \n>> ... this is indeed a better solution. I written out the cases for\n>> updated test_cmp to straighten out my thinking:\n>\n> I'd be OK pursuing either this line, or what you showed originally.\n\nAs I do find [1] to be a real concern, I'd prefer not to flag empty\ninput to test_cmp as special.  But if we _were_ to do something, I\nagree that \"$2 can be anything---that is the output from the\npotentially buggy program we are testing\" is the right attitude to\ntake.\n\n[Footnote]\n\n*1*\nhttps://public-inbox.org/git/CAM0VKjkT7fBJRie_3f4B13BHT9hp9MxRhuX5r1sogh2x7KQzbg@mail.gmail.com/\n"},{"id":"356296","messageId":"20180822181444.169611-1-matvore@google.com","threadId":"49122","inReplyTo":"4d6ffc81-a1e8-a60f-d53f-2ec159160fcd@gmail.com","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"Matthew DeVore","fromEmail":"matvore@google.com","sentAt":"2018-08-22T18:14:44Z","receivedAt":"2018-08-22T18:14:49Z","isPatch":true,"sender":{"key":"matvore@google.com","avatar":"https://avatars.githubusercontent.com/u/946637?v=4"},"body":"I would encourage use of an existing function to check for emptiness,\nbut require a particular argument for it to be considered \"the right\nway:\"\n\ntest_cmp /dev/null actual\n\nThis means less vocabulary to memorize for test writers. It's usually a\ncode smell to have special logic for a specific value for a specific\nargument - a sign that a separate function ought to be created - but\nsince we want to add an error or warning in test_cmp anyway when\n<EXPECTED> is empty, I think this special logic is OK.\n\nAs for comparing against a file that *might* be empty, like a utility\nfunction, might I suggest requiring the file name be formatted in a\nspecific way if it may be empty? Like require a certain substring. Then\nthe syntax for comparison would be:\n\n# If the emptiness is unconditional\ntest_cmp /dev/null actual\n\n# If the emptiness is unknown ahead of time\ntest_cmp maybe_empty_expected actual\n\nThen, issue an error for something like:\n> expected && test_cmp expected actual\n\nwhich says: \"\nUse test_cmp /dev/null <ACTUAL> to verify a file is empty.\nIf the <EXPECTED> file may or may not be empty (as in a utility\nfunction), include the string \"maybe_empty\" in the <EXPECTED> file name.\n\"\n"},{"id":"356571","messageId":"20180827102238.GA26738@deco.navytux.spb.ru","threadId":"49122","inReplyTo":"20180814114721.25577-1-szeder.dev@gmail.com","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"Kirill Smelkov","fromEmail":"kirr@nexedi.com","sentAt":"2018-08-27T10:22:46Z","receivedAt":"2018-08-27T10:37:49Z","isPatch":true,"sender":{"key":"kirr@nexedi.com","avatar":"https://avatars.githubusercontent.com/u/1794248?v=4"},"body":"On Tue, Aug 14, 2018 at 01:47:21PM +0200, SZEDER Gábor wrote:\n> The test 'pack-objects to file can use bitmap' added in 645c432d61\n> (pack-objects: use reachability bitmap index when generating\n> non-stdout pack, 2016-09-10) is silently buggy and doesn't check what\n> it's supposed to.\n> \n> In 't5310-pack-bitmaps.sh', the 'list_packed_objects' helper function\n> does what its name implies by running:\n> \n>   git show-index <\"$1\" | cut -d' ' -f2\n> \n> The test in question invokes this function like this:\n> \n>   list_packed_objects <packa-$packasha1.idx >packa.objects &&\n>   list_packed_objects <packb-$packbsha1.idx >packb.objects &&\n>   test_cmp packa.objects packb.objects\n> \n> Note how these two callsites don't specify the name of the pack index\n> file as the function's parameter, but redirect the function's standard\n> input from it.  This triggers an error message from the shell, as it\n> has no filename to redirect from in the function, but this error is\n> ignored, because it happens upstream of a pipe.  Consequently, both\n> invocations produce empty 'pack{a,b}.objects' files, and the\n> subsequent 'test_cmp' happily finds those two empty files identical.\n> \n> Fix these two 'list_packed_objects' invocations by specifying the pack\n> index files as parameters.  Furthermore, eliminate the pipe in that\n> function by replacing it with an &&-chained pair of commands using an\n> intermediate file, so a failure of 'git show-index' or the shell\n> redirection will fail the test.\n> \n> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n> ---\n>  t/t5310-pack-bitmaps.sh | 7 ++++---\n>  1 file changed, 4 insertions(+), 3 deletions(-)\n> \n> diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh\n> index 6ee4d3f2d9..557bd0d0c0 100755\n> --- a/t/t5310-pack-bitmaps.sh\n> +++ b/t/t5310-pack-bitmaps.sh\n> @@ -9,7 +9,8 @@ objpath () {\n>  \n>  # show objects present in pack ($1 should be associated *.idx)\n>  list_packed_objects () {\n> -\tgit show-index <\"$1\" | cut -d' ' -f2\n> +\tgit show-index <\"$1\" >object-list &&\n> +\tcut -d' ' -f2 object-list\n>  }\n>  \n>  # has_any pattern-file content-file\n> @@ -204,8 +205,8 @@ test_expect_success 'pack-objects to file can use bitmap' '\n>  \t# verify equivalent packs are generated with/without using bitmap index\n>  \tpackasha1=$(git pack-objects --no-use-bitmap-index --all packa </dev/null) &&\n>  \tpackbsha1=$(git pack-objects --use-bitmap-index --all packb </dev/null) &&\n> -\tlist_packed_objects <packa-$packasha1.idx >packa.objects &&\n> -\tlist_packed_objects <packb-$packbsha1.idx >packb.objects &&\n> +\tlist_packed_objects packa-$packasha1.idx >packa.objects &&\n> +\tlist_packed_objects packb-$packbsha1.idx >packb.objects &&\n>  \ttest_cmp packa.objects packb.objects\n>  '\n\nThanks for catching and correcting this.\n\nA minor comment from outside observer: running tests under something\nlike\n\n\t-e and -o pipefail\n\nwould automatically catch the mistake in the first place. Maybe `-o\npipefail` is bashism (I had not checked), but `git grep \" | \" t/` shows\nthere are a lot of pipelines being used, and thus similar errors might be\nsilently resting there. Something like -o pipefail would catch all such\nproblems automatically.\n\nKirill\n\n"},{"id":"356638","messageId":"20180827230452.GC10402@sigill.intra.peff.net","threadId":"49122","inReplyTo":"20180827102238.GA26738@deco.navytux.spb.ru","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-08-27T23:04:52Z","receivedAt":"2018-08-27T23:04:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 27, 2018 at 10:22:46AM +0000, Kirill Smelkov wrote:\n\n> A minor comment from outside observer: running tests under something\n> like\n> \n> \t-e and -o pipefail\n> \n> would automatically catch the mistake in the first place. Maybe `-o\n> pipefail` is bashism (I had not checked), but `git grep \" | \" t/` shows\n> there are a lot of pipelines being used, and thus similar errors might be\n> silently resting there. Something like -o pipefail would catch all such\n> problems automatically.\n\nYes, \"pipefail\" is a bash-ism that we can't rely on.\n\nI will say that I have been bitten before by \"set -e -o pipefail\" and\nits subtle handling of SIGPIPE. Try this:\n\n  set -e -o pipefail\n  yes | head\n\n-Peff\n"},{"id":"356647","messageId":"20180828063723.GA10059@deco.navytux.spb.ru","threadId":"49122","inReplyTo":"20180827230452.GC10402@sigill.intra.peff.net","subject":"Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test","fromName":"Kirill Smelkov","fromEmail":"kirr@nexedi.com","sentAt":"2018-08-28T06:37:28Z","receivedAt":"2018-08-28T06:52:32Z","isPatch":true,"sender":{"key":"kirr@nexedi.com","avatar":"https://avatars.githubusercontent.com/u/1794248?v=4"},"body":"On Mon, Aug 27, 2018 at 07:04:52PM -0400, Jeff King wrote:\n> On Mon, Aug 27, 2018 at 10:22:46AM +0000, Kirill Smelkov wrote:\n> \n> > A minor comment from outside observer: running tests under something\n> > like\n> > \n> > \t-e and -o pipefail\n> > \n> > would automatically catch the mistake in the first place. Maybe `-o\n> > pipefail` is bashism (I had not checked), but `git grep \" | \" t/` shows\n> > there are a lot of pipelines being used, and thus similar errors might be\n> > silently resting there. Something like -o pipefail would catch all such\n> > problems automatically.\n> \n> Yes, \"pipefail\" is a bash-ism that we can't rely on.\n> \n> I will say that I have been bitten before by \"set -e -o pipefail\" and\n> its subtle handling of SIGPIPE. Try this:\n> \n>   set -e -o pipefail\n>   yes | head\n\nThanks for the information. Oh well...\n"}]}