Re: [PATCH v2] add, rm, mv: fix bug that prevents the update of non-sparse dirs

[Derrick Stolee <stolee@gmail.com>]

On 10/26/2021 6:43 PM, Matheus Tavares wrote:> On Tue, Oct 26, 2021 at 9:53 AM Derrick Stolee <stolee@gmail.com> wrote:
>>> - Inside the loop, made sure to change dtype to DT_DIR when going to parent
>>>   directories. Without this, the pattern match would fail if we had a path
>>>   like "a/b/c" and the pattern "b/" (with trailing slash).
>>
>> Very good. We typically need to detect the type for the first path given,
>> but we know that all parents are directories. I've used this trick elsewhere.
>>
>> I see in the code that the first path is used as DT_REG. It's my fault, but
>> perhaps it should be made more clear that path_in_sparse_checkout() will
>> consider the given path as a file, not a directory. The current users of the
>> method are using it properly, but I'm suddenly worried about another caller
>> misinterpreting the generality of the problem.
> 
> Yeah, I was thinking about this too... I'm afraid there might be at
> least two users of this function which already pass non-regular files
> to it: builtin/add.c:refresh() and
> sparse-index.c:convert_to_sparse_rec().
> 
> The first calls the function passing the user-given pathspec, which
> may be a directory. But this one is easy to solve: I think we don't
> even need the path_in_sparse_checkout() here as the `git add
> --refresh` only work on tracked files, and the previous
> matches_skip_worktree() call covers both skip_worktree and
> non-skip_worktree index entries (maybe we should rename this function
> to matches_sparse_ce()?)
> 
> As for convert_to_sparse_rec(), it seems to call
> path_in_sparse_checkout() with the directory components of paths, so
> something like "dir/". Perhaps we can make path_in_sparse_checkout()
> receive a dtype argument and pass DT_UNKNOWN in this case?

This might be necessary. Thanks for digging into the details here.

> Another case I haven't given much thought yet is submodules. For example:
> 
> git init sub &&
> test_commit -C sub file &&
> git submodule add ./sub &&
> git commit -m sub &&
> git sparse-checkout set 'sub/' &&
> git mv sub sub2
> 
> Erroneously gives:
> The following paths and/or pathspecs matched paths that exist
> outside of your sparse-checkout definition, so will not be
> updated in the index:
> sub
> 
> But it works if we change DT_REG to DT_UNKNOWN in
> path_in_sparse_checkout(). So, I'm not sure, should we use DT_UNKNOWN
> for all calls?

This is interesting. Submodules aren't controlled by the sparse-checkout,
so we should probably check the cache entry to see if it is a gitlink
and skip the path_in_sparse_checkout() if so.

Good find!
-Stolee