[08/13] add: prevent adding sparse conflict files

Commit Message

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

From: Derrick Stolee <[e-mail]>

When a merge results in a conflict outside of the sparse-checkout cone,
the conflicted file is written to the working tree and the index entry
loses the SKIP_WORKTREE bit. This allows users to add the file to the
index without realizing that the file might leave the working tree in a
later Git command.

Block this behavior, but keep in mind that the user can override the
failure using the '--sparse' option.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 pathspec.c                               | 2 +-
 t/t1091-sparse-checkout-builtin.sh       | 4 +++-
 t/t1092-sparse-checkout-compatibility.sh | 6 +++---
 3 files changed, 7 insertions(+), 5 deletions(-)

Comments

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

On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
>
> From: Derrick Stolee <[e-mail]>
>
> When a merge results in a conflict outside of the sparse-checkout cone,
> the conflicted file is written to the working tree and the index entry
> loses the SKIP_WORKTREE bit. This allows users to add the file to the
> index without realizing that the file might leave the working tree in a
> later Git command.
>
> Block this behavior, but keep in mind that the user can override the
> failure using the '--sparse' option.

Hmm, didn't we already block this behavior at patch 6?

Nevertheless, as I mentioned there, I think the change to
find_pathspecs_matching_skip_worktree() from this patch should be
together with the other changes from 6.

Patch

diff --git a/pathspec.c b/pathspec.c
index 0e6e60fdc5a..ddeeba79114 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -71,7 +71,7 @@  char *find_pathspecs_matching_skip_worktree(const struct pathspec *pathspec)
 
 	for (i = 0; i < istate->cache_nr; i++) {
 		struct cache_entry *ce = istate->cache[i];
-		if (ce_skip_worktree(ce))
+		if (ce_skip_worktree(ce) || !path_in_sparse_checkout(ce->name, istate))
 		    ce_path_match(istate, ce, pathspec, seen);
 	}
 
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index e0f31186d89..b6efdb3c52f 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -406,7 +406,7 @@  test_expect_success 'sparse-checkout (init|set|disable) warns with unmerged stat
 	git -C unmerged sparse-checkout disable
 '
 
-test_expect_success 'sparse-checkout reapply' '
+test_expect_failure 'sparse-checkout reapply' '
 	git clone repo tweak &&
 
 	echo dirty >tweak/deep/deeper2/a &&
@@ -438,6 +438,8 @@  test_expect_success 'sparse-checkout reapply' '
 	test_i18ngrep "warning.*The following paths are unmerged" err &&
 	test_path_is_file tweak/folder1/a &&
 
+	# NEEDSWORK: We are asking to update a file outside of the
+	# sparse-checkout cone, but this is no longer allowed.
 	git -C tweak add folder1/a &&
 	git -C tweak sparse-checkout reapply 2>err &&
 	test_must_be_empty err &&
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 1e7799fd76a..65998e664a9 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -518,9 +518,8 @@  test_expect_success 'merge with conflict outside cone' '
 	test_all_match git status --porcelain=v2 &&
 
 	# 2. Add the file with conflict markers
-	# NEEDSWORK: Even though the merge conflict removed the
-	# SKIP_WORKTREE bit from the index entry for folder1/a, we should
-	# warn that this is a problematic add when --sparse is not set.
+	test_sparse_match test_must_fail git add folder1/a &&
+	test_i18ngrep "Disable or modify the sparsity rules" sparse-checkout-err &&
 	test_all_match git add --sparse folder1/a &&
 	test_all_match git status --porcelain=v2 &&
 
@@ -528,6 +527,7 @@  test_expect_success 'merge with conflict outside cone' '
 	#    accept conflict markers as resolved content.
 	run_on_all mv folder2/a folder2/z &&
 	test_sparse_match test_must_fail git add folder2 &&
+	test_i18ngrep "Disable or modify the sparsity rules" sparse-checkout-err &&
 	test_all_match git add --sparse folder2 &&
 	test_all_match git status --porcelain=v2 &&