[PATCH 2/2] read-tree -m -u: always remove relevant files when narrowing checkout

[Jonathan Nieder <jrnieder@gmail.com>]

When narrowing the checkout pattern to exclude a file that the merge
was going to delete, read-tree decides two wrongs make a right and
leaves the file behind.

Or that’s what it looks like.  Actually, for some reason
apply_sparse_checkout() is not paying proper attention to entries
removed by the merge, so they look like ordinary deletions outside
the sparse area.

While fixing that, clarify the semantics of the unpack-trees flags:

 - CE_UPDATE marks an entry for which the worktree might need
   to be updated to match.  If CE_UPDATE is not set, changes
   only apply to the index file.

 - CE_REMOVE marks an entry removed by the merge (just like
   it did before).

 - CE_WT_REMOVE marks an entry that no longer matches the sparse
   pattern.

Cc: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Sorry for the lack of clarity before.  This is just to give the
idea; I have no confidence that I am not breaking something,
especially because I have not checked why the "if" I am removing
in apply_sparse_checkout was added.

 cache.h                              |    3 +-
 t/t1011-read-tree-sparse-checkout.sh |   21 +++++++++++++++++
 unpack-trees.c                       |   42 +++++++++------------------------
 3 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/cache.h b/cache.h
index 3c7fb6f..c9fa3df 100644
--- a/cache.h
+++ b/cache.h
@@ -179,7 +179,8 @@ struct cache_entry {
 #define CE_UNHASHED  (0x200000)
 #define CE_CONFLICTED (0x800000)
 
-#define CE_WT_REMOVE (0x400000) /* remove in work directory */
+/* Only remove in work directory, not index */
+#define CE_WT_REMOVE (0x400000)
 
 #define CE_UNPACKED  (0x1000000)
 
diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 2955071..247f295 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -155,6 +155,27 @@ test_expect_failure 'read-tree adds to worktree, dirty case' '
 	grep -q dirty sub/added
 '
 
+test_expect_success 'narrow checkout while removing file' '
+	echo "H init.t" >expected.swt-after &&
+	printf "%s\n" init.t sub/added | sort >expected.ls-before
+	echo init.t >expected.ls-after &&
+
+	printf "%s\n" init.t sub/added >.git/info/sparse-checkout &&
+	git checkout -f top &&
+	git ls-files -t >index.before &&
+	ls init.t sub/added | sort >worktree.before &&
+
+	echo init.t >.git/info/sparse-checkout &&
+	git read-tree -m -u HEAD^ &&
+	git ls-files -t >index.after &&
+	ls init.t sub/added | sort >worktree.after &&
+
+	test_cmp expected.swt index.before &&
+	test_cmp expected.swt-after index.after &&
+	test_cmp expected.ls-before worktree.before &&
+	test_cmp expected.ls-after worktree.after
+'
+
 test_expect_success 'read-tree --reset removes outside worktree' '
 	>empty &&
 	echo init.t >.git/info/sparse-checkout &&
diff --git a/unpack-trees.c b/unpack-trees.c
index 523e1de..deabfd9 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -53,9 +53,6 @@ static void add_entry(struct unpack_trees_options *o, struct cache_entry *ce,
 
 	clear |= CE_HASHED | CE_UNHASHED;
 
-	if (set & CE_REMOVE)
-		set |= CE_WT_REMOVE;
-
 	memcpy(new, ce, size);
 	new->next = NULL;
 	new->ce_flags = (new->ce_flags & ~clear) | set;
@@ -87,7 +84,7 @@ static int check_updates(struct unpack_trees_options *o)
 	if (o->update && o->verbose_update) {
 		for (total = cnt = 0; cnt < index->cache_nr; cnt++) {
 			struct cache_entry *ce = index->cache[cnt];
-			if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
+			if (ce->ce_flags & CE_UPDATE)
 				total++;
 		}
 
@@ -101,11 +98,12 @@ static int check_updates(struct unpack_trees_options *o)
 	for (i = 0; i < index->cache_nr; i++) {
 		struct cache_entry *ce = index->cache[i];
 
-		if (ce->ce_flags & CE_WT_REMOVE) {
+		if (ce->ce_flags & CE_UPDATE &&
+		    ce->ce_flags & (CE_REMOVE | CE_WT_REMOVE)) {
 			display_progress(progress, ++cnt);
+			ce->ce_flags &= ~CE_UPDATE & ~CE_WT_REMOVE;
 			if (o->update)
 				unlink_entry(ce);
-			continue;
 		}
 	}
 	remove_marked_cache_entries(&o->result);
@@ -152,15 +150,6 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt
 	else
 		ce->ce_flags &= ~CE_SKIP_WORKTREE;
 
-	/*
-	 * We only care about files getting into the checkout area
-	 * If merge strategies want to remove some, go ahead, this
-	 * flag will be removed eventually in unpack_trees() if it's
-	 * outside checkout area.
-	 */
-	if (ce->ce_flags & CE_REMOVE)
-		return 0;
-
 	if (!was_skip_worktree && ce_skip_worktree(ce)) {
 		/*
 		 * If CE_UPDATE is set, verify_uptodate() must be called already
@@ -169,7 +158,7 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt
 		 */
 		if (!(ce->ce_flags & CE_UPDATE) && verify_uptodate_sparse(ce, o))
 			return -1;
-		ce->ce_flags |= CE_WT_REMOVE;
+		ce->ce_flags |= CE_WT_REMOVE | CE_UPDATE;
 	}
 	if (was_skip_worktree && !ce_skip_worktree(ce)) {
 		if (verify_absent_sparse(ce, "overwritten", o))
@@ -796,20 +785,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 				goto done;
 			}
 			/*
-			 * Merge strategies may set CE_UPDATE|CE_REMOVE outside checkout
-			 * area as a result of ce_skip_worktree() shortcuts in
-			 * verify_absent() and verify_uptodate().
-			 * Make sure they don't modify worktree.
+			 * Do not modify worktree outside of sparse checkout area,
+			 * except to make the checkout more narrow.
 			 */
-			if (ce_skip_worktree(ce)) {
-				ce->ce_flags &= ~CE_UPDATE;
-
-				if (ce->ce_flags & CE_REMOVE)
-					ce->ce_flags &= ~CE_WT_REMOVE;
-			}
-			else
+			if (!ce_skip_worktree(ce))
 				empty_worktree = 0;
-
+			else if (!(ce->ce_flags & CE_WT_REMOVE))
+				ce->ce_flags &= ~CE_UPDATE;
 		}
 		if (o->result.cache_nr && empty_worktree) {
 			ret = unpack_failed(o, "Sparse checkout leaves no entry on working directory");
@@ -968,7 +950,7 @@ static int verify_clean_subdirectory(struct cache_entry *ce, const char *action,
 		if (!ce_stage(ce2)) {
 			if (verify_uptodate(ce2, o))
 				return -1;
-			add_entry(o, ce2, CE_REMOVE, 0);
+			add_entry(o, ce2, CE_REMOVE|CE_UPDATE, 0);
 			mark_ce_used(ce2, o);
 		}
 		cnt++;
@@ -1138,7 +1120,7 @@ static int deleted_entry(struct cache_entry *ce, struct cache_entry *old,
 	}
 	if (!(old->ce_flags & CE_CONFLICTED) && verify_uptodate(old, o))
 		return -1;
-	add_entry(o, ce, CE_REMOVE, 0);
+	add_entry(o, ce, CE_REMOVE|CE_UPDATE, 0);
 	invalidate_ce_path(ce, o);
 	return 1;
 }
-- 
1.7.2.1.544.ga752d.dirty