unpack-trees: only clear CE_UPDATE|CE_REMOVE when skip-worktree is always set

The purpose of this clearing is, as explained in comment, because
verify_*() may set those bits before apply_sparse_checkout() is
called. By that time, it's not clear whether an entry will stay in
checkout area or out. After $GIT_DIR/info/sparse-checkout is applied,
we know what entries will be in finally. It's time to clean unwanted
bits.

That works perfectly when checkout area remains unchanged. When
checkout area changes, apply_sparse_checkout() may set CE_UPDATE
or CE_WT_REMOVE to widen/narrow checkout area. Doing the clearing
after apply_sparse_checkout() may clear those widening/narrowing
bits unexpectedly.

So, only do that on entries that are not affected by checkout area
changes (i.e. skip-worktree bit does not change after
apply_sparse_checkout).

This code does not actually fix anything though, just
future-proof. The removed code and the narrow/widen code inside
apply_sparse_checkout are currently independent (narrow code never
sets CE_REMOVE, widen code sets CE_UPDATE, but ce_skip_worktree()
would be false).

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

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

@@ -155,4 +155,16 @@ test_expect_success 'read-tree adds to worktree, dirty case' '
 	grep -q dirty sub/added
 '
 
+test_expect_success 'index removal and worktree narrowing at the same time' '
+	>empty &&
+	echo init.t >.git/info/sparse-checkout &&
+	echo sub/added >>.git/info/sparse-checkout &&
+	git checkout -f top &&
+	echo init.t >.git/info/sparse-checkout &&
+	git checkout removed &&
+	git ls-files sub/added >result &&
+	test ! -f sub/added &&
+	test_cmp empty result
+'
+
 test_done

unpack-trees.c

@@ -164,13 +164,18 @@ static int apply_sparse_checkout(struct cache_entry *ce, struct unpack_trees_opt
 		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 (!was_skip_worktree && !ce_skip_worktree()) {
+	 *	This is perfectly normal. Move on;
+	 * }
 	 */
-	if (ce->ce_flags & CE_REMOVE)
-		return 0;
+
+	/*
+	 * 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.
+	 */
+	if (was_skip_worktree && ce_skip_worktree(ce))
+		ce->ce_flags &= ~(CE_UPDATE | CE_REMOVE);
 
 	if (!was_skip_worktree && ce_skip_worktree(ce)) {
 		/*
@@ -796,14 +801,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 				ret = -1;
 				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(). Clear them.
-			 */
-			if (ce_skip_worktree(ce))
-				ce->ce_flags &= ~(CE_UPDATE | CE_REMOVE);
-			else
+			if (!ce_skip_worktree(ce))
 				empty_worktree = 0;
 
 		}