disallow test_when_finished in subshells
Get series with b4
b4 am [email protected]Versions newest first
- v1 Latest5 patches 6 repliesIn review
Patches in v1 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/5 | t7610: don't use test_config in a subshellt/ | +1 | 0 replies | |
| 2/5 | t5801: don't use test_when_finished in a subshellt/ | +4 | 0 replies | |
| 3/5 | test-lib-functions: support "test_config -C <dir> ..."t/ | +17 | 0 replies | |
| 4/5 | t7800: don't use test_config in a subshellt/ | +4 | 0 replies | |
| 5/5 | test-lib-functions: detect test_when_finished in subshellt/ | +5 | 3 replies |
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.
Cover letter [PATCH 0/5] disallow test_when_finished in subshells
Quoted 23 lines · Jeff King
Quoted 9 lines · Junio C Hamano
Quoted 5 lines
t7800 (in its final test) calls
test_configin 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.
Quoted 8 lines
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_SUBSHELLis 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:
Quoted 8 lines
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(-)
Signature · 1 line
2.5.0.466.g9af26fa