[2/2] tests(gpg): increase verbosity to allow debugging

Commit Message

Johannes Schindelin via GitGitGadget March 23, 2020, 1:09 p.m. UTC

From: Johannes Schindelin <[e-mail]>

Especially when debugging a test failure that can only be reproduced in
the CI build (e.g. when the developer has no access to a macOS machine
other than running the tests on a macOS build agent), output should not
be suppressed.

In the instance of `hi/gpg-prefer-check-signature`, where one
GPG-related test failed for no apparent reason, the entire output of
`gpg` and `gpgsm` was suppressed, even in verbose mode, leaving
interested readers no clue what was going wrong.

Let's fix this by redirecting the output not to `/dev/null`, but to the
file descriptors that may, or may not, be redirected via
`--verbose-log`. For good measure, also turn on tracing if the user
asked for it, and prefix it with a helpful info message.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
 t/lib-gpg.sh | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

Comments

Jeff King March 23, 2020, 5:32 p.m. UTC | #1

On Mon, Mar 23, 2020 at 01:09:50PM +0000, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <[e-mail]>
> 
> Especially when debugging a test failure that can only be reproduced in
> the CI build (e.g. when the developer has no access to a macOS machine
> other than running the tests on a macOS build agent), output should not
> be suppressed.
> 
> In the instance of `hi/gpg-prefer-check-signature`, where one
> GPG-related test failed for no apparent reason, the entire output of
> `gpg` and `gpgsm` was suppressed, even in verbose mode, leaving
> interested readers no clue what was going wrong.
> 
> Let's fix this by redirecting the output not to `/dev/null`, but to the
> file descriptors that may, or may not, be redirected via
> `--verbose-log`. For good measure, also turn on tracing if the user
> asked for it, and prefix it with a helpful info message.

It definitely makes sense for this info to be shown in verbose (and
"-x") mode. I'm OK with this patch as easiest way to get from A to B,
but I think the existing code shows a bit of an anti-pattern: trying to
do too much outside of test blocks where verbosity and tracing is
already handled.

In this case if the whole thing were in a "test_lazy_prereq" we would
have gotten all that for free. I don't think the laziness would matter
too much in practice, though it is a little funny that it has side
effects (like setting up $GPGHOME). Having an immediate version like
"test_check_prereq" would make sense to me.

I don't know if it's worth re-doing this patch (I'll leave that decision
to you). But something to keep in mind when we run into similar
situations (or are writing new prereq code).

-Peff

Jeff King March 23, 2020, 6:04 p.m. UTC | #2

On Mon, Mar 23, 2020 at 01:32:58PM -0400, Jeff King wrote:

> In this case if the whole thing were in a "test_lazy_prereq" we would
> have gotten all that for free. I don't think the laziness would matter
> too much in practice, though it is a little funny that it has side
> effects (like setting up $GPGHOME). Having an immediate version like
> "test_check_prereq" would make sense to me.
> 
> I don't know if it's worth re-doing this patch (I'll leave that decision
> to you). But something to keep in mind when we run into similar
> situations (or are writing new prereq code).

Actually, it's pretty straight-forward and I think the resulting code is
cleaner. Note that we do have to use a real expect_success because of
the side effects. That does increment the test counter. IMHO that's not
a big deal, but if we're concerned it wouldn't be too hard to add a
non-lazy prereq block.

Patch is below (I pulled GPGSM into its own block, which involved
reindenting; view with "-w").

---
diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 11b83b8c24..6aa936e3ac 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -1,13 +1,16 @@
 #!/bin/sh
 
-gpg_version=$(gpg --version 2>&1)
-if test $? != 127
-then
+# This has to happen in a real test and not a lazy_prereq because it
+# has side effects (like setting up $GNUPGHOME).
+test_expect_success 'set up GPG prereq' '
+	test_might_fail gpg --version &&
+	test $? != 127 &&
+
 	# As said here: http://www.gnupg.org/documentation/faqs.html#q6.19
-	# the gpg version 1.0.6 didn't parse trust packets correctly, so for
+	# the gpg version 1.0.6 did not parse trust packets correctly, so for
 	# that version, creation of signed tags using the generated key fails.
 	case "$gpg_version" in
-	'gpg (GnuPG) 1.0.6'*)
+	"gpg (GnuPG) 1.0.6"*)
 		say "Your version of gpg (1.0.6) is too buggy for testing"
 		;;
 	*)
@@ -31,51 +34,51 @@ then
 		chmod 0700 ./gpghome &&
 		GNUPGHOME="$PWD/gpghome" &&
 		export GNUPGHOME &&
-		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
-		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
+		(gpgconf --kill gpg-agent || : ) &&
+		gpg --homedir "${GNUPGHOME}" --import \
 			"$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
-		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \
+		gpg --homedir "${GNUPGHOME}" --import-ownertrust \
 			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
-		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null 2>&1 \
-			--sign -u [e-mail] &&
-		test_set_prereq GPG &&
-		# Available key info:
-		# * see t/lib-gpg/gpgsm-gen-key.in
-		# To generate new certificate:
-		#  * no passphrase
-		#	gpgsm --homedir /tmp/gpghome/ \
-		#		-o /tmp/gpgsm.crt.user \
-		#		--generate-key \
-		#		--batch t/lib-gpg/gpgsm-gen-key.in
-		# To import certificate:
-		#	gpgsm --homedir /tmp/gpghome/ \
-		#		--import /tmp/gpgsm.crt.user
-		# To export into a .p12 we can later import:
-		#	gpgsm --homedir /tmp/gpghome/ \
-		#		-o t/lib-gpg/gpgsm_cert.p12 \
-		#		--export-secret-key-p12 "[e-mail]"
-		echo | gpgsm --homedir "${GNUPGHOME}" 2>/dev/null \
-			--passphrase-fd 0 --pinentry-mode loopback \
-			--import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
-
-		gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K |
-		grep fingerprint: |
-		cut -d" " -f4 |
-		tr -d '\n' >"${GNUPGHOME}/trustlist.txt" &&
-
-		echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
-		echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
-			-u [e-mail] -o /dev/null --sign - 2>&1 &&
-		test_set_prereq GPGSM
+		gpg --homedir "${GNUPGHOME}" \
+			--sign -u [e-mail] >/dev/null &&
+		test_set_prereq GPG
 		;;
 	esac
-fi
+'
+
+test_have_prereq GPG &&
+test_lazy_prereq GPGSM '
+	# Available key info:
+	# * see t/lib-gpg/gpgsm-gen-key.in
+	# To generate new certificate:
+	#  * no passphrase
+	#	gpgsm --homedir /tmp/gpghome/ \
+	#		-o /tmp/gpgsm.crt.user \
+	#		--generate-key \
+	#		--batch t/lib-gpg/gpgsm-gen-key.in
+	# To import certificate:
+	#	gpgsm --homedir /tmp/gpghome/ \
+	#		--import /tmp/gpgsm.crt.user
+	# To export into a .p12 we can later import:
+	#	gpgsm --homedir /tmp/gpghome/ \
+	#		-o t/lib-gpg/gpgsm_cert.p12 \
+	#		--export-secret-key-p12 "[e-mail]"
+	echo | gpgsm --homedir "${GNUPGHOME}" \
+		--passphrase-fd 0 --pinentry-mode loopback \
+		--import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
+	gpgsm --homedir "${GNUPGHOME}" -K |
+	grep fingerprint: |
+	cut -d" " -f4 |
+	tr -d "\\n" >"${GNUPGHOME}/trustlist.txt" &&
+	echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
+	echo hello | gpgsm --homedir "${GNUPGHOME}" \
+		-u [e-mail] -o /dev/null --sign -
+'
 
-if test_have_prereq GPG &&
-    echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1
-then
-	test_set_prereq RFC1991
-fi
+test_have_prereq GPG &&
+test_lazy_prereq RFC1991 '
+    echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991
+'
 
 sanitize_pgp() {
 	perl -ne '

Junio C Hamano March 23, 2020, 7:21 p.m. UTC | #3

Jeff King <[e-mail]> writes:

> Actually, it's pretty straight-forward and I think the resulting code is
> cleaner. Note that we do have to use a real expect_success because of
> the side effects. That does increment the test counter. IMHO that's not
> a big deal, but if we're concerned it wouldn't be too hard to add a
> non-lazy prereq block.
>
> Patch is below (I pulled GPGSM into its own block, which involved
> reindenting; view with "-w").

Nice.  Certainly a lot nicer than having to reason about what fd#3
and fd#4 are, which I always have keeping in my head.

> @@ -31,51 +34,51 @@ then
>  		chmod 0700 ./gpghome &&
>  		GNUPGHOME="$PWD/gpghome" &&
>  		export GNUPGHOME &&
> -		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
> -		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
> +		(gpgconf --kill gpg-agent || : ) &&
> +		gpg --homedir "${GNUPGHOME}" --import \
>  			"$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
> -		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \
> +		gpg --homedir "${GNUPGHOME}" --import-ownertrust \
>  			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&

Good to see all "discard output" go.

> -		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null 2>&1 \
> -			--sign -u [e-mail] &&
> -		test_set_prereq GPG &&

> +		gpg --homedir "${GNUPGHOME}" \
> +			--sign -u [e-mail] >/dev/null &&

We lost input redirection; I am assuming that it was unnecessary as
the standard input of our tests are all redirected from /dev/null
anyway?

We are not interested in seeing the signed output (we have nothing
to compare to validate the correctness anyway), so discarding the
standard output is fine.  We do not want to see it even while we are
debugging, either.

Looking good.

Jeff King March 23, 2020, 8:15 p.m. UTC | #4

On Mon, Mar 23, 2020 at 12:21:08PM -0700, Junio C Hamano wrote:

> > +		gpg --homedir "${GNUPGHOME}" \
> > +			--sign -u [e-mail] >/dev/null &&
> 
> We lost input redirection; I am assuming that it was unnecessary as
> the standard input of our tests are all redirected from /dev/null
> anyway?

Yes (though it also wouldn't hurt to leave it to explicitly document
that we're ignoring it for this command).

> We are not interested in seeing the signed output (we have nothing
> to compare to validate the correctness anyway), so discarding the
> standard output is fine.  We do not want to see it even while we are
> debugging, either.

Yes, exactly. I originally dropped it, but after looking at the "-v"
output I saw it was full of signed binary goo. :)

> Looking good.

There were actually a few subtle breakages in what I posted before. One
was just a dumb conversion: I forgot to set $gpg_version correctly. But
the other is much trickier: we have to hide our exit codes from
test_expect_success, since in the non-GPG case we want it to still
report success. So any breakage in the big &&-chain would say "test
failure", when it should just quietly continue without setting the GPG
prereq.

Here's what I came up with that I think is suitable for applying (though
if you find the GNUPGHOME thing below too gross, I can rework it as
indicated):

-- >8 --
Subject: [PATCH] t/lib-gpg: run setup code in test blocks

The steps to check the GPG prereq and set up GNUPGHOME are run in the
main script, with stdout and stderr redirected. This avoids spewing
useless output when GPG isn't available. But it also means that there's
no easy way to see what did happen if you're using "-v" or "-x".

Let's push this as much as possible into a lazy_prereq blocks, which
handle verbosity and tracing for us. There's one tricky thing here: part
of the setup involves setting $GNUPGHOME, but lazy_prereq blocks are
evaluated in a subshell in order to avoid accidental environment
contamination. Splitting the setup from the prereq is tricky; the prereq
is basically "did we successfully set things up".

We could run all of the GPG prereq code in its own test_expect_success
block. But that gets awkward because we _don't_ want to report failure
if a command fails (we just want to not set the prereq).

I've solved it here by pulling the GNUPGHOME setup into its own separate
setup step, that happens _before_ we check the prereq. That means we'd
set up the variable even if we don't have gpg, but that should be OK;
we'll be skipping any gpg tests in that case anyway. (If it's not, the
alternative is to put the big &&-chain into a separate function of "{}"
block).

Now that the code is inside test blocks, we can take advantage of this
to use &&-chaining and early returns, and avoid indenting everything
inside a big case statement.

Signed-off-by: Jeff King <[e-mail]>
---
On top of Dscho's patch 1, since it uses $PWD/gpghome.

 t/lib-gpg.sh | 145 +++++++++++++++++++++++++++------------------------
 1 file changed, 76 insertions(+), 69 deletions(-)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 11b83b8c24..56153b3123 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -1,81 +1,88 @@
 #!/bin/sh
 
-gpg_version=$(gpg --version 2>&1)
-if test $? != 127
-then
+# This can't run as part of the lazy_prereq below because it has the side
+# effect of setting an environment variable.
+test_expect_success 'set up GNUPGHOME' '
+	mkdir ./gpghome &&
+	chmod 0700 ./gpghome &&
+	GNUPGHOME="$PWD/gpghome" &&
+	export GNUPGHOME
+'
+
+test_lazy_prereq GPG '
+	{
+		gpg_version=$(gpg --version)
+		test $? != 127
+	} &&
+
 	# As said here: http://www.gnupg.org/documentation/faqs.html#q6.19
-	# the gpg version 1.0.6 didn't parse trust packets correctly, so for
+	# the gpg version 1.0.6 did not parse trust packets correctly, so for
 	# that version, creation of signed tags using the generated key fails.
 	case "$gpg_version" in
-	'gpg (GnuPG) 1.0.6'*)
-		say "Your version of gpg (1.0.6) is too buggy for testing"
+		"gpg (GnuPG) 1.0.6"*)
+		echo >&2 "Your version of gpg (1.0.6) is too buggy for testing"
+		return 1
 		;;
-	*)
-		# Available key info:
-		# * Type DSA and Elgamal, size 2048 bits, no expiration date,
-		#   name and email: C O Mitter <[e-mail]>
-		# * Type RSA, size 2048 bits, no expiration date,
-		#   name and email: Eris Discordia <[e-mail]>
-		# No password given, to enable non-interactive operation.
-		# To generate new key:
-		#	gpg --homedir /tmp/gpghome --gen-key
-		# To write armored exported key to keyring:
-		#	gpg --homedir /tmp/gpghome --export-secret-keys \
-		#		--armor 0xDEADBEEF >> lib-gpg/keyring.gpg
-		#	gpg --homedir /tmp/gpghome --export \
-		#		--armor 0xDEADBEEF >> lib-gpg/keyring.gpg
-		# To export ownertrust:
-		#	gpg --homedir /tmp/gpghome --export-ownertrust \
-		#		> lib-gpg/ownertrust
-		mkdir ./gpghome &&
-		chmod 0700 ./gpghome &&
-		GNUPGHOME="$PWD/gpghome" &&
-		export GNUPGHOME &&
-		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
-		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
-			"$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
-		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \
-			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
-		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null 2>&1 \
-			--sign -u [e-mail] &&
-		test_set_prereq GPG &&
-		# Available key info:
-		# * see t/lib-gpg/gpgsm-gen-key.in
-		# To generate new certificate:
-		#  * no passphrase
-		#	gpgsm --homedir /tmp/gpghome/ \
-		#		-o /tmp/gpgsm.crt.user \
-		#		--generate-key \
-		#		--batch t/lib-gpg/gpgsm-gen-key.in
-		# To import certificate:
-		#	gpgsm --homedir /tmp/gpghome/ \
-		#		--import /tmp/gpgsm.crt.user
-		# To export into a .p12 we can later import:
-		#	gpgsm --homedir /tmp/gpghome/ \
-		#		-o t/lib-gpg/gpgsm_cert.p12 \
-		#		--export-secret-key-p12 "[e-mail]"
-		echo | gpgsm --homedir "${GNUPGHOME}" 2>/dev/null \
-			--passphrase-fd 0 --pinentry-mode loopback \
-			--import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
+	esac &&
 
-		gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K |
-		grep fingerprint: |
-		cut -d" " -f4 |
-		tr -d '\n' >"${GNUPGHOME}/trustlist.txt" &&
+	# Available key info:
+	# * Type DSA and Elgamal, size 2048 bits, no expiration date,
+	#   name and email: C O Mitter <[e-mail]>
+	# * Type RSA, size 2048 bits, no expiration date,
+	#   name and email: Eris Discordia <[e-mail]>
+	# No password given, to enable non-interactive operation.
+	# To generate new key:
+	#	gpg --homedir /tmp/gpghome --gen-key
+	# To write armored exported key to keyring:
+	#	gpg --homedir /tmp/gpghome --export-secret-keys \
+	#		--armor 0xDEADBEEF >> lib-gpg/keyring.gpg
+	#	gpg --homedir /tmp/gpghome --export \
+	#		--armor 0xDEADBEEF >> lib-gpg/keyring.gpg
+	# To export ownertrust:
+	#	gpg --homedir /tmp/gpghome --export-ownertrust \
+	#		> lib-gpg/ownertrust
+	(gpgconf --kill gpg-agent || : ) &&
+	gpg --homedir "${GNUPGHOME}" --import \
+		"$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
+	gpg --homedir "${GNUPGHOME}" --import-ownertrust \
+		"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
+	gpg --homedir "${GNUPGHOME}" \
+		--sign -u [e-mail] >/dev/null
+'
 
-		echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
-		echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
-			-u [e-mail] -o /dev/null --sign - 2>&1 &&
-		test_set_prereq GPGSM
-		;;
-	esac
-fi
+test_have_prereq GPG &&
+test_lazy_prereq GPGSM '
+	# Available key info:
+	# * see t/lib-gpg/gpgsm-gen-key.in
+	# To generate new certificate:
+	#  * no passphrase
+	#	gpgsm --homedir /tmp/gpghome/ \
+	#		-o /tmp/gpgsm.crt.user \
+	#		--generate-key \
+	#		--batch t/lib-gpg/gpgsm-gen-key.in
+	# To import certificate:
+	#	gpgsm --homedir /tmp/gpghome/ \
+	#		--import /tmp/gpgsm.crt.user
+	# To export into a .p12 we can later import:
+	#	gpgsm --homedir /tmp/gpghome/ \
+	#		-o t/lib-gpg/gpgsm_cert.p12 \
+	#		--export-secret-key-p12 "[e-mail]"
+	echo | gpgsm --homedir "${GNUPGHOME}" \
+		--passphrase-fd 0 --pinentry-mode loopback \
+		--import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
+	gpgsm --homedir "${GNUPGHOME}" -K |
+	grep fingerprint: |
+	cut -d" " -f4 |
+	tr -d "\\n" >"${GNUPGHOME}/trustlist.txt" &&
+	echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
+	echo hello | gpgsm --homedir "${GNUPGHOME}" \
+		-u [e-mail] -o /dev/null --sign -
+'
 
-if test_have_prereq GPG &&
-    echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1
-then
-	test_set_prereq RFC1991
-fi
+test_have_prereq GPG &&
+test_lazy_prereq RFC1991 '
+    echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991
+'
 
 sanitize_pgp() {
 	perl -ne '

Junio C Hamano March 23, 2020, 9:28 p.m. UTC | #5

Jeff King <[e-mail]> writes:

> Here's what I came up with that I think is suitable for applying (though
> if you find the GNUPGHOME thing below too gross, I can rework it as
> indicated):

I actually think it is perfectly fine to mkdir and set the
environment even outside test_expect_success; that way, even
GIT_SKIP_TESTS cannot omit the necessary initialization.  And as you
said, leaving the environment pointing into the trash repository's
working tree should be fine when we fail the GPG prereq.  We
shouldn't be running GPG at all in such a case.

> -- >8 --
> Subject: [PATCH] t/lib-gpg: run setup code in test blocks
>
> The steps to check the GPG prereq and set up GNUPGHOME are run in the
> main script, with stdout and stderr redirected. This avoids spewing
> useless output when GPG isn't available. But it also means that there's
> no easy way to see what did happen if you're using "-v" or "-x".
>
> Let's push this as much as possible into a lazy_prereq blocks, which
> handle verbosity and tracing for us. There's one tricky thing here: part
> of the setup involves setting $GNUPGHOME, but lazy_prereq blocks are
> evaluated in a subshell in order to avoid accidental environment
> contamination. Splitting the setup from the prereq is tricky; the prereq
> is basically "did we successfully set things up".
>
> We could run all of the GPG prereq code in its own test_expect_success
> block. But that gets awkward because we _don't_ want to report failure
> if a command fails (we just want to not set the prereq).
>
> I've solved it here by pulling the GNUPGHOME setup into its own separate
> setup step, that happens _before_ we check the prereq. That means we'd
> set up the variable even if we don't have gpg, but that should be OK;
> we'll be skipping any gpg tests in that case anyway. (If it's not, the
> alternative is to put the big &&-chain into a separate function of "{}"
> block).
>
> Now that the code is inside test blocks, we can take advantage of this
> to use &&-chaining and early returns, and avoid indenting everything
> inside a big case statement.
>
> Signed-off-by: Jeff King <[e-mail]>
> ---
> On top of Dscho's patch 1, since it uses $PWD/gpghome.

Looking good.

>  t/lib-gpg.sh | 145 +++++++++++++++++++++++++++------------------------
>  1 file changed, 76 insertions(+), 69 deletions(-)
>
> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> index 11b83b8c24..56153b3123 100755
> --- a/t/lib-gpg.sh
> +++ b/t/lib-gpg.sh
> @@ -1,81 +1,88 @@
>  #!/bin/sh
>  
> -gpg_version=$(gpg --version 2>&1)
> -if test $? != 127
> -then
> +# This can't run as part of the lazy_prereq below because it has the side
> +# effect of setting an environment variable.
> +test_expect_success 'set up GNUPGHOME' '
> +	mkdir ./gpghome &&
> +	chmod 0700 ./gpghome &&
> +	GNUPGHOME="$PWD/gpghome" &&
> +	export GNUPGHOME
> +'
> +
> +test_lazy_prereq GPG '
> +	{
> +		gpg_version=$(gpg --version)
> +		test $? != 127
> +	} &&
> +
>  	# As said here: http://www.gnupg.org/documentation/faqs.html#q6.19
> -	# the gpg version 1.0.6 didn't parse trust packets correctly, so for
> +	# the gpg version 1.0.6 did not parse trust packets correctly, so for
>  	# that version, creation of signed tags using the generated key fails.
>  	case "$gpg_version" in
> -	'gpg (GnuPG) 1.0.6'*)
> -		say "Your version of gpg (1.0.6) is too buggy for testing"
> +		"gpg (GnuPG) 1.0.6"*)
> +		echo >&2 "Your version of gpg (1.0.6) is too buggy for testing"
> +		return 1
>  		;;
> -	*)
> -		# Available key info:
> -		# * Type DSA and Elgamal, size 2048 bits, no expiration date,
> -		#   name and email: C O Mitter <[e-mail]>
> -		# * Type RSA, size 2048 bits, no expiration date,
> -		#   name and email: Eris Discordia <[e-mail]>
> -		# No password given, to enable non-interactive operation.
> -		# To generate new key:
> -		#	gpg --homedir /tmp/gpghome --gen-key
> -		# To write armored exported key to keyring:
> -		#	gpg --homedir /tmp/gpghome --export-secret-keys \
> -		#		--armor 0xDEADBEEF >> lib-gpg/keyring.gpg
> -		#	gpg --homedir /tmp/gpghome --export \
> -		#		--armor 0xDEADBEEF >> lib-gpg/keyring.gpg
> -		# To export ownertrust:
> -		#	gpg --homedir /tmp/gpghome --export-ownertrust \
> -		#		> lib-gpg/ownertrust
> -		mkdir ./gpghome &&
> -		chmod 0700 ./gpghome &&
> -		GNUPGHOME="$PWD/gpghome" &&
> -		export GNUPGHOME &&
> -		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
> -		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
> -			"$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
> -		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \
> -			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
> -		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null 2>&1 \
> -			--sign -u [e-mail] &&
> -		test_set_prereq GPG &&
> -		# Available key info:
> -		# * see t/lib-gpg/gpgsm-gen-key.in
> -		# To generate new certificate:
> -		#  * no passphrase
> -		#	gpgsm --homedir /tmp/gpghome/ \
> -		#		-o /tmp/gpgsm.crt.user \
> -		#		--generate-key \
> -		#		--batch t/lib-gpg/gpgsm-gen-key.in
> -		# To import certificate:
> -		#	gpgsm --homedir /tmp/gpghome/ \
> -		#		--import /tmp/gpgsm.crt.user
> -		# To export into a .p12 we can later import:
> -		#	gpgsm --homedir /tmp/gpghome/ \
> -		#		-o t/lib-gpg/gpgsm_cert.p12 \
> -		#		--export-secret-key-p12 "[e-mail]"
> -		echo | gpgsm --homedir "${GNUPGHOME}" 2>/dev/null \
> -			--passphrase-fd 0 --pinentry-mode loopback \
> -			--import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
> +	esac &&
>  
> -		gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K |
> -		grep fingerprint: |
> -		cut -d" " -f4 |
> -		tr -d '\n' >"${GNUPGHOME}/trustlist.txt" &&
> +	# Available key info:
> +	# * Type DSA and Elgamal, size 2048 bits, no expiration date,
> +	#   name and email: C O Mitter <[e-mail]>
> +	# * Type RSA, size 2048 bits, no expiration date,
> +	#   name and email: Eris Discordia <[e-mail]>
> +	# No password given, to enable non-interactive operation.
> +	# To generate new key:
> +	#	gpg --homedir /tmp/gpghome --gen-key
> +	# To write armored exported key to keyring:
> +	#	gpg --homedir /tmp/gpghome --export-secret-keys \
> +	#		--armor 0xDEADBEEF >> lib-gpg/keyring.gpg
> +	#	gpg --homedir /tmp/gpghome --export \
> +	#		--armor 0xDEADBEEF >> lib-gpg/keyring.gpg
> +	# To export ownertrust:
> +	#	gpg --homedir /tmp/gpghome --export-ownertrust \
> +	#		> lib-gpg/ownertrust
> +	(gpgconf --kill gpg-agent || : ) &&
> +	gpg --homedir "${GNUPGHOME}" --import \
> +		"$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
> +	gpg --homedir "${GNUPGHOME}" --import-ownertrust \
> +		"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
> +	gpg --homedir "${GNUPGHOME}" \
> +		--sign -u [e-mail] >/dev/null
> +'
>  
> -		echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
> -		echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
> -			-u [e-mail] -o /dev/null --sign - 2>&1 &&
> -		test_set_prereq GPGSM
> -		;;
> -	esac
> -fi
> +test_have_prereq GPG &&
> +test_lazy_prereq GPGSM '
> +	# Available key info:
> +	# * see t/lib-gpg/gpgsm-gen-key.in
> +	# To generate new certificate:
> +	#  * no passphrase
> +	#	gpgsm --homedir /tmp/gpghome/ \
> +	#		-o /tmp/gpgsm.crt.user \
> +	#		--generate-key \
> +	#		--batch t/lib-gpg/gpgsm-gen-key.in
> +	# To import certificate:
> +	#	gpgsm --homedir /tmp/gpghome/ \
> +	#		--import /tmp/gpgsm.crt.user
> +	# To export into a .p12 we can later import:
> +	#	gpgsm --homedir /tmp/gpghome/ \
> +	#		-o t/lib-gpg/gpgsm_cert.p12 \
> +	#		--export-secret-key-p12 "[e-mail]"
> +	echo | gpgsm --homedir "${GNUPGHOME}" \
> +		--passphrase-fd 0 --pinentry-mode loopback \
> +		--import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
> +	gpgsm --homedir "${GNUPGHOME}" -K |
> +	grep fingerprint: |
> +	cut -d" " -f4 |
> +	tr -d "\\n" >"${GNUPGHOME}/trustlist.txt" &&
> +	echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
> +	echo hello | gpgsm --homedir "${GNUPGHOME}" \
> +		-u [e-mail] -o /dev/null --sign -
> +'
>  
> -if test_have_prereq GPG &&
> -    echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1
> -then
> -	test_set_prereq RFC1991
> -fi
> +test_have_prereq GPG &&
> +test_lazy_prereq RFC1991 '
> +    echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991
> +'
>  
>  sanitize_pgp() {
>  	perl -ne '

Jeff King March 23, 2020, 9:31 p.m. UTC | #6

On Mon, Mar 23, 2020 at 02:28:58PM -0700, Junio C Hamano wrote:

> Jeff King <[e-mail]> writes:
> 
> > Here's what I came up with that I think is suitable for applying (though
> > if you find the GNUPGHOME thing below too gross, I can rework it as
> > indicated):
> 
> I actually think it is perfectly fine to mkdir and set the
> environment even outside test_expect_success; that way, even
> GIT_SKIP_TESTS cannot omit the necessary initialization.  And as you
> said, leaving the environment pointing into the trash repository's
> working tree should be fine when we fail the GPG prereq.  We
> shouldn't be running GPG at all in such a case.

I have a slight preference to do it in an expect_success block, because
then we'd notice the error more readily (and it gets the benefit
verbosity and tracing, too!).

The thing I was more worried about is that it's technically a behavior
change to set up GNUPGHOME when we're not going to use it (as well as
create the directory). But I find it hard to imagine a test that would
be affected where my suggested solution wouldn't be "fix the test".

-Peff

Johannes Schindelin March 24, 2020, 9:41 p.m. UTC | #7

Hi Peff,

On Mon, 23 Mar 2020, Jeff King wrote:

> On Mon, Mar 23, 2020 at 02:28:58PM -0700, Junio C Hamano wrote:
>
> > Jeff King <[e-mail]> writes:
> >
> > > Here's what I came up with that I think is suitable for applying (though
> > > if you find the GNUPGHOME thing below too gross, I can rework it as
> > > indicated):
> >
> > I actually think it is perfectly fine to mkdir and set the
> > environment even outside test_expect_success; that way, even
> > GIT_SKIP_TESTS cannot omit the necessary initialization.  And as you
> > said, leaving the environment pointing into the trash repository's
> > working tree should be fine when we fail the GPG prereq.  We
> > shouldn't be running GPG at all in such a case.
>
> I have a slight preference to do it in an expect_success block, because
> then we'd notice the error more readily (and it gets the benefit
> verbosity and tracing, too!).
>
> The thing I was more worried about is that it's technically a behavior
> change to set up GNUPGHOME when we're not going to use it (as well as
> create the directory). But I find it hard to imagine a test that would
> be affected where my suggested solution wouldn't be "fix the test".

It is _half_ a change in behavior: in case that `gpg` was found, and does
not have a known-bad version, we set up the environment variable, _even
if_ the test-signing fails. In other words, we don't roll back the
environment variable.

As such, I figure that setting it globally _before_ even evaluating the
prereq is okay.

Therefore, it is relatively easy to turn this thing into a set of lazy
prereqs, which is better, conceptually, I think. I am in the process of
making it so.

Ciao,
Dscho

Jeff King March 24, 2020, 10:05 p.m. UTC | #8

On Tue, Mar 24, 2020 at 10:41:58PM +0100, Johannes Schindelin wrote:

> > The thing I was more worried about is that it's technically a behavior
> > change to set up GNUPGHOME when we're not going to use it (as well as
> > create the directory). But I find it hard to imagine a test that would
> > be affected where my suggested solution wouldn't be "fix the test".
> 
> It is _half_ a change in behavior: in case that `gpg` was found, and does
> not have a known-bad version, we set up the environment variable, _even
> if_ the test-signing fails. In other words, we don't roll back the
> environment variable.
> 
> As such, I figure that setting it globally _before_ even evaluating the
> prereq is okay.
> 
> Therefore, it is relatively easy to turn this thing into a set of lazy
> prereqs, which is better, conceptually, I think. I am in the process of
> making it so.

Er, isn't that what my patch did? I'm fine if you have another approach
to present, but I'm worried we might be duplicating effort.

-Peff

Johannes Schindelin March 24, 2020, 10:25 p.m. UTC | #9

Hi Peff,

On Tue, 24 Mar 2020, Jeff King wrote:

> On Tue, Mar 24, 2020 at 10:41:58PM +0100, Johannes Schindelin wrote:
>
> > > The thing I was more worried about is that it's technically a behavior
> > > change to set up GNUPGHOME when we're not going to use it (as well as
> > > create the directory). But I find it hard to imagine a test that would
> > > be affected where my suggested solution wouldn't be "fix the test".
> >
> > It is _half_ a change in behavior: in case that `gpg` was found, and does
> > not have a known-bad version, we set up the environment variable, _even
> > if_ the test-signing fails. In other words, we don't roll back the
> > environment variable.
> >
> > As such, I figure that setting it globally _before_ even evaluating the
> > prereq is okay.
> >
> > Therefore, it is relatively easy to turn this thing into a set of lazy
> > prereqs, which is better, conceptually, I think. I am in the process of
> > making it so.
>
> Er, isn't that what my patch did? I'm fine if you have another approach
> to present, but I'm worried we might be duplicating effort.

I missed that your second patch made `GPG` lazy, too.

My version is slightly different from yours, though: I do not insist on
setting the environment variable `GNUPGHOME` only after the `mkdir`
succeeds, as the `gpg --sign` later on might fail anyway, which means that
we _already_ could end up with `GNUPGHOME` set and the prereq `GPG` _not_
set.

Ciao,
Dscho

Jeff King March 24, 2020, 10:33 p.m. UTC | #10

On Tue, Mar 24, 2020 at 11:25:09PM +0100, Johannes Schindelin wrote:

> > Er, isn't that what my patch did? I'm fine if you have another approach
> > to present, but I'm worried we might be duplicating effort.
> 
> I missed that your second patch made `GPG` lazy, too.
> 
> My version is slightly different from yours, though: I do not insist on
> setting the environment variable `GNUPGHOME` only after the `mkdir`
> succeeds, as the `gpg --sign` later on might fail anyway, which means that
> we _already_ could end up with `GNUPGHOME` set and the prereq `GPG` _not_
> set.

Yes. That mkdir could also be pushed down into the GPG prereq for the
same reason. It's pretty unlikely the mkdir would fail, and I thought it
would be less confusing (in the off chance that anybody even looks at it
when GPG isn't set) if we had a GNUPGHOME that pointed to an _actual_
directory, instead of something that didn't exist.

But that's kind of an arbitrary cutoff. The GPG prereq is also importing
keys and other stuff, so the state of that directory when GPG isn't set
is undefined (but again, nobody's supposed to be looking at it, so...).

-Peff

Patch

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 11b83b8c24a..a7d0708f5df 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -11,6 +11,8 @@  then
 		say "Your version of gpg (1.0.6) is too buggy for testing"
 		;;
 	*)
+		say_color info >&4 "Trying to set up GPG"
+		want_trace && set -x
 		# Available key info:
 		# * Type DSA and Elgamal, size 2048 bits, no expiration date,
 		#   name and email: C O Mitter <[e-mail]>
@@ -31,13 +33,13 @@  then
 		chmod 0700 ./gpghome &&
 		GNUPGHOME="$PWD/gpghome" &&
 		export GNUPGHOME &&
-		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
-		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
-			"$TEST_DIRECTORY"/lib-gpg/keyring.gpg &&
-		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \
-			"$TEST_DIRECTORY"/lib-gpg/ownertrust &&
-		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null 2>&1 \
-			--sign -u [e-mail] &&
+		(gpgconf --kill gpg-agent >&3 2>&4 || : ) &&
+		gpg --homedir "${GNUPGHOME}" --import \
+			"$TEST_DIRECTORY"/lib-gpg/keyring.gpg >&3 2>&4 &&
+		gpg --homedir "${GNUPGHOME}" --import-ownertrust \
+			"$TEST_DIRECTORY"/lib-gpg/ownertrust >&3 2>&4 &&
+		gpg --homedir "${GNUPGHOME}" </dev/null \
+			--sign -u [e-mail] >&3 2>&4 &&
 		test_set_prereq GPG &&
 		# Available key info:
 		# * see t/lib-gpg/gpgsm-gen-key.in
@@ -54,28 +56,29 @@  then
 		#	gpgsm --homedir /tmp/gpghome/ \
 		#		-o t/lib-gpg/gpgsm_cert.p12 \
 		#		--export-secret-key-p12 "[e-mail]"
-		echo | gpgsm --homedir "${GNUPGHOME}" 2>/dev/null \
+		echo | gpgsm --homedir "${GNUPGHOME}" >&3 2>&4 \
 			--passphrase-fd 0 --pinentry-mode loopback \
 			--import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
 
-		gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K |
+		gpgsm --homedir "${GNUPGHOME}" -K 2>&4 |
 		grep fingerprint: |
 		cut -d" " -f4 |
 		tr -d '\n' >"${GNUPGHOME}/trustlist.txt" &&
 
 		echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
-		echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
-			-u [e-mail] -o /dev/null --sign - 2>&1 &&
+		echo hello | gpgsm --homedir "${GNUPGHOME}" >&3 2>&4 \
+			-u [e-mail] -o /dev/null --sign - &&
 		test_set_prereq GPGSM
 		;;
 	esac
 fi
 
 if test_have_prereq GPG &&
-    echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1
+    echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >&3 2>&4
 then
 	test_set_prereq RFC1991
 fi
+want_trace && set +x
 
 sanitize_pgp() {
 	perl -ne '