Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells

[Jeff King <peff@peff.net>]

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