{"thread":{"id":"41298","subject":"[PATCH] t6302: drop unnecessary GPG requirement","startedAt":"2016-01-31T20:19:22Z","lastAt":"2016-02-02T07:06:18Z","messageCount":5,"participants":["Eric Sunshine","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"277140","messageId":"1454271562-24291-1-git-send-email-sunshine@sunshineco.com","threadId":"41298","inReplyTo":null,"subject":"[PATCH] t6302: drop unnecessary GPG requirement","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-31T20:19:22Z","receivedAt":"2016-01-31T20:19:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"These tests are concerned specifically with filtering, sorting,\nformatting behavior of git-for-each-ref, yet if GPG is not present, the\nentire test script is skipped even though none of the tests depend upon\nor care whether the tags are signed. This unnecessary dependency upon\nGPG may prevent these tests from being more widely run, so drop it.\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\n\nI noticed this while reviewing[1] v3 of Karthik's \"optimize\nref-filter.c:populate_value()\" series when I tried to run tests the\nseries added but couldn't due to missing GPG.\n\n[1]: http://article.gmane.org/gmane.comp.version-control.git/284766\n\nt/t6302-for-each-ref-filter.sh | 11 ++---------\n 1 file changed, 2 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex fe4796c..dea2a9e 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -3,13 +3,6 @@\n test_description='test for-each-refs usage of ref-filter APIs'\n \n . ./test-lib.sh\n-. \"$TEST_DIRECTORY\"/lib-gpg.sh\n-\n-if ! test_have_prereq GPG\n-then\n-\tskip_all=\"skipping for-each-ref tests, GPG not available\"\n-\ttest_done\n-fi\n \n test_expect_success 'setup some history and refs' '\n \ttest_commit one &&\n@@ -17,8 +10,8 @@ test_expect_success 'setup some history and refs' '\n \ttest_commit three &&\n \tgit checkout -b side &&\n \ttest_commit four &&\n-\tgit tag -s -m \"A signed tag message\" signed-tag &&\n-\tgit tag -s -m \"Annonated doubly\" double-tag signed-tag &&\n+\tgit tag -m \"A signed tag message\" signed-tag &&\n+\tgit tag -m \"Annonated doubly\" double-tag signed-tag &&\n \tgit checkout master &&\n \tgit update-ref refs/odd/spot master\n '\n-- \n2.7.0.333.g9c3d022\n"},{"id":"277146","messageId":"xmqq4mdtxjxf.fsf@gitster.mtv.corp.google.com","threadId":"41298","inReplyTo":"1454271562-24291-1-git-send-email-sunshine@sunshineco.com","subject":"Re: [PATCH] t6302: drop unnecessary GPG requirement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-01T02:07:24Z","receivedAt":"2016-02-01T02:07:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine <sunshine@sunshineco.com> writes:\n\n> These tests are concerned specifically with filtering, sorting,\n> formatting behavior of git-for-each-ref, yet if GPG is not present, the\n> entire test script is skipped even though none of the tests depend upon\n> or care whether the tags are signed. This unnecessary dependency upon\n> GPG may prevent these tests from being more widely run, so drop it.\n\nIt is conceivable, if not highly plausible, that a change to the\ncode that does the filtering and formatting can become buggy because\npayload with GPG signature looks somewhat differently from what is\nin an annotated but not signed tag.  Even if \"these are specifically\nfiltering...\"  is true, including tests for signed tags is valuable.\n\nIt seems that we are not currently testing with unsigned but\nannotated tags, and tests that use them would be good, too.\n\nWould it make sense to introduce a helper function specific to this\nscript to be used to prepare the expected output, to replace cat <<,\nthat goes like this?\n\n    test_prepare_expect () {\n\tif test_have_prereq GPG\n        then\n\t\tcat\n\telse\n        \tsed -e '/signed/d'\n\tfi\n    }\n\nAnd then use it like this?\n\n    test_expect_success 'check \"%(contents:lines=99999)\"' '\n        test_prepare_expect <<-\\EOF &&\n\tmaster |three\n        ...\n        signed-tag |A signed tag message\n        ...\n        EOF\n\tgit for-each-ref --format=... >actual &&\n        test_cmp expect actual\n    '\n\nThe setup part would need to make GPG conditional, e.g.\n\ntest_expect_success 'setup some history and refs' '\n\ttest_commit one &&\n\ttest_commit two &&\n\ttest_commit three &&\n\tgit checkout -b side &&\n\ttest_commit four &&\n\tgit tag -m \"An annotated tag\" annotated-tag &&\n        git tag -m \"Annotated doubly\" doubly-annotated-tag annotated-tag &&\n\tif test_have_prereq GPG\n\tthen\n                git tag -s -m \"A signed tag message\" signed-tag &&\n                git tag -s -m \"Annonated doubly\" doubly-signed-tag signed-tag\n\tfi &&\n\tgit checkout master &&\n\tgit update-ref refs/odd/spot master\n'\n\nPerhaps?\n"},{"id":"277160","messageId":"alpine.DEB.2.20.1602010759480.2964@virtualbox","threadId":"41298","inReplyTo":"xmqq4mdtxjxf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t6302: drop unnecessary GPG requirement","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-01T07:24:34Z","receivedAt":"2016-02-01T07:24:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric & Junio,\n\nOn Sun, 31 Jan 2016, Junio C Hamano wrote:\n\n> Eric Sunshine <sunshine@sunshineco.com> writes:\n> \n> > These tests are concerned specifically with filtering, sorting,\n> > formatting behavior of git-for-each-ref, yet if GPG is not present,\n> > the entire test script is skipped even though none of the tests depend\n> > upon or care whether the tags are signed. This unnecessary dependency\n> > upon GPG may prevent these tests from being more widely run, so drop\n> > it.\n> \n> [...]\n>\n> Would it make sense to introduce a helper function specific to this\n> script to be used to prepare the expected output, to replace cat <<,\n> that goes like this?\n> \n> [...]\n\nAn even easier solution might be to *not* set up the signed tags in the\n'setup' part, but only in the respective test case, and delete them right\naway after said test case?\n\nSomething like this (I even tested this with and without the GPG prereq):\n\n-- snipsnap --\nFrom: Johannes Schindelin <johannes.schindelin@gmx.de>\nSubject: [PATCH] Do not make t6302 depend on gpg wholesale\n\nThere is but a single test case, in fact, that depends on gpg. Let's just\nmake the other test cases independent of gpg and add the GPG prereq to\nsaid single test case.\n\nNoticed by Eric Sunshine.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n t/t6302-for-each-ref-filter.sh | 33 ++++-----------------------------\n 1 file changed, 4 insertions(+), 29 deletions(-)\n\ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex fe4796c..e3a5636 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -5,20 +5,12 @@ test_description='test for-each-refs usage of ref-filter APIs'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n \n-if ! test_have_prereq GPG\n-then\n-\tskip_all=\"skipping for-each-ref tests, GPG not available\"\n-\ttest_done\n-fi\n-\n test_expect_success 'setup some history and refs' '\n \ttest_commit one &&\n \ttest_commit two &&\n \ttest_commit three &&\n \tgit checkout -b side &&\n \ttest_commit four &&\n-\tgit tag -s -m \"A signed tag message\" signed-tag &&\n-\tgit tag -s -m \"Annonated doubly\" double-tag signed-tag &&\n \tgit checkout master &&\n \tgit update-ref refs/odd/spot master\n '\n@@ -33,7 +25,10 @@ test_expect_success 'filtering with --points-at' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'check signed tags with --points-at' '\n+test_expect_success GPG 'check signed tags with --points-at' '\n+\tgit tag -s -m \"A signed tag message\" signed-tag side &&\n+\tgit tag -s -m \"Annonated doubly\" double-tag signed-tag &&\n+\ttest_when_finished git tag -d signed-tag &&\n \tsed -e \"s/Z$//\" >expect <<-\\EOF &&\n \trefs/heads/side Z\n \trefs/tags/four Z\n@@ -58,9 +53,7 @@ test_expect_success 'filtering with --merged' '\n test_expect_success 'filtering with --no-merged' '\n \tcat >expect <<-\\EOF &&\n \trefs/heads/side\n-\trefs/tags/double-tag\n \trefs/tags/four\n-\trefs/tags/signed-tag\n \tEOF\n \tgit for-each-ref --format=\"%(refname)\" --no-merged=master >actual &&\n \ttest_cmp expect actual\n@@ -71,9 +64,7 @@ test_expect_success 'filtering with --contains' '\n \trefs/heads/master\n \trefs/heads/side\n \trefs/odd/spot\n-\trefs/tags/double-tag\n \trefs/tags/four\n-\trefs/tags/signed-tag\n \trefs/tags/three\n \trefs/tags/two\n \tEOF\n@@ -90,10 +81,8 @@ test_expect_success 'left alignment is default' '\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-\trefname is refs/tags/double-tag|refs/tags/double-tag\n \trefname is refs/tags/four     |refs/tags/four\n \trefname is refs/tags/one      |refs/tags/one\n-\trefname is refs/tags/signed-tag|refs/tags/signed-tag\n \trefname is refs/tags/three    |refs/tags/three\n \trefname is refs/tags/two      |refs/tags/two\n \tEOF\n@@ -106,10 +95,8 @@ test_expect_success 'middle alignment' '\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-\t|refname is refs/tags/double-tag|refs/tags/double-tag\n \t|  refname is refs/tags/four   |refs/tags/four\n \t|   refname is refs/tags/one   |refs/tags/one\n-\t|refname is refs/tags/signed-tag|refs/tags/signed-tag\n \t|  refname is refs/tags/three  |refs/tags/three\n \t|   refname is refs/tags/two   |refs/tags/two\n \tEOF\n@@ -122,10 +109,8 @@ test_expect_success 'right alignment' '\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-\t|refname is refs/tags/double-tag|refs/tags/double-tag\n \t|     refname is refs/tags/four|refs/tags/four\n \t|      refname is refs/tags/one|refs/tags/one\n-\t|refname is refs/tags/signed-tag|refs/tags/signed-tag\n \t|    refname is refs/tags/three|refs/tags/three\n \t|      refname is refs/tags/two|refs/tags/two\n \tEOF\n@@ -140,10 +125,8 @@ test_expect_success 'alignment with format quote' \"\n \t|'      '\\''master| A U Thor'\\''      '|\n \t|'       '\\''side| A U Thor'\\''       '|\n \t|'     '\\''odd/spot| A U Thor'\\''     '|\n-\t|'        '\\''double-tag| '\\''        '|\n \t|'       '\\''four| A U Thor'\\''       '|\n \t|'       '\\''one| A U Thor'\\''        '|\n-\t|'        '\\''signed-tag| '\\''        '|\n \t|'      '\\''three| A U Thor'\\''       '|\n \t|'       '\\''two| A U Thor'\\''        '|\n \tEOF\n@@ -156,10 +139,8 @@ test_expect_success 'nested alignment with quote formatting' \"\n \t|'         master               '|\n \t|'           side               '|\n \t|'       odd/spot               '|\n-\t|'     double-tag               '|\n \t|'           four               '|\n \t|'            one               '|\n-\t|'     signed-tag               '|\n \t|'          three               '|\n \t|'            two               '|\n \tEOF\n@@ -172,10 +153,8 @@ test_expect_success 'check `%(contents:lines=1)`' '\n \tmaster |three\n \tside |four\n \todd/spot |three\n-\tdouble-tag |Annonated doubly\n \tfour |four\n \tone |one\n-\tsigned-tag |A signed tag message\n \tthree |three\n \ttwo |two\n \tEOF\n@@ -188,10 +167,8 @@ test_expect_success 'check `%(contents:lines=0)`' '\n \tmaster |\n \tside |\n \todd/spot |\n-\tdouble-tag |\n \tfour |\n \tone |\n-\tsigned-tag |\n \tthree |\n \ttwo |\n \tEOF\n@@ -204,10 +181,8 @@ test_expect_success 'check `%(contents:lines=99999)`' '\n \tmaster |three\n \tside |four\n \todd/spot |three\n-\tdouble-tag |Annonated doubly\n \tfour |four\n \tone |one\n-\tsigned-tag |A signed tag message\n \tthree |three\n \ttwo |two\n \tEOF\n-- \n2.7.0.windows.1.7.g55a05c8\n"},{"id":"277178","messageId":"xmqq4mdsw949.fsf@gitster.mtv.corp.google.com","threadId":"41298","inReplyTo":"alpine.DEB.2.20.1602010759480.2964@virtualbox","subject":"Re: [PATCH] t6302: drop unnecessary GPG requirement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-01T18:58:30Z","receivedAt":"2016-02-01T18:58:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> An even easier solution might be to *not* set up the signed tags in the\n> 'setup' part, but only in the respective test case, and delete them right\n> away after said test case?\n\nAfter reading your patch, I do not find it an \"easier solution\", at\nleast with the definition of the word \"solution\" I would use.  It\nstops testing signed or doubly signed tags everywhere, assuming that\nfuture regressions can ever break only --points-at tests and no\nother tests around signed tags.  The easiest solution along that\nsame line of thought taken to the extreme would be to say test_done\nwithout having any test ;-)\n\nThe \"filter entries about signed tags from the expected output\"\napproach I suggested would not work if the expected output files are\nnot line oriented and lines that would appear only when signed tags\nare tested cannot be easily filtered out (like with the sed script\nin the message you are responding to), but I think for the existing\ntest cases (with or without additions of annotated but unsigned\ntags) it would work.\n\n> Something like this (I even tested this with and without the GPG prereq):\n> ...\n> -test_expect_success 'check signed tags with --points-at' '\n> +test_expect_success GPG 'check signed tags with --points-at' '\n> +\tgit tag -s -m \"A signed tag message\" signed-tag side &&\n> +\tgit tag -s -m \"Annonated doubly\" double-tag signed-tag &&\n> +\ttest_when_finished git tag -d signed-tag &&\n\nInterestingly, double-tag is not removed here.\n"},{"id":"277210","messageId":"alpine.DEB.2.20.1602020805280.2964@virtualbox","threadId":"41298","inReplyTo":"xmqq4mdsw949.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] t6302: drop unnecessary GPG requirement","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-02-02T07:06:18Z","receivedAt":"2016-02-02T07:06:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 1 Feb 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > An even easier solution might be to *not* set up the signed tags in the\n> > 'setup' part, but only in the respective test case, and delete them right\n> > away after said test case?\n> \n> After reading your patch, I do not find it an \"easier solution\", at\n> least with the definition of the word \"solution\" I would use.  It\n> stops testing signed or doubly signed tags everywhere, assuming that\n> future regressions can ever break only --points-at tests and no\n> other tests around signed tags.\n\nTrue.\n\n> > Something like this (I even tested this with and without the GPG prereq):\n> > ...\n> > -test_expect_success 'check signed tags with --points-at' '\n> > +test_expect_success GPG 'check signed tags with --points-at' '\n> > +\tgit tag -s -m \"A signed tag message\" signed-tag side &&\n> > +\tgit tag -s -m \"Annonated doubly\" double-tag signed-tag &&\n> > +\ttest_when_finished git tag -d signed-tag &&\n> \n> Interestingly, double-tag is not removed here.\n\nWhooopsie.\n\nI retract the patch in any case ;-)\n\nCiao,\nDscho\n"}]}