test_when_finished in subshells

A recent thread [0] made me realise that using test_when_finished in a subshell is likely to be a bug, since the change to $test_cleanup will be lost when the subshell exits.

There is no POSIX way to detect that we are in a subshell ($$ and $PPID are specified to remain unchanged), but we can detect it on Bash and gracefully fall back to disabling the test on other shells, which is what the patch below does.

There are three tests currently in master that fail with this patch (at least on my system):

Test Summary Report
-------------------
t7610-mergetool.sh (Wstat: 256 Tests: 18 Failed: 1)
Failed test: 7
Non-zero exit status: 1
t7800-difftool.sh (Wstat: 256 Tests: 56 Failed: 1)
Failed test: 56
Non-zero exit status: 1
t5801-remote-helpers.sh (Wstat: 256 Tests: 30 Failed: 1)
Failed test: 27
Non-zero exit status: 1

All are harmless at the moment and t7610 and t5801 can be fixed by moving the test_when_finished call out of the subshell relatively easily.

t7800 (in its final test) calls test_config in a subshell which has cd'd into a submodule.

Is this something worth worrying about, or is it sufficiently rare that we can live with the current behaviour?

[0] http://article.gmane.org/gmane.comp.version-control.git/277199

-- >8 --

index e8d3c0f..d29cd7b 100644
@@ -722,6 +722,8 @@ test_seq () {
 # what went wrong.
 
 test_when_finished () {
+	test "${BASH_SUBSHELL-0}" = 0 ||
+	error "bug in test script: test_when_finished does nothing in a subshell"
 	test_cleanup="{ $*
 		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup"
 }

2.5.0.466.g9af26fa

Fixing the instances you found is good, obviously ;-). Thanks for working on this.

Even though the proposed detection is BASH-ism, I think it would not hurt other shells (they obviously do not help you catch bugs, but they would not misbehave as long as you make sure BASH_SUBSHELL is either unset or set to 0 at the beginning of the test), and the only impact to them would be a invocation of (often built-in) 'test' utility, whose performance impact should be miniscule.

I'll wait for opinion from others, of course.

I like it. In general I'm in favor of any lint-like fixes (whether for the tests or the C code itself) as long as:

1. they don't create false positive noise

2. they don't require extra effort at each call-site

3. they don't have a performance impact

And I think this passes all three. Of course it would be nice if the new check ran on all shells, but even this seems like a strict improvement.

And I couldn't come up with anything better. I thought we might be able to use a canary trap, like this:

trap canary QUIT
echo outside
trap
echo inside
(trap)

Because traps are reset inside a subshell, the first "trap" without arguments should print a string with "canary" in it, and the second should print nothing.

Except that it isn't remotely how it works in practice. :) Bash leaves the trap in place inside the subshell. And while dash does the right thing, any attempt to actually _look_ at the output will put us in a subshell. So:

case "$(trap)" in
*canary*) ... not in a subshell ...
*) ... in a subshell ...
esac

doesn't work; the trap report inside backticks is always empty (despite there being an explicit example in the POSIX manpage for trap of using $(trap) to get the value for a later eval).

It looks like if we use the EXIT trap rather than a signal, bash _does_ do the right thing (which kind of makes sense, I guess). So rather than a custom canary, we could use our regular EXIT handler as the canary. But I couldn't find a way to work around dash's issue.

Looks like there was even some discussion in 2010:

http://www.spinics.net/lists/dash/msg00331.html

but my dash 0.5.7 remains broken. Oh, well.

-Peff

Here are the changes to do this. The first two are pretty straightforward, but the t7800 change may be more controversial; in this particular case we could get away with using "git config" instead of test_config but I think the more generic solution will be better for the future.

I don't think it's worth trying to clear $BASH_SUBSHELL before the tests start because to do so we have to reliably detect that we're not running under Bash, and if we don't trust people not to set $BASH_SUBSHELL why do we trust them not to set $BASH?

John Keeping (5):
t7610: don't use test_config in a subshell
t5801: don't use test_when_finished in a subshell
test-lib-functions: support "test_config -C <dir> ..."
t7800: don't use test_config in a subshell
test-lib-functions: detect test_when_finished in subshell

 t/t5801-remote-helpers.sh | 12 ++++--------
 t/t7610-mergetool.sh      |  2 +-
 t/t7800-difftool.sh       |  8 ++++----
 t/test-lib-functions.sh   | 25 ++++++++++++++++++++++---
 4 files changed, 31 insertions(+), 16 deletions(-)

2.5.0.466.g9af26fa

test_config uses test_when_finished to reset the configuration after the test, but this does not work inside a subshell. This does not cause a problem here because the first thing the next test does is to set this config variable itself, but we are about to add a check that will complain when test_when_finished is used in a subshell.

In this case, "subdir" not a submodule so test_config has the same effect when run at the top level and can simply be moved out of the subshell.

---
 t/t7610-mergetool.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 7eeb207..6f12b23 100755
@@ -174,9 +174,9 @@ test_expect_success 'mergetool skips autoresolved' '
 '
 
 test_expect_success 'mergetool merges all from subdir' '
+	test_config rerere.enabled false &&
 	(
 		cd subdir &&
-		test_config rerere.enabled false &&
 		test_must_fail git merge master &&
 		( yes "r" | git mergetool ../submod ) &&
 		( yes "d" "d" | git mergetool --no-prompt ) &&

2.5.0.466.g9af26fa

test_when_finished has no effect in a subshell. Since the cmp_marks function is only used once, inline it at its call site and move the test_when_finished invocation to the start of the test.

---
 t/t5801-remote-helpers.sh | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

index c9d3ed1..362b158 100755
@@ -242,13 +242,6 @@ clean_mark () {
 	sort >$(basename "$1")
 }
 
-cmp_marks () {
-	test_when_finished "rm -rf git.marks testgit.marks" &&
-	clean_mark ".git/testgit/$1/git.marks" &&
-	clean_mark ".git/testgit/$1/testgit.marks" &&
-	test_cmp git.marks testgit.marks
-}
-
 test_expect_success 'proper failure checks for fetching' '
 	(cd local &&
 	test_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git fetch 2>error &&

@@ -258,12 +251,15 @@ test_expect_success 'proper failure checks for fetching' '
 '
 
 test_expect_success 'proper failure checks for pushing' '
+	test_when_finished "rm -rf local/git.marks local/testgit.marks" &&
 	(cd local &&
 	git checkout -b crash master &&
 	echo crash >>file &&
 	git commit -a -m crash &&
 	test_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git push --all &&
-	cmp_marks origin
+	clean_mark ".git/testgit/origin/git.marks" &&
+	clean_mark ".git/testgit/origin/testgit.marks" &&
+	test_cmp git.marks testgit.marks
 	)
 '
 

2.5.0.466.g9af26fa

If used in a subshell, test_config cannot unset variables at the end of a test. This is a problem when testing submodules because we do not want to "cd" at to top level of a test script in order to run the command inside the submodule.

Add a "-C" option to test_config (and test_unconfig) so that test_config can be kept outside subshells and still affect subrepositories.

---
 t/test-lib-functions.sh | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

index e8d3c0f..0e80f37 100644
@@ -201,7 +201,14 @@ test_chmod () {
 
 # Unset a configuration variable, but don't fail if it doesn't exist.
 test_unconfig () {
-	git config --unset-all "$@"
+	config_dir=
+	if test "$1" = -C
+	then
+		shift
+		config_dir=$1
+		shift
+	fi
+	git ${config_dir:+-C "$config_dir"} config --unset-all "$@"
 	config_status=$?
 	case "$config_status" in
 	5) # ok, nothing to unset

@@ -213,8 +220,15 @@ test_unconfig () {
 
 # Set git config, automatically unsetting it after the test is over.
 test_config () {
-	test_when_finished "test_unconfig '$1'" &&
-	git config "$@"
+	config_dir=
+	if test "$1" = -C
+	then
+		shift
+		config_dir=$1
+		shift
+	fi
+	test_when_finished "test_unconfig ${config_dir:+-C '$config_dir'} '$1'" &&
+	git ${config_dir:+-C "$config_dir"} config "$@"
 }
 
 test_config_global () {

2.5.0.466.g9af26fa

Use the new "-C" option to test_config to change the configuration in the submodule from the top level of the test so that it can be unset correctly when the test finishes.

---
 t/t7800-difftool.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

index ea35a02..48c6e2b 100755
@@ -492,12 +492,12 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' '
 
 test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '
 	git submodule add ./. submod/ule &&
+	test_config -C submod/ule diff.tool checktrees &&
+	test_config -C submod/ule difftool.checktrees.cmd '\''
+		test -d "$LOCAL" && test -d "$REMOTE" && echo good
+		'\'' &&
 	(
 		cd submod/ule &&
-		test_config diff.tool checktrees &&
-		test_config difftool.checktrees.cmd '\''
-			test -d "$LOCAL" && test -d "$REMOTE" && echo good
-		'\'' &&
 		echo good >expect &&
 		git difftool --tool=checktrees --dir-diff HEAD~ >actual &&
 		test_cmp expect actual

2.5.0.466.g9af26fa

I am not worried about evil people who do funny things to deliberately break other people's arrangement. I am more worried about stupid people (e.g. those who export CDPATH).

In bash a stupid person may attempt to export BASH_SUBSHELL and then have a script that runs our test suite, setting SHELL_PATH to point at a non-bash while building Git and running the tests under a non-bash shell. I am hesitant to believe that we will know the variable will never leak through to the test via environment.

Isn't it just the matter of resetting the variable regardless of $BASH (and ignoring
a possible refusal to do so under bash) at the beginning of the test, or do you really have to rely on the value of $BASH and do things differently?

Bash doesn't refuse to set it, it lets you update the value; I did think that it wouldn't update it if the user had overridden the value, but it looks like that was only because I had unset it first. It seems that the variable is magic (autoincrementing in subshells and can only be set to integer values) but if you unset it then it becomes a normal variable.

I'm wary of relying on that behaviour being unchanged across all versions of bash, so I'd prefer to avoid writing the variable if running under bash.

We do already have t/lib-bash.sh which has an optimization to avoid exec'ing bash if "$BASH" is set, which will break in the same way if someone exports BASH and then runs t9902 or t9903 under non-bash.

Yes, resetting to =0 at the very beginning is what I have in mind.

test_when_finished does nothing in a subshell because the change to test_cleanup does not affect the parent.

There is no POSIX way to detect that we are in a subshell ($$ and $PPID are specified to remain unchanged), but we can detect it on Bash and fall back to ignoring the bug on other shells.

---
 t/test-lib-functions.sh | 5 +++++
 1 file changed, 5 insertions(+)

index 0e80f37..6dffb8b 100644
@@ -736,6 +736,11 @@ test_seq () {
 # what went wrong.
 
 test_when_finished () {
+	# We cannot detect when we are in a subshell in general, but by
+	# doing so on Bash is better than nothing (the test will
+	# silently pass on other shells).
+	test "${BASH_SUBSHELL-0}" = 0 ||
+	error "bug in test script: test_when_finished does nothing in a subshell"
 	test_cleanup="{ $*
 		} && (exit \"\$eval_ret\"); eval_ret=\$?; $test_cleanup"
 }

2.5.0.466.g9af26fa

I'm not necessarily advocating this, but think it's worth mentioning that an alternate solution would be to fix test_when_finished() to work correctly in subshells rather than disallowing its use. This can be done by having test_when_finished() collect the cleanup actions in a file rather than in a shell variable.

Pros:
* works in subshells
* portable across all shells (no Bash special-case)
* one less rule (restriction) for test writers to remember

Cons:
* slower
* could interfere with tests expecting very specific 'trash' directory contents (but locating this file under .git might make it safe)

Another con is that we have to worry about the working directory, and since we can't reliably detect if we're in a subshell every cleanup action probably has to be something like:

	( cd '$(pwd)' && $* )

It's certainly possible but it adds another bit of complexity.

Since there are only 3 out of over 13,000 tests that use this functionality (and it's quite easy to change them not to) I'm not sure it's worth supporting it.

That's an argument against allowing test_when_finished() inside subshells, in general, isn't it? Subshell callers of test_when_finished(), if correctly written, would already have had to protect against that anyhow, so it's not a "con" of the idea of collecting cleanup actions in a file.

Of course, patches 2/5 and 4/5 demonstrate that a couple of those subshell callers did *not* correctly protect against directory (or other) changes which would invalidate their cleanup actions, and were thus buggy anyhow, even if the cleanup actions had been invoked. That's a good argument in favor of disallowing test_when_finished() in subshells, but not a "con" of the suggestion.

I'm probably arguing semantics here, thus being annoying, so I'll stop now...

No argument there.