Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
- From
Jeff King <peff@peff.net>
- Date
- Mar 26, 2020, 08:35 UTC
- Message-ID
- <20200326083519.GD2200716@coredump.intra.peff.net>
- In-Reply-To
- <85457a7b61874e8e9f3af9c231451df0aba7a7b5.1585114881.git.gitgitgadget@gmail.com>
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. ;)
Show 5 quoted lines
> 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).
Show 5 quoted lines
> 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.
Show 10 quoted lines
> @@ -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