Skip to content

unpack-trees: skip lstats for deleted VFS entries in checkout #865

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
tyrielv wants to merge 1 commit into
base: vfs-2.53.0
Choose a base branch
from
Open

unpack-trees: skip lstats for deleted VFS entries in checkout #865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions t/t1093-virtualfilesystem.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,58 @@ test_expect_success 'folder with same prefix as file' '
test_cmp expected actual
'

test_expect_success 'checkout skips lstat for deleted skip-worktree entries in VFS mode' '
# When switching branches, entries present in the old tree but absent
# in the new tree go through deleted_entry() -> verify_absent_if_directory().
# Without the CE_NEW_SKIP_WORKTREE propagation fix, the tree entry
# lacks that flag, so the fast-path fails and verify_absent_1() lstats
# the path. If a directory exists where the deleted file entry was
# (simulating a worst-case scenario), the lstat finds it and
# verify_clean_subdirectory() rejects the checkout due to untracked
# content inside.
#
# With the fix, CE_NEW_SKIP_WORKTREE is propagated and the fast-path
# succeeds — no lstat, no rejection, checkout completes.
#
# Set up two branches: main has dir1/ + dir2/, side has only dir1/
clean_repo &&

git -c core.virtualfilesystem= checkout -b side &&
git -c core.virtualfilesystem= rm -rf dir2 &&
git -c core.virtualfilesystem= commit -m "remove dir2" &&
git -c core.virtualfilesystem= checkout main &&

# Configure VFS hook that returns nothing (0% hydration)
write_script .git/hooks/virtualfilesystem <<-\EOF &&
printf ""
EOF

# Create a directory where the deleted file entry is, with
# untracked content inside. This would not happen with a real
# VFS because the VFS would report the file-to-directory change
# in the virtualfilesystem hook results, clearing skip-worktree.
# But it lets us verify that the lstat is not called: without
# the fix, verify_absent_1() lstats this path, finds a directory,
# and verify_clean_subdirectory() rejects the checkout because of
# the untracked file inside.
rm -f dir2/file1.txt &&
mkdir -p dir2/file1.txt &&
echo "untracked" >dir2/file1.txt/trap.txt &&

# Verify all entries are skip-worktree before checkout
git ls-files -v >actual &&
! grep "^H " actual &&

# Checkout to side branch. Without the fix this fails because
# verify_absent_1 finds untracked content in the directory at
# dir2/file1.txt. With the fix the lstat is skipped entirely.
git checkout side &&

# Clean up: return to main so subsequent tests have dir2/
rm -rf dir2/file1.txt &&
git -c core.virtualfilesystem= checkout main
'

test_expect_success MINGW,FSMONITOR_DAEMON 'virtualfilesystem hook disables built-in FSMonitor' '
clean_repo &&
test_config core.usebuiltinfsmonitor true &&
Expand Down
16 changes: 14 additions & 2 deletions unpack-trees.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2720,8 +2720,20 @@ static int deleted_entry(const struct cache_entry *ce,
if (verify_absent(ce, ERROR_WOULD_LOSE_UNTRACKED_REMOVED, o))
return -1;
return 0;
} else if (verify_absent_if_directory(ce, ERROR_WOULD_LOSE_UNTRACKED_REMOVED, o)) {
return -1;
} else {
Copy link

@derrickstolee derrickstolee Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an experiment, I wanted to see what changed if we wrote it like this:

} else if (core_virtualfilesystem &&
           old->ce_flags & CE_NEW_SKIP_WORKTREE) {
        ((struct cache_entry *)ce)->ce_flags |= CE_NEW_SKIP_WORKTREE;
} else if (verify_absent_if_directory(ce, ERROR_WOULD_LOSE_UNTRACKED_REMOVED, o)) {
        return -1;
}

This takes the guesswork out of it in terms of whether this is effective. But the key difference lies inside the implementation of verify_absent_if_directory() which only short-circuits on CE_NEW_SKIP_WORKTREE if o->skip_sparse_checkout is unset. I wonder if there is a world where core_virtualfilesystem and o->skip_sparse_checkout could ever be enabled at the same time.

Copy link
Author

@tyrielv tyrielv Mar 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right - it looks like the only current scenario where skip_sparse_checkout could be false when core_virtualfilesystem is true is if validation of the sparse checkout patterns fails, and we're already in trouble in that case.
I'm not sure whether it's necessary to still propagate the flag if we skip verify_absent_if_directory entirely. I'll look into it, but I'd also be fine if this merged in the meantime. (I don't have permissions to merge).
@dscho what do you think?

/*
* When core_virtualfilesystem is set, 'ce' may be a tree
* entry from traverse_trees() that lacks CE_NEW_SKIP_WORKTREE
* (only src_index entries get that flag from
* mark_new_skip_worktree()). Propagate it from the index
* entry so verify_absent_if_directory()'s fast-path works,
* avoiding unnecessary lstats on virtualized paths.
*/
if (core_virtualfilesystem &&
old->ce_flags & CE_NEW_SKIP_WORKTREE)
((struct cache_entry *)ce)->ce_flags |= CE_NEW_SKIP_WORKTREE;
if (verify_absent_if_directory(ce, ERROR_WOULD_LOSE_UNTRACKED_REMOVED, o))
return -1;
}

if (!(old->ce_flags & CE_CONFLICTED) && verify_uptodate(old, o))
Expand Down
Loading