| Message ID | 85457a7b61874e8e9f3af9c231451df0aba7a7b5.1585114881.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable GPG in the Windows part of the CI/PR builds | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > -if test_have_prereq GPG && > - echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1 > -then > - test_set_prereq RFC1991 > -fi > +test_lazy_prereq RFC1991 ' > + test_have_prereq GPG && > + echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1 > +' OK. To make it fully lazy, we do need to test GPG while lazily checking for RFC1991. Makes sense. Thanks.
On Wed, Mar 25, 2020 at 05:41:19AM +0000, Johannes Schindelin via GitGitGadget wrote: > In preparation for fixing that, let's move all of this code into lazy > prereqs. OK. This looks good, even if I cannot help feel that my earlier patch was perfectly sufficient. ;) > Side note: it was quite tempting to use a hack that is possible because > we do not validate what is passed to `test_lazy_prereq` (and it is > therefore possible to "break out" of the lazy_prereq subshell: > > test_lazy_prereq GPG '...) && GNUPGHOME=... && (...' No, it is not tempting at all to me to do something so gross. :) > +test_lazy_prereq GPG ' > + gpg_version=$(gpg --version 2>&1) One thing I observed while doing my patch is that lazy_prereq blocks do not get run through the &&-chain linter. So this is OK, but I wonder if we should be future-proofing with braces. I don't care _too_ much either way, though. > + test $? != 127 || exit 1 I have a slight preference for "return 1" here. The "exit 1" works because test_lazy_prereq puts us in an implicit subshell. But I think this sets a bad example for people writing regular tests, where there is no such subshell (and "return 1" is the only correct way to do it). > case "$gpg_version" in > - 'gpg (GnuPG) 1.0.6'*) > + "gpg (GnuPG) 1.0.6"*) > say "Your version of gpg (1.0.6) is too buggy for testing" > + exit 1 Ditto here. > @@ -25,55 +38,54 @@ then > # To export ownertrust: > # gpg --homedir /tmp/gpghome --export-ownertrust \ > # > lib-gpg/ownertrust > - mkdir ./gpghome && > - chmod 0700 ./gpghome && > - GNUPGHOME="$PWD/gpghome" && > - export GNUPGHOME && > + mkdir "$GNUPGHOME" && > + chmod 0700 "$GNUPGHOME" && Compared to mine this keeps the mkdir in the prereq. That seems fine to me. Other prereqs do depend on the directory existing, but they all depend on GPG itself, so they'd be fine. > +test_lazy_prereq GPGSM ' > + test_have_prereq GPG && In mine I put the test_have_prereq outside the lazy prereq. I don't think it really matters either way (when we later ask if GPGSM is set, there is no difference between nobody having defined it, and having a lazy definition that said "no"). -Peff
Hi Peff, On Thu, 26 Mar 2020, Jeff King wrote: > On Wed, Mar 25, 2020 at 05:41:19AM +0000, Johannes Schindelin via GitGitGadget wrote: > > > In preparation for fixing that, let's move all of this code into lazy > > prereqs. > > OK. This looks good, even if I cannot help feel that my earlier patch > was perfectly sufficient. ;) The mistake is all mine. I had totally missed that you turned GPG into a lazy prereq. So I had my patch finalized already before you pointed my nose at that fact. Sorry about that. > > Side note: it was quite tempting to use a hack that is possible because > > we do not validate what is passed to `test_lazy_prereq` (and it is > > therefore possible to "break out" of the lazy_prereq subshell: > > > > test_lazy_prereq GPG '...) && GNUPGHOME=... && (...' > > No, it is not tempting at all to me to do something so gross. :) Well, maybe it was not tempting to _you_, but to _me_, it was so tempting that I had implemented and committed it before I made up my mind and changed it again. > > +test_lazy_prereq GPG ' > > + gpg_version=$(gpg --version 2>&1) > > One thing I observed while doing my patch is that lazy_prereq blocks do > not get run through the &&-chain linter. So this is OK, but I wonder if > we should be future-proofing with braces. I don't care _too_ much either > way, though. I actually like that the prereq blocks are exempt from this && chain linting, and would like to refrain from adding braces "just because". > > + test $? != 127 || exit 1 > > I have a slight preference for "return 1" here. The "exit 1" works > because test_lazy_prereq puts us in an implicit subshell. But I think > this sets a bad example for people writing regular tests, where there is > no such subshell (and "return 1" is the only correct way to do it). There are two reasons why I did not use `return` here: - To me, prereq code is very distinct from writing tests. Just like we do not &&-chain all the shell functions that live outside of tests, I don't want to &&-chain all the prereq code. The point of the tests' commands is not to fail. The point of prereq's code is to fail if the prereq is not met. - Since this code is outside of a function, `return` felt like the wrong semantic concept to me. And indeed, I see this (rather scary) part in Bash's documentation of `return` (I did not even bother to look at the POSIX semantics, it scared me so much): The return status is non-zero if `return` is supplied a non-numeric argument, or is used outside a function and not during execution of a script by `.` or `source`. So: the `1` is totally ignored in this context. That alone is reason enough for me to completely avoid it, and use `exit` instead. > > case "$gpg_version" in > > - 'gpg (GnuPG) 1.0.6'*) > > + "gpg (GnuPG) 1.0.6"*) > > say "Your version of gpg (1.0.6) is too buggy for testing" > > + exit 1 > > Ditto here. > > > @@ -25,55 +38,54 @@ then > > # To export ownertrust: > > # gpg --homedir /tmp/gpghome --export-ownertrust \ > > # > lib-gpg/ownertrust > > - mkdir ./gpghome && > > - chmod 0700 ./gpghome && > > - GNUPGHOME="$PWD/gpghome" && > > - export GNUPGHOME && > > + mkdir "$GNUPGHOME" && > > + chmod 0700 "$GNUPGHOME" && > > Compared to mine this keeps the mkdir in the prereq. That seems fine to > me. Other prereqs do depend on the directory existing, but they all > depend on GPG itself, so they'd be fine. Yes. And conceptually, I like that the prereq is responsible for creating that directory. > > +test_lazy_prereq GPGSM ' > > + test_have_prereq GPG && > > In mine I put the test_have_prereq outside the lazy prereq. That makes it essentially a non-lazy prereq. > I don't think it really matters either way (when we later ask if GPGSM > is set, there is no difference between nobody having defined it, and > having a lazy definition that said "no"). The difference is when running a test with `--run=<n>` where `<n>` refers to a test case that requires neither GPG nor GPGSM or RFC1991. My version will not evaluate the GPG prereq, yours still will. Ciao, Dscho
On Thu, Mar 26, 2020 at 03:27:19PM +0100, Johannes Schindelin wrote: > > OK. This looks good, even if I cannot help feel that my earlier patch > > was perfectly sufficient. ;) > > The mistake is all mine. I had totally missed that you turned GPG into a > lazy prereq. So I had my patch finalized already before you pointed my > nose at that fact. > > Sorry about that. No problem. And I hope my review didn't sound too passive-aggressive with the "well, in MY version we did this...". I focused on the differences because those were the parts that were new (and therefore interesting) to me. But I don't think any of them are too important either way. > > I have a slight preference for "return 1" here. The "exit 1" works > > because test_lazy_prereq puts us in an implicit subshell. But I think > > this sets a bad example for people writing regular tests, where there is > > no such subshell (and "return 1" is the only correct way to do it). > > There are two reasons why I did not use `return` here: > > - To me, prereq code is very distinct from writing tests. Just like we do > not &&-chain all the shell functions that live outside of tests, I don't > want to &&-chain all the prereq code. > > The point of the tests' commands is not to fail. The point of prereq's > code is to fail if the prereq is not met. My only concern is whether people cargo-culting will notice the distinction. But it's probably not a big deal. > - Since this code is outside of a function, `return` felt like the wrong > semantic concept to me. And indeed, I see this (rather scary) part in > Bash's documentation of `return` (I did not even bother to look at the > POSIX semantics, it scared me so much): > > The return status is non-zero if `return` is supplied a non-numeric > argument, or is used outside a function and not during execution of > a script by `.` or `source`. > > So: the `1` is totally ignored in this context. That alone is reason > enough for me to completely avoid it, and use `exit` instead. I agree the portability rules there are scary, but none of that applies because we _are_ in a function: test_eval_inner_(). This behavior is intentional and due to a7c58f280a (test: cope better with use of return for errors, 2011-08-08). I thought we specifically advertised this feature in t/README, but I can't seem to find it. So my perspective was the opposite of yours: "return" is the officially sanctioned way to exit early from a test snippet, and "exit" only happens to work because of the undocumented fact that lazy prereqs happen in a subshell. But it turns out neither was documented. :) > > In mine I put the test_have_prereq outside the lazy prereq. > > That makes it essentially a non-lazy prereq. > > > I don't think it really matters either way (when we later ask if GPGSM > > is set, there is no difference between nobody having defined it, and > > having a lazy definition that said "no"). > > The difference is when running a test with `--run=<n>` where `<n>` refers > to a test case that requires neither GPG nor GPGSM or RFC1991. My version > will not evaluate the GPG prereq, yours still will. Yes. Part of the reason I kept mine as it was is because it matched the original behavior better (and I was really only using a lazy prereq because we didn't have a non-lazy version). But there's really no reason _not_ to be lazy, so as long as it isn't breaking anything I think that's a better way forward. (And if it is breaking something, that something should be fixed). -Peff
Jeff King <peff@peff.net> writes: > So my perspective was the opposite of yours: "return" is the officially > sanctioned way to exit early from a test snippet, and "exit" only > happens to work because of the undocumented fact that lazy prereqs > happen in a subshell. But it turns out neither was documented. :) Good miniproject to document them, I presume. A rough draft attached. I do not mind adding "exit 1 also works in this and that case, but not in that other case" if the rule can be given succinct enough, but I opted for simplicity (mostly because I couldn't come up with such a clear rule for "exit 1"). As long as we are consciouly ensuring that "return 1" consistently works everywhere, I didn't see much point offering multiple ways to do the same thing. > Yes. Part of the reason I kept mine as it was is because it matched the > original behavior better (and I was really only using a lazy prereq > because we didn't have a non-lazy version). But there's really no reason > _not_ to be lazy, so as long as it isn't breaking anything I think > that's a better way forward. (And if it is breaking something, that > something should be fixed). Yup, I too liked that part of Dscho's version better. -- >8 -- Subject: [PATCH] t/README: suggest how to leave test early with failure Over time, we added the support to our test framework to make it easy to leave a test early with failure, but it was not clearly documented in t/README to help developers writing new tests. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * A tangent. 441ee35d (t/README: reformat Do, Don't, Keep in mind lists, 2018-10-05) added these lines Here are the "do's:" And here are the "don'ts:" Is the placement of the colons on these lines right? I am tempted to chase them out of the dq pair and move them at the end of their lines, but obviously that is outside of the scope of this patch. t/README | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/t/README b/t/README index 369e3a9ded..08c8d00bb6 100644 --- a/t/README +++ b/t/README @@ -546,6 +546,61 @@ Here are the "do's:" reports "ok" or "not ok" to the end user running the tests. Under --verbose, they are shown to help debug the tests. + - Be careful when you loop + + You may need to test multiple things in a loop, but the following + does not work correctly: + + test_expect_success 'git frotz on various versions' ' + for revision in one two three + do + echo "frotz $revision" >expect && + git frotz "$revision" >actual && + test_cmp expect actual + done && + test something else + ' + + If the output from the "git frotz" command did not match what is + expected for 'one' and 'two', but it did for 'three', the loop + itself would not fail, and the test goes on happily. This is not + what you want. + + You can work it around in two ways. You could use a separate + "flag" variable to carry the failed state out of the loop: + + test_expect_success 'git frotz on various versions' ' + failed= + for revision in one two three + do + echo "frotz $revision" >expect && + git frotz "$revision" >actual && + test_cmp expect actual || + failed="$failed${failed:+ }$revision" + done && + test -z "$failed" && + test something else + ' + + Or you can fail the entire loop immediately when you see the + first failure by using "return 1" from inside the loop, like so: + + test_expect_success 'git frotz on various versions' ' + for revision in one two three + do + echo "frotz $revision" >expect && + git frotz "$revision" >actual && + test_cmp expect actual || return 1 + done && + test something else + ' + + Note that this is only possible in our test suite, where we + arrange to run your test <script> wrapped inside a helper + function to ensure that return values matter; in your own script + outside any function, this technique may not work. + + And here are the "don'ts:" - Don't exit() within a <script> part.
On Fri, Mar 27, 2020 at 1:44 PM Junio C Hamano <gitster@pobox.com> wrote: > Subject: [PATCH] t/README: suggest how to leave test early with failure > + test_expect_success 'git frotz on various versions' ' > + for revision in one two three > + do > + echo "frotz $revision" >expect && > + git frotz "$revision" >actual && > + test_cmp expect actual || return 1 > + done && > + test something else > + ' > + > + Note that this is only possible in our test suite, where we > + arrange to run your test <script> wrapped inside a helper > + function to ensure that return values matter; in your own script > + outside any function, this technique may not work. > + > And here are the "don'ts:" > > - Don't exit() within a <script> part. We use 'exit 1' to terminate subshells[1] inside tests. [1]: https://lore.kernel.org/git/20150325052952.GE31924@peff.net/
Eric Sunshine <sunshine@sunshineco.com> writes: > On Fri, Mar 27, 2020 at 1:44 PM Junio C Hamano <gitster@pobox.com> wrote: >> Subject: [PATCH] t/README: suggest how to leave test early with failure >> + test_expect_success 'git frotz on various versions' ' >> + for revision in one two three >> + do >> + echo "frotz $revision" >expect && >> + git frotz "$revision" >actual && >> + test_cmp expect actual || return 1 >> + done && >> + test something else >> + ' >> + >> + Note that this is only possible in our test suite, where we >> + arrange to run your test <script> wrapped inside a helper >> + function to ensure that return values matter; in your own script >> + outside any function, this technique may not work. >> + >> And here are the "don'ts:" >> >> - Don't exit() within a <script> part. > > We use 'exit 1' to terminate subshells[1] inside tests. > > [1]: https://lore.kernel.org/git/20150325052952.GE31924@peff.net/ Yeah, I gave two alternatives, but the third one could be test_expect_success 'git frotz on various versions' ' ( for revision in one two three do echo "frotz $revision" >expect && git frotz "$revision" >actual && test_cmp expect actual || exit 1 done ) && test something else ' Anyway, that existing rule is not what I added in the rough draft under discussion. To clarify it, we'd end up needing "unless A, B or C" that may be too complex. I dunno. Thanks.
On Fri, Mar 27, 2020 at 10:44:27AM -0700, Junio C Hamano wrote: > -- >8 -- > Subject: [PATCH] t/README: suggest how to leave test early with failure > > Over time, we added the support to our test framework to make it > easy to leave a test early with failure, but it was not clearly > documented in t/README to help developers writing new tests. Thanks for getting started on this. Everything you wrote looks correct, but I think we can be more succinct. And I think it's worth being so, since there are a lot of "do's" already, and we don't want to overwhelm the user. > + - Be careful when you loop > + > + You may need to test multiple things in a loop, but the following > + does not work correctly: > + > + test_expect_success 'git frotz on various versions' ' > + for revision in one two three > + do > + echo "frotz $revision" >expect && > + git frotz "$revision" >actual && > + test_cmp expect actual > + done && > + test something else > + ' We could shrink this example down to the bare minimum. Perhaps: for i in a b c; do do_something "$i" done && do_something_else The key thing is that the "&&" will pick up only the value of "do_something $c" and will ignore "a" and "b". That might be too dense, but it does reduce any cognitive burden from unimportant details. > + If the output from the "git frotz" command did not match what is > + expected for 'one' and 'two', but it did for 'three', the loop > + itself would not fail, and the test goes on happily. This is not > + what you want. This explanation works fine, though I think you can also explain it as: The result code of a shell loop is the result of the final iteration; the results of do_something for "a" and "b" are discarded, and we'd pass the test even if they fail. (I'm happy with either, just thinking out loud). > + You can work it around in two ways. You could use a separate > + "flag" variable to carry the failed state out of the loop: IMHO it's not worth giving this alternative. It's perfectly valid, but we promise the "return" version works exactly so we don't have to deal deal with this ugly and error-prone code. > + Or you can fail the entire loop immediately when you see the > + first failure by using "return 1" from inside the loop, like so: I think we can jump straight to "in our test suite", like: One way around this is to break out of the loop when we see a failure. All test_expect_* snippets are executed inside a function, allowing an immediate return on failure: for i in a b c; do do_something "$i" || return 1 done && do_something_else Possibly we'd also want to say: Note that we still &&-chain the loop to propagate failures from earlier commands. but certainly the &&-chain linter would remind them of that. :) > And here are the "don'ts:" > > - Don't exit() within a <script> part. > * A tangent. 441ee35d (t/README: reformat Do, Don't, Keep in mind > lists, 2018-10-05) added these lines > > Here are the "do's:" > And here are the "don'ts:" > > Is the placement of the colons on these lines right? I am > tempted to chase them out of the dq pair and move them at the end > of their lines, but obviously that is outside of the scope of > this patch. I think it's a matter of taste. Most writing style guides put punctuation inside quotes, but they're also expecting the output to be typeset, where a trailing period ends up more under the quotes than beside. I'm not sure what such style guides would say about colons, as they're a bit taller. But regardless, I actually prefer putting punctuation outside of the quotes. It looks better to me in a fixed-width terminal setting. Plus I guess as a programmer it feels like a parsing error. ;) I don't know that it matters too much either way, though. -Peff
On Fri, Mar 27, 2020 at 02:37:02PM -0700, Junio C Hamano wrote: > >> And here are the "don'ts:" > >> > >> - Don't exit() within a <script> part. > > > > We use 'exit 1' to terminate subshells[1] inside tests. > > > > [1]: https://lore.kernel.org/git/20150325052952.GE31924@peff.net/ > > Yeah, I gave two alternatives, but the third one could be > > test_expect_success 'git frotz on various versions' ' > ( > for revision in one two three > do > echo "frotz $revision" >expect && > git frotz "$revision" >actual && > test_cmp expect actual || exit 1 > done > ) && > test something else > ' We definitely shouldn't suggest _introducing_ a subshell for this purpose. It's longer to write and less efficient. > Anyway, that existing rule is not what I added in the rough draft > under discussion. To clarify it, we'd end up needing "unless A, B > or C" that may be too complex. I dunno. I think the existing rule is OK. If you know enough to create the situation where "exit 1" is useful, then you probably know enough to know when to break that rule. That said, I'm not opposed to something like: - Don't call "exit" within a <script> part, unless you're in a subshell. It will cause the whole to test script to exit (and fail). or something. -Peff
Hi Peff, On Fri, 27 Mar 2020, Jeff King wrote: > On Thu, Mar 26, 2020 at 03:27:19PM +0100, Johannes Schindelin wrote: > > > > OK. This looks good, even if I cannot help feel that my earlier patch > > > was perfectly sufficient. ;) > > > > The mistake is all mine. I had totally missed that you turned GPG into a > > lazy prereq. So I had my patch finalized already before you pointed my > > nose at that fact. > > > > Sorry about that. > > No problem. And I hope my review didn't sound too passive-aggressive > with the "well, in MY version we did this...". FWIW I failed to interpret anything in your reply as passive-aggressive, probably because I am just too used to receive competent, helpful and friendly replies from you. > I focused on the differences because those were the parts that were new > (and therefore interesting) to me. But I don't think any of them are too > important either way. To me, it all sounded like a constructive discussion we had, and since you already had a working patch that did something very similar to mine, it made sense to look at their differences. > > - Since this code is outside of a function, `return` felt like the wrong > > semantic concept to me. And indeed, I see this (rather scary) part in > > Bash's documentation of `return` (I did not even bother to look at the > > POSIX semantics, it scared me so much): > > > > The return status is non-zero if `return` is supplied a non-numeric > > argument, or is used outside a function and not during execution of > > a script by `.` or `source`. > > > > So: the `1` is totally ignored in this context. That alone is reason > > enough for me to completely avoid it, and use `exit` instead. > > I agree the portability rules there are scary, but none of that applies > because we _are_ in a function: test_eval_inner_(). This behavior is > intentional and due to a7c58f280a (test: cope better with use of return > for errors, 2011-08-08). I thought we specifically advertised this > feature in t/README, but I can't seem to find it. > > So my perspective was the opposite of yours: "return" is the officially > sanctioned way to exit early from a test snippet, and "exit" only > happens to work because of the undocumented fact that lazy prereqs > happen in a subshell. But it turns out neither was documented. :) Can a subshell inside a function cause a `return` from said function? I don't think so, but let's put that to a test: function return_from_a_subshell () { echo before (echo subshell; return; echo unreachable) echo after $? } Let's run that. $ return_from_a_subshell before subshell after 0 To me, the fact that that `return` does not return from the function, but only exits the subshell, in my mind lends more credence to the idea that `exit` is more appropriate in this context than `return`. For shiggles, I also added that `$?` because I really, _really_ wanted to know whether my reading of GNU Bash's documentation was correct, and it appears I was mistaken: apparently `return` used outside a function does _not_ cause a non-zero exit code. > > > In mine I put the test_have_prereq outside the lazy prereq. > > > > That makes it essentially a non-lazy prereq. > > > > > I don't think it really matters either way (when we later ask if GPGSM > > > is set, there is no difference between nobody having defined it, and > > > having a lazy definition that said "no"). > > > > The difference is when running a test with `--run=<n>` where `<n>` refers > > to a test case that requires neither GPG nor GPGSM or RFC1991. My version > > will not evaluate the GPG prereq, yours still will. > > Yes. Part of the reason I kept mine as it was is because it matched the > original behavior better (and I was really only using a lazy prereq > because we didn't have a non-lazy version). But there's really no reason > _not_ to be lazy, so as long as it isn't breaking anything I think > that's a better way forward. (And if it is breaking something, that > something should be fixed). I'm glad you agree! Thanks, Dscho
On Mon, Mar 30, 2020 at 08:39:08PM +0200, Johannes Schindelin wrote: > > So my perspective was the opposite of yours: "return" is the officially > > sanctioned way to exit early from a test snippet, and "exit" only > > happens to work because of the undocumented fact that lazy prereqs > > happen in a subshell. But it turns out neither was documented. :) > > Can a subshell inside a function cause a `return` from said function? I > don't think so, but let's put that to a test: > [...] > To me, the fact that that `return` does not return from the function, but > only exits the subshell, in my mind lends more credence to the idea that > `exit` is more appropriate in this context than `return`. Hmm, yeah, I was wrong about it actually returning from the function. Thanks for demonstrating. Returning from just the subshell in the case of lazy_prereq is OK for our purposes, since the exit value of the subshell is taken as the result of the prereq check (and in turn becomes the return value of that function anyway). But it does make more sympathetic to the idea that "exit" is appropriate here. Especially given the prodding below (which you can skip to the last paragraph if you're not interested in shell arcana): > For shiggles, I also added that `$?` because I really, _really_ wanted to > know whether my reading of GNU Bash's documentation was correct, and it > appears I was mistaken: apparently `return` used outside a function does > _not_ cause a non-zero exit code. I think the issue may be in the definition of "outside a function". If we really are at the top-level outside of a function, then return gives a non-zero exit but _doesn't_ return in bash: $ bash -c 'return 2; echo inside=$?'; echo outside=$? bash: line 0: return: can only `return' from a function or sourced script inside=1 outside=0 So even though we asked to return 2, it gave us a generic "1" return code and continued executing (and then outside=0 because the echo was successful). And that's true even in a subshell (not we moved "outside" into the bash process so we can see that it keeps going): $ bash -c '(return 2; echo inside=$?); echo outside=$?' bash: line 0: return: can only `return' from a function or sourced script inside=1 outside=0 But if we actually _are_ inside a function, even inside a subshell, then return "works", by stopping execution in the subshell and returning the value we asked (in your example we got "0" because you didn't specify a value for "return", so it just propagated the exit code of the earlier "echo"). $ bash -c 'f() { (return 2; echo inside=$?); echo outside=$?; }; f' outside=2 It's just a bit odd (to me) that it still runs the rest of the function. Dash behaves a bit more sensibly with an out-of-function return, just returning from the script: $ dash -c 'return 2; echo inside=$?'; echo outside=$? outside=2 and with a subshell, it returns only from that subshell: $ dash -c '(return 2; echo inside=$?); echo outside=$?' outside=2 So inside a subshell-in-a-function, it behaves exactly the same (returning from the subshell but not the function). I think the behavior of both shells is fine for our purposes. We _are_ in a function, so as long as we return from the subshell immediately we're happy. But given the oddities in how bash behaves, and the fact that POSIX says: If the shell is not currently executing a function or dot script, the results are unspecified. it may be better to stay away from the question entirely. -Peff
diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh index 4ead1268351..7a78c562e8d 100755 --- a/t/lib-gpg.sh +++ b/t/lib-gpg.sh @@ -1,12 +1,25 @@ -gpg_version=$(gpg --version 2>&1) -if test $? != 127 -then +# We always set GNUPGHOME, even if no usable GPG was found, as +# +# - It does not hurt, and +# +# - we cannot set global environment variables in lazy prereqs because they are +# executed in an eval'ed subshell that changes the working directory to a +# temporary one. + +GNUPGHOME="$PWD/gpghome" +export GNUPGHOME + +test_lazy_prereq GPG ' + gpg_version=$(gpg --version 2>&1) + test $? != 127 || exit 1 + # As said here: http://www.gnupg.org/documentation/faqs.html#q6.19 - # the gpg version 1.0.6 didn't parse trust packets correctly, so for + # the gpg version 1.0.6 did not parse trust packets correctly, so for # that version, creation of signed tags using the generated key fails. case "$gpg_version" in - 'gpg (GnuPG) 1.0.6'*) + "gpg (GnuPG) 1.0.6"*) say "Your version of gpg (1.0.6) is too buggy for testing" + exit 1 ;; *) # Available key info: @@ -25,55 +38,54 @@ then # To export ownertrust: # gpg --homedir /tmp/gpghome --export-ownertrust \ # > lib-gpg/ownertrust - mkdir ./gpghome && - chmod 0700 ./gpghome && - GNUPGHOME="$PWD/gpghome" && - export GNUPGHOME && + mkdir "$GNUPGHOME" && + chmod 0700 "$GNUPGHOME" && (gpgconf --kill gpg-agent >/dev/null 2>&1 || : ) && gpg --homedir "${GNUPGHOME}" 2>/dev/null --import \ "$TEST_DIRECTORY"/lib-gpg/keyring.gpg && gpg --homedir "${GNUPGHOME}" 2>/dev/null --import-ownertrust \ "$TEST_DIRECTORY"/lib-gpg/ownertrust && gpg --homedir "${GNUPGHOME}" </dev/null >/dev/null 2>&1 \ - --sign -u committer@example.com && - test_set_prereq GPG && - # Available key info: - # * see t/lib-gpg/gpgsm-gen-key.in - # To generate new certificate: - # * no passphrase - # gpgsm --homedir /tmp/gpghome/ \ - # -o /tmp/gpgsm.crt.user \ - # --generate-key \ - # --batch t/lib-gpg/gpgsm-gen-key.in - # To import certificate: - # gpgsm --homedir /tmp/gpghome/ \ - # --import /tmp/gpgsm.crt.user - # To export into a .p12 we can later import: - # gpgsm --homedir /tmp/gpghome/ \ - # -o t/lib-gpg/gpgsm_cert.p12 \ - # --export-secret-key-p12 "committer@example.com" - echo | gpgsm --homedir "${GNUPGHOME}" 2>/dev/null \ - --passphrase-fd 0 --pinentry-mode loopback \ - --import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 && - - gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K | - grep fingerprint: | - cut -d" " -f4 | - tr -d '\n' >"${GNUPGHOME}/trustlist.txt" && - - echo " S relax" >>"${GNUPGHOME}/trustlist.txt" && - echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \ - -u committer@example.com -o /dev/null --sign - 2>&1 && - test_set_prereq GPGSM + --sign -u committer@example.com ;; esac -fi +' + +test_lazy_prereq GPGSM ' + test_have_prereq GPG && + # Available key info: + # * see t/lib-gpg/gpgsm-gen-key.in + # To generate new certificate: + # * no passphrase + # gpgsm --homedir /tmp/gpghome/ \ + # -o /tmp/gpgsm.crt.user \ + # --generate-key \ + # --batch t/lib-gpg/gpgsm-gen-key.in + # To import certificate: + # gpgsm --homedir /tmp/gpghome/ \ + # --import /tmp/gpgsm.crt.user + # To export into a .p12 we can later import: + # gpgsm --homedir /tmp/gpghome/ \ + # -o t/lib-gpg/gpgsm_cert.p12 \ + # --export-secret-key-p12 "committer@example.com" + echo | gpgsm --homedir "${GNUPGHOME}" 2>/dev/null \ + --passphrase-fd 0 --pinentry-mode loopback \ + --import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 && + + gpgsm --homedir "${GNUPGHOME}" 2>/dev/null -K | + grep fingerprint: | + cut -d" " -f4 | + tr -d "\\n" >"${GNUPGHOME}/trustlist.txt" && + + echo " S relax" >>"${GNUPGHOME}/trustlist.txt" && + echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \ + -u committer@example.com -o /dev/null --sign - 2>&1 +' -if test_have_prereq GPG && - echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1 -then - test_set_prereq RFC1991 -fi +test_lazy_prereq RFC1991 ' + test_have_prereq GPG && + echo | gpg --homedir "${GNUPGHOME}" -b --rfc1991 >/dev/null 2>&1 +' sanitize_pgp() { perl -ne '