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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 26, 2018, 19:52 UTC
Message-ID: <CAPig+cSLyie8mr+u8Thv9cJ0J12nCA+RU6Mg3S5F8U68q1+nzQ@mail.gmail.com>
In-Reply-To: <xmqqwouljr5e.fsf@gitster-ct.c.googlers.com>

On Tue, Jun 26, 2018 at 3:15 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 21 quoted lines

> so, with --chain-lint, we would transform this
>
>         mkdir -p a/b/c &&
>         (
>                 cd a/b/c
>                 rm -fr ../../*
>         ) &&
>         statement 4
>
> into this sequence
>
>         (exit $sentinel) &&
>         mkdir -p a/b/c &&
>                 cd a/b/c
>                 rm -fr ../../* &&
>         statement 4
>
> and then rely on the non-zero exit to cancel all the remainder?
>
> We didn't create nor cd to the t/trash$num/a/b/c thanks to the &&
> chain, and end up running rm -fr ../../* from inside t/trash$num?

Yes, I did take that into account and, no, I don't have a good answer to the issue.

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.

Thanks for thinking critically about it.