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) }