Fix source control compare base ambiguity (#5127)

Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Jinwoo Hong
2026-06-11 12:39:58 -07:00
committed by Jinwoo-H
co-authored by Orca
parent 16d9e03c49
commit 0163e9a7de
6 changed files with 104 additions and 31 deletions
+54 -1
View File
@@ -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') {
+9 -2
View File
@@ -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'
+10 -4
View File
@@ -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<WorktreeMeta> = {
// 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
}
: {}),
+22 -21
View File
@@ -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 () => {
+4 -1
View File
@@ -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()
}
+5 -2
View File
@@ -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
}
: {}),