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