On 6/15/2022 7:18 PM, Eric Sunshine wrote: > On Tue, Jun 14, 2022 at 5:36 PM Derrick Stolee via GitGitGadget > <gitgitgadget@xxxxxxxxx> wrote: >> 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@xxxxxxxxxx> >> --- >> 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. >> 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
