{"thread":{"id":"53068","subject":"[PATCH 0/2] Enable GPG in the Windows part of the CI/PR builds","startedAt":"2020-03-23T13:09:56Z","lastAt":"2020-03-31T09:34:41Z","messageCount":55,"participants":["Johannes Schindelin via GitGitGadget","Jeff King","Junio C Hamano","Johannes Schindelin","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"393763","messageId":"pull.728.git.git.1584968990.gitgitgadget@gmail.com","threadId":"53068","inReplyTo":null,"subject":"[PATCH 0/2] Enable GPG in the Windows part of the CI/PR builds","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-23T13:09:48Z","receivedAt":"2020-03-23T13:09:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"While debugging the breakages introduced by hi/gpg-prefer-check-signature, I\nnoticed that the GPG prereq was not available on Windows, even if Git for\nWindows' SDK comes with a fully functional GPG2.\n\nThe fix was easy, but finding out what was going on was not, so for good\nmeasure, the fix is accompanied by a second patch that will hopefully make\nfuture investigations into GPG-related problems much, much easier.\n\nJohannes Schindelin (2):\n tests(gpg): allow the gpg-agent to start on Windows\n tests(gpg): increase verbosity to allow debugging\n\n t/lib-gpg.sh | 29 ++++++++++++++++-------------\n 1 file changed, 16 insertions(+), 13 deletions(-)\n\n\nbase-commit: 30e9940356dc67959877f4b2417da33ebdefbb79\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-728%2Fdscho%2Fci-windows-gpg-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-728/dscho/ci-windows-gpg-v1\nPull-Request: https://github.com/git/git/pull/728\n-- \ngitgitgadget\n"},{"id":"393764","messageId":"dd26cb05a37a54d9d245823772d33fe0daab8ffa.1584968990.git.gitgitgadget@gmail.com","threadId":"53068","inReplyTo":"pull.728.git.git.1584968990.gitgitgadget@gmail.com","subject":"[PATCH 2/2] tests(gpg): increase verbosity to allow debugging","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-23T13:09:50Z","receivedAt":"2020-03-23T13:09:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nEspecially when debugging a test failure that can only be reproduced in\nthe CI build (e.g. when the developer has no access to a macOS machine\nother than running the tests on a macOS build agent), output should not\nbe suppressed.\n\nIn the instance of `hi/gpg-prefer-check-signature`, where one\nGPG-related test failed for no apparent reason, the entire output of\n`gpg` and `gpgsm` was suppressed, even in verbose mode, leaving\ninterested readers no clue what was going wrong.\n\nLet's fix this by redirecting the output not to `/dev/null`, but to the\nfile descriptors that may, or may not, be redirected via\n`--verbose-log`. For good measure, also turn on tracing if the user\nasked for it, and prefix it with a helpful info message.\n\nSigned-off-by: Johannes Schindelin \n---\n t/lib-gpg.sh | 27 +++++++++++++++------------\n 1 file changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex 11b83b8c24a..a7d0708f5df 100755\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -11,6 +11,8 @@ then\n \t\tsay \"Your version of gpg (1.0.6) is too buggy for testing\"\n \t\t;;\n \t*)\n+\t\tsay_color info >&4 \"Trying to set up GPG\"\n+\t\twant_trace && set -x\n \t\t# Available key info:\n \t\t# * Type DSA and Elgamal, size 2048 bits, no expiration date,\n \t\t# name and email: C O Mitter \n@@ -31,13 +33,13 @@ then\n \t\tchmod 0700 ./gpghome &&\n \t\tGNUPGHOME=\"$PWD/gpghome\" &&\n \t\texport GNUPGHOME &&\n-\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n-\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n-\t\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg &&\n-\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import-ownertrust \\\n-\t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n-\t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n-\t\t\t--sign -u committer@example.com &&\n+\t\t(gpgconf --kill gpg-agent >&3 2>&4 || : ) &&\n+\t\tgpg --homedir \"${GNUPGHOME}\" --import \\\n+\t\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg >&3 2>&4 &&\n+\t\tgpg --homedir \"${GNUPGHOME}\" --import-ownertrust \\\n+\t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust >&3 2>&4 &&\n+\t\tgpg --homedir \"${GNUPGHOME}\" &3 2>&4 &&\n \t\ttest_set_prereq GPG &&\n \t\t# Available key info:\n \t\t# * see t/lib-gpg/gpgsm-gen-key.in\n@@ -54,28 +56,29 @@ then\n \t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n \t\t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n \t\t#\t\t--export-secret-key-p12 \"committer@example.com\"\n-\t\techo | gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null \\\n+\t\techo | gpgsm --homedir \"${GNUPGHOME}\" >&3 2>&4 \\\n \t\t\t--passphrase-fd 0 --pinentry-mode loopback \\\n \t\t\t--import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n \n-\t\tgpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null -K |\n+\t\tgpgsm --homedir \"${GNUPGHOME}\" -K 2>&4 |\n \t\tgrep fingerprint: |\n \t\tcut -d\" \" -f4 |\n \t\ttr -d '\\n' >\"${GNUPGHOME}/trustlist.txt\" &&\n \n \t\techo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n-\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" >/dev/null \\\n-\t\t\t-u committer@example.com -o /dev/null --sign - 2>&1 &&\n+\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" >&3 2>&4 \\\n+\t\t\t-u committer@example.com -o /dev/null --sign - &&\n \t\ttest_set_prereq GPGSM\n \t\t;;\n \tesac\n fi\n \n if test_have_prereq GPG &&\n- echo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null 2>&1\n+ echo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >&3 2>&4\n then\n \ttest_set_prereq RFC1991\n fi\n+want_trace && set +x\n \n sanitize_pgp() {\n \tperl -ne '\n-- \ngitgitgadget\n"},{"id":"393765","messageId":"287a21f1033b2a74420029c529ad4db956051a85.1584968990.git.gitgitgadget@gmail.com","threadId":"53068","inReplyTo":"pull.728.git.git.1584968990.gitgitgadget@gmail.com","subject":"[PATCH 1/2] tests(gpg): allow the gpg-agent to start on Windows","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-23T13:09:49Z","receivedAt":"2020-03-23T13:10:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn Git for Windows' SDK, we use the MSYS2 version of OpenSSH, meaning\nthat the `gpg-agent` will fail horribly when being passed a `--homedir`\nthat contains colons.\n\nPreviously, we did pass the Windows version of the absolute path,\nthough, which starts in the drive letter followed by, you guessed it, a\ncolon.\n\nLet's use the same trick found elsewhere in our test suite where `$PWD`\nis used to refer to the pseudo-Unix path (which works only within the\nMSYS2 Bash/OpenSSH/Perl/etc, as opposed to `$(pwd)` which refers to the\nWindows path that `git.exe` understands, too).\n\nSigned-off-by: Johannes Schindelin \n---\n t/lib-gpg.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex 8d28652b729..11b83b8c24a 100755\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -29,7 +29,7 @@ then\n \t\t#\t\t> lib-gpg/ownertrust\n \t\tmkdir ./gpghome &&\n \t\tchmod 0700 ./gpghome &&\n-\t\tGNUPGHOME=\"$(pwd)/gpghome\" &&\n+\t\tGNUPGHOME=\"$PWD/gpghome\" &&\n \t\texport GNUPGHOME &&\n \t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n \t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n-- \ngitgitgadget\n\n"},{"id":"393786","messageId":"20200323173258.GA3977@coredump.intra.peff.net","threadId":"53068","inReplyTo":"dd26cb05a37a54d9d245823772d33fe0daab8ffa.1584968990.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] tests(gpg): increase verbosity to allow debugging","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-23T17:32:58Z","receivedAt":"2020-03-23T17:33:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 23, 2020 at 01:09:50PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> Especially when debugging a test failure that can only be reproduced in\n> the CI build (e.g. when the developer has no access to a macOS machine\n> other than running the tests on a macOS build agent), output should not\n> be suppressed.\n> \n> In the instance of `hi/gpg-prefer-check-signature`, where one\n> GPG-related test failed for no apparent reason, the entire output of\n> `gpg` and `gpgsm` was suppressed, even in verbose mode, leaving\n> interested readers no clue what was going wrong.\n> \n> Let's fix this by redirecting the output not to `/dev/null`, but to the\n> file descriptors that may, or may not, be redirected via\n> `--verbose-log`. For good measure, also turn on tracing if the user\n> asked for it, and prefix it with a helpful info message.\n\nIt definitely makes sense for this info to be shown in verbose (and\n\"-x\") mode. I'm OK with this patch as easiest way to get from A to B,\nbut I think the existing code shows a bit of an anti-pattern: trying to\ndo too much outside of test blocks where verbosity and tracing is\nalready handled.\n\nIn this case if the whole thing were in a \"test_lazy_prereq\" we would\nhave gotten all that for free. I don't think the laziness would matter\ntoo much in practice, though it is a little funny that it has side\neffects (like setting up $GPGHOME). Having an immediate version like\n\"test_check_prereq\" would make sense to me.\n\nI don't know if it's worth re-doing this patch (I'll leave that decision\nto you). But something to keep in mind when we run into similar\nsituations (or are writing new prereq code).\n\n-Peff\n"},{"id":"393788","messageId":"xmqqd09355h7.fsf@gitster.c.googlers.com","threadId":"53068","inReplyTo":"287a21f1033b2a74420029c529ad4db956051a85.1584968990.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] tests(gpg): allow the gpg-agent to start on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-23T17:46:44Z","receivedAt":"2020-03-23T17:46:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In Git for Windows' SDK, we use the MSYS2 version of OpenSSH, meaning\n> that the `gpg-agent` will fail horribly when being passed a `--homedir`\n> that contains colons.\n>\n> Previously, we did pass the Windows version of the absolute path,\n> though, which starts in the drive letter followed by, you guessed it, a\n> colon.\n>\n> Let's use the same trick found elsewhere in our test suite where `$PWD`\n> is used to refer to the pseudo-Unix path (which works only within the\n> MSYS2 Bash/OpenSSH/Perl/etc, as opposed to `$(pwd)` which refers to the\n> Windows path that `git.exe` understands, too).\n\nMakes sense. \n\nDo we have a short/concise instruction, e.g. \"You should use $(pwd)\nin most cases, but for such and such purposes use $PWD instead\", in\nt/README for test writers, who are not familiar with the distinction\nbetween $(pwd) and $PWD, to help them decide which one to use in\nwhat situation? I see this kind of fix-ups from time to time, and\nam wondering if there is a way to reduce the need for you or J6t to\nspot and fix the new ones.\n\nThanks.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> t/lib-gpg.sh | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\n> index 8d28652b729..11b83b8c24a 100755\n> --- a/t/lib-gpg.sh\n> +++ b/t/lib-gpg.sh\n> @@ -29,7 +29,7 @@ then\n> \t\t#\t\t> lib-gpg/ownertrust\n> \t\tmkdir ./gpghome &&\n> \t\tchmod 0700 ./gpghome &&\n> -\t\tGNUPGHOME=\"$(pwd)/gpghome\" &&\n> +\t\tGNUPGHOME=\"$PWD/gpghome\" &&\n> \t\texport GNUPGHOME &&\n> \t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n> \t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n"},{"id":"393790","messageId":"20200323180445.GA31401@coredump.intra.peff.net","threadId":"53068","inReplyTo":"20200323173258.GA3977@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] tests(gpg): increase verbosity to allow debugging","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-23T18:04:45Z","receivedAt":"2020-03-23T18:04:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 23, 2020 at 01:32:58PM -0400, Jeff King wrote:\n\n> In this case if the whole thing were in a \"test_lazy_prereq\" we would\n> have gotten all that for free. I don't think the laziness would matter\n> too much in practice, though it is a little funny that it has side\n> effects (like setting up $GPGHOME). Having an immediate version like\n> \"test_check_prereq\" would make sense to me.\n> \n> I don't know if it's worth re-doing this patch (I'll leave that decision\n> to you). But something to keep in mind when we run into similar\n> situations (or are writing new prereq code).\n\nActually, it's pretty straight-forward and I think the resulting code is\ncleaner. Note that we do have to use a real expect_success because of\nthe side effects. That does increment the test counter. IMHO that's not\na big deal, but if we're concerned it wouldn't be too hard to add a\nnon-lazy prereq block.\n\nPatch is below (I pulled GPGSM into its own block, which involved\nreindenting; view with \"-w\").\n\n---\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex 11b83b8c24..6aa936e3ac 100755\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -1,13 +1,16 @@\n #!/bin/sh\n \n-gpg_version=$(gpg --version 2>&1)\n-if test $? != 127\n-then\n+# This has to happen in a real test and not a lazy_prereq because it\n+# has side effects (like setting up $GNUPGHOME).\n+test_expect_success 'set up GPG prereq' '\n+\ttest_might_fail gpg --version &&\n+\ttest $? != 127 &&\n+\n \t# As said here: http://www.gnupg.org/documentation/faqs.html#q6.19\n-\t# the gpg version 1.0.6 didn't parse trust packets correctly, so for\n+\t# the gpg version 1.0.6 did not parse trust packets correctly, so for\n \t# that version, creation of signed tags using the generated key fails.\n \tcase \"$gpg_version\" in\n-\t'gpg (GnuPG) 1.0.6'*)\n+\t\"gpg (GnuPG) 1.0.6\"*)\n \t\tsay \"Your version of gpg (1.0.6) is too buggy for testing\"\n \t\t;;\n \t*)\n@@ -31,51 +34,51 @@ then\n \t\tchmod 0700 ./gpghome &&\n \t\tGNUPGHOME=\"$PWD/gpghome\" &&\n \t\texport GNUPGHOME &&\n-\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n-\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n+\t\t(gpgconf --kill gpg-agent || : ) &&\n+\t\tgpg --homedir \"${GNUPGHOME}\" --import \\\n \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg &&\n-\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import-ownertrust \\\n+\t\tgpg --homedir \"${GNUPGHOME}\" --import-ownertrust \\\n \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n-\t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n-\t\t\t--sign -u committer@example.com &&\n-\t\ttest_set_prereq GPG &&\n-\t\t# Available key info:\n-\t\t# * see t/lib-gpg/gpgsm-gen-key.in\n-\t\t# To generate new certificate:\n-\t\t# * no passphrase\n-\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n-\t\t#\t\t-o /tmp/gpgsm.crt.user \\\n-\t\t#\t\t--generate-key \\\n-\t\t#\t\t--batch t/lib-gpg/gpgsm-gen-key.in\n-\t\t# To import certificate:\n-\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n-\t\t#\t\t--import /tmp/gpgsm.crt.user\n-\t\t# To export into a .p12 we can later import:\n-\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n-\t\t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n-\t\t#\t\t--export-secret-key-p12 \"committer@example.com\"\n-\t\techo | gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null \\\n-\t\t\t--passphrase-fd 0 --pinentry-mode loopback \\\n-\t\t\t--import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n-\n-\t\tgpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null -K |\n-\t\tgrep fingerprint: |\n-\t\tcut -d\" \" -f4 |\n-\t\ttr -d '\\n' >\"${GNUPGHOME}/trustlist.txt\" &&\n-\n-\t\techo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n-\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" >/dev/null \\\n-\t\t\t-u committer@example.com -o /dev/null --sign - 2>&1 &&\n-\t\ttest_set_prereq GPGSM\n+\t\tgpg --homedir \"${GNUPGHOME}\" \\\n+\t\t\t--sign -u committer@example.com >/dev/null &&\n+\t\ttest_set_prereq GPG\n \t\t;;\n \tesac\n-fi\n+'\n+\n+test_have_prereq GPG &&\n+test_lazy_prereq GPGSM '\n+\t# Available key info:\n+\t# * see t/lib-gpg/gpgsm-gen-key.in\n+\t# To generate new certificate:\n+\t# * no passphrase\n+\t#\tgpgsm --homedir /tmp/gpghome/ \\\n+\t#\t\t-o /tmp/gpgsm.crt.user \\\n+\t#\t\t--generate-key \\\n+\t#\t\t--batch t/lib-gpg/gpgsm-gen-key.in\n+\t# To import certificate:\n+\t#\tgpgsm --homedir /tmp/gpghome/ \\\n+\t#\t\t--import /tmp/gpgsm.crt.user\n+\t# To export into a .p12 we can later import:\n+\t#\tgpgsm --homedir /tmp/gpghome/ \\\n+\t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n+\t#\t\t--export-secret-key-p12 \"committer@example.com\"\n+\techo | gpgsm --homedir \"${GNUPGHOME}\" \\\n+\t\t--passphrase-fd 0 --pinentry-mode loopback \\\n+\t\t--import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n+\tgpgsm --homedir \"${GNUPGHOME}\" -K |\n+\tgrep fingerprint: |\n+\tcut -d\" \" -f4 |\n+\ttr -d \"\\\\n\" >\"${GNUPGHOME}/trustlist.txt\" &&\n+\techo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n+\techo hello | gpgsm --homedir \"${GNUPGHOME}\" \\\n+\t\t-u committer@example.com -o /dev/null --sign -\n+'\n \n-if test_have_prereq GPG &&\n- echo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null 2>&1\n-then\n-\ttest_set_prereq RFC1991\n-fi\n+test_have_prereq GPG &&\n+test_lazy_prereq RFC1991 '\n+ echo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991\n+'\n \n sanitize_pgp() {\n \tperl -ne '\n"},{"id":"393803","messageId":"xmqq8sjq6fob.fsf@gitster.c.googlers.com","threadId":"53068","inReplyTo":"20200323180445.GA31401@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] tests(gpg): increase verbosity to allow debugging","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-23T19:21:08Z","receivedAt":"2020-03-23T19:21:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Actually, it's pretty straight-forward and I think the resulting code is\n> cleaner. Note that we do have to use a real expect_success because of\n> the side effects. That does increment the test counter. IMHO that's not\n> a big deal, but if we're concerned it wouldn't be too hard to add a\n> non-lazy prereq block.\n>\n> Patch is below (I pulled GPGSM into its own block, which involved\n> reindenting; view with \"-w\").\n\nNice. Certainly a lot nicer than having to reason about what fd#3\nand fd#4 are, which I always have keeping in my head.\n\n> @@ -31,51 +34,51 @@ then\n> \t\tchmod 0700 ./gpghome &&\n> \t\tGNUPGHOME=\"$PWD/gpghome\" &&\n> \t\texport GNUPGHOME &&\n> -\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n> -\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n> +\t\t(gpgconf --kill gpg-agent || : ) &&\n> +\t\tgpg --homedir \"${GNUPGHOME}\" --import \\\n> \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg &&\n> -\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import-ownertrust \\\n> +\t\tgpg --homedir \"${GNUPGHOME}\" --import-ownertrust \\\n> \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n\nGood to see all \"discard output\" go.\n\n> -\t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n> -\t\t\t--sign -u committer@example.com &&\n> -\t\ttest_set_prereq GPG &&\n\n> +\t\tgpg --homedir \"${GNUPGHOME}\" \\\n> +\t\t\t--sign -u committer@example.com >/dev/null &&\n\nWe lost input redirection; I am assuming that it was unnecessary as\nthe standard input of our tests are all redirected from /dev/null\nanyway?\n\nWe are not interested in seeing the signed output (we have nothing\nto compare to validate the correctness anyway), so discarding the\nstandard output is fine. We do not want to see it even while we are\ndebugging, either.\n\nLooking good.\n"},{"id":"393810","messageId":"20200323201547.GA35429@coredump.intra.peff.net","threadId":"53068","inReplyTo":"xmqq8sjq6fob.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] tests(gpg): increase verbosity to allow debugging","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-23T20:15:47Z","receivedAt":"2020-03-23T20:15:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 23, 2020 at 12:21:08PM -0700, Junio C Hamano wrote:\n\n> > +\t\tgpg --homedir \"${GNUPGHOME}\" \\\n> > +\t\t\t--sign -u committer@example.com >/dev/null &&\n> \n> We lost input redirection; I am assuming that it was unnecessary as\n> the standard input of our tests are all redirected from /dev/null\n> anyway?\n\nYes (though it also wouldn't hurt to leave it to explicitly document\nthat we're ignoring it for this command).\n\n> We are not interested in seeing the signed output (we have nothing\n> to compare to validate the correctness anyway), so discarding the\n> standard output is fine. We do not want to see it even while we are\n> debugging, either.\n\nYes, exactly. I originally dropped it, but after looking at the \"-v\"\noutput I saw it was full of signed binary goo. :)\n\n> Looking good.\n\nThere were actually a few subtle breakages in what I posted before. One\nwas just a dumb conversion: I forgot to set $gpg_version correctly. But\nthe other is much trickier: we have to hide our exit codes from\ntest_expect_success, since in the non-GPG case we want it to still\nreport success. So any breakage in the big &&-chain would say \"test\nfailure\", when it should just quietly continue without setting the GPG\nprereq.\n\nHere's what I came up with that I think is suitable for applying (though\nif you find the GNUPGHOME thing below too gross, I can rework it as\nindicated):\n\n-- >8 --\nSubject: [PATCH] t/lib-gpg: run setup code in test blocks\n\nThe steps to check the GPG prereq and set up GNUPGHOME are run in the\nmain script, with stdout and stderr redirected. This avoids spewing\nuseless output when GPG isn't available. But it also means that there's\nno easy way to see what did happen if you're using \"-v\" or \"-x\".\n\nLet's push this as much as possible into a lazy_prereq blocks, which\nhandle verbosity and tracing for us. There's one tricky thing here: part\nof the setup involves setting $GNUPGHOME, but lazy_prereq blocks are\nevaluated in a subshell in order to avoid accidental environment\ncontamination. Splitting the setup from the prereq is tricky; the prereq\nis basically \"did we successfully set things up\".\n\nWe could run all of the GPG prereq code in its own test_expect_success\nblock. But that gets awkward because we _don't_ want to report failure\nif a command fails (we just want to not set the prereq).\n\nI've solved it here by pulling the GNUPGHOME setup into its own separate\nsetup step, that happens _before_ we check the prereq. That means we'd\nset up the variable even if we don't have gpg, but that should be OK;\nwe'll be skipping any gpg tests in that case anyway. (If it's not, the\nalternative is to put the big &&-chain into a separate function of \"{}\"\nblock).\n\nNow that the code is inside test blocks, we can take advantage of this\nto use &&-chaining and early returns, and avoid indenting everything\ninside a big case statement.\n\nSigned-off-by: Jeff King \n---\nOn top of Dscho's patch 1, since it uses $PWD/gpghome.\n\n t/lib-gpg.sh | 145 +++++++++++++++++++++++++++------------------------\n 1 file changed, 76 insertions(+), 69 deletions(-)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex 11b83b8c24..56153b3123 100755\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -1,81 +1,88 @@\n #!/bin/sh\n \n-gpg_version=$(gpg --version 2>&1)\n-if test $? != 127\n-then\n+# This can't run as part of the lazy_prereq below because it has the side\n+# effect of setting an environment variable.\n+test_expect_success 'set up GNUPGHOME' '\n+\tmkdir ./gpghome &&\n+\tchmod 0700 ./gpghome &&\n+\tGNUPGHOME=\"$PWD/gpghome\" &&\n+\texport GNUPGHOME\n+'\n+\n+test_lazy_prereq GPG '\n+\t{\n+\t\tgpg_version=$(gpg --version)\n+\t\ttest $? != 127\n+\t} &&\n+\n \t# As said here: http://www.gnupg.org/documentation/faqs.html#q6.19\n-\t# the gpg version 1.0.6 didn't parse trust packets correctly, so for\n+\t# the gpg version 1.0.6 did not parse trust packets correctly, so for\n \t# that version, creation of signed tags using the generated key fails.\n \tcase \"$gpg_version\" in\n-\t'gpg (GnuPG) 1.0.6'*)\n-\t\tsay \"Your version of gpg (1.0.6) is too buggy for testing\"\n+\t\t\"gpg (GnuPG) 1.0.6\"*)\n+\t\techo >&2 \"Your version of gpg (1.0.6) is too buggy for testing\"\n+\t\treturn 1\n \t\t;;\n-\t*)\n-\t\t# Available key info:\n-\t\t# * Type DSA and Elgamal, size 2048 bits, no expiration date,\n-\t\t# name and email: C O Mitter \n-\t\t# * Type RSA, size 2048 bits, no expiration date,\n-\t\t# name and email: Eris Discordia \n-\t\t# No password given, to enable non-interactive operation.\n-\t\t# To generate new key:\n-\t\t#\tgpg --homedir /tmp/gpghome --gen-key\n-\t\t# To write armored exported key to keyring:\n-\t\t#\tgpg --homedir /tmp/gpghome --export-secret-keys \\\n-\t\t#\t\t--armor 0xDEADBEEF >> lib-gpg/keyring.gpg\n-\t\t#\tgpg --homedir /tmp/gpghome --export \\\n-\t\t#\t\t--armor 0xDEADBEEF >> lib-gpg/keyring.gpg\n-\t\t# To export ownertrust:\n-\t\t#\tgpg --homedir /tmp/gpghome --export-ownertrust \\\n-\t\t#\t\t> lib-gpg/ownertrust\n-\t\tmkdir ./gpghome &&\n-\t\tchmod 0700 ./gpghome &&\n-\t\tGNUPGHOME=\"$PWD/gpghome\" &&\n-\t\texport GNUPGHOME &&\n-\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n-\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n-\t\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg &&\n-\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import-ownertrust \\\n-\t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n-\t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n-\t\t\t--sign -u committer@example.com &&\n-\t\ttest_set_prereq GPG &&\n-\t\t# Available key info:\n-\t\t# * see t/lib-gpg/gpgsm-gen-key.in\n-\t\t# To generate new certificate:\n-\t\t# * no passphrase\n-\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n-\t\t#\t\t-o /tmp/gpgsm.crt.user \\\n-\t\t#\t\t--generate-key \\\n-\t\t#\t\t--batch t/lib-gpg/gpgsm-gen-key.in\n-\t\t# To import certificate:\n-\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n-\t\t#\t\t--import /tmp/gpgsm.crt.user\n-\t\t# To export into a .p12 we can later import:\n-\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n-\t\t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n-\t\t#\t\t--export-secret-key-p12 \"committer@example.com\"\n-\t\techo | gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null \\\n-\t\t\t--passphrase-fd 0 --pinentry-mode loopback \\\n-\t\t\t--import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n+\tesac &&\n \n-\t\tgpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null -K |\n-\t\tgrep fingerprint: |\n-\t\tcut -d\" \" -f4 |\n-\t\ttr -d '\\n' >\"${GNUPGHOME}/trustlist.txt\" &&\n+\t# Available key info:\n+\t# * Type DSA and Elgamal, size 2048 bits, no expiration date,\n+\t# name and email: C O Mitter \n+\t# * Type RSA, size 2048 bits, no expiration date,\n+\t# name and email: Eris Discordia \n+\t# No password given, to enable non-interactive operation.\n+\t# To generate new key:\n+\t#\tgpg --homedir /tmp/gpghome --gen-key\n+\t# To write armored exported key to keyring:\n+\t#\tgpg --homedir /tmp/gpghome --export-secret-keys \\\n+\t#\t\t--armor 0xDEADBEEF >> lib-gpg/keyring.gpg\n+\t#\tgpg --homedir /tmp/gpghome --export \\\n+\t#\t\t--armor 0xDEADBEEF >> lib-gpg/keyring.gpg\n+\t# To export ownertrust:\n+\t#\tgpg --homedir /tmp/gpghome --export-ownertrust \\\n+\t#\t\t> lib-gpg/ownertrust\n+\t(gpgconf --kill gpg-agent || : ) &&\n+\tgpg --homedir \"${GNUPGHOME}\" --import \\\n+\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg &&\n+\tgpg --homedir \"${GNUPGHOME}\" --import-ownertrust \\\n+\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n+\tgpg --homedir \"${GNUPGHOME}\" \\\n+\t\t--sign -u committer@example.com >/dev/null\n+'\n \n-\t\techo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n-\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" >/dev/null \\\n-\t\t\t-u committer@example.com -o /dev/null --sign - 2>&1 &&\n-\t\ttest_set_prereq GPGSM\n-\t\t;;\n-\tesac\n-fi\n+test_have_prereq GPG &&\n+test_lazy_prereq GPGSM '\n+\t# Available key info:\n+\t# * see t/lib-gpg/gpgsm-gen-key.in\n+\t# To generate new certificate:\n+\t# * no passphrase\n+\t#\tgpgsm --homedir /tmp/gpghome/ \\\n+\t#\t\t-o /tmp/gpgsm.crt.user \\\n+\t#\t\t--generate-key \\\n+\t#\t\t--batch t/lib-gpg/gpgsm-gen-key.in\n+\t# To import certificate:\n+\t#\tgpgsm --homedir /tmp/gpghome/ \\\n+\t#\t\t--import /tmp/gpgsm.crt.user\n+\t# To export into a .p12 we can later import:\n+\t#\tgpgsm --homedir /tmp/gpghome/ \\\n+\t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n+\t#\t\t--export-secret-key-p12 \"committer@example.com\"\n+\techo | gpgsm --homedir \"${GNUPGHOME}\" \\\n+\t\t--passphrase-fd 0 --pinentry-mode loopback \\\n+\t\t--import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n+\tgpgsm --homedir \"${GNUPGHOME}\" -K |\n+\tgrep fingerprint: |\n+\tcut -d\" \" -f4 |\n+\ttr -d \"\\\\n\" >\"${GNUPGHOME}/trustlist.txt\" &&\n+\techo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n+\techo hello | gpgsm --homedir \"${GNUPGHOME}\" \\\n+\t\t-u committer@example.com -o /dev/null --sign -\n+'\n \n-if test_have_prereq GPG &&\n- echo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null 2>&1\n-then\n-\ttest_set_prereq RFC1991\n-fi\n+test_have_prereq GPG &&\n+test_lazy_prereq RFC1991 '\n+ echo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991\n+'\n \n sanitize_pgp() {\n \tperl -ne '\n-- \n2.26.0.561.g096102dc45\n\n"},{"id":"393824","messageId":"xmqqzhc63gmd.fsf@gitster.c.googlers.com","threadId":"53068","inReplyTo":"20200323201547.GA35429@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] tests(gpg): increase verbosity to allow debugging","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-23T21:28:58Z","receivedAt":"2020-03-23T21:29:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Here's what I came up with that I think is suitable for applying (though\n> if you find the GNUPGHOME thing below too gross, I can rework it as\n> indicated):\n\nI actually think it is perfectly fine to mkdir and set the\nenvironment even outside test_expect_success; that way, even\nGIT_SKIP_TESTS cannot omit the necessary initialization. And as you\nsaid, leaving the environment pointing into the trash repository's\nworking tree should be fine when we fail the GPG prereq. We\nshouldn't be running GPG at all in such a case.\n\n> -- >8 --\n> Subject: [PATCH] t/lib-gpg: run setup code in test blocks\n>\n> The steps to check the GPG prereq and set up GNUPGHOME are run in the\n> main script, with stdout and stderr redirected. This avoids spewing\n> useless output when GPG isn't available. But it also means that there's\n> no easy way to see what did happen if you're using \"-v\" or \"-x\".\n>\n> Let's push this as much as possible into a lazy_prereq blocks, which\n> handle verbosity and tracing for us. There's one tricky thing here: part\n> of the setup involves setting $GNUPGHOME, but lazy_prereq blocks are\n> evaluated in a subshell in order to avoid accidental environment\n> contamination. Splitting the setup from the prereq is tricky; the prereq\n> is basically \"did we successfully set things up\".\n>\n> We could run all of the GPG prereq code in its own test_expect_success\n> block. But that gets awkward because we _don't_ want to report failure\n> if a command fails (we just want to not set the prereq).\n>\n> I've solved it here by pulling the GNUPGHOME setup into its own separate\n> setup step, that happens _before_ we check the prereq. That means we'd\n> set up the variable even if we don't have gpg, but that should be OK;\n> we'll be skipping any gpg tests in that case anyway. (If it's not, the\n> alternative is to put the big &&-chain into a separate function of \"{}\"\n> block).\n>\n> Now that the code is inside test blocks, we can take advantage of this\n> to use &&-chaining and early returns, and avoid indenting everything\n> inside a big case statement.\n>\n> Signed-off-by: Jeff King \n> ---\n> On top of Dscho's patch 1, since it uses $PWD/gpghome.\n\nLooking good.\n\n> t/lib-gpg.sh | 145 +++++++++++++++++++++++++++------------------------\n> 1 file changed, 76 insertions(+), 69 deletions(-)\n>\n> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\n> index 11b83b8c24..56153b3123 100755\n> --- a/t/lib-gpg.sh\n> +++ b/t/lib-gpg.sh\n> @@ -1,81 +1,88 @@\n> #!/bin/sh\n> \n> -gpg_version=$(gpg --version 2>&1)\n> -if test $? != 127\n> -then\n> +# This can't run as part of the lazy_prereq below because it has the side\n> +# effect of setting an environment variable.\n> +test_expect_success 'set up GNUPGHOME' '\n> +\tmkdir ./gpghome &&\n> +\tchmod 0700 ./gpghome &&\n> +\tGNUPGHOME=\"$PWD/gpghome\" &&\n> +\texport GNUPGHOME\n> +'\n> +\n> +test_lazy_prereq GPG '\n> +\t{\n> +\t\tgpg_version=$(gpg --version)\n> +\t\ttest $? != 127\n> +\t} &&\n> +\n> \t# As said here: http://www.gnupg.org/documentation/faqs.html#q6.19\n> -\t# the gpg version 1.0.6 didn't parse trust packets correctly, so for\n> +\t# the gpg version 1.0.6 did not parse trust packets correctly, so for\n> \t# that version, creation of signed tags using the generated key fails.\n> \tcase \"$gpg_version\" in\n> -\t'gpg (GnuPG) 1.0.6'*)\n> -\t\tsay \"Your version of gpg (1.0.6) is too buggy for testing\"\n> +\t\t\"gpg (GnuPG) 1.0.6\"*)\n> +\t\techo >&2 \"Your version of gpg (1.0.6) is too buggy for testing\"\n> +\t\treturn 1\n> \t\t;;\n> -\t*)\n> -\t\t# Available key info:\n> -\t\t# * Type DSA and Elgamal, size 2048 bits, no expiration date,\n> -\t\t# name and email: C O Mitter \n> -\t\t# * Type RSA, size 2048 bits, no expiration date,\n> -\t\t# name and email: Eris Discordia \n> -\t\t# No password given, to enable non-interactive operation.\n> -\t\t# To generate new key:\n> -\t\t#\tgpg --homedir /tmp/gpghome --gen-key\n> -\t\t# To write armored exported key to keyring:\n> -\t\t#\tgpg --homedir /tmp/gpghome --export-secret-keys \\\n> -\t\t#\t\t--armor 0xDEADBEEF >> lib-gpg/keyring.gpg\n> -\t\t#\tgpg --homedir /tmp/gpghome --export \\\n> -\t\t#\t\t--armor 0xDEADBEEF >> lib-gpg/keyring.gpg\n> -\t\t# To export ownertrust:\n> -\t\t#\tgpg --homedir /tmp/gpghome --export-ownertrust \\\n> -\t\t#\t\t> lib-gpg/ownertrust\n> -\t\tmkdir ./gpghome &&\n> -\t\tchmod 0700 ./gpghome &&\n> -\t\tGNUPGHOME=\"$PWD/gpghome\" &&\n> -\t\texport GNUPGHOME &&\n> -\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n> -\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n> -\t\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg &&\n> -\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import-ownertrust \\\n> -\t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n> -\t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n> -\t\t\t--sign -u committer@example.com &&\n> -\t\ttest_set_prereq GPG &&\n> -\t\t# Available key info:\n> -\t\t# * see t/lib-gpg/gpgsm-gen-key.in\n> -\t\t# To generate new certificate:\n> -\t\t# * no passphrase\n> -\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n> -\t\t#\t\t-o /tmp/gpgsm.crt.user \\\n> -\t\t#\t\t--generate-key \\\n> -\t\t#\t\t--batch t/lib-gpg/gpgsm-gen-key.in\n> -\t\t# To import certificate:\n> -\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n> -\t\t#\t\t--import /tmp/gpgsm.crt.user\n> -\t\t# To export into a .p12 we can later import:\n> -\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n> -\t\t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n> -\t\t#\t\t--export-secret-key-p12 \"committer@example.com\"\n> -\t\techo | gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null \\\n> -\t\t\t--passphrase-fd 0 --pinentry-mode loopback \\\n> -\t\t\t--import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n> +\tesac &&\n> \n> -\t\tgpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null -K |\n> -\t\tgrep fingerprint: |\n> -\t\tcut -d\" \" -f4 |\n> -\t\ttr -d '\\n' >\"${GNUPGHOME}/trustlist.txt\" &&\n> +\t# Available key info:\n> +\t# * Type DSA and Elgamal, size 2048 bits, no expiration date,\n> +\t# name and email: C O Mitter \n> +\t# * Type RSA, size 2048 bits, no expiration date,\n> +\t# name and email: Eris Discordia \n> +\t# No password given, to enable non-interactive operation.\n> +\t# To generate new key:\n> +\t#\tgpg --homedir /tmp/gpghome --gen-key\n> +\t# To write armored exported key to keyring:\n> +\t#\tgpg --homedir /tmp/gpghome --export-secret-keys \\\n> +\t#\t\t--armor 0xDEADBEEF >> lib-gpg/keyring.gpg\n> +\t#\tgpg --homedir /tmp/gpghome --export \\\n> +\t#\t\t--armor 0xDEADBEEF >> lib-gpg/keyring.gpg\n> +\t# To export ownertrust:\n> +\t#\tgpg --homedir /tmp/gpghome --export-ownertrust \\\n> +\t#\t\t> lib-gpg/ownertrust\n> +\t(gpgconf --kill gpg-agent || : ) &&\n> +\tgpg --homedir \"${GNUPGHOME}\" --import \\\n> +\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg &&\n> +\tgpg --homedir \"${GNUPGHOME}\" --import-ownertrust \\\n> +\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n> +\tgpg --homedir \"${GNUPGHOME}\" \\\n> +\t\t--sign -u committer@example.com >/dev/null\n> +'\n> \n> -\t\techo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n> -\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" >/dev/null \\\n> -\t\t\t-u committer@example.com -o /dev/null --sign - 2>&1 &&\n> -\t\ttest_set_prereq GPGSM\n> -\t\t;;\n> -\tesac\n> -fi\n> +test_have_prereq GPG &&\n> +test_lazy_prereq GPGSM '\n> +\t# Available key info:\n> +\t# * see t/lib-gpg/gpgsm-gen-key.in\n> +\t# To generate new certificate:\n> +\t# * no passphrase\n> +\t#\tgpgsm --homedir /tmp/gpghome/ \\\n> +\t#\t\t-o /tmp/gpgsm.crt.user \\\n> +\t#\t\t--generate-key \\\n> +\t#\t\t--batch t/lib-gpg/gpgsm-gen-key.in\n> +\t# To import certificate:\n> +\t#\tgpgsm --homedir /tmp/gpghome/ \\\n> +\t#\t\t--import /tmp/gpgsm.crt.user\n> +\t# To export into a .p12 we can later import:\n> +\t#\tgpgsm --homedir /tmp/gpghome/ \\\n> +\t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n> +\t#\t\t--export-secret-key-p12 \"committer@example.com\"\n> +\techo | gpgsm --homedir \"${GNUPGHOME}\" \\\n> +\t\t--passphrase-fd 0 --pinentry-mode loopback \\\n> +\t\t--import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n> +\tgpgsm --homedir \"${GNUPGHOME}\" -K |\n> +\tgrep fingerprint: |\n> +\tcut -d\" \" -f4 |\n> +\ttr -d \"\\\\n\" >\"${GNUPGHOME}/trustlist.txt\" &&\n> +\techo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n> +\techo hello | gpgsm --homedir \"${GNUPGHOME}\" \\\n> +\t\t-u committer@example.com -o /dev/null --sign -\n> +'\n> \n> -if test_have_prereq GPG &&\n> - echo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null 2>&1\n> -then\n> -\ttest_set_prereq RFC1991\n> -fi\n> +test_have_prereq GPG &&\n> +test_lazy_prereq RFC1991 '\n> + echo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991\n> +'\n> \n> sanitize_pgp() {\n> \tperl -ne '\n"},{"id":"393825","messageId":"20200323213109.GA44297@coredump.intra.peff.net","threadId":"53068","inReplyTo":"xmqqzhc63gmd.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 2/2] tests(gpg): increase verbosity to allow debugging","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-23T21:31:09Z","receivedAt":"2020-03-23T21:31:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 23, 2020 at 02:28:58PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Here's what I came up with that I think is suitable for applying (though\n> > if you find the GNUPGHOME thing below too gross, I can rework it as\n> > indicated):\n> \n> I actually think it is perfectly fine to mkdir and set the\n> environment even outside test_expect_success; that way, even\n> GIT_SKIP_TESTS cannot omit the necessary initialization. And as you\n> said, leaving the environment pointing into the trash repository's\n> working tree should be fine when we fail the GPG prereq. We\n> shouldn't be running GPG at all in such a case.\n\nI have a slight preference to do it in an expect_success block, because\nthen we'd notice the error more readily (and it gets the benefit\nverbosity and tracing, too!).\n\nThe thing I was more worried about is that it's technically a behavior\nchange to set up GNUPGHOME when we're not going to use it (as well as\ncreate the directory). But I find it hard to imagine a test that would\nbe affected where my suggested solution wouldn't be \"fix the test\".\n\n-Peff\n"},{"id":"393890","messageId":"nycvar.QRO.7.76.6.2003242043050.46@tvgsbejvaqbjf.bet","threadId":"53068","inReplyTo":"xmqqd09355h7.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] tests(gpg): allow the gpg-agent to start on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-03-24T19:55:04Z","receivedAt":"2020-03-24T19:55:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 23 Mar 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > In Git for Windows' SDK, we use the MSYS2 version of OpenSSH, meaning\n> > that the `gpg-agent` will fail horribly when being passed a `--homedir`\n> > that contains colons.\n> >\n> > Previously, we did pass the Windows version of the absolute path,\n> > though, which starts in the drive letter followed by, you guessed it, a\n> > colon.\n> >\n> > Let's use the same trick found elsewhere in our test suite where `$PWD`\n> > is used to refer to the pseudo-Unix path (which works only within the\n> > MSYS2 Bash/OpenSSH/Perl/etc, as opposed to `$(pwd)` which refers to the\n> > Windows path that `git.exe` understands, too).\n>\n> Makes sense.\n>\n> Do we have a short/concise instruction, e.g. \"You should use $(pwd)\n> in most cases, but for such and such purposes use $PWD instead\", in\n> t/README for test writers, who are not familiar with the distinction\n> between $(pwd) and $PWD, to help them decide which one to use in\n> what situation? I see this kind of fix-ups from time to time, and\n> am wondering if there is a way to reduce the need for you or J6t to\n> spot and fix the new ones.\n\nI fear that this distinction really is lost on anybody who does not have\nto deal with MSYS2 on Windows.\n\nIt is subtle enough a distinction, too: whenever Bash or Perl is\nconcerned, we _might_ run into this issue. I say _might_ because _some_\nscripts actually handle Windows paths correctly, but others don't (testing\nfor absolute paths by looking for a slash at the beginning would be an\nexample, and it gets really hairy when you slap Windows paths at the end\nof `PATH`, separated by, you guessed it, a colon).\n\nIt gets even worse: you might think that you have to use `$(pwd)` when\npassing the path to `git.exe` (because it is a non-MSYS2 program). But you\ndon't, in many cases. For example, when you call\n\n\tgit config my.pwd \"$PWD\"\n\nit totally works (because the MSYS2 runtime on top of which Bash runs will\nconvert the parameters that are passed to a non-MSYS2 program when they\nlook like paths).\n\nThe problem solved by _this here_ patch is the opposite, of course: we are\npassing a Windows path (`$(pwd)` implicitly calls `$(pwd -W)` in our test\nsuite on Windows) to an _MSYS2_ program: `gpg.exe`. And how would\ncontributors whose main development platform isn't Windows be able to\nguess that `gpg.exe` is an MSYS2 program as opposed to, say, `tclsh.exe`\nwhich is a non-MSYS2 program? They wouldn't.\n\nIn short: I am convinced that this is a subtlety in our test suite that we\ncannot reasonably expect any contributors other than Windows-based ones to\nget right, and I am fairly certain that we will just have to keep\nmonitoring the CI/PR builds for similar issues and then help the\ncontributors by suggesting the appropriate fixes.\n\nCiao,\nDscho\n\n> Thanks.\n>\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > t/lib-gpg.sh | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\n> > index 8d28652b729..11b83b8c24a 100755\n> > --- a/t/lib-gpg.sh\n> > +++ b/t/lib-gpg.sh\n> > @@ -29,7 +29,7 @@ then\n> > \t\t#\t\t> lib-gpg/ownertrust\n> > \t\tmkdir ./gpghome &&\n> > \t\tchmod 0700 ./gpghome &&\n> > -\t\tGNUPGHOME=\"$(pwd)/gpghome\" &&\n> > +\t\tGNUPGHOME=\"$PWD/gpghome\" &&\n> > \t\texport GNUPGHOME &&\n> > \t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n> > \t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n>\n"},{"id":"393894","messageId":"xmqqh7yd1nbh.fsf@gitster.c.googlers.com","threadId":"53068","inReplyTo":"nycvar.QRO.7.76.6.2003242043050.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/2] tests(gpg): allow the gpg-agent to start on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-24T20:59:30Z","receivedAt":"2020-03-24T20:59:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I fear that this distinction really is lost on anybody who does not have\n> to deal with MSYS2 on Windows.\n> ...\n> In short: I am convinced that this is a subtlety in our test suite that we\n> cannot reasonably expect any contributors other than Windows-based ones to\n> get right, and I am fairly certain that we will just have to keep\n> monitoring the CI/PR builds for similar issues and then help the\n> contributors by suggesting the appropriate fixes.\n\nIOW, this cannot be made an engineering problem and has to stay a\nblack art? ;-)\n\nThat's somewhat sad, but I guess we have to live with it.\n\nThanks.\n\n"},{"id":"393898","messageId":"nycvar.QRO.7.76.6.2003242239300.46@tvgsbejvaqbjf.bet","threadId":"53068","inReplyTo":"20200323213109.GA44297@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] tests(gpg): increase verbosity to allow debugging","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-03-24T21:41:58Z","receivedAt":"2020-03-24T21:42:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 23 Mar 2020, Jeff King wrote:\n\n> On Mon, Mar 23, 2020 at 02:28:58PM -0700, Junio C Hamano wrote:\n>\n> > Jeff King writes:\n> >\n> > > Here's what I came up with that I think is suitable for applying (though\n> > > if you find the GNUPGHOME thing below too gross, I can rework it as\n> > > indicated):\n> >\n> > I actually think it is perfectly fine to mkdir and set the\n> > environment even outside test_expect_success; that way, even\n> > GIT_SKIP_TESTS cannot omit the necessary initialization. And as you\n> > said, leaving the environment pointing into the trash repository's\n> > working tree should be fine when we fail the GPG prereq. We\n> > shouldn't be running GPG at all in such a case.\n>\n> I have a slight preference to do it in an expect_success block, because\n> then we'd notice the error more readily (and it gets the benefit\n> verbosity and tracing, too!).\n>\n> The thing I was more worried about is that it's technically a behavior\n> change to set up GNUPGHOME when we're not going to use it (as well as\n> create the directory). But I find it hard to imagine a test that would\n> be affected where my suggested solution wouldn't be \"fix the test\".\n\nIt is _half_ a change in behavior: in case that `gpg` was found, and does\nnot have a known-bad version, we set up the environment variable, _even\nif_ the test-signing fails. In other words, we don't roll back the\nenvironment variable.\n\nAs such, I figure that setting it globally _before_ even evaluating the\nprereq is okay.\n\nTherefore, it is relatively easy to turn this thing into a set of lazy\nprereqs, which is better, conceptually, I think. I am in the process of\nmaking it so.\n\nCiao,\nDscho\n"},{"id":"393899","messageId":"20200324220532.GA647727@coredump.intra.peff.net","threadId":"53068","inReplyTo":"nycvar.QRO.7.76.6.2003242239300.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] tests(gpg): increase verbosity to allow debugging","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-24T22:05:32Z","receivedAt":"2020-03-24T22:05:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 24, 2020 at 10:41:58PM +0100, Johannes Schindelin wrote:\n\n> > The thing I was more worried about is that it's technically a behavior\n> > change to set up GNUPGHOME when we're not going to use it (as well as\n> > create the directory). But I find it hard to imagine a test that would\n> > be affected where my suggested solution wouldn't be \"fix the test\".\n> \n> It is _half_ a change in behavior: in case that `gpg` was found, and does\n> not have a known-bad version, we set up the environment variable, _even\n> if_ the test-signing fails. In other words, we don't roll back the\n> environment variable.\n> \n> As such, I figure that setting it globally _before_ even evaluating the\n> prereq is okay.\n> \n> Therefore, it is relatively easy to turn this thing into a set of lazy\n> prereqs, which is better, conceptually, I think. I am in the process of\n> making it so.\n\nEr, isn't that what my patch did? I'm fine if you have another approach\nto present, but I'm worried we might be duplicating effort.\n\n-Peff\n"},{"id":"393900","messageId":"nycvar.QRO.7.76.6.2003242323120.46@tvgsbejvaqbjf.bet","threadId":"53068","inReplyTo":"20200324220532.GA647727@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] tests(gpg): increase verbosity to allow debugging","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-03-24T22:25:09Z","receivedAt":"2020-03-24T22:25:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 24 Mar 2020, Jeff King wrote:\n\n> On Tue, Mar 24, 2020 at 10:41:58PM +0100, Johannes Schindelin wrote:\n>\n> > > The thing I was more worried about is that it's technically a behavior\n> > > change to set up GNUPGHOME when we're not going to use it (as well as\n> > > create the directory). But I find it hard to imagine a test that would\n> > > be affected where my suggested solution wouldn't be \"fix the test\".\n> >\n> > It is _half_ a change in behavior: in case that `gpg` was found, and does\n> > not have a known-bad version, we set up the environment variable, _even\n> > if_ the test-signing fails. In other words, we don't roll back the\n> > environment variable.\n> >\n> > As such, I figure that setting it globally _before_ even evaluating the\n> > prereq is okay.\n> >\n> > Therefore, it is relatively easy to turn this thing into a set of lazy\n> > prereqs, which is better, conceptually, I think. I am in the process of\n> > making it so.\n>\n> Er, isn't that what my patch did? I'm fine if you have another approach\n> to present, but I'm worried we might be duplicating effort.\n\nI missed that your second patch made `GPG` lazy, too.\n\nMy version is slightly different from yours, though: I do not insist on\nsetting the environment variable `GNUPGHOME` only after the `mkdir`\nsucceeds, as the `gpg --sign` later on might fail anyway, which means that\nwe _already_ could end up with `GNUPGHOME` set and the prereq `GPG` _not_\nset.\n\nCiao,\nDscho\n"},{"id":"393901","messageId":"nycvar.QRO.7.76.6.2003242326190.46@tvgsbejvaqbjf.bet","threadId":"53068","inReplyTo":"xmqqh7yd1nbh.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] tests(gpg): allow the gpg-agent to start on Windows","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-03-24T22:26:36Z","receivedAt":"2020-03-24T22:26:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 24 Mar 2020, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > I fear that this distinction really is lost on anybody who does not have\n> > to deal with MSYS2 on Windows.\n> > ...\n> > In short: I am convinced that this is a subtlety in our test suite that we\n> > cannot reasonably expect any contributors other than Windows-based ones to\n> > get right, and I am fairly certain that we will just have to keep\n> > monitoring the CI/PR builds for similar issues and then help the\n> > contributors by suggesting the appropriate fixes.\n>\n> IOW, this cannot be made an engineering problem and has to stay a\n> black art? ;-)\n>\n> That's somewhat sad, but I guess we have to live with it.\n\nI am afraid so.\n\nCiao,\nDscho\n\n>\n> Thanks.\n>\n>\n>\n"},{"id":"393904","messageId":"20200324223307.GA648188@coredump.intra.peff.net","threadId":"53068","inReplyTo":"nycvar.QRO.7.76.6.2003242323120.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 2/2] tests(gpg): increase verbosity to allow debugging","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-24T22:33:07Z","receivedAt":"2020-03-24T22:33:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 24, 2020 at 11:25:09PM +0100, Johannes Schindelin wrote:\n\n> > Er, isn't that what my patch did? I'm fine if you have another approach\n> > to present, but I'm worried we might be duplicating effort.\n> \n> I missed that your second patch made `GPG` lazy, too.\n> \n> My version is slightly different from yours, though: I do not insist on\n> setting the environment variable `GNUPGHOME` only after the `mkdir`\n> succeeds, as the `gpg --sign` later on might fail anyway, which means that\n> we _already_ could end up with `GNUPGHOME` set and the prereq `GPG` _not_\n> set.\n\nYes. That mkdir could also be pushed down into the GPG prereq for the\nsame reason. It's pretty unlikely the mkdir would fail, and I thought it\nwould be less confusing (in the off chance that anybody even looks at it\nwhen GPG isn't set) if we had a GNUPGHOME that pointed to an _actual_\ndirectory, instead of something that didn't exist.\n\nBut that's kind of an arbitrary cutoff. The GPG prereq is also importing\nkeys and other stuff, so the state of that directory when GPG isn't set\nis undefined (but again, nobody's supposed to be looking at it, so...).\n\n-Peff\n"},{"id":"393910","messageId":"xmqqo8slz5hx.fsf@gitster.c.googlers.com","threadId":"53068","inReplyTo":"nycvar.QRO.7.76.6.2003242326190.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 1/2] tests(gpg): allow the gpg-agent to start on Windows","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-24T23:40:26Z","receivedAt":"2020-03-24T23:40:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hi Junio,\n>\n> On Tue, 24 Mar 2020, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>>\n>> > I fear that this distinction really is lost on anybody who does not have\n>> > to deal with MSYS2 on Windows.\n>> > ...\n>> > In short: I am convinced that this is a subtlety in our test suite that we\n>> > cannot reasonably expect any contributors other than Windows-based ones to\n>> > get right, and I am fairly certain that we will just have to keep\n>> > monitoring the CI/PR builds for similar issues and then help the\n>> > contributors by suggesting the appropriate fixes.\n>>\n>> IOW, this cannot be made an engineering problem and has to stay a\n>> black art? ;-)\n>>\n>> That's somewhat sad, but I guess we have to live with it.\n>\n> I am afraid so.\n\nAs long as there are our users on Windows, we can trust that we'd\nhave some folks who are as familiar as you are with these\nsubtleties, even if you may get bored working on it yourself,\nleaving us with the right bus factor, right? ;-)\n\nThanks.\n"},{"id":"393923","messageId":"287a21f1033b2a74420029c529ad4db956051a85.1585114881.git.gitgitgadget@gmail.com","threadId":"53068","inReplyTo":"pull.728.v2.git.git.1585114881.gitgitgadget@gmail.com","subject":"[PATCH v2 1/5] tests(gpg): allow the gpg-agent to start on Windows","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-25T05:41:17Z","receivedAt":"2020-03-25T05:41:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn Git for Windows' SDK, we use the MSYS2 version of OpenSSH, meaning\nthat the `gpg-agent` will fail horribly when being passed a `--homedir`\nthat contains colons.\n\nPreviously, we did pass the Windows version of the absolute path,\nthough, which starts in the drive letter followed by, you guessed it, a\ncolon.\n\nLet's use the same trick found elsewhere in our test suite where `$PWD`\nis used to refer to the pseudo-Unix path (which works only within the\nMSYS2 Bash/OpenSSH/Perl/etc, as opposed to `$(pwd)` which refers to the\nWindows path that `git.exe` understands, too).\n\nSigned-off-by: Johannes Schindelin \n---\n t/lib-gpg.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex 8d28652b729..11b83b8c24a 100755\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -29,7 +29,7 @@ then\n \t\t#\t\t> lib-gpg/ownertrust\n \t\tmkdir ./gpghome &&\n \t\tchmod 0700 ./gpghome &&\n-\t\tGNUPGHOME=\"$(pwd)/gpghome\" &&\n+\t\tGNUPGHOME=\"$PWD/gpghome\" &&\n \t\texport GNUPGHOME &&\n \t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n \t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n-- \ngitgitgadget\n\n"},{"id":"393924","messageId":"c1811d54190a62e3d35505e2b806c89238b56de8.1585114881.git.gitgitgadget@gmail.com","threadId":"53068","inReplyTo":"pull.728.v2.git.git.1585114881.gitgitgadget@gmail.com","subject":"[PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-25T05:41:18Z","receivedAt":"2020-03-25T05:41:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIt makes no sense to call `./lib-gpg.sh`. Therefore the hash-bang line\nis unnecessary.\n\nSigned-off-by: Johannes Schindelin \n---\n t/lib-gpg.sh | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex 11b83b8c24a..4ead1268351 100755\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -1,5 +1,3 @@\n-#!/bin/sh\n-\n gpg_version=$(gpg --version 2>&1)\n if test $? != 127\n then\n-- \ngitgitgadget\n\n"},{"id":"393925","messageId":"85457a7b61874e8e9f3af9c231451df0aba7a7b5.1585114881.git.gitgitgadget@gmail.com","threadId":"53068","inReplyTo":"pull.728.v2.git.git.1585114881.gitgitgadget@gmail.com","subject":"[PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-25T05:41:19Z","receivedAt":"2020-03-25T05:41:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe code to set those prereqs is executed completely outside of any\n`test_eval_` block. As a consequence, its output had to be suppressed so\nthat it does not clutter the output of a regular test script run.\n\nUnfortunately, the output *stays* suppressed even when the `--verbose`\noption is in effect.\n\nThis hid important output when debugging why the GPG prereq was not\nenabled in the Windows part of our CI builds.\n\nIn preparation for fixing that, let's move all of this code into lazy\nprereqs.\n\nThe only slightly tricky part is the global environment variable\n`GNUPGHOME`. Originally, it was configured only when we verified that\nthere is a `gpg` in the `PATH` that we can use. This is now no longer\npossible, as lazy prereqs are evaluated in a subshell that changes the\nworking directory to a temporary one. Therefore, we simply _always_ set\nthat environment variable: it does not hurt anything because it does not\nindicate the presence of a working GPG.\n\nSide note: it was quite tempting to use a hack that is possible because\nwe do not validate what is passed to `test_lazy_prereq` (and it is\ntherefore possible to \"break out\" of the lazy_prereq subshell:\n\n\ttest_lazy_prereq GPG '...) && GNUPGHOME=... && (...'\n\nHowever, this is rather tricksy hobbitses code, and the current patch is\n_much_ easier to understand.\n\nSigned-off-by: Johannes Schindelin \n---\n t/lib-gpg.sh | 102 ++++++++++++++++++++++++++++-----------------------\n 1 file changed, 57 insertions(+), 45 deletions(-)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex 4ead1268351..7a78c562e8d 100755\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -1,12 +1,25 @@\n-gpg_version=$(gpg --version 2>&1)\n-if test $? != 127\n-then\n+# We always set GNUPGHOME, even if no usable GPG was found, as\n+#\n+# - It does not hurt, and\n+#\n+# - we cannot set global environment variables in lazy prereqs because they are\n+# executed in an eval'ed subshell that changes the working directory to a\n+# temporary one.\n+\n+GNUPGHOME=\"$PWD/gpghome\"\n+export GNUPGHOME\n+\n+test_lazy_prereq GPG '\n+\tgpg_version=$(gpg --version 2>&1)\n+\ttest $? != 127 || exit 1\n+\n \t# As said here: http://www.gnupg.org/documentation/faqs.html#q6.19\n-\t# the gpg version 1.0.6 didn't parse trust packets correctly, so for\n+\t# the gpg version 1.0.6 did not parse trust packets correctly, so for\n \t# that version, creation of signed tags using the generated key fails.\n \tcase \"$gpg_version\" in\n-\t'gpg (GnuPG) 1.0.6'*)\n+\t\"gpg (GnuPG) 1.0.6\"*)\n \t\tsay \"Your version of gpg (1.0.6) is too buggy for testing\"\n+\t\texit 1\n \t\t;;\n \t*)\n \t\t# Available key info:\n@@ -25,55 +38,54 @@ then\n \t\t# To export ownertrust:\n \t\t#\tgpg --homedir /tmp/gpghome --export-ownertrust \\\n \t\t#\t\t> lib-gpg/ownertrust\n-\t\tmkdir ./gpghome &&\n-\t\tchmod 0700 ./gpghome &&\n-\t\tGNUPGHOME=\"$PWD/gpghome\" &&\n-\t\texport GNUPGHOME &&\n+\t\tmkdir \"$GNUPGHOME\" &&\n+\t\tchmod 0700 \"$GNUPGHOME\" &&\n \t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n \t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg &&\n \t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import-ownertrust \\\n \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n \t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n-\t\t\t--sign -u committer@example.com &&\n-\t\ttest_set_prereq GPG &&\n-\t\t# Available key info:\n-\t\t# * see t/lib-gpg/gpgsm-gen-key.in\n-\t\t# To generate new certificate:\n-\t\t# * no passphrase\n-\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n-\t\t#\t\t-o /tmp/gpgsm.crt.user \\\n-\t\t#\t\t--generate-key \\\n-\t\t#\t\t--batch t/lib-gpg/gpgsm-gen-key.in\n-\t\t# To import certificate:\n-\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n-\t\t#\t\t--import /tmp/gpgsm.crt.user\n-\t\t# To export into a .p12 we can later import:\n-\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n-\t\t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n-\t\t#\t\t--export-secret-key-p12 \"committer@example.com\"\n-\t\techo | gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null \\\n-\t\t\t--passphrase-fd 0 --pinentry-mode loopback \\\n-\t\t\t--import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n-\n-\t\tgpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null -K |\n-\t\tgrep fingerprint: |\n-\t\tcut -d\" \" -f4 |\n-\t\ttr -d '\\n' >\"${GNUPGHOME}/trustlist.txt\" &&\n-\n-\t\techo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n-\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" >/dev/null \\\n-\t\t\t-u committer@example.com -o /dev/null --sign - 2>&1 &&\n-\t\ttest_set_prereq GPGSM\n+\t\t\t--sign -u committer@example.com\n \t\t;;\n \tesac\n-fi\n+'\n+\n+test_lazy_prereq GPGSM '\n+\ttest_have_prereq GPG &&\n+\t# Available key info:\n+\t# * see t/lib-gpg/gpgsm-gen-key.in\n+\t# To generate new certificate:\n+\t# * no passphrase\n+\t#\tgpgsm --homedir /tmp/gpghome/ \\\n+\t#\t\t-o /tmp/gpgsm.crt.user \\\n+\t#\t\t--generate-key \\\n+\t#\t\t--batch t/lib-gpg/gpgsm-gen-key.in\n+\t# To import certificate:\n+\t#\tgpgsm --homedir /tmp/gpghome/ \\\n+\t#\t\t--import /tmp/gpgsm.crt.user\n+\t# To export into a .p12 we can later import:\n+\t#\tgpgsm --homedir /tmp/gpghome/ \\\n+\t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n+\t#\t\t--export-secret-key-p12 \"committer@example.com\"\n+ echo | gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null \\\n+\t --passphrase-fd 0 --pinentry-mode loopback \\\n+\t --import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n+\n+ gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null -K |\n+ grep fingerprint: |\n+ cut -d\" \" -f4 |\n+\ttr -d \"\\\\n\" >\"${GNUPGHOME}/trustlist.txt\" &&\n+\n+ echo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n+ echo hello | gpgsm --homedir \"${GNUPGHOME}\" >/dev/null \\\n+\t -u committer@example.com -o /dev/null --sign - 2>&1\n+'\n \n-if test_have_prereq GPG &&\n- echo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null 2>&1\n-then\n-\ttest_set_prereq RFC1991\n-fi\n+test_lazy_prereq RFC1991 '\n+\ttest_have_prereq GPG &&\n+\techo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null 2>&1\n+'\n \n sanitize_pgp() {\n \tperl -ne '\n-- \ngitgitgadget\n\n"},{"id":"393926","messageId":"pull.728.v2.git.git.1585114881.gitgitgadget@gmail.com","threadId":"53068","inReplyTo":"pull.728.git.git.1584968990.gitgitgadget@gmail.com","subject":"[PATCH v2 0/5] Enable GPG in the Windows part of the CI/PR builds","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-25T05:41:16Z","receivedAt":"2020-03-25T05:41:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"While debugging the breakages introduced by hi/gpg-prefer-check-signature, I\nnoticed that the GPG prereq was not available on Windows, even if Git for\nWindows' SDK comes with a fully functional GPG2.\n\nThe fix was easy, but finding out what was going on was not, so for good\nmeasure, the fix is accompanied by a patch that will hopefully make future\ninvestigations into GPG-related problems much, much easier.\n\nChanges since v1:\n\n * The prereqs are now lazy ones.\n \n \n * A new patch was introduced to make tracing via -x work even with those\n inter-dependent prereqs.\n \n \n * The test-signing's stdout is redirected to /dev/null because it is\n unreadable and unhelpful binary gibberish, anyway. (This imitates Peff's\n patch.)\n\nJohannes Schindelin (5):\n tests(gpg): allow the gpg-agent to start on Windows\n t/lib-gpg.sh: stop pretending to be a stand-alone script\n tests: turn GPG, GPGSM and RFC1991 into lazy prereqs\n tests: do not let lazy prereqs inside `test_expect_*` turn off tracing\n tests: increase the verbosity of the GPG-related prereqs\n\n t/lib-gpg.sh | 110 ++++++++++++++++++++++++++---------------------\n t/t0000-basic.sh | 13 ++++++\n t/test-lib.sh | 6 ++-\n 3 files changed, 77 insertions(+), 52 deletions(-)\n\n\nbase-commit: 30e9940356dc67959877f4b2417da33ebdefbb79\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-728%2Fdscho%2Fci-windows-gpg-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-728/dscho/ci-windows-gpg-v2\nPull-Request: https://github.com/git/git/pull/728\n\nRange-diff vs v1:\n\n 1: 287a21f1033 = 1: 287a21f1033 tests(gpg): allow the gpg-agent to start on Windows\n -: ----------- > 2: c1811d54190 t/lib-gpg.sh: stop pretending to be a stand-alone script\n 2: dd26cb05a37 ! 3: 85457a7b618 tests(gpg): increase verbosity to allow debugging\n @@ -1,21 +1,36 @@\n Author: Johannes Schindelin \n \n - tests(gpg): increase verbosity to allow debugging\n + tests: turn GPG, GPGSM and RFC1991 into lazy prereqs\n \n - Especially when debugging a test failure that can only be reproduced in\n - the CI build (e.g. when the developer has no access to a macOS machine\n - other than running the tests on a macOS build agent), output should not\n - be suppressed.\n + The code to set those prereqs is executed completely outside of any\n + `test_eval_` block. As a consequence, its output had to be suppressed so\n + that it does not clutter the output of a regular test script run.\n \n - In the instance of `hi/gpg-prefer-check-signature`, where one\n - GPG-related test failed for no apparent reason, the entire output of\n - `gpg` and `gpgsm` was suppressed, even in verbose mode, leaving\n - interested readers no clue what was going wrong.\n + Unfortunately, the output *stays* suppressed even when the `--verbose`\n + option is in effect.\n \n - Let's fix this by redirecting the output not to `/dev/null`, but to the\n - file descriptors that may, or may not, be redirected via\n - `--verbose-log`. For good measure, also turn on tracing if the user\n - asked for it, and prefix it with a helpful info message.\n + This hid important output when debugging why the GPG prereq was not\n + enabled in the Windows part of our CI builds.\n +\n + In preparation for fixing that, let's move all of this code into lazy\n + prereqs.\n +\n + The only slightly tricky part is the global environment variable\n + `GNUPGHOME`. Originally, it was configured only when we verified that\n + there is a `gpg` in the `PATH` that we can use. This is now no longer\n + possible, as lazy prereqs are evaluated in a subshell that changes the\n + working directory to a temporary one. Therefore, we simply _always_ set\n + that environment variable: it does not hurt anything because it does not\n + indicate the presence of a working GPG.\n +\n + Side note: it was quite tempting to use a hack that is possible because\n + we do not validate what is passed to `test_lazy_prereq` (and it is\n + therefore possible to \"break out\" of the lazy_prereq subshell:\n +\n + test_lazy_prereq GPG '...) && GNUPGHOME=... && (...'\n +\n + However, this is rather tricksy hobbitses code, and the current patch is\n + _much_ easier to understand.\n \n Signed-off-by: Johannes Schindelin \n \n @@ -23,67 +38,128 @@\n --- a/t/lib-gpg.sh\n +++ b/t/lib-gpg.sh\n @@\n +-gpg_version=$(gpg --version 2>&1)\n +-if test $? != 127\n +-then\n ++# We always set GNUPGHOME, even if no usable GPG was found, as\n ++#\n ++# - It does not hurt, and\n ++#\n ++# - we cannot set global environment variables in lazy prereqs because they are\n ++# executed in an eval'ed subshell that changes the working directory to a\n ++# temporary one.\n ++\n ++GNUPGHOME=\"$PWD/gpghome\"\n ++export GNUPGHOME\n ++\n ++test_lazy_prereq GPG '\n ++\tgpg_version=$(gpg --version 2>&1)\n ++\ttest $? != 127 || exit 1\n ++\n + \t# As said here: http://www.gnupg.org/documentation/faqs.html#q6.19\n +-\t# the gpg version 1.0.6 didn't parse trust packets correctly, so for\n ++\t# the gpg version 1.0.6 did not parse trust packets correctly, so for\n + \t# that version, creation of signed tags using the generated key fails.\n + \tcase \"$gpg_version\" in\n +-\t'gpg (GnuPG) 1.0.6'*)\n ++\t\"gpg (GnuPG) 1.0.6\"*)\n \t\tsay \"Your version of gpg (1.0.6) is too buggy for testing\"\n ++\t\texit 1\n \t\t;;\n \t*)\n -+\t\tsay_color info >&4 \"Trying to set up GPG\"\n -+\t\twant_trace && set -x\n \t\t# Available key info:\n - \t\t# * Type DSA and Elgamal, size 2048 bits, no expiration date,\n - \t\t# name and email: C O Mitter \n @@\n - \t\tchmod 0700 ./gpghome &&\n - \t\tGNUPGHOME=\"$PWD/gpghome\" &&\n - \t\texport GNUPGHOME &&\n --\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n --\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n --\t\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg &&\n --\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import-ownertrust \\\n --\t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n --\t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n + \t\t# To export ownertrust:\n + \t\t#\tgpg --homedir /tmp/gpghome --export-ownertrust \\\n + \t\t#\t\t> lib-gpg/ownertrust\n +-\t\tmkdir ./gpghome &&\n +-\t\tchmod 0700 ./gpghome &&\n +-\t\tGNUPGHOME=\"$PWD/gpghome\" &&\n +-\t\texport GNUPGHOME &&\n ++\t\tmkdir \"$GNUPGHOME\" &&\n ++\t\tchmod 0700 \"$GNUPGHOME\" &&\n + \t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n + \t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n + \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg &&\n + \t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import-ownertrust \\\n + \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n + \t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n -\t\t\t--sign -u committer@example.com &&\n -+\t\t(gpgconf --kill gpg-agent >&3 2>&4 || : ) &&\n -+\t\tgpg --homedir \"${GNUPGHOME}\" --import \\\n -+\t\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg >&3 2>&4 &&\n -+\t\tgpg --homedir \"${GNUPGHOME}\" --import-ownertrust \\\n -+\t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust >&3 2>&4 &&\n -+\t\tgpg --homedir \"${GNUPGHOME}\" &3 2>&4 &&\n - \t\ttest_set_prereq GPG &&\n - \t\t# Available key info:\n - \t\t# * see t/lib-gpg/gpgsm-gen-key.in\n -@@\n - \t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n - \t\t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n - \t\t#\t\t--export-secret-key-p12 \"committer@example.com\"\n +-\t\ttest_set_prereq GPG &&\n +-\t\t# Available key info:\n +-\t\t# * see t/lib-gpg/gpgsm-gen-key.in\n +-\t\t# To generate new certificate:\n +-\t\t# * no passphrase\n +-\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n +-\t\t#\t\t-o /tmp/gpgsm.crt.user \\\n +-\t\t#\t\t--generate-key \\\n +-\t\t#\t\t--batch t/lib-gpg/gpgsm-gen-key.in\n +-\t\t# To import certificate:\n +-\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n +-\t\t#\t\t--import /tmp/gpgsm.crt.user\n +-\t\t# To export into a .p12 we can later import:\n +-\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n +-\t\t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n +-\t\t#\t\t--export-secret-key-p12 \"committer@example.com\"\n -\t\techo | gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null \\\n -+\t\techo | gpgsm --homedir \"${GNUPGHOME}\" >&3 2>&4 \\\n - \t\t\t--passphrase-fd 0 --pinentry-mode loopback \\\n - \t\t\t--import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n - \n +-\t\t\t--passphrase-fd 0 --pinentry-mode loopback \\\n +-\t\t\t--import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n +-\n -\t\tgpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null -K |\n -+\t\tgpgsm --homedir \"${GNUPGHOME}\" -K 2>&4 |\n - \t\tgrep fingerprint: |\n - \t\tcut -d\" \" -f4 |\n - \t\ttr -d '\\n' >\"${GNUPGHOME}/trustlist.txt\" &&\n - \n - \t\techo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n +-\t\tgrep fingerprint: |\n +-\t\tcut -d\" \" -f4 |\n +-\t\ttr -d '\\n' >\"${GNUPGHOME}/trustlist.txt\" &&\n +-\n +-\t\techo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n -\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" >/dev/null \\\n -\t\t\t-u committer@example.com -o /dev/null --sign - 2>&1 &&\n -+\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" >&3 2>&4 \\\n -+\t\t\t-u committer@example.com -o /dev/null --sign - &&\n - \t\ttest_set_prereq GPGSM\n +-\t\ttest_set_prereq GPGSM\n ++\t\t\t--sign -u committer@example.com\n \t\t;;\n \tesac\n - fi\n +-fi\n ++'\n ++\n ++test_lazy_prereq GPGSM '\n ++\ttest_have_prereq GPG &&\n ++\t# Available key info:\n ++\t# * see t/lib-gpg/gpgsm-gen-key.in\n ++\t# To generate new certificate:\n ++\t# * no passphrase\n ++\t#\tgpgsm --homedir /tmp/gpghome/ \\\n ++\t#\t\t-o /tmp/gpgsm.crt.user \\\n ++\t#\t\t--generate-key \\\n ++\t#\t\t--batch t/lib-gpg/gpgsm-gen-key.in\n ++\t# To import certificate:\n ++\t#\tgpgsm --homedir /tmp/gpghome/ \\\n ++\t#\t\t--import /tmp/gpgsm.crt.user\n ++\t# To export into a .p12 we can later import:\n ++\t#\tgpgsm --homedir /tmp/gpghome/ \\\n ++\t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n ++\t#\t\t--export-secret-key-p12 \"committer@example.com\"\n ++ echo | gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null \\\n ++\t --passphrase-fd 0 --pinentry-mode loopback \\\n ++\t --import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n ++\n ++ gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null -K |\n ++ grep fingerprint: |\n ++ cut -d\" \" -f4 |\n ++\ttr -d \"\\\\n\" >\"${GNUPGHOME}/trustlist.txt\" &&\n ++\n ++ echo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n ++ echo hello | gpgsm --homedir \"${GNUPGHOME}\" >/dev/null \\\n ++\t -u committer@example.com -o /dev/null --sign - 2>&1\n ++'\n \n - if test_have_prereq GPG &&\n +-if test_have_prereq GPG &&\n - echo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null 2>&1\n -+ echo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >&3 2>&4\n - then\n - \ttest_set_prereq RFC1991\n - fi\n -+want_trace && set +x\n +-then\n +-\ttest_set_prereq RFC1991\n +-fi\n ++test_lazy_prereq RFC1991 '\n ++\ttest_have_prereq GPG &&\n ++\techo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null 2>&1\n ++'\n \n sanitize_pgp() {\n \tperl -ne '\n -: ----------- > 4: 0767c8b77c8 tests: do not let lazy prereqs inside `test_expect_*` turn off tracing\n -: ----------- > 5: 5e89b512513 tests: increase the verbosity of the GPG-related prereqs\n\n-- \ngitgitgadget\n"},{"id":"393927","messageId":"0767c8b77c820cfc03bbc617da4dc9f20ba4a46a.1585114881.git.gitgitgadget@gmail.com","threadId":"53068","inReplyTo":"pull.728.v2.git.git.1585114881.gitgitgadget@gmail.com","subject":"[PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-25T05:41:20Z","receivedAt":"2020-03-25T05:41:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `test_expect_*` functions use `test_eval_` and so does\n`test_run_lazy_prereq_`. If tracing is enabled via the `-x` option,\n`test_eval_` turns on tracing while evaluating the code block, and turns\nit off directly after it.\n\nThis is unwanted for nested invocations.\n\nOne somewhat surprising example of this is when running a test that\ncalls `test_i18ngrep`: that function requires the `C_LOCALE_OUTPUT`\nprereq, and that prereq is a lazy one, so it is evaluated via\n`test_eval_`, the command tracing is turned off, and the test case\ncontinues to run _without tracing the commands_.\n\nAnother somewhat surprising example is when one lazy prereq depends on\nanother lazy prereq: the former will call `test_have_prereq` with the\nlatter one, which in turn calls `test_eval_` and -- you guessed it --\ntracing (if enabled) will be turned off _before_ returning to evaluating\nthe other lazy prereq.\n\nAs we will introduce just such a scenario with the GPG, GPGSM and\nRFC1991 prereqs, let's fix that by introducing a variable that keeps\ntrack of the current trace level: nested `test_eval_` calls will\nincrement and then decrement the level, and only when it reaches 0, the\ntracing will _actually_ be turned off.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t0000-basic.sh | 13 +++++++++++++\n t/test-lib.sh | 6 ++++--\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 3e440c078d5..b8597216200 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -833,6 +833,19 @@ then\n \texit 1\n fi\n \n+test_expect_success 'lazy prereqs do not turn off tracing' \"\n+\trun_sub_test_lib_test lazy-prereq-and-tracing \\\n+\t\t'lazy prereqs and -x' -v -x <<-\\\\EOF &&\n+\ttest_lazy_prereq LAZY true\n+\n+\ttest_expect_success lazy 'test_have_prereq LAZY && echo trace'\n+\n+\ttest_done\n+\tEOF\n+\n+\tgrep 'echo trace' lazy-prereq-and-tracing/err\n+\"\n+\n test_expect_success 'tests clean up even on failures' \"\n \trun_sub_test_lib_test_err \\\n \t\tfailing-cleanup 'Failing tests with cleanup commands' <<-\\\\EOF &&\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 0ea1e5a05ed..dbf25348106 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -882,6 +882,7 @@ maybe_setup_valgrind () {\n \tfi\n }\n \n+_trace_level=0\n want_trace () {\n \ttest \"$trace\" = t && {\n \t\ttest \"$verbose\" = t || test \"$verbose_log\" = t\n@@ -895,7 +896,7 @@ want_trace () {\n test_eval_inner_ () {\n \t# Do not add anything extra (including LF) after '$*'\n \teval \"\n-\t\twant_trace && set -x\n+\t\twant_trace && _trace_level=$(($_trace_level+1)) && set -x\n \t\t$*\"\n }\n \n@@ -926,7 +927,8 @@ test_eval_ () {\n \t\ttest_eval_ret_=$?\n \t\tif want_trace\n \t\tthen\n-\t\t\tset +x\n+\t\t\ttest 1 = $_trace_level && set +x\n+\t\t\t_trace_level=$(($_trace_level-1))\n \t\tfi\n \t} 2>/dev/null 4>&2\n \n-- \ngitgitgadget\n\n"},{"id":"393928","messageId":"5e89b512513af0e2e16dc93c86ae3d1145061a82.1585114881.git.gitgitgadget@gmail.com","threadId":"53068","inReplyTo":"pull.728.v2.git.git.1585114881.gitgitgadget@gmail.com","subject":"[PATCH v2 5/5] tests: increase the verbosity of the GPG-related prereqs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-25T05:41:21Z","receivedAt":"2020-03-25T05:41:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nEspecially when debugging a test failure that can only be reproduced in\nthe CI build (e.g. when the developer has no access to a macOS machine\nother than running the tests on a macOS build agent), output should not\nbe suppressed.\n\nIn the instance of `hi/gpg-prefer-check-signature`, where one\nGPG-related test failed for no apparent reason, the entire output of\n`gpg` and `gpgsm` was suppressed, even in verbose mode, leaving\ninterested readers no clue what was going wrong.\n\nLet's fix this by no longer redirecting the output not to `/dev/null`.\nThis is now possible because the affected prereqs were turned into lazy\nones (and are therefore evaluated via `test_eval_` which respects the\n`--verbose` option).\n\nNote that we _still_ redirect `stdout` to `/dev/null` for those commands\nthat sign their `stdin`, as the output would be binary (and useless\nanyway, because the reader would not have anything against which to\ncompare the output).\n\nSigned-off-by: Johannes Schindelin \n---\n t/lib-gpg.sh | 28 ++++++++++++++--------------\n 1 file changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex 7a78c562e8d..9fc5241228e 100755\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -40,12 +40,12 @@ test_lazy_prereq GPG '\n \t\t#\t\t> lib-gpg/ownertrust\n \t\tmkdir \"$GNUPGHOME\" &&\n \t\tchmod 0700 \"$GNUPGHOME\" &&\n-\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n-\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n+\t\t(gpgconf --kill gpg-agent || : ) &&\n+\t\tgpg --homedir \"${GNUPGHOME}\" --import \\\n \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg &&\n-\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import-ownertrust \\\n+\t\tgpg --homedir \"${GNUPGHOME}\" --import-ownertrust \\\n \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n-\t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n+\t\tgpg --homedir \"${GNUPGHOME}\" /dev/null \\\n \t\t\t--sign -u committer@example.com\n \t\t;;\n \tesac\n@@ -68,23 +68,23 @@ test_lazy_prereq GPGSM '\n \t#\tgpgsm --homedir /tmp/gpghome/ \\\n \t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n \t#\t\t--export-secret-key-p12 \"committer@example.com\"\n- echo | gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null \\\n-\t --passphrase-fd 0 --pinentry-mode loopback \\\n-\t --import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n+\techo | gpgsm --homedir \"${GNUPGHOME}\" \\\n+\t\t--passphrase-fd 0 --pinentry-mode loopback \\\n+\t\t--import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n \n- gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null -K |\n- grep fingerprint: |\n- cut -d\" \" -f4 |\n+\tgpgsm --homedir \"${GNUPGHOME}\" -K |\n+\tgrep fingerprint: |\n+\tcut -d\" \" -f4 |\n \ttr -d \"\\\\n\" >\"${GNUPGHOME}/trustlist.txt\" &&\n \n- echo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n- echo hello | gpgsm --homedir \"${GNUPGHOME}\" >/dev/null \\\n-\t -u committer@example.com -o /dev/null --sign - 2>&1\n+\techo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n+\techo hello | gpgsm --homedir \"${GNUPGHOME}\" >/dev/null \\\n+\t -u committer@example.com -o /dev/null --sign -\n '\n \n test_lazy_prereq RFC1991 '\n \ttest_have_prereq GPG &&\n-\techo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null 2>&1\n+\techo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null\n '\n \n sanitize_pgp() {\n-- \ngitgitgadget\n"},{"id":"393987","messageId":"xmqqwo78wdq9.fsf@gitster.c.googlers.com","threadId":"53068","inReplyTo":"0767c8b77c820cfc03bbc617da4dc9f20ba4a46a.1585114881.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-25T17:23:10Z","receivedAt":"2020-03-25T17:23:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> The `test_expect_*` functions use `test_eval_` and so does\n> `test_run_lazy_prereq_`. If tracing is enabled via the `-x` option,\n> `test_eval_` turns on tracing while evaluating the code block, and turns\n> it off directly after it.\n>\n> This is unwanted for nested invocations.\n\nNice finding.\n\n> As we will introduce just such a scenario with the GPG, GPGSM and\n> RFC1991 prereqs, let's fix that by introducing a variable that keeps\n> track of the current trace level: nested `test_eval_` calls will\n> increment and then decrement the level, and only when it reaches 0, the\n> tracing will _actually_ be turned off.\n\nDoesn't this explanation urge us to reorder these patches? It\nsounds to me that it argues to have this step before 3/5.\n\nOther than that, both the explanation and the code look correctly\ndone. It looks to me that the surrounding code favors trailing \"_\"\ninstead of leading one for private names, so we might want to rename\nthe variable to $trace_level_ but that is minor.\n\nThanks.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> t/t0000-basic.sh | 13 +++++++++++++\n> t/test-lib.sh | 6 ++++--\n> 2 files changed, 17 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> index 3e440c078d5..b8597216200 100755\n> --- a/t/t0000-basic.sh\n> +++ b/t/t0000-basic.sh\n> @@ -833,6 +833,19 @@ then\n> \texit 1\n> fi\n> \n> +test_expect_success 'lazy prereqs do not turn off tracing' \"\n> +\trun_sub_test_lib_test lazy-prereq-and-tracing \\\n> +\t\t'lazy prereqs and -x' -v -x <<-\\\\EOF &&\n> +\ttest_lazy_prereq LAZY true\n> +\n> +\ttest_expect_success lazy 'test_have_prereq LAZY && echo trace'\n> +\n> +\ttest_done\n> +\tEOF\n> +\n> +\tgrep 'echo trace' lazy-prereq-and-tracing/err\n> +\"\n> +\n> test_expect_success 'tests clean up even on failures' \"\n> \trun_sub_test_lib_test_err \\\n> \t\tfailing-cleanup 'Failing tests with cleanup commands' <<-\\\\EOF &&\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 0ea1e5a05ed..dbf25348106 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -882,6 +882,7 @@ maybe_setup_valgrind () {\n> \tfi\n> }\n> \n> +_trace_level=0\n> want_trace () {\n> \ttest \"$trace\" = t && {\n> \t\ttest \"$verbose\" = t || test \"$verbose_log\" = t\n> @@ -895,7 +896,7 @@ want_trace () {\n> test_eval_inner_ () {\n> \t# Do not add anything extra (including LF) after '$*'\n> \teval \"\n> -\t\twant_trace && set -x\n> +\t\twant_trace && _trace_level=$(($_trace_level+1)) && set -x\n> \t\t$*\"\n> }\n> \n> @@ -926,7 +927,8 @@ test_eval_ () {\n> \t\ttest_eval_ret_=$?\n> \t\tif want_trace\n> \t\tthen\n> -\t\t\tset +x\n> +\t\t\ttest 1 = $_trace_level && set +x\n> +\t\t\t_trace_level=$(($_trace_level-1))\n> \t\tfi\n> \t} 2>/dev/null 4>&2\n"},{"id":"393988","messageId":"xmqqsghwwdlp.fsf@gitster.c.googlers.com","threadId":"53068","inReplyTo":"85457a7b61874e8e9f3af9c231451df0aba7a7b5.1585114881.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-25T17:25:54Z","receivedAt":"2020-03-25T17:25:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> -if test_have_prereq GPG &&\n> - echo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null 2>&1\n> -then\n> -\ttest_set_prereq RFC1991\n> -fi\n> +test_lazy_prereq RFC1991 '\n> +\ttest_have_prereq GPG &&\n> +\techo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null 2>&1\n> +'\n\nOK. To make it fully lazy, we do need to test GPG while lazily\nchecking for RFC1991. Makes sense.\n\nThanks.\n"},{"id":"394062","messageId":"20200326082140.GC2200716@coredump.intra.peff.net","threadId":"53068","inReplyTo":"c1811d54190a62e3d35505e2b806c89238b56de8.1585114881.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-26T08:21:40Z","receivedAt":"2020-03-26T08:21:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 25, 2020 at 05:41:18AM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> It makes no sense to call `./lib-gpg.sh`. Therefore the hash-bang line\n> is unnecessary.\n\nI wondered if it might be useful to identify the file for editors, etc.\nBut \"*.sh\" is quite sufficient, and we already stripped most of these\nout long ago in b54a31243b (t/lib-*.sh: drop executable bit,\n2020-03-26).\n\nThere's some other related cleanup. I'll prepare a separate series.\n\n-Peff\n"},{"id":"394076","messageId":"20200326083519.GD2200716@coredump.intra.peff.net","threadId":"53068","inReplyTo":"85457a7b61874e8e9f3af9c231451df0aba7a7b5.1585114881.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-26T08:35:19Z","receivedAt":"2020-03-26T08:35:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 25, 2020 at 05:41:19AM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> In preparation for fixing that, let's move all of this code into lazy\n> prereqs.\n\nOK. This looks good, even if I cannot help feel that my earlier patch\nwas perfectly sufficient. ;)\n\n> Side note: it was quite tempting to use a hack that is possible because\n> we do not validate what is passed to `test_lazy_prereq` (and it is\n> therefore possible to \"break out\" of the lazy_prereq subshell:\n> \n> \ttest_lazy_prereq GPG '...) && GNUPGHOME=... && (...'\n\nNo, it is not tempting at all to me to do something so gross. :)\n\n> +test_lazy_prereq GPG '\n> +\tgpg_version=$(gpg --version 2>&1)\n\nOne thing I observed while doing my patch is that lazy_prereq blocks do\nnot get run through the &&-chain linter. So this is OK, but I wonder if\nwe should be future-proofing with braces. I don't care _too_ much either\nway, though.\n\n> +\ttest $? != 127 || exit 1\n\nI have a slight preference for \"return 1\" here. The \"exit 1\" works\nbecause test_lazy_prereq puts us in an implicit subshell. But I think\nthis sets a bad example for people writing regular tests, where there is\nno such subshell (and \"return 1\" is the only correct way to do it).\n\n> \tcase \"$gpg_version\" in\n> -\t'gpg (GnuPG) 1.0.6'*)\n> +\t\"gpg (GnuPG) 1.0.6\"*)\n> \t\tsay \"Your version of gpg (1.0.6) is too buggy for testing\"\n> +\t\texit 1\n\nDitto here.\n\n> @@ -25,55 +38,54 @@ then\n> \t\t# To export ownertrust:\n> \t\t#\tgpg --homedir /tmp/gpghome --export-ownertrust \\\n> \t\t#\t\t> lib-gpg/ownertrust\n> -\t\tmkdir ./gpghome &&\n> -\t\tchmod 0700 ./gpghome &&\n> -\t\tGNUPGHOME=\"$PWD/gpghome\" &&\n> -\t\texport GNUPGHOME &&\n> +\t\tmkdir \"$GNUPGHOME\" &&\n> +\t\tchmod 0700 \"$GNUPGHOME\" &&\n\nCompared to mine this keeps the mkdir in the prereq. That seems fine to\nme. Other prereqs do depend on the directory existing, but they all\ndepend on GPG itself, so they'd be fine.\n\n> +test_lazy_prereq GPGSM '\n> +\ttest_have_prereq GPG &&\n\nIn mine I put the test_have_prereq outside the lazy prereq. I don't\nthink it really matters either way (when we later ask if GPGSM is set,\nthere is no difference between nobody having defined it, and having a\nlazy definition that said \"no\").\n\n-Peff\n"},{"id":"394077","messageId":"20200326084903.GE2200716@coredump.intra.peff.net","threadId":"53068","inReplyTo":"0767c8b77c820cfc03bbc617da4dc9f20ba4a46a.1585114881.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-26T08:49:03Z","receivedAt":"2020-03-26T08:49:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 25, 2020 at 05:41:20AM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> The `test_expect_*` functions use `test_eval_` and so does\n> `test_run_lazy_prereq_`. If tracing is enabled via the `-x` option,\n> `test_eval_` turns on tracing while evaluating the code block, and turns\n> it off directly after it.\n> \n> This is unwanted for nested invocations.\n> \n> One somewhat surprising example of this is when running a test that\n> calls `test_i18ngrep`: that function requires the `C_LOCALE_OUTPUT`\n> prereq, and that prereq is a lazy one, so it is evaluated via\n> `test_eval_`, the command tracing is turned off, and the test case\n> continues to run _without tracing the commands_.\n\nGood catch. I didn't see this when looking at the GPG stuff earlier\nbecause I didn't nest the lazy prereqs. But it is worth fixing\nregardless, because as you note it comes up in other places.\n\n> @@ -926,7 +927,8 @@ test_eval_ () {\n> \t\ttest_eval_ret_=$?\n> \t\tif want_trace\n> \t\tthen\n> -\t\t\tset +x\n> +\t\t\ttest 1 = $_trace_level && set +x\n> +\t\t\t_trace_level=$(($_trace_level-1))\n> \t\tfi\n> \t} 2>/dev/null 4>&2\n\nI briefly wondered if adding more logic here might upset our delicate\nbalance of avoiding writing cruft to the \"-x\" output. But the answer is\n\"no\", due to the descriptor hackery at the end of that {} block.\n\nOf course, any test evaluating a lazy prereq already gets tons of cruft\nanyway, because the outer level of tracing sees all of our internal\nfunction calls checking the prereq and setting up the inner level of\ntracing. But there's not much we can do about that.\n\n-Peff\n"},{"id":"394078","messageId":"20200326085056.GF2200716@coredump.intra.peff.net","threadId":"53068","inReplyTo":"5e89b512513af0e2e16dc93c86ae3d1145061a82.1585114881.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 5/5] tests: increase the verbosity of the GPG-related prereqs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-26T08:50:56Z","receivedAt":"2020-03-26T08:50:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 25, 2020 at 05:41:21AM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> Let's fix this by no longer redirecting the output not to `/dev/null`.\n> This is now possible because the affected prereqs were turned into lazy\n> ones (and are therefore evaluated via `test_eval_` which respects the\n> `--verbose` option).\n> \n> Note that we _still_ redirect `stdout` to `/dev/null` for those commands\n> that sign their `stdin`, as the output would be binary (and useless\n> anyway, because the reader would not have anything against which to\n> compare the output).\n\nThis looks good. You could drop the redirection of stdin in one case,\ntoo (since test_eval_ already does so) but I don't mind leaving it as\ndocumentation.\n\n-Peff\n"},{"id":"394089","messageId":"nycvar.QRO.7.76.6.2003261443550.46@tvgsbejvaqbjf.bet","threadId":"53068","inReplyTo":"xmqqwo78wdq9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-03-26T13:45:53Z","receivedAt":"2020-03-26T13:46:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Wed, 25 Mar 2020, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > The `test_expect_*` functions use `test_eval_` and so does\n> > `test_run_lazy_prereq_`. If tracing is enabled via the `-x` option,\n> > `test_eval_` turns on tracing while evaluating the code block, and turns\n> > it off directly after it.\n> >\n> > This is unwanted for nested invocations.\n>\n> Nice finding.\n\nHeh. I found this ages ago, and mistook it for \"all prereqs turn off\ntracing\" when I reported it, but Gábor pointed out that that is incorrect.\n\nThat was a long time ago, maybe even a year, and I finally used this patch\nseries as an excuse to finally dig deep on this.\n\n> > As we will introduce just such a scenario with the GPG, GPGSM and\n> > RFC1991 prereqs, let's fix that by introducing a variable that keeps\n> > track of the current trace level: nested `test_eval_` calls will\n> > increment and then decrement the level, and only when it reaches 0, the\n> > tracing will _actually_ be turned off.\n>\n> Doesn't this explanation urge us to reorder these patches? It\n> sounds to me that it argues to have this step before 3/5.\n\nYes, that's where I had meant to put this patch. Sorry for the confusion.\n\n> Other than that, both the explanation and the code look correctly\n> done. It looks to me that the surrounding code favors trailing \"_\"\n> instead of leading one for private names, so we might want to rename\n> the variable to $trace_level_ but that is minor.\n\nGood point, I had missed that. Will be fixed in the next iteration.\n\nCiao,\nDscho\n\n>\n> Thanks.\n>\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > t/t0000-basic.sh | 13 +++++++++++++\n> > t/test-lib.sh | 6 ++++--\n> > 2 files changed, 17 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\n> > index 3e440c078d5..b8597216200 100755\n> > --- a/t/t0000-basic.sh\n> > +++ b/t/t0000-basic.sh\n> > @@ -833,6 +833,19 @@ then\n> > \texit 1\n> > fi\n> >\n> > +test_expect_success 'lazy prereqs do not turn off tracing' \"\n> > +\trun_sub_test_lib_test lazy-prereq-and-tracing \\\n> > +\t\t'lazy prereqs and -x' -v -x <<-\\\\EOF &&\n> > +\ttest_lazy_prereq LAZY true\n> > +\n> > +\ttest_expect_success lazy 'test_have_prereq LAZY && echo trace'\n> > +\n> > +\ttest_done\n> > +\tEOF\n> > +\n> > +\tgrep 'echo trace' lazy-prereq-and-tracing/err\n> > +\"\n> > +\n> > test_expect_success 'tests clean up even on failures' \"\n> > \trun_sub_test_lib_test_err \\\n> > \t\tfailing-cleanup 'Failing tests with cleanup commands' <<-\\\\EOF &&\n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > index 0ea1e5a05ed..dbf25348106 100644\n> > --- a/t/test-lib.sh\n> > +++ b/t/test-lib.sh\n> > @@ -882,6 +882,7 @@ maybe_setup_valgrind () {\n> > \tfi\n> > }\n> >\n> > +_trace_level=0\n> > want_trace () {\n> > \ttest \"$trace\" = t && {\n> > \t\ttest \"$verbose\" = t || test \"$verbose_log\" = t\n> > @@ -895,7 +896,7 @@ want_trace () {\n> > test_eval_inner_ () {\n> > \t# Do not add anything extra (including LF) after '$*'\n> > \teval \"\n> > -\t\twant_trace && set -x\n> > +\t\twant_trace && _trace_level=$(($_trace_level+1)) && set -x\n> > \t\t$*\"\n> > }\n> >\n> > @@ -926,7 +927,8 @@ test_eval_ () {\n> > \t\ttest_eval_ret_=$?\n> > \t\tif want_trace\n> > \t\tthen\n> > -\t\t\tset +x\n> > +\t\t\ttest 1 = $_trace_level && set +x\n> > +\t\t\t_trace_level=$(($_trace_level-1))\n> > \t\tfi\n> > \t} 2>/dev/null 4>&2\n>\n"},{"id":"394090","messageId":"nycvar.QRO.7.76.6.2003261446160.46@tvgsbejvaqbjf.bet","threadId":"53068","inReplyTo":"20200326082140.GC2200716@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-03-26T13:48:32Z","receivedAt":"2020-03-26T13:48:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 26 Mar 2020, Jeff King wrote:\n\n> On Wed, Mar 25, 2020 at 05:41:18AM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n> > From: Johannes Schindelin \n> >\n> > It makes no sense to call `./lib-gpg.sh`. Therefore the hash-bang line\n> > is unnecessary.\n>\n> I wondered if it might be useful to identify the file for editors, etc.\n> But \"*.sh\" is quite sufficient, and we already stripped most of these\n> out long ago in b54a31243b (t/lib-*.sh: drop executable bit,\n> 2020-03-26).\n>\n> There's some other related cleanup. I'll prepare a separate series.\n\nI noticed that, but forgot to add a note about this to the commit message.\nSorry. The next iteration of this patch series will have that note.\n\n$ git grep '#! */' t/*.sh | grep -v 't[0-9]'\nt/aggregate-results.sh:#!/bin/sh\nt/gitweb-lib.sh:#!/usr/bin/perl\nt/lib-credential.sh:#!/bin/sh\n\nThank you for volunteering to clean them up!\n\nCiao,\nDscho\n"},{"id":"394091","messageId":"nycvar.QRO.7.76.6.2003261450590.46@tvgsbejvaqbjf.bet","threadId":"53068","inReplyTo":"20200326083519.GD2200716@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-03-26T14:27:19Z","receivedAt":"2020-03-26T14:27:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 26 Mar 2020, Jeff King wrote:\n\n> On Wed, Mar 25, 2020 at 05:41:19AM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n> > In preparation for fixing that, let's move all of this code into lazy\n> > prereqs.\n>\n> OK. This looks good, even if I cannot help feel that my earlier patch\n> was perfectly sufficient. ;)\n\nThe mistake is all mine. I had totally missed that you turned GPG into a\nlazy prereq. So I had my patch finalized already before you pointed my\nnose at that fact.\n\nSorry about that.\n\n> > Side note: it was quite tempting to use a hack that is possible because\n> > we do not validate what is passed to `test_lazy_prereq` (and it is\n> > therefore possible to \"break out\" of the lazy_prereq subshell:\n> >\n> > \ttest_lazy_prereq GPG '...) && GNUPGHOME=... && (...'\n>\n> No, it is not tempting at all to me to do something so gross. :)\n\nWell, maybe it was not tempting to _you_, but to _me_, it was so tempting\nthat I had implemented and committed it before I made up my mind and\nchanged it again.\n\n> > +test_lazy_prereq GPG '\n> > +\tgpg_version=$(gpg --version 2>&1)\n>\n> One thing I observed while doing my patch is that lazy_prereq blocks do\n> not get run through the &&-chain linter. So this is OK, but I wonder if\n> we should be future-proofing with braces. I don't care _too_ much either\n> way, though.\n\nI actually like that the prereq blocks are exempt from this && chain\nlinting, and would like to refrain from adding braces \"just because\".\n\n> > +\ttest $? != 127 || exit 1\n>\n> I have a slight preference for \"return 1\" here. The \"exit 1\" works\n> because test_lazy_prereq puts us in an implicit subshell. But I think\n> this sets a bad example for people writing regular tests, where there is\n> no such subshell (and \"return 1\" is the only correct way to do it).\n\nThere are two reasons why I did not use `return` here:\n\n- To me, prereq code is very distinct from writing tests. Just like we do\n not &&-chain all the shell functions that live outside of tests, I don't\n want to &&-chain all the prereq code.\n\n The point of the tests' commands is not to fail. The point of prereq's\n code is to fail if the prereq is not met.\n\n- Since this code is outside of a function, `return` felt like the wrong\n semantic concept to me. And indeed, I see this (rather scary) part in\n Bash's documentation of `return` (I did not even bother to look at the\n POSIX semantics, it scared me so much):\n\n The return status is non-zero if `return` is supplied a non-numeric\n argument, or is used outside a function and not during execution of\n a script by `.` or `source`.\n\n So: the `1` is totally ignored in this context. That alone is reason\n enough for me to completely avoid it, and use `exit` instead.\n\n> > \tcase \"$gpg_version\" in\n> > -\t'gpg (GnuPG) 1.0.6'*)\n> > +\t\"gpg (GnuPG) 1.0.6\"*)\n> > \t\tsay \"Your version of gpg (1.0.6) is too buggy for testing\"\n> > +\t\texit 1\n>\n> Ditto here.\n>\n> > @@ -25,55 +38,54 @@ then\n> > \t\t# To export ownertrust:\n> > \t\t#\tgpg --homedir /tmp/gpghome --export-ownertrust \\\n> > \t\t#\t\t> lib-gpg/ownertrust\n> > -\t\tmkdir ./gpghome &&\n> > -\t\tchmod 0700 ./gpghome &&\n> > -\t\tGNUPGHOME=\"$PWD/gpghome\" &&\n> > -\t\texport GNUPGHOME &&\n> > +\t\tmkdir \"$GNUPGHOME\" &&\n> > +\t\tchmod 0700 \"$GNUPGHOME\" &&\n>\n> Compared to mine this keeps the mkdir in the prereq. That seems fine to\n> me. Other prereqs do depend on the directory existing, but they all\n> depend on GPG itself, so they'd be fine.\n\nYes. And conceptually, I like that the prereq is responsible for creating\nthat directory.\n\n> > +test_lazy_prereq GPGSM '\n> > +\ttest_have_prereq GPG &&\n>\n> In mine I put the test_have_prereq outside the lazy prereq.\n\nThat makes it essentially a non-lazy prereq.\n\n> I don't think it really matters either way (when we later ask if GPGSM\n> is set, there is no difference between nobody having defined it, and\n> having a lazy definition that said \"no\").\n\nThe difference is when running a test with `--run=` where `` refers\nto a test case that requires neither GPG nor GPGSM or RFC1991. My version\nwill not evaluate the GPG prereq, yours still will.\n\nCiao,\nDscho\n"},{"id":"394092","messageId":"nycvar.QRO.7.76.6.2003261528050.46@tvgsbejvaqbjf.bet","threadId":"53068","inReplyTo":"20200326084903.GE2200716@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-03-26T14:34:32Z","receivedAt":"2020-03-26T14:34:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 26 Mar 2020, Jeff King wrote:\n\n> On Wed, Mar 25, 2020 at 05:41:20AM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n> > From: Johannes Schindelin \n> >\n> > The `test_expect_*` functions use `test_eval_` and so does\n> > `test_run_lazy_prereq_`. If tracing is enabled via the `-x` option,\n> > `test_eval_` turns on tracing while evaluating the code block, and turns\n> > it off directly after it.\n> >\n> > This is unwanted for nested invocations.\n> >\n> > One somewhat surprising example of this is when running a test that\n> > calls `test_i18ngrep`: that function requires the `C_LOCALE_OUTPUT`\n> > prereq, and that prereq is a lazy one, so it is evaluated via\n> > `test_eval_`, the command tracing is turned off, and the test case\n> > continues to run _without tracing the commands_.\n>\n> Good catch. I didn't see this when looking at the GPG stuff earlier\n> because I didn't nest the lazy prereqs. But it is worth fixing\n> regardless, because as you note it comes up in other places.\n>\n> > @@ -926,7 +927,8 @@ test_eval_ () {\n> > \t\ttest_eval_ret_=$?\n> > \t\tif want_trace\n> > \t\tthen\n> > -\t\t\tset +x\n> > +\t\t\ttest 1 = $_trace_level && set +x\n> > +\t\t\t_trace_level=$(($_trace_level-1))\n> > \t\tfi\n> > \t} 2>/dev/null 4>&2\n>\n> I briefly wondered if adding more logic here might upset our delicate\n> balance of avoiding writing cruft to the \"-x\" output. But the answer is\n> \"no\", due to the descriptor hackery at the end of that {} block.\n>\n> Of course, any test evaluating a lazy prereq already gets tons of cruft\n> anyway, because the outer level of tracing sees all of our internal\n> function calls checking the prereq and setting up the inner level of\n> tracing. But there's not much we can do about that.\n\nWe could turn off the tracing specifically in those cases. At some point,\nthough, it strikes me as rather ridiculous through how many hoops we jump\njust because our test suite framework is implemented in portable Unix\nshell script, as opposed to a more powerful language such as, say, C.\n\nFor example, we could prevent the `test_eval_ret_=$?` line from being\ntraced by default, simply by redirecting fd `4` to `/dev/null`. At that\nstage, we would of course have to open yet another fd to support the use\ncase where the `-x` is passed to `sh` itself (`sh -x t0000-*.sh -i -v`).\n\nIn the context of this here patch series, which really is about enabling\nthe GPG-related tests on Windows, I will refrain from going down that\nparticular rabbit hole. My ego might get stuck.\n\nCiao,\nDscho\n\n"},{"id":"394093","messageId":"nycvar.QRO.7.76.6.2003261534410.46@tvgsbejvaqbjf.bet","threadId":"53068","inReplyTo":"20200326085056.GF2200716@coredump.intra.peff.net","subject":"Re: [PATCH v2 5/5] tests: increase the verbosity of the GPG-related prereqs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-03-26T14:36:36Z","receivedAt":"2020-03-26T14:36:41Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 26 Mar 2020, Jeff King wrote:\n\n> On Wed, Mar 25, 2020 at 05:41:21AM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n> > Let's fix this by no longer redirecting the output not to `/dev/null`.\n> > This is now possible because the affected prereqs were turned into lazy\n> > ones (and are therefore evaluated via `test_eval_` which respects the\n> > `--verbose` option).\n> >\n> > Note that we _still_ redirect `stdout` to `/dev/null` for those commands\n> > that sign their `stdin`, as the output would be binary (and useless\n> > anyway, because the reader would not have anything against which to\n> > compare the output).\n>\n> This looks good. You could drop the redirection of stdin in one case,\n> too (since test_eval_ already does so) but I don't mind leaving it as\n> documentation.\n\nI considered that, but decided that I'd rather save myself the brain\ncycles in the future when reading that code (I would ask myself \"*what* is\nsigned here?\").\n\nThat's why I left the `\n \n diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\n 4: 0767c8b77c8 ! 3: f35830c0eba tests: do not let lazy prereqs inside `test_expect_*` turn off tracing\n @@ -60,7 +60,7 @@\n \tfi\n }\n \n -+_trace_level=0\n ++trace_level_=0\n want_trace () {\n \ttest \"$trace\" = t && {\n \t\ttest \"$verbose\" = t || test \"$verbose_log\" = t\n @@ -69,7 +69,7 @@\n \t# Do not add anything extra (including LF) after '$*'\n \teval \"\n -\t\twant_trace && set -x\n -+\t\twant_trace && _trace_level=$(($_trace_level+1)) && set -x\n ++\t\twant_trace && trace_level_=$(($trace_level_+1)) && set -x\n \t\t$*\"\n }\n \n @@ -78,8 +78,8 @@\n \t\tif want_trace\n \t\tthen\n -\t\t\tset +x\n -+\t\t\ttest 1 = $_trace_level && set +x\n -+\t\t\t_trace_level=$(($_trace_level-1))\n ++\t\t\ttest 1 = $trace_level_ && set +x\n ++\t\t\ttrace_level_=$(($trace_level_-1))\n \t\tfi\n \t} 2>/dev/null 4>&2\n \n 3: 85457a7b618 = 4: f69f97e24ba tests: turn GPG, GPGSM and RFC1991 into lazy prereqs\n 5: 5e89b512513 = 5: 064f4e541b8 tests: increase the verbosity of the GPG-related prereqs\n\n-- \ngitgitgadget\n"},{"id":"394099","messageId":"b4217c36070e504ddacf4cfe71897fb324f65db1.1585236929.git.gitgitgadget@gmail.com","threadId":"53068","inReplyTo":"pull.728.v3.git.git.1585236929.gitgitgadget@gmail.com","subject":"[PATCH v3 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-26T15:35:25Z","receivedAt":"2020-03-26T15:35:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIt makes no sense to call `./lib-gpg.sh`. Therefore the hash-bang line\nis unnecessary.\n\nThere are other similar instances in `t/`, but they are too far from the\ncontext of the enclosing patch series, so they will be addressed\nseparately.\n\nSigned-off-by: Johannes Schindelin \n---\n t/lib-gpg.sh | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex 11b83b8c24a..4ead1268351 100755\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -1,5 +1,3 @@\n-#!/bin/sh\n-\n gpg_version=$(gpg --version 2>&1)\n if test $? != 127\n then\n-- \ngitgitgadget\n\n"},{"id":"394100","messageId":"287a21f1033b2a74420029c529ad4db956051a85.1585236929.git.gitgitgadget@gmail.com","threadId":"53068","inReplyTo":"pull.728.v3.git.git.1585236929.gitgitgadget@gmail.com","subject":"[PATCH v3 1/5] tests(gpg): allow the gpg-agent to start on Windows","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-26T15:35:24Z","receivedAt":"2020-03-26T15:35:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn Git for Windows' SDK, we use the MSYS2 version of OpenSSH, meaning\nthat the `gpg-agent` will fail horribly when being passed a `--homedir`\nthat contains colons.\n\nPreviously, we did pass the Windows version of the absolute path,\nthough, which starts in the drive letter followed by, you guessed it, a\ncolon.\n\nLet's use the same trick found elsewhere in our test suite where `$PWD`\nis used to refer to the pseudo-Unix path (which works only within the\nMSYS2 Bash/OpenSSH/Perl/etc, as opposed to `$(pwd)` which refers to the\nWindows path that `git.exe` understands, too).\n\nSigned-off-by: Johannes Schindelin \n---\n t/lib-gpg.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex 8d28652b729..11b83b8c24a 100755\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -29,7 +29,7 @@ then\n \t\t#\t\t> lib-gpg/ownertrust\n \t\tmkdir ./gpghome &&\n \t\tchmod 0700 ./gpghome &&\n-\t\tGNUPGHOME=\"$(pwd)/gpghome\" &&\n+\t\tGNUPGHOME=\"$PWD/gpghome\" &&\n \t\texport GNUPGHOME &&\n \t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n \t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n-- \ngitgitgadget\n\n"},{"id":"394101","messageId":"f35830c0eba216b7b4f144409e302a87ff8b5c06.1585236929.git.gitgitgadget@gmail.com","threadId":"53068","inReplyTo":"pull.728.v3.git.git.1585236929.gitgitgadget@gmail.com","subject":"[PATCH v3 3/5] tests: do not let lazy prereqs inside `test_expect_*` turn off tracing","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-26T15:35:26Z","receivedAt":"2020-03-26T15:35:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `test_expect_*` functions use `test_eval_` and so does\n`test_run_lazy_prereq_`. If tracing is enabled via the `-x` option,\n`test_eval_` turns on tracing while evaluating the code block, and turns\nit off directly after it.\n\nThis is unwanted for nested invocations.\n\nOne somewhat surprising example of this is when running a test that\ncalls `test_i18ngrep`: that function requires the `C_LOCALE_OUTPUT`\nprereq, and that prereq is a lazy one, so it is evaluated via\n`test_eval_`, the command tracing is turned off, and the test case\ncontinues to run _without tracing the commands_.\n\nAnother somewhat surprising example is when one lazy prereq depends on\nanother lazy prereq: the former will call `test_have_prereq` with the\nlatter one, which in turn calls `test_eval_` and -- you guessed it --\ntracing (if enabled) will be turned off _before_ returning to evaluating\nthe other lazy prereq.\n\nAs we will introduce just such a scenario with the GPG, GPGSM and\nRFC1991 prereqs, let's fix that by introducing a variable that keeps\ntrack of the current trace level: nested `test_eval_` calls will\nincrement and then decrement the level, and only when it reaches 0, the\ntracing will _actually_ be turned off.\n\nSigned-off-by: Johannes Schindelin \n---\n t/t0000-basic.sh | 13 +++++++++++++\n t/test-lib.sh | 6 ++++--\n 2 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0000-basic.sh b/t/t0000-basic.sh\nindex 3e440c078d5..b8597216200 100755\n--- a/t/t0000-basic.sh\n+++ b/t/t0000-basic.sh\n@@ -833,6 +833,19 @@ then\n \texit 1\n fi\n \n+test_expect_success 'lazy prereqs do not turn off tracing' \"\n+\trun_sub_test_lib_test lazy-prereq-and-tracing \\\n+\t\t'lazy prereqs and -x' -v -x <<-\\\\EOF &&\n+\ttest_lazy_prereq LAZY true\n+\n+\ttest_expect_success lazy 'test_have_prereq LAZY && echo trace'\n+\n+\ttest_done\n+\tEOF\n+\n+\tgrep 'echo trace' lazy-prereq-and-tracing/err\n+\"\n+\n test_expect_success 'tests clean up even on failures' \"\n \trun_sub_test_lib_test_err \\\n \t\tfailing-cleanup 'Failing tests with cleanup commands' <<-\\\\EOF &&\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 0ea1e5a05ed..529056be497 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -882,6 +882,7 @@ maybe_setup_valgrind () {\n \tfi\n }\n \n+trace_level_=0\n want_trace () {\n \ttest \"$trace\" = t && {\n \t\ttest \"$verbose\" = t || test \"$verbose_log\" = t\n@@ -895,7 +896,7 @@ want_trace () {\n test_eval_inner_ () {\n \t# Do not add anything extra (including LF) after '$*'\n \teval \"\n-\t\twant_trace && set -x\n+\t\twant_trace && trace_level_=$(($trace_level_+1)) && set -x\n \t\t$*\"\n }\n \n@@ -926,7 +927,8 @@ test_eval_ () {\n \t\ttest_eval_ret_=$?\n \t\tif want_trace\n \t\tthen\n-\t\t\tset +x\n+\t\t\ttest 1 = $trace_level_ && set +x\n+\t\t\ttrace_level_=$(($trace_level_-1))\n \t\tfi\n \t} 2>/dev/null 4>&2\n \n-- \ngitgitgadget\n\n"},{"id":"394102","messageId":"f69f97e24ba8fc931e7c86931756e1a176348f3c.1585236929.git.gitgitgadget@gmail.com","threadId":"53068","inReplyTo":"pull.728.v3.git.git.1585236929.gitgitgadget@gmail.com","subject":"[PATCH v3 4/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-26T15:35:27Z","receivedAt":"2020-03-26T15:35:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe code to set those prereqs is executed completely outside of any\n`test_eval_` block. As a consequence, its output had to be suppressed so\nthat it does not clutter the output of a regular test script run.\n\nUnfortunately, the output *stays* suppressed even when the `--verbose`\noption is in effect.\n\nThis hid important output when debugging why the GPG prereq was not\nenabled in the Windows part of our CI builds.\n\nIn preparation for fixing that, let's move all of this code into lazy\nprereqs.\n\nThe only slightly tricky part is the global environment variable\n`GNUPGHOME`. Originally, it was configured only when we verified that\nthere is a `gpg` in the `PATH` that we can use. This is now no longer\npossible, as lazy prereqs are evaluated in a subshell that changes the\nworking directory to a temporary one. Therefore, we simply _always_ set\nthat environment variable: it does not hurt anything because it does not\nindicate the presence of a working GPG.\n\nSide note: it was quite tempting to use a hack that is possible because\nwe do not validate what is passed to `test_lazy_prereq` (and it is\ntherefore possible to \"break out\" of the lazy_prereq subshell:\n\n\ttest_lazy_prereq GPG '...) && GNUPGHOME=... && (...'\n\nHowever, this is rather tricksy hobbitses code, and the current patch is\n_much_ easier to understand.\n\nSigned-off-by: Johannes Schindelin \n---\n t/lib-gpg.sh | 102 ++++++++++++++++++++++++++++-----------------------\n 1 file changed, 57 insertions(+), 45 deletions(-)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex 4ead1268351..7a78c562e8d 100755\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -1,12 +1,25 @@\n-gpg_version=$(gpg --version 2>&1)\n-if test $? != 127\n-then\n+# We always set GNUPGHOME, even if no usable GPG was found, as\n+#\n+# - It does not hurt, and\n+#\n+# - we cannot set global environment variables in lazy prereqs because they are\n+# executed in an eval'ed subshell that changes the working directory to a\n+# temporary one.\n+\n+GNUPGHOME=\"$PWD/gpghome\"\n+export GNUPGHOME\n+\n+test_lazy_prereq GPG '\n+\tgpg_version=$(gpg --version 2>&1)\n+\ttest $? != 127 || exit 1\n+\n \t# As said here: http://www.gnupg.org/documentation/faqs.html#q6.19\n-\t# the gpg version 1.0.6 didn't parse trust packets correctly, so for\n+\t# the gpg version 1.0.6 did not parse trust packets correctly, so for\n \t# that version, creation of signed tags using the generated key fails.\n \tcase \"$gpg_version\" in\n-\t'gpg (GnuPG) 1.0.6'*)\n+\t\"gpg (GnuPG) 1.0.6\"*)\n \t\tsay \"Your version of gpg (1.0.6) is too buggy for testing\"\n+\t\texit 1\n \t\t;;\n \t*)\n \t\t# Available key info:\n@@ -25,55 +38,54 @@ then\n \t\t# To export ownertrust:\n \t\t#\tgpg --homedir /tmp/gpghome --export-ownertrust \\\n \t\t#\t\t> lib-gpg/ownertrust\n-\t\tmkdir ./gpghome &&\n-\t\tchmod 0700 ./gpghome &&\n-\t\tGNUPGHOME=\"$PWD/gpghome\" &&\n-\t\texport GNUPGHOME &&\n+\t\tmkdir \"$GNUPGHOME\" &&\n+\t\tchmod 0700 \"$GNUPGHOME\" &&\n \t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n \t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg &&\n \t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import-ownertrust \\\n \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n \t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n-\t\t\t--sign -u committer@example.com &&\n-\t\ttest_set_prereq GPG &&\n-\t\t# Available key info:\n-\t\t# * see t/lib-gpg/gpgsm-gen-key.in\n-\t\t# To generate new certificate:\n-\t\t# * no passphrase\n-\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n-\t\t#\t\t-o /tmp/gpgsm.crt.user \\\n-\t\t#\t\t--generate-key \\\n-\t\t#\t\t--batch t/lib-gpg/gpgsm-gen-key.in\n-\t\t# To import certificate:\n-\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n-\t\t#\t\t--import /tmp/gpgsm.crt.user\n-\t\t# To export into a .p12 we can later import:\n-\t\t#\tgpgsm --homedir /tmp/gpghome/ \\\n-\t\t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n-\t\t#\t\t--export-secret-key-p12 \"committer@example.com\"\n-\t\techo | gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null \\\n-\t\t\t--passphrase-fd 0 --pinentry-mode loopback \\\n-\t\t\t--import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n-\n-\t\tgpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null -K |\n-\t\tgrep fingerprint: |\n-\t\tcut -d\" \" -f4 |\n-\t\ttr -d '\\n' >\"${GNUPGHOME}/trustlist.txt\" &&\n-\n-\t\techo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n-\t\techo hello | gpgsm --homedir \"${GNUPGHOME}\" >/dev/null \\\n-\t\t\t-u committer@example.com -o /dev/null --sign - 2>&1 &&\n-\t\ttest_set_prereq GPGSM\n+\t\t\t--sign -u committer@example.com\n \t\t;;\n \tesac\n-fi\n+'\n+\n+test_lazy_prereq GPGSM '\n+\ttest_have_prereq GPG &&\n+\t# Available key info:\n+\t# * see t/lib-gpg/gpgsm-gen-key.in\n+\t# To generate new certificate:\n+\t# * no passphrase\n+\t#\tgpgsm --homedir /tmp/gpghome/ \\\n+\t#\t\t-o /tmp/gpgsm.crt.user \\\n+\t#\t\t--generate-key \\\n+\t#\t\t--batch t/lib-gpg/gpgsm-gen-key.in\n+\t# To import certificate:\n+\t#\tgpgsm --homedir /tmp/gpghome/ \\\n+\t#\t\t--import /tmp/gpgsm.crt.user\n+\t# To export into a .p12 we can later import:\n+\t#\tgpgsm --homedir /tmp/gpghome/ \\\n+\t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n+\t#\t\t--export-secret-key-p12 \"committer@example.com\"\n+ echo | gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null \\\n+\t --passphrase-fd 0 --pinentry-mode loopback \\\n+\t --import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n+\n+ gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null -K |\n+ grep fingerprint: |\n+ cut -d\" \" -f4 |\n+\ttr -d \"\\\\n\" >\"${GNUPGHOME}/trustlist.txt\" &&\n+\n+ echo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n+ echo hello | gpgsm --homedir \"${GNUPGHOME}\" >/dev/null \\\n+\t -u committer@example.com -o /dev/null --sign - 2>&1\n+'\n \n-if test_have_prereq GPG &&\n- echo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null 2>&1\n-then\n-\ttest_set_prereq RFC1991\n-fi\n+test_lazy_prereq RFC1991 '\n+\ttest_have_prereq GPG &&\n+\techo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null 2>&1\n+'\n \n sanitize_pgp() {\n \tperl -ne '\n-- \ngitgitgadget\n\n"},{"id":"394103","messageId":"064f4e541b8968d05bbd471ec347b9ed91181d72.1585236929.git.gitgitgadget@gmail.com","threadId":"53068","inReplyTo":"pull.728.v3.git.git.1585236929.gitgitgadget@gmail.com","subject":"[PATCH v3 5/5] tests: increase the verbosity of the GPG-related prereqs","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-03-26T15:35:28Z","receivedAt":"2020-03-26T15:35:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nEspecially when debugging a test failure that can only be reproduced in\nthe CI build (e.g. when the developer has no access to a macOS machine\nother than running the tests on a macOS build agent), output should not\nbe suppressed.\n\nIn the instance of `hi/gpg-prefer-check-signature`, where one\nGPG-related test failed for no apparent reason, the entire output of\n`gpg` and `gpgsm` was suppressed, even in verbose mode, leaving\ninterested readers no clue what was going wrong.\n\nLet's fix this by no longer redirecting the output not to `/dev/null`.\nThis is now possible because the affected prereqs were turned into lazy\nones (and are therefore evaluated via `test_eval_` which respects the\n`--verbose` option).\n\nNote that we _still_ redirect `stdout` to `/dev/null` for those commands\nthat sign their `stdin`, as the output would be binary (and useless\nanyway, because the reader would not have anything against which to\ncompare the output).\n\nSigned-off-by: Johannes Schindelin \n---\n t/lib-gpg.sh | 28 ++++++++++++++--------------\n 1 file changed, 14 insertions(+), 14 deletions(-)\n\ndiff --git a/t/lib-gpg.sh b/t/lib-gpg.sh\nindex 7a78c562e8d..9fc5241228e 100755\n--- a/t/lib-gpg.sh\n+++ b/t/lib-gpg.sh\n@@ -40,12 +40,12 @@ test_lazy_prereq GPG '\n \t\t#\t\t> lib-gpg/ownertrust\n \t\tmkdir \"$GNUPGHOME\" &&\n \t\tchmod 0700 \"$GNUPGHOME\" &&\n-\t\t(gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) &&\n-\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import \\\n+\t\t(gpgconf --kill gpg-agent || : ) &&\n+\t\tgpg --homedir \"${GNUPGHOME}\" --import \\\n \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/keyring.gpg &&\n-\t\tgpg --homedir \"${GNUPGHOME}\" 2>/dev/null --import-ownertrust \\\n+\t\tgpg --homedir \"${GNUPGHOME}\" --import-ownertrust \\\n \t\t\t\"$TEST_DIRECTORY\"/lib-gpg/ownertrust &&\n-\t\tgpg --homedir \"${GNUPGHOME}\" /dev/null 2>&1 \\\n+\t\tgpg --homedir \"${GNUPGHOME}\" /dev/null \\\n \t\t\t--sign -u committer@example.com\n \t\t;;\n \tesac\n@@ -68,23 +68,23 @@ test_lazy_prereq GPGSM '\n \t#\tgpgsm --homedir /tmp/gpghome/ \\\n \t#\t\t-o t/lib-gpg/gpgsm_cert.p12 \\\n \t#\t\t--export-secret-key-p12 \"committer@example.com\"\n- echo | gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null \\\n-\t --passphrase-fd 0 --pinentry-mode loopback \\\n-\t --import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n+\techo | gpgsm --homedir \"${GNUPGHOME}\" \\\n+\t\t--passphrase-fd 0 --pinentry-mode loopback \\\n+\t\t--import \"$TEST_DIRECTORY\"/lib-gpg/gpgsm_cert.p12 &&\n \n- gpgsm --homedir \"${GNUPGHOME}\" 2>/dev/null -K |\n- grep fingerprint: |\n- cut -d\" \" -f4 |\n+\tgpgsm --homedir \"${GNUPGHOME}\" -K |\n+\tgrep fingerprint: |\n+\tcut -d\" \" -f4 |\n \ttr -d \"\\\\n\" >\"${GNUPGHOME}/trustlist.txt\" &&\n \n- echo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n- echo hello | gpgsm --homedir \"${GNUPGHOME}\" >/dev/null \\\n-\t -u committer@example.com -o /dev/null --sign - 2>&1\n+\techo \" S relax\" >>\"${GNUPGHOME}/trustlist.txt\" &&\n+\techo hello | gpgsm --homedir \"${GNUPGHOME}\" >/dev/null \\\n+\t -u committer@example.com -o /dev/null --sign -\n '\n \n test_lazy_prereq RFC1991 '\n \ttest_have_prereq GPG &&\n-\techo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null 2>&1\n+\techo | gpg --homedir \"${GNUPGHOME}\" -b --rfc1991 >/dev/null\n '\n \n sanitize_pgp() {\n-- \ngitgitgadget\n"},{"id":"394109","messageId":"xmqqzhc2j4kl.fsf@gitster.c.googlers.com","threadId":"53068","inReplyTo":"20200326082140.GC2200716@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/5] t/lib-gpg.sh: stop pretending to be a stand-alone script","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-26T19:31:38Z","receivedAt":"2020-03-26T19:31:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I wondered if it might be useful to identify the file for editors, etc.\n> But \"*.sh\" is quite sufficient, and we already stripped most of these\n> out long ago in b54a31243b (t/lib-*.sh: drop executable bit,\n> 2020-03-26).\n\nThat timestamp look too fresh to me ;-)\n\nc74c7203 (test: replace shebangs with descriptions in shell\nlibraries, 2013-11-25) was described quite well.\n"},{"id":"394159","messageId":"20200327091004.GA610157@coredump.intra.peff.net","threadId":"53068","inReplyTo":"nycvar.QRO.7.76.6.2003261450590.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-27T09:10:04Z","receivedAt":"2020-03-27T09:10:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 26, 2020 at 03:27:19PM +0100, Johannes Schindelin wrote:\n\n> > OK. This looks good, even if I cannot help feel that my earlier patch\n> > was perfectly sufficient. ;)\n> \n> The mistake is all mine. I had totally missed that you turned GPG into a\n> lazy prereq. So I had my patch finalized already before you pointed my\n> nose at that fact.\n> \n> Sorry about that.\n\nNo problem. And I hope my review didn't sound too passive-aggressive\nwith the \"well, in MY version we did this...\". I focused on the\ndifferences because those were the parts that were new (and therefore\ninteresting) to me. But I don't think any of them are too important\neither way.\n\n> > I have a slight preference for \"return 1\" here. The \"exit 1\" works\n> > because test_lazy_prereq puts us in an implicit subshell. But I think\n> > this sets a bad example for people writing regular tests, where there is\n> > no such subshell (and \"return 1\" is the only correct way to do it).\n> \n> There are two reasons why I did not use `return` here:\n> \n> - To me, prereq code is very distinct from writing tests. Just like we do\n> not &&-chain all the shell functions that live outside of tests, I don't\n> want to &&-chain all the prereq code.\n> \n> The point of the tests' commands is not to fail. The point of prereq's\n> code is to fail if the prereq is not met.\n\nMy only concern is whether people cargo-culting will notice the\ndistinction. But it's probably not a big deal.\n\n> - Since this code is outside of a function, `return` felt like the wrong\n> semantic concept to me. And indeed, I see this (rather scary) part in\n> Bash's documentation of `return` (I did not even bother to look at the\n> POSIX semantics, it scared me so much):\n> \n> The return status is non-zero if `return` is supplied a non-numeric\n> argument, or is used outside a function and not during execution of\n> a script by `.` or `source`.\n> \n> So: the `1` is totally ignored in this context. That alone is reason\n> enough for me to completely avoid it, and use `exit` instead.\n\nI agree the portability rules there are scary, but none of that applies\nbecause we _are_ in a function: test_eval_inner_(). This behavior is\nintentional and due to a7c58f280a (test: cope better with use of return\nfor errors, 2011-08-08). I thought we specifically advertised this\nfeature in t/README, but I can't seem to find it.\n\nSo my perspective was the opposite of yours: \"return\" is the officially\nsanctioned way to exit early from a test snippet, and \"exit\" only\nhappens to work because of the undocumented fact that lazy prereqs\nhappen in a subshell. But it turns out neither was documented. :)\n\n> > In mine I put the test_have_prereq outside the lazy prereq.\n> \n> That makes it essentially a non-lazy prereq.\n> \n> > I don't think it really matters either way (when we later ask if GPGSM\n> > is set, there is no difference between nobody having defined it, and\n> > having a lazy definition that said \"no\").\n> \n> The difference is when running a test with `--run=` where `` refers\n> to a test case that requires neither GPG nor GPGSM or RFC1991. My version\n> will not evaluate the GPG prereq, yours still will.\n\nYes. Part of the reason I kept mine as it was is because it matched the\noriginal behavior better (and I was really only using a lazy prereq\nbecause we didn't have a non-lazy version). But there's really no reason\n_not_ to be lazy, so as long as it isn't breaking anything I think\nthat's a better way forward. (And if it is breaking something, that\nsomething should be fixed).\n\n-Peff\n"},{"id":"394160","messageId":"20200327091206.GB610157@coredump.intra.peff.net","threadId":"53068","inReplyTo":"pull.728.v3.git.git.1585236929.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 0/5] Enable GPG in the Windows part of the CI/PR builds","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-27T09:12:06Z","receivedAt":"2020-03-27T09:12:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 26, 2020 at 03:35:23PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> Changes since v2:\n> \n> * Reordered 4/5 before 3/5, as I had intended originally.\n> \n> \n> * Renamed _trace_level to have a trailing underscore, in line with the\n> surrounding code.\n> \n> \n> * Added a note to the commit message why only lib-gpg.sh loses its\n> hash-bang line, and no other files in t/.\n\nThanks, this version looks fine to me. I left a few other comments\nregarding exit/return in the other part of the thread, but frankly all\nof it is too arcane and insignificant to spend more brain cycles going\nback and forth on. So if I convinced/inspired you on that point, feel\nfree to switch it, but otherwise I'm happy with this iteration.\n\n-Peff\n"},{"id":"394178","messageId":"xmqqr1xdhev8.fsf@gitster.c.googlers.com","threadId":"53068","inReplyTo":"20200327091004.GA610157@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/5] tests: turn GPG, GPGSM and RFC1991 into lazy prereqs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-27T17:44:27Z","receivedAt":"2020-03-27T17:44:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So my perspective was the opposite of yours: \"return\" is the officially\n> sanctioned way to exit early from a test snippet, and \"exit\" only\n> happens to work because of the undocumented fact that lazy prereqs\n> happen in a subshell. But it turns out neither was documented. :)\n\nGood miniproject to document them, I presume. A rough draft\nattached. I do not mind adding \"exit 1 also works in this and that\ncase, but not in that other case\" if the rule can be given succinct\nenough, but I opted for simplicity (mostly because I couldn't come\nup with such a clear rule for \"exit 1\"). \n\nAs long as we are consciouly ensuring that \"return 1\" consistently\nworks everywhere, I didn't see much point offering multiple ways to\ndo the same thing.\n\n> Yes. Part of the reason I kept mine as it was is because it matched the\n> original behavior better (and I was really only using a lazy prereq\n> because we didn't have a non-lazy version). But there's really no reason\n> _not_ to be lazy, so as long as it isn't breaking anything I think\n> that's a better way forward. (And if it is breaking something, that\n> something should be fixed).\n\nYup, I too liked that part of Dscho's version better.\n\n-- >8 --\nSubject: [PATCH] t/README: suggest how to leave test early with failure\n\nOver time, we added the support to our test framework to make it\neasy to leave a test early with failure, but it was not clearly\ndocumented in t/README to help developers writing new tests.\n\nSigned-off-by: Junio C Hamano \n---\n\n * A tangent. 441ee35d (t/README: reformat Do, Don't, Keep in mind\n lists, 2018-10-05) added these lines\n\n Here are the \"do's:\"\n And here are the \"don'ts:\"\n\n Is the placement of the colons on these lines right? I am\n tempted to chase them out of the dq pair and move them at the end\n of their lines, but obviously that is outside of the scope of\n this patch.\n\n t/README | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 55 insertions(+)\n\ndiff --git a/t/README b/t/README\nindex 369e3a9ded..08c8d00bb6 100644\n--- a/t/README\n+++ b/t/README\n@@ -546,6 +546,61 @@ Here are the \"do's:\"\n reports \"ok\" or \"not ok\" to the end user running the tests. Under\n --verbose, they are shown to help debug the tests.\n \n+ - Be careful when you loop\n+\n+ You may need to test multiple things in a loop, but the following\n+ does not work correctly:\n+\n+\ttest_expect_success 'git frotz on various versions' '\n+\t for revision in one two three\n+\t do\n+\t\t echo \"frotz $revision\" >expect &&\n+\t\t git frotz \"$revision\" >actual &&\n+\t\t test_cmp expect actual\n+\t done &&\n+\t test something else\n+\t'\n+\n+ If the output from the \"git frotz\" command did not match what is\n+ expected for 'one' and 'two', but it did for 'three', the loop\n+ itself would not fail, and the test goes on happily. This is not\n+ what you want.\n+\n+ You can work it around in two ways. You could use a separate\n+ \"flag\" variable to carry the failed state out of the loop:\n+\n+\ttest_expect_success 'git frotz on various versions' '\n+\t failed=\n+\t for revision in one two three\n+\t do\n+\t\t echo \"frotz $revision\" >expect &&\n+\t\t git frotz \"$revision\" >actual &&\n+\t\t test_cmp expect actual ||\n+\t\t failed=\"$failed${failed:+ }$revision\"\n+\t done &&\n+\t test -z \"$failed\" &&\n+\t test something else\n+\t'\n+ \n+ Or you can fail the entire loop immediately when you see the\n+ first failure by using \"return 1\" from inside the loop, like so:\n+\n+\ttest_expect_success 'git frotz on various versions' '\n+\t for revision in one two three\n+\t do\n+\t\t echo \"frotz $revision\" >expect &&\n+\t\t git frotz \"$revision\" >actual &&\n+\t\t test_cmp expect actual || return 1\n+\t done &&\n+\t test something else\n+\t'\n+\n+ Note that this is only possible in our test suite, where we\n+ arrange to run your test