unpack-trees: let read-tree -u remove index entries outside sparse area

To avoid touching the worktree outside a sparse checkout,
when the update flag is enabled unpack_trees() clears the
CE_UPDATE and CE_REMOVE flags on entries that do not match the
sparse pattern before actually committing any updates to the
index file or worktree.

The effect on the index was unintentional; sparse checkout was
never meant to prevent index updates outside the area checked
out.  And the result is very confusing: for example, after a
failed merge, currently "git reset --hard" does not reset the
state completely but an additional "git reset --mixed" will.

So stop clearing the CE_REMOVE flag.  Instead, maintain a
CE_WT_REMOVE flag to separately track whether a particular
file removal should apply to the worktree in addition to the
index or not.

The CE_WT_REMOVE flag is used already to mark files that
should be removed because of a narrowing checkout area.  That
usage will still apply; do not clear the CE_WT_REMOVE flag
in that case (detectable because the CE_REMOVE flag is not
set).

This bug masked some other bugs illustrated by the test
suite, which will be addressed by later patches.

Reported-by: Frédéric Brière <[email protected]>
Fixes: http://bugs.debian.org/583699

Signed-off-by: Nguyễn Thái Ngọc Duy <[email protected]>
Reviewed-by: Jonathan Nieder <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>

Author: pclouds

cache.h

@@ -179,8 +179,7 @@ struct cache_entry {
 #define CE_UNHASHED  (0x200000)
 #define CE_CONFLICTED (0x800000)
 
-/* Only remove in work directory, not index */
-#define CE_WT_REMOVE (0x400000)
+#define CE_WT_REMOVE (0x400000) /* remove in work directory */
 
 #define CE_UNPACKED  (0x1000000)
 

t/t1011-read-tree-sparse-checkout.sh

@@ -146,7 +146,7 @@ test_expect_success 'read-tree adds to worktree, absent case' '
 	test ! -f sub/added
 '
 
-test_expect_success 'read-tree adds to worktree, dirty case' '
+test_expect_failure 'read-tree adds to worktree, dirty case' '
 	echo init.t >.git/info/sparse-checkout &&
 	git checkout -f removed &&
 	mkdir sub &&
@@ -167,4 +167,13 @@ test_expect_success 'index removal and worktree narrowing at the same time' '
 	test_cmp empty result
 '
 
+test_expect_success 'read-tree --reset removes outside worktree' '
+	>empty &&
+	echo init.t >.git/info/sparse-checkout &&
+	git checkout -f top &&
+	git reset --hard removed &&
+	git ls-files sub/added >result &&
+	test_cmp empty result
+'
+
 test_done

unpack-trees.c

@@ -53,6 +53,9 @@ 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;
@@ -92,7 +95,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_REMOVE | CE_WT_REMOVE))
+			if (ce->ce_flags & (CE_UPDATE | CE_WT_REMOVE))
 				total++;
 		}
 
@@ -112,12 +115,6 @@ static int check_updates(struct unpack_trees_options *o)
 				unlink_entry(ce);
 			continue;
 		}
-
-		if (ce->ce_flags & CE_REMOVE) {
-			display_progress(progress, ++cnt);
-			if (o->update)
-				unlink_entry(ce);
-		}
 	}
 	remove_marked_cache_entries(&o->result);
 	remove_scheduled_dirs();
@@ -172,10 +169,22 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt
 	/*
 	 * 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(). Clear them.
+	 * verify_absent() and verify_uptodate().
+	 * Make sure they don't modify worktree if they are already
+	 * outside checkout area
 	 */
-	if (was_skip_worktree && ce_skip_worktree(ce))
-		ce->ce_flags &= ~(CE_UPDATE | CE_REMOVE);
+	if (was_skip_worktree && ce_skip_worktree(ce)) {
+		ce->ce_flags &= ~CE_UPDATE;
+
+		/*
+		 * By default, when CE_REMOVE is on, CE_WT_REMOVE is also
+		 * on to get that file removed from both index and worktree.
+		 * If that file is already outside worktree area, don't
+		 * bother remove it.
+		 */
+		if (ce->ce_flags & CE_REMOVE)
+			ce->ce_flags &= ~CE_WT_REMOVE;
+	}
 
 	if (!was_skip_worktree && ce_skip_worktree(ce)) {
 		/*