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

From: Derrick Stolee <derrickstolee@github.com>
Date: Aug 3, 2022, 14:40 UTC
Message-ID: <475e8617-2adf-c75a-b697-d239dc4830b8@github.com>
In-Reply-To: <20220803045118.1243087-4-shaoxuan.yuan02@gmail.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.

Show 29 quoted lines

> 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