Fix restore --staged for certain vfs ops#804
Fix restore --staged for certain vfs ops#804tyrielv wants to merge 2 commits intomicrosoft:vfs-2.51.0from
restore --staged for certain vfs ops#804Conversation
When using virtualfilesystem, Running `git restore --staged` after `git cherry-pick -n` or `git reset --soft` would result in an incorrect state - the modified files would still have their modified contents, but git would no longer recognize them as being modified; and the added files would be deleted from disk instead of just unstaged. See microsoft/VFSForGit#1855 for more details. This commit fixes these issues with two changes: 1) Add a new flag to the index state structure, `clear_skip_worktree_for_added_entries`. This flag is set during cherry-pick -n before calculating the new index state, and causes newly added entries to have their SKIP_WORKTREE bit cleared. This ensures that newly added files are added to both the index and the worktree in the next step. Otherwise, sparse-checkout would prevent them from being added to the worktree. 3) Set the existing flag updated_skipworktree on the index when running a checkout index (aka `restore --staged`) operation. These operations already will clear SKIP_WORKTREE bits for modified files, but without this flag set the virtualfilesystem hook notification does not indicate that they changed, causing the VFS to not update its state correctly.
The failing test `t7113` is due to the change in `builtin/checkout.c` that now sets the `updated_skipworktree` flag. The test checked that this flag was not set, which is no longer valid.
|
|
||
| /* | ||
| * 3. If clear_skip_worktree_for_added_entries is set and we are checking | ||
| * for added entries, clear skip_wt_flag from all added entries. This is | ||
| * used when running with virtualfilesystem to ensure that added entries are | ||
| * also checked out in the working tree - otherwise skip_wt_flag will | ||
| * prevent that. | ||
| */ | ||
| if ((select_flag & CE_ADDED) | ||
| && istate->clear_skip_worktree_for_added_entries) { | ||
| for (i = 0; i < istate->cache_nr; i++) { | ||
| struct cache_entry *ce = istate->cache[i]; | ||
| if ((ce->ce_flags & (CE_ADDED | skip_wt_flag)) | ||
| == (CE_ADDED | skip_wt_flag)) { | ||
| ce->ce_flags &= ~skip_wt_flag; | ||
| istate->updated_skipworktree = 1; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
So this new loop runs after the previous two loops over the entire index (even clear_ce_flags() contains such a loop).
However, the virtual filesystem is already special-cased in clear_ce_flags(), via the call to `clear_ce_flags_virtualfilesystem.
Which makes me wonder whether it's that clear_ce_flags_virtualfilesystem() function that introduces that bug, and this here patch would only paper over that bug?
There was a problem hiding this comment.
From what I can tell, the loop you mention in clear_ce_flags is not run if virtualfilesystem is enabled - clear_ce_flags_virtualfilesystem is run in its place.
clear_ce_flags_virtualfilesystem does not have a full loop of the index - instead it only loops over the files in the virtual file system (which does not include the files that this loop is intended to update).
clear_ce_flags_virtualfilesystem may be a better place to put this loop though.
| git checkout -- dir1/file1.txt && | ||
| test_path_is_file testsuccess && rm -f testsuccess && | ||
| test_path_is_missing testfailure && |
There was a problem hiding this comment.
I don't think it's a good idea to remove these lines. Would the test be failing with the changes? That would be more indicative of a bug introduced by the changes, I think; The post-index-change hook is actively used in Office, and they use Scalar instead of VFSforGit, therefore virtual filesystem-specific changes should not affect them, I'd think.
There was a problem hiding this comment.
post-index-change is still called by git checkout, but due to this change it is setting one of the arguments with less discrimination, and this test case is for scenarios where neither of the arguments are set.
I will look into setting the argument more precisely so this test case will pass without change.
|
Withdrawing for now. |
2 participants
When using virtualfilesystem, Running
git restore --stagedaftergit cherry-pick -norgit reset --softwould result in an incorrect state - the modified files would still have their modified contents, but git would no longer recognize them as being modified; and the added files would be deleted from disk instead of just unstaged.See microsoft/VFSForGit#1855 for more details.
This commit fixes these issues with two changes:
Add a new flag to the index state structure,
clear_skip_worktree_for_added_entries. This flag is set during cherry-pick -n before calculating the new index state, and causes newly added entries to have their SKIP_WORKTREE bit cleared. This ensures that newly added files are added to both the index and the worktree in the next step. Otherwise, sparse-checkout would prevent them from being added to the worktree.Set the existing flag updated_skipworktree on the index when running a checkout index (aka
restore --staged) operation. These operations already will clear SKIP_WORKTREE bits for modified files, but without this flag set the virtualfilesystem hook notification does not indicate that they changed, causing the VFS to not update its state correctly.Thanks for taking the time to contribute to Git!
This fork contains changes specific to monorepo scenarios. If you are an
external contributor, then please detail your reason for submitting to
this fork:
GVFS Protocol.