[10/13] rm: skip sparse paths with missing SKIP_WORKTREE

Commit Message

Derrick Stolee Aug. 24, 2021, 9:54 p.m. UTC

From: Derrick Stolee <[e-mail]>

If a path does not match the sparse-checkout cone but is somehow missing
the SKIP_WORKTREE bit, then 'git rm' currently succeeds in removing the
file. One reason a user might be in this situation is a merge conflict
outside of the sparse-checkout cone. Removing such a file might be
problematic for users who are not sure what they are doing.

Add a check to path_in_sparse_checkout() when 'git rm' is checking if a
path should be considered for deletion. Of course, this check is ignored
if the '--sparse' option is specified, allowing users who accept the
risks to continue with the removal.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 builtin/rm.c                  |  4 +++-
 t/t3602-rm-sparse-checkout.sh | 11 +++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

Matheus Tavares Aug. 27, 2021, 9:18 p.m. UTC | #1

On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
>
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 4208f3f9a5f..a6da03da2be 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -301,7 +301,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>         for (i = 0; i < active_nr; i++) {
>                 const struct cache_entry *ce = active_cache[i];
>
> -               if (!include_sparse && ce_skip_worktree(ce))
> +               if (!include_sparse &&
> +                   (ce_skip_worktree(ce) ||
> +                    !path_in_sparse_checkout(ce->name, &the_index)))
>                         continue;

OK. And we don't need to update the advice reporting code below,
because `matches_skip_worktree()` already uses the
`(ce_skip_worktree(ce) || !path_in_sparse_checkout())`  logic.

> +                    !path_in_sparse_checkout"

>                 if (!ce_path_match(&the_index, ce, &pathspec, seen))
>                         continue;
> diff --git a/t/t3602-rm-sparse-checkout.sh b/t/t3602-rm-sparse-checkout.sh
> index a34b978bfd8..44f3e923164 100755
> --- a/t/t3602-rm-sparse-checkout.sh
> +++ b/t/t3602-rm-sparse-checkout.sh
> @@ -87,4 +87,15 @@ test_expect_success 'do not warn about sparse entries with --ignore-unmatch' '
>         git ls-files --error-unmatch b
>  '
>
> +test_expect_success 'refuse to rm a non-skip-worktree path outside sparse cone' '
> +       git reset --hard &&
> +       git sparse-checkout set a &&
> +       git update-index --no-skip-worktree b &&
> +       test_must_fail git rm b 2>stderr &&
> +       test_cmp b_error_and_hint stderr &&
> +       git rm --sparse b 2>stderr &&
> +       test_must_be_empty stderr &&
> +       test_path_is_missing b
> +'

OK.

Patch

diff --git a/builtin/rm.c b/builtin/rm.c
index 4208f3f9a5f..a6da03da2be 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -301,7 +301,9 @@  int cmd_rm(int argc, const char **argv, const char *prefix)
 	for (i = 0; i < active_nr; i++) {
 		const struct cache_entry *ce = active_cache[i];
 
-		if (!include_sparse && ce_skip_worktree(ce))
+		if (!include_sparse &&
+		    (ce_skip_worktree(ce) ||
+		     !path_in_sparse_checkout(ce->name, &the_index)))
 			continue;
 		if (!ce_path_match(&the_index, ce, &pathspec, seen))
 			continue;
diff --git a/t/t3602-rm-sparse-checkout.sh b/t/t3602-rm-sparse-checkout.sh
index a34b978bfd8..44f3e923164 100755
--- a/t/t3602-rm-sparse-checkout.sh
+++ b/t/t3602-rm-sparse-checkout.sh
@@ -87,4 +87,15 @@  test_expect_success 'do not warn about sparse entries with --ignore-unmatch' '
 	git ls-files --error-unmatch b
 '
 
+test_expect_success 'refuse to rm a non-skip-worktree path outside sparse cone' '
+	git reset --hard &&
+	git sparse-checkout set a &&
+	git update-index --no-skip-worktree b &&
+	test_must_fail git rm b 2>stderr &&
+	test_cmp b_error_and_hint stderr &&
+	git rm --sparse b 2>stderr &&
+	test_must_be_empty stderr &&
+	test_path_is_missing b
+'
+
 test_done