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

[Eric Sunshine <sunshine@sunshineco.com>]

On Sat, Oct 17, 2020 at 12:02 PM Amanda Shafack via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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