Repository navigation
Pin and document GitProcess git-dir resolution in linked worktrees - #2102
Conversation
d0c3c11 to
820d9db
Compare
b77d49a to
87856a4
Compare
87856a4 to
ec7b8ae
Compare
Marli Remash (mashmouse)
left a comment
There was a problem hiding this comment.
Left some comments to bulk up my understanding.
ec7b8ae to
6316857
Compare
Nathan Baird (ShiningMassXAcc)
left a comment
There was a problem hiding this comment.
Review pass over this PR. 5 comment(s).
These findings concern code the diff does not touch directly, so GitHub cannot anchor them inline:
GVFS/GVFS.FunctionalTests/Tests/EnlistmentPerFixture/WorktreeTests.cs (around line 430) - MEDIUM
The behavior motivating this PR — that a command run in a linked worktree reports that worktree's HEAD and branch rather than the primary enlistment's — is not asserted anywhere. The existing tests cover mount liveness, git status --porcelain, projected file visibility and the per-worktree placeholder database, all of which would still pass if git-dir resolution regressed.
A single assertion that rev-parse HEAD (or rev-parse --abbrev-ref HEAD) in the worktree differs from the primary enlistment's would close that gap directly.
|
Note 🤖 Machine-drafted reply, reviewed and approved by Tyrie Vella (@tyrielv) before posting. The functional harness invokes git directly rather than through GitProcess, so a rev-parse assertion there would not catch a regression in the --git-dir that GitProcess passes. The new unit tests pin that wiring instead. |
In a linked git worktree, Enlistment.DotGitRoot and the git directory that
GitProcess passes as --git-dir point at different locations:
Enlistment.DotGitRoot -> the shared .git directory of the main repo
GitProcess git dir -> "<worktree>\.git", a file holding
"gitdir: <shared>/.git/worktrees/<name>"
The two values look inconsistent, but each is correct for its purpose. Git
follows the gitdir: pointer, so the worktree .git file resolves the
per-worktree state (HEAD, index, per-worktree refs, reflog) and, through
commondir, the shared state (config, objects, packed-refs). DotGitRoot is used
for shared state that GVFS reads or writes directly on disk (objects,
objects/info/alternates, hooks).
Making GitProcess use DotGitRoot would be a regression. Every command would
bind to the main worktree. From inside a linked worktree, "rev-parse HEAD" and
"name-rev --name-only HEAD" would report the main worktree's branch and commit.
No behavior changes. This adds comments on both sides of the divergence, an
internal GitDirPath accessor, and tests that pin the current values. The tests
cover both linked-worktree and regular enlistment behavior, plus the null
working-directory constructor case.
Assisted-by: Claude Opus 4.5
Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Correct the GitProcess comment: only HEAD, index, ORIG_HEAD, logs/HEAD, refs/bisect and refs/worktree are per-worktree. config, objects, packed-refs and refs/heads stay in the shared git directory. Reword it so it no longer reads as following the gitdir: target. Document Enlistment.DotGitRoot as a contract that subclasses supporting linked worktrees must satisfy. The base constructor sets it to a path that is a file in a linked worktree. Replace the tests that re-derived the path formula with tests that assert the --git-dir value that reaches InvokeGitImpl. MockGitProcess now records that value and has constructors for a specific enlistment and a bare working directory root. Remove the internal GitDirPath accessor, which only the old tests used. Assisted-by: Claude Opus 4.5 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
71f2cb2 to
5acdc4d
Compare
Nathan Baird (ShiningMassXAcc)
left a comment
There was a problem hiding this comment.
Re-review of the second commit. All four of my earlier threads are addressed, and the pinning change itself looks right — thank you. One correction to the new comment plus three nits below; none of them block.
CI is green across every leg.
Git resolves files in the per-worktree git directory by default and redirects only an explicit shared set through commondir. The GitProcess comment listed a few per-worktree files and described everything else as shared. That is wrong: MERGE_HEAD, CHERRY_PICK_HEAD, BISECT_START, FETCH_HEAD, rebase-merge/ and refs/rewritten are also per-worktree. The two earlier commit messages carry the same wording. Document that a GitProcess built from an Enlistment uses WorkingDirectoryBackingRoot, the root that Enlistment uses to derive DotGitRoot. Document that subclasses set DotGitRoot during construction, since it is a non-virtual property with a protected setter and cannot be overridden. Restore the final CRLF line ending of WorktreeEnlistmentTests.cs, which an earlier edit changed to LF. Assisted-by: Claude Opus 4.5 Signed-off-by: Tyrie Vella <tyrielv@gmail.com>
Problem and Context
Enlistment.DotGitRootand the path thatGitProcesspasses to--git-dirlook like duplicate state.In a linked worktree, they must point to different paths.
Enlistment.DotGitRootpoints to the shared git directory.GVFS uses that path when it reads or writes shared repository state directly.
Examples include objects,
objects/info/alternates, and hooks.GitProcessmust pass the worktree.gitfile to--git-dir.This is unintuitive, because that path is a file with a
gitdir:pointer.Git accepts the file path and resolves the pointer itself.
GitProcessmust not pre-resolve thegitdir:target.Given the
.gitfile, Git resolves files in the per-worktree git directory by default.Examples are
HEAD,index,ORIG_HEAD,logs/HEAD,MERGE_HEAD,FETCH_HEAD,rebase-merge/,refs/bisect, andrefs/worktree.Only an explicit shared set goes through
commondirto the shared git directory: config, objects,packed-refs,refs/heads, and hooks.If
GitProcessusedEnlistment.DotGitRoot, linked worktree commands would bind to the main worktree.For example,
rev-parse HEADandname-rev --name-only HEADwould report the main worktree.Changes
Enlistment.DotGitRootis shared repository metadata that subclasses supporting linked worktrees set during construction.GitProcessfield togitDirPath.GitProcesspasses the worktree.gitfile to--git-dir.MockGitProcessrecord the--git-dirvalue that reachesInvokeGitImpl, and add constructors for a specific enlistment and for a bare working directory root..gitfile, notDotGitRootor thegitdir:target.--git-dirthat matchesDotGitRoot.--git-diris passed when the working directory root is null.No production behavior changes.