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

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Jun 26, 2018 at 4:17 PM Jeff King <peff@peff.net> wrote:
> On Tue, Jun 26, 2018 at 03:52:54PM -0400, Eric Sunshine wrote:
> > So, this isn't a new problem introduced by this series, though this
> > series may exacerbate it.
>
> 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.

Indeed, it could be that the "rm -fr" worry is tending toward the
hypothetical. Seasoned developers tend to be pretty careful and
usually avoid indiscriminately loose "rm -fr" invocations, so I'm
somewhat less worried about them. I do share the concern, though, that
newcomers crafting or extending tests could shoot themselves in the
foot with this. However, newcomers are also the ones most likely to
use the "cd foo && bar && cd .." idiom, so they are already at risk.

(As for not blasting my home directory when fixing all the existing
tests, I did run into a few cases where one or two "foreign" files
were deposited into the "t/" directory, but nothing was deleted or
overwritten.)

> 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

I thought of that too, but the inner (exit 117) doesn't even get
invoked unless there is &&-chain breakage somewhere above that point
(for instance, if "one" lacks "&&"), so the inner (exit 117) doesn't
participate in the linting process at all.

> 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.

Just thinking aloud...

Aside from "rm -fr", there are numerous ways to clobber files
unexpectedly when the "cd" is skipped:

    echo x >../git.c
    cp x ../git.c
    mv x ../git.c
    ln [-s] x ../git.c
    /bin/rm ../git.c
    some-cmd -o ../git.c

Some of these dangers can be de-thoothed during the linting phase by
defining do-nothing shell functions:

    cp () { :; }
    mv () { :; }
    ln () { :; }

That, at least, makes the scariest case ("rm") much less so.