Skip to content

worktree remove: use gvfs_config_is_set for skip-clean-check gate#875

Merged
mjcheetham merged 1 commit intomicrosoft:vfs-2.53.0from
tyrielv:tyrielv/fix-worktree-remove
Apr 2, 2026
Merged

worktree remove: use gvfs_config_is_set for skip-clean-check gate#875
mjcheetham merged 1 commit intomicrosoft:vfs-2.53.0from
tyrielv:tyrielv/fix-worktree-remove

Conversation

@tyrielv
Copy link
Copy Markdown

@tyrielv tyrielv commented Mar 30, 2026

The skip-clean-check guard in remove_worktree() was gated on core_virtualfilesystem, which is only initialized by repo_config_get_virtualfilesystem() during index loading. Since the worktree remove path never loads the index before this check, the variable was always NULL, causing check_clean_worktree() to run even when VFSForGit had already unmounted the projection and written the skip-clean-check marker file. This made 'git worktree remove' fail with 'fatal: failed to run git status' in GVFS repos.

Replace core_virtualfilesystem with gvfs_config_is_set(GVFS_USE_VIRTUAL_FILESYSTEM), which is already loaded from core.gvfs by cmd_worktree() before dispatch to remove_worktree().

dscho
dscho previously approved these changes Mar 31, 2026
Copy link
Copy Markdown
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

Good rationale.

A request going forward: Could you add an Assisted-by: Claude Opus 4.6 and your Signed-off-by: trailer, and drop that Co-authored-by: <bogus-email> trailer in your commit messages, please?

Also: I wonder whether any other bare core_virtualfilesystems need the same treatment?

@dscho
Copy link
Copy Markdown
Member

dscho commented Mar 31, 2026

Also: I wonder whether any other bare core_virtualfilesystems need the same treatment?

I guess that the core_virtualfilesystem variable needs to go, actually, and be replaced by accessing gvfs_config_is_set(the_repository, GVFS_USE_VIRTUAL_FILESYSTEM) always, right?

@tyrielv
Copy link
Copy Markdown
Author

tyrielv commented Mar 31, 2026

Good rationale.

A request going forward: Could you add an Assisted-by: Claude Opus 4.6 and your Signed-off-by: trailer, and drop that Co-authored-by: <bogus-email> trailer in your commit messages, please?

Acknowledged

Also: I wonder whether any other bare core_virtualfilesystems need the same treatment?

I guess that the core_virtualfilesystem variable needs to go, actually, and be replaced by accessing gvfs_config_is_set(the_repository, GVFS_USE_VIRTUAL_FILESYSTEM) always, right?

Anywhere it's used as a flag, it probably should be changed (since I think we've long given up on making the contributions generic enough that a different virtual filesystem could be used than VFSForGit). The content of core_virtualfilesystem is a hook to call to get the list of modified files tracked by the vfs, so when it's used as that hook it would stay as-is.

I'll work on that.

@tyrielv tyrielv force-pushed the tyrielv/fix-worktree-remove branch from e3e19e3 to 778fea4 Compare March 31, 2026 15:33
@tyrielv
Copy link
Copy Markdown
Author

tyrielv commented Mar 31, 2026

Also: I wonder whether any other bare core_virtualfilesystems need the same treatment?

I guess that the core_virtualfilesystem variable needs to go, actually, and be replaced by accessing gvfs_config_is_set(the_repository, GVFS_USE_VIRTUAL_FILESYSTEM) always, right?

Anywhere it's used as a flag, it probably should be changed (since I think we've long given up on making the contributions generic enough that a different virtual filesystem could be used than VFSForGit). The content of core_virtualfilesystem is a hook to call to get the list of modified files tracked by the vfs, so when it's used as that hook it would stay as-is.

I'll work on that.

Every other place core_virtualfilesystem is used as a guard it's related to working with a git index. core_virtualfilesystem is deliberately cleared even if it was configured if the index isn't from the on-disk .git/index file, so it makes sense to use it as a guard in those cases. Also, core_virtualfilesystem is initialized when reading an index, so it's always initialized in the other cases.

This location should definitely use gvfs_config_is_set - but now I think it should use the new GVFS_SUPPORT_WORKTREES bit instead of GVFS_USE_VIRTUAL_FILESYSTEM to be clearer.

The skip-clean-check guard in remove_worktree() was gated on
core_virtualfilesystem, which is only initialized by
repo_config_get_virtualfilesystem() during index loading. Since the
worktree remove path never loads the index before this check, the
variable was always NULL, causing check_clean_worktree() to run even
when VFSForGit had already unmounted the projection and written the
skip-clean-check marker file. This made 'git worktree remove' fail
with 'fatal: failed to run git status' in GVFS repos.

Replace core_virtualfilesystem with
gvfs_config_is_set(GVFS_SUPPORTS_WORKTREES). This is the correct bit
to check here: remove_worktree() can only be reached when
GVFS_SUPPORTS_WORKTREES is set (cmd_worktree blocks otherwise at line
1501), and it directly expresses that the VFS layer supports worktree
operations and knows how to signal when a clean check can be skipped.
Unlike core_virtualfilesystem, gvfs_config_is_set() is self-loading
from core.gvfs and does not depend on the index having been read.

Assisted-by: Claude Opus 4.6
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Copy link
Copy Markdown
Member

@mjcheetham mjcheetham left a comment

Choose a reason for hiding this comment

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

Given the use of core_virtualfilesystem in every other place in the codebase, and the issue this caused here, maybe it's worth just a small comment in the code about why we're calling gvfs_config_is_set instead. Future readers may wonder why not just use core_virtualfileystem.

Approving to not block.

@mjcheetham mjcheetham merged commit 7f1a5a4 into microsoft:vfs-2.53.0 Apr 2, 2026
122 of 123 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants