[PATCH 0/5] disallow test_when_finished in subshells

From: John Keeping <john@keeping.me.uk>
Date: Sep 5, 2015, 13:12 UTC
Message-ID: <cover.1441458341.git.john@keeping.me.uk>
In-Reply-To: <20150905085429.GB25039@sigill.intra.peff.net>

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

Show 31 quoted lines

> 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