{"thread":{"id":"56131","subject":"[PATCH 1/2] bundle tests: use \">file\" not \": >file\"","startedAt":"2021-07-20T11:52:37Z","lastAt":"2021-07-22T18:20:23Z","messageCount":14,"participants":["Ævar Arnfjörð Bjarmason","Taylor Blau","Junio C Hamano","Felipe Contreras"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"430617","messageId":"patch-1.2-746d727113b-20210720T115052Z-avarab@gmail.com","threadId":"56131","inReplyTo":"cover-0.2-00000000000-20210720T115052Z-avarab@gmail.com","subject":"[PATCH 1/2] bundle tests: use \">file\" not \": >file\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T11:52:08Z","receivedAt":"2021-07-20T11:52:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change redundant uses of \":\" on the LHS of a \">\" to the more commonly\nuse \">file\" pattern.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t5607-clone-bundle.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh\nindex f4c383cd5ce..c9323a08fe8 100755\n--- a/t/t5607-clone-bundle.sh\n+++ b/t/t5607-clone-bundle.sh\n@@ -54,14 +54,14 @@ test_expect_success 'bundle --stdin <rev-list options>' '\n '\n \n test_expect_success 'empty bundle file is rejected' '\n-\t: >empty-bundle &&\n+\t>empty-bundle &&\n \ttest_must_fail git fetch empty-bundle\n '\n \n # This triggers a bug in older versions where the resulting line (with\n # --pretty=oneline) was longer than a 1024-char buffer.\n test_expect_success 'ridiculously long subject in boundary' '\n-\t: >file4 &&\n+\t>file4 &&\n \ttest_tick &&\n \tgit add file4 &&\n \tprintf \"%01200d\\n\" 0 | git commit -F - &&\n@@ -75,7 +75,7 @@ test_expect_success 'ridiculously long subject in boundary' '\n '\n \n test_expect_success 'prerequisites with an empty commit message' '\n-\t: >file1 &&\n+\t>file1 &&\n \tgit add file1 &&\n \ttest_tick &&\n \tgit commit --allow-empty-message -m \"\" &&\n-- \n2.32.0.874.ge7a9d58bfcf\n\n"},{"id":"430618","messageId":"cover-0.2-00000000000-20210720T115052Z-avarab@gmail.com","threadId":"56131","inReplyTo":null,"subject":"[PATCH 0/2] bundle tests: modernize, fix missing coverage & test_cmp","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T11:52:07Z","receivedAt":"2021-07-20T11:52:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A trivial series to improve the bundle tests a bit. This is split off\nfrom some larger changes to \"git bundle\" I have cooking, where the\n\"test_cmp\" in 2/2 helped assert & catch regressions.\n\nÆvar Arnfjörð Bjarmason (2):\n  bundle tests: use \">file\" not \": >file\"\n  bundle tests: use test_cmp instead of grep\n\n t/t5607-clone-bundle.sh | 74 ++++++++++++++++++++++++++++++++---------\n 1 file changed, 58 insertions(+), 16 deletions(-)\n\n-- \n2.32.0.874.ge7a9d58bfcf\n\n"},{"id":"430619","messageId":"patch-2.2-062f34abf1a-20210720T115052Z-avarab@gmail.com","threadId":"56131","inReplyTo":"cover-0.2-00000000000-20210720T115052Z-avarab@gmail.com","subject":"[PATCH 2/2] bundle tests: use test_cmp instead of grep","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-20T11:52:09Z","receivedAt":"2021-07-20T11:53:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the bundle tests to fully compare the expected \"git ls-remote\"\nor \"git bundle list-heads\" output, instead of merely grepping it.\n\nThis avoids subtle regressions in the tests. In\nf62e0a39b6 (t5704 (bundle): add tests for bundle --stdin, 2010-04-19)\nthe \"bundle --stdin <rev-list options>\" test was added to make sure we\ndidn't include the tag.\n\nBut since the --stdin mode didn't work until 5bb0fd2cab (bundle:\narguments can be read from stdin, 2021-01-11) our grepping of\n\"master\" (later \"main\") missed the important part of the test.\n\nNamely that we should not include the \"refs/tags/tag\" tag in that\ncase. Since the test only grepped for \"main\" in the output we'd miss a\nregression in that code.\n\nSo let's use test_cmp instead, and also in the other nearby tests\nwhere it's easy.\n\nThis does make things a bit more verbose in the case of the test\nthat's checking the bundle header, since it's different under SHA1 and\nSHA256. I think this makes test easier to follow.\n\nI've got some WIP changes to extend the \"git bundle\" command to dump\nparts of the header out, which are easier to understand if we test the\noutput explicitly like this.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t5607-clone-bundle.sh | 68 +++++++++++++++++++++++++++++++++--------\n 1 file changed, 55 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh\nindex c9323a08fe8..a7f18407e32 100755\n--- a/t/t5607-clone-bundle.sh\n+++ b/t/t5607-clone-bundle.sh\n@@ -29,11 +29,21 @@ test_expect_success '\"verify\" needs a worktree' '\n \n test_expect_success 'annotated tags can be excluded by rev-list options' '\n \tgit bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 &&\n-\tgit ls-remote bundle > output &&\n-\tgrep tag output &&\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse HEAD)\tHEAD\n+\t$(git rev-parse tag)\trefs/tags/tag\n+\t$(git rev-parse main)\trefs/heads/main\n+\tEOF\n+\tgit ls-remote bundle >actual &&\n+\ttest_cmp expect actual &&\n+\n \tgit bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&\n-\tgit ls-remote bundle > output &&\n-\t! grep tag output\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse HEAD)\tHEAD\n+\t$(git rev-parse main)\trefs/heads/main\n+\tEOF\n+\tgit ls-remote bundle >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'die if bundle file cannot be created' '\n@@ -43,14 +53,20 @@ test_expect_success 'die if bundle file cannot be created' '\n \n test_expect_success 'bundle --stdin' '\n \techo main | git bundle create stdin-bundle.bdl --stdin &&\n-\tgit ls-remote stdin-bundle.bdl >output &&\n-\tgrep main output\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main)\trefs/heads/main\n+\tEOF\n+\tgit ls-remote stdin-bundle.bdl >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'bundle --stdin <rev-list options>' '\n \techo main | git bundle create hybrid-bundle.bdl --stdin tag &&\n-\tgit ls-remote hybrid-bundle.bdl >output &&\n-\tgrep main output\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main)\trefs/heads/main\n+\tEOF\n+\tgit ls-remote stdin-bundle.bdl >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'empty bundle file is rejected' '\n@@ -67,11 +83,33 @@ test_expect_success 'ridiculously long subject in boundary' '\n \tprintf \"%01200d\\n\" 0 | git commit -F - &&\n \ttest_commit fifth &&\n \tgit bundle create long-subject-bundle.bdl HEAD^..HEAD &&\n-\tgit bundle list-heads long-subject-bundle.bdl >heads &&\n-\ttest -s heads &&\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main) HEAD\n+\tEOF\n+\tgit bundle list-heads long-subject-bundle.bdl >actual &&\n+\ttest_cmp expect actual &&\n+\n \tgit fetch long-subject-bundle.bdl &&\n-\tsed -n \"/^-/{p;q;}\" long-subject-bundle.bdl >boundary &&\n-\tgrep \"^-$OID_REGEX \" boundary\n+\n+\tcat >expect.common <<-EOF &&\n+\t-$(git log --pretty=format:\"%H %s\" -1 HEAD^)\n+\t$(git rev-parse HEAD) HEAD\n+\tEOF\n+\tif test_have_prereq SHA1\n+\tthen\n+\t\tcp expect.common expect\n+\telse\n+\t\techo @object-format=sha256 >expect\n+\t\tcat expect.common >>expect\n+\tfi &&\n+\tif test_have_prereq SHA1\n+\tthen\n+\t\thead -n 3 long-subject-bundle.bdl >bundle-header\n+\telse\n+\t\thead -n 4 long-subject-bundle.bdl >bundle-header\n+\tfi &&\n+\tgrep -v \"^#\" bundle-header >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'prerequisites with an empty commit message' '\n@@ -103,7 +141,11 @@ test_expect_success 'fetch SHA-1 from bundle' '\n \n test_expect_success 'git bundle uses expected default format' '\n \tgit bundle create bundle HEAD^.. &&\n-\thead -n1 bundle | grep \"^# v$(test_oid version) git bundle$\"\n+\tcat >expect <<-EOF &&\n+\t# v$(test_oid version) git bundle\n+\tEOF\n+\thead -n1 bundle >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'git bundle v3 has expected contents' '\n-- \n2.32.0.874.ge7a9d58bfcf\n\n"},{"id":"430668","messageId":"YPcE24InpY4evFyE@nand.local","threadId":"56131","inReplyTo":"patch-2.2-062f34abf1a-20210720T115052Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] bundle tests: use test_cmp instead of grep","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-20T17:16:11Z","receivedAt":"2021-07-20T17:16:33Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jul 20, 2021 at 01:52:09PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> So let's use test_cmp instead, and also in the other nearby tests\n> where it's easy.\n\nI took a look at this patch carefully to make sure that this\ntransformation also improved the readability, too.\n\nLooking around, I think that this was a good improvement in readability,\nbut also hardened the tests (for the reasons that you mentioned). One\ntiny note below.\n\n>  test_expect_success 'empty bundle file is rejected' '\n> @@ -67,11 +83,33 @@ test_expect_success 'ridiculously long subject in boundary' '\n>  \tprintf \"%01200d\\n\" 0 | git commit -F - &&\n>  \ttest_commit fifth &&\n>  \tgit bundle create long-subject-bundle.bdl HEAD^..HEAD &&\n> -\tgit bundle list-heads long-subject-bundle.bdl >heads &&\n> -\ttest -s heads &&\n> +\tcat >expect <<-EOF &&\n> +\t$(git rev-parse main) HEAD\n> +\tEOF\n> +\tgit bundle list-heads long-subject-bundle.bdl >actual &&\n> +\ttest_cmp expect actual &&\n\nThis is quite readable, but the assertion below gets much more\ncomplicated as a result of the change.\n\n>  \tgit fetch long-subject-bundle.bdl &&\n> -\tsed -n \"/^-/{p;q;}\" long-subject-bundle.bdl >boundary &&\n> -\tgrep \"^-$OID_REGEX \" boundary\n> +\n> +\tcat >expect.common <<-EOF &&\n> +\t-$(git log --pretty=format:\"%H %s\" -1 HEAD^)\n> +\t$(git rev-parse HEAD) HEAD\n> +\tEOF\n> +\tif test_have_prereq SHA1\n> +\tthen\n> +\t\tcp expect.common expect\n> +\telse\n> +\t\techo @object-format=sha256 >expect\n> +\t\tcat expect.common >>expect\n> +\tfi &&\n\nHere we're setting up expect, but I think flipping the order might make\nthings a little easier to follow. Maybe something like this:\n\n    rm expect &&\n    if ! test_have_prereq SHA1\n    then\n      echo \"@object-format=sha256\" >expect\n    fi &&\n    cat >>expect <<-EOF &&\n    -$(git log --pretty=format:\"%H %s\" -1 HEAD^)\n    $(git rev-parse HEAD) HEAD\n    EOF &&\n\nOr, if you wanted to go further, you could do something like:\n\n    cat >expect <<-EOF\n    $(test_have_prereq SHA1 || echo \"@object-format=sha256\")\n    -$(git log --pretty=format:\"%H %s\" -1 HEAD^)\n    $(git rev-parse HEAD) HEAD\n    EOF\n\nwhich is arguably a little tighter (although I find the\necho-in-a-heredoc thing to be kind of ugly).\n\n> +\tif test_have_prereq SHA1\n> +\tthen\n> +\t\thead -n 3 long-subject-bundle.bdl >bundle-header\n> +\telse\n> +\t\thead -n 4 long-subject-bundle.bdl >bundle-header\n> +\tfi &&\n> +\tgrep -v \"^#\" bundle-header >actual &&\n\nHere I would suggest getting rid of the bundle-header intermediary and\ninstead writing:\n\n    if test_have_prereq SHA1\n    then\n      head -n 3 long-subject-bundle.bdl\n    else\n      head -n 4 long-subject-bundle.bdl\n    fi | grep -v \"^#\" >actual\n\nand then having your\n\n> +\ttest_cmp expect actual\n\nbelow.\n\nThanks,\nTaylor\n"},{"id":"430669","messageId":"YPcFccdfhW1rvxaw@nand.local","threadId":"56131","inReplyTo":"cover-0.2-00000000000-20210720T115052Z-avarab@gmail.com","subject":"Re: [PATCH 0/2] bundle tests: modernize, fix missing coverage & test_cmp","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-20T17:18:41Z","receivedAt":"2021-07-20T17:20:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jul 20, 2021 at 01:52:07PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> A trivial series to improve the bundle tests a bit. This is split off\n> from some larger changes to \"git bundle\" I have cooking, where the\n> \"test_cmp\" in 2/2 helped assert & catch regressions.\n\nThanks. Both patches looked good to me, and I left a couple of minor\nsuggestions on the latter patch to potentially clean things up a little\nbit.\n\nBut please feel free to ignore them, and only pick them up if you feel\nthey do improve the readability. Since I would be happy to see this\npicked up as-is, it has my:\n\n    Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\nThanks,\nTaylor\n"},{"id":"430680","messageId":"xmqqwnpkhi6p.fsf@gitster.g","threadId":"56131","inReplyTo":"patch-1.2-746d727113b-20210720T115052Z-avarab@gmail.com","subject":"Re: [PATCH 1/2] bundle tests: use \">file\" not \": >file\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-20T19:57:34Z","receivedAt":"2021-07-20T19:58:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Change redundant uses of \":\" on the LHS of a \">\" to the more commonly\n\nWhile the changes are correct, I am not sure if the adjective\n\"redundant\" would apply.  The word refers to uses of two or more\nwhen one would suffice, but in this case, you use one explicit no-op\ncommand \":\" when you need zero.\n\n> use \">file\" pattern.\n\n\"use\" -> \"used\".\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  t/t5607-clone-bundle.sh | 6 +++---\n>  1 file changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh\n> index f4c383cd5ce..c9323a08fe8 100755\n> --- a/t/t5607-clone-bundle.sh\n> +++ b/t/t5607-clone-bundle.sh\n> @@ -54,14 +54,14 @@ test_expect_success 'bundle --stdin <rev-list options>' '\n>  '\n>  \n>  test_expect_success 'empty bundle file is rejected' '\n> -\t: >empty-bundle &&\n> +\t>empty-bundle &&\n>  \ttest_must_fail git fetch empty-bundle\n>  '\n>  \n>  # This triggers a bug in older versions where the resulting line (with\n>  # --pretty=oneline) was longer than a 1024-char buffer.\n>  test_expect_success 'ridiculously long subject in boundary' '\n> -\t: >file4 &&\n> +\t>file4 &&\n>  \ttest_tick &&\n>  \tgit add file4 &&\n>  \tprintf \"%01200d\\n\" 0 | git commit -F - &&\n> @@ -75,7 +75,7 @@ test_expect_success 'ridiculously long subject in boundary' '\n>  '\n>  \n>  test_expect_success 'prerequisites with an empty commit message' '\n> -\t: >file1 &&\n> +\t>file1 &&\n>  \tgit add file1 &&\n>  \ttest_tick &&\n>  \tgit commit --allow-empty-message -m \"\" &&\n"},{"id":"430698","messageId":"60f738b161f2_14a79920870@natae.notmuch","threadId":"56131","inReplyTo":"patch-1.2-746d727113b-20210720T115052Z-avarab@gmail.com","subject":"RE: [PATCH 1/2] bundle tests: use \">file\" not \": >file\"","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-20T20:57:21Z","receivedAt":"2021-07-20T21:03:53Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> Change redundant uses of \":\" on the LHS of a \">\" to the more commonly\n> use \">file\" pattern.\n\nWhile this is redundant in bash, it's not redundant in zsh.\n\nProbably not a big deal since I don't think the test suite can run in\nzsh right now, but it's taking us on the opposite direction.\n\n-- \nFelipe Contreras"},{"id":"430846","messageId":"cover-0.2-00000000000-20210721T235251Z-avarab@gmail.com","threadId":"56131","inReplyTo":"cover-0.2-00000000000-20210720T115052Z-avarab@gmail.com","subject":"[PATCH v2 0/2] bundle tests: modernize, fix missing coverage & test_cmp","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-21T23:53:27Z","receivedAt":"2021-07-21T23:53:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A trivial series to improve the bundle tests a bit. This is split off\nfrom some larger changes to \"git bundle\" I have cooking, where the\n\"test_cmp\" in 2/2 helped assert & catch regressions.\n\nÆvar Arnfjörð Bjarmason (2):\n  bundle tests: use \">file\" not \": >file\"\n  bundle tests: use test_cmp instead of grep\n\n t/t5607-clone-bundle.sh | 72 ++++++++++++++++++++++++++++++++---------\n 1 file changed, 56 insertions(+), 16 deletions(-)\n\nRange-diff against v1:\n1:  746d727113b ! 1:  2a77f80725d bundle tests: use \">file\" not \": >file\"\n    @@ Metadata\n      ## Commit message ##\n         bundle tests: use \">file\" not \": >file\"\n     \n    -    Change redundant uses of \":\" on the LHS of a \">\" to the more commonly\n    -    use \">file\" pattern.\n    +    Change uses of \":\" on the LHS of a \">\" to the more commonly used\n    +    \">file\" pattern in t/t5607-clone-bundle.sh.\n     \n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n     \n2:  062f34abf1a ! 2:  d5504fd764c bundle tests: use test_cmp instead of grep\n    @@ t/t5607-clone-bundle.sh: test_expect_success 'ridiculously long subject in bound\n     -\tsed -n \"/^-/{p;q;}\" long-subject-bundle.bdl >boundary &&\n     -\tgrep \"^-$OID_REGEX \" boundary\n     +\n    -+\tcat >expect.common <<-EOF &&\n    ++\tif ! test_have_prereq SHA1\n    ++\tthen\n    ++\t\techo \"@object-format=sha256\"\n    ++\tfi >expect &&\n    ++\tcat >>expect <<-EOF &&\n     +\t-$(git log --pretty=format:\"%H %s\" -1 HEAD^)\n     +\t$(git rev-parse HEAD) HEAD\n     +\tEOF\n    ++\n     +\tif test_have_prereq SHA1\n     +\tthen\n    -+\t\tcp expect.common expect\n    -+\telse\n    -+\t\techo @object-format=sha256 >expect\n    -+\t\tcat expect.common >>expect\n    -+\tfi &&\n    -+\tif test_have_prereq SHA1\n    -+\tthen\n    -+\t\thead -n 3 long-subject-bundle.bdl >bundle-header\n    ++\t\thead -n 3 long-subject-bundle.bdl\n     +\telse\n    -+\t\thead -n 4 long-subject-bundle.bdl >bundle-header\n    -+\tfi &&\n    -+\tgrep -v \"^#\" bundle-header >actual &&\n    ++\t\thead -n 4 long-subject-bundle.bdl\n    ++\tfi | grep -v \"^#\" >actual &&\n    ++\n     +\ttest_cmp expect actual\n      '\n      \n-- \n2.32.0.955.ge7c5360f7e7\n\n"},{"id":"430847","messageId":"patch-1.2-2a77f80725d-20210721T235251Z-avarab@gmail.com","threadId":"56131","inReplyTo":"cover-0.2-00000000000-20210721T235251Z-avarab@gmail.com","subject":"[PATCH v2 1/2] bundle tests: use \">file\" not \": >file\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-21T23:53:28Z","receivedAt":"2021-07-21T23:53:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change uses of \":\" on the LHS of a \">\" to the more commonly used\n\">file\" pattern in t/t5607-clone-bundle.sh.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t5607-clone-bundle.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh\nindex f4c383cd5ce..c9323a08fe8 100755\n--- a/t/t5607-clone-bundle.sh\n+++ b/t/t5607-clone-bundle.sh\n@@ -54,14 +54,14 @@ test_expect_success 'bundle --stdin <rev-list options>' '\n '\n \n test_expect_success 'empty bundle file is rejected' '\n-\t: >empty-bundle &&\n+\t>empty-bundle &&\n \ttest_must_fail git fetch empty-bundle\n '\n \n # This triggers a bug in older versions where the resulting line (with\n # --pretty=oneline) was longer than a 1024-char buffer.\n test_expect_success 'ridiculously long subject in boundary' '\n-\t: >file4 &&\n+\t>file4 &&\n \ttest_tick &&\n \tgit add file4 &&\n \tprintf \"%01200d\\n\" 0 | git commit -F - &&\n@@ -75,7 +75,7 @@ test_expect_success 'ridiculously long subject in boundary' '\n '\n \n test_expect_success 'prerequisites with an empty commit message' '\n-\t: >file1 &&\n+\t>file1 &&\n \tgit add file1 &&\n \ttest_tick &&\n \tgit commit --allow-empty-message -m \"\" &&\n-- \n2.32.0.955.ge7c5360f7e7\n\n"},{"id":"430848","messageId":"patch-2.2-d5504fd764c-20210721T235251Z-avarab@gmail.com","threadId":"56131","inReplyTo":"cover-0.2-00000000000-20210721T235251Z-avarab@gmail.com","subject":"[PATCH v2 2/2] bundle tests: use test_cmp instead of grep","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-21T23:53:29Z","receivedAt":"2021-07-21T23:53:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the bundle tests to fully compare the expected \"git ls-remote\"\nor \"git bundle list-heads\" output, instead of merely grepping it.\n\nThis avoids subtle regressions in the tests. In\nf62e0a39b6 (t5704 (bundle): add tests for bundle --stdin, 2010-04-19)\nthe \"bundle --stdin <rev-list options>\" test was added to make sure we\ndidn't include the tag.\n\nBut since the --stdin mode didn't work until 5bb0fd2cab (bundle:\narguments can be read from stdin, 2021-01-11) our grepping of\n\"master\" (later \"main\") missed the important part of the test.\n\nNamely that we should not include the \"refs/tags/tag\" tag in that\ncase. Since the test only grepped for \"main\" in the output we'd miss a\nregression in that code.\n\nSo let's use test_cmp instead, and also in the other nearby tests\nwhere it's easy.\n\nThis does make things a bit more verbose in the case of the test\nthat's checking the bundle header, since it's different under SHA1 and\nSHA256. I think this makes test easier to follow.\n\nI've got some WIP changes to extend the \"git bundle\" command to dump\nparts of the header out, which are easier to understand if we test the\noutput explicitly like this.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n t/t5607-clone-bundle.sh | 66 +++++++++++++++++++++++++++++++++--------\n 1 file changed, 53 insertions(+), 13 deletions(-)\n\ndiff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh\nindex c9323a08fe8..ed0d911e953 100755\n--- a/t/t5607-clone-bundle.sh\n+++ b/t/t5607-clone-bundle.sh\n@@ -29,11 +29,21 @@ test_expect_success '\"verify\" needs a worktree' '\n \n test_expect_success 'annotated tags can be excluded by rev-list options' '\n \tgit bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 &&\n-\tgit ls-remote bundle > output &&\n-\tgrep tag output &&\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse HEAD)\tHEAD\n+\t$(git rev-parse tag)\trefs/tags/tag\n+\t$(git rev-parse main)\trefs/heads/main\n+\tEOF\n+\tgit ls-remote bundle >actual &&\n+\ttest_cmp expect actual &&\n+\n \tgit bundle create bundle --all --since=7.Apr.2005.15:16:00.-0700 &&\n-\tgit ls-remote bundle > output &&\n-\t! grep tag output\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse HEAD)\tHEAD\n+\t$(git rev-parse main)\trefs/heads/main\n+\tEOF\n+\tgit ls-remote bundle >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'die if bundle file cannot be created' '\n@@ -43,14 +53,20 @@ test_expect_success 'die if bundle file cannot be created' '\n \n test_expect_success 'bundle --stdin' '\n \techo main | git bundle create stdin-bundle.bdl --stdin &&\n-\tgit ls-remote stdin-bundle.bdl >output &&\n-\tgrep main output\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main)\trefs/heads/main\n+\tEOF\n+\tgit ls-remote stdin-bundle.bdl >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'bundle --stdin <rev-list options>' '\n \techo main | git bundle create hybrid-bundle.bdl --stdin tag &&\n-\tgit ls-remote hybrid-bundle.bdl >output &&\n-\tgrep main output\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main)\trefs/heads/main\n+\tEOF\n+\tgit ls-remote stdin-bundle.bdl >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'empty bundle file is rejected' '\n@@ -67,11 +83,31 @@ test_expect_success 'ridiculously long subject in boundary' '\n \tprintf \"%01200d\\n\" 0 | git commit -F - &&\n \ttest_commit fifth &&\n \tgit bundle create long-subject-bundle.bdl HEAD^..HEAD &&\n-\tgit bundle list-heads long-subject-bundle.bdl >heads &&\n-\ttest -s heads &&\n+\tcat >expect <<-EOF &&\n+\t$(git rev-parse main) HEAD\n+\tEOF\n+\tgit bundle list-heads long-subject-bundle.bdl >actual &&\n+\ttest_cmp expect actual &&\n+\n \tgit fetch long-subject-bundle.bdl &&\n-\tsed -n \"/^-/{p;q;}\" long-subject-bundle.bdl >boundary &&\n-\tgrep \"^-$OID_REGEX \" boundary\n+\n+\tif ! test_have_prereq SHA1\n+\tthen\n+\t\techo \"@object-format=sha256\"\n+\tfi >expect &&\n+\tcat >>expect <<-EOF &&\n+\t-$(git log --pretty=format:\"%H %s\" -1 HEAD^)\n+\t$(git rev-parse HEAD) HEAD\n+\tEOF\n+\n+\tif test_have_prereq SHA1\n+\tthen\n+\t\thead -n 3 long-subject-bundle.bdl\n+\telse\n+\t\thead -n 4 long-subject-bundle.bdl\n+\tfi | grep -v \"^#\" >actual &&\n+\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'prerequisites with an empty commit message' '\n@@ -103,7 +139,11 @@ test_expect_success 'fetch SHA-1 from bundle' '\n \n test_expect_success 'git bundle uses expected default format' '\n \tgit bundle create bundle HEAD^.. &&\n-\thead -n1 bundle | grep \"^# v$(test_oid version) git bundle$\"\n+\tcat >expect <<-EOF &&\n+\t# v$(test_oid version) git bundle\n+\tEOF\n+\thead -n1 bundle >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'git bundle v3 has expected contents' '\n-- \n2.32.0.955.ge7c5360f7e7\n\n"},{"id":"430849","messageId":"87fsw7ussi.fsf@evledraar.gmail.com","threadId":"56131","inReplyTo":"YPcE24InpY4evFyE@nand.local","subject":"Re: [PATCH 2/2] bundle tests: use test_cmp instead of grep","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-21T23:53:54Z","receivedAt":"2021-07-21T23:54:23Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jul 20 2021, Taylor Blau wrote:\n\n> On Tue, Jul 20, 2021 at 01:52:09PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> So let's use test_cmp instead, and also in the other nearby tests\n>> where it's easy.\n>\n> I took a look at this patch carefully to make sure that this\n> transformation also improved the readability, too.\n>\n> Looking around, I think that this was a good improvement in readability,\n> but also hardened the tests (for the reasons that you mentioned). One\n> tiny note below.\n>\n>>  test_expect_success 'empty bundle file is rejected' '\n>> @@ -67,11 +83,33 @@ test_expect_success 'ridiculously long subject in boundary' '\n>>  \tprintf \"%01200d\\n\" 0 | git commit -F - &&\n>>  \ttest_commit fifth &&\n>>  \tgit bundle create long-subject-bundle.bdl HEAD^..HEAD &&\n>> -\tgit bundle list-heads long-subject-bundle.bdl >heads &&\n>> -\ttest -s heads &&\n>> +\tcat >expect <<-EOF &&\n>> +\t$(git rev-parse main) HEAD\n>> +\tEOF\n>> +\tgit bundle list-heads long-subject-bundle.bdl >actual &&\n>> +\ttest_cmp expect actual &&\n>\n> This is quite readable, but the assertion below gets much more\n> complicated as a result of the change.\n>\n>>  \tgit fetch long-subject-bundle.bdl &&\n>> -\tsed -n \"/^-/{p;q;}\" long-subject-bundle.bdl >boundary &&\n>> -\tgrep \"^-$OID_REGEX \" boundary\n>> +\n>> +\tcat >expect.common <<-EOF &&\n>> +\t-$(git log --pretty=format:\"%H %s\" -1 HEAD^)\n>> +\t$(git rev-parse HEAD) HEAD\n>> +\tEOF\n>> +\tif test_have_prereq SHA1\n>> +\tthen\n>> +\t\tcp expect.common expect\n>> +\telse\n>> +\t\techo @object-format=sha256 >expect\n>> +\t\tcat expect.common >>expect\n>> +\tfi &&\n>\n> Here we're setting up expect, but I think flipping the order might make\n> things a little easier to follow. Maybe something like this:\n>\n>     rm expect &&\n>     if ! test_have_prereq SHA1\n>     then\n>       echo \"@object-format=sha256\" >expect\n>     fi &&\n>     cat >>expect <<-EOF &&\n>     -$(git log --pretty=format:\"%H %s\" -1 HEAD^)\n>     $(git rev-parse HEAD) HEAD\n>     EOF &&\n\nThanks, I used pretty much that as-is for v2.\n\n> Or, if you wanted to go further, you could do something like:\n>\n>     cat >expect <<-EOF\n>     $(test_have_prereq SHA1 || echo \"@object-format=sha256\")\n>     -$(git log --pretty=format:\"%H %s\" -1 HEAD^)\n>     $(git rev-parse HEAD) HEAD\n>     EOF\n>\n> which is arguably a little tighter (although I find the\n> echo-in-a-heredoc thing to be kind of ugly).\n\nThis one won't work because you'll have an empty line at the start under\nSHA-1.\n\n>> +\tif test_have_prereq SHA1\n>> +\tthen\n>> +\t\thead -n 3 long-subject-bundle.bdl >bundle-header\n>> +\telse\n>> +\t\thead -n 4 long-subject-bundle.bdl >bundle-header\n>> +\tfi &&\n>> +\tgrep -v \"^#\" bundle-header >actual &&\n>\n> Here I would suggest getting rid of the bundle-header intermediary and\n> instead writing:\n>\n>     if test_have_prereq SHA1\n>     then\n>       head -n 3 long-subject-bundle.bdl\n>     else\n>       head -n 4 long-subject-bundle.bdl\n>     fi | grep -v \"^#\" >actual\n>\n> and then having your\n\nThanks, used that.\n"},{"id":"430940","messageId":"YPm1ODTi2wX103My@nand.local","threadId":"56131","inReplyTo":"87fsw7ussi.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/2] bundle tests: use test_cmp instead of grep","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-22T18:13:12Z","receivedAt":"2021-07-22T18:20:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jul 22, 2021 at 01:53:54AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> > Or, if you wanted to go further, you could do something like:\n> >\n> >     cat >expect <<-EOF\n> >     $(test_have_prereq SHA1 || echo \"@object-format=sha256\")\n> >     -$(git log --pretty=format:\"%H %s\" -1 HEAD^)\n> >     $(git rev-parse HEAD) HEAD\n> >     EOF\n> >\n> > which is arguably a little tighter (although I find the\n> > echo-in-a-heredoc thing to be kind of ugly).\n>\n> This one won't work because you'll have an empty line at the start under\n> SHA-1.\n\nAh, you're totally right: good catch. I think it's avoidable by smashing\nthe first two lines into one. If the subshell prints nothing, then the\nfirst line will start with \"-$(git log ...)\", otherwise, if it does\nprint something, then the echo will print a newline to separate it from\nthe \"-$(git log ...) output which will go on the second line.\n\nBut that is definitely uglier than my first suggestion, so I'm glad that\nyou didn't try and make what I wrote above work :).\n\nThanks,\nTaylor\n"},{"id":"430941","messageId":"YPm2QwoDu9TtkXQJ@nand.local","threadId":"56131","inReplyTo":"cover-0.2-00000000000-20210721T235251Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/2] bundle tests: modernize, fix missing coverage & test_cmp","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-22T18:17:39Z","receivedAt":"2021-07-22T18:20:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jul 22, 2021 at 01:53:27AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> A trivial series to improve the bundle tests a bit. This is split off\n> from some larger changes to \"git bundle\" I have cooking, where the\n> \"test_cmp\" in 2/2 helped assert & catch regressions.\n\nThanks, this version looks great to me, so I'd be happy to see it get\npicked up.\n\n    Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\nThanks,\nTaylor\n"},{"id":"430942","messageId":"YPm2KsNQxaZTIjGz@nand.local","threadId":"56131","inReplyTo":"patch-2.2-d5504fd764c-20210721T235251Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/2] bundle tests: use test_cmp instead of grep","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-07-22T18:17:14Z","receivedAt":"2021-07-22T18:20:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Jul 22, 2021 at 01:53:29AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> Change the bundle tests to fully compare the expected \"git ls-remote\"\n> or \"git bundle list-heads\" output, instead of merely grepping it.\n\nThanks; this version makes all of those tests more readable. I did have\none idle thought while reading, but what you wrote is right (so I'm just\nthinking out loud as opposed to anything else).\n\n> -\tsed -n \"/^-/{p;q;}\" long-subject-bundle.bdl >boundary &&\n> -\tgrep \"^-$OID_REGEX \" boundary\n> +\n> +\tif ! test_have_prereq SHA1\n> +\tthen\n> +\t\techo \"@object-format=sha256\"\n> +\tfi >expect &&\n> +\tcat >>expect <<-EOF &&\n\nOn my first read, I worried that this 'cat >>expect' when we're in\nSHA-1 mode would append to the 'expect' we generated a few lines up. But\nit doesn't, since we redirect the output of the 'if' statement into\n'expect' instead of doing the redirect attached to the echo.\n\nSubtle, but that's right. Good.\n\nThanks,\nTaylor\n"}]}