| Message ID | 20220803045118.1243087-4-shaoxuan.yuan02@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rm: integrate with sparse-index | expand |
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
On 8/3/2022 10:40 PM, Derrick Stolee wrote: > 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. I have absolutely no idea why I wrote this paragraph this way, maybe I was zoning out composing it. Will fix. >> 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. Makes sense. Will fix. -- Thanks, Shaoxuan
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];
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. 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(-)