Re: [PATCH v1 3/4] rm: expand the index only when necessary

[Derrick Stolee <derrickstolee@github.com>]

On 8/3/2022 12:51 AM, Shaoxuan Yuan wrote:
> Originally, rm a pathspec that is out-of-cone in a sparse-index
> environment, Git dies with "pathspec '<x>' did not match any files",
> mainly because it does not expand the index so nothing is matched.

This paragraph appears to be assuming that we've stopped expanding the
sparse index already. It might be worthwhile to rewrite this to say
"Before integrating 'git rm' with the sparse index, we need to..." or
something like that. 

> Remove the `ensure_full_index()` method so `git-rm` does not always
> expand the index when the expansion is unnecessary, i.e. when
> <pathspec> does not have any possibilities to match anything outside
> of sparse-checkout definition.
> 
> Expand the index when the <pathspec> needs an expanded index, i.e. the
> <pathspec> contains wildcard that may need a full-index or the
> <pathspec> is simply outside of sparse-checkout definition.
> 
> Signed-off-by: Shaoxuan Yuan <shaoxuan.yuan02@gmail.com>
> ---
>  builtin/rm.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 84a935a16e..58ed924f0d 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -296,8 +296,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>  
>  	seen = xcalloc(pathspec.nr, 1);
>  
> -	/* TODO: audit for interaction with sparse-index. */
> -	ensure_full_index(&the_index);
> +	if (pathspec_needs_expanded_index(&the_index, &pathspec))
> +		ensure_full_index(&the_index);
> +
>  	for (i = 0; i < active_nr; i++) {
>  		const struct cache_entry *ce = active_cache[i];

Looking back on the tests in patch 1, I don't see any tests that really
emphasize the kinds of pathspecs that could not ever integrate with the
sparse index. They are all of the form "folder1/*" or similar, making it
be something that could be seen as a prefix match. Such a pattern _could_
be integrated carefully with the sparse index.

Instead, something like `git rm "*/a"` would be much harder to integrate
with the sparse index. Could we add a test (in this patch) that checks
that kind of case. That would also help justify this as its own patch and
not squashed with patch 4.

Thanks,
-Stolee