On Tue, Jun 26, 2018 at 03:52:54PM -0400, Eric Sunshine wrote: > The existing --chain-lint already suffers the same shortcoming. Older > (or even new poorly-written) tests, even without subshells, can fall > victim already: > > (exit $sentinel) && > mkdir -p a/b/c && > cd a/b/c > rm -fr ../../* && > cd ../../.. && > statement4 > > As in your example, 'mkdir' and 'cd' are skipped, but 'rm -fr ../../*' is not. > > This snippet from the commit message of bb79af9d09 (t/test-lib: > introduce --chain-lint option, 2015-03-20): > > When we encounter a failure of this check, we abort the test > script entirely. For one thing, we have no clue which subset > of the commands in the test snippet were actually run. > > suggests that the issue was considered, in some form, even then > (though, it doesn't say explicitly that Peff had the 'rm -fr' case in > mind). > > So, this isn't a new problem introduced by this series, though this > series may exacerbate it. One way this series might be worse in practice is that we tend not to change process state too much outside of the subshells. So if we skip some early commands and execute a later "rm", for example, it tends to be in the same directory (and I think as time goes on we have been cleaning up old tests which did a "cd foo && bar && cd .." into using a subshell). Whereas once you start collapsing subshells into the main logic chain, there's a very high chance that the subshell is doing a "cd", since that's typically the main reason for the subshell in the first place. And with the current --chain-lint logic, that subshell is either executed or not executed as a unit. Obviously that's a bit of a hand-waving argument. If you've fixed all of the existing cases without accidentally deleting your home directory, then maybe it's not so likely to be a problem after all. I'm not sure if there's a good solution, though. Even if you retained the subshells and instead did a chain-lint inside each subshell, like this: (exit 117) && one && ( (exit 117) && cd foo two ) && three that doesn't really help. The fundamental issue is that we may skip the "cd" inside the subshell. Whether it's in a subshell or not, that's dangerous. True, we don't run "three" in this case, which is slightly better. But it didn't expect to be in a different directory anyway. It's running "two" that is dangerous. -Peff
