[PATCH v2 8/8] sparse-checkout: make 'clean' clear more files

["Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>]

From: Derrick Stolee <stolee@gmail.com>

The 'git sparse-checkout clean' command is designed to be a one-command
way to get the worktree in a state such that a sparse index would
operate efficiently. The previous change demonstrated that files outside
the sparse-checkout that were committed due to a merge conflict would
persist despite attempts to run 'git sparse-checkout clean' and instead
a 'git sparse-checkout reapply' would be required.

Instead of requiring users to run both commands, update 'clean' to be
more ruthless about tracked sparse directories. The key here is to make
sure that the SKIP_WORKTREE bit is removed from more paths in the index
using update_sparsity() before compressing the index to a sparse one
in-memory.

The tricky part here is that update_sparsity() was previously assuming
that it would be in 'update' mode and would change the worktree as it
made changes. However, we do not want to make these worktree changes at
this point, instead relying on our later logic (that integrates with
--dry-run and --verbose options) to perform those steps.

One side-effect here is that we also clear out staged files that exist
in the worktree, but they would also appear in the verbose output as
part of the dry run.

The final test in t1091 demonstrates that we no longer need the
'reapply' subcommand for merge resolutions. It also fixes an earlier
case where 'git add --sparse' clears the SKIP_WORKTREE bit and avoids a
directory deletion.

Signed-off-by: Derrick Stolee <stolee@gmail.com>
---
 builtin/sparse-checkout.c          |  8 ++++++++
 t/t1091-sparse-checkout-builtin.sh | 24 +++++++++++++++++-------
 unpack-trees.c                     |  2 +-
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index f38a0809c098..1d1d5208a3ba 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -962,6 +962,7 @@ static int sparse_checkout_clean(int argc, const char **argv,
 	size_t worktree_len;
 	int force = 0, dry_run = 0, verbose = 0;
 	int require_force = 1;
+	struct unpack_trees_options o = { 0 };
 
 	struct option builtin_sparse_checkout_clean_options[] = {
 		OPT__DRY_RUN(&dry_run, N_("dry run")),
@@ -990,6 +991,13 @@ static int sparse_checkout_clean(int argc, const char **argv,
 	if (repo_read_index(repo) < 0)
 		die(_("failed to read index"));
 
+	o.verbose_update = verbose;
+	o.update = 0; /* skip modifying the worktree here. */
+	o.head_idx = -1;
+	o.src_index = o.dst_index = repo->index;
+	if (update_sparsity(&o, NULL))
+		warning(_("failed to reapply sparse-checkout patterns"));
+
 	if (convert_to_sparse(repo->index, SPARSE_INDEX_MEMORY_ONLY) ||
 	    repo->index->sparse_index == INDEX_EXPANDED)
 		die(_("failed to convert index to a sparse index; resolve merge conflicts and try again"));
diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
index 116ad7c9a20e..4b9078d90a61 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -1104,6 +1104,7 @@ test_expect_success 'clean with staged sparse change' '
 
 	cat >expect <<-\EOF &&
 	Would remove deep/deeper2/
+	Would remove folder1/
 	EOF
 
 	git -C repo sparse-checkout clean --dry-run >out &&
@@ -1115,6 +1116,7 @@ test_expect_success 'clean with staged sparse change' '
 	# deletes deep/deeper2/ but leaves folder1/ and folder2/
 	cat >expect <<-\EOF &&
 	Removing deep/deeper2/
+	Removing folder1/
 	EOF
 
 	# The previous test case checked the -f option, so
@@ -1124,7 +1126,7 @@ test_expect_success 'clean with staged sparse change' '
 	test_cmp expect out &&
 
 	test_path_is_missing repo/deep/deeper2 &&
-	test_path_exists repo/folder1 &&
+	test_path_is_missing repo/folder1 &&
 	test_path_exists repo/folder2
 '
 
@@ -1147,7 +1149,11 @@ test_expect_success 'sparse-checkout operations with merge conflicts' '
 		git commit -a -m "left" &&
 
 		git checkout -b merge &&
-		git sparse-checkout set deep/deeper1 &&
+
+		touch deep/deeper2/extra &&
+		git sparse-checkout set deep/deeper1 2>err &&
+		grep "contains untracked files" err &&
+		test_path_exists deep/deeper2/extra &&
 
 		test_must_fail git merge -m "will-conflict" right &&
 
@@ -1159,15 +1165,19 @@ test_expect_success 'sparse-checkout operations with merge conflicts' '
 		git merge --continue &&
 
 		test_path_exists folder1/even/more/dirs/file &&
+		test_path_exists deep/deeper2/extra &&
+
+		cat >expect <<-\EOF &&
+		Removing deep/deeper2/
+		Removing folder1/
+		EOF
 
 		# clean does not remove the file, because the
 		# SKIP_WORKTREE bit was not cleared by the merge command.
 		git sparse-checkout clean -f >out &&
-		test_line_count = 0 out &&
-		test_path_exists folder1/even/more/dirs/file &&
-
-		git sparse-checkout reapply &&
-		test_path_is_missing folder1
+		test_cmp expect out &&
+		test_path_is_missing folder1 &&
+		test_path_is_missing deep/deeper2
 	)
 '
 
diff --git a/unpack-trees.c b/unpack-trees.c
index 0e9813bddf04..b8814af1b07c 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2138,7 +2138,7 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o,
 	index_state_init(&o->internal.result, o->src_index->repo);
 
 	/* Sanity checks */
-	if (!o->update || o->index_only || o->skip_sparse_checkout)
+	if (o->index_only || o->skip_sparse_checkout)
 		BUG("update_sparsity() is for reflecting sparsity patterns in working directory");
 	if (o->src_index != o->dst_index || o->fn)
 		BUG("update_sparsity() called wrong");
-- 
gitgitgadget