Re: [PATCH v4 7/7] mv: add check_dir_in_index() and solve general dir check issue

[Derrick Stolee <derrickstolee@github.com>]

On 6/24/2022 3:57 AM, Shaoxuan Yuan wrote:
> On 6/23/2022 11:14 PM, Derrick Stolee wrote:
>> On 6/23/2022 7:41 AM, Shaoxuan Yuan wrote:
>>> +/*
>>> + * Check if an out-of-cone directory should be in the index. Imagine this case
>>> + * that all the files under a directory are marked with 'CE_SKIP_WORKTREE' bit
>>> + * and thus the directory is sparsified.
>>> + *
>>> + * Return 0 if such directory exist (i.e. with any of its contained files not
>>> + * marked with CE_SKIP_WORKTREE, the directory would be present in working tree).
>>> + * Return 1 otherwise.
>>> + */
>>
>> This description and the implementation seems like it will work
>> even if the path exists as a sparse directory in a sparse index.
>>
>> It would be good to consider testing this kind of move for a
>> directory on the sparse boundary (where it would be a sparse
>> directory in a sparse index) _and_ if it is deeper than the
>> boundary (so the sparse index would expand in the cache_name_pos()
>> method). These tests can be written now for correctness, but later
>> the first case can be updated to use the 'ensure_not_expanded'
>> helper in t1092.
> 
> I'm a bit confused here. Shouldn't we turn on the sparse-index
> feature for 'mv' before adding sparse-index related tests? Since this
> series does not go into sparse-index, I'm not sure how the tests can
> pass. Perhaps we can test about this in the future sparse-index
> integration series, no?

I mean that you are making a change right now that might lead to
different behavior _when you enable the sparse index later_. Since
we are looking at this behavior now, it might be interesting to add
the extra test coverage now while we are thinking about this data
shape.

We can add tests to t1092-sparse-checkout-compatibility.sh even
though the sparse index is expanded on every 'git mv' instance, but
it can help when you eventually update 'git mv' to not expand on a
sparse index.

The bit about ensure_not_expanded can't be added until you integrate
with the sparse index, but it is easier to add that when you already
have tests that check these boundary cases.

Thanks,
-Stolee