Re: [PATCH] t7510: Skip all if GPG isn't installed

Brian Gernhardt [off-list ref] writes:

Since the setup requires the GPG prerequisite, it doesn't make much
sense to try and run any tests without it.  So rather than using a
prereq on each individual test and possibly forgetting it on new ones
(as just happened), skip the entire file if GPG isn't found.

Signed-off-by: Brian Gernhardt <redacted>
---

I think by "just happend" you mean aa4b78d4 (pretty: avoid reading
past end-of-string with "%G", 2014-06-16), which adds one that is
not protected (Cc'ed peff).

As there are a few additional test pieces to this file in flight
that come from another topic (which by the way protects them with
the prerequiste), I'd rather fix it up with the necessary GPG
prerequisite, at least for now, instead of doing it this way.

After the dust settles, we should definitely consider taking the
approach of this patch to simplify everything, but not now.

Another thing we may want to take into account is that we would also
want to make sure that builds of Git without GPG installed still
behave sensibly (with some definition of sensible) when faced with
GPG signatures in existing commit objects and tag objects.  I do not
think we currently test that combination at all, but we may want to
introduce a new directory t/t7510/ to hold store pre-existing commit
objects in the loose form (or in the textual form, suitable for
fast-import) and use them to populate the test repository in the
set-up step.  And new test pieces that do not require GPG (or those
that do require that GPG is *not* installed) would make sure that
various commands like "show --show-signature", "verify-commit" would
say "I cannot verify them" but still do what they are asked to do in
a sensible way (e.g. "show --show-signature" may not be able to show
the signature obviously but still will give you the header, the log
message and the patch; "verify-commit" should fail because it cannot
verify).  If that will happen in this same script, then skipping all
by requiring GPG upfront may not be a good change, but it is likely
that we would want a NOGPG prerequisite for "No GPG installed" case
and have a separate test script, in which case, this will just skip
all without GPG, and the other new one will just skip all without
NOGPG.  We'll see.

On Wed, Jun 25, 2014 at 02:16:55PM -0700, Junio C Hamano wrote:

Brian Gernhardt [off-list ref] writes:

quoted

Since the setup requires the GPG prerequisite, it doesn't make much
sense to try and run any tests without it.  So rather than using a
prereq on each individual test and possibly forgetting it on new ones
(as just happened), skip the entire file if GPG isn't found.

Signed-off-by: Brian Gernhardt <redacted>
---

I think by "just happend" you mean aa4b78d4 (pretty: avoid reading
past end-of-string with "%G", 2014-06-16), which adds one that is
not protected (Cc'ed peff).

If that is the one Brian means (and that is the only one I see in pu),
leaving it off was intentional. You do not need to have the GPG
prerequisite to verify the handling of %G and %GX, as the point is that
they are not actually gpg format placeholders.

That being said, I did botch the commit, because not having GPG means
we would not create any commits, and therefore "git log -1" fails for
the wrong reason.

The "right" fix is actually to make sure there is at least one commit in
that final test (e.g., by adding one base commit before the others).
Then it can run regardless of whether the GPG tests ran. And in that
sense, Brian's patch is working in the opposite direction.

Thinking on the explanation I gave above, though, I think it means the
test would probably be better placed in t6006 along with the other
format-specifier tests. That fixes the problem, and means that Brian's
simplification (to just skip all tests) becomes the right thing to do.

As there are a few additional test pieces to this file in flight
that come from another topic (which by the way protects them with
the prerequiste), I'd rather fix it up with the necessary GPG
prerequisite, at least for now, instead of doing it this way.

The patch below should fix it with minimal fuss. I think Brian's patch
makes sense on top, but I agree it would be nice to wait until the
existing topics settle.

Another thing we may want to take into account is that we would also
want to make sure that builds of Git without GPG installed still
behave sensibly (with some definition of sensible) when faced with
GPG signatures in existing commit objects and tag objects.

Yeah, I agree that is a good thing to test.

If that will happen in this same script, then skipping all by
requiring GPG upfront may not be a good change, but it is likely that
we would want a NOGPG prerequisite for "No GPG installed" case and
have a separate test script, in which case, this will just skip all
without GPG, and the other new one will just skip all without NOGPG.
We'll see.

I think it may make more sense to just configure gpg.program to "false"
for the NOGPG case. Then you get coverage both on systems with it
installed, and without (you could also just test it on GPG systems, and
drop the "ship commits in fast-import form" part of the plan).

Anyway, that is all outside the scope of the immediate problem. Here's
the patch to fix jk/pretty-G-format-fixes.

-- >8 --
Subject: move "%G" format test from t7510 to t6006

The final test in t7510 checks that "--format" placeholders
that look similar to GPG placeholders (but that we don't
actually understand) are passed through. That test was
placed in t7510, since the other GPG placeholder tests are
there. However, it does not have a GPG prerequisite, because
it is not actually checking any signed commits.

This causes the test to erroneously fail when gpg is not
installed on a system, however. Not because we need signed
commits, but because we need _any_ commit to run "git log".
If we don't have gpg installed, t7510 doesn't create any
commits at all.

We can fix this by moving the test into t6006. This is
arguably a better place anyway, because it is where we test
most of the other placeholders (we do not test GPG
placeholders there because of the infrastructure needed to
make signed commits).

Signed-off-by: Jeff King <redacted>
---
 t/t6006-rev-list-format.sh | 6 ++++++
 t/t7510-signed-commit.sh   | 6 ------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index c277db6..88ed319 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh

@@ -468,4 +468,10 @@ test_expect_success 'single-character name is parsed correctly' '
 	test_cmp expect actual
 '
 
+test_expect_success 'unused %G placeholders are passed through' '
+	echo "%GX %G" >expect &&
+	git log -1 --format="%GX %G" >actual &&
+	test_cmp expect actual
+'
+
 test_done

diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index 9810242..e97477a 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh

@@ -147,10 +147,4 @@ test_expect_success GPG 'show lack of signature with custom format' '
 	test_cmp expect actual
 '
 
-test_expect_success 'unused %G placeholders are passed through' '
-	echo "%GX %G" >expect &&
-	git log -1 --format="%GX %G" >actual &&
-	test_cmp expect actual
-'
-
 test_done

-- 
2.0.0.566.gfe3e6b2