From 99ef433efdc939bde453ec88b32a3261d71a6875 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Mon, 14 Sep 2026 15:29:06 -0700 Subject: [PATCH] fix(worktrees): follow gitdir/commondir markers in disk witness The disk witness validates created worktrees by reading the repo's common directory from disk. Previously it only checked for a direct .git directory and returned a status object that conflated different failure modes. Now it properly follows .gitdir and commondir pointer files to locate the true common directory, fixing detection on repos with linked git directories (worktrees, submodules) and WSL scenarios. Error handling is simplified: definitive absence returns undefined, other read failures throw with proper cause chains, eliminating the ambiguous "unverifiable" state that would mask real errors. --- .../git/worktree-created-disk-witness.test.ts | 48 +++++++---- ...tree-listing-created-sparse-distro.test.ts | 23 +++++ src/main/git/worktree-listing.ts | 84 ++++++++++--------- .../created-worktree-reconciliation.test.ts | 21 ++++- .../ipc/created-worktree-reconciliation.ts | 39 ++++----- 5 files changed, 133 insertions(+), 82 deletions(-) diff --git a/src/main/git/worktree-created-disk-witness.test.ts b/src/main/git/worktree-created-disk-witness.test.ts index ff38e05e88b..d6263904905 100644 --- a/src/main/git/worktree-created-disk-witness.test.ts +++ b/src/main/git/worktree-created-disk-witness.test.ts @@ -1,6 +1,3 @@ -// The disk witness is only read after Git disagreed about the common dir, so what it answers decides -// whether a worktree Git already wrote is reported as created or abandoned (#16520). A read that -// failed must not be spent as a disagreement. import { chmodSync, mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' @@ -31,6 +28,9 @@ const readRepoCommonDirFromGitMock = vi.mocked(readRepoCommonDirFromGit) const readCheckedOutBranchRefMock = vi.mocked(readCheckedOutBranchRef) const readWorktreeHeadOidMock = vi.mocked(readWorktreeHeadOid) +/** Repo convention: root bypasses the mode bits, so `chmod 000` denies nothing there. */ +const CAN_DENY_READ = process.platform !== 'win32' && process.getuid?.() !== 0 + let scratchDir = '' let repoPath = '' let worktreePath = '' @@ -68,7 +68,7 @@ describe('describeCreatedWorktree when Git and the repo disagree', () => { }) it('reports nothing for a bare repo, whose missing .git is a real answer', async () => { - // No `.git` at all -> ENOENT -> `{ status: 'read', commonDir: undefined }`, not unverifiable. + // No `.git` at all is definitive absence, not an unreadable witness. await expect( describeCreatedWorktree(repoPath, worktreePath, 'feature') ).resolves.toBeUndefined() @@ -83,16 +83,30 @@ describe('describeCreatedWorktree when Git and the repo disagree', () => { ).resolves.toBeUndefined() }) - it.runIf(process.platform !== 'win32')( - 'throws rather than claiming a mismatch when the witness cannot be read', - async () => { - writeFileSync(join(repoPath, '.git'), 'gitdir: /somewhere\n') - chmodSync(repoPath, 0o000) - await expect(describeCreatedWorktree(repoPath, worktreePath, 'feature')).rejects.toThrow( - /^repo common dir unverifiable: could not read .*\.git: / - ) - } - ) + it('follows gitdir and commondir markers', async () => { + const commonDir = join(scratchDir, 'main', '.git') + const linkedGitDir = join(commonDir, 'worktrees', 'source') + mkdirSync(linkedGitDir, { recursive: true }) + writeFileSync(join(repoPath, '.git'), `gitdir: ${linkedGitDir}\n`) + writeFileSync(join(linkedGitDir, 'commondir'), '../..\n') + readRepoLocationMock.mockResolvedValue({ topLevel: worktreePath, commonDir }) + + await expect(describeCreatedWorktree(repoPath, worktreePath, 'feature')).resolves.toMatchObject( + { + branch: 'refs/heads/feature' + } + ) + }) + + it.skipIf(!CAN_DENY_READ)('throws when the .git marker exists but cannot be read', async () => { + const dotGit = join(repoPath, '.git') + writeFileSync(dotGit, 'gitdir: /somewhere\n') + chmodSync(dotGit, 0o000) + await expect(describeCreatedWorktree(repoPath, worktreePath, 'feature')).rejects.toMatchObject({ + message: expect.stringMatching(/^repo common dir unverifiable: could not read .*\.git: /), + cause: expect.objectContaining({ code: 'EACCES' }) + }) + }) // The other unverifiable branch -- the deadline firing on a `.git` that never answers -- needs a // read that really blocks, so it lives in worktree-created-description-real-git.test.ts behind a @@ -120,7 +134,7 @@ describe('describeCreatedWorktree before the witness is reached', () => { readRepoLocationMock.mockResolvedValue({ topLevel: worktreePath, commonDir }) readRepoCommonDirFromGitMock.mockResolvedValue(commonDir) // chmod 000 would make the witness unverifiable; agreement means it is never opened. - if (process.platform !== 'win32') { + if (CAN_DENY_READ) { chmodSync(repoPath, 0o000) } await expect(describeCreatedWorktree(repoPath, worktreePath, 'feature')).resolves.toMatchObject( @@ -134,7 +148,7 @@ describe('describeCreatedWorktree before the witness is reached', () => { readRepoLocationMock.mockResolvedValue(undefined) // An unconfirmed worktree is not an unverifiable common dir: resolving undefined under a repo // whose witness cannot be read is how we know the witness was never consulted. - if (process.platform !== 'win32') { + if (CAN_DENY_READ) { chmodSync(repoPath, 0o000) } await expect( @@ -144,7 +158,7 @@ describe('describeCreatedWorktree before the witness is reached', () => { it('reports nothing when the worktree has the wrong branch checked out', async () => { readCheckedOutBranchRefMock.mockResolvedValue('refs/heads/other') - if (process.platform !== 'win32') { + if (CAN_DENY_READ) { chmodSync(repoPath, 0o000) } await expect( diff --git a/src/main/git/worktree-listing-created-sparse-distro.test.ts b/src/main/git/worktree-listing-created-sparse-distro.test.ts index 2b3922d5b9b..ab0326dcee2 100644 --- a/src/main/git/worktree-listing-created-sparse-distro.test.ts +++ b/src/main/git/worktree-listing-created-sparse-distro.test.ts @@ -102,4 +102,27 @@ describe('describeCreatedWorktree on a drvfs-spelled WSL worktree', () => { platformSpy.mockRestore() } }) + + it('uses the repo disk witness in the WSL execution namespace', async () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + readRepoCommonDirFromGitMock.mockResolvedValue('/other/.git') + statMock.mockImplementation(async (target: string) => { + const value = slashed(target) + if (value === `${slashed(REPO)}/.git`) { + return { isDirectory: () => true } + } + if (value === `${HOST_GIT_DIR}/info/sparse-checkout`) { + return { isFile: () => true, size: 12 } + } + throw missing() + }) + + try { + await expect( + describeCreatedWorktree(REPO, 'C:\\wt\\x', 'feature', { wslDistro: 'Ubuntu' }) + ).resolves.toMatchObject({ branch: 'refs/heads/feature' }) + } finally { + platformSpy.mockRestore() + } + }) }) diff --git a/src/main/git/worktree-listing.ts b/src/main/git/worktree-listing.ts index 3fb557fec4c..93afbcc76e1 100644 --- a/src/main/git/worktree-listing.ts +++ b/src/main/git/worktree-listing.ts @@ -1,6 +1,8 @@ -import { realpath, stat } from 'node:fs/promises' +import { readFile, realpath, stat } from 'node:fs/promises' import { join, posix } from 'node:path' import { isDefinitiveAbsence } from '../../shared/definitive-filesystem-absence' +import { resolveGitMetadataPath } from '../../shared/git-metadata-path' +import { parseGitdirMarkerPayload } from '../../shared/gitdir-marker-payload' import { isWorktreeCreatePreparation } from '../../shared/worktree/create-preparation' import { toWslExecutionSpace } from '../../shared/wsl-paths' import type { GitWorktreeInfo } from '../../shared/worktree/types' @@ -21,8 +23,6 @@ import { } from './worktree-operation-options' import { areWorktreePathsEqual, translateWorktreePath } from './worktree-path-comparison' import { detectSparseCheckoutCached } from './worktree-sparse-checkout-cache' -import { resolveGitCommonDir } from './worktree-sparse-state' -import { resolveGitDir } from './source-control/resolve-git-dir' const SPARSE_CHECKOUT_DETECTION_CONCURRENCY = 8 @@ -153,44 +153,56 @@ export async function annotateSparseCheckoutStatus( * Deadlined because a `.git` on a hung mount (dead NFS/SSHFS, stalled WSL 9p) never rejects, and an * unbounded read here would leave the whole create IPC pending instead of failing like it used to. * - * Three outcomes, not two: a missing `.git` is a real "no candidate", but a deadline or a permission - * error is `unverifiable`, and the caller must not spend it as a disagreement. This witness is only - * read after Git already disagreed, so a failed read here decides whether a worktree that exists on - * disk is reported as created or abandoned (#16520). + * A missing `.git` is a real "no candidate"; every other read failure is unverifiable and rejects. */ -type RepoDiskCommonDir = - | { status: 'read'; commonDir: string | undefined } - | { status: 'unverifiable'; reason: string } - async function readRepoCommonDirFromDisk( repoPath: string, timeoutMs: number -): Promise { +): Promise { const dotGit = join(repoPath, '.git') try { - await withDeadline(stat(dotGit), timeoutMs) + const commonDir = await withDeadline(resolveRepoCommonDirFromDisk(repoPath, dotGit), timeoutMs) + return commonDir ? toWslExecutionSpace(commonDir) : undefined } catch (error) { - // A bare repo has no `.git`, and resolveGitDir would fabricate one; offer no candidate instead. - return isDefinitiveAbsence(error) - ? { status: 'read', commonDir: undefined } - : { status: 'unverifiable', reason: describeWitnessFailure(dotGit, error) } - } - try { - const commonDir = await withDeadline( - resolveGitDir(repoPath).then(resolveGitCommonDir), - timeoutMs - ) - // Node answers in the caller's space, Git in the distro's. Without this the WSL candidate is a UNC - // path that can never equal Git's `/home/...`, leaving this witness inert on exactly the fallback - // path that needs it (realpath cannot bridge the two: a Linux path has no local inode). - return { status: 'read', commonDir: toWslExecutionSpace(commonDir) } - } catch (error) { - return { status: 'unverifiable', reason: describeWitnessFailure(dotGit, error) } + // A bare repo has no `.git`; do not fabricate a candidate for it. + if (isDefinitiveAbsence(error)) { + return undefined + } + const reason = error instanceof Error ? error.message : String(error) + throw new Error(`repo common dir unverifiable: could not read ${dotGit}: ${reason}`, { + cause: error + }) } } -function describeWitnessFailure(path: string, error: unknown): string { - return `could not read ${path}: ${error instanceof Error ? error.message : String(error)}` +async function resolveRepoCommonDirFromDisk( + repoPath: string, + dotGit: string +): Promise { + // The general metadata resolvers are intentionally best effort; a witness must preserve read failures. + const dotGitStats = await stat(dotGit) + let gitDir = dotGit + if (!dotGitStats.isDirectory()) { + const pointer = parseGitdirMarkerPayload(await readFile(dotGit, 'utf8')) + if (!pointer) { + return undefined + } + gitDir = resolveGitMetadataPath(repoPath, pointer) ?? dotGit + } + + return readCommonDirMarker(gitDir) +} + +async function readCommonDirMarker(gitDir: string): Promise { + try { + const pointer = await readFile(join(gitDir, 'commondir'), 'utf8') + return resolveGitMetadataPath(gitDir, pointer) ?? gitDir + } catch (error) { + if (!isDefinitiveAbsence(error)) { + throw error + } + return gitDir + } } async function withDeadline(work: Promise, timeoutMs: number): Promise { @@ -288,17 +300,11 @@ export async function describeCreatedWorktree( if (!(await isSameRepoCommonDir(created.commonDir, [repoGitCommonDir]))) { // Only now read the second opinion from disk: a `.git` on a hung mount pins a threadpool thread // that no deadline can reclaim, so never pay that on the path where Git already agreed. - const diskWitness = await readRepoCommonDirFromDisk( + const repoDiskCommonDir = await readRepoCommonDirFromDisk( repoPath, deadlined.timeout ?? WORKTREE_LIST_TIMEOUT_MS ) - // Throwing rather than returning undefined on purpose: the caller folds a thrown reason into - // its error, while undefined becomes a bare "created worktree not found", which claims Git - // placed the worktree somewhere else. A stalled mount proves no such thing. - if (diskWitness.status === 'unverifiable') { - throw new Error(`repo common dir unverifiable: ${diskWitness.reason}`) - } - if (!(await isSameRepoCommonDir(created.commonDir, [diskWitness.commonDir]))) { + if (!(await isSameRepoCommonDir(created.commonDir, [repoDiskCommonDir]))) { return undefined } } diff --git a/src/main/ipc/created-worktree-reconciliation.test.ts b/src/main/ipc/created-worktree-reconciliation.test.ts index c95e1245c8a..b62088ef105 100644 --- a/src/main/ipc/created-worktree-reconciliation.test.ts +++ b/src/main/ipc/created-worktree-reconciliation.test.ts @@ -139,6 +139,14 @@ describe('resolveCreatedWorktree', () => { ) }) + it('does not mistake a falsy rejection for a successful listing', async () => { + vi.mocked(listWorktreesSharedStrict).mockRejectedValue(undefined) + + await expect(resolveCreatedWorktree('/repo', '/workspaces/feature', 'feature')).rejects.toThrow( + 'undefined' + ) + }) + it('keeps the listing failure when the direct read itself throws', async () => { const failure = new Error('fatal: not a git repository') const recoveryFailure = new Error('repo common dir unverifiable: deadline exceeded') @@ -166,11 +174,16 @@ describe('resolveCreatedWorktree', () => { it("adds the direct read's failure when the listing merely omitted the row", async () => { vi.mocked(listWorktreesSharedStrict).mockResolvedValue([MAIN]) - vi.mocked(describeCreatedWorktree).mockRejectedValue(new Error('rev-parse exploded')) + const recoveryFailure = new Error('rev-parse exploded') + vi.mocked(describeCreatedWorktree).mockRejectedValue(recoveryFailure) - await expect(resolveCreatedWorktree('/repo', '/workspaces/feature', 'feature')).rejects.toThrow( - 'Worktree created but not found in listing: /workspaces/feature (branch feature): rev-parse exploded' - ) + await expect( + resolveCreatedWorktree('/repo', '/workspaces/feature', 'feature') + ).rejects.toMatchObject({ + message: + 'Worktree created but not found in listing: /workspaces/feature (branch feature): rev-parse exploded', + cause: recoveryFailure + }) }) it('charges the recovery what the listing left of the budget, not a fresh one', async () => { diff --git a/src/main/ipc/created-worktree-reconciliation.ts b/src/main/ipc/created-worktree-reconciliation.ts index 3f1ecccd854..ac5b79954c2 100644 --- a/src/main/ipc/created-worktree-reconciliation.ts +++ b/src/main/ipc/created-worktree-reconciliation.ts @@ -53,7 +53,7 @@ export async function resolveCreatedWorktree( options?: GitWorktreeExecOptions ): Promise { const startedAt = Date.now() - let listingError: unknown + let listingError: Error | undefined try { const worktrees = options ? await listWorktreesSharedStrict(repoPath, options) @@ -63,11 +63,9 @@ export async function resolveCreatedWorktree( return { created, worktrees, listingComplete: true } } } catch (err) { - listingError = err + listingError = err instanceof Error ? err : new Error(String(err)) } - let described: GitWorktreeInfo | undefined - let describeError: unknown try { // One budget for verifying the create, not one per attempt: a hung Git already spent the // listing's deadline, and charging the recovery a fresh one doubles the wait before the error. @@ -75,34 +73,31 @@ export async function resolveCreatedWorktree( WORKTREE_LIST_TIMEOUT_MS - (Date.now() - startedAt), MIN_CREATED_WORKTREE_RECOVERY_MS ) - described = await describeCreatedWorktree(repoPath, worktreePath, branchName, { + const described = await describeCreatedWorktree(repoPath, worktreePath, branchName, { ...options, timeout: options?.timeout ?? remainingMs }) + if (described) { + return { created: described, worktrees: [], listingComplete: false } + } } catch (err) { - // Why keep, not rethrow: the recovery must not replace the listing's own, more informative failure. - describeError = err - } - if (described) { - return { created: described, worktrees: [], listingComplete: false } - } - if (listingError) { - if (describeError) { + if (listingError) { // The listing's failure stays the thrown one, but the recovery's reason -- often // `repo common dir unverifiable: ...` -- would otherwise vanish from the record entirely. console.warn('[worktrees:create] created-worktree recovery also failed', { - err: describeError, + err, worktreePath }) + throw listingError } + // The listing simply omitted the row, so the direct read holds the only actionable failure. + const notFound = createdWorktreeNotFoundError(worktreePath, branchName) + throw new Error(`${notFound.message}: ${err instanceof Error ? err.message : String(err)}`, { + cause: err + }) + } + if (listingError) { throw listingError } - const notFound = createdWorktreeNotFoundError(worktreePath, branchName) - if (describeError) { - // The listing simply omitted the row, so the direct read holds the only actionable failure. - throw new Error( - `${notFound.message}: ${describeError instanceof Error ? describeError.message : String(describeError)}` - ) - } - throw notFound + throw createdWorktreeNotFoundError(worktreePath, branchName) }