Re: [PATCH 2/2] t2200: modify code syntax

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 18, 2020, 05:55 UTC
Message-ID: <CAPig+cRuutHJ64aXQy5X9h-Vg4=_NKKyL57KdwbvXj4cGRS1PQ@mail.gmail.com>
In-Reply-To: <0a1550cb225591a53118bd8f7782d95de5a4df34.1602950552.git.gitgitgadget@gmail.com>

On Sat, Oct 17, 2020 at 12:02 PM Amanda Shafack via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 14 quoted lines

> Using the logical not operator on both the git and grep command is
> redundant, since the main check is done at the level of the grep
> command.
>
> Apply the logical not operator only to the grep command.
> ---
> diff --git a/t/t2200-add-update.sh b/t/t2200-add-update.sh
> @@ -179,10 +179,8 @@ test_expect_success 'add -u resolves unmerged paths' '
> -       ! (
> -               git ls-files >actual &&
> -               grep "non-existent" actual
> -       )
> +       git ls-files >actual &&
> +       ! grep "non-existent" actual

The commit message talks only about redundancy of applying the logical NOT operator to the combined subshell content, but what it misses is that transformation of:

    ! (git ls-files | grep "non-existent")

in patch [1/2] into:

    ! (
        git ls-files >actual &&
        grep "non-existent" actual
    )

is actively wrong. In particular, if `git ls-files` itself fails, then the `grep` will never get run, and the subshell will exit with a failure, however, the NOT then turns that failure into a success, which is quite undesirable. The test should fail if either `git ls-files` fails or if the `grep` expectation is not met; it should not succeed when `git ls-files` fails.

So, the correct thing to do is to merge [1/2] and [2/2] into a single patch which directly transforms:

    ! (git ls-files | grep "non-existent")

into:

    git ls-files >actual &&
    ! grep "non-existent" actual