Repository navigation
Use worktree-specific Git index paths - #2113
Tyrie Vella (tyrielv) wants to merge 6 commits into
Conversation
Route index generation, repair, FastFetch, and dehydrate backups through GitIndexPath. Reject full dehydrate for linked worktrees before it moves the .git file. Assisted-by: GPT-5.6 Sol Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Reject full dehydrate when linked worktrees exist. Store worktree backups beside each worktree with unique paths. Preserve backup errors and add regression coverage. Assisted-by: GPT-5.6 Sol Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Nathan Baird (ShiningMassXAcc)
left a comment
Nathan Baird (ShiningMassXAcc)
left a comment
There was a problem hiding this comment.
Review pass over this PR. 7 comment(s).
| this.ReportErrorAndExit( | ||
| tracer, | ||
| "Dehydrate --full is not supported while linked worktrees exist. Remove all linked worktrees first."); | ||
| } |
There was a problem hiding this comment.
This guard is the thing standing between a --full dehydrate and a worktree's data, but the signal it depends on fails open. GVFSEnlistment.GetKnownWorktreePaths (in GVFSEnlistment.Shared.cs) wraps the gitdir read in a bare catch { } and silently skips any entry it cannot read, and also skips entries with no gitdir file at all. So "I could not read the worktree registrations" is indistinguishable here from "there are no worktrees" — and the destructive path proceeds, with nothing traced.
Given CONTRIBUTING.md's fail-fast-on-data-loss guidance, could this fail closed instead? A TryGetKnownWorktreePaths(out string[] paths, out string error) that returns false when an entry is unreadable would let this site refuse rather than guess, and would also give the user something to act on.
| repoUrl: "https://mock/repo"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new unit tests are well constructed, but everything they assert is built from mocks — nothing in the suite exercises these call sites against an enlistment produced by a real git worktree add. WorktreeTests.cs does not cover dehydrate or index regeneration, and the whole DehydrateTests fixture is [SkipInCI("Atrophied: ...")].
The failure mode this PR prevents — a worktree operation writing through to the primary enlistment's index — is precisely the kind that mocks cannot catch, because it depends on what git actually puts in .git/worktrees/<name>/.
Would you add one functional test that creates a worktree, runs the dehydrate path, and asserts the primary enlistment's src\.git\index is byte-identical before and after? That pins the behavior the PR is about.
| this.ReportErrorAndExit( | ||
| tracer, | ||
| "Dehydrate --full is not supported while linked worktrees exist. Remove all linked worktrees first."); | ||
| } |
There was a problem hiding this comment.
The rejection message is a dead end in the stale-registration case. GetKnownWorktreePaths never checks whether the worktree directory still exists, so a user who deleted a worktree folder by hand is told to "remove all linked worktrees" when there is nothing left to remove, and --full stays blocked with no way forward.
Suggest listing the paths that were resolved and pointing at git worktree remove and git worktree prune — the latter is the actual fix for the stale case.
| { | ||
| errorMessage = "Failed to backup .git lock files."; | ||
| } | ||
|
|
There was a problem hiding this comment.
TryBackupFilesInFolder / TryCopyFilesInFolder produce a precise per-file error, which is assigned to a local that is never read, and then errorMessage is overwritten with the generic "Failed to backup .git lock files."
That is the message the user sees partway through a dehydrate, with no indication of which file or why. Suggest plumbing the real error out — out string errorMessage through to the caller, or at minimum appending it to the generic text.
| // commands to lock out. | ||
| this.indexLockPath = Path.Combine(enlistment.DotGitRoot, GVFSConstants.DotGit.IndexName + ".lock2"); | ||
| this.indexPath = enlistment.GitIndexPath; | ||
| this.indexLockPath = this.indexPath + ".lock2"; |
There was a problem hiding this comment.
Now that the index destination is resolved dynamically rather than being a fixed DotGitRoot-relative path, ReplaceExistingIndex deletes and replaces a file whose path appears nowhere in the trace. If the resolution is ever wrong, the symptom is a corrupted index with no record of which one we wrote.
Adding this.indexPath (and the IsWorktree flag) as metadata on the CreateFromHeadTree activity would make that visible. CheckoutPrefetcher.MoveUpdatedIndexToFinalLocation already traces its destination at Informational, so this would match existing practice.
| "dehydrate_backup", | ||
| backupSubfolder, | ||
| timestamp.ToString("yyyyMMdd_HHmmss_fffffff"))); | ||
| } |
There was a problem hiding this comment.
Two user-visible changes here that the description does not call out:
-
The backup folder timestamp gains sub-second precision (
yyyyMMdd_HHmmss→yyyyMMdd_HHmmss_fffffff), so the folder names users see and any tooling that parses them changes shape. Worth a line in the description. Note also thatBackupRootIsUniqueAndWorktreeLocalpasses a fixed timestamp, so the uniqueness the added precision buys is never actually asserted. -
For worktrees the backup root lands in
Path.GetDirectoryName(WorkingDirectoryBackingRoot)— a directory GVFS does not own, next to the user's worktree. That is a reasonable call, but it is now a third convention for where worktree-adjacent state lives, alongside what Pin and document GitProcess git-dir resolution in linked worktrees #2102 and Store worktree mount and enlistment ids outside the shared git config #2129 do. Might be worth settling on one across the three.
| Directory.CreateDirectory(worktreePath); | ||
| Directory.CreateDirectory(worktreeGitDir); | ||
|
|
||
| GVFSEnlistment.WorktreeInfo worktreeInfo = new GVFSEnlistment.WorktreeInfo |
There was a problem hiding this comment.
"https://mock/repo" has a real scheme and host shape. CONTRIBUTING.md asks for mock:// so that anything not properly mocked fails loudly instead of attempting a real request — GVFSEnlistmentTests.cs:35 has the form.
Block full dehydrate when worktree registration reads fail. List the registered paths and explain stale-registration cleanup. Preserve per-file backup errors and trace the resolved index destination. Add a real-worktree dehydrate test and unit coverage for guard failures, trace metadata, and sub-second backup paths. Assisted-by: GPT-6.1 Sol Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Keep backups outside worktrees when paths include a trailing separator. Resolve relative worktree registrations and retain full exceptions in diagnostic logs. Report both index backup paths and return a failed dehydrate exit code after backup errors. Add CLI tests for full-dehydrate guards, backup failures, and trailing separators. Assisted-by: GPT-6.1 Sol Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Match Git by ignoring directories that contain no worktree registration. Continue to block full dehydrate when a present gitdir file is unreadable or malformed. Cover empty metadata entries alone and alongside a valid worktree registration. Assisted-by: GPT-6.1 Sol Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Skip missing, zero-byte, and unreadable gitdir files. Record skipped read failures in diagnostic logs instead of blocking full dehydrate on entries Git does not list. Keep nonempty malformed registrations and enumeration errors blocking. Test whitespace-only content and mixed readable and unreadable registrations. Assisted-by: GPT-6.1 Sol Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
2 participants
Problem and Context
Linked worktrees store their index in a per-worktree Git directory.
DotGitRootpoints to shared Git state instead.Several index callers still used
DotGitRoot. They could read, replace, repair, or back up the primary index from a linked worktree.Full dehydrate also assumes that
.gitis a directory. It cannot safely run from a worktree or while linked worktrees exist.Worktree backups used the primary enlistment root. Separate worktrees could collide or cross storage volumes.
Changes
GitIndexPathfor repair, FastFetch, index generation (used by repair and FastFetch), and dehydrate backups.