{"thread":{"id":"42257","subject":"t6392 broken on pu (Mac OS X)","startedAt":"2016-05-07T16:15:19Z","lastAt":"2016-05-10T06:03:14Z","messageCount":9,"participants":["Torsten Bögershausen","Jeff King","Eric Sunshine","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"285823","messageId":"dea0877d-fe83-fb47-4df3-21fd69d8421d@web.de","threadId":"42257","inReplyTo":null,"subject":"t6392 broken on pu (Mac OS X)","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-05-07T16:15:19Z","receivedAt":"2016-05-07T16:15:19Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"These tests fail here under Mac OS,\nthey pass under Linux:\ncommit ff3d9b660a4b6e9d3eeb664ce1febe717adff737\nI haven't had a chance to dig further.\n\n\nexpecting success:\n\tgit for-each-ref --format=\"%(if)%(authorname)%(then)%(authorname):\n%(refname)%(end)\" >actual &&\n\tcat >expect <<-\\EOF &&\n\tA U Thor: refs/heads/master\n\tA U Thor: refs/heads/side\n\tA U Thor: refs/odd/spot\n\n\n\n\tA U Thor: refs/tags/foo1.10\n\tA U Thor: refs/tags/foo1.3\n\tA U Thor: refs/tags/foo1.6\n\tA U Thor: refs/tags/four\n\tA U Thor: refs/tags/one\n\n\tA U Thor: refs/tags/three\n\tA U Thor: refs/tags/two\n\tEOF\n\ttest_cmp expect actual\n\n--- expect\t2016-05-07 16:08:32.000000000 +0000\n+++ actual\t2016-05-07 16:08:32.000000000 +0000\n@@ -3,12 +3,10 @@\n A U Thor: refs/odd/spot\n\n\n-\n A U Thor: refs/tags/foo1.10\n A U Thor: refs/tags/foo1.3\n A U Thor: refs/tags/foo1.6\n A U Thor: refs/tags/four\n A U Thor: refs/tags/one\n-\n A U Thor: refs/tags/three\n A U Thor: refs/tags/two\nnot ok 34 - check %(if)...%(then)...%(end) atoms\n#\t\n#\t\tgit for-each-ref --format=\"%(if)%(authorname)%(then)%(authorname):\n%(refname)%(end)\" >actual &&\n#\t\tcat >expect <<-\\EOF &&\n#\t\tA U Thor: refs/heads/master\n#\t\tA U Thor: refs/heads/side\n#\t\tA U Thor: refs/odd/spot\n#\t\n#\t\n#\t\n#\t\tA U Thor: refs/tags/foo1.10\n#\t\tA U Thor: refs/tags/foo1.3\n#\t\tA U Thor: refs/tags/foo1.6\n#\t\tA U Thor: refs/tags/four\n#\t\tA U Thor: refs/tags/one\n#\t\n#\t\tA U Thor: refs/tags/three\n#\t\tA U Thor: refs/tags/two\n#\t\tEOF\n#\t\ttest_cmp expect actual\n#\t\n\nexpecting success:\n\tgit for-each-ref --format=\"%(if)%(authorname)%(then)%(authorname)%(else)No\nauthor%(end): %(refname)\" >actual &&\n\tcat >expect <<-\\EOF &&\n\tA U Thor: refs/heads/master\n\tA U Thor: refs/heads/side\n\tA U Thor: refs/odd/spot\n\tNo author: refs/tags/annotated-tag\n\tNo author: refs/tags/doubly-annotated-tag\n\tNo author: refs/tags/doubly-signed-tag\n\tA U Thor: refs/tags/foo1.10\n\tA U Thor: refs/tags/foo1.3\n\tA U Thor: refs/tags/foo1.6\n\tA U Thor: refs/tags/four\n\tA U Thor: refs/tags/one\n\tNo author: refs/tags/signed-tag\n\tA U Thor: refs/tags/three\n\tA U Thor: refs/tags/two\n\tEOF\n\ttest_cmp expect actual\n\n--- expect\t2016-05-07 16:08:32.000000000 +0000\n+++ actual\t2016-05-07 16:08:32.000000000 +0000\n@@ -3,12 +3,10 @@\n A U Thor: refs/odd/spot\n No author: refs/tags/annotated-tag\n No author: refs/tags/doubly-annotated-tag\n-No author: refs/tags/doubly-signed-tag\n A U Thor: refs/tags/foo1.10\n A U Thor: refs/tags/foo1.3\n A U Thor: refs/tags/foo1.6\n A U Thor: refs/tags/four\n A U Thor: refs/tags/one\n-No author: refs/tags/signed-tag\n A U Thor: refs/tags/three\n A U Thor: refs/tags/two\nnot ok 35 - check %(if)...%(then)...%(else)...%(end) atoms\n#\t\n#\t\tgit for-each-ref --format=\"%(if)%(authorname)%(then)%(authorname)%(else)No\nauthor%(end): %(refname)\" >actual &&\n#\t\tcat >expect <<-\\EOF &&\n#\t\tA U Thor: refs/heads/master\n#\t\tA U Thor: refs/heads/side\n#\t\tA U Thor: refs/odd/spot\n#\t\tNo author: refs/tags/annotated-tag\n#\t\tNo author: refs/tags/doubly-annotated-tag\n#\t\tNo author: refs/tags/doubly-signed-tag\n#\t\tA U Thor: refs/tags/foo1.10\n#\t\tA U Thor: refs/tags/foo1.3\n#\t\tA U Thor: refs/tags/foo1.6\n#\t\tA U Thor: refs/tags/four\n#\t\tA U Thor: refs/tags/one\n#\t\tNo author: refs/tags/signed-tag\n#\t\tA U Thor: refs/tags/three\n#\t\tA U Thor: refs/tags/two\n#\t\tEOF\n#\t\ttest_cmp expect actual\n#\t\n\nexpecting success:\n\tgit for-each-ref --format=\"%(refname:short): %(if)%(HEAD)%(then)Head\nref%(else)Not Head ref%(end)\" >actual &&\n\tcat >expect <<-\\EOF &&\n\tmaster: Head ref\n\tside: Not Head ref\n\todd/spot: Not Head ref\n\tannotated-tag: Not Head ref\n\tdoubly-annotated-tag: Not Head ref\n\tdoubly-signed-tag: Not Head ref\n\tfoo1.10: Not Head ref\n\tfoo1.3: Not Head ref\n\tfoo1.6: Not Head ref\n\tfour: Not Head ref\n\tone: Not Head ref\n\tsigned-tag: Not Head ref\n\tthree: Not Head ref\n\ttwo: Not Head ref\n\tEOF\n\ttest_cmp expect actual\n\n--- expect\t2016-05-07 16:08:32.000000000 +0000\n+++ actual\t2016-05-07 16:08:32.000000000 +0000\n@@ -3,12 +3,10 @@\n odd/spot: Not Head ref\n annotated-tag: Not Head ref\n doubly-annotated-tag: Not Head ref\n-doubly-signed-tag: Not Head ref\n foo1.10: Not Head ref\n foo1.3: Not Head ref\n foo1.6: Not Head ref\n four: Not Head ref\n one: Not Head ref\n-signed-tag: Not Head ref\n three: Not Head ref\n two: Not Head ref\nnot ok 36 - ignore spaces in %(if) atom usage\n#\t\n#\t\tgit for-each-ref --format=\"%(refname:short): %(if)%(HEAD)%(then)Head\nref%(else)Not Head ref%(end)\" >actual &&\n#\t\tcat >expect <<-\\EOF &&\n#\t\tmaster: Head ref\n#\t\tside: Not Head ref\n#\t\todd/spot: Not Head ref\n#\t\tannotated-tag: Not Head ref\n#\t\tdoubly-annotated-tag: Not Head ref\n#\t\tdoubly-signed-tag: Not Head ref\n#\t\tfoo1.10: Not Head ref\n#\t\tfoo1.3: Not Head ref\n#\t\tfoo1.6: Not Head ref\n#\t\tfour: Not Head ref\n#\t\tone: Not Head ref\n#\t\tsigned-tag: Not Head ref\n#\t\tthree: Not Head ref\n#\t\ttwo: Not Head ref\n#\t\tEOF\n#\t\ttest_cmp expect actual\n#\t\n"},{"id":"285927","messageId":"20160509160725.GA11861@sigill.intra.peff.net","threadId":"42257","inReplyTo":"dea0877d-fe83-fb47-4df3-21fd69d8421d@web.de","subject":"Re: t6392 broken on pu (Mac OS X)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-09T16:07:25Z","receivedAt":"2016-05-09T16:07:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 07, 2016 at 06:15:19PM +0200, Torsten Bögershausen wrote:\n\n> These tests fail here under Mac OS,\n> they pass under Linux:\n> commit ff3d9b660a4b6e9d3eeb664ce1febe717adff737\n> I haven't had a chance to dig further.\n\nI assume you mean t6302. It looks like the difference is not Mac OS, but\nrather that the GPG prerequisite is not fulfilled, so we are missing a\nfew of the tags.\n\nCommit 618310a introduced a helper to munge the \"expect\" output. Using\nthat fixes some of the cases, but not test 34. That one is expecting\nblank lines for tags, so test_prepare_expect doesn't know which lines\nare related to GPG.\n\nWe could fix it by tweaking the test like this:\n\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 7420e48..04042e1 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -343,29 +343,27 @@ test_expect_success 'improper usage of %(if), %(then), %(else) and %(end) atoms'\n '\n \n test_expect_success 'check %(if)...%(then)...%(end) atoms' '\n-\tgit for-each-ref --format=\"%(if)%(authorname)%(then)%(authorname): %(refname)%(end)\" >actual &&\n-\tcat >expect <<-\\EOF &&\n-\tA U Thor: refs/heads/master\n-\tA U Thor: refs/heads/side\n-\tA U Thor: refs/odd/spot\n-\n-\n-\n-\tA U Thor: refs/tags/foo1.10\n-\tA U Thor: refs/tags/foo1.3\n-\tA U Thor: refs/tags/foo1.6\n-\tA U Thor: refs/tags/four\n-\tA U Thor: refs/tags/one\n-\n-\tA U Thor: refs/tags/three\n-\tA U Thor: refs/tags/two\n+\tgit for-each-ref --format=\"%(refname):%(if)%(authorname)%(then) author=%(authorname)%(end)\" >actual &&\n+\ttest_prepare_expect >expect <<-\\EOF &&\n+\trefs/heads/master: author=A U Thor\n+\trefs/heads/side: author=A U Thor\n+\trefs/odd/spot: author=A U Thor\n+\trefs/tags/annotated-tag:\n+\trefs/tags/doubly-annotated-tag:\n+\trefs/tags/foo1.10: author=A U Thor\n+\trefs/tags/foo1.3: author=A U Thor\n+\trefs/tags/foo1.6: author=A U Thor\n+\trefs/tags/four: author=A U Thor\n+\trefs/tags/one: author=A U Thor\n+\trefs/tags/three: author=A U Thor\n+\trefs/tags/two: author=A U Thor\n \tEOF\n \ttest_cmp expect actual\n '\n \n test_expect_success 'check %(if)...%(then)...%(else)...%(end) atoms' '\n \tgit for-each-ref --format=\"%(if)%(authorname)%(then)%(authorname)%(else)No author%(end): %(refname)\" >actual &&\n-\tcat >expect <<-\\EOF &&\n+\ttest_prepare_expect >expect <<-\\EOF &&\n \tA U Thor: refs/heads/master\n \tA U Thor: refs/heads/side\n \tA U Thor: refs/odd/spot\n@@ -385,7 +383,7 @@ test_expect_success 'check %(if)...%(then)...%(else)...%(end) atoms' '\n '\n test_expect_success 'ignore spaces in %(if) atom usage' '\n \tgit for-each-ref --format=\"%(refname:short): %(if)%(HEAD)%(then)Head ref%(else)Not Head ref%(end)\" >actual &&\n-\tcat >expect <<-\\EOF &&\n+\ttest_prepare_expect >expect <<-\\EOF &&\n \tmaster: Head ref\n \tside: Not Head ref\n \todd/spot: Not Head ref\n\n\nThough we'd perhaps want to tweak the subsequent tests to use the same\nformat, just to make things easier to read later.\n\nHowever, I wonder if we could improve on the strategy in 618310a, and\nsimply create non-signed versions of the \"signed\" tags when GPG is not\navailable. That would make tests looking at the whole ref namespace\nmore consistent. And any tests which wanted to look specifically at the\nsigned attributes should be protected with the GPG prereq anyway (it\ndoesn't look like there are any currently, though).\n\nI.e., something like:\n\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 7420e48..a3df472 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -6,12 +6,8 @@ test_description='test for-each-refs usage of ref-filter APIs'\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n \n test_prepare_expect () {\n-\tif test_have_prereq GPG\n-\tthen\n-\t\tcat\n-\telse\n-\t\tsed '/signed/d'\n-\tfi\n+\t# XXX this could now go away entirely, and just use cat in each test\n+\tcat\n }\n \n test_expect_success 'setup some history and refs' '\n@@ -24,9 +20,12 @@ test_expect_success 'setup some history and refs' '\n \tgit tag -m \"Annonated doubly\" doubly-annotated-tag annotated-tag &&\n \tif test_have_prereq GPG\n \tthen\n-\t\tgit tag -s -m \"A signed tag\" signed-tag &&\n-\t\tgit tag -s -m \"Signed doubly\" doubly-signed-tag signed-tag\n+\t\tsign=-s\n+\telse\n+\t\tsign=\n \tfi &&\n+\tgit tag $sign -m \"A signed tag\" signed-tag &&\n+\tgit tag $sign -m \"Signed doubly\" doubly-signed-tag signed-tag &&\n \tgit checkout master &&\n \tgit update-ref refs/odd/spot master\n '\n\n-Peff\n"},{"id":"285934","messageId":"CAPig+cQ2kSVzy0K303J3Guhk3-NzcReb5V7ohqOy2pPL_5GrSA@mail.gmail.com","threadId":"42257","inReplyTo":"20160509160725.GA11861@sigill.intra.peff.net","subject":"Re: t6392 broken on pu (Mac OS X)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-09T16:30:43Z","receivedAt":"2016-05-09T16:30:43Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 9, 2016 at 12:07 PM, Jeff King <peff@peff.net> wrote:\n> On Sat, May 07, 2016 at 06:15:19PM +0200, Torsten Bögershausen wrote:\n>> These tests fail here under Mac OS,\n>> they pass under Linux:\n>> commit ff3d9b660a4b6e9d3eeb664ce1febe717adff737\n>> I haven't had a chance to dig further.\n>\n> I assume you mean t6302. It looks like the difference is not Mac OS, but\n> rather that the GPG prerequisite is not fulfilled, so we are missing a\n> few of the tags.\n>\n> Commit 618310a introduced a helper to munge the \"expect\" output. Using\n> that fixes some of the cases, but not test 34. That one is expecting\n> blank lines for tags, so test_prepare_expect doesn't know which lines\n> are related to GPG.\n>\n> We could fix it by tweaking the test like this:\n> [...snip...]\n> However, I wonder if we could improve on the strategy in 618310a, and\n> simply create non-signed versions of the \"signed\" tags when GPG is not\n> available. That would make tests looking at the whole ref namespace\n> more consistent. And any tests which wanted to look specifically at the\n> signed attributes should be protected with the GPG prereq anyway (it\n> doesn't look like there are any currently, though).\n>\n> I.e., something like:\n> [...snip...]\n>  test_expect_success 'setup some history and refs' '\n> @@ -24,9 +20,12 @@ test_expect_success 'setup some history and refs' '\n>         git tag -m \"Annonated doubly\" doubly-annotated-tag annotated-tag &&\n>         if test_have_prereq GPG\n>         then\n> -               git tag -s -m \"A signed tag\" signed-tag &&\n> -               git tag -s -m \"Signed doubly\" doubly-signed-tag signed-tag\n> +               sign=-s\n> +       else\n> +               sign=\n>         fi &&\n> +       git tag $sign -m \"A signed tag\" signed-tag &&\n> +       git tag $sign -m \"Signed doubly\" doubly-signed-tag signed-tag &&\n>         git checkout master &&\n>         git update-ref refs/odd/spot master\n>  '\n\nThe latter seems very preferable, though perhaps it could be made more\nconcise like this?\n\n    sign=\n    test_have_prereq GPG && sign=-s\n\n(But that's a minor issue.)\n"},{"id":"285937","messageId":"20160509164859.GA8231@sigill.intra.peff.net","threadId":"42257","inReplyTo":"CAPig+cQ2kSVzy0K303J3Guhk3-NzcReb5V7ohqOy2pPL_5GrSA@mail.gmail.com","subject":"[PATCH] t6302: simplify non-gpg cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-09T16:49:00Z","receivedAt":"2016-05-09T16:49:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 09, 2016 at 12:30:43PM -0400, Eric Sunshine wrote:\n\n> The latter seems very preferable, though perhaps it could be made more\n> concise like this?\n> \n>     sign=\n>     test_have_prereq GPG && sign=-s\n> \n> (But that's a minor issue.)\n\nI agree that is nicer, but I wanted to keep the definition inside the\ntest_expect_success close to its point of use. And that means we to deal\nwith the existing &&-chain (you can get around it with a {} block, but\nat that point you might as well if/then).\n\nSince you as the author of 618310a seem to agree with this direction,\nhere it is as a real patch.\n\n-- >8 --\nSubject: [PATCH] t6302: simplify non-gpg cases\n\nWhen commit 618310a taught t6302 to run without the GPG\nprerequisite, it did so by conditionally creating the signed\ntags only when gpg is available. As a result, further tests\nneed to take this into account, which they can do with the\ntest_prepare_expect helper. This is a minor hassle, though,\nas the helper cannot easily cover all cases (it just matches\n\"signed\" in the output, so all output must include the\nactual refname).\n\nInstead, let's take a different approach. We'll always\ncreate the tags, and only conditionally sign them. This does\nmean our tag-names are a minor lie, but it lets the tests\nwhich do not care about signing easily behave the same in\nall settings. We'll include a comment to document our lie\nand avoid confusing further test-writers.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t6302-for-each-ref-filter.sh | 45 +++++++++++++++++++++---------------------\n 1 file changed, 22 insertions(+), 23 deletions(-)\n\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 70afb44..3225a0b 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -5,15 +5,6 @@ test_description='test for-each-refs usage of ref-filter APIs'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n \n-test_prepare_expect () {\n-\tif test_have_prereq GPG\n-\tthen\n-\t\tcat\n-\telse\n-\t\tsed '/signed/d'\n-\tfi\n-}\n-\n test_expect_success 'setup some history and refs' '\n \ttest_commit one &&\n \ttest_commit two &&\n@@ -22,11 +13,19 @@ test_expect_success 'setup some history and refs' '\n \ttest_commit four &&\n \tgit tag -m \"An annotated tag\" annotated-tag &&\n \tgit tag -m \"Annonated doubly\" doubly-annotated-tag annotated-tag &&\n+\n+\t# Note that these \"signed\" tags might not actually be signed.\n+\t# Tests which care about the distinction should be marked\n+\t# with the GPG prereq.\n \tif test_have_prereq GPG\n \tthen\n-\t\tgit tag -s -m \"A signed tag\" signed-tag &&\n-\t\tgit tag -s -m \"Signed doubly\" doubly-signed-tag signed-tag\n+\t\tsign=-s\n+\telse\n+\t\tsign=\n \tfi &&\n+\tgit tag $sign -m \"A signed tag\" signed-tag &&\n+\tgit tag $sign -m \"Signed doubly\" doubly-signed-tag signed-tag &&\n+\n \tgit checkout master &&\n \tgit update-ref refs/odd/spot master\n '\n@@ -42,7 +41,7 @@ test_expect_success 'filtering with --points-at' '\n '\n \n test_expect_success 'check signed tags with --points-at' '\n-\ttest_prepare_expect <<-\\EOF | sed -e \"s/Z$//\" >expect &&\n+\tcat <<-\\EOF | sed -e \"s/Z$//\" >expect &&\n \trefs/heads/side Z\n \trefs/tags/annotated-tag four\n \trefs/tags/four Z\n@@ -65,7 +64,7 @@ test_expect_success 'filtering with --merged' '\n '\n \n test_expect_success 'filtering with --no-merged' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \trefs/heads/side\n \trefs/tags/annotated-tag\n \trefs/tags/doubly-annotated-tag\n@@ -78,7 +77,7 @@ test_expect_success 'filtering with --no-merged' '\n '\n \n test_expect_success 'filtering with --contains' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \trefs/heads/master\n \trefs/heads/side\n \trefs/odd/spot\n@@ -99,7 +98,7 @@ test_expect_success '%(color) must fail' '\n '\n \n test_expect_success 'left alignment is default' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \trefname is refs/heads/master  |refs/heads/master\n \trefname is refs/heads/side    |refs/heads/side\n \trefname is refs/odd/spot      |refs/odd/spot\n@@ -117,7 +116,7 @@ test_expect_success 'left alignment is default' '\n '\n \n test_expect_success 'middle alignment' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \t| refname is refs/heads/master |refs/heads/master\n \t|  refname is refs/heads/side  |refs/heads/side\n \t|   refname is refs/odd/spot   |refs/odd/spot\n@@ -135,7 +134,7 @@ test_expect_success 'middle alignment' '\n '\n \n test_expect_success 'right alignment' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \t|  refname is refs/heads/master|refs/heads/master\n \t|    refname is refs/heads/side|refs/heads/side\n \t|      refname is refs/odd/spot|refs/odd/spot\n@@ -152,7 +151,7 @@ test_expect_success 'right alignment' '\n \ttest_cmp expect actual\n '\n \n-test_prepare_expect >expect <<-\\EOF\n+cat >expect <<-\\EOF\n |       refname is refs/heads/master       |refs/heads/master\n |        refname is refs/heads/side        |refs/heads/side\n |         refname is refs/odd/spot         |refs/odd/spot\n@@ -199,7 +198,7 @@ EOF\n # Individual atoms inside %(align:...) and %(end) must not be quoted.\n \n test_expect_success 'alignment with format quote' \"\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \t|'      '\\''master| A U Thor'\\''      '|\n \t|'       '\\''side| A U Thor'\\''       '|\n \t|'     '\\''odd/spot| A U Thor'\\''     '|\n@@ -217,7 +216,7 @@ test_expect_success 'alignment with format quote' \"\n \"\n \n test_expect_success 'nested alignment with quote formatting' \"\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \t|'         master               '|\n \t|'           side               '|\n \t|'       odd/spot               '|\n@@ -235,7 +234,7 @@ test_expect_success 'nested alignment with quote formatting' \"\n \"\n \n test_expect_success 'check `%(contents:lines=1)`' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \tmaster |three\n \tside |four\n \todd/spot |three\n@@ -253,7 +252,7 @@ test_expect_success 'check `%(contents:lines=1)`' '\n '\n \n test_expect_success 'check `%(contents:lines=0)`' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \tmaster |\n \tside |\n \todd/spot |\n@@ -271,7 +270,7 @@ test_expect_success 'check `%(contents:lines=0)`' '\n '\n \n test_expect_success 'check `%(contents:lines=99999)`' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \tmaster |three\n \tside |four\n \todd/spot |three\n-- \n2.8.2.643.g361a07a\n"},{"id":"285950","messageId":"CAPig+cQPbwM0+6yruK0VKKq2ujFLoCLogS7eQNN7WWgRjG5V0w@mail.gmail.com","threadId":"42257","inReplyTo":"20160509164859.GA8231@sigill.intra.peff.net","subject":"Re: [PATCH] t6302: simplify non-gpg cases","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-09T17:47:35Z","receivedAt":"2016-05-09T17:47:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 9, 2016 at 12:49 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, May 09, 2016 at 12:30:43PM -0400, Eric Sunshine wrote:\n> Since you as the author of 618310a seem to agree with this direction,\n> here it is as a real patch.\n\nThanks for working on this.\n\n> Subject: [PATCH] t6302: simplify non-gpg cases\n>\n> When commit 618310a taught t6302 to run without the GPG\n\n618310a (t6302: skip only signed tags rather than all tests when GPG\nis missing, 2016-03-06)\n\n> prerequisite, it did so by conditionally creating the signed\n> tags only when gpg is available. As a result, further tests\n> need to take this into account, which they can do with the\n> test_prepare_expect helper. This is a minor hassle, though,\n> as the helper cannot easily cover all cases (it just matches\n> \"signed\" in the output, so all output must include the\n> actual refname).\n\nShould we cite bc9acea (ref-filter: implement %(if), %(then), and\n%(else) atoms, 2016-04-25) here as an example of a commit for which\nthis was problematic (and which indeed broke the tests when GPG is\nunavailable)?\n\n> Instead, let's take a different approach. We'll always\n> create the tags, and only conditionally sign them. This does\n> mean our tag-names are a minor lie, but it lets the tests\n> which do not care about signing easily behave the same in\n> all settings. We'll include a comment to document our lie\n> and avoid confusing further test-writers.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nLooks good. With or without the minor change below, this patch is:\n\n    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>\n\n> ---\n> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\n>  test_expect_success 'check signed tags with --points-at' '\n> -       test_prepare_expect <<-\\EOF | sed -e \"s/Z$//\" >expect &&\n> +       cat <<-\\EOF | sed -e \"s/Z$//\" >expect &&\n\nTo make this as close to a reversion as possible, this could be\nrestored to the original (sans 'cat'):\n\n    sed -e \"s/Z$//\" >expect <<-\\EOF &&\n\n>         refs/heads/side Z\n>         refs/tags/annotated-tag four\n>         refs/tags/four Z\n"},{"id":"285996","messageId":"CAPig+cT_njtzMuuk=mFWraUtpZwU3OrJPho7=dbLgKf=inBFAg@mail.gmail.com","threadId":"42257","inReplyTo":"CAOLa=ZSZqs=++Hf8CF3pWEnJqmOA9ajuX03hzLMkuQ+ehXXCVQ@mail.gmail.com","subject":"Re: [PATCH] t6302: simplify non-gpg cases","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-09T21:37:54Z","receivedAt":"2016-05-09T21:37:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 9, 2016 at 4:24 PM, Karthik Nayak <karthik.188@gmail.com> wrote:\n> On Mon, May 9, 2016 at 11:17 PM, Eric Sunshine <sunshine@sunshineco.com>\n> wrote:\n>> Should we cite bc9acea (ref-filter: implement %(if), %(then), and\n>> %(else) atoms, 2016-04-25) here as an example of a commit for which\n>> this was problematic (and which indeed broke the tests when GPG is\n>> unavailable)?\n>\n> But it's still in pu and I'll be re-rolling it, would that be acceptable?\n\nAh right, therefore, no reason to cite that particular commit. Peff's\ndescription is fine as-is.\n"},{"id":"286006","messageId":"20160510024032.GA16965@sigill.intra.peff.net","threadId":"42257","inReplyTo":"CAPig+cQPbwM0+6yruK0VKKq2ujFLoCLogS7eQNN7WWgRjG5V0w@mail.gmail.com","subject":"[PATCH v2] t6302: simplify non-gpg cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-10T02:40:33Z","receivedAt":"2016-05-10T02:40:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc Junio as this should be the final version]\n\nOn Mon, May 09, 2016 at 01:47:35PM -0400, Eric Sunshine wrote:\n\n> > Subject: [PATCH] t6302: simplify non-gpg cases\n> >\n> > When commit 618310a taught t6302 to run without the GPG\n> \n> 618310a (t6302: skip only signed tags rather than all tests when GPG\n> is missing, 2016-03-06)\n\nI sometimes intentionally avoid using that longer form when the title of\nthe commit does not convey what I want to communicate, and I have to\nsummarize the change in my own words anyway (in this case the\ninteresting thing is not _what_ it did, but _how_ it chose to do it). So\nI find including the original subject line just bloats the sentence and\nmakes the point harder to find.\n\nBut I'm curious whether other people run into that problem, or if\nreaders would prefer an unconditional full-citation. If I were writing\na book, I would probably footnote a case like this (to give extra\ncontext if somebody wants it, but not break the flow of the text). But\n\"618310a\" is already a footnote of sorts. So I dunno.\n\n> Should we cite bc9acea (ref-filter: implement %(if), %(then), and\n> %(else) atoms, 2016-04-25) here as an example of a commit for which\n> this was problematic (and which indeed broke the tests when GPG is\n> unavailable)?\n\nNope, as Karthik mentioned, we don't know the sha1 of that commit yet.\n:(\n\n> Looks good. With or without the minor change below, this patch is:\n> \n>     Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>\n\nThanks.\n\n> > -       test_prepare_expect <<-\\EOF | sed -e \"s/Z$//\" >expect &&\n> > +       cat <<-\\EOF | sed -e \"s/Z$//\" >expect &&\n> \n> To make this as close to a reversion as possible, this could be\n> restored to the original (sans 'cat'):\n> \n>     sed -e \"s/Z$//\" >expect <<-\\EOF &&\n\nThanks, I did the reversion with s/test_prepare_expect/cat/ rather than\nreverting 618310a, but I agree dropping this useless-use-of-cat is worth\ndoing. Here's v2 with that change and your reviewed-by.\n\n-- >8 --\nSubject: t6302: simplify non-gpg cases\n\nWhen commit 618310a taught t6302 to run without the GPG\nprerequisite, it did so by conditionally creating the signed\ntags only when gpg is available. As a result, further tests\nneed to take this into account, which they can do with the\ntest_prepare_expect helper. This is a minor hassle, though,\nas the helper cannot easily cover all cases (it just matches\n\"signed\" in the output, so all output must include the\nactual refname).\n\nInstead, let's take a different approach. We'll always\ncreate the tags, and only conditionally sign them. This does\nmean our tag-names are a minor lie, but it lets the tests\nwhich do not care about signing easily behave the same in\nall settings. We'll include a comment to document our lie\nand avoid confusing further test-writers.\n\nReviewed-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t6302-for-each-ref-filter.sh | 45 +++++++++++++++++++++---------------------\n 1 file changed, 22 insertions(+), 23 deletions(-)\n\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 70afb44..d0ab09f 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -5,15 +5,6 @@ test_description='test for-each-refs usage of ref-filter APIs'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n \n-test_prepare_expect () {\n-\tif test_have_prereq GPG\n-\tthen\n-\t\tcat\n-\telse\n-\t\tsed '/signed/d'\n-\tfi\n-}\n-\n test_expect_success 'setup some history and refs' '\n \ttest_commit one &&\n \ttest_commit two &&\n@@ -22,11 +13,19 @@ test_expect_success 'setup some history and refs' '\n \ttest_commit four &&\n \tgit tag -m \"An annotated tag\" annotated-tag &&\n \tgit tag -m \"Annonated doubly\" doubly-annotated-tag annotated-tag &&\n+\n+\t# Note that these \"signed\" tags might not actually be signed.\n+\t# Tests which care about the distinction should be marked\n+\t# with the GPG prereq.\n \tif test_have_prereq GPG\n \tthen\n-\t\tgit tag -s -m \"A signed tag\" signed-tag &&\n-\t\tgit tag -s -m \"Signed doubly\" doubly-signed-tag signed-tag\n+\t\tsign=-s\n+\telse\n+\t\tsign=\n \tfi &&\n+\tgit tag $sign -m \"A signed tag\" signed-tag &&\n+\tgit tag $sign -m \"Signed doubly\" doubly-signed-tag signed-tag &&\n+\n \tgit checkout master &&\n \tgit update-ref refs/odd/spot master\n '\n@@ -42,7 +41,7 @@ test_expect_success 'filtering with --points-at' '\n '\n \n test_expect_success 'check signed tags with --points-at' '\n-\ttest_prepare_expect <<-\\EOF | sed -e \"s/Z$//\" >expect &&\n+\tsed -e \"s/Z$//\" >expect <<-\\EOF &&\n \trefs/heads/side Z\n \trefs/tags/annotated-tag four\n \trefs/tags/four Z\n@@ -65,7 +64,7 @@ test_expect_success 'filtering with --merged' '\n '\n \n test_expect_success 'filtering with --no-merged' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \trefs/heads/side\n \trefs/tags/annotated-tag\n \trefs/tags/doubly-annotated-tag\n@@ -78,7 +77,7 @@ test_expect_success 'filtering with --no-merged' '\n '\n \n test_expect_success 'filtering with --contains' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \trefs/heads/master\n \trefs/heads/side\n \trefs/odd/spot\n@@ -99,7 +98,7 @@ test_expect_success '%(color) must fail' '\n '\n \n test_expect_success 'left alignment is default' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \trefname is refs/heads/master  |refs/heads/master\n \trefname is refs/heads/side    |refs/heads/side\n \trefname is refs/odd/spot      |refs/odd/spot\n@@ -117,7 +116,7 @@ test_expect_success 'left alignment is default' '\n '\n \n test_expect_success 'middle alignment' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \t| refname is refs/heads/master |refs/heads/master\n \t|  refname is refs/heads/side  |refs/heads/side\n \t|   refname is refs/odd/spot   |refs/odd/spot\n@@ -135,7 +134,7 @@ test_expect_success 'middle alignment' '\n '\n \n test_expect_success 'right alignment' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \t|  refname is refs/heads/master|refs/heads/master\n \t|    refname is refs/heads/side|refs/heads/side\n \t|      refname is refs/odd/spot|refs/odd/spot\n@@ -152,7 +151,7 @@ test_expect_success 'right alignment' '\n \ttest_cmp expect actual\n '\n \n-test_prepare_expect >expect <<-\\EOF\n+cat >expect <<-\\EOF\n |       refname is refs/heads/master       |refs/heads/master\n |        refname is refs/heads/side        |refs/heads/side\n |         refname is refs/odd/spot         |refs/odd/spot\n@@ -199,7 +198,7 @@ EOF\n # Individual atoms inside %(align:...) and %(end) must not be quoted.\n \n test_expect_success 'alignment with format quote' \"\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \t|'      '\\''master| A U Thor'\\''      '|\n \t|'       '\\''side| A U Thor'\\''       '|\n \t|'     '\\''odd/spot| A U Thor'\\''     '|\n@@ -217,7 +216,7 @@ test_expect_success 'alignment with format quote' \"\n \"\n \n test_expect_success 'nested alignment with quote formatting' \"\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \t|'         master               '|\n \t|'           side               '|\n \t|'       odd/spot               '|\n@@ -235,7 +234,7 @@ test_expect_success 'nested alignment with quote formatting' \"\n \"\n \n test_expect_success 'check `%(contents:lines=1)`' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \tmaster |three\n \tside |four\n \todd/spot |three\n@@ -253,7 +252,7 @@ test_expect_success 'check `%(contents:lines=1)`' '\n '\n \n test_expect_success 'check `%(contents:lines=0)`' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \tmaster |\n \tside |\n \todd/spot |\n@@ -271,7 +270,7 @@ test_expect_success 'check `%(contents:lines=0)`' '\n '\n \n test_expect_success 'check `%(contents:lines=99999)`' '\n-\ttest_prepare_expect >expect <<-\\EOF &&\n+\tcat >expect <<-\\EOF &&\n \tmaster |three\n \tside |four\n \todd/spot |three\n-- \n2.8.2.643.g361a07a\n"},{"id":"286008","messageId":"CAPig+cTa-7-cneOaFgtUnXPFbMZZA3b5m-3kJK2=qB5ASRmCqg@mail.gmail.com","threadId":"42257","inReplyTo":"20160510024032.GA16965@sigill.intra.peff.net","subject":"Re: [PATCH v2] t6302: simplify non-gpg cases","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-05-10T02:49:06Z","receivedAt":"2016-05-10T02:49:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, May 9, 2016 at 10:40 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, May 09, 2016 at 01:47:35PM -0400, Eric Sunshine wrote:\n>> > -       test_prepare_expect <<-\\EOF | sed -e \"s/Z$//\" >expect &&\n>> > +       cat <<-\\EOF | sed -e \"s/Z$//\" >expect &&\n>>\n>> To make this as close to a reversion as possible, this could be\n>> restored to the original (sans 'cat'):\n>>\n>>     sed -e \"s/Z$//\" >expect <<-\\EOF &&\n>\n> Thanks, I did the reversion with s/test_prepare_expect/cat/ rather than\n> reverting 618310a, but I agree dropping this useless-use-of-cat is worth\n> doing. Here's v2 with that change and your reviewed-by.\n\nLooks good, thanks.\n"},{"id":"286019","messageId":"xmqqk2j2sb4t.fsf@gitster.mtv.corp.google.com","threadId":"42257","inReplyTo":"20160510024032.GA16965@sigill.intra.peff.net","subject":"Re: [PATCH v2] t6302: simplify non-gpg cases","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T06:03:14Z","receivedAt":"2016-05-10T06:03:14Z","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> [+cc Junio as this should be the final version]\n\nThanks, I think I queued with \"do not cat a single file to a pipe\"\ntweak already.\n\n>> > When commit 618310a taught t6302 to run without the GPG\n>> \n>> 618310a (t6302: skip only signed tags rather than all tests when GPG\n>> is missing, 2016-03-06)\n>\n> I sometimes intentionally avoid using that longer form when the title of\n> the commit does not convey what I want to communicate, and I have to\n> summarize the change in my own words anyway (in this case the\n> interesting thing is not _what_ it did, but _how_ it chose to do it). So\n> I find including the original subject line just bloats the sentence and\n> makes the point harder to find.\n>\n> But I'm curious whether other people run into that problem, or if\n> readers would prefer an unconditional full-citation.\n\nPersonally, I find your version better in this case, simply because,\nas you said, the focus is different, and because the readers\nfamiliar with the recent history can still tell from your\ndescription which commit you are talking about without resorting to\n\"git show 618310a\".  The only thing we are losing is the datestamp,\nwhich is more relevant when referring to a commit in more distant\npast.  But in general, not everybody writes a good log message like\nyou do, so if they try to imitate what you did above, the end result\nis likely to end up being a cryptic mess that does not help identify\nwhich commit they are talking about. For that reason, I am a bit\nhesitant to say everybody should omit the original when they (think\nthey) do their own rephrasing.\n\n> Thanks, I did the reversion with s/test_prepare_expect/cat/ rather than\n> reverting 618310a, but I agree dropping this useless-use-of-cat is worth\n> doing. Here's v2 with that change and your reviewed-by.\n"}]}