refactor(session): use canonical Git metadata resolution (split part 3) - #2285
Open
MuskanPaliwal wants to merge 2 commits into
Open
refactor(session): use canonical Git metadata resolution (split part 3)#2285MuskanPaliwal wants to merge 2 commits into
MuskanPaliwal wants to merge 2 commits into
Conversation
Resolve session store directories, adoption worktree IDs, and sweep metadata through gitrepo.ResolveWorktreeMetadata. Preserve rooted storage, Git repository validation, adoption fallback, and sweep throttling. Keep legacy metadata APIs and caches for consumers that have not migrated. Cover metadata repair, linked-worktree storage, repository override isolation, adoption validation, and sweep failure policy. Trace2 verifies that known-root construction no longer launches a Git metadata subprocess. Entire-Checkpoint: 01M1SKZWY3DCRM6WE1YZAV00ET
MuskanPaliwal
marked this pull request as ready for review
September 6, 2026 08:04
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part 3 of #2190's agreed split, following the merged canonical resolver in #2241 and checkpoint migration in #2254.
Session construction, adoption, and sweep still use separate paths to resolve Git metadata. This PR moves those consumers to
gitrepo.ResolveWorktreeMetadata, so the canonical resolver supplies the common directory and worktree ID while each caller keeps its own storage and failure policy.Session-store construction
Previously, constructing a session store ran
git rev-parse --git-common-direven when the caller already knew the worktree root. The CWD constructor also used a cached common-directory result, which could hide metadata damage after an earlier successful lookup.NewStateStorenow discovers the root throughpaths.WorktreeRootand delegates toNewStateStoreForWorktree. The explicit-root constructor resolvesWorktreeMetadata.CommonDirdirectly. Known-root construction therefore starts no Git subprocesses, and each construction checks the current metadata without adding another cache.For example, if
commondiris changed to point at a missing directory after a successful construction, the next construction fails. Repairing the metadata makes construction succeed again and preserves access to the saved session. InheritedGIT_DIR,GIT_COMMON_DIR, andGIT_WORK_TREEvalues also cannot redirect an explicitly scoped store into another repository.The common directory still feeds the existing rooted store. Linked worktrees share session files, and empty-root rejection, test isolation, no-follow I/O, atomic writes, and locking remain in place.
Adoption and sweep
Adoption uses the canonical resolver for source and target worktree IDs. This removes its dependency on the legacy parser's expected directory names. A linked worktree backed by a bare repository named
storage, for example, can now resolve its ID fromstorage/worktrees/<id>.The source and target retain different failure policies. If source metadata cannot resolve, adoption can still match the session by its recorded worktree path. If target metadata cannot resolve, adoption stops before saving the adopted state. The combined Git query in
stateStoreForWorktreestays because it validates an arbitrary user-supplied repository; a directory that merely contains an empty.gitfolder must still be rejected. Source-retirement rollback remains unchanged.Sweep now resolves metadata from the worktree root it has already discovered. If that fails, it skips spawning. On success, it uses the common directory for the existing repository-wide throttle, so session starts in different worktrees continue to share the same throttle marker.
Why the legacy APIs and cache remain
session.GetGitCommonDir, the cache-clearing API, andgitdir's common-directory resolver/cache still serve strategy and other consumers outside this slice. Removing them now would require migrating those callers too. Their deletion waits until those callers have migrated and been verified.This PR is limited to three production files and their tests. The ownership guard remains active. Rooted-handle lifetime and the strategy/session-lock migration stay with their existing owners.
Verification
mise run lint: passed with zero issues.mise run test: passed, 10,670 tests reported with 8 skipped.mise run test:integration: passed on a standalone rerun, 562 tests reported with 3 skipped. The initial run alongside the unit suite timed out waiting for the external-command SIGINT test plugin to start.git diff upstream/main...HEAD --check: passed.Focused tests cover linked-worktree save/load/clear, invalid roots, metadata damage and repair, inherited Git variables, adoption validation and source-path fallback, bare-backed worktree IDs, and sweep failure/throttle behavior. Existing tests exercise rooted storage, symlink refusal, and adoption rollback.
Selected new tests were also run against the old production code through a temporary Go overlay. They failed for the expected cached-metadata, repository-override, worktree-ID, and subprocess-count differences, and passed on this branch.
Real Git Trace2 events show constructor subprocess counts:
The Trace2 test includes a positive control that launches Git and checks that an additional event is recorded. These measurements cover store construction, not total hook latency.