git worktree support for VFSForGit#1911
Conversation
2ff5914 to
ab088d8
Compare
Add TryGetWorktreeInfo() to detect git worktrees by checking for a .git file (not directory) and reading its gitdir pointer. WorktreeInfo carries the worktree name, paths, and derived pipe suffix. Add GVFSEnlistment.CreateForWorktree() factory that constructs an enlistment with worktree-specific paths: WorkingDirectoryRoot points to the worktree, DotGitRoot uses the shared .git directory, and NamedPipeName includes a worktree-specific suffix. Add WorktreeCommandParser to extract subcommands and positional args from git worktree hook arguments. Add GVFS_SUPPORTS_WORKTREES to GitCoreGVFSFlags enum. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Update GetGVFSPipeName() in common.windows.cpp to detect when running inside a git worktree. If the current directory contains a .git file (not directory), read the gitdir pointer, extract the worktree name, and append a _WT_<NAME> suffix to the pipe name. This single change makes all native hooks (read-object, post-index-changed, virtual-filesystem) connect to the correct worktree-specific GVFS mount process. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
MountVerb: detect worktree paths via TryGetWorktreeInfo(), create worktree-specific GVFSEnlistment, check worktree-specific pipe for already-mounted state, register worktrees by their own path (not the primary enlistment root). UnmountVerb: resolve worktree pipe name for unmount, unregister by worktree path so the primary enlistment registration is not affected. InProcessMount: bootstrap worktree metadata (.gvfs/ inside worktree gitdir), set absolute paths for core.hookspath and core.virtualfilesystem, skip hook installation for worktree mounts (hooks are shared via hookspath), set GVFS_SUPPORTS_WORKTREES bit. GitIndexProjection/FileSystemCallbacks: use worktree-specific index path instead of assuming primary .git/index. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
153f071 to
07dbad0
Compare
In the managed pre/post-command hooks, intercept git worktree
subcommands to transparently manage GVFS mounts:
add: Post-command runs 'git checkout -f' to create the index,
then 'gvfs mount' to start ProjFS projection.
remove: Pre-command checks for uncommitted changes while ProjFS
is alive, writes skip-clean-check marker, unmounts.
Post-command remounts if removal failed (dir + .git exist).
move: Pre-command unmounts old path, post-command mounts new.
prune: Post-command cleans stale worktree metadata.
Add WorktreeCommandParser reference to GVFS.Hooks.csproj.
Assisted-by: Claude Opus 4.6
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Unit tests: WorktreeInfoTests — TryGetWorktreeInfo detection, pipe suffix WorktreeEnlistmentTests — CreateForWorktree path mappings WorktreeCommandParserTests — subcommand and arg extraction Functional tests: WorktreeTests — end-to-end add/list/remove with live GVFS mount GitBlockCommandsTests — update existing test for conditional block Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
07dbad0 to
e4314b3
Compare
ProjFS cannot handle nested virtualization roots. Add a pre-command check that blocks 'git worktree add' when the target path is inside the primary enlistment's working directory. Add IsPathInsideDirectory() utility to GVFSEnlistment.Shared.cs with unit tests for path matching (case-insensitive, sibling paths allowed). Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Before removing a worktree, probe the named pipe to verify the GVFS mount is running. If not mounted: - Without --force: error with guidance to mount or use --force - With --force: skip unmount and let git proceed Refactor UnmountWorktree to accept a pre-resolved WorktreeInfo to avoid redundant TryGetWorktreeInfo calls. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Normalize paths in IsPathInsideDirectory using Path.GetFullPath to prevent traversal attacks with segments like '/../'. Add GetKnownWorktreePaths to enumerate existing worktrees from the .git/worktrees directory, and block creating a worktree inside any existing worktree — not just the primary VFS working directory. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
KeithIsSleeping
left a comment
There was a problem hiding this comment.
Risk Analysis Review
Reviewed from an OS developer perspective, focusing on GVFS + worktree integration risks, ProjFS concerns, test coverage, and deployment safety.
Assumption: GVFS is only used in the OS repo, so OS-specific layout assumptions (e.g., src/ subdirectory) are acceptable.
Summary: 3 critical issues, 4 high-risk issues, several medium concerns and test gaps. See inline comments for details.
GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/WorktreeTests.cs
Outdated
Show resolved
Hide resolved
Copy the primary index to a temp file first, then rename atomically into the worktree's index path. A direct File.Copy on a live index risks a torn read on large indexes. Clean up the temp file on failure. Wrap the .vfs-empty-hook script creation and deletion in try/finally so the file is always cleaned up even if git checkout crashes. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Guarantee RepoMetadata.Shutdown() is called even if an unexpected exception occurs between TryInitialize and Shutdown. Without this, the process-global singleton could be left pointing at the wrong metadata directory. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Replace MAX_PATH fixed buffers in GetWorktreePipeSuffix() with std::wstring/std::string to handle long worktree paths safely. Use dynamic MultiByteToWideChar sizing instead of fixed buffer. Replace the 10-iteration pipe polling loop after gvfs unmount with a simple sleep. The unmount command already blocks until the mount process exits; the sleep allows remaining ProjFS handles to close. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
…ions Write the primary enlistment root to a marker file in the worktree gitdir during creation (gvfs-enlistment-root). WorktreeInfo.GetEnlistmentRoot() reads this marker, falling back to the GetDirectoryName chain for worktrees created before this change. Replace all GetDirectoryName(GetDirectoryName(SharedGitDir)) chains in MountVerb, UnmountVerb, GVFSMountProcess, and GVFSEnlistment with the new GetEnlistmentRoot() method. Replace hardcoded "src" with GVFSConstants.WorkingDirectoryRootName in the nested worktree path check. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Restore enlistmentRoot parameter in UnmountVerb.AcquireLock so the "Run gvfs log" message appears on lock acquisition failure. Add backward-compatible WaitUntilMounted(tracer, enlistmentRoot, unattended, out error) overload for out-of-tree callers. Narrow bare catch in TryGetWorktreeInfo to IOException and UnauthorizedAccessException to avoid swallowing unexpected errors. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Process.StartInfo.Arguments is empty for externally-launched processes, so the old code that matched GVFS.Mount by arguments would never find or kill stuck mounts. Replace with gvfs unmount which uses the named pipe to cleanly shut down the mount process. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Replace the single-worktree functional test with a concurrent test that creates two worktrees in parallel, verifies both have projected files and clean status, commits in both, verifies cross-visibility of commits (shared objects), and removes both in parallel. Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
KeithIsSleeping
left a comment
There was a problem hiding this comment.
All review items have been addressed in commits 6-15:
| Review Item | Status | Commit |
|---|---|---|
| CRITICAL: Index copy race | ✅ Fixed — atomic temp+rename | e1ec486 |
| CRITICAL: RepoMetadata singleton | ✅ Fixed — try/finally | 0b19c7a |
| CRITICAL: core.gvfs unconditional | ✅ Acknowledged — bit-check is safe | noted in thread |
| HIGH: MAX_PATH native hooks | ✅ Fixed — dynamic buffers | 137571b |
| HIGH: Hardcoded "src" path | ✅ Fixed — uses WorkingDirectoryRootName | 8351b71 |
| HIGH: Fragile enlistment root derivation | ✅ Fixed — explicit marker file | 8351b71 |
| HIGH: Unmount timeout | ✅ Fixed — simplified wait | 137571b |
| MEDIUM: Service registration orphaning | Open — see thread | — |
| MEDIUM: No worktree limit | ✅ Acknowledged | noted in thread |
| MEDIUM: null gvfsEnlistmentRoot | ✅ Fixed | b58d060 |
| MEDIUM: Shell script cleanup | ✅ Fixed — try/finally | e1ec486 |
| MEDIUM: WaitUntilMounted API break | ✅ Fixed — compat overload | b58d060 |
| MINOR: Bare catch | ✅ Fixed — specific exceptions | b58d060 |
| MINOR: --orphan flag | ✅ N/A — my comment was incorrect, --orphan is a boolean flag |
|
| TEST GAP: Broken cleanup | ✅ Fixed — uses gvfs unmount | 6b4e6b2 |
| TEST GAP: Concurrent tests | ✅ Added | 321b5a4 |
LGTM — all critical and high items resolved. Only the service registration orphaning item remains open as a future consideration.
…tree-2 # Conflicts: # GVFS/GVFS.Common/GVFSEnlistment.cs # GVFS/GVFS.Mount/InProcessMount.cs Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
mjcheetham
left a comment
There was a problem hiding this comment.
Since this is a pretty big PR I'll be reviewing in waves to save losing unposted review comments. Follow up review rounds incoming after this one.
mjcheetham
left a comment
There was a problem hiding this comment.
Here's the rest of my review!
GVFS/GVFS.Hooks/Program.Worktree.cs
Outdated
| ProcessHelper.Run("gvfs", $"unmount \"{fullPath}\"", redirectOutput: false); | ||
|
|
||
| // After gvfs unmount exits, ProjFS handles may still be closing. | ||
| // Wait briefly to allow the OS to release all handles before git | ||
| // attempts to delete the worktree directory. | ||
| System.Threading.Thread.Sleep(200); |
There was a problem hiding this comment.
If unmount fails then we may wish to stop Git from continuing to try and delete the still-mounted directory?
Add GVFS_SUPPORTS_WORKTREES flag (1<<8) to core.gvfs bitmask. When set, allow git worktree commands to run on VFS-enabled repos instead of blocking them with BLOCK_ON_VFS_ENABLED. Force --no-checkout during worktree add when VFS is active so ProjFS can be mounted before files are projected. Support skip-clean-check marker file in worktree gitdir: if .git/worktrees/<name>/skip-clean-check exists, skip the cleanliness check during worktree remove. This allows VFSForGit's pre-command hook to unmount ProjFS after its own status check, then let git proceed without re-checking (which would fail without the virtual filesystem). The corresponding change in VFSForGit is microsoft/VFSForGit#1911 * [x] This change only applies to the virtualization hook and VFS for Git.
TryGetWorktreeInfo: - Walk up from subdirectories to find worktree root - Canonicalize directory to absolute path - Require commondir file for valid worktree (return null if missing) - Add out string error overload; callers fail or warn on IO errors - Add GitDirPrefix, CommonDirName, SkipCleanCheckName constants WorktreeCommandParser: - Handle combined short flags (e.g., -fd, -fb branch, -bfd) - Separate long/short option handling - Handle --git-pid/--exit_code as separate-value options - Document assumptions and note Mono.Options as future improvement Hooks: - Write empty marker file instead of "1" for skip-clean-check - Check unmount exit code; block git on failure unless --force - Reference PhysicalFileSystem.TryCopyToTempFileAndRename in comment Other: - Revert whitespace-only changes in InProcessMountVerb.cs - New unit tests for subdirectory detection, combined flags, baked-in values Assisted-by: Claude Opus 4.6 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
f42cf3d to
d933fae
Compare
git worktreesupport for VFSForGitMotivation
VS Code Copilot Chat background agents use
git worktreeto create isolated working directories for code editing. On VFSForGit enlistments,git worktreeis blocked byBLOCK_ON_VFS_ENABLEDbecause the VFS layer had no support for multiple working trees. This PR enables worktree support by running an independent GVFS mount (ProjFS instance) per worktree.Companion PR
microsoft/git: tyrielv/gvfs-worktree (1 commit, 4 files, +82 lines)
GVFS_SUPPORTS_WORKTREES(1<<8) to thecore.gvfsbitmaskgit worktreewhen the bit is set--no-checkoutwhen VFS is active (GVFS handles checkout after mount)skip-clean-checkmarker for pre-unmount cleanliness verificationt0402-block-command-on-gvfs.shDesign
Each worktree gets its own:
GVFS_<ROOT>_WT_<NAME>) for IPC with hooks.git/worktrees/<name>/.gvfs/Worktrees share:
core.hookspath(absolute path).git/configvia git'scommondirmechanismHow it works
git worktree add <path>→ git creates the worktree structure with--no-checkout(forced by the git-side change)worktree add→ runsgit checkout -fto create the index → runsgvfs mount <path>.gitfile →gitdir:→.git/worktrees/<name>/) → creates worktree-specificGVFSEnlistment→ starts ProjFS → files appear_WT_<NAME>pipe suffixgit worktree remove→ pre-command hook unmounts ProjFS → git deletes the directoryCommits (review in order)
TryGetWorktreeInfo()detection logic,CreateForWorktree()path mappings.gitfile detection inGetGVFSPipeName(), pipe suffix derivationMountVerb/UnmountVerbworktree detection, service registration by worktree path,InProcessMountmetadata bootstrap, index path fixesskip-clean-checkflow, remount-on-failure recovery,WorktreeCommandParserarg extractionTesting done
git worktree add— auto-mounts, files visible via ProjFSgit worktree list— shows primary + worktreesgit worktree move— unmounts old, mounts newgit worktree remove— unmounts, cleans upgit worktree prune— cleans stale entriesgvfs service --unmount-all/--mount-all— worktrees survive the cyclet0402) — 18/18 passKnown limitations
worktree removepartially succeeds (ProjFS unmounted but directory deletion fails due to locked file), the worktree is left in a degraded state. The post-command hook attempts remount only if both the directory and.gitfile still exist.