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

[Jeff King <peff@peff.net>]

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