{"thread":{"id":"36987","subject":"[PATCH] t7510: Skip all if GPG isn't installed","startedAt":"2014-06-24T04:52:16Z","lastAt":"2014-06-25T22:24:00Z","messageCount":4,"participants":["Brian Gernhardt","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"244909","messageId":"1403585536-32185-1-git-send-email-brian@gernhardtsoftware.com","threadId":"36987","inReplyTo":null,"subject":"[PATCH] t7510: Skip all if GPG isn't installed","fromName":"Brian Gernhardt","fromEmail":"brian@gernhardtsoftware.com","sentAt":"2014-06-24T04:52:16Z","receivedAt":"2014-06-24T04:52:16Z","isPatch":true,"sender":{"key":"brian@gernhardtsoftware.com","avatar":"https://avatars.githubusercontent.com/u/133455?v=4"},"body":"Since the setup requires the GPG prerequisite, it doesn't make much\nsense to try and run any tests without it.  So rather than using a\nprereq on each individual test and possibly forgetting it on new ones\n(as just happened), skip the entire file if GPG isn't found.\n\nSigned-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>\n---\n t/t7510-signed-commit.sh | 24 +++++++++++++++---------\n 1 file changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex 9810242..414f9d1 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -4,7 +4,13 @@ test_description='signed commit tests'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY/lib-gpg.sh\"\n \n-test_expect_success GPG 'create signed commits' '\n+if ! test_have_prereq GPG\n+then\n+\tskip_all='skipping signed commit tests; gpg not available'\n+\ttest_done\n+fi\n+\n+test_expect_success 'create signed commits' '\n \ttest_when_finished \"test_unconfig commit.gpgsign\" &&\n \n \techo 1 >file && git add file &&\n@@ -48,7 +54,7 @@ test_expect_success GPG 'create signed commits' '\n \tgit tag eighth-signed-alt\n '\n \n-test_expect_success GPG 'show signatures' '\n+test_expect_success 'show signatures' '\n \t(\n \t\tfor commit in initial second merge fourth-signed fifth-signed sixth-signed seventh-signed\n \t\tdo\n@@ -79,7 +85,7 @@ test_expect_success GPG 'show signatures' '\n \t)\n '\n \n-test_expect_success GPG 'detect fudged signature' '\n+test_expect_success 'detect fudged signature' '\n \tgit cat-file commit seventh-signed >raw &&\n \n \tsed -e \"s/seventh/7th forged/\" raw >forged1 &&\n@@ -89,7 +95,7 @@ test_expect_success GPG 'detect fudged signature' '\n \t! grep \"Good signature from\" actual1\n '\n \n-test_expect_success GPG 'detect fudged signature with NUL' '\n+test_expect_success 'detect fudged signature with NUL' '\n \tgit cat-file commit seventh-signed >raw &&\n \tcat raw >forged2 &&\n \techo Qwik | tr \"Q\" \"\\000\" >>forged2 &&\n@@ -99,7 +105,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '\n \t! grep \"Good signature from\" actual2\n '\n \n-test_expect_success GPG 'amending already signed commit' '\n+test_expect_success 'amending already signed commit' '\n \tgit checkout fourth-signed^0 &&\n \tgit commit --amend -S --no-edit &&\n \tgit show -s --show-signature HEAD >actual &&\n@@ -107,7 +113,7 @@ test_expect_success GPG 'amending already signed commit' '\n \t! grep \"BAD signature from\" actual\n '\n \n-test_expect_success GPG 'show good signature with custom format' '\n+test_expect_success 'show good signature with custom format' '\n \tcat >expect <<-\\EOF &&\n \tG\n \t13B6F51ECDDE430D\n@@ -117,7 +123,7 @@ test_expect_success GPG 'show good signature with custom format' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success GPG 'show bad signature with custom format' '\n+test_expect_success 'show bad signature with custom format' '\n \tcat >expect <<-\\EOF &&\n \tB\n \t13B6F51ECDDE430D\n@@ -127,7 +133,7 @@ test_expect_success GPG 'show bad signature with custom format' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success GPG 'show unknown signature with custom format' '\n+test_expect_success 'show unknown signature with custom format' '\n \tcat >expect <<-\\EOF &&\n \tU\n \t61092E85B7227189\n@@ -137,7 +143,7 @@ test_expect_success GPG 'show unknown signature with custom format' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success GPG 'show lack of signature with custom format' '\n+test_expect_success 'show lack of signature with custom format' '\n \tcat >expect <<-\\EOF &&\n \tN\n \n-- \n2.0.0.495.gf681aa8\n"},{"id":"244982","messageId":"xmqqfvis8zaw.fsf@gitster.dls.corp.google.com","threadId":"36987","inReplyTo":"1403585536-32185-1-git-send-email-brian@gernhardtsoftware.com","subject":"Re: [PATCH] t7510: Skip all if GPG isn't installed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-25T21:16:55Z","receivedAt":"2014-06-25T21:16:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brian Gernhardt <brian@gernhardtsoftware.com> writes:\n\n> Since the setup requires the GPG prerequisite, it doesn't make much\n> sense to try and run any tests without it.  So rather than using a\n> prereq on each individual test and possibly forgetting it on new ones\n> (as just happened), skip the entire file if GPG isn't found.\n>\n> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>\n> ---\n\nI think by \"just happend\" you mean aa4b78d4 (pretty: avoid reading\npast end-of-string with \"%G\", 2014-06-16), which adds one that is\nnot protected (Cc'ed peff).\n\nAs there are a few additional test pieces to this file in flight\nthat come from another topic (which by the way protects them with\nthe prerequiste), I'd rather fix it up with the necessary GPG\nprerequisite, at least for now, instead of doing it this way.\n\nAfter the dust settles, we should definitely consider taking the\napproach of this patch to simplify everything, but not now.\n\nAnother thing we may want to take into account is that we would also\nwant to make sure that builds of Git without GPG installed still\nbehave sensibly (with some definition of sensible) when faced with\nGPG signatures in existing commit objects and tag objects.  I do not\nthink we currently test that combination at all, but we may want to\nintroduce a new directory t/t7510/ to hold store pre-existing commit\nobjects in the loose form (or in the textual form, suitable for\nfast-import) and use them to populate the test repository in the\nset-up step.  And new test pieces that do not require GPG (or those\nthat do require that GPG is *not* installed) would make sure that\nvarious commands like \"show --show-signature\", \"verify-commit\" would\nsay \"I cannot verify them\" but still do what they are asked to do in\na sensible way (e.g. \"show --show-signature\" may not be able to show\nthe signature obviously but still will give you the header, the log\nmessage and the patch; \"verify-commit\" should fail because it cannot\nverify).  If that will happen in this same script, then skipping all\nby requiring GPG upfront may not be a good change, but it is likely\nthat we would want a NOGPG prerequisite for \"No GPG installed\" case\nand have a separate test script, in which case, this will just skip\nall without GPG, and the other new one will just skip all without\nNOGPG.  We'll see.\n\n\n>  t/t7510-signed-commit.sh | 24 +++++++++++++++---------\n>  1 file changed, 15 insertions(+), 9 deletions(-)\n>\n> diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\n> index 9810242..414f9d1 100755\n> --- a/t/t7510-signed-commit.sh\n> +++ b/t/t7510-signed-commit.sh\n> @@ -4,7 +4,13 @@ test_description='signed commit tests'\n>  . ./test-lib.sh\n>  . \"$TEST_DIRECTORY/lib-gpg.sh\"\n>  \n> -test_expect_success GPG 'create signed commits' '\n> +if ! test_have_prereq GPG\n> +then\n> +\tskip_all='skipping signed commit tests; gpg not available'\n> +\ttest_done\n> +fi\n> +\n> +test_expect_success 'create signed commits' '\n>  \ttest_when_finished \"test_unconfig commit.gpgsign\" &&\n>  \n>  \techo 1 >file && git add file &&\n> @@ -48,7 +54,7 @@ test_expect_success GPG 'create signed commits' '\n>  \tgit tag eighth-signed-alt\n>  '\n>  \n> -test_expect_success GPG 'show signatures' '\n> +test_expect_success 'show signatures' '\n>  \t(\n>  \t\tfor commit in initial second merge fourth-signed fifth-signed sixth-signed seventh-signed\n>  \t\tdo\n> @@ -79,7 +85,7 @@ test_expect_success GPG 'show signatures' '\n>  \t)\n>  '\n>  \n> -test_expect_success GPG 'detect fudged signature' '\n> +test_expect_success 'detect fudged signature' '\n>  \tgit cat-file commit seventh-signed >raw &&\n>  \n>  \tsed -e \"s/seventh/7th forged/\" raw >forged1 &&\n> @@ -89,7 +95,7 @@ test_expect_success GPG 'detect fudged signature' '\n>  \t! grep \"Good signature from\" actual1\n>  '\n>  \n> -test_expect_success GPG 'detect fudged signature with NUL' '\n> +test_expect_success 'detect fudged signature with NUL' '\n>  \tgit cat-file commit seventh-signed >raw &&\n>  \tcat raw >forged2 &&\n>  \techo Qwik | tr \"Q\" \"\\000\" >>forged2 &&\n> @@ -99,7 +105,7 @@ test_expect_success GPG 'detect fudged signature with NUL' '\n>  \t! grep \"Good signature from\" actual2\n>  '\n>  \n> -test_expect_success GPG 'amending already signed commit' '\n> +test_expect_success 'amending already signed commit' '\n>  \tgit checkout fourth-signed^0 &&\n>  \tgit commit --amend -S --no-edit &&\n>  \tgit show -s --show-signature HEAD >actual &&\n> @@ -107,7 +113,7 @@ test_expect_success GPG 'amending already signed commit' '\n>  \t! grep \"BAD signature from\" actual\n>  '\n>  \n> -test_expect_success GPG 'show good signature with custom format' '\n> +test_expect_success 'show good signature with custom format' '\n>  \tcat >expect <<-\\EOF &&\n>  \tG\n>  \t13B6F51ECDDE430D\n> @@ -117,7 +123,7 @@ test_expect_success GPG 'show good signature with custom format' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> -test_expect_success GPG 'show bad signature with custom format' '\n> +test_expect_success 'show bad signature with custom format' '\n>  \tcat >expect <<-\\EOF &&\n>  \tB\n>  \t13B6F51ECDDE430D\n> @@ -127,7 +133,7 @@ test_expect_success GPG 'show bad signature with custom format' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> -test_expect_success GPG 'show unknown signature with custom format' '\n> +test_expect_success 'show unknown signature with custom format' '\n>  \tcat >expect <<-\\EOF &&\n>  \tU\n>  \t61092E85B7227189\n> @@ -137,7 +143,7 @@ test_expect_success GPG 'show unknown signature with custom format' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> -test_expect_success GPG 'show lack of signature with custom format' '\n> +test_expect_success 'show lack of signature with custom format' '\n>  \tcat >expect <<-\\EOF &&\n>  \tN\n"},{"id":"244988","messageId":"20140625214217.GA13564@sigill.intra.peff.net","threadId":"36987","inReplyTo":"xmqqfvis8zaw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] t7510: Skip all if GPG isn't installed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-25T21:42:17Z","receivedAt":"2014-06-25T21:42:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 25, 2014 at 02:16:55PM -0700, Junio C Hamano wrote:\n\n> Brian Gernhardt <brian@gernhardtsoftware.com> writes:\n> \n> > Since the setup requires the GPG prerequisite, it doesn't make much\n> > sense to try and run any tests without it.  So rather than using a\n> > prereq on each individual test and possibly forgetting it on new ones\n> > (as just happened), skip the entire file if GPG isn't found.\n> >\n> > Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>\n> > ---\n> \n> I think by \"just happend\" you mean aa4b78d4 (pretty: avoid reading\n> past end-of-string with \"%G\", 2014-06-16), which adds one that is\n> not protected (Cc'ed peff).\n\nIf that is the one Brian means (and that is the only one I see in pu),\nleaving it off was intentional. You do not need to have the GPG\nprerequisite to verify the handling of %G and %GX, as the point is that\nthey are not actually gpg format placeholders.\n\nThat being said, I did botch the commit, because not having GPG means\nwe would not create any commits, and therefore \"git log -1\" fails for\nthe wrong reason.\n\nThe \"right\" fix is actually to make sure there is at least one commit in\nthat final test (e.g., by adding one base commit before the others).\nThen it can run regardless of whether the GPG tests ran. And in that\nsense, Brian's patch is working in the opposite direction.\n\nThinking on the explanation I gave above, though, I think it means the\ntest would probably be better placed in t6006 along with the other\nformat-specifier tests. That fixes the problem, and means that Brian's\nsimplification (to just skip all tests) becomes the right thing to do.\n\n> As there are a few additional test pieces to this file in flight\n> that come from another topic (which by the way protects them with\n> the prerequiste), I'd rather fix it up with the necessary GPG\n> prerequisite, at least for now, instead of doing it this way.\n\nThe patch below should fix it with minimal fuss. I think Brian's patch\nmakes sense on top, but I agree it would be nice to wait until the\nexisting topics settle.\n\n> Another thing we may want to take into account is that we would also\n> want to make sure that builds of Git without GPG installed still\n> behave sensibly (with some definition of sensible) when faced with\n> GPG signatures in existing commit objects and tag objects.\n\nYeah, I agree that is a good thing to test.\n\n> If that will happen in this same script, then skipping all by\n> requiring GPG upfront may not be a good change, but it is likely that\n> we would want a NOGPG prerequisite for \"No GPG installed\" case and\n> have a separate test script, in which case, this will just skip all\n> without GPG, and the other new one will just skip all without NOGPG.\n> We'll see.\n\nI think it may make more sense to just configure gpg.program to \"false\"\nfor the NOGPG case. Then you get coverage both on systems with it\ninstalled, and without (you could also just test it on GPG systems, and\ndrop the \"ship commits in fast-import form\" part of the plan).\n\nAnyway, that is all outside the scope of the immediate problem. Here's\nthe patch to fix jk/pretty-G-format-fixes.\n\n-- >8 --\nSubject: move \"%G\" format test from t7510 to t6006\n\nThe final test in t7510 checks that \"--format\" placeholders\nthat look similar to GPG placeholders (but that we don't\nactually understand) are passed through. That test was\nplaced in t7510, since the other GPG placeholder tests are\nthere. However, it does not have a GPG prerequisite, because\nit is not actually checking any signed commits.\n\nThis causes the test to erroneously fail when gpg is not\ninstalled on a system, however. Not because we need signed\ncommits, but because we need _any_ commit to run \"git log\".\nIf we don't have gpg installed, t7510 doesn't create any\ncommits at all.\n\nWe can fix this by moving the test into t6006. This is\narguably a better place anyway, because it is where we test\nmost of the other placeholders (we do not test GPG\nplaceholders there because of the infrastructure needed to\nmake signed commits).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t6006-rev-list-format.sh | 6 ++++++\n t/t7510-signed-commit.sh   | 6 ------\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh\nindex c277db6..88ed319 100755\n--- a/t/t6006-rev-list-format.sh\n+++ b/t/t6006-rev-list-format.sh\n@@ -468,4 +468,10 @@ test_expect_success 'single-character name is parsed correctly' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'unused %G placeholders are passed through' '\n+\techo \"%GX %G\" >expect &&\n+\tgit log -1 --format=\"%GX %G\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh\nindex 9810242..e97477a 100755\n--- a/t/t7510-signed-commit.sh\n+++ b/t/t7510-signed-commit.sh\n@@ -147,10 +147,4 @@ test_expect_success GPG 'show lack of signature with custom format' '\n \ttest_cmp expect actual\n '\n \n-test_expect_success 'unused %G placeholders are passed through' '\n-\techo \"%GX %G\" >expect &&\n-\tgit log -1 --format=\"%GX %G\" >actual &&\n-\ttest_cmp expect actual\n-'\n-\n test_done\n-- \n2.0.0.566.gfe3e6b2\n"},{"id":"244990","messageId":"xmqqwqc46327.fsf@gitster.dls.corp.google.com","threadId":"36987","inReplyTo":"20140625214217.GA13564@sigill.intra.peff.net","subject":"Re: [PATCH] t7510: Skip all if GPG isn't installed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-25T22:24:00Z","receivedAt":"2014-06-25T22:24:00Z","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> ...\n> I think it may make more sense to just configure gpg.program to \"false\"\n> for the NOGPG case. Then you get coverage both on systems with it\n> installed, and without (you could also just test it on GPG systems, and\n> drop the \"ship commits in fast-import form\" part of the plan).\n>\n> Anyway, that is all outside the scope of the immediate problem. Here's\n> the patch to fix jk/pretty-G-format-fixes.\n\nAll sounds sensible.  Thanks.\n"}]}