Re: [PATCH] rm: honor sparse checkout patterns
- From
Derrick Stolee <stolee@gmail.com>
- Date
- Nov 13, 2020, 13:47 UTC
- Message-ID
- <70ae5b1d-b1d2-1b3f-11cf-8e5ceafe6048@gmail.com>
- In-Reply-To
- <CABPp-BGkkNwdjLB4HSygvU43OZRiuhDyt7RjrDGodWrfNgkBXQ@mail.gmail.com>
On 11/12/2020 6:54 PM, Elijah Newren wrote:
Show 15 quoted lines
> Hi, > > On Thu, Nov 12, 2020 at 1:02 PM Matheus Tavares > <matheus.bernardino@usp.br> wrote: >> >> Make git-rm honor the 'sparse.restrictCmds' setting, by restricting its >> operation to the paths that match both the command line pathspecs and >> the repository's sparsity patterns. This better matches the expectations >> of users with sparse-checkout definitions, while still allowing them >> to optionally enable the old behavior with 'sparse.restrictCmds=false' >> or the global '--no-restrict-to-sparse-paths' option. > > (For Stolee:) Did this arise when a user specified a directory to > delete, and a (possibly small) part of that directory was in the > sparse checkout while other portions of it were outside?
The user who suggested this used a command like 'git rm */*.csprojx' to remove all paths with that file extension, but then realized that they were deleting all of those files from the entire repo, not just the current sparse-checkout.
Show 5 quoted lines
> I can easily see users thinking they are dealing with just the files > relevant to them, and expecting the directory deletion to only affect > that relevant subset, so this seems like a great idea. We'd just want > to make sure we have a good error message if they explicitly list a > single path outside the sparse checkout.
We should definitely consider how to make this more usable for users who operate within a sparse-checkout but try to modify files outside the sparse-checkout.
Is there a warning message such as "the supplied pathspec doesn't match any known file" that we could extend to recommend possibly disabling the sparse.restrictCmds config? (I see that you identify one below.)
Show 16 quoted lines
>> +CONFIGURATION >> +------------- >> + >> +sparse.restrictCmds:: >> + By default, git-rm only matches and removes paths within the >> + sparse-checkout patterns. This behavior can be changed with the >> + `sparse.restrictCmds` setting or the global >> + `--no-restrict-to-sparse-paths` option. For more details, see the >> + full `sparse.restrictCmds` definition in linkgit:git-config[1]. > > Hmm, I wonder what people will think who are reading through the > manual and have never used sparse-checkout. This seems prone to > confusion for them. Maybe instead we could word this as: > > When sparse-checkouts are in use, by default git-rm will only match > and remove paths within the sparse-checkout patterns...
A preface such as "When using sparse-checkouts..." can help users ignore these config settings if they are unfamiliar with the concept.
Show 13 quoted lines
>> @@ -293,8 +294,12 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>>
>> seen = xcalloc(pathspec.nr, 1);
>>
>> + sparse_paths_only = restrict_to_sparse_paths(the_repository);
>> +
>> for (i = 0; i < active_nr; i++) {
>> const struct cache_entry *ce = active_cache[i];
>> + if (sparse_paths_only && ce_skip_worktree(ce))
>> + continue;
>> if (!ce_path_match(&the_index, ce, &pathspec, seen))
>> continue;
>> ALLOC_GROW(list.entry, list.nr + 1, list.alloc);This seems like an incredibly simple implementation! Excellent.
Show 21 quoted lines
>> +test_expect_success 'rm should respect --[no]-restrict-to-sparse-paths' ' >> + git init sparse-repo && >> + ( >> + cd sparse-repo && >> + touch a b c && >> + git add -A && >> + git commit -m files && >> + git sparse-checkout set "/a" && >> + >> + # By default, it should not rm paths outside the sparse-checkout >> + test_must_fail git rm b 2>stderr && >> + test_i18ngrep "fatal: pathspec .b. did not match any files" stderr && > > Ah, this answers my question about whether the user gets an error > message when they explicitly call out a single path outside the sparse > checkout. I'm curious if we want to be slightly more verbose on the > error message when sparse-checkouts are in effect. In particular, if > no paths match the sparsity patterns, but some paths would have > matched the pathspec ignoring the sparsity patterns, then perhaps the > error message should include a reference to the > --no-restrict-to-sparse-paths flag.
The error message could be modified similar to below:
if (!seen[i]) {
if (!ignore_unmatch) {
die(_("pathspec '%s' did not match any files%s"),
original,
sparse_paths_only
? _("; disable sparse.restrictCmds if you intend to edit outside the current sparse-checkout definition")
: "");
}
}Show 16 quoted lines
>> + >> + # But it should rm them with --no-restrict-to-sparse-paths >> + git --no-restrict-to-sparse-paths rm b && >> + >> + # And also with sparse.restrictCmds=false >> + git reset && >> + git -c sparse.restrictCmds=false rm b >> + ) >> +' >> + >> test_done > > Do we also want to include a testcase where the user specifies a > directory and part of that directory is within the sparsity paths and > part is out? E.g. 'git sparse-checkout set /sub/dir && git rm -r > sub' ?
That is definitely an interesting case. I'm not sure the current implementation will do the "right" thing here. Definitely worth testing, and it might require a more complicated implementation.
Show 13 quoted lines
>> diff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh >> index 26852586ac..1761a2b1b9 100755 >> --- a/t/t7011-skip-worktree-reading.sh >> +++ b/t/t7011-skip-worktree-reading.sh >> @@ -132,11 +132,6 @@ test_expect_success 'diff-files does not examine skip-worktree dirty entries' ' >> test -z "$(git diff-files -- one)" >> ' >> >> -test_expect_success 'git-rm succeeds on skip-worktree absent entries' ' >> - setup_absent && >> - git rm 1 >> -' >> -
Instead of deleting this case, perhaps we should just use "-c sparse.restrictCmds=false" in the 'git rm' command, so we are still testing this case?
Thanks again! I appreciate that you jumped on this suggestion.
-Stolee