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

On 6/15/2022 7:18 PM, Eric Sunshine wrote:

On Tue, Jun 14, 2022 at 5:36 PM Derrick Stolee via GitGitGadget
[off-list ref] wrote:

quoted

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

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.

Thanks for pointing this out. I shouldn't have left this change in.
 

quoted

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.

I understand that the test wants to test specific behavior, and that
behavior is focused on certain inputs to 'git update-index', but I
also think that the --verbose option presents _additional information_
about what is expected from these behaviors. It doesn't change the
already-tested behavior, only enhances it.

If I separate things out and only had test for --verbose, I would need
to replicate many of these behaviors just for that option, which would
be wasteful.

In this particular case, I'm demonstrating that the --verbose mode
still reports the file as added (because of the earlier 'git add file')
even though the command as a whole failed due to an invalid OID.

Thanks,
-Stolee