From 0163e9a7de6348bdbdcb4ada313971edb4d0fa39 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Wed, 10 Jun 2026 14:12:12 -0700 Subject: [PATCH] Fix source control compare base ambiguity (#5127) Co-authored-by: Orca --- src/main/git/status.test.ts | 55 ++++++++++++++++++++++++++- src/main/git/status.ts | 11 +++++- src/main/ipc/worktree-remote.ts | 14 +++++-- src/main/ipc/worktrees.test.ts | 43 +++++++++++---------- src/main/runtime/orca-runtime.test.ts | 5 ++- src/main/runtime/orca-runtime.ts | 7 +++- 6 files changed, 104 insertions(+), 31 deletions(-) diff --git a/src/main/git/status.test.ts b/src/main/git/status.test.ts index 5dbf9aa5ec4..c1fd590775d 100644 --- a/src/main/git/status.test.ts +++ b/src/main/git/status.test.ts @@ -905,6 +905,7 @@ describe('getBranchCompare', () => { it('returns a pinned branch compare snapshot and parsed branch entries', async () => { gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'main\n' }) + .mockResolvedValueOnce({ stdout: 'remote-base-oid\n' }) .mockResolvedValueOnce({ stdout: 'head-oid\n' }) .mockResolvedValueOnce({ stdout: 'base-oid\n' }) .mockResolvedValueOnce({ stdout: 'merge-base-oid\n' }) @@ -939,6 +940,8 @@ describe('getBranchCompare', () => { it('returns invalid-base when the compare ref does not resolve', async () => { gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'main\n' }) + .mockRejectedValueOnce(new Error('missing remote base')) + .mockRejectedValueOnce(new Error('missing local base')) .mockResolvedValueOnce({ stdout: 'head-oid\n' }) .mockRejectedValueOnce(new Error('missing base')) @@ -952,6 +955,7 @@ describe('getBranchCompare', () => { it('returns unborn-head when HEAD cannot be resolved', async () => { gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'main\n' }) + .mockResolvedValueOnce({ stdout: 'remote-base-oid\n' }) .mockRejectedValueOnce(new Error('unborn')) .mockRejectedValueOnce(new Error('missing base')) @@ -965,6 +969,7 @@ describe('getBranchCompare', () => { it('treats an unborn branch with a resolvable base as having no committed branch changes', async () => { gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'feature\n' }) + .mockResolvedValueOnce({ stdout: 'remote-base-oid\n' }) .mockRejectedValueOnce(new Error('unborn')) .mockResolvedValueOnce({ stdout: 'base-oid\n' }) @@ -986,6 +991,7 @@ describe('getBranchCompare', () => { it('returns no-merge-base when histories do not intersect', async () => { gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'main\n' }) + .mockResolvedValueOnce({ stdout: 'remote-base-oid\n' }) .mockResolvedValueOnce({ stdout: 'head-oid\n' }) .mockResolvedValueOnce({ stdout: 'base-oid\n' }) .mockRejectedValueOnce(new Error('no merge base')) @@ -1000,6 +1006,7 @@ describe('getBranchCompare', () => { it('passes core.quotePath=false to diff --name-status and parses UTF-8 paths', async () => { gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'main\n' }) + .mockResolvedValueOnce({ stdout: 'remote-base-oid\n' }) .mockResolvedValueOnce({ stdout: 'head-oid\n' }) .mockResolvedValueOnce({ stdout: 'base-oid\n' }) .mockResolvedValueOnce({ stdout: 'merge-base-oid\n' }) @@ -1010,7 +1017,7 @@ describe('getBranchCompare', () => { const result = await getBranchCompare('/repo', 'origin/main') expect(gitExecFileAsyncMock).toHaveBeenNthCalledWith( - 5, + 6, [ '-c', 'core.quotePath=false', @@ -1028,6 +1035,52 @@ describe('getBranchCompare', () => { ]) }) + it('compares short remote labels through fully qualified remote-tracking refs', async () => { + gitExecFileAsyncMock.mockImplementation((args: string[]) => { + if (args[0] === 'branch') { + return Promise.resolve({ stdout: 'feature\n' }) + } + if ( + args[0] === 'rev-parse' && + args.includes('--quiet') && + args.includes('refs/remotes/origin/main^{commit}') + ) { + return Promise.resolve({ stdout: 'remote-base-oid\n' }) + } + if (args[0] === 'rev-parse' && args.includes('HEAD')) { + return Promise.resolve({ stdout: 'head-oid\n' }) + } + if (args[0] === 'rev-parse' && args.includes('refs/remotes/origin/main')) { + return Promise.resolve({ stdout: 'base-oid\n' }) + } + if (args[0] === 'merge-base') { + return Promise.resolve({ stdout: 'merge-base-oid\n' }) + } + if (args.includes('--name-status')) { + return Promise.resolve({ stdout: '' }) + } + if (args.includes('--numstat')) { + return Promise.resolve({ stdout: '' }) + } + if (args[0] === 'rev-list') { + return Promise.resolve({ stdout: '0\n' }) + } + throw new Error(`unexpected git args: ${args.join(' ')}`) + }) + + const result = await getBranchCompare('/repo', 'origin/main') + + expect(result.summary).toMatchObject({ + baseRef: 'origin/main', + baseOid: 'base-oid', + status: 'ready' + }) + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['rev-parse', '--verify', '--end-of-options', 'refs/remotes/origin/main'], + { cwd: '/repo' } + ) + }) + it('attaches counts for branch compare paths containing rename markers', async () => { gitExecFileAsyncMock.mockImplementation((args: string[]) => { if (args[0] === 'branch') { diff --git a/src/main/git/status.ts b/src/main/git/status.ts index 6ffc44b9cd5..8af5a4aaac6 100644 --- a/src/main/git/status.ts +++ b/src/main/git/status.ts @@ -35,6 +35,8 @@ import { removeSafeUntrackedDiscardTarget, removeSafeUntrackedDiscardTargets } from '../../shared/git-discard-path-safety' +import { resolveWorktreeAddBaseRef } from '../../shared/worktree-base-ref' +import { hasWorktreeBaseCommitRef } from './worktree-base-ref-probe' const MAX_GIT_SHOW_BYTES = 10 * 1024 * 1024 const MAX_STAGED_COMMIT_CONTEXT_BYTES = MAX_GIT_SHOW_BYTES @@ -702,6 +704,11 @@ export async function getBranchCompare( const compareRef = await resolveCompareRef(worktreePath) summary.compareRef = compareRef + // Why: short remote display refs like "origin/main" can collide with a local + // branch of the same name. Compare against the proven remote-tracking ref. + const resolvedBaseRef = await resolveWorktreeAddBaseRef(baseRef, (qualifiedRef) => + hasWorktreeBaseCommitRef(worktreePath, qualifiedRef) + ) let headOid = '' let baseOid = '' @@ -710,7 +717,7 @@ export async function getBranchCompare( summary.headOid = headOid } catch { try { - baseOid = await resolveRefOid(worktreePath, baseRef) + baseOid = await resolveRefOid(worktreePath, resolvedBaseRef) summary.baseOid = baseOid // Why: new remote worktrees can be on an unborn branch until the first // commit. There are no committed branch changes yet; surfacing this as a @@ -730,7 +737,7 @@ export async function getBranchCompare( } try { - baseOid = await resolveRefOid(worktreePath, baseRef) + baseOid = await resolveRefOid(worktreePath, resolvedBaseRef) summary.baseOid = baseOid } catch { summary.status = 'invalid-base' diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index 350a532a680..7100396c4f0 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -1403,6 +1403,9 @@ export async function createRemoteWorktree( const worktreeId = `${repo.id}::${created.path}` const now = Date.now() + // Why: persisted compare refs must survive local branches whose names look + // like remote labels, e.g. a local branch literally named "origin/main". + const metadataBaseRef = remoteTrackingBase?.ref ?? baseBranch let configuredPushTarget: GitPushTarget | undefined if (preparedPushTarget) { configuredPushTarget = await configureCreatedWorktreePushTargetSsh( @@ -1431,7 +1434,7 @@ export async function createRemoteWorktree( orcaCreatedAt: now, orcaCreationSource: 'ssh', orcaCreationWorkspaceLayout: getWorktreeCreationLayout(repo, settings), - baseRef: baseBranch, + baseRef: metadataBaseRef, ...(checkoutExistingBranch ? { preserveBranchOnDelete: true } : {}), ...(configuredPushTarget ? { pushTarget: configuredPushTarget } : {}), ...(requestedDisplayName @@ -1446,7 +1449,7 @@ export async function createRemoteWorktree( ...(sparseDirectories.length > 0 ? { sparseDirectories, - sparseBaseRef: baseBranch, + sparseBaseRef: metadataBaseRef, sparsePresetId } : {}), @@ -1909,6 +1912,9 @@ export async function createLocalWorktree( const worktreeId = `${repo.id}::${created.path}` const now = Date.now() + // Why: persisted compare refs must survive local branches whose names look + // like remote labels, e.g. a local branch literally named "origin/main". + const metadataBaseRef = remoteTrackingBase?.ref ?? baseBranch const metaUpdates: Partial = { // Why: path-derived worktree IDs can be reused after external deletion. // Fresh creations must rotate instance identity so stale lineage cannot @@ -1927,7 +1933,7 @@ export async function createLocalWorktree( orcaCreatedAt: now, orcaCreationSource: 'desktop', orcaCreationWorkspaceLayout: getWorktreeCreationLayout(repo, settings), - baseRef: baseBranch, + baseRef: metadataBaseRef, ...(checkoutExistingBranch ? { preserveBranchOnDelete: true } : {}), ...(configuredPushTarget ? { pushTarget: configuredPushTarget } : {}), ...(requestedDisplayName @@ -1938,7 +1944,7 @@ export async function createLocalWorktree( ...(sparseDirectories.length > 0 ? { sparseDirectories, - sparseBaseRef: baseBranch, + sparseBaseRef: metadataBaseRef, sparsePresetId } : {}), diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index bb30f47a3ae..1876dd524f1 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -563,10 +563,10 @@ describe('registerWorktreeHandlers', () => { } ]) - const result = await handlers['worktrees:create'](null, { + const result = (await handlers['worktrees:create'](null, { repoId: 'repo-1', name: 'improve-dashboard' - }) + })) as CreateWorktreeResult expect(addWorktreeMock).toHaveBeenCalledWith( '/workspace/repo', @@ -2171,7 +2171,8 @@ describe('registerWorktreeHandlers', () => { 'repo-ssh::/remote/sparse-dashboard', expect.objectContaining({ sparseDirectories: ['apps/mobile', 'packages/shared'], - sparseBaseRef: 'origin/main', + baseRef: 'refs/remotes/origin/main', + sparseBaseRef: 'refs/remotes/origin/main', sparsePresetId: 'preset-1' }) ) @@ -2179,7 +2180,7 @@ describe('registerWorktreeHandlers', () => { worktree: expect.objectContaining({ isSparse: true, sparseDirectories: ['apps/mobile', 'packages/shared'], - sparseBaseRef: 'origin/main', + sparseBaseRef: 'refs/remotes/origin/main', sparsePresetId: 'preset-1' }) }) @@ -2896,12 +2897,12 @@ describe('registerWorktreeHandlers', () => { ) expect(runtimeStub.fetchRemoteWithCache).not.toHaveBeenCalled() resolveFetch() - const result = await createPromise + const result = (await createPromise) as CreateWorktreeResult expect(addWorktreeMock).toHaveBeenCalled() - expect(result).toEqual( - expect.objectContaining({ - worktree: expect.objectContaining({ id: 'repo-1::/workspace/improve-dashboard' }) - }) + expect(result.worktree.id).toBe('repo-1::/workspace/improve-dashboard') + expect(store.setWorktreeMeta).toHaveBeenCalledWith( + 'repo-1::/workspace/improve-dashboard', + expect.objectContaining({ baseRef: 'refs/remotes/origin/main' }) ) }) @@ -2951,10 +2952,10 @@ describe('registerWorktreeHandlers', () => { ]) gitExecFileAsyncMock.mockResolvedValue({ stdout: 'created-sha\n', stderr: '' }) - const result = await handlers['worktrees:create'](null, { + const result = (await handlers['worktrees:create'](null, { repoId: 'repo-1', name: 'improve-dashboard' - }) + })) as CreateWorktreeResult expect(runtimeStub.getOrStartRemoteTrackingBaseRefresh).toHaveBeenCalledWith( '/workspace/repo', @@ -2994,10 +2995,10 @@ describe('registerWorktreeHandlers', () => { ]) gitExecFileAsyncMock.mockResolvedValue({ stdout: 'created-sha\n', stderr: '' }) - const result = await handlers['worktrees:create'](null, { + const result = (await handlers['worktrees:create'](null, { repoId: 'repo-1', name: 'improve-dashboard' - }) + })) as CreateWorktreeResult expect(addWorktreeMock).toHaveBeenCalledWith( '/workspace/repo', @@ -3016,15 +3017,15 @@ describe('registerWorktreeHandlers', () => { } } ) - expect(result).toEqual( - expect.objectContaining({ - localBaseRefUpdateSuggestion: { - baseRef: 'origin/main', - localBranch: 'main', - behind: 2 - } - }) + expect(store.setWorktreeMeta).toHaveBeenCalledWith( + 'repo-1::/workspace/improve-dashboard', + expect.objectContaining({ baseRef: 'refs/remotes/origin/main' }) ) + expect(result.localBaseRefUpdateSuggestion).toEqual({ + baseRef: 'origin/main', + localBranch: 'main', + behind: 2 + }) }) it('throws a clear error when no default base ref can be resolved', async () => { diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 0879e6df4f8..cfc33929428 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -1398,7 +1398,10 @@ describe('OrcaRuntimeService', () => { } } ) - expect(result.worktree).toMatchObject({ path: createdWorktree.path }) + expect(result.worktree).toMatchObject({ + path: createdWorktree.path, + baseRef: 'refs/remotes/origin/main' + }) } finally { gitSpy.mockRestore() } diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 93385dc3aad..3c4fbea3fd6 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -9461,6 +9461,9 @@ export class OrcaRuntimeService { const worktreeId = `${repo.id}::${created.path}` const now = Date.now() + // Why: persisted compare refs must survive local branches whose names look + // like remote labels, e.g. a local branch literally named "origin/main". + const metadataBaseRef = remoteTrackingBase?.ref ?? baseBranch const displayNameMeta = requestedDisplayName ? { displayName: requestedDisplayName } : shouldSetDisplayName(effectiveRequestedName, branchName, effectiveSanitizedName) @@ -9482,13 +9485,13 @@ export class OrcaRuntimeService { orcaCreationSource: 'runtime', orcaCreationWorkspaceLayout: getWorktreeCreationLayout(repo, settings), ...displayNameMeta, - baseRef: baseBranch, + baseRef: metadataBaseRef, ...(checkoutExistingBranch ? { preserveBranchOnDelete: true } : {}), ...(configuredPushTarget ? { pushTarget: configuredPushTarget } : {}), ...(sparseDirectories.length > 0 ? { sparseDirectories, - sparseBaseRef: baseBranch, + sparseBaseRef: metadataBaseRef, sparsePresetId: args.sparseCheckout?.presetId } : {}),