{"thread":{"id":"40274","subject":"[RFC] test_when_finished in subshells","startedAt":"2015-09-04T17:58:45Z","lastAt":"2015-09-06T16:22:19Z","messageCount":15,"participants":["John Keeping","Junio C Hamano","Jeff King","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"269433","messageId":"54923cf9cc5a66bf9034051b3c2f930fa7ef88a4.1441388803.git.john@keeping.me.uk","threadId":"40274","inReplyTo":null,"subject":"[RFC] test_when_finished in subshells","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-09-04T17:58:45Z","receivedAt":"2015-09-04T17:58:45Z","isPatch":false,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"A recent thread [0] made me realise that using test_when_finished in a\nsubshell is likely to be a bug, since the change to $test_cleanup will\nbe lost when the subshell exits.\n\nThere is no POSIX way to detect that we are in a subshell ($$ and $PPID\nare specified to remain unchanged), but we can detect it on Bash and\ngracefully fall back to disabling the test on other shells, which is\nwhat the patch below does.\n\nThere are three tests currently in master that fail with this patch (at\nleast on my system):\n\n\tTest Summary Report\n\t-------------------\n\tt7610-mergetool.sh                               (Wstat: 256 Tests: 18 Failed: 1)\n\t  Failed test:  7\n\t  Non-zero exit status: 1\n\tt7800-difftool.sh                                (Wstat: 256 Tests: 56 Failed: 1)\n\t  Failed test:  56\n\t  Non-zero exit status: 1\n\tt5801-remote-helpers.sh                          (Wstat: 256 Tests: 30 Failed: 1)\n\t  Failed test:  27\n\t  Non-zero exit status: 1\n\nAll are harmless at the moment and t7610 and t5801 can be fixed by\nmoving the test_when_finished call out of the subshell relatively\neasily.\n\nt7800 (in its final test) calls test_config in a subshell which has cd'd\ninto a submodule.\n\nIs this something worth worrying about, or is it sufficiently rare that\nwe can live with the current behaviour?\n\n[0] http://article.gmane.org/gmane.comp.version-control.git/277199\n\n-- >8 --\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex e8d3c0f..d29cd7b 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -722,6 +722,8 @@ test_seq () {\n # what went wrong.\n \n test_when_finished () {\n+\ttest \"${BASH_SUBSHELL-0}\" = 0 ||\n+\terror \"bug in test script: test_when_finished does nothing in a subshell\"\n \ttest_cleanup=\"{ $*\n \t\t} && (exit \\\"\\$eval_ret\\\"); eval_ret=\\$?; $test_cleanup\"\n }\n-- \n2.5.0.466.g9af26fa\n"},{"id":"269440","messageId":"xmqqfv2uf2kc.fsf@gitster.mtv.corp.google.com","threadId":"40274","inReplyTo":"54923cf9cc5a66bf9034051b3c2f930fa7ef88a4.1441388803.git.john@keeping.me.uk","subject":"Re: [RFC] test_when_finished in subshells","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-04T18:43:15Z","receivedAt":"2015-09-04T18:43:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping <john@keeping.me.uk> writes:\n\n> All are harmless at the moment and t7610 and t5801 can be fixed by\n> moving the test_when_finished call out of the subshell relatively\n> easily.\n>\n> t7800 (in its final test) calls test_config in a subshell which has cd'd\n> into a submodule.\n>\n> Is this something worth worrying about, or is it sufficiently rare that\n> we can live with the current behaviour?\n\nFixing the instances you found is good, obviously ;-).  Thanks for\nworking on this.\n\nEven though the proposed detection is BASH-ism, I think it would not\nhurt other shells (they obviously do not help you catch bugs, but\nthey would not misbehave as long as you make sure BASH_SUBSHELL is\neither unset or set to 0 at the beginning of the test), and the only\nimpact to them would be a invocation of (often built-in) 'test'\nutility, whose performance impact should be miniscule.\n\nI'll wait for opinion from others, of course.\n\n>\n> [0] http://article.gmane.org/gmane.comp.version-control.git/277199\n>\n> -- >8 --\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index e8d3c0f..d29cd7b 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -722,6 +722,8 @@ test_seq () {\n>  # what went wrong.\n>  \n>  test_when_finished () {\n> +\ttest \"${BASH_SUBSHELL-0}\" = 0 ||\n> +\terror \"bug in test script: test_when_finished does nothing in a subshell\"\n>  \ttest_cleanup=\"{ $*\n>  \t\t} && (exit \\\"\\$eval_ret\\\"); eval_ret=\\$?; $test_cleanup\"\n>  }\n"},{"id":"269474","messageId":"20150905085429.GB25039@sigill.intra.peff.net","threadId":"40274","inReplyTo":"xmqqfv2uf2kc.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] test_when_finished in subshells","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-05T08:54:30Z","receivedAt":"2015-09-05T08:54:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 04, 2015 at 11:43:15AM -0700, Junio C Hamano wrote:\n\n> > t7800 (in its final test) calls test_config in a subshell which has cd'd\n> > into a submodule.\n> >\n> > Is this something worth worrying about, or is it sufficiently rare that\n> > we can live with the current behaviour?\n> \n> Fixing the instances you found is good, obviously ;-).  Thanks for\n> working on this.\n> \n> Even though the proposed detection is BASH-ism, I think it would not\n> hurt other shells (they obviously do not help you catch bugs, but\n> they would not misbehave as long as you make sure BASH_SUBSHELL is\n> either unset or set to 0 at the beginning of the test), and the only\n> impact to them would be a invocation of (often built-in) 'test'\n> utility, whose performance impact should be miniscule.\n> \n> I'll wait for opinion from others, of course.\n\nI like it. In general I'm in favor of any lint-like fixes (whether for\nthe tests or the C code itself) as long as:\n\n  1. they don't create false positive noise\n\n  2. they don't require extra effort at each call-site\n\n  3. they don't have a performance impact\n\nAnd I think this passes all three. Of course it would be nice if the new\ncheck ran on all shells, but even this seems like a strict improvement.\n\nAnd I couldn't come up with anything better. I thought we might be able\nto use a canary trap, like this:\n\n  trap canary QUIT\n  echo outside\n  trap\n  echo inside\n  (trap)\n\nBecause traps are reset inside a subshell, the first \"trap\" without\narguments should print a string with \"canary\" in it, and the second\nshould print nothing.\n\nExcept that it isn't remotely how it works in practice. :) Bash leaves\nthe trap in place inside the subshell. And while dash does the right\nthing, any attempt to actually _look_ at the output will put us in a\nsubshell. So:\n\n  case \"$(trap)\" in\n  *canary*) ... not in a subshell ...\n  *) ... in a subshell ...\n  esac\n\ndoesn't work; the trap report inside backticks is always empty (despite\nthere being an explicit example in the POSIX manpage for trap of using\n$(trap) to get the value for a later eval).\n\nIt looks like if we use the EXIT trap rather than a signal, bash _does_\ndo the right thing (which kind of makes sense, I guess). So rather than\na custom canary, we could use our regular EXIT handler as the canary.\nBut I couldn't find a way to work around dash's issue.\n\nLooks like there was even some discussion in 2010:\n\n  http://www.spinics.net/lists/dash/msg00331.html\n\nbut my dash 0.5.7 remains broken. Oh, well.\n\n-Peff\n"},{"id":"269480","messageId":"cover.1441458341.git.john@keeping.me.uk","threadId":"40274","inReplyTo":"20150905085429.GB25039@sigill.intra.peff.net","subject":"[PATCH 0/5] disallow test_when_finished in subshells","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-09-05T13:12:44Z","receivedAt":"2015-09-05T13:12:44Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sat, Sep 05, 2015 at 04:54:30AM -0400, Jeff King wrote:\n> On Fri, Sep 04, 2015 at 11:43:15AM -0700, Junio C Hamano wrote:\n> \n> > > t7800 (in its final test) calls test_config in a subshell which has cd'd\n> > > into a submodule.\n> > >\n> > > Is this something worth worrying about, or is it sufficiently rare that\n> > > we can live with the current behaviour?\n> > \n> > Fixing the instances you found is good, obviously ;-).  Thanks for\n> > working on this.\n> > \n> > Even though the proposed detection is BASH-ism, I think it would not\n> > hurt other shells (they obviously do not help you catch bugs, but\n> > they would not misbehave as long as you make sure BASH_SUBSHELL is\n> > either unset or set to 0 at the beginning of the test), and the only\n> > impact to them would be a invocation of (often built-in) 'test'\n> > utility, whose performance impact should be miniscule.\n> > \n> > I'll wait for opinion from others, of course.\n> \n> I like it. In general I'm in favor of any lint-like fixes (whether for\n> the tests or the C code itself) as long as:\n> \n>   1. they don't create false positive noise\n> \n>   2. they don't require extra effort at each call-site\n> \n>   3. they don't have a performance impact\n> \n> And I think this passes all three. Of course it would be nice if the new\n> check ran on all shells, but even this seems like a strict improvement.\n\nHere are the changes to do this.  The first two are pretty\nstraightforward, but the t7800 change may be more controversial; in this\nparticular case we could get away with using \"git config\" instead of\ntest_config but I think the more generic solution will be better for the\nfuture.\n\nI don't think it's worth trying to clear $BASH_SUBSHELL before the tests\nstart because to do so we have to reliably detect that we're not running\nunder Bash, and if we don't trust people not to set $BASH_SUBSHELL why\ndo we trust them not to set $BASH?\n\n\nJohn Keeping (5):\n  t7610: don't use test_config in a subshell\n  t5801: don't use test_when_finished in a subshell\n  test-lib-functions: support \"test_config -C <dir> ...\"\n  t7800: don't use test_config in a subshell\n  test-lib-functions: detect test_when_finished in subshell\n\n t/t5801-remote-helpers.sh | 12 ++++--------\n t/t7610-mergetool.sh      |  2 +-\n t/t7800-difftool.sh       |  8 ++++----\n t/test-lib-functions.sh   | 25 ++++++++++++++++++++++---\n 4 files changed, 31 insertions(+), 16 deletions(-)\n\n-- \n2.5.0.466.g9af26fa\n"},{"id":"269479","messageId":"dc4119cc27db2636dfefdd9045161fe47302f780.1441458341.git.john@keeping.me.uk","threadId":"40274","inReplyTo":"cover.1441458341.git.john@keeping.me.uk","subject":"[PATCH 1/5] t7610: don't use test_config in a subshell","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-09-05T13:12:45Z","receivedAt":"2015-09-05T13:12:45Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"test_config uses test_when_finished to reset the configuration after the\ntest, but this does not work inside a subshell.  This does not cause a\nproblem here because the first thing the next test does is to set this\nconfig variable itself, but we are about to add a check that will\ncomplain when test_when_finished is used in a subshell.\n\nIn this case, \"subdir\" not a submodule so test_config has the same\neffect when run at the top level and can simply be moved out of the\nsubshell.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n t/t7610-mergetool.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex 7eeb207..6f12b23 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -174,9 +174,9 @@ test_expect_success 'mergetool skips autoresolved' '\n '\n \n test_expect_success 'mergetool merges all from subdir' '\n+\ttest_config rerere.enabled false &&\n \t(\n \t\tcd subdir &&\n-\t\ttest_config rerere.enabled false &&\n \t\ttest_must_fail git merge master &&\n \t\t( yes \"r\" | git mergetool ../submod ) &&\n \t\t( yes \"d\" \"d\" | git mergetool --no-prompt ) &&\n-- \n2.5.0.466.g9af26fa\n"},{"id":"269481","messageId":"41e0d2245954e80135d33acbb616c2a56605759a.1441458341.git.john@keeping.me.uk","threadId":"40274","inReplyTo":"cover.1441458341.git.john@keeping.me.uk","subject":"[PATCH 2/5] t5801: don't use test_when_finished in a subshell","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-09-05T13:12:46Z","receivedAt":"2015-09-05T13:12:46Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"test_when_finished has no effect in a subshell.  Since the cmp_marks\nfunction is only used once, inline it at its call site and move the\ntest_when_finished invocation to the start of the test.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n t/t5801-remote-helpers.sh | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh\nindex c9d3ed1..362b158 100755\n--- a/t/t5801-remote-helpers.sh\n+++ b/t/t5801-remote-helpers.sh\n@@ -242,13 +242,6 @@ clean_mark () {\n \tsort >$(basename \"$1\")\n }\n \n-cmp_marks () {\n-\ttest_when_finished \"rm -rf git.marks testgit.marks\" &&\n-\tclean_mark \".git/testgit/$1/git.marks\" &&\n-\tclean_mark \".git/testgit/$1/testgit.marks\" &&\n-\ttest_cmp git.marks testgit.marks\n-}\n-\n test_expect_success 'proper failure checks for fetching' '\n \t(cd local &&\n \ttest_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git fetch 2>error &&\n@@ -258,12 +251,15 @@ test_expect_success 'proper failure checks for fetching' '\n '\n \n test_expect_success 'proper failure checks for pushing' '\n+\ttest_when_finished \"rm -rf local/git.marks local/testgit.marks\" &&\n \t(cd local &&\n \tgit checkout -b crash master &&\n \techo crash >>file &&\n \tgit commit -a -m crash &&\n \ttest_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git push --all &&\n-\tcmp_marks origin\n+\tclean_mark \".git/testgit/origin/git.marks\" &&\n+\tclean_mark \".git/testgit/origin/testgit.marks\" &&\n+\ttest_cmp git.marks testgit.marks\n \t)\n '\n \n-- \n2.5.0.466.g9af26fa\n"},{"id":"269482","messageId":"1f82fa1a0f185ab4944ecbf6dd65adb21e77e04f.1441458341.git.john@keeping.me.uk","threadId":"40274","inReplyTo":"cover.1441458341.git.john@keeping.me.uk","subject":"[PATCH 3/5] test-lib-functions: support \"test_config -C <dir> ...\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-09-05T13:12:47Z","receivedAt":"2015-09-05T13:12:47Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"If used in a subshell, test_config cannot unset variables at the end of\na test.  This is a problem when testing submodules because we do not\nwant to \"cd\" at to top level of a test script in order to run the\ncommand inside the submodule.\n\nAdd a \"-C\" option to test_config (and test_unconfig) so that test_config\ncan be kept outside subshells and still affect subrepositories.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n t/test-lib-functions.sh | 20 +++++++++++++++++---\n 1 file changed, 17 insertions(+), 3 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex e8d3c0f..0e80f37 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -201,7 +201,14 @@ test_chmod () {\n \n # Unset a configuration variable, but don't fail if it doesn't exist.\n test_unconfig () {\n-\tgit config --unset-all \"$@\"\n+\tconfig_dir=\n+\tif test \"$1\" = -C\n+\tthen\n+\t\tshift\n+\t\tconfig_dir=$1\n+\t\tshift\n+\tfi\n+\tgit ${config_dir:+-C \"$config_dir\"} config --unset-all \"$@\"\n \tconfig_status=$?\n \tcase \"$config_status\" in\n \t5) # ok, nothing to unset\n@@ -213,8 +220,15 @@ test_unconfig () {\n \n # Set git config, automatically unsetting it after the test is over.\n test_config () {\n-\ttest_when_finished \"test_unconfig '$1'\" &&\n-\tgit config \"$@\"\n+\tconfig_dir=\n+\tif test \"$1\" = -C\n+\tthen\n+\t\tshift\n+\t\tconfig_dir=$1\n+\t\tshift\n+\tfi\n+\ttest_when_finished \"test_unconfig ${config_dir:+-C '$config_dir'} '$1'\" &&\n+\tgit ${config_dir:+-C \"$config_dir\"} config \"$@\"\n }\n \n test_config_global () {\n-- \n2.5.0.466.g9af26fa\n"},{"id":"269483","messageId":"5aa306f56f544de4f71be98f66b912b72336835f.1441458341.git.john@keeping.me.uk","threadId":"40274","inReplyTo":"cover.1441458341.git.john@keeping.me.uk","subject":"[PATCH 4/5] t7800: don't use test_config in a subshell","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-09-05T13:12:48Z","receivedAt":"2015-09-05T13:12:48Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"Use the new \"-C\" option to test_config to change the configuration in\nthe submodule from the top level of the test so that it can be unset\ncorrectly when the test finishes.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n t/t7800-difftool.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex ea35a02..48c6e2b 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -492,12 +492,12 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' '\n \n test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '\n \tgit submodule add ./. submod/ule &&\n+\ttest_config -C submod/ule diff.tool checktrees &&\n+\ttest_config -C submod/ule difftool.checktrees.cmd '\\''\n+\t\ttest -d \"$LOCAL\" && test -d \"$REMOTE\" && echo good\n+\t\t'\\'' &&\n \t(\n \t\tcd submod/ule &&\n-\t\ttest_config diff.tool checktrees &&\n-\t\ttest_config difftool.checktrees.cmd '\\''\n-\t\t\ttest -d \"$LOCAL\" && test -d \"$REMOTE\" && echo good\n-\t\t'\\'' &&\n \t\techo good >expect &&\n \t\tgit difftool --tool=checktrees --dir-diff HEAD~ >actual &&\n \t\ttest_cmp expect actual\n-- \n2.5.0.466.g9af26fa\n"},{"id":"269484","messageId":"59a8583f79c5ac4661f140262e01cd602286f740.1441458341.git.john@keeping.me.uk","threadId":"40274","inReplyTo":"cover.1441458341.git.john@keeping.me.uk","subject":"[PATCH 5/5] test-lib-functions: detect test_when_finished in subshell","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-09-05T13:12:49Z","receivedAt":"2015-09-05T13:12:49Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"test_when_finished does nothing in a subshell because the change to\ntest_cleanup does not affect the parent.\n\nThere is no POSIX way to detect that we are in a subshell ($$ and $PPID\nare specified to remain unchanged), but we can detect it on Bash and\nfall back to ignoring the bug on other shells.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n t/test-lib-functions.sh | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 0e80f37..6dffb8b 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -736,6 +736,11 @@ test_seq () {\n # what went wrong.\n \n test_when_finished () {\n+\t# We cannot detect when we are in a subshell in general, but by\n+\t# doing so on Bash is better than nothing (the test will\n+\t# silently pass on other shells).\n+\ttest \"${BASH_SUBSHELL-0}\" = 0 ||\n+\terror \"bug in test script: test_when_finished does nothing in a subshell\"\n \ttest_cleanup=\"{ $*\n \t\t} && (exit \\\"\\$eval_ret\\\"); eval_ret=\\$?; $test_cleanup\"\n }\n-- \n2.5.0.466.g9af26fa\n"},{"id":"269500","messageId":"CAPc5daUsd8KD77EfF-SBwwOKn0hNPvYXo8UmY-sHBg9S4vUQXg@mail.gmail.com","threadId":"40274","inReplyTo":"cover.1441458341.git.john@keeping.me.uk","subject":"Re: [PATCH 0/5] disallow test_when_finished in subshells","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-05T17:36:29Z","receivedAt":"2015-09-05T17:36:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Sat, Sep 5, 2015 at 6:12 AM, John Keeping <john@keeping.me.uk> wrote:\n>\n> I don't think it's worth trying to clear $BASH_SUBSHELL before the tests\n> start because to do so we have to reliably detect that we're not running\n> under Bash, and if we don't trust people not to set $BASH_SUBSHELL why\n> do we trust them not to set $BASH?\n\nI am not worried about evil people who do funny things to deliberately break\nother people's arrangement. I am more worried about stupid people (e.g. those\nwho export CDPATH).\n\nIn bash a stupid person may attempt to export BASH_SUBSHELL and then\nhave a script that runs our test suite, setting SHELL_PATH to point at a\nnon-bash while building Git and running the tests under a non-bash shell. I\nam hesitant to believe that we will know the variable will never leak through\nto the test via environment.\n\nIsn't it just the matter of resetting the variable regardless of $BASH\n(and ignoring\na possible refusal to do so under bash) at the beginning of the test, or do you\nreally have to rely on the value of $BASH and do things differently?\n"},{"id":"269502","messageId":"20150905175756.GE27660@serenity.lan","threadId":"40274","inReplyTo":"CAPc5daUsd8KD77EfF-SBwwOKn0hNPvYXo8UmY-sHBg9S4vUQXg@mail.gmail.com","subject":"Re: [PATCH 0/5] disallow test_when_finished in subshells","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-09-05T17:57:56Z","receivedAt":"2015-09-05T17:57:56Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sat, Sep 05, 2015 at 10:36:29AM -0700, Junio C Hamano wrote:\n> On Sat, Sep 5, 2015 at 6:12 AM, John Keeping <john@keeping.me.uk> wrote:\n> >\n> > I don't think it's worth trying to clear $BASH_SUBSHELL before the tests\n> > start because to do so we have to reliably detect that we're not running\n> > under Bash, and if we don't trust people not to set $BASH_SUBSHELL why\n> > do we trust them not to set $BASH?\n> \n> I am not worried about evil people who do funny things to deliberately break\n> other people's arrangement. I am more worried about stupid people (e.g. those\n> who export CDPATH).\n> \n> In bash a stupid person may attempt to export BASH_SUBSHELL and then\n> have a script that runs our test suite, setting SHELL_PATH to point at a\n> non-bash while building Git and running the tests under a non-bash shell. I\n> am hesitant to believe that we will know the variable will never leak through\n> to the test via environment.\n> \n> Isn't it just the matter of resetting the variable regardless of $BASH\n> (and ignoring\n> a possible refusal to do so under bash) at the beginning of the test, or do you\n> really have to rely on the value of $BASH and do things differently?\n\nBash doesn't refuse to set it, it lets you update the value; I did think\nthat it wouldn't update it if the user had overridden the value, but it\nlooks like that was only because I had unset it first.  It seems that\nthe variable is magic (autoincrementing in subshells and can only be set\nto integer values) but if you unset it then it becomes a normal\nvariable.\n\nI'm wary of relying on that behaviour being unchanged across all\nversions of bash, so I'd prefer to avoid writing the variable if running\nunder bash.\n\nWe do already have t/lib-bash.sh which has an optimization to avoid\nexec'ing bash if \"$BASH\" is set, which will break in the same way if\nsomeone exports BASH and then runs t9902 or t9903 under non-bash.\n"},{"id":"269519","messageId":"CAPc5daX5w5yoRyp=Squdhi8nj87bb4zfqea-=wLyze6GhLPV+w@mail.gmail.com","threadId":"40274","inReplyTo":"20150905175756.GE27660@serenity.lan","subject":"Re: [PATCH 0/5] disallow test_when_finished in subshells","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-05T20:17:35Z","receivedAt":"2015-09-05T20:17:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":">> Isn't it just the matter of resetting the variable regardless of $BASH\n>> (and ignoring\n>> a possible refusal to do so under bash) at the beginning of the test, or do you\n>> really have to rely on the value of $BASH and do things differently?\n>\n> Bash doesn't refuse to set it, it lets you update the value; I did think\n> that it wouldn't update it if the user had overridden the value, but it\n> looks like that was only because I had unset it first.  It seems that\n> the variable is magic (autoincrementing in subshells and can only be set\n> to integer values) but if you unset it then it becomes a normal\n> variable.\n\nYes, resetting to =0 at the very beginning is what I have in mind.\n"},{"id":"269529","messageId":"CAPig+cR+_nPFbJmOf3s90oB=Jedg1B3YgOCzSwWa8nwP8-QbeA@mail.gmail.com","threadId":"40274","inReplyTo":"59a8583f79c5ac4661f140262e01cd602286f740.1441458341.git.john@keeping.me.uk","subject":"Re: [PATCH 5/5] test-lib-functions: detect test_when_finished in subshell","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-06T09:51:43Z","receivedAt":"2015-09-06T09:51:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Sep 5, 2015 at 9:12 AM, John Keeping <john@keeping.me.uk> wrote:\n> test_when_finished does nothing in a subshell because the change to\n> test_cleanup does not affect the parent.\n>\n> There is no POSIX way to detect that we are in a subshell ($$ and $PPID\n> are specified to remain unchanged), but we can detect it on Bash and\n> fall back to ignoring the bug on other shells.\n\nI'm not necessarily advocating this, but think it's worth mentioning\nthat an alternate solution would be to fix test_when_finished() to work\ncorrectly in subshells rather than disallowing its use. This can be done\nby having test_when_finished() collect the cleanup actions in a file\nrather than in a shell variable.\n\nPros:\n* works in subshells\n* portable across all shells (no Bash special-case)\n* one less rule (restriction) for test writers to remember\n\nCons:\n* slower\n* could interfere with tests expecting very specific 'trash' directory\n  contents (but locating this file under .git might make it safe)\n\n> Signed-off-by: John Keeping <john@keeping.me.uk>\n> ---\n>  t/test-lib-functions.sh | 5 +++++\n>  1 file changed, 5 insertions(+)\n>\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 0e80f37..6dffb8b 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -736,6 +736,11 @@ test_seq () {\n>  # what went wrong.\n>\n>  test_when_finished () {\n> +       # We cannot detect when we are in a subshell in general, but by\n> +       # doing so on Bash is better than nothing (the test will\n> +       # silently pass on other shells).\n> +       test \"${BASH_SUBSHELL-0}\" = 0 ||\n> +       error \"bug in test script: test_when_finished does nothing in a subshell\"\n>         test_cleanup=\"{ $*\n>                 } && (exit \\\"\\$eval_ret\\\"); eval_ret=\\$?; $test_cleanup\"\n>  }\n> --\n> 2.5.0.466.g9af26fa\n"},{"id":"269531","messageId":"20150906114635.GF27660@serenity.lan","threadId":"40274","inReplyTo":"CAPig+cR+_nPFbJmOf3s90oB=Jedg1B3YgOCzSwWa8nwP8-QbeA@mail.gmail.com","subject":"Re: [PATCH 5/5] test-lib-functions: detect test_when_finished in subshell","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2015-09-06T11:46:35Z","receivedAt":"2015-09-06T11:46:35Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Sun, Sep 06, 2015 at 05:51:43AM -0400, Eric Sunshine wrote:\n> On Sat, Sep 5, 2015 at 9:12 AM, John Keeping <john@keeping.me.uk> wrote:\n> > test_when_finished does nothing in a subshell because the change to\n> > test_cleanup does not affect the parent.\n> >\n> > There is no POSIX way to detect that we are in a subshell ($$ and $PPID\n> > are specified to remain unchanged), but we can detect it on Bash and\n> > fall back to ignoring the bug on other shells.\n> \n> I'm not necessarily advocating this, but think it's worth mentioning\n> that an alternate solution would be to fix test_when_finished() to work\n> correctly in subshells rather than disallowing its use. This can be done\n> by having test_when_finished() collect the cleanup actions in a file\n> rather than in a shell variable.\n> \n> Pros:\n> * works in subshells\n> * portable across all shells (no Bash special-case)\n> * one less rule (restriction) for test writers to remember\n> \n> Cons:\n> * slower\n> * could interfere with tests expecting very specific 'trash' directory\n>   contents (but locating this file under .git might make it safe)\n\nAnother con is that we have to worry about the working directory, and\nsince we can't reliably detect if we're in a subshell every cleanup\naction probably has to be something like:\n\n\t( cd '$(pwd)' && $* )\n\nIt's certainly possible but it adds another bit of complexity.\n\nSince there are only 3 out of over 13,000 tests that use this\nfunctionality (and it's quite easy to change them not to) I'm not sure\nit's worth supporting it.\n\n> > Signed-off-by: John Keeping <john@keeping.me.uk>\n> > ---\n> >  t/test-lib-functions.sh | 5 +++++\n> >  1 file changed, 5 insertions(+)\n> >\n> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> > index 0e80f37..6dffb8b 100644\n> > --- a/t/test-lib-functions.sh\n> > +++ b/t/test-lib-functions.sh\n> > @@ -736,6 +736,11 @@ test_seq () {\n> >  # what went wrong.\n> >\n> >  test_when_finished () {\n> > +       # We cannot detect when we are in a subshell in general, but by\n> > +       # doing so on Bash is better than nothing (the test will\n> > +       # silently pass on other shells).\n> > +       test \"${BASH_SUBSHELL-0}\" = 0 ||\n> > +       error \"bug in test script: test_when_finished does nothing in a subshell\"\n> >         test_cleanup=\"{ $*\n> >                 } && (exit \\\"\\$eval_ret\\\"); eval_ret=\\$?; $test_cleanup\"\n> >  }\n> > --\n> > 2.5.0.466.g9af26fa\n"},{"id":"269536","messageId":"CAPig+cQixsVT-5Ke2Ccedijc1HeBZ6=mbKeeN95V3zExBxSz2A@mail.gmail.com","threadId":"40274","inReplyTo":"20150906114635.GF27660@serenity.lan","subject":"Re: [PATCH 5/5] test-lib-functions: detect test_when_finished in subshell","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-09-06T16:22:19Z","receivedAt":"2015-09-06T16:22:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 6, 2015 at 7:46 AM, John Keeping <john@keeping.me.uk> wrote:\n> On Sun, Sep 06, 2015 at 05:51:43AM -0400, Eric Sunshine wrote:\n>> I'm not necessarily advocating this, but think it's worth mentioning\n>> that an alternate solution would be to fix test_when_finished() to work\n>> correctly in subshells rather than disallowing its use. This can be done\n>> by having test_when_finished() collect the cleanup actions in a file\n>> rather than in a shell variable.\n>>\n>> Pros:\n>> * works in subshells\n>> * portable across all shells (no Bash special-case)\n>> * one less rule (restriction) for test writers to remember\n>>\n>> Cons:\n>> * slower\n>> * could interfere with tests expecting very specific 'trash' directory\n>>   contents (but locating this file under .git might make it safe)\n>\n> Another con is that we have to worry about the working directory, and\n> since we can't reliably detect if we're in a subshell every cleanup\n> action probably has to be something like:\n>\n>         ( cd '$(pwd)' && $* )\n\nThat's an argument against allowing test_when_finished() inside\nsubshells, in general, isn't it? Subshell callers of\ntest_when_finished(), if correctly written, would already have had to\nprotect against that anyhow, so it's not a \"con\" of the idea of\ncollecting cleanup actions in a file.\n\nOf course, patches 2/5 and 4/5 demonstrate that a couple of those\nsubshell callers did *not* correctly protect against directory (or\nother) changes which would invalidate their cleanup actions, and were\nthus buggy anyhow, even if the cleanup actions had been invoked.\nThat's a good argument in favor of disallowing test_when_finished() in\nsubshells, but not a \"con\" of the suggestion.\n\nI'm probably arguing semantics here, thus being annoying, so I'll stop now...\n\n> It's certainly possible but it adds another bit of complexity.\n>\n> Since there are only 3 out of over 13,000 tests that use this\n> functionality (and it's quite easy to change them not to) I'm not sure\n> it's worth supporting it.\n\nNo argument there.\n"}]}