[RFC] 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 --

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

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.

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

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

On Sat, Sep 05, 2015 at 04:54:30AM -0400, Jeff King wrote:

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(-)

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(-)

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(-)

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(-)

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(-)

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(+)

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

On Sun, Sep 06, 2015 at 05:51:43AM -0400, Eric Sunshine wrote:

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.

On Sun, Sep 6, 2015 at 7:46 AM, John Keeping <john@keeping.me.uk> wrote:

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.

On Sat, Sep 5, 2015 at 6:12 AM, John Keeping <john@keeping.me.uk> wrote:

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?

On Sat, Sep 05, 2015 at 10:36:29AM -0700, Junio C Hamano wrote:

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.