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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html