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

From: Derrick Stolee <derrickstolee@github.com>
Date: Jun 16, 2022, 12:54 UTC
Message-ID: <a5c4ca33-abf5-7adf-2a91-2dadf7a98d9b@github.com>
In-Reply-To: <CAPig+cTtZ3=r62XDTE4gpy0Fk_gkzOROm34gW6gkuGRjdQRBTA@mail.gmail.com>

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

Show 22 quoted lines

> On Tue, Jun 14, 2022 at 5:36 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> 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@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.

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

Show 26 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.

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