Re: [PATCH 1/4] t2107: test 'git update-index --verbose'

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 15, 2022, 23:18 UTC
Message-ID: <CAPig+cTtZ3=r62XDTE4gpy0Fk_gkzOROm34gW6gkuGRjdQRBTA@mail.gmail.com>
In-Reply-To: <c6803df1b6afead99a0a6a383ab9aa563920f464.1655242070.git.gitgitgadget@gmail.com>

On Tue, Jun 14, 2022 at 5:36 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 14 quoted lines

> The '--verbose' option reports what is being added and removed from the
> index, but has not been tested up to this point. Augment the tests in
> t2107 to check the '--verbose' option in some scenarios.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
> diff --git a/t/t2106-update-index-assume-unchanged.sh b/t/t2106-update-index-assume-unchanged.sh
> @@ -20,7 +20,7 @@ test_expect_success 'do not switch branches with dirty file' '
>         echo dirt >file &&
> -       git update-index --assume-unchanged file &&
> +       git update-index --verbose --assume-unchanged file &&
>         test_must_fail git checkout - 2>err &&
>         test_i18ngrep overwritten err
>  '

If this test passes with or without the addition of `--verbose`, then adding `--verbose` unnecessarily only pollutes what is (presumably) the minimum code necessary to implement what the test is checking, and may confuse future readers into thinking that something subtle is going on.

Show 16 quoted lines

> diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
> @@ -36,9 +36,14 @@ test_expect_success '--cacheinfo does not accept blob null sha1' '
>         echo content >file &&
>         git add file &&
>         git rev-parse :file >expect &&
> -       test_must_fail git update-index --cacheinfo 100644 $ZERO_OID file &&
> +       test_must_fail git update-index --verbose --cacheinfo 100644 $ZERO_OID file >out &&
>         git rev-parse :file >actual &&
> -       test_cmp expect actual
> +       test_cmp expect actual &&
> +
> +       cat >expect <<-\EOF &&
> +       add '\''file'\''
> +       EOF
> +       test_cmp expect out
>  '

While I understand your desire to address a gap in the test coverage, I worry that this sort of change, which is orthogonal to the test's stated purpose, has the same downsides as mentioned above (i.e. polluting the minimum necessary code, and potentially confusing readers). Rather than piggybacking on existing tests, adding one or two new standalone tests dedicated to checking `--verbose` would be more palatable, more understandable, and be less likely to confuse future readers. The same comment applies to the remaining changes in this patch.