From 26b5d6b0e56898a0284d0f45dd5a69f59257a6ab Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Fri, 14 Jan 2022 15:59:40 +0000 Subject: [PATCH] unpack-trees: fix accidental loss of user changes For sparse-checkouts, we don't want unpack-trees to error out on files that are missing from the worktree, so there has traditionally been logic to make it skip the verify_uptodate() check for these. Unfortunately, it was skipping the verify_uptodate() check for files that were expected to *become* SKIP_WORKTREE. For files that were not already SKIP_WORKTREE, that can cause us to later delete the file in apply_sparse_checkout(). Only skip the check for files that were already SKIP_WORKTREE as well to avoid lightly discarding important changes users may have made to files. Note 1: unpack-trees.c is already a bit complex, and the logic around CE_SKIP_WORKTREE and CE_NEW_SKIP_WORKTREE in that file are no exception. I also tried just replacing CE_NEW_SKIP_WORKTREE with CE_SKIP_WORKTREE in the verify_uptodate() check instead of checking for both flags, and found that it also fixed this bug and passed all the tests. I also attempted to devise a few testcases that might trip either variant of my fix and was unable to find any problems. It may be that just checking CE_SKIP_WORKTREE is a better fix, but I'm not sure. I thought it was a bit safer to strictly reduce the number of cases where we skip the up-to-date check rather than just toggling which kind of cases skip it, and thus went with the current variant of the fix. Note 2: I also wondered if verify_absent() might have a similar bug, but despite my attempts to try to devise a testcase that would trigger such a thing, I couldn't find any problematic testcases. Thus, this patch makes no attempt to apply similar changes to verify_absent() and verify_absent_if_directory(). Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano --- t/t1011-read-tree-sparse-checkout.sh | 2 +- unpack-trees.c | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh index 1b2395b8a82670..4ed0885bf2fa3d 100755 --- a/t/t1011-read-tree-sparse-checkout.sh +++ b/t/t1011-read-tree-sparse-checkout.sh @@ -197,7 +197,7 @@ test_expect_success 'read-tree will not throw away dirty changes, non-sparse' ' grep -q dirty init.t ' -test_expect_failure 'read-tree will not throw away dirty changes, sparse' ' +test_expect_success 'read-tree will not throw away dirty changes, sparse' ' echo "/*" >.git/info/sparse-checkout && read_tree_u_must_succeed -m -u HEAD && diff --git a/unpack-trees.c b/unpack-trees.c index 360844bda3ab97..96525d2ec265ce 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -2065,7 +2065,9 @@ static int verify_uptodate_1(const struct cache_entry *ce, int verify_uptodate(const struct cache_entry *ce, struct unpack_trees_options *o) { - if (!o->skip_sparse_checkout && (ce->ce_flags & CE_NEW_SKIP_WORKTREE)) + if (!o->skip_sparse_checkout && + (ce->ce_flags & CE_SKIP_WORKTREE) && + (ce->ce_flags & CE_NEW_SKIP_WORKTREE)) return 0; return verify_uptodate_1(ce, o, ERROR_NOT_UPTODATE_FILE); }