reset: fix mixed reset when using virtual filesystem #494

derrickstolee · 2022-04-01T19:08:03Z

This replaces #493 (can't reopen a PR after a force-push...).

I updated this commit with a more firm version of the fix. This hopefully answers Victoria's excellent concerns with the previous approach.

I did not manage to get an automated test for this, but I did carefully verify this manually with a few commits in a VFS for Git enlistment (with different files every time). I updated the commit message with more details about why this works.

This fork contains changes specific to monorepo scenarios. If you are an
external contributor, then please detail your reason for submitting to
this fork:

This change only applies to the virtualization hook and VFS for Git.

Resolves #490.

During the 2.35.0 rebase, we ejected 570f64b (Fix reset when using the sparse-checkout feature., 2017-03-15) because of a similar change upstream that actually works with the expected behavior of sparse-checkout. That commit only ever existed in microsoft/git, but when it was considered for upstream we realized that it behaved strangely for a sparse-checkout scenario. The root problem is that during a mixed reset, 'git reset <commit>' updates the index to aggree with <commit> but leaves the worktree the same as it was before. The issue with sparse-checkout is that some files might not be in the worktree and thus the information from those files would be "lost". The upstream decision was to leave these files as ignored, because that's what the SKIP_WORKTREE bit means: don't put these files in the worktree and ignore their contents. If there already were files in the worktree, then Git does not change them. The case for "losing" data is if a committed change outside of the sparse-checkout was in the previous HEAD position. However, this information could be recovered from the reflog. The case where this is different is in a virtualized filesystem. The virtualization is projecting the index contents onto the filesystem, so we need to do something different here. In a virtual environment, every file is considered "important" and we abuse the SKIP_WORKTREE bit to indicate that Git does not need to process a projected file. When a file is populated, the virtual filesystem hook provides the information for removing the SKIP_WORKTREE bit. In the case of these mixed resets, we have the issue where we change the projection of the worktree for these cache entries that change. If a file is populated in the worktree, then the populated file will persist and appear in a follow-up 'git status'. However, if the file is not populated and only projected, we change the projection from the current value to the new value, leaving a clean 'git status'. The previous version of this commit includes a call to checkout_entry(), which populates the file. This causes the file to be actually in the working tree and no longer projected. To make this work with the upstream changes, stop setting the skip-worktree bit for the new cache entry. This seemed to work fine without this change, but it's likely due to some indirection with the virtual filesystem. Better to do the best-possible thing here so we don't hide a corner-case bug by accident. Helped-by: Victoria Dye <vdye@github.com> Signed-off-by: Kevin Willford <kewillf@microsoft.com> Signed-off-by: Derrick Stolee <derrickstolee@github.com>

vdye

…lesystem This replaces #493 (can't reopen a PR after a force-push...). I updated this commit with a more firm version of the fix. This hopefully answers Victoria's excellent concerns with the previous approach. I did not manage to get an automated test for this, but I did carefully verify this manually with a few commits in a VFS for Git enlistment (with different files every time). I updated the commit message with more details about why this works. --- This fork contains changes specific to monorepo scenarios. If you are an external contributor, then please detail your reason for submitting to this fork: * [X] This change only applies to the virtualization hook and VFS for Git. Resolves #490.

This should fix #490, at least in the case that I've been able to verify. The issue is that reset was setting the skip-worktree bit more frequently than it should have. The previous fix in #494 was focused on the case where a file is added or removed across the diff. However, when the file exists on both sides but still needs to be staged in a mixed reset then we should avoid the skip-worktree bit. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

This should fix #646, at least in the case that I've been able to verify. The issue is that reset was setting the skip-worktree bit more frequently than it should have. The previous fix in #494 was focused on the case where a file is added or removed across the diff. However, when the file exists on both sides but still needs to be staged in a mixed reset then we should avoid the skip-worktree bit. --- This fork contains changes specific to monorepo scenarios. If you are an external contributor, then please detail your reason for submitting to this fork: * [X] This change only applies to the virtualization hook and VFS for Git.

This should fix #490, at least in the case that I've been able to verify. The issue is that reset was setting the skip-worktree bit more frequently than it should have. The previous fix in #494 was focused on the case where a file is added or removed across the diff. However, when the file exists on both sides but still needs to be staged in a mixed reset then we should avoid the skip-worktree bit. Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

…lesystem This replaces #493 (can't reopen a PR after a force-push...). I updated this commit with a more firm version of the fix. This hopefully answers Victoria's excellent concerns with the previous approach. I did not manage to get an automated test for this, but I did carefully verify this manually with a few commits in a VFS for Git enlistment (with different files every time). I updated the commit message with more details about why this works. --- This fork contains changes specific to monorepo scenarios. If you are an external contributor, then please detail your reason for submitting to this fork: * [X] This change only applies to the virtualization hook and VFS for Git. Resolves #490.

derrickstolee requested a review from vdye April 1, 2022 19:08

derrickstolee self-assigned this Apr 1, 2022

derrickstolee added the bug label Apr 1, 2022

vdye approved these changes Apr 1, 2022

View reviewed changes

derrickstolee merged commit 7e32301 into microsoft:vfs-2.35.1 Apr 4, 2022

derrickstolee deleted the vfs-reset-fix branch April 4, 2022 12:58

derrickstolee mentioned this pull request May 10, 2024

reset: fix mixed reset when using virtual filesystem (again) #651

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reset: fix mixed reset when using virtual filesystem #494

reset: fix mixed reset when using virtual filesystem #494

derrickstolee commented Apr 1, 2022 •

edited

Loading

vdye left a comment

reset: fix mixed reset when using virtual filesystem #494

reset: fix mixed reset when using virtual filesystem #494

Conversation

derrickstolee commented Apr 1, 2022 • edited Loading

vdye left a comment

Choose a reason for hiding this comment

derrickstolee commented Apr 1, 2022 •

edited

Loading