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
