perf(git): reuse pinned OIDs for SSH file diffs

This commit is contained in:
Neil
2026-08-10 03:35:44 -07:00
parent a63df91790
commit a7e662aabb
11 changed files with 568 additions and 7 deletions
+36
View File
@@ -3098,6 +3098,42 @@ describe('registerFilesystemHandlers', () => {
)
})
it('forwards the pinned head through SSH branch diff queries', async () => {
const result = {
kind: 'text',
originalContent: 'left',
modifiedContent: 'right',
originalIsBinary: false,
modifiedIsBinary: false
}
const getBranchDiff = vi.fn().mockResolvedValue([result])
getSshGitProviderMock.mockReturnValue({ getBranchDiff })
registerFilesystemHandlers(store as never)
await expect(
handlers.get('git:branchDiff')!(null, {
worktreePath: '/home/user/project',
compare: {
baseRef: 'origin/main',
baseOid: 'base-oid',
headOid: 'head-oid',
mergeBase: 'merge-base-oid'
},
filePath: 'src/file.ts',
oldPath: 'src/old-file.ts',
connectionId: 'conn-1'
})
).resolves.toEqual(result)
expect(getBranchDiff).toHaveBeenCalledWith('/home/user/project', 'merge-base-oid', {
includePatch: true,
headOid: 'head-oid',
filePath: 'src/file.ts',
oldPath: 'src/old-file.ts'
})
})
// Why: the original SSH Quick Open bug had two halves — relay-side policy
// drift AND the main dispatcher silently dropping excludePaths before the
// provider saw them. This test guards the second half: regardless of
+1
View File
@@ -2042,6 +2042,7 @@ export function registerFilesystemHandlers(
}
const results = await provider.getBranchDiff(args.worktreePath, args.compare.mergeBase, {
includePatch: true,
headOid: args.compare.headOid,
filePath: args.filePath,
oldPath: args.oldPath
})
+1 -1
View File
@@ -66,7 +66,7 @@ export type IGitProvider = {
getBranchDiff(
worktreePath: string,
baseRef: string,
options?: { includePatch?: boolean; filePath?: string; oldPath?: string }
options?: { includePatch?: boolean; filePath?: string; oldPath?: string; headOid?: string }
): Promise<GitDiffResult[]>
getCommitDiff(
worktreePath: string,
@@ -1081,6 +1081,79 @@ describe('SshGitProvider', () => {
expect(result).toEqual(diffs)
})
it('getBranchDiff forwards a pinned head OID without adding it when omitted', async () => {
const headOid = 'a'.repeat(40)
mux.request.mockResolvedValue([])
await provider.getBranchDiff('/home/user/repo', 'main', {
includePatch: true,
filePath: 'src/file.ts',
headOid
})
await provider.getBranchDiff('/home/user/repo', 'main', {
includePatch: true,
filePath: 'src/other-file.ts',
headOid: undefined
})
expect(mux.request).toHaveBeenNthCalledWith(1, 'git.branchDiff', {
worktreePath: '/home/user/repo',
baseRef: 'main',
includePatch: true,
filePath: 'src/file.ts',
headOid,
__streamResponse: true
})
expect(mux.request).toHaveBeenNthCalledWith(2, 'git.branchDiff', {
worktreePath: '/home/user/repo',
baseRef: 'main',
includePatch: true,
filePath: 'src/other-file.ts',
__streamResponse: true
})
})
it('coalesces matching branch diff heads while keeping distinct and absent heads separate', async () => {
const diffs = [{ kind: 'text', originalContent: 'old', modifiedContent: 'new' }]
const pendingDiff = deferredValue(diffs)
const firstHeadOid = 'a'.repeat(40)
const secondHeadOid = 'b'.repeat(40)
mux.request.mockReturnValue(pendingDiff.promise)
const reads = [
provider.getBranchDiff('/home/user/repo', 'main', {
includePatch: true,
filePath: 'src/file.ts',
headOid: firstHeadOid
}),
provider.getBranchDiff('/home/user/repo', 'main', {
filePath: 'src/file.ts',
headOid: firstHeadOid,
includePatch: true
}),
provider.getBranchDiff('/home/user/repo', 'main', {
includePatch: true,
filePath: 'src/file.ts',
headOid: secondHeadOid
}),
provider.getBranchDiff('/home/user/repo', 'main', {
includePatch: true,
filePath: 'src/file.ts'
}),
provider.getBranchDiff('/home/user/repo', 'main', {
includePatch: true,
filePath: 'src/file.ts',
headOid: undefined
})
]
await waitForRequestCount(mux.request, 3)
expect(mux.request).toHaveBeenCalledTimes(3)
pendingDiff.resolve()
await expect(Promise.all(reads)).resolves.toEqual(Array.from({ length: 5 }, () => diffs))
})
it('coalesces concurrent identical diff RPCs while in flight', async () => {
const diff = {
kind: 'text',
+7 -3
View File
@@ -41,6 +41,7 @@ type NonInteractiveExecQueueEntry = {
}
const NON_INTERACTIVE_TRANSPORT_TIMEOUT_MARGIN_MS = 5_000
const ABSENT_BRANCH_DIFF_HEAD_OID = { absent: true } as const
function isJsonRpcMethodNotFoundError(error: unknown): boolean {
if (!error || typeof error !== 'object') {
@@ -658,9 +659,10 @@ export class SshGitProvider implements IGitProvider {
async getBranchDiff(
worktreePath: string,
baseRef: string,
options?: { includePatch?: boolean; filePath?: string; oldPath?: string }
options?: { includePatch?: boolean; filePath?: string; oldPath?: string; headOid?: string }
): Promise<GitDiffResult[]> {
const keyOptions = options ?? {}
const { headOid, ...relayOptions } = keyOptions
return this.gitDiffReadDedupe.run(
stableInFlightKey([
'branchDiff',
@@ -668,13 +670,15 @@ export class SshGitProvider implements IGitProvider {
baseRef,
keyOptions.includePatch ?? null,
keyOptions.filePath ?? null,
keyOptions.oldPath ?? null
keyOptions.oldPath ?? null,
headOid === undefined ? ABSENT_BRANCH_DIFF_HEAD_OID : headOid
]),
async () =>
(await requestGitStreamable(this.mux, 'git.branchDiff', {
worktreePath,
baseRef,
...options
...relayOptions,
...(headOid === undefined ? {} : { headOid })
})) as GitDiffResult[]
) as Promise<GitDiffResult[]>
}
@@ -0,0 +1,69 @@
import { beforeEach, describe, expect, it, vi } from 'vitest'
import type { GlobalSettings } from '../../shared/types'
import { RuntimeGitCommands, type ResolvedRuntimeGitWorktree } from './orca-runtime-git'
const getSshGitProviderMock = vi.hoisted(() => vi.fn())
vi.mock('../providers/ssh-git-dispatch', () => ({
getSshGitProvider: getSshGitProviderMock
}))
function makeWorktree(path: string): ResolvedRuntimeGitWorktree {
const worktree = {
id: 'wt-1',
repoId: 'repo-1',
path,
linkedIssue: null,
git: {
path,
branch: 'main',
isBare: false,
isMainWorktree: false,
head: 'a'.repeat(40)
}
} satisfies Partial<ResolvedRuntimeGitWorktree>
return worktree as unknown as ResolvedRuntimeGitWorktree
}
describe('RuntimeGitCommands branch diff', () => {
beforeEach(() => {
getSshGitProviderMock.mockReset()
})
it('forwards the pinned head through normalized nested SSH paths', async () => {
const mergeBase = 'a'.repeat(40)
const headOid = 'b'.repeat(40)
const result = {
kind: 'text',
originalContent: 'left',
modifiedContent: 'right',
originalIsBinary: false,
modifiedIsBinary: false
}
const provider = { getBranchDiff: vi.fn().mockResolvedValue([result]) }
getSshGitProviderMock.mockReturnValue(provider)
const commands = new RuntimeGitCommands({
resolveRuntimeGitTarget: async () => ({
worktree: makeWorktree('/remote/repo'),
connectionId: 'conn-1'
}),
getRuntimeSettings: () => ({}) as GlobalSettings
})
await expect(
commands.getRuntimeGitBranchDiff(
'id:wt-1',
{ mergeBase, headOid },
'src\\file.ts',
'src\\old-file.ts'
)
).resolves.toEqual(result)
expect(provider.getBranchDiff).toHaveBeenCalledWith('/remote/repo', mergeBase, {
includePatch: true,
headOid,
filePath: 'src/file.ts',
oldPath: 'src/old-file.ts'
})
})
})
+1
View File
@@ -538,6 +538,7 @@ export class RuntimeGitCommands {
}
const results = await provider.getBranchDiff(target.worktree.path, compare.mergeBase, {
includePatch: true,
headOid: compare.headOid,
filePath: relativePath,
oldPath: oldRelativePath
})
+50
View File
@@ -0,0 +1,50 @@
import { buildDiffResult } from './git-diff-result'
import { readBlobAtOid, type GitBufferExec } from './git-handler-ops'
const FULL_GIT_OBJECT_ID_PATTERN = /^(?:[0-9a-fA-F]{40}|[0-9a-fA-F]{64})$/
function assertFullGitObjectId(value: unknown, label: string): asserts value is string {
if (typeof value !== 'string' || !FULL_GIT_OBJECT_ID_PATTERN.test(value)) {
throw new Error(`${label} must be a full git object id`)
}
}
export function parseOptionalBranchDiffHeadOid(
params: Record<string, unknown>
): string | undefined {
if (!Object.hasOwn(params, 'headOid')) {
return undefined
}
const { headOid } = params
assertFullGitObjectId(headOid, 'headOid')
return headOid
}
export async function branchDiffEntryAtPinnedOids(
gitBuffer: GitBufferExec,
worktreePath: string,
baseOid: string,
headOid: string,
filePath: string,
oldPath?: string
) {
assertFullGitObjectId(baseOid, 'baseRef')
assertFullGitObjectId(headOid, 'headOid')
try {
const [left, right] = await Promise.all([
readBlobAtOid(gitBuffer, worktreePath, baseOid, oldPath ?? filePath),
readBlobAtOid(gitBuffer, worktreePath, headOid, filePath)
])
return [buildDiffResult(left.content, right.content, left.isBinary, right.isBinary, filePath)]
} catch {
return [
{
kind: 'text' as const,
originalContent: '',
modifiedContent: '',
originalIsBinary: false,
modifiedIsBinary: false
}
]
}
}
+305
View File
@@ -0,0 +1,305 @@
import { execFileSync } from 'node:child_process'
import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'
import { tmpdir } from 'node:os'
import * as path from 'node:path'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import { RelayContext } from './context'
import {
branchDiffEntryAtPinnedOids,
parseOptionalBranchDiffHeadOid
} from './git-handler-branch-diff-ops'
import { GitHandler } from './git-handler'
import { branchDiffEntries, type GitBufferExec, type GitExec } from './git-handler-ops'
import {
createMockDispatcher,
gitCommit,
gitInit,
type MockDispatcher,
type RelayDispatcher
} from './git-handler-test-setup'
const BASE_OID = 'a'.repeat(40)
const HEAD_OID = 'b'.repeat(40)
const OTHER_HEAD_OID = 'c'.repeat(40)
const FILE_PATH = 'src/file.ts'
type GitBufferTarget = {
gitBuffer(args: string[], cwd: string): Promise<Buffer>
}
type GitTarget = {
git(args: string[], cwd: string): Promise<{ stdout: string; stderr: string }>
}
function deferredBuffer(): { promise: Promise<Buffer>; resolve: (content: string) => void } {
let resolve!: (value: Buffer) => void
const promise = new Promise<Buffer>((innerResolve) => {
resolve = innerResolve
})
return { promise, resolve: (content) => resolve(Buffer.from(content)) }
}
describe('pinned relay branch diff operation', () => {
it.each([
{
name: 'addition',
status: `A\t${FILE_PATH}\n`,
left: new Error('missing'),
right: Buffer.from('added\n')
},
{
name: 'deletion',
status: `D\t${FILE_PATH}\n`,
left: Buffer.from('deleted\n'),
right: new Error('missing')
},
{
name: 'binary content',
status: `M\t${FILE_PATH}\n`,
left: Buffer.from([0, 1]),
right: Buffer.from([0, 2])
},
{
name: 'blob read failure',
status: `M\t${FILE_PATH}\n`,
left: new Error('left failed'),
right: new Error('right failed')
}
])('matches the legacy $name result', async ({ status, left, right }) => {
const createGitBuffer = (): GitBufferExec =>
vi.fn<GitBufferExec>(async (args) => {
const value = args[2].startsWith(`${BASE_OID}:`) ? left : right
if (value instanceof Error) {
throw value
}
return value
})
const git = vi.fn<GitExec>(async (args) => {
if (args[0] === 'rev-parse' && args.includes('HEAD')) {
return { stdout: `${HEAD_OID}\n`, stderr: '' }
}
if (args[0] === 'rev-parse' || args[0] === 'merge-base') {
return { stdout: `${BASE_OID}\n`, stderr: '' }
}
return { stdout: status, stderr: '' }
})
const pinned = await branchDiffEntryAtPinnedOids(
createGitBuffer(),
'/repo',
BASE_OID,
HEAD_OID,
FILE_PATH
)
const legacy = await branchDiffEntries(git, createGitBuffer(), '/repo', BASE_OID, {
includePatch: true,
filePath: FILE_PATH
})
expect(pinned).toEqual(legacy)
})
it('treats an omitted head OID differently from a supplied invalid value', () => {
expect(parseOptionalBranchDiffHeadOid({})).toBeUndefined()
expect(() => parseOptionalBranchDiffHeadOid({ headOid: null })).toThrow(
'headOid must be a full git object id'
)
})
})
describe('GitHandler pinned branch diff route', () => {
let dispatcher: MockDispatcher
let handler: GitHandler
beforeEach(() => {
dispatcher = createMockDispatcher()
handler = new GitHandler(dispatcher as unknown as RelayDispatcher, new RelayContext())
})
afterEach(() => {
handler.dispose()
vi.restoreAllMocks()
})
function request(overrides: Record<string, unknown> = {}): Promise<unknown> {
return dispatcher.callRequest('git.branchDiff', {
worktreePath: '/repo',
baseRef: BASE_OID,
headOid: HEAD_OID,
includePatch: true,
filePath: FILE_PATH,
...overrides
})
}
it('starts exactly two pinned blob reads concurrently without metadata commands', async () => {
const left = deferredBuffer()
const right = deferredBuffer()
const gitBufferSpy = vi
.spyOn(handler as unknown as GitBufferTarget, 'gitBuffer')
.mockImplementation((args) => {
if (args[2] === `${BASE_OID}:${FILE_PATH}`) {
return left.promise
}
if (args[2] === `${HEAD_OID}:${FILE_PATH}`) {
return right.promise
}
throw new Error(`unexpected blob spec: ${args[2]}`)
})
const gitSpy = vi
.spyOn(handler as unknown as GitTarget, 'git')
.mockRejectedValue(new Error('metadata command should not run'))
const resultPromise = request()
await vi.waitFor(() => expect(gitBufferSpy).toHaveBeenCalledTimes(2))
expect(gitSpy).not.toHaveBeenCalled()
expect(gitBufferSpy.mock.calls.map(([args]) => args)).toEqual([
['show', '--end-of-options', `${BASE_OID}:${FILE_PATH}`],
['show', '--end-of-options', `${HEAD_OID}:${FILE_PATH}`]
])
left.resolve('before\n')
right.resolve('after\n')
await expect(resultPromise).resolves.toEqual([
{
kind: 'text',
originalContent: 'before\n',
modifiedContent: 'after\n',
originalIsBinary: false,
modifiedIsBinary: false
}
])
})
it('reads the old path only from the base OID', async () => {
const oldPath = 'src/old-file.ts'
const gitBufferSpy = vi
.spyOn(handler as unknown as GitBufferTarget, 'gitBuffer')
.mockImplementation(async (args) => Buffer.from(args[2].startsWith(BASE_OID) ? 'old' : 'new'))
await request({ oldPath })
expect(gitBufferSpy.mock.calls.map(([args]) => args[2])).toEqual([
`${BASE_OID}:${oldPath}`,
`${HEAD_OID}:${FILE_PATH}`
])
})
it('coalesces the same head OID while keeping different heads independent', async () => {
const pending = deferredBuffer()
const gitBufferSpy = vi
.spyOn(handler as unknown as GitBufferTarget, 'gitBuffer')
.mockImplementation(() => pending.promise)
const requests = [request(), request(), request({ headOid: OTHER_HEAD_OID })]
await vi.waitFor(() => expect(gitBufferSpy).toHaveBeenCalledTimes(4))
const specs = gitBufferSpy.mock.calls.map(([args]) => args[2])
expect(specs.filter((spec) => spec === `${BASE_OID}:${FILE_PATH}`)).toHaveLength(2)
expect(specs.filter((spec) => spec === `${HEAD_OID}:${FILE_PATH}`)).toHaveLength(1)
expect(specs.filter((spec) => spec === `${OTHER_HEAD_OID}:${FILE_PATH}`)).toHaveLength(1)
pending.resolve('content\n')
await Promise.all(requests)
})
it('rejects every supplied malformed head OID before running Git', async () => {
const gitBufferSpy = vi.spyOn(handler as unknown as GitBufferTarget, 'gitBuffer')
const invalidHeadOids = [null, 7, {}, '', 'a'.repeat(39), 'g'.repeat(40)]
for (const headOid of invalidHeadOids) {
await expect(request({ headOid })).rejects.toThrow('headOid must be a full git object id')
}
expect(gitBufferSpy).not.toHaveBeenCalled()
})
it('accepts SHA-256 object IDs', async () => {
const baseOid = 'A'.repeat(64)
const headOid = 'b'.repeat(64)
const gitBufferSpy = vi
.spyOn(handler as unknown as GitBufferTarget, 'gitBuffer')
.mockResolvedValue(Buffer.from('content\n'))
await request({ baseRef: baseOid, headOid })
expect(gitBufferSpy.mock.calls.map(([args]) => args[2])).toEqual([
`${baseOid}:${FILE_PATH}`,
`${headOid}:${FILE_PATH}`
])
})
it('requires a full base OID only on the pinned route', async () => {
const gitBufferSpy = vi.spyOn(handler as unknown as GitBufferTarget, 'gitBuffer')
await expect(request({ baseRef: 'main' })).rejects.toThrow(
'baseRef must be a full git object id'
)
expect(gitBufferSpy).not.toHaveBeenCalled()
})
it('uses the legacy path when patch or file-path conditions are not met', async () => {
const gitSpy = vi
.spyOn(handler as unknown as GitTarget, 'git')
.mockImplementation(async (args) => {
if (args[0] === 'rev-parse' && args.includes('HEAD')) {
return { stdout: `${HEAD_OID}\n`, stderr: '' }
}
if (args[0] === 'rev-parse' || args[0] === 'merge-base') {
return { stdout: `${BASE_OID}\n`, stderr: '' }
}
return { stdout: '', stderr: '' }
})
const gitBufferSpy = vi.spyOn(handler as unknown as GitBufferTarget, 'gitBuffer')
await Promise.all([
request({ baseRef: 'main', includePatch: false }),
request({ baseRef: 'main', filePath: '' }),
request({ baseRef: 'main', filePath: { malformed: true } })
])
expect(gitSpy).toHaveBeenCalledTimes(12)
expect(gitBufferSpy).not.toHaveBeenCalled()
})
it('keeps the requested head pinned after repository HEAD moves', async () => {
const repoPath = mkdtempSync(path.join(tmpdir(), 'relay-pinned-branch-diff-'))
try {
gitInit(repoPath)
writeFileSync(path.join(repoPath, 'file.txt'), 'base\n')
gitCommit(repoPath, 'base')
const baseOid = execFileSync('git', ['rev-parse', 'HEAD'], {
cwd: repoPath,
encoding: 'utf8'
}).trim()
writeFileSync(path.join(repoPath, 'file.txt'), 'pinned\n')
gitCommit(repoPath, 'pinned')
const pinnedHeadOid = execFileSync('git', ['rev-parse', 'HEAD'], {
cwd: repoPath,
encoding: 'utf8'
}).trim()
writeFileSync(path.join(repoPath, 'file.txt'), 'moved\n')
gitCommit(repoPath, 'moved')
const result = (await request({
worktreePath: repoPath,
baseRef: baseOid,
headOid: pinnedHeadOid,
filePath: 'file.txt'
})) as { originalContent: string; modifiedContent: string }[]
expect(result[0]).toMatchObject({
originalContent: 'base\n',
modifiedContent: 'pinned\n'
})
expect(
execFileSync('git', ['show', 'HEAD:file.txt'], { cwd: repoPath, encoding: 'utf8' })
).toBe('moved\n')
} finally {
rmSync(repoPath, { recursive: true, force: true })
}
})
})
+24 -2
View File
@@ -18,6 +18,10 @@ import {
validateGitExecArgs,
type GitExec
} from './git-handler-ops'
import {
branchDiffEntryAtPinnedOids,
parseOptionalBranchDiffHeadOid
} from './git-handler-branch-diff-ops'
import {
buildSubmoduleInnerCommitRangeDiff,
computeSubmodulePointerDiff,
@@ -1148,6 +1152,7 @@ export class GitHandler {
if (baseRef.startsWith('-')) {
throw new Error('Base ref must not start with "-"')
}
const headOid = parseOptionalBranchDiffHeadOid(params)
const options = {
includePatch: params.includePatch as boolean | undefined,
filePath: params.filePath as string | undefined,
@@ -1158,18 +1163,35 @@ export class GitHandler {
'branchDiff',
worktreePath,
baseRef,
headOid ?? null,
options.includePatch ?? null,
options.filePath ?? null,
options.oldPath ?? null
]),
() =>
branchDiffEntries(
() => {
if (
headOid &&
options.includePatch === true &&
typeof options.filePath === 'string' &&
options.filePath.length > 0
) {
return branchDiffEntryAtPinnedOids(
this.gitBuffer.bind(this),
worktreePath,
baseRef,
headOid,
options.filePath,
options.oldPath
)
}
return branchDiffEntries(
this.git.bind(this),
this.gitBuffer.bind(this),
worktreePath,
baseRef,
options
)
}
)
return this.maybeStreamResponse(result, params, context)
}
@@ -426,7 +426,7 @@ describe('useEditorPanelContentState', () => {
connectionId: 'ssh-1'
}),
expect.objectContaining({
compare: expect.objectContaining({ mergeBase: 'merge-base' }),
compare: expect.objectContaining({ headOid: 'head', mergeBase: 'merge-base' }),
filePath: 'api/src/file.ts'
})
)