* [PATCH v2 1/5] tests(gpg): allow the gpg-agent to start on Windows
2020-03-25 5:41 ` [PATCH v2 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
@ 2020-03-25 5:41 ` Johannes Schindelin via GitGitGadget
2020-03-25 5:41 ` [PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script Johannes Schindelin via GitGitGadget
` (4 subsequent siblings)
5 siblings, 0 replies; 55+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-03-25 5:41 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Johannes Schindelin, Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script
2020-03-25 5:41 ` [PATCH v2 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
2020-03-25 5:41 ` [PATCH v2 1/5] tests(gpg): allow the gpg-agent to start on Windows Johannes Schindelin via GitGitGadget
@ 2020-03-25 5:41 ` Johannes Schindelin via GitGitGadget
2020-03-26 8:21 ` Jeff King
2020-03-25 5:41 ` [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs Johannes Schindelin via GitGitGadget
` (3 subsequent siblings)
5 siblings, 1 reply; 55+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-03-25 5:41 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Johannes Schindelin, Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script
2020-03-25 5:41 ` [PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script Johannes Schindelin via GitGitGadget
@ 2020-03-26 8:21 ` Jeff King
2020-03-26 13:48 ` Johannes Schindelin
2020-03-26 19:31 ` Junio C Hamano
0 siblings, 2 replies; 55+ messages in thread
From: Jeff King @ 2020-03-26 8:21 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Junio C Hamano, Johannes Schindelin
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script
2020-03-26 8:21 ` Jeff King
@ 2020-03-26 13:48 ` Johannes Schindelin
2020-03-26 19:31 ` Junio C Hamano
1 sibling, 0 replies; 55+ messages in thread
From: Johannes Schindelin @ 2020-03-26 13:48 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script
2020-03-26 8:21 ` Jeff King
2020-03-26 13:48 ` Johannes Schindelin
@ 2020-03-26 19:31 ` Junio C Hamano
1 sibling, 0 replies; 55+ messages in thread
From: Junio C Hamano @ 2020-03-26 19:31 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin
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.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
2020-03-25 5:41 ` [PATCH v2 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
2020-03-25 5:41 ` [PATCH v2 1/5] tests(gpg): allow the gpg-agent to start on Windows Johannes Schindelin via GitGitGadget
2020-03-25 5:41 ` [PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script Johannes Schindelin via GitGitGadget
@ 2020-03-25 5:41 ` Johannes Schindelin via GitGitGadget
2020-03-25 17:25 ` Junio C Hamano
2020-03-26 8:35 ` Jeff King
2020-03-25 5:41 ` [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing Johannes Schindelin via GitGitGadget
` (2 subsequent siblings)
5 siblings, 2 replies; 55+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-03-25 5:41 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Johannes Schindelin, Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
2020-03-25 5:41 ` [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs Johannes Schindelin via GitGitGadget
@ 2020-03-25 17:25 ` Junio C Hamano
2020-03-26 8:35 ` Jeff King
1 sibling, 0 replies; 55+ messages in thread
From: Junio C Hamano @ 2020-03-25 17:25 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Jeff King, Johannes Schindelin
"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.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
2020-03-25 5:41 ` [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs Johannes Schindelin via GitGitGadget
2020-03-25 17:25 ` Junio C Hamano
@ 2020-03-26 8:35 ` Jeff King
2020-03-26 14:27 ` Johannes Schindelin
1 sibling, 1 reply; 55+ messages in thread
From: Jeff King @ 2020-03-26 8:35 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Junio C Hamano, Johannes Schindelin
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
2020-03-26 8:35 ` Jeff King
@ 2020-03-26 14:27 ` Johannes Schindelin
2020-03-27 9:10 ` Jeff King
0 siblings, 1 reply; 55+ messages in thread
From: Johannes Schindelin @ 2020-03-26 14:27 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
2020-03-26 14:27 ` Johannes Schindelin
@ 2020-03-27 9:10 ` Jeff King
2020-03-27 17:44 ` Junio C Hamano
2020-03-30 18:39 ` [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs Johannes Schindelin
0 siblings, 2 replies; 55+ messages in thread
From: Jeff King @ 2020-03-27 9:10 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
2020-03-27 9:10 ` Jeff King
@ 2020-03-27 17:44 ` Junio C Hamano
2020-03-27 20:24 ` Eric Sunshine
2020-03-28 10:54 ` Jeff King
2020-03-30 18:39 ` [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs Johannes Schindelin
1 sibling, 2 replies; 55+ messages in thread
From: Junio C Hamano @ 2020-03-27 17:44 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, Johannes Schindelin via GitGitGadget, git
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.
^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
2020-03-27 17:44 ` Junio C Hamano
@ 2020-03-27 20:24 ` Eric Sunshine
2020-03-27 21:37 ` Junio C Hamano
2020-03-28 10:54 ` Jeff King
1 sibling, 1 reply; 55+ messages in thread
From: Eric Sunshine @ 2020-03-27 20:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, Johannes Schindelin,
Johannes Schindelin via GitGitGadget, Git List
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/
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
2020-03-27 20:24 ` Eric Sunshine
@ 2020-03-27 21:37 ` Junio C Hamano
2020-03-28 10:58 ` Jeff King
0 siblings, 1 reply; 55+ messages in thread
From: Junio C Hamano @ 2020-03-27 21:37 UTC (permalink / raw)
To: Eric Sunshine
Cc: Jeff King, Johannes Schindelin,
Johannes Schindelin via GitGitGadget, Git List
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.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
2020-03-27 21:37 ` Junio C Hamano
@ 2020-03-28 10:58 ` Jeff King
0 siblings, 0 replies; 55+ messages in thread
From: Jeff King @ 2020-03-28 10:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: Eric Sunshine, Johannes Schindelin,
Johannes Schindelin via GitGitGadget, Git List
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
2020-03-27 17:44 ` Junio C Hamano
2020-03-27 20:24 ` Eric Sunshine
@ 2020-03-28 10:54 ` Jeff King
2020-03-28 23:49 ` [PATCH v2] t/README: suggest how to leave test early with failure Junio C Hamano
1 sibling, 1 reply; 55+ messages in thread
From: Jeff King @ 2020-03-28 10:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, Johannes Schindelin via GitGitGadget, git
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2] t/README: suggest how to leave test early with failure
2020-03-28 10:54 ` Jeff King
@ 2020-03-28 23:49 ` Junio C Hamano
2020-03-29 7:23 ` Eric Sunshine
2020-03-29 14:33 ` Jeff King
0 siblings, 2 replies; 55+ messages in thread
From: Junio C Hamano @ 2020-03-28 23:49 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, git
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
^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v2] t/README: suggest how to leave test early with failure
2020-03-28 23:49 ` [PATCH v2] t/README: suggest how to leave test early with failure Junio C Hamano
@ 2020-03-29 7:23 ` Eric Sunshine
2020-03-29 14:33 ` Jeff King
1 sibling, 0 replies; 55+ messages in thread
From: Eric Sunshine @ 2020-03-29 7:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, Git List
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/
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2] t/README: suggest how to leave test early with failure
2020-03-28 23:49 ` [PATCH v2] t/README: suggest how to leave test early with failure Junio C Hamano
2020-03-29 7:23 ` Eric Sunshine
@ 2020-03-29 14:33 ` Jeff King
1 sibling, 0 replies; 55+ messages in thread
From: Jeff King @ 2020-03-29 14:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
2020-03-27 9:10 ` Jeff King
2020-03-27 17:44 ` Junio C Hamano
@ 2020-03-30 18:39 ` Johannes Schindelin
2020-03-31 9:34 ` Jeff King
1 sibling, 1 reply; 55+ messages in thread
From: Johannes Schindelin @ 2020-03-30 18:39 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
2020-03-30 18:39 ` [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs Johannes Schindelin
@ 2020-03-31 9:34 ` Jeff King
0 siblings, 0 replies; 55+ messages in thread
From: Jeff King @ 2020-03-31 9:34 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing
2020-03-25 5:41 ` [PATCH v2 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
` (2 preceding siblings ...)
2020-03-25 5:41 ` [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs Johannes Schindelin via GitGitGadget
@ 2020-03-25 5:41 ` Johannes Schindelin via GitGitGadget
2020-03-25 17:23 ` Junio C Hamano
2020-03-26 8:49 ` Jeff King
2020-03-25 5:41 ` [PATCH v2 5/5] tests: increase the verbosity of the GPG-related prereqs Johannes Schindelin via GitGitGadget
2020-03-26 15:35 ` [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
5 siblings, 2 replies; 55+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-03-25 5:41 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Johannes Schindelin, Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing
2020-03-25 5:41 ` [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing Johannes Schindelin via GitGitGadget
@ 2020-03-25 17:23 ` Junio C Hamano
2020-03-26 13:45 ` Johannes Schindelin
2020-03-26 8:49 ` Jeff King
1 sibling, 1 reply; 55+ messages in thread
From: Junio C Hamano @ 2020-03-25 17:23 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget; +Cc: git, Jeff King, Johannes Schindelin
"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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing
2020-03-25 17:23 ` Junio C Hamano
@ 2020-03-26 13:45 ` Johannes Schindelin
0 siblings, 0 replies; 55+ messages in thread
From: Johannes Schindelin @ 2020-03-26 13:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin via GitGitGadget, git, Jeff King
[-- 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
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing
2020-03-25 5:41 ` [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing Johannes Schindelin via GitGitGadget
2020-03-25 17:23 ` Junio C Hamano
@ 2020-03-26 8:49 ` Jeff King
2020-03-26 14:34 ` Johannes Schindelin
1 sibling, 1 reply; 55+ messages in thread
From: Jeff King @ 2020-03-26 8:49 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Junio C Hamano, Johannes Schindelin
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing
2020-03-26 8:49 ` Jeff King
@ 2020-03-26 14:34 ` Johannes Schindelin
0 siblings, 0 replies; 55+ messages in thread
From: Johannes Schindelin @ 2020-03-26 14:34 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v2 5/5] tests: increase the verbosity of the GPG-related prereqs
2020-03-25 5:41 ` [PATCH v2 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
` (3 preceding siblings ...)
2020-03-25 5:41 ` [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing Johannes Schindelin via GitGitGadget
@ 2020-03-25 5:41 ` Johannes Schindelin via GitGitGadget
2020-03-26 8:50 ` Jeff King
2020-03-26 15:35 ` [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
5 siblings, 1 reply; 55+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-03-25 5:41 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Johannes Schindelin, Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v2 5/5] tests: increase the verbosity of the GPG-related prereqs
2020-03-25 5:41 ` [PATCH v2 5/5] tests: increase the verbosity of the GPG-related prereqs Johannes Schindelin via GitGitGadget
@ 2020-03-26 8:50 ` Jeff King
2020-03-26 14:36 ` Johannes Schindelin
0 siblings, 1 reply; 55+ messages in thread
From: Jeff King @ 2020-03-26 8:50 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Junio C Hamano, Johannes Schindelin
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v2 5/5] tests: increase the verbosity of the GPG-related prereqs
2020-03-26 8:50 ` Jeff King
@ 2020-03-26 14:36 ` Johannes Schindelin
0 siblings, 0 replies; 55+ messages in thread
From: Johannes Schindelin @ 2020-03-26 14:36 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git, Junio C Hamano
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds
2020-03-25 5:41 ` [PATCH v2 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
` (4 preceding siblings ...)
2020-03-25 5:41 ` [PATCH v2 5/5] tests: increase the verbosity of the GPG-related prereqs Johannes Schindelin via GitGitGadget
@ 2020-03-26 15:35 ` Johannes Schindelin via GitGitGadget
2020-03-26 15:35 ` [PATCH v3 1/5] tests(gpg): allow the gpg-agent to start on Windows Johannes Schindelin via GitGitGadget
` (5 more replies)
5 siblings, 6 replies; 55+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-03-26 15:35 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Johannes Schindelin
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v3 1/5] tests(gpg): allow the gpg-agent to start on Windows
2020-03-26 15:35 ` [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
@ 2020-03-26 15:35 ` Johannes Schindelin via GitGitGadget
2020-03-26 15:35 ` [PATCH v3 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script Johannes Schindelin via GitGitGadget
` (4 subsequent siblings)
5 siblings, 0 replies; 55+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-03-26 15:35 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Johannes Schindelin, Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v3 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script
2020-03-26 15:35 ` [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
2020-03-26 15:35 ` [PATCH v3 1/5] tests(gpg): allow the gpg-agent to start on Windows Johannes Schindelin via GitGitGadget
@ 2020-03-26 15:35 ` Johannes Schindelin via GitGitGadget
2020-03-26 15:35 ` [PATCH v3 3/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing Johannes Schindelin via GitGitGadget
` (3 subsequent siblings)
5 siblings, 0 replies; 55+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-03-26 15:35 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Johannes Schindelin, Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v3 3/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing
2020-03-26 15:35 ` [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
2020-03-26 15:35 ` [PATCH v3 1/5] tests(gpg): allow the gpg-agent to start on Windows Johannes Schindelin via GitGitGadget
2020-03-26 15:35 ` [PATCH v3 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script Johannes Schindelin via GitGitGadget
@ 2020-03-26 15:35 ` Johannes Schindelin via GitGitGadget
2020-03-26 15:35 ` [PATCH v3 4/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs Johannes Schindelin via GitGitGadget
` (2 subsequent siblings)
5 siblings, 0 replies; 55+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-03-26 15:35 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Johannes Schindelin, Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v3 4/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs
2020-03-26 15:35 ` [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
` (2 preceding siblings ...)
2020-03-26 15:35 ` [PATCH v3 3/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing Johannes Schindelin via GitGitGadget
@ 2020-03-26 15:35 ` Johannes Schindelin via GitGitGadget
2020-03-26 15:35 ` [PATCH v3 5/5] tests: increase the verbosity of the GPG-related prereqs Johannes Schindelin via GitGitGadget
2020-03-27 9:12 ` [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds Jeff King
5 siblings, 0 replies; 55+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-03-26 15:35 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Johannes Schindelin, Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v3 5/5] tests: increase the verbosity of the GPG-related prereqs
2020-03-26 15:35 ` [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
` (3 preceding siblings ...)
2020-03-26 15:35 ` [PATCH v3 4/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs Johannes Schindelin via GitGitGadget
@ 2020-03-26 15:35 ` Johannes Schindelin via GitGitGadget
2020-03-27 9:12 ` [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds Jeff King
5 siblings, 0 replies; 55+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2020-03-26 15:35 UTC (permalink / raw)
To: git; +Cc: Jeff King, Junio C Hamano, Johannes Schindelin, Johannes Schindelin
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
^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds
2020-03-26 15:35 ` [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds Johannes Schindelin via GitGitGadget
` (4 preceding siblings ...)
2020-03-26 15:35 ` [PATCH v3 5/5] tests: increase the verbosity of the GPG-related prereqs Johannes Schindelin via GitGitGadget
@ 2020-03-27 9:12 ` Jeff King
2020-03-27 17:45 ` Junio C Hamano
5 siblings, 1 reply; 55+ messages in thread
From: Jeff King @ 2020-03-27 9:12 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Junio C Hamano, Johannes Schindelin
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
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds
2020-03-27 9:12 ` [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds Jeff King
@ 2020-03-27 17:45 ` Junio C Hamano
0 siblings, 0 replies; 55+ messages in thread
From: Junio C Hamano @ 2020-03-27 17:45 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin
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.
^ permalink raw reply [flat|nested] 55+ messages in thread