diff --git a/docs/reference/git-compatibility.md b/docs/reference/git-compatibility.md new file mode 100644 index 00000000000..6ca8e8796b7 --- /dev/null +++ b/docs/reference/git-compatibility.md @@ -0,0 +1,59 @@ +# Git Compatibility Policy + +## Scope + +Orca executes the user's Git binary on three kinds of execution host: native, +WSL, and SSH. Each host can have a different Git version, so compatibility +state must be scoped to the host that actually runs the command. + +Git 2.25 is the core-workflow compatibility baseline for command selection. It +is the oldest line that covers Orca's baseline use of porcelain v2, `branch +--show-current`, `restore`, and sparse checkout. Optional features that need a +newer Git must degrade safely and cache the missing capability. Orca does not +currently block older Git at startup, but new command construction should not +assume features introduced after this baseline. + +## Capability Rules + +When a newer Git feature materially improves correctness or performance: + +1. Keep a baseline-compatible command or parser as the fallback. +2. Detect rejection with a narrow predicate for that option or subcommand. +3. Run the preferred command through `GitCapabilityCache` so a rejection is + remembered for the native host, WSL distro, or SSH provider that produced it. +4. Retry after the cache interval so an in-place Git upgrade self-heals without + restarting Orca. +5. Test the first fallback, later calls that skip the rejected probe, concurrent + probe coalescing, and execution-host isolation where applicable. + +Do not branch only on a parsed `git --version`. Vendor builds can backport +features, and wrappers can report a host version that differs from the binary +used inside WSL or SSH. A behavior probe plus a precise fallback is the final +authority. + +## Current Capabilities + +| Capability | Preferred behavior | Compatibility behavior | +| ----------------------- | ------------------------------------------------- | --------------------------------------------------------------------------------------- | +| `worktree-list-z` | NUL-delimited worktree paths | Line-block parser for Git before `worktree list -z` | +| `rev-parse-path-format` | Absolute repo metadata paths | Resolve legacy relative output against the scanned repo | +| `for-each-ref-exclude` | Exclude remote HEAD before the output limit | Request extra refs, then filter remote HEAD in Orca | +| `merge-tree-write-tree` | Derive real-merge conflicts and no-op tree proofs | Omit the conflict summary and keep conservative branch cleanup behavior before Git 2.38 | +| `merge-tree-merge-base` | Supply the already-resolved merge base | Use the older two-commit `merge-tree --write-tree` form | + +## Why Not `simple-git` + +`simple-git` is a process wrapper around the installed Git binary. Its custom +options and `raw` API pass arguments through to Git, so it cannot make a newer +flag work on an older binary or choose Orca's semantic fallback automatically. +It provides version reporting and subprocess queueing, but Orca already needs +its own WSL/SSH routing, cancellation, tracing, redaction, process cleanup, and +bounded output handling. Replacing the runner would move—not remove—the +capability problem. + +## Reliability Follow-up + +The strongest remaining gate is a small old-Git contract matrix that runs the +capability tests against Git 2.25, a pre-2.36 release, and current Git. Keep the +unit tests regardless: they cover localized error text, WSL/SSH isolation, and +concurrent probes that a single container run does not exercise. diff --git a/src/main/git/git-capability-state.ts b/src/main/git/git-capability-state.ts new file mode 100644 index 00000000000..aec7a369a26 --- /dev/null +++ b/src/main/git/git-capability-state.ts @@ -0,0 +1,44 @@ +import { GitCapabilityCache } from '../../shared/git-capability-cache' +import { parseWslUncPath } from '../../shared/wsl-paths' + +type LocalGitCapabilityTarget = { + cwd?: string + wslDistro?: string +} + +const localCapabilitiesByExecutionHost = new Map() +// Why: reconnecting creates a new provider, while concurrent IPC/runtime users +// of one SSH connection must share the same remote Git capability results. +let sshCapabilitiesByProvider = new WeakMap() + +function getLocalGitExecutionHostKey(target: LocalGitCapabilityTarget): string { + const wslDistro = + target.wslDistro ?? (target.cwd ? parseWslUncPath(target.cwd)?.distro : undefined) + return wslDistro ? `wsl:${wslDistro}` : 'local' +} + +export function getLocalGitCapabilityCache( + target: LocalGitCapabilityTarget = {} +): GitCapabilityCache { + const executionHost = getLocalGitExecutionHostKey(target) + let cache = localCapabilitiesByExecutionHost.get(executionHost) + if (!cache) { + cache = new GitCapabilityCache() + localCapabilitiesByExecutionHost.set(executionHost, cache) + } + return cache +} + +export function getSshGitCapabilityCache(provider: object): GitCapabilityCache { + let cache = sshCapabilitiesByProvider.get(provider) + if (!cache) { + cache = new GitCapabilityCache() + sshCapabilitiesByProvider.set(provider, cache) + } + return cache +} + +export function clearGitCapabilityStateForTests(): void { + localCapabilitiesByExecutionHost.clear() + sshCapabilitiesByProvider = new WeakMap() +} diff --git a/src/main/git/remove-worktree.test.ts b/src/main/git/remove-worktree.test.ts index f2f356a8734..b0fac2d3c56 100644 --- a/src/main/git/remove-worktree.test.ts +++ b/src/main/git/remove-worktree.test.ts @@ -32,6 +32,8 @@ vi.mock('fs/promises', async () => { return { ...actual, stat: statMock } }) +import { clearGitCapabilityStateForTests } from './git-capability-state' + import { addSparseWorktree, assertWorktreeCleanForRemoval, @@ -40,6 +42,10 @@ import { removeWorktree } from './worktree' +beforeEach(() => { + clearGitCapabilityStateForTests() +}) + type MockResult = { error?: Error stdout?: string diff --git a/src/main/git/repo-search-ref-compat.test.ts b/src/main/git/repo-search-ref-compat.test.ts index 9e9659a11b2..7c29357f55d 100644 --- a/src/main/git/repo-search-ref-compat.test.ts +++ b/src/main/git/repo-search-ref-compat.test.ts @@ -9,10 +9,12 @@ vi.mock('./runner', () => ({ gitExecFileSync: vi.fn() })) +import { clearGitCapabilityStateForTests } from './git-capability-state' import { searchBaseRefs } from './repo' describe('searchBaseRefs git compatibility', () => { afterEach(() => { + clearGitCapabilityStateForTests() gitExecFileAsyncMock.mockReset() }) @@ -35,13 +37,15 @@ describe('searchBaseRefs git compatibility', () => { } }) + await expect(searchBaseRefs('/repo', '', 1)).resolves.toEqual(['origin/main']) await expect(searchBaseRefs('/repo', '', 1)).resolves.toEqual(['origin/main']) const forEachRefCalls = gitExecFileAsyncMock.mock.calls.filter( (call) => (call[0] as string[])[0] === 'for-each-ref' ) - expect(forEachRefCalls).toHaveLength(2) + expect(forEachRefCalls).toHaveLength(3) expect(forEachRefCalls[0][0]).toContain('--exclude=refs/remotes/**/HEAD') expect(forEachRefCalls[1][0]).not.toContain('--exclude=refs/remotes/**/HEAD') expect(forEachRefCalls[1][0]).toContain('--count=104') + expect(forEachRefCalls[2][0]).not.toContain('--exclude=refs/remotes/**/HEAD') }) }) diff --git a/src/main/git/repo.ts b/src/main/git/repo.ts index 68900f810bf..b69dc226c7a 100644 --- a/src/main/git/repo.ts +++ b/src/main/git/repo.ts @@ -8,6 +8,7 @@ import { normalizeRuntimePathSeparators } from '../../shared/cross-platform-path import { parseWslUncPath } from '../../shared/wsl-paths' import { toWindowsWslPath } from '../wsl' import { buildHostedRemoteCommitUrl, buildHostedRemoteFileUrl } from './hosted-remote-url' +import { getLocalGitCapabilityCache } from './git-capability-state' type LocalGitExecOptions = { wslDistro?: string @@ -788,27 +789,27 @@ async function runSearchBaseRefsGit( limit: number, options: { remoteNames: readonly string[]; patternGroup?: RefSearchPatternGroup } ): Promise<{ stdout: string }> { - try { - return await gitExecFileAsync( - buildSearchBaseRefsArgv(normalizedQuery, limit, { - remoteNames: options.remoteNames, - patternGroup: options.patternGroup - }), - { cwd: path } - ) - } catch (err) { - if (!isForEachRefExcludeUnsupportedError(err)) { - throw err - } - return gitExecFileAsync( - buildSearchBaseRefsArgv(normalizedQuery, limit, { - excludeRemoteHead: false, - remoteNames: options.remoteNames, - patternGroup: options.patternGroup - }), - { cwd: path } - ) - } + return getLocalGitCapabilityCache({ cwd: path }).runWithFallback( + 'for-each-ref-exclude', + () => + gitExecFileAsync( + buildSearchBaseRefsArgv(normalizedQuery, limit, { + remoteNames: options.remoteNames, + patternGroup: options.patternGroup + }), + { cwd: path } + ), + () => + gitExecFileAsync( + buildSearchBaseRefsArgv(normalizedQuery, limit, { + excludeRemoteHead: false, + remoteNames: options.remoteNames, + patternGroup: options.patternGroup + }), + { cwd: path } + ), + isForEachRefExcludeUnsupportedError + ) } export function mergeBaseRefSearchResultGroups( diff --git a/src/main/git/worktree-git-capabilities.test.ts b/src/main/git/worktree-git-capabilities.test.ts new file mode 100644 index 00000000000..c6d7cdfc6ce --- /dev/null +++ b/src/main/git/worktree-git-capabilities.test.ts @@ -0,0 +1,138 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { gitExecFileAsyncMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn() +})) + +vi.mock('./runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock, + gitExecFileSync: vi.fn(), + translateWslOutputPaths: (output: string) => output +})) + +import { clearGitCapabilityStateForTests } from './git-capability-state' +import { listWorktrees } from './worktree' + +const WORKTREE_LIST_OUTPUT = `worktree /repo +HEAD abc123 +branch refs/heads/main +` + +describe('worktree Git capabilities', () => { + beforeEach(() => { + clearGitCapabilityStateForTests() + gitExecFileAsyncMock.mockReset() + }) + + it('does not repeat a known-unsupported -z probe on later scans', async () => { + gitExecFileAsyncMock.mockImplementation((args: string[]) => { + if (args.includes('-z')) { + return Promise.reject( + Object.assign(new Error('git usage error'), { + code: 129, + stderr: 'usage: git worktree list []\n' + }) + ) + } + return Promise.resolve({ stdout: WORKTREE_LIST_OUTPUT }) + }) + + await listWorktrees('/repo') + await listWorktrees('/repo') + + expect(gitExecFileAsyncMock.mock.calls.map(([args]) => args)).toEqual([ + ['worktree', 'list', '--porcelain', '-z'], + ['worktree', 'list', '--porcelain'], + ['worktree', 'list', '--porcelain'] + ]) + }) + + it('keeps native and WSL Git capability results separate', async () => { + gitExecFileAsyncMock.mockImplementation( + (args: string[], options: { cwd: string; wslDistro?: string }) => { + if (args.includes('-z') && !options.wslDistro) { + return Promise.reject( + Object.assign(new Error('git usage error'), { + code: 129, + stderr: 'usage: git worktree list []\n' + }) + ) + } + return Promise.resolve({ + stdout: `worktree ${options.cwd}\nHEAD abc123\nbranch refs/heads/main\n` + }) + } + ) + + await listWorktrees('/native-repo') + await listWorktrees('/wsl-repo', { wslDistro: 'Ubuntu' }) + + expect( + gitExecFileAsyncMock.mock.calls.map(([args, options]) => ({ + args, + wslDistro: options.wslDistro + })) + ).toEqual([ + { args: ['worktree', 'list', '--porcelain', '-z'], wslDistro: undefined }, + { args: ['worktree', 'list', '--porcelain'], wslDistro: undefined }, + { args: ['worktree', 'list', '--porcelain', '-z'], wslDistro: 'Ubuntu' } + ]) + }) + + it('does not repeat a known-unsupported rev-parse --path-format probe', async () => { + gitExecFileAsyncMock.mockImplementation((args: string[]) => { + if (args[0] === 'worktree') { + return Promise.resolve({ + stdout: 'worktree /git-store/project.git\nHEAD abc123\nbranch refs/heads/main\n' + }) + } + if (args.includes('--path-format=absolute')) { + return Promise.reject( + Object.assign(new Error('unknown option: --path-format=absolute'), { + stderr: 'error: unknown option `path-format=absolute`\n' + }) + ) + } + return Promise.resolve({ stdout: '/repo\n/git-store/project.git\n' }) + }) + + await listWorktrees('/repo') + await listWorktrees('/repo') + + const revParseCalls = gitExecFileAsyncMock.mock.calls.filter( + ([args]) => (args as string[])[0] === 'rev-parse' + ) + expect(revParseCalls.map(([args]) => args)).toEqual([ + ['rev-parse', '--path-format=absolute', '--show-toplevel', '--git-common-dir'], + ['rev-parse', '--show-toplevel', '--git-common-dir'], + ['rev-parse', '--show-toplevel', '--git-common-dir'] + ]) + }) + + it('remembers old Git that echoes --path-format while exiting successfully', async () => { + gitExecFileAsyncMock.mockImplementation((args: string[]) => { + if (args[0] === 'worktree') { + return Promise.resolve({ + stdout: 'worktree /git-store/project.git\nHEAD abc123\nbranch refs/heads/main\n' + }) + } + if (args.includes('--path-format=absolute')) { + return Promise.resolve({ + stdout: '--path-format=absolute\n/repo\n/git-store/project.git\n' + }) + } + return Promise.resolve({ stdout: '/repo\n/git-store/project.git\n' }) + }) + + await listWorktrees('/repo') + await listWorktrees('/repo') + + const revParseCalls = gitExecFileAsyncMock.mock.calls.filter( + ([args]) => (args as string[])[0] === 'rev-parse' + ) + expect(revParseCalls.map(([args]) => args)).toEqual([ + ['rev-parse', '--path-format=absolute', '--show-toplevel', '--git-common-dir'], + ['rev-parse', '--show-toplevel', '--git-common-dir'] + ]) + }) +}) diff --git a/src/main/git/worktree.test.ts b/src/main/git/worktree.test.ts index 43269a065a1..611fb35c26b 100644 --- a/src/main/git/worktree.test.ts +++ b/src/main/git/worktree.test.ts @@ -18,6 +18,8 @@ vi.mock('./runner', () => ({ translateWslOutputPaths: translateWslOutputPathsMock })) +import { clearGitCapabilityStateForTests } from './git-capability-state' + import { addSparseWorktree, addWorktree, @@ -29,6 +31,10 @@ import { WORKTREE_ADD_TIMEOUT_MS } from './worktree' +beforeEach(() => { + clearGitCapabilityStateForTests() +}) + describe('listWorktrees in-flight sharing', () => { beforeEach(() => { gitExecFileAsyncMock.mockReset() diff --git a/src/main/git/worktree.ts b/src/main/git/worktree.ts index eb30e13ad0a..a3df6e37aca 100644 --- a/src/main/git/worktree.ts +++ b/src/main/git/worktree.ts @@ -15,6 +15,7 @@ import type { } from '../../shared/types' import { parseGitRevListAheadBehindCounts } from '../../shared/git-rev-list-output' import { parseWslUncPath } from '../../shared/wsl-paths' +import { getLocalGitCapabilityCache } from './git-capability-state' import { gitExecFileAsync, translateWslOutputPaths } from './runner' import { resolveGitDir } from './status' import { hasWorktreeBaseCommitRef } from './worktree-base-ref-probe' @@ -403,29 +404,43 @@ function parseRepoLocation(repoPath: string, output: string): RepoLocation | und } } +function echoedUnsupportedPathFormatOption(output: string): boolean { + return output.split(/\r?\n/).some((line) => line.startsWith('--path-format')) +} + async function readRepoLocation( repoPath: string, resolveBasePath: string, options: GitWorktreeExecOptions = {} ): Promise { + const capabilities = getLocalGitCapabilityCache({ + cwd: repoPath, + wslDistro: options.wslDistro + }) try { - const { stdout } = await gitExecFileAsync( - ['rev-parse', '--path-format=absolute', '--show-toplevel', '--git-common-dir'], - gitExecOptions(repoPath, options) + return await capabilities.runWithFallback( + 'rev-parse-path-format', + async () => { + const { stdout } = await gitExecFileAsync( + ['rev-parse', '--path-format=absolute', '--show-toplevel', '--git-common-dir'], + gitExecOptions(repoPath, options) + ) + if (echoedUnsupportedPathFormatOption(stdout)) { + // Why: some old Git versions echo the unknown option and exit zero; + // remember that compatibility signal even though parsing can recover. + capabilities.rememberUnsupported('rev-parse-path-format') + } + return parseRepoLocation(resolveBasePath, stdout) + }, + async () => { + const { stdout } = await gitExecFileAsync( + ['rev-parse', '--show-toplevel', '--git-common-dir'], + gitExecOptions(repoPath, options) + ) + return parseRepoLocation(resolveBasePath, stdout) + }, + isUnsupportedRevParsePathFormatError ) - return parseRepoLocation(resolveBasePath, stdout) - } catch (error) { - if (!isUnsupportedRevParsePathFormatError(error)) { - return undefined - } - } - - try { - const { stdout } = await gitExecFileAsync( - ['rev-parse', '--show-toplevel', '--git-common-dir'], - gitExecOptions(repoPath, options) - ) - return parseRepoLocation(resolveBasePath, stdout) } catch { return undefined } @@ -555,29 +570,34 @@ async function readWorktreeList( repoPath: string, options: GitWorktreeExecOptions = {} ): Promise { - try { - const { stdout } = await gitExecFileAsync(['worktree', 'list', '--porcelain', '-z'], { - cwd: repoPath, - ...options - }) - return normalizeMainWorktreePath( - repoPath, - parseWorktreeList(stdout, { nulDelimited: true }), - options - ) - } catch (error) { - if (!isUnsupportedWorktreeListZError(error)) { - throw error - } - } - - // Why: `-z` is required to preserve worktree paths containing newlines, but - // Git <2.36 rejects it. Keep the old parser as a compatibility fallback. - const { stdout } = await gitExecFileAsync(['worktree', 'list', '--porcelain'], { + const capabilities = getLocalGitCapabilityCache({ cwd: repoPath, - ...options + wslDistro: options.wslDistro }) - return normalizeMainWorktreePath(repoPath, parseWorktreeList(stdout), options) + return capabilities.runWithFallback( + 'worktree-list-z', + async () => { + const { stdout } = await gitExecFileAsync(['worktree', 'list', '--porcelain', '-z'], { + cwd: repoPath, + ...options + }) + return normalizeMainWorktreePath( + repoPath, + parseWorktreeList(stdout, { nulDelimited: true }), + options + ) + }, + async () => { + // Why: `-z` is required to preserve worktree paths containing newlines, + // but Git <2.36 rejects it. Keep the line parser as the fallback. + const { stdout } = await gitExecFileAsync(['worktree', 'list', '--porcelain'], { + cwd: repoPath, + ...options + }) + return normalizeMainWorktreePath(repoPath, parseWorktreeList(stdout), options) + }, + isUnsupportedWorktreeListZError + ) } async function readTranslatedWorktreeGraph( @@ -1196,7 +1216,14 @@ async function deleteAlreadyMergedBranchAfterSafeDeleteFailure( // Why: squash merges rewrite commit IDs, so `branch -d` can reject a branch // whose changes are already on the base ref. Delete only when Git can prove // the branch contributes no tree changes to that base. - if (!(await branchHasNoUnmergedChangesOnAnyTarget(runGit, branchName, targetRefs))) { + if ( + !(await branchHasNoUnmergedChangesOnAnyTarget( + runGit, + branchName, + targetRefs, + getLocalGitCapabilityCache({ cwd: repoPath, wslDistro: options.wslDistro }) + )) + ) { return false } await forceDeleteLocalBranch(repoPath, branchName, branchHead, (args, cwd) => diff --git a/src/main/github/conflict-summary.test.ts b/src/main/github/conflict-summary.test.ts index a9b5e9aee9a..4f3df93b13c 100644 --- a/src/main/github/conflict-summary.test.ts +++ b/src/main/github/conflict-summary.test.ts @@ -328,4 +328,21 @@ describe('getPRConflictSummary caching', () => { await expect(deriveSummary()).resolves.toEqual(expectedSummary) expect(spawnCount('merge-base')).toBe(2) }) + + it('does not repeat merge-tree --write-tree after an old-Git rejection', async () => { + mockGitDispatch({ + 'merge-tree': () => + Promise.reject( + Object.assign(new Error('unknown option'), { + stderr: 'usage: git merge-tree ' + }) + ) + }) + + await expect(deriveSummary('head-oid-1')).resolves.toBeUndefined() + await expect(deriveSummary('head-oid-2')).resolves.toBeUndefined() + + expect(spawnCount('merge-base')).toBe(2) + expect(spawnCount('merge-tree')).toBe(1) + }) }) diff --git a/src/main/github/conflict-summary.ts b/src/main/github/conflict-summary.ts index 9644555048a..95e630ccef7 100644 --- a/src/main/github/conflict-summary.ts +++ b/src/main/github/conflict-summary.ts @@ -1,5 +1,10 @@ import type { PRConflictSummary } from '../../shared/types' +import { isUnsupportedMergeTreeWriteTreeError } from '../../shared/git-merge-tree-capability' import { gitExecFileAsync } from '../git/runner' +import { + clearGitCapabilityStateForTests, + getLocalGitCapabilityCache +} from '../git/git-capability-state' import { __resetPRConflictSummaryDerivationCachesForTests, buildConflictSummaryCacheKey, @@ -17,10 +22,8 @@ type LocalGitExecOptions = { wslDistro?: string } -const mergeTreeMergeBaseUnsupportedRuntimes = new Set() - export function __resetPRConflictSummaryCachesForTests(): void { - mergeTreeMergeBaseUnsupportedRuntimes.clear() + clearGitCapabilityStateForTests() __resetPRConflictSummaryDerivationCachesForTests() } @@ -213,7 +216,10 @@ async function loadConflictingFiles( baseOid: string, localGitOptions: LocalGitExecOptions ): Promise { - const capabilityKey = getConflictSummaryGitRuntimeKey(localGitOptions.wslDistro) + const capabilities = getLocalGitCapabilityCache({ + cwd: repoPath, + wslDistro: localGitOptions.wslDistro + }) const modernArgs = [ 'merge-tree', '--write-tree', @@ -235,32 +241,38 @@ async function loadConflictingFiles( baseOid ] - if (mergeTreeMergeBaseUnsupportedRuntimes.has(capabilityKey)) { - return loadConflictingFilesWithLegacyMergeTree(repoPath, legacyArgs, localGitOptions) - } - - try { - const result = await gitExecFileAsync(modernArgs, { - cwd: repoPath, - ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}) - }) - return parseMergeTreeNameOnlyOutput(result.stdout) - } catch (error) { - // Why: `git merge-tree --write-tree` exits with status 1 when it finds - // conflicts, but still writes the conflicted file list to stdout. Treat - // that stdout as the useful result instead of dropping the summary. - const stdoutFromError = getGitErrorOutput(error, 'stdout') - if (stdoutFromError) { - return parseMergeTreeNameOnlyOutput(stdoutFromError) - } - - if (!isUnsupportedMergeBaseOption(error)) { - throw error - } - - mergeTreeMergeBaseUnsupportedRuntimes.add(capabilityKey) - return loadConflictingFilesWithLegacyMergeTree(repoPath, legacyArgs, localGitOptions) - } + return capabilities.runWithFallback( + 'merge-tree-write-tree', + () => + capabilities.runWithFallback( + 'merge-tree-merge-base', + async () => { + try { + const result = await gitExecFileAsync(modernArgs, { + cwd: repoPath, + ...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}) + }) + return parseMergeTreeNameOnlyOutput(result.stdout) + } catch (error) { + // Why: `git merge-tree --write-tree` exits 1 for conflicts but still + // writes the useful file list; only option rejection reaches fallback. + const stdoutFromError = getGitErrorOutput(error, 'stdout') + if (stdoutFromError) { + return parseMergeTreeNameOnlyOutput(stdoutFromError) + } + throw error + } + }, + () => loadConflictingFilesWithLegacyMergeTree(repoPath, legacyArgs, localGitOptions), + isUnsupportedMergeBaseOption + ), + async () => { + // Why: Git before 2.38 cannot derive a reliable real-merge conflict list; + // fail closed without respawning the same rejected command every refresh. + throw new Error('Git merge-tree --write-tree is unavailable on this execution host.') + }, + isUnsupportedMergeTreeWriteTreeError + ) } async function loadConflictingFilesWithLegacyMergeTree( diff --git a/src/main/ipc/repos-remote.test.ts b/src/main/ipc/repos-remote.test.ts index bd2541c7ebd..7a23522ac92 100644 --- a/src/main/ipc/repos-remote.test.ts +++ b/src/main/ipc/repos-remote.test.ts @@ -12,6 +12,7 @@ import { join } from 'node:path' import type * as RepoModule from '../git/repo' import { DEFAULT_REPO_BADGE_COLOR } from '../../shared/constants' import { getGitRepoRoot, isGitRepo } from '../git/repo' +import { clearGitCapabilityStateForTests } from '../git/git-capability-state' const { handleMock, @@ -147,6 +148,10 @@ vi.mock('./ssh', () => ({ import { registerRepoHandlers } from './repos' +beforeEach(() => { + clearGitCapabilityStateForTests() +}) + describe('projectGroups IPC validation', () => { const handlers = new Map unknown>() const mockWindow = { @@ -2698,15 +2703,22 @@ describe('repos:searchBaseRefs SSH relay', () => { query: '', limit: 1 }) + const repeatedResult = await handlers.get('repos:searchBaseRefs')!(null, { + repoId: 'r1', + query: '', + limit: 1 + }) expect(result).toEqual(['origin/main']) + expect(repeatedResult).toEqual(['origin/main']) const forEachRefCalls = mockGitProvider.exec.mock.calls.filter( (call) => (call[0] as string[])[0] === 'for-each-ref' ) - expect(forEachRefCalls).toHaveLength(2) + expect(forEachRefCalls).toHaveLength(3) expect(forEachRefCalls[0][0]).toContain('--exclude=refs/remotes/**/HEAD') expect(forEachRefCalls[1][0]).not.toContain('--exclude=refs/remotes/**/HEAD') expect(forEachRefCalls[1][0]).toContain('--count=104') + expect(forEachRefCalls[2][0]).not.toContain('--exclude=refs/remotes/**/HEAD') }) it('sends the widened `**` argv so all remotes and slash-named branches are discoverable', async () => { diff --git a/src/main/ipc/repos.ts b/src/main/ipc/repos.ts index c5d1c8f2a9c..57225c63c28 100644 --- a/src/main/ipc/repos.ts +++ b/src/main/ipc/repos.ts @@ -74,6 +74,7 @@ import { searchBaseRefDetails } from '../git/repo' import { getSshGitProvider } from '../providers/ssh-git-dispatch' +import { getSshGitCapabilityCache } from '../git/git-capability-state' import { getSshFilesystemProvider } from '../providers/ssh-filesystem-dispatch' import { getSshGitUsername, resolveLocalGitUsername } from '../git/git-username' import { enrichRepoGitUsernames } from '../repo-git-username-enrichment' @@ -2548,32 +2549,33 @@ async function searchBaseRefDetailsForRepo( .split('\n') .map((line) => line.trim()) .filter(Boolean) + const capabilities = getSshGitCapabilityCache(provider) const runSearch = async (patternGroup?: 'segmented' | 'branchRoot'): Promise => { - try { - return ( - await provider.exec( - buildSearchBaseRefsArgv(normalizedQuery, limit, { - remoteNames: remotes, - patternGroup - }), - repo.path - ) - ).stdout - } catch (err) { - if (!isForEachRefExcludeUnsupportedError(err)) { - throw err - } - return ( - await provider.exec( - buildSearchBaseRefsArgv(normalizedQuery, limit, { - excludeRemoteHead: false, - remoteNames: remotes, - patternGroup - }), - repo.path - ) - ).stdout - } + return capabilities.runWithFallback( + 'for-each-ref-exclude', + async () => + ( + await provider.exec( + buildSearchBaseRefsArgv(normalizedQuery, limit, { + remoteNames: remotes, + patternGroup + }), + repo.path + ) + ).stdout, + async () => + ( + await provider.exec( + buildSearchBaseRefsArgv(normalizedQuery, limit, { + excludeRemoteHead: false, + remoteNames: remotes, + patternGroup + }), + repo.path + ) + ).stdout, + isForEachRefExcludeUnsupportedError + ) } // Why: delegate the NUL-parse + HEAD filter + dedup + limit pipeline // to the shared helper so the SSH and local paths cannot diverge. diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index b5fb46e500c..c40fce96418 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -21059,19 +21059,22 @@ describe('OrcaRuntimeService', () => { const runtime = new OrcaRuntimeService(runtimeStore as never) const result = await runtime.searchRepoRefs('id:remote-repo', '', 1) + const repeatedResult = await runtime.searchRepoRefs('id:remote-repo', '', 1) expect(result).toEqual({ refs: ['origin/main'], refDetails: [{ refName: 'origin/main', localBranchName: 'main' }], truncated: true }) + expect(repeatedResult).toEqual(result) const forEachRefCalls = provider.exec.mock.calls.filter( (call) => (call[0] as string[])[0] === 'for-each-ref' ) - expect(forEachRefCalls).toHaveLength(2) + expect(forEachRefCalls).toHaveLength(3) expect(forEachRefCalls[0][0]).toContain('--exclude=refs/remotes/**/HEAD') expect(forEachRefCalls[1][0]).not.toContain('--exclude=refs/remotes/**/HEAD') expect(forEachRefCalls[1][0]).toContain('--count=108') + expect(forEachRefCalls[2][0]).not.toContain('--exclude=refs/remotes/**/HEAD') }) it('resolves SSH worktrees when manually updating lineage', async () => { diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index f420ebd0031..714e5965f85 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -601,6 +601,7 @@ import { import { hasCommitObjectViaGitExec } from '../git/commit-object-ref' import { hasWorktreeBaseCommitRef } from '../git/worktree-base-ref-probe' import { resolveLocalGitUsername } from '../git/git-username' +import { getSshGitCapabilityCache } from '../git/git-capability-state' import { listWorktrees, listWorktreesStrict, @@ -10997,32 +10998,33 @@ export class OrcaRuntimeService { .split('\n') .map((line) => line.trim()) .filter(Boolean) + const capabilities = getSshGitCapabilityCache(provider) const runSearch = async (patternGroup?: 'segmented' | 'branchRoot'): Promise => { - try { - return ( - await provider.exec( - buildSearchBaseRefsArgv(normalizedQuery, limit, { - remoteNames: remotes, - patternGroup - }), - repo.path - ) - ).stdout - } catch (err) { - if (!isForEachRefExcludeUnsupportedError(err)) { - throw err - } - return ( - await provider.exec( - buildSearchBaseRefsArgv(normalizedQuery, limit, { - excludeRemoteHead: false, - remoteNames: remotes, - patternGroup - }), - repo.path - ) - ).stdout - } + return capabilities.runWithFallback( + 'for-each-ref-exclude', + async () => + ( + await provider.exec( + buildSearchBaseRefsArgv(normalizedQuery, limit, { + remoteNames: remotes, + patternGroup + }), + repo.path + ) + ).stdout, + async () => + ( + await provider.exec( + buildSearchBaseRefsArgv(normalizedQuery, limit, { + excludeRemoteHead: false, + remoteNames: remotes, + patternGroup + }), + repo.path + ) + ).stdout, + isForEachRefExcludeUnsupportedError + ) } const searchTokens = normalizedQuery.split('/').filter((token) => token.length > 0) if (searchTokens.length > 1) { diff --git a/src/relay/git-handler-branch-cleanup.ts b/src/relay/git-handler-branch-cleanup.ts index 1454334db25..51aa0be521a 100644 --- a/src/relay/git-handler-branch-cleanup.ts +++ b/src/relay/git-handler-branch-cleanup.ts @@ -3,6 +3,7 @@ import { getBranchCleanupTargetRefs, refreshBranchCleanupTargetRefs } from '../shared/git-branch-cleanup' +import type { GitCapabilityCache } from '../shared/git-capability-cache' import type { GitExec } from './git-handler-ops' import { parseWorktreeList } from './git-handler-utils' @@ -10,7 +11,8 @@ export async function deleteAlreadyMergedRelayBranchAfterSafeDeleteFailure( git: GitExec, repoPath: string, branchName: string, - branchHead: string + branchHead: string, + capabilities: GitCapabilityCache ): Promise { const runGit = (args: string[], options?: { stdin?: string }) => options ? git(args, repoPath, options) : git(args, repoPath) @@ -19,7 +21,9 @@ export async function deleteAlreadyMergedRelayBranchAfterSafeDeleteFailure( // Why: SSH worktrees hit the same squash-merge shape as local worktrees. // Git's no-op merge proof lets us clean up only branches whose changes // already exist on the saved base ref. - if (!(await branchHasNoUnmergedChangesOnAnyTarget(runGit, branchName, targetRefs))) { + if ( + !(await branchHasNoUnmergedChangesOnAnyTarget(runGit, branchName, targetRefs, capabilities)) + ) { return false } await deleteRelayBranchAtExpectedHead(git, repoPath, branchName, branchHead) diff --git a/src/relay/git-handler-local-base-ref-refresh.ts b/src/relay/git-handler-local-base-ref-refresh.ts index 2ff1967a7d3..ae4cfaa6ef3 100644 --- a/src/relay/git-handler-local-base-ref-refresh.ts +++ b/src/relay/git-handler-local-base-ref-refresh.ts @@ -1,9 +1,11 @@ import type { GitExec } from './git-handler-ops' import { areRelayWorktreePathsEqual, readRelayWorktreeList } from './git-handler-worktree-ops' +import { GitCapabilityCache } from '../shared/git-capability-cache' export async function refreshLocalBaseRefForWorktreeCreateOp( git: GitExec, - params: Record + params: Record, + capabilities = new GitCapabilityCache() ): Promise { const repoPath = params.repoPath as string const fullRef = params.fullRef as string @@ -42,7 +44,7 @@ export async function refreshLocalBaseRefForWorktreeCreateOp( throw new Error('Local base ref is not a fast-forward update.') } - const worktrees = await readRelayWorktreeList(git, repoPath) + const worktrees = await readRelayWorktreeList(git, repoPath, capabilities) const ownerWorktree = worktrees.find((worktree) => worktree.branch === fullRef) if (ownerWorktree) { if (ownerWorktreePath && !areRelayWorktreePathsEqual(ownerWorktree.path, ownerWorktreePath)) { diff --git a/src/relay/git-handler-worktree-git-capabilities.test.ts b/src/relay/git-handler-worktree-git-capabilities.test.ts new file mode 100644 index 00000000000..3c5e569cf8e --- /dev/null +++ b/src/relay/git-handler-worktree-git-capabilities.test.ts @@ -0,0 +1,86 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { RelayContext } from './context' +import { GitHandler } from './git-handler' +import { + createMockDispatcher, + type MockDispatcher, + type RelayDispatcher +} from './git-handler-test-setup' + +type GitSpyTarget = { + git(args: string[], cwd: string): Promise<{ stdout: string; stderr: string }> +} + +const WORKTREE_LIST_OUTPUT = `worktree /repo +HEAD abc123 +branch refs/heads/main +` + +describe('relay worktree Git capabilities', () => { + let dispatcher: MockDispatcher + let handler: GitHandler + + beforeEach(() => { + dispatcher = createMockDispatcher() + handler = new GitHandler(dispatcher as unknown as RelayDispatcher, new RelayContext()) + }) + + it('does not repeat a known-unsupported -z probe on later scans', async () => { + const gitSpy = vi + .spyOn(handler as unknown as GitSpyTarget, 'git') + .mockImplementation((args: string[]) => { + if (args.includes('-z')) { + return Promise.reject( + Object.assign(new Error('git usage error'), { + code: 129, + stderr: 'usage: git worktree list []\n' + }) + ) + } + return Promise.resolve({ stdout: WORKTREE_LIST_OUTPUT, stderr: '' }) + }) + + await dispatcher.callRequest('git.listWorktrees', { repoPath: '/repo' }) + await dispatcher.callRequest('git.listWorktrees', { repoPath: '/repo' }) + + expect(gitSpy.mock.calls.map(([args]) => args)).toEqual([ + ['worktree', 'list', '--porcelain', '-z'], + ['worktree', 'list', '--porcelain'], + ['worktree', 'list', '--porcelain'] + ]) + }) + + it('does not repeat a known-unsupported rev-parse --path-format probe', async () => { + const gitSpy = vi + .spyOn(handler as unknown as GitSpyTarget, 'git') + .mockImplementation((args: string[]) => { + if (args[0] === 'worktree') { + return Promise.resolve({ + stdout: 'worktree /git-store/project.git\nHEAD abc123\nbranch refs/heads/main\n', + stderr: '' + }) + } + if (args.includes('--path-format=absolute')) { + return Promise.reject( + Object.assign(new Error('unknown option: --path-format=absolute'), { + stderr: 'error: unknown option `path-format=absolute`\n' + }) + ) + } + return Promise.resolve({ + stdout: '/repo\n/git-store/project.git\n', + stderr: '' + }) + }) + + await dispatcher.callRequest('git.listWorktrees', { repoPath: '/repo' }) + await dispatcher.callRequest('git.listWorktrees', { repoPath: '/repo' }) + + const revParseCalls = gitSpy.mock.calls.filter(([args]) => args[0] === 'rev-parse') + expect(revParseCalls.map(([args]) => args)).toEqual([ + ['rev-parse', '--path-format=absolute', '--show-toplevel', '--git-common-dir'], + ['rev-parse', '--show-toplevel', '--git-common-dir'], + ['rev-parse', '--show-toplevel', '--git-common-dir'] + ]) + }) +}) diff --git a/src/relay/git-handler-worktree-ops.ts b/src/relay/git-handler-worktree-ops.ts index a1fc319446a..34ee8f34afd 100644 --- a/src/relay/git-handler-worktree-ops.ts +++ b/src/relay/git-handler-worktree-ops.ts @@ -2,6 +2,7 @@ import * as path from 'node:path' import { resolveWorktreeAddBaseRef } from '../shared/worktree-base-ref' import type { GitExec } from './git-handler-ops' import { isUnsupportedWorktreeListZError, parseWorktreeList } from './git-handler-utils' +import { GitCapabilityCache } from '../shared/git-capability-cache' export { removeWorktreeOp } from './git-handler-worktree-remove' async function persistRelayWorktreeCreationBase( @@ -119,20 +120,22 @@ type RelayWorktreeInfo = { export async function readRelayWorktreeList( git: GitExec, - repoPath: string + repoPath: string, + capabilities = new GitCapabilityCache() ): Promise { - try { - const { stdout } = await git(['worktree', 'list', '--porcelain', '-z'], repoPath) - return normalizeRelayWorktrees(parseWorktreeList(stdout, { nulDelimited: true })) - } catch (error) { - if (!isUnsupportedWorktreeListZError(error)) { - throw error - } - } - - // Why: `-z` preserves newlines; fallback keeps Git <2.36 compatible. - const { stdout } = await git(['worktree', 'list', '--porcelain'], repoPath) - return normalizeRelayWorktrees(parseWorktreeList(stdout)) + return capabilities.runWithFallback( + 'worktree-list-z', + async () => { + const { stdout } = await git(['worktree', 'list', '--porcelain', '-z'], repoPath) + return normalizeRelayWorktrees(parseWorktreeList(stdout, { nulDelimited: true })) + }, + async () => { + // Why: `-z` preserves newlines; fallback keeps Git <2.36 compatible. + const { stdout } = await git(['worktree', 'list', '--porcelain'], repoPath) + return normalizeRelayWorktrees(parseWorktreeList(stdout)) + }, + isUnsupportedWorktreeListZError + ) } function normalizeRelayWorktrees(worktrees: Record[]): RelayWorktreeInfo[] { diff --git a/src/relay/git-handler-worktree-remove.ts b/src/relay/git-handler-worktree-remove.ts index edd3932ef3b..fbac5b13918 100644 --- a/src/relay/git-handler-worktree-remove.ts +++ b/src/relay/git-handler-worktree-remove.ts @@ -3,6 +3,7 @@ import type { RemoveWorktreeResult } from '../shared/types' import { deleteAlreadyMergedRelayBranchAfterSafeDeleteFailure } from './git-handler-branch-cleanup' import type { GitExec } from './git-handler-ops' import { isUnsupportedWorktreeListZError, parseWorktreeList } from './git-handler-utils' +import { GitCapabilityCache } from '../shared/git-capability-cache' type RelayWorktreeInfo = { path: string @@ -84,24 +85,33 @@ function normalizeRelayWorktrees(worktrees: Record[]): RelayWor .filter((worktree) => worktree.path.length > 0) } -async function readRelayWorktreeList(git: GitExec, repoPath: string): Promise { - try { - const { stdout } = await git(['worktree', 'list', '--porcelain', '-z'], repoPath) - return normalizeRelayWorktrees(parseWorktreeList(stdout, { nulDelimited: true })) - } catch (error) { - if (!isUnsupportedWorktreeListZError(error)) { - throw error - } - } - - // Why: `-z` preserves newlines; fallback keeps Git <2.36 compatible. - const { stdout } = await git(['worktree', 'list', '--porcelain'], repoPath) - return normalizeRelayWorktrees(parseWorktreeList(stdout)) +async function readRelayWorktreeList( + git: GitExec, + repoPath: string, + capabilities: GitCapabilityCache +): Promise { + return capabilities.runWithFallback( + 'worktree-list-z', + async () => { + const { stdout } = await git(['worktree', 'list', '--porcelain', '-z'], repoPath) + return normalizeRelayWorktrees(parseWorktreeList(stdout, { nulDelimited: true })) + }, + async () => { + // Why: `-z` preserves newlines; fallback keeps Git <2.36 compatible. + const { stdout } = await git(['worktree', 'list', '--porcelain'], repoPath) + return normalizeRelayWorktrees(parseWorktreeList(stdout)) + }, + isUnsupportedWorktreeListZError + ) } -async function listRelayWorktreesForRemoval(git: GitExec, repoPath: string) { +async function listRelayWorktreesForRemoval( + git: GitExec, + repoPath: string, + capabilities: GitCapabilityCache +) { try { - return await readRelayWorktreeList(git, repoPath) + return await readRelayWorktreeList(git, repoPath, capabilities) } catch { return [] } @@ -148,7 +158,8 @@ async function deleteRelayBranchAfterWorktreeRemoval( export async function removeWorktreeOp( git: GitExec, - params: Record + params: Record, + capabilities = new GitCapabilityCache() ): Promise { const worktreePath = params.worktreePath as string const force = params.force as boolean | undefined @@ -166,7 +177,7 @@ export async function removeWorktreeOp( // fall through with worktreePath as repo } - const worktreesBeforeRemoval = await listRelayWorktreesForRemoval(git, repoPath) + const worktreesBeforeRemoval = await listRelayWorktreesForRemoval(git, repoPath, capabilities) const removedWorktree = worktreesBeforeRemoval.find((worktree) => areRelayWorktreePathsEqual(worktree.path, worktreePath) ) @@ -210,7 +221,8 @@ export async function removeWorktreeOp( git, repoPath, branchName, - branchHead + branchHead, + capabilities ) ) { return {} diff --git a/src/relay/git-handler.ts b/src/relay/git-handler.ts index c878dfd563e..2e102f33889 100644 --- a/src/relay/git-handler.ts +++ b/src/relay/git-handler.ts @@ -61,6 +61,7 @@ import { getGitCloneFailureMessage } from '../shared/git-clone-failure-message' import { syncForkDefaultBranch, validateGitForkSyncExpectedUpstream } from '../shared/git-fork-sync' import { InFlightPromiseDedupe, stableInFlightKey } from '../shared/in-flight-promise-dedupe' import { GIT_FETCH_SKIP_AUTO_MAINTENANCE_CONFIG_ARGS } from '../shared/git-fetch-auto-maintenance' +import { GitCapabilityCache } from '../shared/git-capability-cache' const execFileAsync = promisify(execFile) const MAX_GIT_BUFFER = 10 * 1024 * 1024 @@ -171,6 +172,7 @@ function execFileWithStdin( export class GitHandler { private dispatcher: RelayDispatcher private readonly gitDiffReadDedupe = new InFlightPromiseDedupe() + private readonly gitCapabilities = new GitCapabilityCache() // Why: configured submodule paths change rarely; an instance-level TTL cache // avoids re-reading `.gitmodules` on every diff click over SSH, and being @@ -1223,23 +1225,29 @@ export class GitHandler { private async readRepoLocation(repoPath: string): Promise { try { - const { stdout } = await this.git( - ['rev-parse', '--path-format=absolute', '--show-toplevel', '--git-common-dir'], - repoPath + return await this.gitCapabilities.runWithFallback( + 'rev-parse-path-format', + async () => { + const { stdout } = await this.git( + ['rev-parse', '--path-format=absolute', '--show-toplevel', '--git-common-dir'], + repoPath + ) + if (stdout.split(/\r?\n/).some((line) => line.startsWith('--path-format'))) { + // Why: some old Git versions echo the unknown option and exit zero; + // remember that signal even though the trailing paths remain usable. + this.gitCapabilities.rememberUnsupported('rev-parse-path-format') + } + return parseRelayRepoLocation(repoPath, stdout) + }, + async () => { + const { stdout } = await this.git( + ['rev-parse', '--show-toplevel', '--git-common-dir'], + repoPath + ) + return parseRelayRepoLocation(repoPath, stdout) + }, + isUnsupportedRevParsePathFormatError ) - return parseRelayRepoLocation(repoPath, stdout) - } catch (error) { - if (!isUnsupportedRevParsePathFormatError(error)) { - return undefined - } - } - - try { - const { stdout } = await this.git( - ['rev-parse', '--show-toplevel', '--git-common-dir'], - repoPath - ) - return parseRelayRepoLocation(repoPath, stdout) } catch { return undefined } @@ -1279,30 +1287,33 @@ export class GitHandler { private async listWorktrees(params: Record, context?: RequestContext) { const repoPath = params.repoPath as string - try { - const { stdout } = await this.git(['worktree', 'list', '--porcelain', '-z'], repoPath, { - signal: context?.signal - }) - return this.normalizeMainWorktreePath( - repoPath, - parseWorktreeList(stdout, { nulDelimited: true }) + return this.gitCapabilities + .runWithFallback( + 'worktree-list-z', + async () => { + const { stdout } = await this.git(['worktree', 'list', '--porcelain', '-z'], repoPath, { + signal: context?.signal + }) + return this.normalizeMainWorktreePath( + repoPath, + parseWorktreeList(stdout, { nulDelimited: true }) + ) + }, + async () => { + // Why: `-z` keeps newline-containing SSH worktree paths intact, but + // Git <2.36 requires the line-block parser. + try { + const { stdout } = await this.git(['worktree', 'list', '--porcelain'], repoPath, { + signal: context?.signal + }) + return this.normalizeMainWorktreePath(repoPath, parseWorktreeList(stdout)) + } catch { + return [] + } + }, + isUnsupportedWorktreeListZError ) - } catch (error) { - if (!isUnsupportedWorktreeListZError(error)) { - return [] - } - } - - // Why: `-z` keeps newline-containing SSH worktree paths intact, but older - // Git rejects it. Fall back to the original line-block parser there. - try { - const { stdout } = await this.git(['worktree', 'list', '--porcelain'], repoPath, { - signal: context?.signal - }) - return this.normalizeMainWorktreePath(repoPath, parseWorktreeList(stdout)) - } catch { - return [] - } + .catch(() => []) } private async addWorktree(params: Record) { @@ -1310,7 +1321,9 @@ export class GitHandler { } private async removeWorktree(params: Record) { - return this.runWithDiffDedupeClear(() => removeWorktreeOp(this.git.bind(this), params)) + return this.runWithDiffDedupeClear(() => + removeWorktreeOp(this.git.bind(this), params, this.gitCapabilities) + ) } private async worktreeIsClean(params: Record) { @@ -1319,7 +1332,7 @@ export class GitHandler { private async refreshLocalBaseRefForWorktreeCreate(params: Record) { return this.runWithDiffDedupeClear(() => - refreshLocalBaseRefForWorktreeCreateOp(this.git.bind(this), params) + refreshLocalBaseRefForWorktreeCreateOp(this.git.bind(this), params, this.gitCapabilities) ) } } diff --git a/src/shared/git-branch-cleanup.test.ts b/src/shared/git-branch-cleanup.test.ts index 6728bf8973e..00be13cdde9 100644 --- a/src/shared/git-branch-cleanup.test.ts +++ b/src/shared/git-branch-cleanup.test.ts @@ -4,6 +4,7 @@ import { refreshBranchCleanupTargetRefs, type GitBranchCleanupExec } from './git-branch-cleanup' +import { GitCapabilityCache } from './git-capability-cache' function baseProofResponses( responses: Partial> = {} @@ -94,7 +95,12 @@ describe('branchHasNoUnmergedChangesOnAnyTarget', () => { const runGit = baseProofResponses() await expect( - branchHasNoUnmergedChangesOnAnyTarget(runGit, 'feature/test', ['refs/remotes/origin/main']) + branchHasNoUnmergedChangesOnAnyTarget( + runGit, + 'feature/test', + ['refs/remotes/origin/main'], + new GitCapabilityCache() + ) ).resolves.toBe(true) expect(runGit).toHaveBeenCalledWith(['patch-id', '--stable'], { stdin: 'branch-diff' }) @@ -106,7 +112,12 @@ describe('branchHasNoUnmergedChangesOnAnyTarget', () => { const runGit = baseProofResponses({ squashPatchId: 'other-patch squash\n' }) await expect( - branchHasNoUnmergedChangesOnAnyTarget(runGit, 'feature/test', ['refs/remotes/origin/main']) + branchHasNoUnmergedChangesOnAnyTarget( + runGit, + 'feature/test', + ['refs/remotes/origin/main'], + new GitCapabilityCache() + ) ).resolves.toBe(false) }) @@ -116,7 +127,12 @@ describe('branchHasNoUnmergedChangesOnAnyTarget', () => { }) await expect( - branchHasNoUnmergedChangesOnAnyTarget(runGit, 'feature/test', ['refs/remotes/origin/main']) + branchHasNoUnmergedChangesOnAnyTarget( + runGit, + 'feature/test', + ['refs/remotes/origin/main'], + new GitCapabilityCache() + ) ).resolves.toBe(false) }) @@ -127,7 +143,12 @@ describe('branchHasNoUnmergedChangesOnAnyTarget', () => { }) await expect( - branchHasNoUnmergedChangesOnAnyTarget(runGit, 'feature/test', ['refs/remotes/origin/main']) + branchHasNoUnmergedChangesOnAnyTarget( + runGit, + 'feature/test', + ['refs/remotes/origin/main'], + new GitCapabilityCache() + ) ).resolves.toBe(false) expect(runGit).not.toHaveBeenCalledWith(['show', '--format=', 'commit-0']) @@ -137,7 +158,40 @@ describe('branchHasNoUnmergedChangesOnAnyTarget', () => { const runGit = baseProofResponses({ branchPatchId: new Error('patch-id failed') }) await expect( - branchHasNoUnmergedChangesOnAnyTarget(runGit, 'feature/test', ['refs/remotes/origin/main']) + branchHasNoUnmergedChangesOnAnyTarget( + runGit, + 'feature/test', + ['refs/remotes/origin/main'], + new GitCapabilityCache() + ) ).resolves.toBe(false) }) + + it('does not repeat a rejected merge-tree --write-tree proof on old Git', async () => { + const unsupported = Object.assign(new Error('unknown option'), { + stderr: 'usage: git merge-tree ' + }) + const runGit = baseProofResponses({ + 'merge-tree --write-tree target refs/heads/feature/test': unsupported, + 'rev-list --right-only --merges --count target...refs/heads/feature/test': '0\n', + 'cherry -v target refs/heads/feature/test': '+ branch-only commit\n' + }) + const capabilities = new GitCapabilityCache() + + await branchHasNoUnmergedChangesOnAnyTarget( + runGit, + 'feature/test', + ['refs/remotes/origin/main'], + capabilities + ) + await branchHasNoUnmergedChangesOnAnyTarget( + runGit, + 'feature/test', + ['refs/remotes/origin/main'], + capabilities + ) + + const mergeTreeCalls = vi.mocked(runGit).mock.calls.filter(([args]) => args[0] === 'merge-tree') + expect(mergeTreeCalls).toHaveLength(1) + }) }) diff --git a/src/shared/git-branch-cleanup.ts b/src/shared/git-branch-cleanup.ts index 3e177bc6cdc..2addd33ec4c 100644 --- a/src/shared/git-branch-cleanup.ts +++ b/src/shared/git-branch-cleanup.ts @@ -1,3 +1,6 @@ +import type { GitCapabilityCache } from './git-capability-cache' +import { isUnsupportedMergeTreeWriteTreeError } from './git-merge-tree-capability' + export type GitBranchCleanupExec = ( argv: string[], options?: { stdin?: string } @@ -102,14 +105,26 @@ async function hasBranchOnlyMergeCommits( async function branchMergesWithoutTreeChanges( runGit: GitBranchCleanupExec, targetOid: string, - branchRef: string + branchRef: string, + capabilities: GitCapabilityCache ): Promise { - const mergedTree = await readOptionalGitStdout(runGit, [ - 'merge-tree', - '--write-tree', - targetOid, - branchRef - ]) + const args = ['merge-tree', '--write-tree', targetOid, branchRef] + const readMergedTree = async (): Promise => { + try { + return await capabilities.runWithFallback( + 'merge-tree-write-tree', + async () => (await runGit(args)).stdout.trim() || null, + async () => null, + isUnsupportedMergeTreeWriteTreeError + ) + } catch { + return null + } + } + const mergedTree = await readMergedTree() + if (!mergedTree) { + return false + } const targetTree = await readOptionalGitStdout(runGit, [ 'rev-parse', '--verify', @@ -159,7 +174,8 @@ async function computeStablePatchId( async function branchNetPatchMatchesTargetSquashCommit( runGit: GitBranchCleanupExec, targetOid: string, - branchRef: string + branchRef: string, + capabilities: GitCapabilityCache ): Promise { const mergeBase = await readOptionalGitStdout(runGit, ['merge-base', targetOid, branchRef]) if (!mergeBase) { @@ -198,7 +214,7 @@ async function branchNetPatchMatchesTargetSquashCommit( // tree merge proves the branch contributes no additional changes there. if ( commitPatchId === branchPatchId && - (await branchMergesWithoutTreeChanges(runGit, commitOid, branchRef)) + (await branchMergesWithoutTreeChanges(runGit, commitOid, branchRef, capabilities)) ) { return true } @@ -209,7 +225,8 @@ async function branchNetPatchMatchesTargetSquashCommit( export async function branchHasNoUnmergedChangesOnAnyTarget( runGit: GitBranchCleanupExec, branchName: string, - targetRefs: string[] + targetRefs: string[], + capabilities: GitCapabilityCache ): Promise { const branchRef = `refs/heads/${branchName}` @@ -218,11 +235,13 @@ export async function branchHasNoUnmergedChangesOnAnyTarget( if (!targetOid) { continue } - if (await branchMergesWithoutTreeChanges(runGit, targetOid, branchRef)) { + if (await branchMergesWithoutTreeChanges(runGit, targetOid, branchRef, capabilities)) { return true } if (await hasBranchOnlyMergeCommits(runGit, targetOid, branchRef)) { - if (await branchNetPatchMatchesTargetSquashCommit(runGit, targetOid, branchRef)) { + if ( + await branchNetPatchMatchesTargetSquashCommit(runGit, targetOid, branchRef, capabilities) + ) { return true } continue diff --git a/src/shared/git-capability-cache.test.ts b/src/shared/git-capability-cache.test.ts new file mode 100644 index 00000000000..14b1f01269b --- /dev/null +++ b/src/shared/git-capability-cache.test.ts @@ -0,0 +1,53 @@ +import { describe, expect, it, vi } from 'vitest' +import { GIT_CAPABILITY_RETRY_INTERVAL_MS, GitCapabilityCache } from './git-capability-cache' + +describe('GitCapabilityCache', () => { + it('retries a capability after the compatibility interval', () => { + const cache = new GitCapabilityCache() + cache.rememberUnsupported('worktree-list-z', 1_000) + + expect(cache.shouldTry('worktree-list-z', 1_000 + GIT_CAPABILITY_RETRY_INTERVAL_MS - 1)).toBe( + false + ) + expect(cache.shouldTry('worktree-list-z', 1_000 + GIT_CAPABILITY_RETRY_INTERVAL_MS)).toBe(true) + }) + + it('coalesces concurrent capability probes after an unsupported result', async () => { + const cache = new GitCapabilityCache() + let rejectProbe!: (error: Error) => void + const firstPreferred = vi.fn( + () => + new Promise((_resolve, reject) => { + rejectProbe = reject + }) + ) + const secondPreferred = vi.fn(async () => 'unexpected') + const firstFallback = vi.fn(async () => 'first-fallback') + const secondFallback = vi.fn(async () => 'second-fallback') + const isUnsupported = (error: unknown): boolean => + error instanceof Error && error.message === 'unsupported' + + const first = cache.runWithFallback( + 'for-each-ref-exclude', + firstPreferred, + firstFallback, + isUnsupported + ) + const second = cache.runWithFallback( + 'for-each-ref-exclude', + secondPreferred, + secondFallback, + isUnsupported + ) + rejectProbe(new Error('unsupported')) + + await expect(Promise.all([first, second])).resolves.toEqual([ + 'first-fallback', + 'second-fallback' + ]) + expect(firstPreferred).toHaveBeenCalledTimes(1) + expect(secondPreferred).not.toHaveBeenCalled() + expect(firstFallback).toHaveBeenCalledTimes(1) + expect(secondFallback).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/shared/git-capability-cache.ts b/src/shared/git-capability-cache.ts new file mode 100644 index 00000000000..9827f11e773 --- /dev/null +++ b/src/shared/git-capability-cache.ts @@ -0,0 +1,101 @@ +// Why: suppress hot-loop failures while still detecting an in-place Git +// upgrade during a long Orca session without requiring a restart. +export const GIT_CAPABILITY_RETRY_INTERVAL_MS = 30 * 60_000 + +export type GitCapability = + | 'for-each-ref-exclude' + | 'merge-tree-merge-base' + | 'merge-tree-write-tree' + | 'rev-parse-path-format' + | 'worktree-list-z' + +type GitCapabilityProbeOutcome = 'supported' | 'unsupported' | 'unknown' + +export class GitCapabilityCache { + private readonly retryAfterByCapability = new Map() + private readonly probesByCapability = new Map>() + + shouldTry(capability: GitCapability, nowMs = Date.now()): boolean { + const retryAfterMs = this.retryAfterByCapability.get(capability) + if (retryAfterMs === undefined) { + return true + } + if (nowMs < retryAfterMs) { + return false + } + this.retryAfterByCapability.delete(capability) + return true + } + + rememberUnsupported(capability: GitCapability, nowMs = Date.now()): void { + // Why: optimistic probes preserve newer Git behavior, but repeating a + // known failure on every poll/search wastes subprocesses and trace space. + this.retryAfterByCapability.set(capability, nowMs + GIT_CAPABILITY_RETRY_INTERVAL_MS) + } + + async runWithFallback( + capability: GitCapability, + runPreferred: () => Promise, + runFallback: () => Promise, + isUnsupportedError: (error: unknown) => boolean + ): Promise { + if (!this.shouldTry(capability)) { + return runFallback() + } + + const inFlightProbe = this.probesByCapability.get(capability) + if (inFlightProbe) { + const outcome = await inFlightProbe + if (outcome === 'unsupported' || !this.shouldTry(capability)) { + return runFallback() + } + return this.runPreferredOrFallback(capability, runPreferred, runFallback, isUnsupportedError) + } + + let settleProbe!: (outcome: GitCapabilityProbeOutcome) => void + const probe = new Promise((resolve) => { + settleProbe = resolve + }) + this.probesByCapability.set(capability, probe) + try { + return await this.runPreferredOrFallback( + capability, + runPreferred, + runFallback, + isUnsupportedError, + settleProbe + ) + } finally { + if (this.probesByCapability.get(capability) === probe) { + this.probesByCapability.delete(capability) + } + } + } + + clear(): void { + this.retryAfterByCapability.clear() + this.probesByCapability.clear() + } + + private async runPreferredOrFallback( + capability: GitCapability, + runPreferred: () => Promise, + runFallback: () => Promise, + isUnsupportedError: (error: unknown) => boolean, + settleProbe?: (outcome: GitCapabilityProbeOutcome) => void + ): Promise { + try { + const result = await runPreferred() + settleProbe?.('supported') + return result + } catch (error) { + if (!isUnsupportedError(error)) { + settleProbe?.('unknown') + throw error + } + this.rememberUnsupported(capability) + settleProbe?.('unsupported') + return runFallback() + } + } +} diff --git a/src/shared/git-merge-tree-capability.ts b/src/shared/git-merge-tree-capability.ts new file mode 100644 index 00000000000..47dc4897dd1 --- /dev/null +++ b/src/shared/git-merge-tree-capability.ts @@ -0,0 +1,18 @@ +function getGitErrorText(error: unknown): string { + if (typeof error !== 'object' || error === null) { + return error instanceof Error ? error.message : String(error) + } + const values = ['message', 'stderr', 'stdout'] + .map((key) => (error as Record)[key]) + .filter((value): value is string => typeof value === 'string') + return values.join('\n') +} + +export function isUnsupportedMergeTreeWriteTreeError(error: unknown): boolean { + const output = getGitErrorText(error) + return ( + /(?:unknown|invalid|unrecognized) option(?::|\s+)[`']?(?:--?)?write-tree[`']?(?:\s|$)/i.test( + output + ) || /usage:\s*git merge-tree\s+\s+\s+/i.test(output) + ) +}