Re: [PATCH v2 01/14] t3705: test that 'sparse_entry' is unstaged

[Derrick Stolee <stolee@gmail.com>]

On 9/15/2021 1:22 AM, Elijah Newren wrote:
> On Sun, Sep 12, 2021 at 6:23 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> The tests in t3705-add-sparse-checkout.sh check to see how 'git add'
>> behaves with paths outside the sparse-checkout definition. These
>> currently check to see if a given warning is present but not that the
>> index is not updated with the sparse entries. Add a new
>> 'test_sparse_entry_unstaged' helper to be sure 'git add' is behaving
>> correctly.
>>
>> We need to modify setup_sparse_entry to actually commit the sparse_entry
>> file so it exists at HEAD but is not already staged in the index.
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>  t/t3705-add-sparse-checkout.sh | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
>> index 2b1fd0d0eef..af81b4b6846 100755
>> --- a/t/t3705-add-sparse-checkout.sh
>> +++ b/t/t3705-add-sparse-checkout.sh
>> @@ -19,6 +19,7 @@ setup_sparse_entry () {
>>         fi &&
>>         git add sparse_entry &&
>>         git update-index --skip-worktree sparse_entry &&
>> +       git commit --allow-empty -m "ensure sparse_entry exists at HEAD" &&
>>         SPARSE_ENTRY_BLOB=$(git rev-parse :sparse_entry)
>>  }
>>
>> @@ -36,6 +37,11 @@ setup_gitignore () {
>>         EOF
>>  }
>>
>> +test_sparse_entry_unstaged () {
>> +       git status --porcelain >actual &&
>> +       ! grep "^M  sparse_entry\$" actual
> 
> Is there a reason this is ^M rather than ^D?  Granted, both would be
> bugs so I wouldn't expect either to appear, but the point of the check
> is looking for likely errors.  Wouldn't the more likely error case for
> a file not present in the working tree be that we stage the deletion
> of the file?

You are right that we should be checking for deletions or adds
_as well_ as modifications. The test_sparse_entry_unstaged helper
is used in a variety of situations that typically would trigger
a modification, but at least one instance in this test is a
possible deletion.

> 
>> +}
>> +
>>  test_expect_success 'setup' "
>>         cat >sparse_error_header <<-EOF &&
>>         The following pathspecs didn't match any eligible path, but they do match index
>> @@ -55,6 +61,7 @@ test_expect_success 'git add does not remove sparse entries' '
>>         setup_sparse_entry &&
>>         rm sparse_entry &&
>>         test_must_fail git add sparse_entry 2>stderr &&
>> +       test_sparse_entry_unstaged &&

Here, sparse_entry could be staged as a deletion.

>>         test_cmp error_and_hint stderr &&
>>         test_sparse_entry_unchanged
>>  '
>> @@ -73,6 +80,7 @@ test_expect_success 'git add . does not remove sparse entries' '
>>         rm sparse_entry &&
>>         setup_gitignore &&
>>         test_must_fail git add . 2>stderr &&
>> +       test_sparse_entry_unstaged &&

Deletion here.

>>         cat sparse_error_header >expect &&
>>         echo . >>expect &&
>> @@ -88,6 +96,7 @@ do
>>                 setup_sparse_entry &&
>>                 echo modified >sparse_entry &&
>>                 test_must_fail git add $opt sparse_entry 2>stderr &&
>> +               test_sparse_entry_unstaged &&

But here would be modified.

>>                 test_cmp error_and_hint stderr &&
>>                 test_sparse_entry_unchanged
>>         '
>> @@ -98,6 +107,7 @@ test_expect_success 'git add --refresh does not update sparse entries' '
>>         git ls-files --debug sparse_entry | grep mtime >before &&
>>         test-tool chmtime -60 sparse_entry &&
>>         test_must_fail git add --refresh sparse_entry 2>stderr &&
>> +       test_sparse_entry_unstaged &&

Same here.

>>         test_cmp error_and_hint stderr &&
>>         git ls-files --debug sparse_entry | grep mtime >after &&
>>         test_cmp before after
>> @@ -106,6 +116,7 @@ test_expect_success 'git add --refresh does not update sparse entries' '
>>  test_expect_success 'git add --chmod does not update sparse entries' '
>>         setup_sparse_entry &&
>>         test_must_fail git add --chmod=+x sparse_entry 2>stderr &&
>> +       test_sparse_entry_unstaged &&

Here it would be modified with a mode change.

Using the pattern "^[MDARCU][M ] sparse_entry\$" seems to work while also
covering these other cases.

Thanks,
-Stolee