[RFC] test_when_finished in subshells

[RFC] test_when_finished in subshells

[John Keeping]

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 --
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e8d3c0f..d29cd7b 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -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

Re: [RFC] test_when_finished in subshells

[Junio C Hamano]

John Keeping <john@keeping.me.uk> writes:

> 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?

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.

>
> [0] http://article.gmane.org/gmane.comp.version-control.git/277199
>
> -- >8 --
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index e8d3c0f..d29cd7b 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -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"
>  }

Re: [RFC] test_when_finished in subshells

[Jeff King]

On Fri, Sep 04, 2015 at 11:43:15AM -0700, Junio C Hamano wrote:

> > 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?
> 
> 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

[PATCH 0/5] disallow test_when_finished in subshells

[John Keeping]

On Sat, Sep 05, 2015 at 04:54:30AM -0400, Jeff King wrote:
> On Fri, Sep 04, 2015 at 11:43:15AM -0700, Junio C Hamano wrote:
> 
> > > 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?
> > 
> > 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.

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

[PATCH 1/5] t7610: don't use test_config in a subshell

[John Keeping]

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.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 t/t7610-mergetool.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 7eeb207..6f12b23 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -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

[PATCH 2/5] t5801: don't use test_when_finished in a subshell

[John Keeping]

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.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 t/t5801-remote-helpers.sh | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index c9d3ed1..362b158 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -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

[PATCH 3/5] test-lib-functions: support "test_config -C <dir> ..."

[John Keeping]

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.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 t/test-lib-functions.sh | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index e8d3c0f..0e80f37 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -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

[PATCH 4/5] t7800: don't use test_config in a subshell

[John Keeping]

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.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 t/t7800-difftool.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index ea35a02..48c6e2b 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -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

[PATCH 5/5] test-lib-functions: detect test_when_finished in subshell

[John Keeping]

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.

Signed-off-by: John Keeping <john@keeping.me.uk>
---
 t/test-lib-functions.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0e80f37..6dffb8b 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -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

Re: [PATCH 0/5] disallow test_when_finished in subshells

[Junio C Hamano]

On Sat, Sep 5, 2015 at 6:12 AM, John Keeping <john@keeping.me.uk> wrote:
>
> 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?

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?

Re: [PATCH 0/5] disallow test_when_finished in subshells

[John Keeping]

On Sat, Sep 05, 2015 at 10:36:29AM -0700, Junio C Hamano wrote:
> On Sat, Sep 5, 2015 at 6:12 AM, John Keeping <john@keeping.me.uk> wrote:
> >
> > 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?
> 
> 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.

Re: [PATCH 0/5] disallow test_when_finished in subshells

[Junio C Hamano]

>> 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.

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

Re: [PATCH 5/5] test-lib-functions: detect test_when_finished in subshell

[Eric Sunshine]

On Sat, Sep 5, 2015 at 9:12 AM, John Keeping <john@keeping.me.uk> wrote:
> 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.

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)

> Signed-off-by: John Keeping <john@keeping.me.uk>
> ---
>  t/test-lib-functions.sh | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 0e80f37..6dffb8b 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -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

Re: [PATCH 5/5] test-lib-functions: detect test_when_finished in subshell

[John Keeping]

On Sun, Sep 06, 2015 at 05:51:43AM -0400, Eric Sunshine wrote:
> On Sat, Sep 5, 2015 at 9:12 AM, John Keeping <john@keeping.me.uk> wrote:
> > 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.
> 
> 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.

> > Signed-off-by: John Keeping <john@keeping.me.uk>
> > ---
> >  t/test-lib-functions.sh | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > index 0e80f37..6dffb8b 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -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

Re: [PATCH 5/5] test-lib-functions: detect test_when_finished in subshell

[Eric Sunshine]

On Sun, Sep 6, 2015 at 7:46 AM, John Keeping <john@keeping.me.uk> wrote:
> On Sun, Sep 06, 2015 at 05:51:43AM -0400, Eric Sunshine wrote:
>> 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)' && $* )

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...

> 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.

No argument there.