[PATCH 0/2] Enable GPG in the Windows part of the CI/PR builds

[PATCH 0/2] Enable GPG in the Windows part of the CI/PR builds

[Johannes Schindelin via GitGitGadget]

While debugging the breakages introduced by hi/gpg-prefer-check-signature, I
noticed that the GPG prereq was not available on Windows, even if Git for
Windows' SDK comes with a fully functional GPG2.

The fix was easy, but finding out what was going on was not, so for good
measure, the fix is accompanied by a second patch that will hopefully make
future investigations into GPG-related problems much, much easier.

Johannes Schindelin (2):
  tests(gpg): allow the gpg-agent to start on Windows
  tests(gpg): increase verbosity to allow debugging

 t/lib-gpg.sh | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)


base-commit: 30e9940356dc67959877f4b2417da33ebdefbb79
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-728%2Fdscho%2Fci-windows-gpg-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-728/dscho/ci-windows-gpg-v1
Pull-Request: https://github.com/git/git/pull/728
-- 
gitgitgadget

[PATCH 1/2] tests(gpg): allow the gpg-agent to start on Windows

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In Git for Windows' SDK, we use the MSYS2 version of OpenSSH, meaning
that the `gpg-agent` will fail horribly when being passed a `--homedir`
that contains colons.

Previously, we did pass the Windows version of the absolute path,
though, which starts in the drive letter followed by, you guessed it, a
colon.

Let's use the same trick found elsewhere in our test suite where `$PWD`
is used to refer to the pseudo-Unix path (which works only within the
MSYS2 Bash/OpenSSH/Perl/etc, as opposed to `$(pwd)` which refers to the
Windows path that `git.exe` understands, too).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/lib-gpg.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 8d28652b729..11b83b8c24a 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -29,7 +29,7 @@ then
 		#		> lib-gpg/ownertrust
 		mkdir ./gpghome &&
 		chmod 0700 ./gpghome &&
-		GNUPGHOME="$(pwd)/gpghome" &&
+		GNUPGHOME="$PWD/gpghome" &&
 		export GNUPGHOME &&
 		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
 		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 <johannes.schindelin@gmx.de>
---
 t/lib-gpg.sh | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

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 <committer@example.com>
@@ -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 committer@example.com &&
+		(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 committer@example.com >&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 "committer@example.com"
-		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 committer@example.com -o /dev/null --sign - 2>&1 &&
+		echo hello | gpgsm --homedir "${GNUPGHOME}" >&3 2>&4 \
+			-u committer@example.com -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 '
-- 
gitgitgadget

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

[Jeff King]

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

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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

Re: [PATCH 1/2] tests(gpg): allow the gpg-agent to start on Windows

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> In Git for Windows' SDK, we use the MSYS2 version of OpenSSH, meaning
> that the `gpg-agent` will fail horribly when being passed a `--homedir`
> that contains colons.
>
> Previously, we did pass the Windows version of the absolute path,
> though, which starts in the drive letter followed by, you guessed it, a
> colon.
>
> Let's use the same trick found elsewhere in our test suite where `$PWD`
> is used to refer to the pseudo-Unix path (which works only within the
> MSYS2 Bash/OpenSSH/Perl/etc, as opposed to `$(pwd)` which refers to the
> Windows path that `git.exe` understands, too).

Makes sense.  

Do we have a short/concise instruction, e.g. "You should use $(pwd)
in most cases, but for such and such purposes use $PWD instead", in
t/README for test writers, who are not familiar with the distinction
between $(pwd) and $PWD, to help them decide which one to use in
what situation?  I see this kind of fix-ups from time to time, and
am wondering if there is a way to reduce the need for you or J6t to
spot and fix the new ones.

Thanks.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/lib-gpg.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> index 8d28652b729..11b83b8c24a 100755
> --- a/t/lib-gpg.sh
> +++ b/t/lib-gpg.sh
> @@ -29,7 +29,7 @@ then
>  		#		> lib-gpg/ownertrust
>  		mkdir ./gpghome &&
>  		chmod 0700 ./gpghome &&
> -		GNUPGHOME="$(pwd)/gpghome" &&
> +		GNUPGHOME="$PWD/gpghome" &&
>  		export GNUPGHOME &&
>  		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
>  		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \

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

[Jeff King]

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 committer@example.com &&
-		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 "committer@example.com"
-		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 committer@example.com -o /dev/null --sign - 2>&1 &&
-		test_set_prereq GPGSM
+		gpg --homedir "${GNUPGHOME}" \
+			--sign -u committer@example.com >/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 "committer@example.com"
+	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 committer@example.com -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 '

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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 committer@example.com &&
> -		test_set_prereq GPG &&

> +		gpg --homedir "${GNUPGHOME}" \
> +			--sign -u committer@example.com >/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.

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

[Jeff King]

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

> > +		gpg --homedir "${GNUPGHOME}" \
> > +			--sign -u committer@example.com >/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 <peff@peff.net>
---
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 <committer@example.com>
-		# * Type RSA, size 2048 bits, no expiration date,
-		#   name and email: Eris Discordia <discord@example.net>
-		# 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 committer@example.com &&
-		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 "committer@example.com"
-		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 <committer@example.com>
+	# * Type RSA, size 2048 bits, no expiration date,
+	#   name and email: Eris Discordia <discord@example.net>
+	# 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 committer@example.com >/dev/null
+'
 
-		echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
-		echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
-			-u committer@example.com -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 "committer@example.com"
+	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 committer@example.com -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 '
-- 
2.26.0.561.g096102dc45

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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 <peff@peff.net>
> ---
> 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 <committer@example.com>
> -		# * Type RSA, size 2048 bits, no expiration date,
> -		#   name and email: Eris Discordia <discord@example.net>
> -		# 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 committer@example.com &&
> -		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 "committer@example.com"
> -		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 <committer@example.com>
> +	# * Type RSA, size 2048 bits, no expiration date,
> +	#   name and email: Eris Discordia <discord@example.net>
> +	# 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 committer@example.com >/dev/null
> +'
>  
> -		echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
> -		echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
> -			-u committer@example.com -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 "committer@example.com"
> +	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 committer@example.com -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 '

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

[Jeff King]

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

> Jeff King <peff@peff.net> 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

Re: [PATCH 1/2] tests(gpg): allow the gpg-agent to start on Windows

[Johannes Schindelin]

Hi Junio,

On Mon, 23 Mar 2020, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > In Git for Windows' SDK, we use the MSYS2 version of OpenSSH, meaning
> > that the `gpg-agent` will fail horribly when being passed a `--homedir`
> > that contains colons.
> >
> > Previously, we did pass the Windows version of the absolute path,
> > though, which starts in the drive letter followed by, you guessed it, a
> > colon.
> >
> > Let's use the same trick found elsewhere in our test suite where `$PWD`
> > is used to refer to the pseudo-Unix path (which works only within the
> > MSYS2 Bash/OpenSSH/Perl/etc, as opposed to `$(pwd)` which refers to the
> > Windows path that `git.exe` understands, too).
>
> Makes sense.
>
> Do we have a short/concise instruction, e.g. "You should use $(pwd)
> in most cases, but for such and such purposes use $PWD instead", in
> t/README for test writers, who are not familiar with the distinction
> between $(pwd) and $PWD, to help them decide which one to use in
> what situation?  I see this kind of fix-ups from time to time, and
> am wondering if there is a way to reduce the need for you or J6t to
> spot and fix the new ones.

I fear that this distinction really is lost on anybody who does not have
to deal with MSYS2 on Windows.

It is subtle enough a distinction, too: whenever Bash or Perl is
concerned, we _might_ run into this issue. I say _might_ because _some_
scripts actually handle Windows paths correctly, but others don't (testing
for absolute paths by looking for a slash at the beginning would be an
example, and it gets really hairy when you slap Windows paths at the end
of `PATH`, separated by, you guessed it, a colon).

It gets even worse: you might think that you have to use `$(pwd)` when
passing the path to `git.exe` (because it is a non-MSYS2 program). But you
don't, in many cases. For example, when you call

	git config my.pwd "$PWD"

it totally works (because the MSYS2 runtime on top of which Bash runs will
convert the parameters that are passed to a non-MSYS2 program when they
look like paths).

The problem solved by _this here_ patch is the opposite, of course: we are
passing a Windows path (`$(pwd)` implicitly calls `$(pwd -W)` in our test
suite on Windows) to an _MSYS2_ program: `gpg.exe`. And how would
contributors whose main development platform isn't Windows be able to
guess that `gpg.exe` is an MSYS2 program as opposed to, say, `tclsh.exe`
which is a non-MSYS2 program? They wouldn't.

In short: I am convinced that this is a subtlety in our test suite that we
cannot reasonably expect any contributors other than Windows-based ones to
get right, and I am fairly certain that we will just have to keep
monitoring the CI/PR builds for similar issues and then help the
contributors by suggesting the appropriate fixes.

Ciao,
Dscho

> Thanks.
>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  t/lib-gpg.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
> > index 8d28652b729..11b83b8c24a 100755
> > --- a/t/lib-gpg.sh
> > +++ b/t/lib-gpg.sh
> > @@ -29,7 +29,7 @@ then
> >  		#		> lib-gpg/ownertrust
> >  		mkdir ./gpghome &&
> >  		chmod 0700 ./gpghome &&
> > -		GNUPGHOME="$(pwd)/gpghome" &&
> > +		GNUPGHOME="$PWD/gpghome" &&
> >  		export GNUPGHOME &&
> >  		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
> >  		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
>

Re: [PATCH 1/2] tests(gpg): allow the gpg-agent to start on Windows

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> I fear that this distinction really is lost on anybody who does not have
> to deal with MSYS2 on Windows.
> ...
> In short: I am convinced that this is a subtlety in our test suite that we
> cannot reasonably expect any contributors other than Windows-based ones to
> get right, and I am fairly certain that we will just have to keep
> monitoring the CI/PR builds for similar issues and then help the
> contributors by suggesting the appropriate fixes.

IOW, this cannot be made an engineering problem and has to stay a
black art? ;-)

That's somewhat sad, but I guess we have to live with it.

Thanks.

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

[Johannes Schindelin]

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 <peff@peff.net> 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

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

[Jeff King]

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

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

[Johannes Schindelin]

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

Re: [PATCH 1/2] tests(gpg): allow the gpg-agent to start on Windows

[Johannes Schindelin]

Hi Junio,

On Tue, 24 Mar 2020, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > I fear that this distinction really is lost on anybody who does not have
> > to deal with MSYS2 on Windows.
> > ...
> > In short: I am convinced that this is a subtlety in our test suite that we
> > cannot reasonably expect any contributors other than Windows-based ones to
> > get right, and I am fairly certain that we will just have to keep
> > monitoring the CI/PR builds for similar issues and then help the
> > contributors by suggesting the appropriate fixes.
>
> IOW, this cannot be made an engineering problem and has to stay a
> black art? ;-)
>
> That's somewhat sad, but I guess we have to live with it.

I am afraid so.

Ciao,
Dscho

>
> Thanks.
>
>
>

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

[Jeff King]

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

Re: [PATCH 1/2] tests(gpg): allow the gpg-agent to start on Windows

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Junio,
>
> On Tue, 24 Mar 2020, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>> > I fear that this distinction really is lost on anybody who does not have
>> > to deal with MSYS2 on Windows.
>> > ...
>> > In short: I am convinced that this is a subtlety in our test suite that we
>> > cannot reasonably expect any contributors other than Windows-based ones to
>> > get right, and I am fairly certain that we will just have to keep
>> > monitoring the CI/PR builds for similar issues and then help the
>> > contributors by suggesting the appropriate fixes.
>>
>> IOW, this cannot be made an engineering problem and has to stay a
>> black art? ;-)
>>
>> That's somewhat sad, but I guess we have to live with it.
>
> I am afraid so.

As long as there are our users on Windows, we can trust that we'd
have some folks who are as familiar as you are with these
subtleties, even if you may get bored working on it yourself,
leaving us with the right bus factor, right?  ;-)

Thanks.

[PATCH v2 0/5] Enable GPG in the Windows part of the CI/PR builds

[Johannes Schindelin via GitGitGadget]

While debugging the breakages introduced by hi/gpg-prefer-check-signature, I
noticed that the GPG prereq was not available on Windows, even if Git for
Windows' SDK comes with a fully functional GPG2.

The fix was easy, but finding out what was going on was not, so for good
measure, the fix is accompanied by a patch that will hopefully make future
investigations into GPG-related problems much, much easier.

Changes since v1:

 * The prereqs are now lazy ones.
   
   
 * A new patch was introduced to make tracing via -x work even with those
   inter-dependent prereqs.
   
   
 * The test-signing's stdout is redirected to /dev/null because it is
   unreadable and unhelpful binary gibberish, anyway. (This imitates Peff's
   patch.)

Johannes Schindelin (5):
  tests(gpg): allow the gpg-agent to start on Windows
  t/lib-gpg.sh: stop pretending to be a stand-alone script
  tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
  tests: do not let lazy prereqs inside `test_expect_*` turn off tracing
  tests: increase the verbosity of the GPG-related prereqs

 t/lib-gpg.sh     | 110 ++++++++++++++++++++++++++---------------------
 t/t0000-basic.sh |  13 ++++++
 t/test-lib.sh    |   6 ++-
 3 files changed, 77 insertions(+), 52 deletions(-)


base-commit: 30e9940356dc67959877f4b2417da33ebdefbb79
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-728%2Fdscho%2Fci-windows-gpg-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-728/dscho/ci-windows-gpg-v2
Pull-Request: https://github.com/git/git/pull/728

Range-diff vs v1:

 1:  287a21f1033 = 1:  287a21f1033 tests(gpg): allow the gpg-agent to start on Windows
 -:  ----------- > 2:  c1811d54190 t/lib-gpg.sh: stop pretending to be a stand-alone script
 2:  dd26cb05a37 ! 3:  85457a7b618 tests(gpg): increase verbosity to allow debugging
     @@ -1,21 +1,36 @@
      Author: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     -    tests(gpg): increase verbosity to allow debugging
     +    tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
      
     -    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.
     +    The code to set those prereqs is executed completely outside of any
     +    `test_eval_` block. As a consequence, its output had to be suppressed so
     +    that it does not clutter the output of a regular test script run.
      
     -    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.
     +    Unfortunately, the output *stays* suppressed even when the `--verbose`
     +    option is in effect.
      
     -    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.
     +    This hid important output when debugging why the GPG prereq was not
     +    enabled in the Windows part of our CI builds.
     +
     +    In preparation for fixing that, let's move all of this code into lazy
     +    prereqs.
     +
     +    The only slightly tricky part is the global environment variable
     +    `GNUPGHOME`. Originally, it was configured only when we verified that
     +    there is a `gpg` in the `PATH` that we can use. This is now no longer
     +    possible, as lazy prereqs are evaluated in a subshell that changes the
     +    working directory to a temporary one. Therefore, we simply _always_ set
     +    that environment variable: it does not hurt anything because it does not
     +    indicate the presence of a working GPG.
     +
     +    Side note: it was quite tempting to use a hack that is possible because
     +    we do not validate what is passed to `test_lazy_prereq` (and it is
     +    therefore possible to "break out" of the lazy_prereq subshell:
     +
     +            test_lazy_prereq GPG '...) && GNUPGHOME=... && (...'
     +
     +    However, this is rather tricksy hobbitses code, and the current patch is
     +    _much_ easier to understand.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
     @@ -23,67 +38,128 @@
       --- a/t/lib-gpg.sh
       +++ b/t/lib-gpg.sh
      @@
     +-gpg_version=$(gpg --version 2>&1)
     +-if test $? != 127
     +-then
     ++# We always set GNUPGHOME, even if no usable GPG was found, as
     ++#
     ++# - It does not hurt, and
     ++#
     ++# - we cannot set global environment variables in lazy prereqs because they are
     ++#   executed in an eval'ed subshell that changes the working directory to a
     ++#   temporary one.
     ++
     ++GNUPGHOME="$PWD/gpghome"
     ++export GNUPGHOME
     ++
     ++test_lazy_prereq GPG '
     ++	gpg_version=$(gpg --version 2>&1)
     ++	test $? != 127 || exit 1
     ++
     + 	# 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"
     ++		exit 1
       		;;
       	*)
     -+		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 <committer@example.com>
      @@
     - 		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 \
     + 		# To export ownertrust:
     + 		#	gpg --homedir /tmp/gpghome --export-ownertrust \
     + 		#		> lib-gpg/ownertrust
     +-		mkdir ./gpghome &&
     +-		chmod 0700 ./gpghome &&
     +-		GNUPGHOME="$PWD/gpghome" &&
     +-		export GNUPGHOME &&
     ++		mkdir "$GNUPGHOME" &&
     ++		chmod 0700 "$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 committer@example.com &&
     -+		(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 committer@example.com >&3 2>&4 &&
     - 		test_set_prereq GPG &&
     - 		# Available key info:
     - 		# * see t/lib-gpg/gpgsm-gen-key.in
     -@@
     - 		#	gpgsm --homedir /tmp/gpghome/ \
     - 		#		-o t/lib-gpg/gpgsm_cert.p12 \
     - 		#		--export-secret-key-p12 "committer@example.com"
     +-		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 "committer@example.com"
      -		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 &&
     - 
     +-			--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" &&
     +-		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 committer@example.com -o /dev/null --sign - 2>&1 &&
     -+		echo hello | gpgsm --homedir "${GNUPGHOME}" >&3 2>&4 \
     -+			-u committer@example.com -o /dev/null --sign - &&
     - 		test_set_prereq GPGSM
     +-		test_set_prereq GPGSM
     ++			--sign -u committer@example.com
       		;;
       	esac
     - fi
     +-fi
     ++'
     ++
     ++test_lazy_prereq GPGSM '
     ++	test_have_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 "committer@example.com"
     ++       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 committer@example.com -o /dev/null --sign - 2>&1
     ++'
       
     - if test_have_prereq GPG &&
     +-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
     +-then
     +-	test_set_prereq RFC1991
     +-fi
     ++test_lazy_prereq RFC1991 '
     ++	test_have_prereq GPG &&
     ++	echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1
     ++'
       
       sanitize_pgp() {
       	perl -ne '
 -:  ----------- > 4:  0767c8b77c8 tests: do not let lazy prereqs inside `test_expect_*` turn off tracing
 -:  ----------- > 5:  5e89b512513 tests: increase the verbosity of the GPG-related prereqs

-- 
gitgitgadget

[PATCH v2 1/5] tests(gpg): allow the gpg-agent to start on Windows

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In Git for Windows' SDK, we use the MSYS2 version of OpenSSH, meaning
that the `gpg-agent` will fail horribly when being passed a `--homedir`
that contains colons.

Previously, we did pass the Windows version of the absolute path,
though, which starts in the drive letter followed by, you guessed it, a
colon.

Let's use the same trick found elsewhere in our test suite where `$PWD`
is used to refer to the pseudo-Unix path (which works only within the
MSYS2 Bash/OpenSSH/Perl/etc, as opposed to `$(pwd)` which refers to the
Windows path that `git.exe` understands, too).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/lib-gpg.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 8d28652b729..11b83b8c24a 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -29,7 +29,7 @@ then
 		#		> lib-gpg/ownertrust
 		mkdir ./gpghome &&
 		chmod 0700 ./gpghome &&
-		GNUPGHOME="$(pwd)/gpghome" &&
+		GNUPGHOME="$PWD/gpghome" &&
 		export GNUPGHOME &&
 		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
 		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
-- 
gitgitgadget

[PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

It makes no sense to call `./lib-gpg.sh`. Therefore the hash-bang line
is unnecessary.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/lib-gpg.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 11b83b8c24a..4ead1268351 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -1,5 +1,3 @@
-#!/bin/sh
-
 gpg_version=$(gpg --version 2>&1)
 if test $? != 127
 then
-- 
gitgitgadget

[PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The code to set those prereqs is executed completely outside of any
`test_eval_` block. As a consequence, its output had to be suppressed so
that it does not clutter the output of a regular test script run.

Unfortunately, the output *stays* suppressed even when the `--verbose`
option is in effect.

This hid important output when debugging why the GPG prereq was not
enabled in the Windows part of our CI builds.

In preparation for fixing that, let's move all of this code into lazy
prereqs.

The only slightly tricky part is the global environment variable
`GNUPGHOME`. Originally, it was configured only when we verified that
there is a `gpg` in the `PATH` that we can use. This is now no longer
possible, as lazy prereqs are evaluated in a subshell that changes the
working directory to a temporary one. Therefore, we simply _always_ set
that environment variable: it does not hurt anything because it does not
indicate the presence of a working GPG.

Side note: it was quite tempting to use a hack that is possible because
we do not validate what is passed to `test_lazy_prereq` (and it is
therefore possible to "break out" of the lazy_prereq subshell:

	test_lazy_prereq GPG '...) && GNUPGHOME=... && (...'

However, this is rather tricksy hobbitses code, and the current patch is
_much_ easier to understand.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/lib-gpg.sh | 102 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 57 insertions(+), 45 deletions(-)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 4ead1268351..7a78c562e8d 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -1,12 +1,25 @@
-gpg_version=$(gpg --version 2>&1)
-if test $? != 127
-then
+# We always set GNUPGHOME, even if no usable GPG was found, as
+#
+# - It does not hurt, and
+#
+# - we cannot set global environment variables in lazy prereqs because they are
+#   executed in an eval'ed subshell that changes the working directory to a
+#   temporary one.
+
+GNUPGHOME="$PWD/gpghome"
+export GNUPGHOME
+
+test_lazy_prereq GPG '
+	gpg_version=$(gpg --version 2>&1)
+	test $? != 127 || exit 1
+
 	# 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"
+		exit 1
 		;;
 	*)
 		# Available key info:
@@ -25,55 +38,54 @@ then
 		# To export ownertrust:
 		#	gpg --homedir /tmp/gpghome --export-ownertrust \
 		#		> lib-gpg/ownertrust
-		mkdir ./gpghome &&
-		chmod 0700 ./gpghome &&
-		GNUPGHOME="$PWD/gpghome" &&
-		export GNUPGHOME &&
+		mkdir "$GNUPGHOME" &&
+		chmod 0700 "$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 committer@example.com &&
-		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 "committer@example.com"
-		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 committer@example.com -o /dev/null --sign - 2>&1 &&
-		test_set_prereq GPGSM
+			--sign -u committer@example.com
 		;;
 	esac
-fi
+'
+
+test_lazy_prereq GPGSM '
+	test_have_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 "committer@example.com"
+       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 committer@example.com -o /dev/null --sign - 2>&1
+'
 
-if test_have_prereq GPG &&
-    echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1
-then
-	test_set_prereq RFC1991
-fi
+test_lazy_prereq RFC1991 '
+	test_have_prereq GPG &&
+	echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1
+'
 
 sanitize_pgp() {
 	perl -ne '
-- 
gitgitgadget

[PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The `test_expect_*` functions use `test_eval_` and so does
`test_run_lazy_prereq_`. If tracing is enabled via the `-x` option,
`test_eval_` turns on tracing while evaluating the code block, and turns
it off directly after it.

This is unwanted for nested invocations.

One somewhat surprising example of this is when running a test that
calls `test_i18ngrep`: that function requires the `C_LOCALE_OUTPUT`
prereq, and that prereq is a lazy one, so it is evaluated via
`test_eval_`, the command tracing is turned off, and the test case
continues to run _without tracing the commands_.

Another somewhat surprising example is when one lazy prereq depends on
another lazy prereq: the former will call `test_have_prereq` with the
latter one, which in turn calls `test_eval_` and -- you guessed it --
tracing (if enabled) will be turned off _before_ returning to evaluating
the other lazy prereq.

As we will introduce just such a scenario with the GPG, GPGSM and
RFC1991 prereqs, let's fix that by introducing a variable that keeps
track of the current trace level: nested `test_eval_` calls will
increment and then decrement the level, and only when it reaches 0, the
tracing will _actually_ be turned off.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t0000-basic.sh | 13 +++++++++++++
 t/test-lib.sh    |  6 ++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 3e440c078d5..b8597216200 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -833,6 +833,19 @@ then
 	exit 1
 fi
 
+test_expect_success 'lazy prereqs do not turn off tracing' "
+	run_sub_test_lib_test lazy-prereq-and-tracing \
+		'lazy prereqs and -x' -v -x <<-\\EOF &&
+	test_lazy_prereq LAZY true
+
+	test_expect_success lazy 'test_have_prereq LAZY && echo trace'
+
+	test_done
+	EOF
+
+	grep 'echo trace' lazy-prereq-and-tracing/err
+"
+
 test_expect_success 'tests clean up even on failures' "
 	run_sub_test_lib_test_err \
 		failing-cleanup 'Failing tests with cleanup commands' <<-\\EOF &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0ea1e5a05ed..dbf25348106 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -882,6 +882,7 @@ maybe_setup_valgrind () {
 	fi
 }
 
+_trace_level=0
 want_trace () {
 	test "$trace" = t && {
 		test "$verbose" = t || test "$verbose_log" = t
@@ -895,7 +896,7 @@ want_trace () {
 test_eval_inner_ () {
 	# Do not add anything extra (including LF) after '$*'
 	eval "
-		want_trace && set -x
+		want_trace && _trace_level=$(($_trace_level+1)) && set -x
 		$*"
 }
 
@@ -926,7 +927,8 @@ test_eval_ () {
 		test_eval_ret_=$?
 		if want_trace
 		then
-			set +x
+			test 1 = $_trace_level && set +x
+			_trace_level=$(($_trace_level-1))
 		fi
 	} 2>/dev/null 4>&2
 
-- 
gitgitgadget

[PATCH v2 5/5] tests: increase the verbosity of the GPG-related prereqs

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 no longer redirecting the output not to `/dev/null`.
This is now possible because the affected prereqs were turned into lazy
ones (and are therefore evaluated via `test_eval_` which respects the
`--verbose` option).

Note that we _still_ redirect `stdout` to `/dev/null` for those commands
that sign their `stdin`, as the output would be binary (and useless
anyway, because the reader would not have anything against which to
compare the output).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/lib-gpg.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 7a78c562e8d..9fc5241228e 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -40,12 +40,12 @@ test_lazy_prereq GPG '
 		#		> lib-gpg/ownertrust
 		mkdir "$GNUPGHOME" &&
 		chmod 0700 "$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 \
+		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null \
 			--sign -u committer@example.com
 		;;
 	esac
@@ -68,23 +68,23 @@ test_lazy_prereq GPGSM '
 	#	gpgsm --homedir /tmp/gpghome/ \
 	#		-o t/lib-gpg/gpgsm_cert.p12 \
 	#		--export-secret-key-p12 "committer@example.com"
-       echo | gpgsm --homedir "${GNUPGHOME}" 2>/dev/null \
-	       --passphrase-fd 0 --pinentry-mode loopback \
-	       --import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
+	echo | gpgsm --homedir "${GNUPGHOME}" \
+		--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 |
+	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}" >/dev/null \
-	       -u committer@example.com -o /dev/null --sign - 2>&1
+	echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
+	echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
+	       -u committer@example.com -o /dev/null --sign -
 '
 
 test_lazy_prereq RFC1991 '
 	test_have_prereq GPG &&
-	echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1
+	echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null
 '
 
 sanitize_pgp() {
-- 
gitgitgadget

Re: [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> The `test_expect_*` functions use `test_eval_` and so does
> `test_run_lazy_prereq_`. If tracing is enabled via the `-x` option,
> `test_eval_` turns on tracing while evaluating the code block, and turns
> it off directly after it.
>
> This is unwanted for nested invocations.

Nice finding.

> As we will introduce just such a scenario with the GPG, GPGSM and
> RFC1991 prereqs, let's fix that by introducing a variable that keeps
> track of the current trace level: nested `test_eval_` calls will
> increment and then decrement the level, and only when it reaches 0, the
> tracing will _actually_ be turned off.

Doesn't this explanation urge us to reorder these patches?  It
sounds to me that it argues to have this step before 3/5.

Other than that, both the explanation and the code look correctly
done.  It looks to me that the surrounding code favors trailing "_"
instead of leading one for private names, so we might want to rename
the variable to $trace_level_ but that is minor.

Thanks.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>  t/t0000-basic.sh | 13 +++++++++++++
>  t/test-lib.sh    |  6 ++++--
>  2 files changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> index 3e440c078d5..b8597216200 100755
> --- a/t/t0000-basic.sh
> +++ b/t/t0000-basic.sh
> @@ -833,6 +833,19 @@ then
>  	exit 1
>  fi
>  
> +test_expect_success 'lazy prereqs do not turn off tracing' "
> +	run_sub_test_lib_test lazy-prereq-and-tracing \
> +		'lazy prereqs and -x' -v -x <<-\\EOF &&
> +	test_lazy_prereq LAZY true
> +
> +	test_expect_success lazy 'test_have_prereq LAZY && echo trace'
> +
> +	test_done
> +	EOF
> +
> +	grep 'echo trace' lazy-prereq-and-tracing/err
> +"
> +
>  test_expect_success 'tests clean up even on failures' "
>  	run_sub_test_lib_test_err \
>  		failing-cleanup 'Failing tests with cleanup commands' <<-\\EOF &&
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 0ea1e5a05ed..dbf25348106 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -882,6 +882,7 @@ maybe_setup_valgrind () {
>  	fi
>  }
>  
> +_trace_level=0
>  want_trace () {
>  	test "$trace" = t && {
>  		test "$verbose" = t || test "$verbose_log" = t
> @@ -895,7 +896,7 @@ want_trace () {
>  test_eval_inner_ () {
>  	# Do not add anything extra (including LF) after '$*'
>  	eval "
> -		want_trace && set -x
> +		want_trace && _trace_level=$(($_trace_level+1)) && set -x
>  		$*"
>  }
>  
> @@ -926,7 +927,8 @@ test_eval_ () {
>  		test_eval_ret_=$?
>  		if want_trace
>  		then
> -			set +x
> +			test 1 = $_trace_level && set +x
> +			_trace_level=$(($_trace_level-1))
>  		fi
>  	} 2>/dev/null 4>&2

Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs

[Junio C Hamano]

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> -if test_have_prereq GPG &&
> -    echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1
> -then
> -	test_set_prereq RFC1991
> -fi
> +test_lazy_prereq RFC1991 '
> +	test_have_prereq GPG &&
> +	echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1
> +'

OK.  To make it fully lazy, we do need to test GPG while lazily
checking for RFC1991.  Makes sense.

Thanks.

Re: [PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script

[Jeff King]

On Wed, Mar 25, 2020 at 05:41:18AM +0000, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> It makes no sense to call `./lib-gpg.sh`. Therefore the hash-bang line
> is unnecessary.

I wondered if it might be useful to identify the file for editors, etc.
But "*.sh" is quite sufficient, and we already stripped most of these
out long ago in b54a31243b (t/lib-*.sh: drop executable bit,
2020-03-26).

There's some other related cleanup. I'll prepare a separate series.

-Peff

Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs

[Jeff King]

On Wed, Mar 25, 2020 at 05:41:19AM +0000, Johannes Schindelin via GitGitGadget wrote:

> In preparation for fixing that, let's move all of this code into lazy
> prereqs.

OK. This looks good, even if I cannot help feel that my earlier patch
was perfectly sufficient. ;)

> Side note: it was quite tempting to use a hack that is possible because
> we do not validate what is passed to `test_lazy_prereq` (and it is
> therefore possible to "break out" of the lazy_prereq subshell:
> 
> 	test_lazy_prereq GPG '...) && GNUPGHOME=... && (...'

No, it is not tempting at all to me to do something so gross. :)

> +test_lazy_prereq GPG '
> +	gpg_version=$(gpg --version 2>&1)

One thing I observed while doing my patch is that lazy_prereq blocks do
not get run through the &&-chain linter. So this is OK, but I wonder if
we should be future-proofing with braces. I don't care _too_ much either
way, though.

> +	test $? != 127 || exit 1

I have a slight preference for "return 1" here. The "exit 1" works
because test_lazy_prereq puts us in an implicit subshell. But I think
this sets a bad example for people writing regular tests, where there is
no such subshell (and "return 1" is the only correct way to do it).

>  	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"
> +		exit 1

Ditto here.

> @@ -25,55 +38,54 @@ then
>  		# To export ownertrust:
>  		#	gpg --homedir /tmp/gpghome --export-ownertrust \
>  		#		> lib-gpg/ownertrust
> -		mkdir ./gpghome &&
> -		chmod 0700 ./gpghome &&
> -		GNUPGHOME="$PWD/gpghome" &&
> -		export GNUPGHOME &&
> +		mkdir "$GNUPGHOME" &&
> +		chmod 0700 "$GNUPGHOME" &&

Compared to mine this keeps the mkdir in the prereq. That seems fine to
me. Other prereqs do depend on the directory existing, but they all
depend on GPG itself, so they'd be fine.

> +test_lazy_prereq GPGSM '
> +	test_have_prereq GPG &&

In mine I put the test_have_prereq outside the lazy prereq. I don't
think it really matters either way (when we later ask if GPGSM is set,
there is no difference between nobody having defined it, and having a
lazy definition that said "no").

-Peff

Re: [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing

[Jeff King]

On Wed, Mar 25, 2020 at 05:41:20AM +0000, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> The `test_expect_*` functions use `test_eval_` and so does
> `test_run_lazy_prereq_`. If tracing is enabled via the `-x` option,
> `test_eval_` turns on tracing while evaluating the code block, and turns
> it off directly after it.
> 
> This is unwanted for nested invocations.
> 
> One somewhat surprising example of this is when running a test that
> calls `test_i18ngrep`: that function requires the `C_LOCALE_OUTPUT`
> prereq, and that prereq is a lazy one, so it is evaluated via
> `test_eval_`, the command tracing is turned off, and the test case
> continues to run _without tracing the commands_.

Good catch. I didn't see this when looking at the GPG stuff earlier
because I didn't nest the lazy prereqs. But it is worth fixing
regardless, because as you note it comes up in other places.

> @@ -926,7 +927,8 @@ test_eval_ () {
>  		test_eval_ret_=$?
>  		if want_trace
>  		then
> -			set +x
> +			test 1 = $_trace_level && set +x
> +			_trace_level=$(($_trace_level-1))
>  		fi
>  	} 2>/dev/null 4>&2

I briefly wondered if adding more logic here might upset our delicate
balance of avoiding writing cruft to the "-x" output. But the answer is
"no", due to the descriptor hackery at the end of that {} block.

Of course, any test evaluating a lazy prereq already gets tons of cruft
anyway, because the outer level of tracing sees all of our internal
function calls checking the prereq and setting up the inner level of
tracing. But there's not much we can do about that.

-Peff

Re: [PATCH v2 5/5] tests: increase the verbosity of the GPG-related prereqs

[Jeff King]

On Wed, Mar 25, 2020 at 05:41:21AM +0000, Johannes Schindelin via GitGitGadget wrote:

> Let's fix this by no longer redirecting the output not to `/dev/null`.
> This is now possible because the affected prereqs were turned into lazy
> ones (and are therefore evaluated via `test_eval_` which respects the
> `--verbose` option).
> 
> Note that we _still_ redirect `stdout` to `/dev/null` for those commands
> that sign their `stdin`, as the output would be binary (and useless
> anyway, because the reader would not have anything against which to
> compare the output).

This looks good. You could drop the redirection of stdin in one case,
too (since test_eval_ already does so) but I don't mind leaving it as
documentation.

-Peff

Re: [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3491 bytes --]

Hi Junio,

On Wed, 25 Mar 2020, Junio C Hamano wrote:

> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > The `test_expect_*` functions use `test_eval_` and so does
> > `test_run_lazy_prereq_`. If tracing is enabled via the `-x` option,
> > `test_eval_` turns on tracing while evaluating the code block, and turns
> > it off directly after it.
> >
> > This is unwanted for nested invocations.
>
> Nice finding.

Heh. I found this ages ago, and mistook it for "all prereqs turn off
tracing" when I reported it, but Gábor pointed out that that is incorrect.

That was a long time ago, maybe even a year, and I finally used this patch
series as an excuse to finally dig deep on this.

> > As we will introduce just such a scenario with the GPG, GPGSM and
> > RFC1991 prereqs, let's fix that by introducing a variable that keeps
> > track of the current trace level: nested `test_eval_` calls will
> > increment and then decrement the level, and only when it reaches 0, the
> > tracing will _actually_ be turned off.
>
> Doesn't this explanation urge us to reorder these patches?  It
> sounds to me that it argues to have this step before 3/5.

Yes, that's where I had meant to put this patch. Sorry for the confusion.

> Other than that, both the explanation and the code look correctly
> done.  It looks to me that the surrounding code favors trailing "_"
> instead of leading one for private names, so we might want to rename
> the variable to $trace_level_ but that is minor.

Good point, I had missed that. Will be fixed in the next iteration.

Ciao,
Dscho

>
> Thanks.
>
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  t/t0000-basic.sh | 13 +++++++++++++
> >  t/test-lib.sh    |  6 ++++--
> >  2 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
> > index 3e440c078d5..b8597216200 100755
> > --- a/t/t0000-basic.sh
> > +++ b/t/t0000-basic.sh
> > @@ -833,6 +833,19 @@ then
> >  	exit 1
> >  fi
> >
> > +test_expect_success 'lazy prereqs do not turn off tracing' "
> > +	run_sub_test_lib_test lazy-prereq-and-tracing \
> > +		'lazy prereqs and -x' -v -x <<-\\EOF &&
> > +	test_lazy_prereq LAZY true
> > +
> > +	test_expect_success lazy 'test_have_prereq LAZY && echo trace'
> > +
> > +	test_done
> > +	EOF
> > +
> > +	grep 'echo trace' lazy-prereq-and-tracing/err
> > +"
> > +
> >  test_expect_success 'tests clean up even on failures' "
> >  	run_sub_test_lib_test_err \
> >  		failing-cleanup 'Failing tests with cleanup commands' <<-\\EOF &&
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > index 0ea1e5a05ed..dbf25348106 100644
> > --- a/t/test-lib.sh
> > +++ b/t/test-lib.sh
> > @@ -882,6 +882,7 @@ maybe_setup_valgrind () {
> >  	fi
> >  }
> >
> > +_trace_level=0
> >  want_trace () {
> >  	test "$trace" = t && {
> >  		test "$verbose" = t || test "$verbose_log" = t
> > @@ -895,7 +896,7 @@ want_trace () {
> >  test_eval_inner_ () {
> >  	# Do not add anything extra (including LF) after '$*'
> >  	eval "
> > -		want_trace && set -x
> > +		want_trace && _trace_level=$(($_trace_level+1)) && set -x
> >  		$*"
> >  }
> >
> > @@ -926,7 +927,8 @@ test_eval_ () {
> >  		test_eval_ret_=$?
> >  		if want_trace
> >  		then
> > -			set +x
> > +			test 1 = $_trace_level && set +x
> > +			_trace_level=$(($_trace_level-1))
> >  		fi
> >  	} 2>/dev/null 4>&2
>

Re: [PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script

[Johannes Schindelin]

Hi Peff,

On Thu, 26 Mar 2020, Jeff King wrote:

> On Wed, Mar 25, 2020 at 05:41:18AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > It makes no sense to call `./lib-gpg.sh`. Therefore the hash-bang line
> > is unnecessary.
>
> I wondered if it might be useful to identify the file for editors, etc.
> But "*.sh" is quite sufficient, and we already stripped most of these
> out long ago in b54a31243b (t/lib-*.sh: drop executable bit,
> 2020-03-26).
>
> There's some other related cleanup. I'll prepare a separate series.

I noticed that, but forgot to add a note about this to the commit message.
Sorry. The next iteration of this patch series will have that note.

$ git grep '#! */' t/*.sh | grep -v 't[0-9]'
t/aggregate-results.sh:#!/bin/sh
t/gitweb-lib.sh:#!/usr/bin/perl
t/lib-credential.sh:#!/bin/sh

Thank you for volunteering to clean them up!

Ciao,
Dscho

Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs

[Johannes Schindelin]

Hi Peff,

On Thu, 26 Mar 2020, Jeff King wrote:

> On Wed, Mar 25, 2020 at 05:41:19AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > In preparation for fixing that, let's move all of this code into lazy
> > prereqs.
>
> OK. This looks good, even if I cannot help feel that my earlier patch
> was perfectly sufficient. ;)

The mistake is all mine. I had totally missed that you turned GPG into a
lazy prereq. So I had my patch finalized already before you pointed my
nose at that fact.

Sorry about that.

> > Side note: it was quite tempting to use a hack that is possible because
> > we do not validate what is passed to `test_lazy_prereq` (and it is
> > therefore possible to "break out" of the lazy_prereq subshell:
> >
> > 	test_lazy_prereq GPG '...) && GNUPGHOME=... && (...'
>
> No, it is not tempting at all to me to do something so gross. :)

Well, maybe it was not tempting to _you_, but to _me_, it was so tempting
that I had implemented and committed it before I made up my mind and
changed it again.

> > +test_lazy_prereq GPG '
> > +	gpg_version=$(gpg --version 2>&1)
>
> One thing I observed while doing my patch is that lazy_prereq blocks do
> not get run through the &&-chain linter. So this is OK, but I wonder if
> we should be future-proofing with braces. I don't care _too_ much either
> way, though.

I actually like that the prereq blocks are exempt from this && chain
linting, and would like to refrain from adding braces "just because".

> > +	test $? != 127 || exit 1
>
> I have a slight preference for "return 1" here. The "exit 1" works
> because test_lazy_prereq puts us in an implicit subshell. But I think
> this sets a bad example for people writing regular tests, where there is
> no such subshell (and "return 1" is the only correct way to do it).

There are two reasons why I did not use `return` here:

- To me, prereq code is very distinct from writing tests. Just like we do
  not &&-chain all the shell functions that live outside of tests, I don't
  want to &&-chain all the prereq code.

  The point of the tests' commands is not to fail. The point of prereq's
  code is to fail if the prereq is not met.

- Since this code is outside of a function, `return` felt like the wrong
  semantic concept to me. And indeed, I see this (rather scary) part in
  Bash's documentation of `return` (I did not even bother to look at the
  POSIX semantics, it scared me so much):

      The return status is non-zero if `return` is supplied a non-numeric
      argument, or is used outside a function and not during execution of
      a script by `.` or `source`.

  So: the `1` is totally ignored in this context. That alone is reason
  enough for me to completely avoid it, and use `exit` instead.

> >  	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"
> > +		exit 1
>
> Ditto here.
>
> > @@ -25,55 +38,54 @@ then
> >  		# To export ownertrust:
> >  		#	gpg --homedir /tmp/gpghome --export-ownertrust \
> >  		#		> lib-gpg/ownertrust
> > -		mkdir ./gpghome &&
> > -		chmod 0700 ./gpghome &&
> > -		GNUPGHOME="$PWD/gpghome" &&
> > -		export GNUPGHOME &&
> > +		mkdir "$GNUPGHOME" &&
> > +		chmod 0700 "$GNUPGHOME" &&
>
> Compared to mine this keeps the mkdir in the prereq. That seems fine to
> me. Other prereqs do depend on the directory existing, but they all
> depend on GPG itself, so they'd be fine.

Yes. And conceptually, I like that the prereq is responsible for creating
that directory.

> > +test_lazy_prereq GPGSM '
> > +	test_have_prereq GPG &&
>
> In mine I put the test_have_prereq outside the lazy prereq.

That makes it essentially a non-lazy prereq.

> I don't think it really matters either way (when we later ask if GPGSM
> is set, there is no difference between nobody having defined it, and
> having a lazy definition that said "no").

The difference is when running a test with `--run=<n>` where `<n>` refers
to a test case that requires neither GPG nor GPGSM or RFC1991. My version
will not evaluate the GPG prereq, yours still will.

Ciao,
Dscho

Re: [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing

[Johannes Schindelin]

Hi Peff,

On Thu, 26 Mar 2020, Jeff King wrote:

> On Wed, Mar 25, 2020 at 05:41:20AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The `test_expect_*` functions use `test_eval_` and so does
> > `test_run_lazy_prereq_`. If tracing is enabled via the `-x` option,
> > `test_eval_` turns on tracing while evaluating the code block, and turns
> > it off directly after it.
> >
> > This is unwanted for nested invocations.
> >
> > One somewhat surprising example of this is when running a test that
> > calls `test_i18ngrep`: that function requires the `C_LOCALE_OUTPUT`
> > prereq, and that prereq is a lazy one, so it is evaluated via
> > `test_eval_`, the command tracing is turned off, and the test case
> > continues to run _without tracing the commands_.
>
> Good catch. I didn't see this when looking at the GPG stuff earlier
> because I didn't nest the lazy prereqs. But it is worth fixing
> regardless, because as you note it comes up in other places.
>
> > @@ -926,7 +927,8 @@ test_eval_ () {
> >  		test_eval_ret_=$?
> >  		if want_trace
> >  		then
> > -			set +x
> > +			test 1 = $_trace_level && set +x
> > +			_trace_level=$(($_trace_level-1))
> >  		fi
> >  	} 2>/dev/null 4>&2
>
> I briefly wondered if adding more logic here might upset our delicate
> balance of avoiding writing cruft to the "-x" output. But the answer is
> "no", due to the descriptor hackery at the end of that {} block.
>
> Of course, any test evaluating a lazy prereq already gets tons of cruft
> anyway, because the outer level of tracing sees all of our internal
> function calls checking the prereq and setting up the inner level of
> tracing. But there's not much we can do about that.

We could turn off the tracing specifically in those cases. At some point,
though, it strikes me as rather ridiculous through how many hoops we jump
just because our test suite framework is implemented in portable Unix
shell script, as opposed to a more powerful language such as, say, C.

For example, we could prevent the `test_eval_ret_=$?` line from being
traced by default, simply by redirecting fd `4` to `/dev/null`. At that
stage, we would of course have to open yet another fd to support the use
case where the `-x` is passed to `sh` itself (`sh -x t0000-*.sh -i -v`).

In the context of this here patch series, which really is about enabling
the GPG-related tests on Windows, I will refrain from going down that
particular rabbit hole. My ego might get stuck.

Ciao,
Dscho

Re: [PATCH v2 5/5] tests: increase the verbosity of the GPG-related prereqs

[Johannes Schindelin]

Hi Peff,

On Thu, 26 Mar 2020, Jeff King wrote:

> On Wed, Mar 25, 2020 at 05:41:21AM +0000, Johannes Schindelin via GitGitGadget wrote:
>
> > Let's fix this by no longer redirecting the output not to `/dev/null`.
> > This is now possible because the affected prereqs were turned into lazy
> > ones (and are therefore evaluated via `test_eval_` which respects the
> > `--verbose` option).
> >
> > Note that we _still_ redirect `stdout` to `/dev/null` for those commands
> > that sign their `stdin`, as the output would be binary (and useless
> > anyway, because the reader would not have anything against which to
> > compare the output).
>
> This looks good. You could drop the redirection of stdin in one case,
> too (since test_eval_ already does so) but I don't mind leaving it as
> documentation.

I considered that, but decided that I'd rather save myself the brain
cycles in the future when reading that code (I would ask myself "*what* is
signed here?").

That's why I left the `</dev/null` in.

Also, I could imagine that there might be some unexpected interaction with
`GIT_DEBUGGER` if I removed that `</dev/null`, and I'd just like to stay
on the safe side here.

Ciao,
Dscho

[PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds

[Johannes Schindelin via GitGitGadget]

While debugging the breakages introduced by hi/gpg-prefer-check-signature, I
noticed that the GPG prereq was not available on Windows, even if Git for
Windows' SDK comes with a fully functional GPG2.

The fix was easy, but finding out what was going on was not, so for good
measure, the fix is accompanied by a patch that will hopefully make future
investigations into GPG-related problems much, much easier.

Changes since v2:

 * Reordered 4/5 before 3/5, as I had intended originally.
   
   
 * Renamed _trace_level to have a trailing underscore, in line with the
   surrounding code.
   
   
 * Added a note to the commit message why only lib-gpg.sh loses its
   hash-bang line, and no other files in t/.
   
   

Changes since v1:

 * The prereqs are now lazy ones.
   
   
 * A new patch was introduced to make tracing via -x work even with those
   inter-dependent prereqs.
   
   
 * The test-signing's stdout is redirected to /dev/null because it is
   unreadable and unhelpful binary gibberish, anyway. (This imitates Peff's
   patch.)

Johannes Schindelin (5):
  tests(gpg): allow the gpg-agent to start on Windows
  t/lib-gpg.sh: stop pretending to be a stand-alone script
  tests: do not let lazy prereqs inside `test_expect_*` turn off tracing
  tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
  tests: increase the verbosity of the GPG-related prereqs

 t/lib-gpg.sh     | 110 ++++++++++++++++++++++++++---------------------
 t/t0000-basic.sh |  13 ++++++
 t/test-lib.sh    |   6 ++-
 3 files changed, 77 insertions(+), 52 deletions(-)


base-commit: 30e9940356dc67959877f4b2417da33ebdefbb79
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-728%2Fdscho%2Fci-windows-gpg-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-728/dscho/ci-windows-gpg-v3
Pull-Request: https://github.com/git/git/pull/728

Range-diff vs v2:

 1:  287a21f1033 = 1:  287a21f1033 tests(gpg): allow the gpg-agent to start on Windows
 2:  c1811d54190 ! 2:  b4217c36070 t/lib-gpg.sh: stop pretending to be a stand-alone script
     @@ -5,6 +5,10 @@
          It makes no sense to call `./lib-gpg.sh`. Therefore the hash-bang line
          is unnecessary.
      
     +    There are other similar instances in `t/`, but they are too far from the
     +    context of the enclosing patch series, so they will be addressed
     +    separately.
     +
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
 4:  0767c8b77c8 ! 3:  f35830c0eba tests: do not let lazy prereqs inside `test_expect_*` turn off tracing
     @@ -60,7 +60,7 @@
       	fi
       }
       
     -+_trace_level=0
     ++trace_level_=0
       want_trace () {
       	test "$trace" = t && {
       		test "$verbose" = t || test "$verbose_log" = t
     @@ -69,7 +69,7 @@
       	# Do not add anything extra (including LF) after '$*'
       	eval "
      -		want_trace && set -x
     -+		want_trace && _trace_level=$(($_trace_level+1)) && set -x
     ++		want_trace && trace_level_=$(($trace_level_+1)) && set -x
       		$*"
       }
       
     @@ -78,8 +78,8 @@
       		if want_trace
       		then
      -			set +x
     -+			test 1 = $_trace_level && set +x
     -+			_trace_level=$(($_trace_level-1))
     ++			test 1 = $trace_level_ && set +x
     ++			trace_level_=$(($trace_level_-1))
       		fi
       	} 2>/dev/null 4>&2
       
 3:  85457a7b618 = 4:  f69f97e24ba tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
 5:  5e89b512513 = 5:  064f4e541b8 tests: increase the verbosity of the GPG-related prereqs

-- 
gitgitgadget

[PATCH v3 1/5] tests(gpg): allow the gpg-agent to start on Windows

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In Git for Windows' SDK, we use the MSYS2 version of OpenSSH, meaning
that the `gpg-agent` will fail horribly when being passed a `--homedir`
that contains colons.

Previously, we did pass the Windows version of the absolute path,
though, which starts in the drive letter followed by, you guessed it, a
colon.

Let's use the same trick found elsewhere in our test suite where `$PWD`
is used to refer to the pseudo-Unix path (which works only within the
MSYS2 Bash/OpenSSH/Perl/etc, as opposed to `$(pwd)` which refers to the
Windows path that `git.exe` understands, too).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/lib-gpg.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 8d28652b729..11b83b8c24a 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -29,7 +29,7 @@ then
 		#		> lib-gpg/ownertrust
 		mkdir ./gpghome &&
 		chmod 0700 ./gpghome &&
-		GNUPGHOME="$(pwd)/gpghome" &&
+		GNUPGHOME="$PWD/gpghome" &&
 		export GNUPGHOME &&
 		(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&
 		gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \
-- 
gitgitgadget

[PATCH v3 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

It makes no sense to call `./lib-gpg.sh`. Therefore the hash-bang line
is unnecessary.

There are other similar instances in `t/`, but they are too far from the
context of the enclosing patch series, so they will be addressed
separately.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/lib-gpg.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 11b83b8c24a..4ead1268351 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -1,5 +1,3 @@
-#!/bin/sh
-
 gpg_version=$(gpg --version 2>&1)
 if test $? != 127
 then
-- 
gitgitgadget

[PATCH v3 3/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The `test_expect_*` functions use `test_eval_` and so does
`test_run_lazy_prereq_`. If tracing is enabled via the `-x` option,
`test_eval_` turns on tracing while evaluating the code block, and turns
it off directly after it.

This is unwanted for nested invocations.

One somewhat surprising example of this is when running a test that
calls `test_i18ngrep`: that function requires the `C_LOCALE_OUTPUT`
prereq, and that prereq is a lazy one, so it is evaluated via
`test_eval_`, the command tracing is turned off, and the test case
continues to run _without tracing the commands_.

Another somewhat surprising example is when one lazy prereq depends on
another lazy prereq: the former will call `test_have_prereq` with the
latter one, which in turn calls `test_eval_` and -- you guessed it --
tracing (if enabled) will be turned off _before_ returning to evaluating
the other lazy prereq.

As we will introduce just such a scenario with the GPG, GPGSM and
RFC1991 prereqs, let's fix that by introducing a variable that keeps
track of the current trace level: nested `test_eval_` calls will
increment and then decrement the level, and only when it reaches 0, the
tracing will _actually_ be turned off.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t0000-basic.sh | 13 +++++++++++++
 t/test-lib.sh    |  6 ++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index 3e440c078d5..b8597216200 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -833,6 +833,19 @@ then
 	exit 1
 fi
 
+test_expect_success 'lazy prereqs do not turn off tracing' "
+	run_sub_test_lib_test lazy-prereq-and-tracing \
+		'lazy prereqs and -x' -v -x <<-\\EOF &&
+	test_lazy_prereq LAZY true
+
+	test_expect_success lazy 'test_have_prereq LAZY && echo trace'
+
+	test_done
+	EOF
+
+	grep 'echo trace' lazy-prereq-and-tracing/err
+"
+
 test_expect_success 'tests clean up even on failures' "
 	run_sub_test_lib_test_err \
 		failing-cleanup 'Failing tests with cleanup commands' <<-\\EOF &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0ea1e5a05ed..529056be497 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -882,6 +882,7 @@ maybe_setup_valgrind () {
 	fi
 }
 
+trace_level_=0
 want_trace () {
 	test "$trace" = t && {
 		test "$verbose" = t || test "$verbose_log" = t
@@ -895,7 +896,7 @@ want_trace () {
 test_eval_inner_ () {
 	# Do not add anything extra (including LF) after '$*'
 	eval "
-		want_trace && set -x
+		want_trace && trace_level_=$(($trace_level_+1)) && set -x
 		$*"
 }
 
@@ -926,7 +927,8 @@ test_eval_ () {
 		test_eval_ret_=$?
 		if want_trace
 		then
-			set +x
+			test 1 = $trace_level_ && set +x
+			trace_level_=$(($trace_level_-1))
 		fi
 	} 2>/dev/null 4>&2
 
-- 
gitgitgadget

[PATCH v3 4/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

The code to set those prereqs is executed completely outside of any
`test_eval_` block. As a consequence, its output had to be suppressed so
that it does not clutter the output of a regular test script run.

Unfortunately, the output *stays* suppressed even when the `--verbose`
option is in effect.

This hid important output when debugging why the GPG prereq was not
enabled in the Windows part of our CI builds.

In preparation for fixing that, let's move all of this code into lazy
prereqs.

The only slightly tricky part is the global environment variable
`GNUPGHOME`. Originally, it was configured only when we verified that
there is a `gpg` in the `PATH` that we can use. This is now no longer
possible, as lazy prereqs are evaluated in a subshell that changes the
working directory to a temporary one. Therefore, we simply _always_ set
that environment variable: it does not hurt anything because it does not
indicate the presence of a working GPG.

Side note: it was quite tempting to use a hack that is possible because
we do not validate what is passed to `test_lazy_prereq` (and it is
therefore possible to "break out" of the lazy_prereq subshell:

	test_lazy_prereq GPG '...) && GNUPGHOME=... && (...'

However, this is rather tricksy hobbitses code, and the current patch is
_much_ easier to understand.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/lib-gpg.sh | 102 ++++++++++++++++++++++++++++-----------------------
 1 file changed, 57 insertions(+), 45 deletions(-)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 4ead1268351..7a78c562e8d 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -1,12 +1,25 @@
-gpg_version=$(gpg --version 2>&1)
-if test $? != 127
-then
+# We always set GNUPGHOME, even if no usable GPG was found, as
+#
+# - It does not hurt, and
+#
+# - we cannot set global environment variables in lazy prereqs because they are
+#   executed in an eval'ed subshell that changes the working directory to a
+#   temporary one.
+
+GNUPGHOME="$PWD/gpghome"
+export GNUPGHOME
+
+test_lazy_prereq GPG '
+	gpg_version=$(gpg --version 2>&1)
+	test $? != 127 || exit 1
+
 	# 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"
+		exit 1
 		;;
 	*)
 		# Available key info:
@@ -25,55 +38,54 @@ then
 		# To export ownertrust:
 		#	gpg --homedir /tmp/gpghome --export-ownertrust \
 		#		> lib-gpg/ownertrust
-		mkdir ./gpghome &&
-		chmod 0700 ./gpghome &&
-		GNUPGHOME="$PWD/gpghome" &&
-		export GNUPGHOME &&
+		mkdir "$GNUPGHOME" &&
+		chmod 0700 "$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 committer@example.com &&
-		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 "committer@example.com"
-		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 committer@example.com -o /dev/null --sign - 2>&1 &&
-		test_set_prereq GPGSM
+			--sign -u committer@example.com
 		;;
 	esac
-fi
+'
+
+test_lazy_prereq GPGSM '
+	test_have_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 "committer@example.com"
+       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 committer@example.com -o /dev/null --sign - 2>&1
+'
 
-if test_have_prereq GPG &&
-    echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1
-then
-	test_set_prereq RFC1991
-fi
+test_lazy_prereq RFC1991 '
+	test_have_prereq GPG &&
+	echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1
+'
 
 sanitize_pgp() {
 	perl -ne '
-- 
gitgitgadget

[PATCH v3 5/5] tests: increase the verbosity of the GPG-related prereqs

[Johannes Schindelin via GitGitGadget]

From: Johannes Schindelin <johannes.schindelin@gmx.de>

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 no longer redirecting the output not to `/dev/null`.
This is now possible because the affected prereqs were turned into lazy
ones (and are therefore evaluated via `test_eval_` which respects the
`--verbose` option).

Note that we _still_ redirect `stdout` to `/dev/null` for those commands
that sign their `stdin`, as the output would be binary (and useless
anyway, because the reader would not have anything against which to
compare the output).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/lib-gpg.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh
index 7a78c562e8d..9fc5241228e 100755
--- a/t/lib-gpg.sh
+++ b/t/lib-gpg.sh
@@ -40,12 +40,12 @@ test_lazy_prereq GPG '
 		#		> lib-gpg/ownertrust
 		mkdir "$GNUPGHOME" &&
 		chmod 0700 "$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 \
+		gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null \
 			--sign -u committer@example.com
 		;;
 	esac
@@ -68,23 +68,23 @@ test_lazy_prereq GPGSM '
 	#	gpgsm --homedir /tmp/gpghome/ \
 	#		-o t/lib-gpg/gpgsm_cert.p12 \
 	#		--export-secret-key-p12 "committer@example.com"
-       echo | gpgsm --homedir "${GNUPGHOME}" 2>/dev/null \
-	       --passphrase-fd 0 --pinentry-mode loopback \
-	       --import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 &&
+	echo | gpgsm --homedir "${GNUPGHOME}" \
+		--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 |
+	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}" >/dev/null \
-	       -u committer@example.com -o /dev/null --sign - 2>&1
+	echo " S relax" >>"${GNUPGHOME}/trustlist.txt" &&
+	echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \
+	       -u committer@example.com -o /dev/null --sign -
 '
 
 test_lazy_prereq RFC1991 '
 	test_have_prereq GPG &&
-	echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1
+	echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null
 '
 
 sanitize_pgp() {
-- 
gitgitgadget

Re: [PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I wondered if it might be useful to identify the file for editors, etc.
> But "*.sh" is quite sufficient, and we already stripped most of these
> out long ago in b54a31243b (t/lib-*.sh: drop executable bit,
> 2020-03-26).

That timestamp look too fresh to me ;-)

c74c7203 (test: replace shebangs with descriptions in shell
libraries, 2013-11-25) was described quite well.

Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs

[Jeff King]

On Thu, Mar 26, 2020 at 03:27:19PM +0100, Johannes Schindelin wrote:

> > OK. This looks good, even if I cannot help feel that my earlier patch
> > was perfectly sufficient. ;)
> 
> The mistake is all mine. I had totally missed that you turned GPG into a
> lazy prereq. So I had my patch finalized already before you pointed my
> nose at that fact.
> 
> Sorry about that.

No problem. And I hope my review didn't sound too passive-aggressive
with the "well, in MY version we did this...". I focused on the
differences because those were the parts that were new (and therefore
interesting) to me. But I don't think any of them are too important
either way.

> > I have a slight preference for "return 1" here. The "exit 1" works
> > because test_lazy_prereq puts us in an implicit subshell. But I think
> > this sets a bad example for people writing regular tests, where there is
> > no such subshell (and "return 1" is the only correct way to do it).
> 
> There are two reasons why I did not use `return` here:
> 
> - To me, prereq code is very distinct from writing tests. Just like we do
>   not &&-chain all the shell functions that live outside of tests, I don't
>   want to &&-chain all the prereq code.
> 
>   The point of the tests' commands is not to fail. The point of prereq's
>   code is to fail if the prereq is not met.

My only concern is whether people cargo-culting will notice the
distinction. But it's probably not a big deal.

> - Since this code is outside of a function, `return` felt like the wrong
>   semantic concept to me. And indeed, I see this (rather scary) part in
>   Bash's documentation of `return` (I did not even bother to look at the
>   POSIX semantics, it scared me so much):
> 
>       The return status is non-zero if `return` is supplied a non-numeric
>       argument, or is used outside a function and not during execution of
>       a script by `.` or `source`.
> 
>   So: the `1` is totally ignored in this context. That alone is reason
>   enough for me to completely avoid it, and use `exit` instead.

I agree the portability rules there are scary, but none of that applies
because we _are_ in a function: test_eval_inner_(). This behavior is
intentional and due to a7c58f280a (test: cope better with use of return
for errors, 2011-08-08). I thought we specifically advertised this
feature in t/README, but I can't seem to find it.

So my perspective was the opposite of yours: "return" is the officially
sanctioned way to exit early from a test snippet, and "exit" only
happens to work because of the undocumented fact that lazy prereqs
happen in a subshell. But it turns out neither was documented. :)

> > In mine I put the test_have_prereq outside the lazy prereq.
> 
> That makes it essentially a non-lazy prereq.
> 
> > I don't think it really matters either way (when we later ask if GPGSM
> > is set, there is no difference between nobody having defined it, and
> > having a lazy definition that said "no").
> 
> The difference is when running a test with `--run=<n>` where `<n>` refers
> to a test case that requires neither GPG nor GPGSM or RFC1991. My version
> will not evaluate the GPG prereq, yours still will.

Yes. Part of the reason I kept mine as it was is because it matched the
original behavior better (and I was really only using a lazy prereq
because we didn't have a non-lazy version). But there's really no reason
_not_ to be lazy, so as long as it isn't breaking anything I think
that's a better way forward. (And if it is breaking something, that
something should be fixed).

-Peff

Re: [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds

[Jeff King]

On Thu, Mar 26, 2020 at 03:35:23PM +0000, Johannes Schindelin via GitGitGadget wrote:

> Changes since v2:
> 
>  * Reordered 4/5 before 3/5, as I had intended originally.
>    
>    
>  * Renamed _trace_level to have a trailing underscore, in line with the
>    surrounding code.
>    
>    
>  * Added a note to the commit message why only lib-gpg.sh loses its
>    hash-bang line, and no other files in t/.

Thanks, this version looks fine to me. I left a few other comments
regarding exit/return in the other part of the thread, but frankly all
of it is too arcane and insignificant to spend more brain cycles going
back and forth on.  So if I convinced/inspired you on that point, feel
free to switch it, but otherwise I'm happy with this iteration.

-Peff

Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So my perspective was the opposite of yours: "return" is the officially
> sanctioned way to exit early from a test snippet, and "exit" only
> happens to work because of the undocumented fact that lazy prereqs
> happen in a subshell. But it turns out neither was documented. :)

Good miniproject to document them, I presume.  A rough draft
attached.  I do not mind adding "exit 1 also works in this and that
case, but not in that other case" if the rule can be given succinct
enough, but I opted for simplicity (mostly because I couldn't come
up with such a clear rule for "exit 1").  

As long as we are consciouly ensuring that "return 1" consistently
works everywhere, I didn't see much point offering multiple ways to
do the same thing.

> Yes. Part of the reason I kept mine as it was is because it matched the
> original behavior better (and I was really only using a lazy prereq
> because we didn't have a non-lazy version). But there's really no reason
> _not_ to be lazy, so as long as it isn't breaking anything I think
> that's a better way forward. (And if it is breaking something, that
> something should be fixed).

Yup, I too liked that part of Dscho's version better.

-- >8 --
Subject: [PATCH] t/README: suggest how to leave test early with failure

Over time, we added the support to our test framework to make it
easy to leave a test early with failure, but it was not clearly
documented in t/README to help developers writing new tests.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * A tangent.  441ee35d (t/README: reformat Do, Don't, Keep in mind
   lists, 2018-10-05) added these lines

    Here are the "do's:"
    And here are the "don'ts:"

   Is the placement of the colons on these lines right?  I am
   tempted to chase them out of the dq pair and move them at the end
   of their lines, but obviously that is outside of the scope of
   this patch.

 t/README | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/t/README b/t/README
index 369e3a9ded..08c8d00bb6 100644
--- a/t/README
+++ b/t/README
@@ -546,6 +546,61 @@ Here are the "do's:"
    reports "ok" or "not ok" to the end user running the tests. Under
    --verbose, they are shown to help debug the tests.
 
+ - Be careful when you loop
+
+   You may need to test multiple things in a loop, but the following
+   does not work correctly:
+
+	test_expect_success 'git frotz on various versions' '
+	    for revision in one two three
+	    do
+		    echo "frotz $revision" >expect &&
+		    git frotz "$revision" >actual &&
+		    test_cmp expect actual
+	    done &&
+	    test something else
+	'
+
+   If the output from the "git frotz" command did not match what is
+   expected for 'one' and 'two', but it did for 'three', the loop
+   itself would not fail, and the test goes on happily.  This is not
+   what you want.
+
+   You can work it around in two ways.  You could use a separate
+   "flag" variable to carry the failed state out of the loop:
+
+	test_expect_success 'git frotz on various versions' '
+	    failed=
+	    for revision in one two three
+	    do
+		    echo "frotz $revision" >expect &&
+		    git frotz "$revision" >actual &&
+		    test_cmp expect actual ||
+		    failed="$failed${failed:+ }$revision"
+	    done &&
+	    test -z "$failed" &&
+	    test something else
+	'
+    
+    Or you can fail the entire loop immediately when you see the
+    first failure by using "return 1" from inside the loop, like so:
+
+	test_expect_success 'git frotz on various versions' '
+	    for revision in one two three
+	    do
+		    echo "frotz $revision" >expect &&
+		    git frotz "$revision" >actual &&
+		    test_cmp expect actual || return 1
+	    done &&
+	    test something else
+	'
+
+    Note that this is only possible in our test suite, where we
+    arrange to run your test <script> wrapped inside a helper
+    function to ensure that return values matter; in your own script
+    outside any function, this technique may not work.
+
+
 And here are the "don'ts:"
 
  - Don't exit() within a <script> part.

Re: [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 26, 2020 at 03:35:23PM +0000, Johannes Schindelin via GitGitGadget wrote:
>
>> Changes since v2:
>> 
>>  * Reordered 4/5 before 3/5, as I had intended originally.
>>    
>>    
>>  * Renamed _trace_level to have a trailing underscore, in line with the
>>    surrounding code.
>>    
>>    
>>  * Added a note to the commit message why only lib-gpg.sh loses its
>>    hash-bang line, and no other files in t/.
>
> Thanks, this version looks fine to me. I left a few other comments
> regarding exit/return in the other part of the thread, but frankly all
> of it is too arcane and insignificant to spend more brain cycles going
> back and forth on.  So if I convinced/inspired you on that point, feel
> free to switch it, but otherwise I'm happy with this iteration.

Likewise.  I agree the above three bullet points are strict
improvements compared to the previous iteration.

Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs

[Eric Sunshine]

On Fri, Mar 27, 2020 at 1:44 PM Junio C Hamano <gitster@pobox.com> wrote:
> Subject: [PATCH] t/README: suggest how to leave test early with failure
> +       test_expect_success 'git frotz on various versions' '
> +           for revision in one two three
> +           do
> +                   echo "frotz $revision" >expect &&
> +                   git frotz "$revision" >actual &&
> +                   test_cmp expect actual || return 1
> +           done &&
> +           test something else
> +       '
> +
> +    Note that this is only possible in our test suite, where we
> +    arrange to run your test <script> wrapped inside a helper
> +    function to ensure that return values matter; in your own script
> +    outside any function, this technique may not work.
> +
>  And here are the "don'ts:"
>
>   - Don't exit() within a <script> part.

We use 'exit 1' to terminate subshells[1] inside tests.

[1]: https://lore.kernel.org/git/20150325052952.GE31924@peff.net/

Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, Mar 27, 2020 at 1:44 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Subject: [PATCH] t/README: suggest how to leave test early with failure
>> +       test_expect_success 'git frotz on various versions' '
>> +           for revision in one two three
>> +           do
>> +                   echo "frotz $revision" >expect &&
>> +                   git frotz "$revision" >actual &&
>> +                   test_cmp expect actual || return 1
>> +           done &&
>> +           test something else
>> +       '
>> +
>> +    Note that this is only possible in our test suite, where we
>> +    arrange to run your test <script> wrapped inside a helper
>> +    function to ensure that return values matter; in your own script
>> +    outside any function, this technique may not work.
>> +
>>  And here are the "don'ts:"
>>
>>   - Don't exit() within a <script> part.
>
> We use 'exit 1' to terminate subshells[1] inside tests.
>
> [1]: https://lore.kernel.org/git/20150325052952.GE31924@peff.net/

Yeah, I gave two alternatives, but the third one could be

       test_expect_success 'git frotz on various versions' '
           (
             for revision in one two three
             do
                     echo "frotz $revision" >expect &&
                     git frotz "$revision" >actual &&
                     test_cmp expect actual || exit 1
             done 
           ) &&
           test something else
       '

Anyway, that existing rule is not what I added in the rough draft
under discussion.  To clarify it, we'd end up needing "unless A, B
or C" that may be too complex.  I dunno.

Thanks.

Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs

[Jeff King]

On Fri, Mar 27, 2020 at 10:44:27AM -0700, Junio C Hamano wrote:

> -- >8 --
> Subject: [PATCH] t/README: suggest how to leave test early with failure
> 
> Over time, we added the support to our test framework to make it
> easy to leave a test early with failure, but it was not clearly
> documented in t/README to help developers writing new tests.

Thanks for getting started on this. Everything you wrote looks correct,
but I think we can be more succinct. And I think it's worth being so,
since there are a lot of "do's" already, and we don't want to overwhelm
the user.

> + - Be careful when you loop
> +
> +   You may need to test multiple things in a loop, but the following
> +   does not work correctly:
> +
> +	test_expect_success 'git frotz on various versions' '
> +	    for revision in one two three
> +	    do
> +		    echo "frotz $revision" >expect &&
> +		    git frotz "$revision" >actual &&
> +		    test_cmp expect actual
> +	    done &&
> +	    test something else
> +	'

We could shrink this example down to the bare minimum. Perhaps:

  for i in a b c; do
    do_something "$i"
  done &&
  do_something_else

The key thing is that the "&&" will pick up only the value of
"do_something $c" and will ignore "a" and "b". That might be too dense,
but it does reduce any cognitive burden from unimportant details.

> +   If the output from the "git frotz" command did not match what is
> +   expected for 'one' and 'two', but it did for 'three', the loop
> +   itself would not fail, and the test goes on happily.  This is not
> +   what you want.

This explanation works fine, though I think you can also explain it as:

  The result code of a shell loop is the result of the final iteration;
  the results of do_something for "a" and "b" are discarded, and we'd
  pass the test even if they fail.

(I'm happy with either, just thinking out loud).

> +   You can work it around in two ways.  You could use a separate
> +   "flag" variable to carry the failed state out of the loop:

IMHO it's not worth giving this alternative. It's perfectly valid, but
we promise the "return" version works exactly so we don't have to deal
deal with this ugly and error-prone code.

> +    Or you can fail the entire loop immediately when you see the
> +    first failure by using "return 1" from inside the loop, like so:

I think we can jump straight to "in our test suite", like:

  One way around this is to break out of the loop when we see a failure.
  All test_expect_* snippets are executed inside a function, allowing an
  immediate return on failure:

    for i in a b c; do
      do_something "$i" || return 1
    done &&
    do_something_else

Possibly we'd also want to say:

  Note that we still &&-chain the loop to propagate failures from
  earlier commands.

but certainly the &&-chain linter would remind them of that. :)

>  And here are the "don'ts:"
>  
>   - Don't exit() within a <script> part.
>  * A tangent.  441ee35d (t/README: reformat Do, Don't, Keep in mind
>    lists, 2018-10-05) added these lines
> 
>     Here are the "do's:"
>     And here are the "don'ts:"
> 
>    Is the placement of the colons on these lines right?  I am
>    tempted to chase them out of the dq pair and move them at the end
>    of their lines, but obviously that is outside of the scope of
>    this patch.

I think it's a matter of taste. Most writing style guides put
punctuation inside quotes, but they're also expecting the output to be
typeset, where a trailing period ends up more under the quotes than
beside. I'm not sure what such style guides would say about colons, as
they're a bit taller.

But regardless, I actually prefer putting punctuation outside of the
quotes. It looks better to me in a fixed-width terminal setting. Plus I
guess as a programmer it feels like a parsing error. ;)

I don't know that it matters too much either way, though.

-Peff

Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs

[Jeff King]

On Fri, Mar 27, 2020 at 02:37:02PM -0700, Junio C Hamano wrote:

> >>  And here are the "don'ts:"
> >>
> >>   - Don't exit() within a <script> part.
> >
> > We use 'exit 1' to terminate subshells[1] inside tests.
> >
> > [1]: https://lore.kernel.org/git/20150325052952.GE31924@peff.net/
> 
> Yeah, I gave two alternatives, but the third one could be
> 
>        test_expect_success 'git frotz on various versions' '
>            (
>              for revision in one two three
>              do
>                      echo "frotz $revision" >expect &&
>                      git frotz "$revision" >actual &&
>                      test_cmp expect actual || exit 1
>              done 
>            ) &&
>            test something else
>        '

We definitely shouldn't suggest _introducing_ a subshell for this
purpose. It's longer to write and less efficient.

> Anyway, that existing rule is not what I added in the rough draft
> under discussion.  To clarify it, we'd end up needing "unless A, B
> or C" that may be too complex.  I dunno.

I think the existing rule is OK. If you know enough to create the
situation where "exit 1" is useful, then you probably know enough to
know when to break that rule. That said, I'm not opposed to something
like:

  - Don't call "exit" within a <script> part, unless you're in a
    subshell. It will cause the whole to test script to exit (and fail).

or something.

-Peff

[PATCH v2] t/README: suggest how to leave test early with failure

[Junio C Hamano]

Over time, we added the support to our test framework to make it
easy to leave a test early with failure, but it was not clearly
documented in t/README to help developers writing new tests.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Trimmed the description quite a bit and dropped alternatives.

 t/README | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/t/README b/t/README
index 9afd61e3ca..0dca346950 100644
--- a/t/README
+++ b/t/README
@@ -550,6 +550,40 @@ Here are the "do's:"
    reports "ok" or "not ok" to the end user running the tests. Under
    --verbose, they are shown to help debug the tests.
 
+ - Be careful when you loop
+
+   You may need to verify multiple things in a loop, but the
+   following does not work correctly:
+
+	test_expect_success 'test three things' '
+	    for i in one two three
+	    do
+		test_something "$i"
+	    done &&
+	    test_something_else
+	'
+
+   Because the status of the loop itself is the exit status of the
+   test_something in the last round, the loop does not fail when
+   "test_something" for "one" or "two".  This is not what you want.
+
+   Instead, you can break out of the loop immediately when you see a
+   failure.  Because all test_expect_* snippets are executed inside
+   a function, "return 1" can be used to fail the test immediately
+   upon a failure:
+
+	test_expect_success 'test three things' '
+	    for i in one two three
+	    do
+		test_something "$i" || return 1
+	    done &&
+	    test_something_else
+	'
+
+   Note that we still &&-chain the loop to propagate failures from
+   earlier commands.
+
+
 And here are the "don'ts:"
 
  - Don't exit() within a <script> part.
-- 
2.26.0-103-g3bab5d5625

Re: [PATCH v2] t/README: suggest how to leave test early with failure

[Eric Sunshine]

On Sat, Mar 28, 2020 at 7:49 PM Junio C Hamano <gitster@pobox.com> wrote:
> Over time, we added the support to our test framework to make it
> easy to leave a test early with failure, but it was not clearly
> documented in t/README to help developers writing new tests.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/t/README b/t/README
> @@ -550,6 +550,40 @@ Here are the "do's:"
> +       test_expect_success 'test three things' '
> +           for i in one two three
> +           do
> +               test_something "$i"
> +           done &&
> +           test_something_else
> +       '
> +
> +   Because the status of the loop itself is the exit status of the
> +   test_something in the last round, the loop does not fail when
> +   "test_something" for "one" or "two".  This is not what you want.

s/"two"/& fails/

Re: [PATCH v2] t/README: suggest how to leave test early with failure

[Jeff King]

On Sat, Mar 28, 2020 at 04:49:07PM -0700, Junio C Hamano wrote:

> Over time, we added the support to our test framework to make it
> easy to leave a test early with failure, but it was not clearly
> documented in t/README to help developers writing new tests.
> 
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

With the exception of the typo-fix from Eric, this looks good to
me. Thanks for tying this up.

-Peff

Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs

[Johannes Schindelin]

Hi Peff,

On Fri, 27 Mar 2020, Jeff King wrote:

> On Thu, Mar 26, 2020 at 03:27:19PM +0100, Johannes Schindelin wrote:
>
> > > OK. This looks good, even if I cannot help feel that my earlier patch
> > > was perfectly sufficient. ;)
> >
> > The mistake is all mine. I had totally missed that you turned GPG into a
> > lazy prereq. So I had my patch finalized already before you pointed my
> > nose at that fact.
> >
> > Sorry about that.
>
> No problem. And I hope my review didn't sound too passive-aggressive
> with the "well, in MY version we did this...".

FWIW I failed to interpret anything in your reply as passive-aggressive,
probably because I am just too used to receive competent, helpful and
friendly replies from you.

> I focused on the differences because those were the parts that were new
> (and therefore interesting) to me. But I don't think any of them are too
> important either way.

To me, it all sounded like a constructive discussion we had, and since you
already had a working patch that did something very similar to mine, it
made sense to look at their differences.

> > - Since this code is outside of a function, `return` felt like the wrong
> >   semantic concept to me. And indeed, I see this (rather scary) part in
> >   Bash's documentation of `return` (I did not even bother to look at the
> >   POSIX semantics, it scared me so much):
> >
> >       The return status is non-zero if `return` is supplied a non-numeric
> >       argument, or is used outside a function and not during execution of
> >       a script by `.` or `source`.
> >
> >   So: the `1` is totally ignored in this context. That alone is reason
> >   enough for me to completely avoid it, and use `exit` instead.
>
> I agree the portability rules there are scary, but none of that applies
> because we _are_ in a function: test_eval_inner_(). This behavior is
> intentional and due to a7c58f280a (test: cope better with use of return
> for errors, 2011-08-08). I thought we specifically advertised this
> feature in t/README, but I can't seem to find it.
>
> So my perspective was the opposite of yours: "return" is the officially
> sanctioned way to exit early from a test snippet, and "exit" only
> happens to work because of the undocumented fact that lazy prereqs
> happen in a subshell. But it turns out neither was documented. :)

Can a subshell inside a function cause a `return` from said function? I
don't think so, but let's put that to a test:

	function return_from_a_subshell () {
		echo before
		(echo subshell; return; echo unreachable)
		echo after $?
	}

Let's run that.

	$ return_from_a_subshell
	before
	subshell
	after 0

To me, the fact that that `return` does not return from the function, but
only exits the subshell, in my mind lends more credence to the idea that
`exit` is more appropriate in this context than `return`.

For shiggles, I also added that `$?` because I really, _really_ wanted to
know whether my reading of GNU Bash's documentation was correct, and it
appears I was mistaken: apparently `return` used outside a function does
_not_ cause a non-zero exit code.

> > > In mine I put the test_have_prereq outside the lazy prereq.
> >
> > That makes it essentially a non-lazy prereq.
> >
> > > I don't think it really matters either way (when we later ask if GPGSM
> > > is set, there is no difference between nobody having defined it, and
> > > having a lazy definition that said "no").
> >
> > The difference is when running a test with `--run=<n>` where `<n>` refers
> > to a test case that requires neither GPG nor GPGSM or RFC1991. My version
> > will not evaluate the GPG prereq, yours still will.
>
> Yes. Part of the reason I kept mine as it was is because it matched the
> original behavior better (and I was really only using a lazy prereq
> because we didn't have a non-lazy version). But there's really no reason
> _not_ to be lazy, so as long as it isn't breaking anything I think
> that's a better way forward. (And if it is breaking something, that
> something should be fixed).

I'm glad you agree!

Thanks,
Dscho

Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs

[Jeff King]

On Mon, Mar 30, 2020 at 08:39:08PM +0200, Johannes Schindelin wrote:

> > So my perspective was the opposite of yours: "return" is the officially
> > sanctioned way to exit early from a test snippet, and "exit" only
> > happens to work because of the undocumented fact that lazy prereqs
> > happen in a subshell. But it turns out neither was documented. :)
> 
> Can a subshell inside a function cause a `return` from said function? I
> don't think so, but let's put that to a test:
> [...]
> To me, the fact that that `return` does not return from the function, but
> only exits the subshell, in my mind lends more credence to the idea that
> `exit` is more appropriate in this context than `return`.

Hmm, yeah, I was wrong about it actually returning from the function.
Thanks for demonstrating.  Returning from just the subshell in the case
of lazy_prereq is OK for our purposes, since the exit value of the
subshell is taken as the result of the prereq check (and in turn becomes
the return value of that function anyway).

But it does make more sympathetic to the idea that "exit" is appropriate
here. Especially given the prodding below (which you can skip to the
last paragraph if you're not interested in shell arcana):

> For shiggles, I also added that `$?` because I really, _really_ wanted to
> know whether my reading of GNU Bash's documentation was correct, and it
> appears I was mistaken: apparently `return` used outside a function does
> _not_ cause a non-zero exit code.

I think the issue may be in the definition of "outside a function".

If we really are at the top-level outside of a function, then return
gives a non-zero exit but _doesn't_ return in bash:

  $ bash -c 'return 2; echo inside=$?'; echo outside=$?
  bash: line 0: return: can only `return' from a function or sourced script
  inside=1
  outside=0

So even though we asked to return 2, it gave us a generic "1" return
code and continued executing (and then outside=0 because the echo was
successful).

And that's true even in a subshell (not we moved "outside" into the bash
process so we can see that it keeps going):

  $ bash -c '(return 2; echo inside=$?); echo outside=$?'
  bash: line 0: return: can only `return' from a function or sourced script
  inside=1
  outside=0

But if we actually _are_ inside a function, even inside a subshell, then
return "works", by stopping execution in the subshell and returning the
value we asked (in your example we got "0" because you didn't specify a
value for "return", so it just propagated the exit code of the earlier
"echo").

  $ bash -c 'f() { (return 2; echo inside=$?); echo outside=$?; }; f'
  outside=2

It's just a bit odd (to me) that it still runs the rest of the function.

Dash behaves a bit more sensibly with an out-of-function return, just
returning from the script:

  $ dash -c 'return 2; echo inside=$?'; echo outside=$?
  outside=2

and with a subshell, it returns only from that subshell:

  $ dash -c '(return 2; echo inside=$?); echo outside=$?'
  outside=2

So inside a subshell-in-a-function, it behaves exactly the same
(returning from the subshell but not the function).

I think the behavior of both shells is fine for our purposes. We _are_
in a function, so as long as we return from the subshell immediately
we're happy. But given the oddities in how bash behaves, and the fact
that POSIX says:

  If the shell is not currently executing a function or dot script, the
  results are unspecified.

it may be better to stay away from the question entirely.

-Peff