From 98d94beff5e28d77f216549cd1122deff0a46b9e Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 21 Aug 2026 23:03:25 -0700 Subject: [PATCH] fix(worktree): collapse path-equal worktree rows without losing lineage Rework the fork's dedupe so it cannot destroy data or rotate ids: - Collapse path-equal rows in place instead of sorting branch-bearing rows first, so git's ordering (and every downstream slice/[0]) is preserved. - Keep the canonical spelling (repo path > spelling that owns metadata > git's first row) so `::` ids, metadata and comments stay stable; the survivor adopts the branch/head/flags of the peers it absorbs. - Prune lineage against the raw scan rows, and match stored ids by path comparison key, so a dropped spelling is never read as a deleted worktree. - Compare remote-host rows with plain POSIX rules rather than the desktop's, so a macOS desktop no longer folds /private/tmp into /tmp for SSH rows. POSIX paths stay case-sensitive on every platform, including macOS: a case-sensitive APFS/HFSX volume can hold both spellings, and dropping a real row is worse than showing a duplicate. Windows keeps its case-insensitive rules. Co-authored-by: Wooseong Kim --- .../path-equal-worktree-row-collapse.test.ts | 113 +++++++++++++++ .../path-equal-worktree-row-collapse.ts | 86 +++++++++++ .../repo-worktree-row-resolution.test.ts | 135 +++++++++++++++--- .../runtime/repo-worktree-row-resolution.ts | 20 +-- src/main/worktree-lineage-pruning.test.ts | 29 ++++ src/main/worktree-lineage-pruning.ts | 27 +++- 6 files changed, 379 insertions(+), 31 deletions(-) create mode 100644 src/main/runtime/path-equal-worktree-row-collapse.test.ts create mode 100644 src/main/runtime/path-equal-worktree-row-collapse.ts diff --git a/src/main/runtime/path-equal-worktree-row-collapse.test.ts b/src/main/runtime/path-equal-worktree-row-collapse.test.ts new file mode 100644 index 00000000000..ef491118ac3 --- /dev/null +++ b/src/main/runtime/path-equal-worktree-row-collapse.test.ts @@ -0,0 +1,113 @@ +import { describe, expect, it } from 'vitest' +import type { Repo } from '../../shared/repo-types' +import type { GitWorktreeInfo } from '../../shared/worktree/types' +import { + collapsePathEqualWorktreeRows, + resolveRepoWorktreePathPlatform +} from './path-equal-worktree-row-collapse' + +function row(path: string, overrides: Partial = {}): GitWorktreeInfo { + return { + path, + head: 'abc123', + branch: 'refs/heads/feature', + isBare: false, + isMainWorktree: false, + ...overrides + } +} + +function repo(overrides: Partial = {}): Repo { + return { + id: 'repo', + path: '/home/me/repo', + displayName: 'repo', + badgeColor: 'blue', + addedAt: 1, + ...overrides + } +} + +describe('collapsePathEqualWorktreeRows', () => { + it.each(['darwin', 'linux'])( + 'keeps case-different POSIX paths apart on %s', + (platform) => { + const rows = [row('/home/me/Repo'), row('/home/me/repo')] + + expect( + collapsePathEqualWorktreeRows(rows, { repoPath: '/home/me/repo', platform }).map( + (entry) => entry.path + ) + ).toEqual(['/home/me/Repo', '/home/me/repo']) + } + ) + + it('collapses Windows slash styles and drive-letter casing', () => { + const rows = [row(String.raw`C:\Work\Feature`), row('c:/work/feature/')] + + expect( + collapsePathEqualWorktreeRows(rows, { + repoPath: String.raw`C:\Work`, + platform: 'win32' + }).map((entry) => entry.path) + ).toEqual([String.raw`C:\Work\Feature`]) + }) + + it('keeps /tmp and /private/tmp apart for rows produced on a non-darwin host', () => { + const rows = [row('/private/tmp/orca/feature'), row('/tmp/orca/feature')] + + expect( + collapsePathEqualWorktreeRows(rows, { repoPath: '/tmp/orca', platform: 'linux' }).map( + (entry) => entry.path + ) + ).toEqual(['/private/tmp/orca/feature', '/tmp/orca/feature']) + expect( + collapsePathEqualWorktreeRows(rows, { repoPath: '/tmp/orca', platform: 'darwin' }) + ).toHaveLength(1) + }) + + it('collapses in place and merges the peer branch, head and flags', () => { + const rows = [ + row('/home/me/repo/', { head: '', branch: '', isMainWorktree: false }), + row('/home/me/wt/feature', { branch: 'refs/heads/feature' }), + row('/home/me/repo', { branch: 'refs/heads/master', isMainWorktree: true, isSparse: true }) + ] + + const collapsed = collapsePathEqualWorktreeRows(rows, { + repoPath: '/home/me/repo', + platform: 'linux' + }) + + expect(collapsed).toHaveLength(2) + expect(collapsed[0]).toMatchObject({ + path: '/home/me/repo', + head: 'abc123', + branch: 'refs/heads/master', + isMainWorktree: true, + isSparse: true + }) + expect(collapsed[1].path).toBe('/home/me/wt/feature') + }) + + it('prefers a spelling that already owns metadata over git row order', () => { + const rows = [row('/home/me/wt/./feature'), row('/home/me/wt/feature')] + + expect( + collapsePathEqualWorktreeRows(rows, { + repoPath: '/home/me/repo', + hasStoredMeta: (path) => path === '/home/me/wt/feature', + platform: 'linux' + })[0].path + ).toBe('/home/me/wt/feature') + }) +}) + +describe('resolveRepoWorktreePathPlatform', () => { + it('treats remote-host rows as plain POSIX regardless of the desktop platform', () => { + expect(resolveRepoWorktreePathPlatform(repo({ connectionId: 'builder' }))).toBe('linux') + expect(resolveRepoWorktreePathPlatform(repo({ executionHostId: 'runtime:sandbox' }))).toBe( + 'linux' + ) + expect(resolveRepoWorktreePathPlatform(repo())).toBe(process.platform) + }) +}) diff --git a/src/main/runtime/path-equal-worktree-row-collapse.ts b/src/main/runtime/path-equal-worktree-row-collapse.ts new file mode 100644 index 00000000000..014ad8d13cc --- /dev/null +++ b/src/main/runtime/path-equal-worktree-row-collapse.ts @@ -0,0 +1,86 @@ +import { LOCAL_EXECUTION_HOST_ID, getRepoExecutionHostId } from '../../shared/execution-host' +import type { Repo } from '../../shared/repo-types' +import type { GitWorktreeInfo } from '../../shared/worktree/types' +import { worktreePathComparisonKey } from '../ipc/worktree-path-comparison' + +export type PathEqualWorktreeRowCollapseOptions = { + /** The repo's own spelling of its root; the preferred survivor so `::` ids stay stable. */ + repoPath: string + /** True when a spelling already owns persisted worktree metadata. */ + hasStoredMeta?: (worktreePath: string) => boolean + /** Platform of the host that produced the rows — not necessarily the desktop's. */ + platform?: NodeJS.Platform +} + +/** + * Which platform's path rules apply to a repo's scanned rows. + * + * Why: rows can come from an SSH or runtime host, and applying the desktop's macOS `/private/tmp` + * firmlink remap to a remote host would collapse two directories that are genuinely distinct there. + */ +export function resolveRepoWorktreePathPlatform(repo: Repo): NodeJS.Platform { + return getRepoExecutionHostId(repo) === LOCAL_EXECUTION_HOST_ID ? process.platform : 'linux' +} + +/** + * Collapse rows naming the same directory under different spellings (`.` or `..` segments, repeated + * or trailing separators, Windows slash style and drive-letter case) into a single row, in place, so + * Git's own ordering survives — downstream `slice(0, limit)` and `candidates[0]` callers depend on it. + * + * The survivor keeps the canonical spelling (the repo's own path, else a spelling that already has + * metadata, else Git's first row) so the derived worktree id stays stable, and adopts the branch, + * head and flags of the peers it absorbs. + * + * POSIX paths are never case-folded, including on macOS: APFS/HFSX can be formatted case-sensitive, + * and silently dropping a real worktree row is worse than showing a duplicate one. + */ +export function collapsePathEqualWorktreeRows( + rows: readonly GitWorktreeInfo[], + options: PathEqualWorktreeRowCollapseOptions +): GitWorktreeInfo[] { + const platform = options.platform ?? process.platform + const survivorIndexByPathKey = new Map() + const collapsed: GitWorktreeInfo[] = [] + for (const row of rows) { + const pathKey = worktreePathComparisonKey(row.path, platform) + const survivorIndex = survivorIndexByPathKey.get(pathKey) + if (survivorIndex === undefined) { + survivorIndexByPathKey.set(pathKey, collapsed.length) + collapsed.push(row) + continue + } + collapsed[survivorIndex] = mergePathEqualRows(collapsed[survivorIndex], row, options) + } + return collapsed +} + +function mergePathEqualRows( + survivor: GitWorktreeInfo, + duplicate: GitWorktreeInfo, + options: PathEqualWorktreeRowCollapseOptions +): GitWorktreeInfo { + return { + ...survivor, + path: pickCanonicalSpelling(survivor.path, duplicate.path, options), + // Why: the stored-metadata fallback row carries an empty head/branch, so a real Git row fills them. + head: survivor.head || duplicate.head, + branch: survivor.branch || duplicate.branch, + isMainWorktree: survivor.isMainWorktree || duplicate.isMainWorktree, + ...(survivor.isSparse || duplicate.isSparse ? { isSparse: true } : {}) + } +} + +function pickCanonicalSpelling( + survivorPath: string, + duplicatePath: string, + options: PathEqualWorktreeRowCollapseOptions +): string { + if (survivorPath === options.repoPath || duplicatePath === options.repoPath) { + return options.repoPath + } + const { hasStoredMeta } = options + if (!hasStoredMeta || hasStoredMeta(survivorPath) || !hasStoredMeta(duplicatePath)) { + return survivorPath + } + return duplicatePath +} diff --git a/src/main/runtime/repo-worktree-row-resolution.test.ts b/src/main/runtime/repo-worktree-row-resolution.test.ts index 0e2ee14e980..d2740f3c698 100644 --- a/src/main/runtime/repo-worktree-row-resolution.test.ts +++ b/src/main/runtime/repo-worktree-row-resolution.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi } from 'vitest' import type { ExecutionHostId } from '../../shared/execution-host' import type { Repo } from '../../shared/repo-types' +import type { WorktreeLineage } from '../../shared/worktree/lineage-types' import type { WorktreeMeta } from '../../shared/worktree/meta-types' import type { GitWorktreeInfo, Worktree } from '../../shared/worktree/types' import type { Store } from '../persistence' @@ -35,16 +36,35 @@ function gitWorktree(path: string): GitWorktreeInfo { } } -function createDeps(repos: Repo[]): RepoWorktreeRowDeps & { +/** + * Lineage removal is opt-in: a store without `removeWorktreeLineage` makes + * `pruneLineageForMissingRepoWorktrees` no-op, which is what the non-lineage cases here want. + */ +function createDeps( + repos: Repo[], + worktreeLineageById?: Record +): RepoWorktreeRowDeps & { metaById: Record + removeWorktreeLineage: ReturnType scanRepo: ReturnType> listFolderWorkspaces: ReturnType> } { const metaById: Record = {} + const removeWorktreeLineage = vi.fn((worktreeId: string) => { + delete worktreeLineageById?.[worktreeId] + }) const store = { getRepos: () => repos, getAllWorktreeMeta: () => metaById, - getAllWorktreeLineage: () => ({}), + getWorktreeMeta: (worktreeId: string) => metaById[worktreeId], + getAllWorktreeLineage: () => worktreeLineageById ?? {}, + ...(worktreeLineageById + ? { + getAllWorkspaceLineage: () => ({}), + removeWorktreeLineage, + removeWorkspaceLineage: vi.fn() + } + : {}), getProjects: () => [], getSettings: () => ({}), setWorktreeMeta: (worktreeId: string, updates: Partial) => { @@ -58,40 +78,117 @@ function createDeps(repos: Repo[]): RepoWorktreeRowDeps & { worktrees: [gitWorktree(owner.id === 'unrelated' ? '/unrelated/worktree' : '/same/worktree')] })) const listFolderWorkspaces = vi.fn(() => []) - return { store, metaById, scanRepo, listFolderWorkspaces } + return { store, metaById, removeWorktreeLineage, scanRepo, listFolderWorkspaces } +} + +function scanRow(path: string, overrides: Partial = {}): GitWorktreeInfo { + return { + path, + head: 'abc123', + branch: 'refs/heads/feature', + isBare: false, + isMainWorktree: false, + ...overrides + } } describe('path-equal worktree row collapse', () => { - it('keeps one row and prefers the git branch when the same path is listed twice', async () => { + it('collapses onto the repo path spelling and adopts the peer branch', async () => { const owner = repo('repo', '/home/me/repo') const deps = createDeps([owner]) deps.scanRepo.mockResolvedValueOnce({ ok: true, worktrees: [ - { - path: '/home/me/repo', - head: '', - branch: '', - isBare: false, - isMainWorktree: true - }, - { - path: '/home/me/./repo', - head: 'abc123', - branch: 'refs/heads/master', - isBare: false, - isMainWorktree: true - } + scanRow('/home/me/repo', { head: '', branch: '', isMainWorktree: true }), + scanRow('/home/me/./repo', { branch: 'refs/heads/master', isMainWorktree: true }) ] }) const rows = await resolveRepoWorktreeRows(deps, owner, deps.metaById, new Map()) + expect(rows).toHaveLength(1) expect(rows[0]).toMatchObject({ + id: 'repo::/home/me/repo', + path: '/home/me/repo', + head: 'abc123', branch: 'refs/heads/master', - path: '/home/me/./repo' + isMainWorktree: true }) }) + + it('keeps the spelling that already owns worktree metadata', async () => { + const owner = repo('repo', '/home/me/repo') + const deps = createDeps([owner]) + deps.metaById['repo::/home/me/wt/feature'] = { + instanceId: 'stable-instance', + comment: 'carried over' + } as WorktreeMeta + deps.scanRepo.mockResolvedValueOnce({ + ok: true, + worktrees: [scanRow('/home/me/wt/./feature'), scanRow('/home/me/wt/feature')] + }) + + const rows = await resolveRepoWorktreeRows(deps, owner, deps.metaById, new Map()) + + expect(rows).toHaveLength(1) + expect(rows[0]).toMatchObject({ + id: 'repo::/home/me/wt/feature', + path: '/home/me/wt/feature', + comment: 'carried over' + }) + expect(deps.metaById['repo::/home/me/wt/./feature']).toBeUndefined() + expect(deps.metaById['repo::/home/me/wt/feature'].instanceId).toBe('stable-instance') + }) + + it('never prunes lineage for a spelling the collapse dropped', async () => { + const owner = repo('repo', '/home/me/repo') + const childId = 'repo::/home/me/wt/./feature' + const worktreeLineageById: Record = {} + const deps = createDeps([owner], worktreeLineageById) + worktreeLineageById[childId] = { + worktreeId: childId, + worktreeInstanceId: 'child-instance', + parentWorktreeId: 'repo::/home/me/repo', + parentWorktreeInstanceId: 'parent-instance', + origin: 'manual', + capture: { source: 'manual-action', confidence: 'explicit' }, + createdAt: 1 + } + deps.scanRepo.mockResolvedValueOnce({ + ok: true, + worktrees: [ + scanRow('/home/me/repo', { isMainWorktree: true }), + scanRow('/home/me/wt/feature'), + scanRow('/home/me/wt/./feature') + ] + }) + + await resolveRepoWorktreeRows(deps, owner, deps.metaById, new Map()) + + expect(deps.removeWorktreeLineage).not.toHaveBeenCalled() + expect(worktreeLineageById[childId]).toBeDefined() + }) + + it('preserves git row order when a worktree has no branch', async () => { + const owner = repo('repo', '/home/me/repo') + const deps = createDeps([owner]) + deps.scanRepo.mockResolvedValueOnce({ + ok: true, + worktrees: [ + scanRow('/home/me/repo', { branch: '', isMainWorktree: true }), + scanRow('/home/me/wt/detached', { branch: '' }), + scanRow('/home/me/wt/feature') + ] + }) + + const rows = await resolveRepoWorktreeRows(deps, owner, deps.metaById, new Map()) + + expect(rows.map((row) => row.path)).toEqual([ + '/home/me/repo', + '/home/me/wt/detached', + '/home/me/wt/feature' + ]) + }) }) describe('host-qualified scoped worktree resolution', () => { diff --git a/src/main/runtime/repo-worktree-row-resolution.ts b/src/main/runtime/repo-worktree-row-resolution.ts index c64c5aff965..d3e5aec1045 100644 --- a/src/main/runtime/repo-worktree-row-resolution.ts +++ b/src/main/runtime/repo-worktree-row-resolution.ts @@ -10,10 +10,13 @@ import type { WorktreeMeta } from '../../shared/worktree/meta-types' import type { ProjectExecutionRuntimeResolution } from '../../shared/project-execution-runtime' import type { Store } from '../persistence' import { areWorktreePathsEqual, mergeWorktree } from '../ipc/worktree-logic' -import { dedupeWorktreesByPath } from '../ipc/worktree-path-comparison' import { pruneLineageForMissingRepoWorktrees } from '../worktree-lineage-pruning' import { getRepoOwnedWorktreeMeta } from '../worktree-metadata-ownership' import { resolveLocalProjectRuntimesForRepos } from '../project-runtime-git-options' +import { + collapsePathEqualWorktreeRows, + resolveRepoWorktreePathPlatform +} from './path-equal-worktree-row-collapse' import type { RuntimeWorktreeScanResult } from './repo-worktree-resolution-scan' /** @@ -122,15 +125,16 @@ export async function resolveRepoWorktreeRows( RESOLVED_WORKTREE_REPO_TIMEOUT_MS, null )) ?? { ok: false, worktrees: listStoredWorktreeRowsForRepo(store, repo, repoOwnerCount) } - const gitWorktrees = dedupeWorktreesByPath( - // Why: keep the git-scanned branch over a stored empty-branch clone of the same path (#15526). - [...scan.worktrees].sort( - (left, right) => Number(Boolean(right.branch)) - Number(Boolean(left.branch)) - ) - ) + const pathPlatform = resolveRepoWorktreePathPlatform(repo) + // Why: prune against the raw rows so a spelling the collapse drops is never mistaken for a deleted worktree. if (scan.ok) { - pruneLineageForMissingRepoWorktrees(store, repo, gitWorktrees) + pruneLineageForMissingRepoWorktrees(store, repo, scan.worktrees, pathPlatform) } + const gitWorktrees = collapsePathEqualWorktreeRows(scan.worktrees, { + repoPath: repo.path, + hasStoredMeta: (worktreePath) => metaById[`${repo.id}::${worktreePath}`] !== undefined, + platform: pathPlatform + }) const expectedHostId = getRepoExecutionHostId(repo) return gitWorktrees.map((gitWorktree) => { const worktreeId = `${repo.id}::${gitWorktree.path}` diff --git a/src/main/worktree-lineage-pruning.test.ts b/src/main/worktree-lineage-pruning.test.ts index db393e864d5..55b3d966085 100644 --- a/src/main/worktree-lineage-pruning.test.ts +++ b/src/main/worktree-lineage-pruning.test.ts @@ -57,6 +57,35 @@ function createStore( } describe('pruneLineageForMissingRepoWorktrees', () => { + it('keeps lineage stored under another spelling of a live worktree path', () => { + const parentId = 'repo-1::/repo' + const childId = 'repo-1::/repo/./child' + const edge = lineage(childId, parentId) + const worktreeLineageById = { [childId]: edge } + const workspaceLineageByChildKey = { + [worktreeWorkspaceKey(childId)]: workspaceLineage(childId, parentId) + } + const metaById = { + [parentId]: { instanceId: edge.parentWorktreeInstanceId } as WorktreeMeta + } + const store = createStore(worktreeLineageById, workspaceLineageByChildKey, metaById) + + pruneLineageForMissingRepoWorktrees( + store as never, + repo, + [ + { path: '/repo', head: 'a', branch: 'main', isBare: false, isMainWorktree: true }, + { path: '/repo/child/', head: 'b', branch: 'child', isBare: false, isMainWorktree: false } + ], + 'linux' + ) + + expect(store.removeWorktreeLineage).not.toHaveBeenCalled() + expect(store.removeWorkspaceLineage).not.toHaveBeenCalled() + expect(store.setWorktreeMeta).not.toHaveBeenCalled() + expect(worktreeLineageById[childId]).toBe(edge) + }) + it('refuses an empty scan when the repo still has registered lineage', () => { const parentId = 'repo-1::/repo/parent' const childId = 'repo-1::/repo/child' diff --git a/src/main/worktree-lineage-pruning.ts b/src/main/worktree-lineage-pruning.ts index 34a99ef6e95..c8c3f95b44d 100644 --- a/src/main/worktree-lineage-pruning.ts +++ b/src/main/worktree-lineage-pruning.ts @@ -4,6 +4,8 @@ import type { WorkspaceLineage, WorktreeLineage } from '../shared/worktree/linea import type { GitWorktreeInfo } from '../shared/worktree/types' import { getRepoExecutionHostId } from '../shared/execution-host' import { isWorkspaceKey, parseWorkspaceKey, worktreeWorkspaceKey } from '../shared/workspace-scope' +import { splitWorktreeId } from '../shared/worktree/id' +import { worktreePathComparisonKey } from './ipc/worktree-path-comparison' import type { Store } from './persistence' function worktreeIdBelongsToRepo(worktreeId: string, repoPrefix: string): boolean { @@ -37,7 +39,8 @@ function hasStoredRepoLineage( export function pruneLineageForMissingRepoWorktrees( store: Store, repo: Repo, - gitWorktrees: GitWorktreeInfo[] + gitWorktrees: GitWorktreeInfo[], + platform: NodeJS.Platform = process.platform ): void { if ( typeof store.getAllWorktreeLineage !== 'function' || @@ -56,6 +59,22 @@ export function pruneLineageForMissingRepoWorktrees( return } const liveIds = new Set(gitWorktrees.map((worktree) => `${repo.id}::${worktree.path}`)) + const livePathKeys = new Set( + gitWorktrees.map((worktree) => worktreePathComparisonKey(worktree.path, platform)) + ) + // Why: lineage is keyed on a stored spelling, so a live worktree reported under another spelling of + // the same directory must never look "proven missing" — that deletion is irreversible. + const isLive = (worktreeId: string): boolean => { + if (liveIds.has(worktreeId)) { + return true + } + // Why not `...ForFilesystem`: folder-workspace instances legitimately share a directory, so their + // id suffix must keep them distinct here. + const worktreePath = splitWorktreeId(worktreeId)?.worktreePath + return worktreePath + ? livePathKeys.has(worktreePathComparisonKey(worktreePath, platform)) + : false + } const expectedHostId = getRepoExecutionHostId(repo) const repoOwners = store.getRepos().filter((candidate) => candidate.id === repo.id) const canMutateWorktree = (worktreeId: string): boolean => { @@ -68,7 +87,7 @@ export function pruneLineageForMissingRepoWorktrees( childScope?.type === 'worktree' && worktreeIdBelongsToRepo(childScope.worktreeId, repoPrefix) && canMutateWorktree(childScope.worktreeId) && - !liveIds.has(childScope.worktreeId) && + !isLive(childScope.worktreeId) && isWorkspaceKey(childWorkspaceKey) ) { store.removeWorkspaceLineage?.(childWorkspaceKey) @@ -78,7 +97,7 @@ export function pruneLineageForMissingRepoWorktrees( if ( worktreeIdBelongsToRepo(childId, repoPrefix) && canMutateWorktree(childId) && - !liveIds.has(childId) + !isLive(childId) ) { // Why: a proven-missing path must not transfer its lineage to a future checkout at that path. store.removeWorktreeLineage(childId) @@ -87,7 +106,7 @@ export function pruneLineageForMissingRepoWorktrees( if ( worktreeIdBelongsToRepo(lineage.parentWorktreeId, repoPrefix) && canMutateWorktree(lineage.parentWorktreeId) && - !liveIds.has(lineage.parentWorktreeId) + !isLive(lineage.parentWorktreeId) ) { const parentMeta = store.getWorktreeMeta(lineage.parentWorktreeId) if (!parentMeta || parentMeta.instanceId === lineage.parentWorktreeInstanceId) {