diff --git a/config/scripts/pr-code-change-scope.mjs b/config/scripts/pr-code-change-scope.mjs index 73986f1d7c6..1a5c7ed3a4d 100644 --- a/config/scripts/pr-code-change-scope.mjs +++ b/config/scripts/pr-code-change-scope.mjs @@ -47,6 +47,7 @@ const GIT_COMPAT_PREFIXES = [ '.github/actions/prepare-git-compatibility/', 'src/shared/git-', 'src/shared/review-head-tracking-ref', + 'src/shared/worktree/local-base-branch-fast-forward', 'src/main/git/', 'src/relay/git-', 'config/scripts/git-binary-compatibility' diff --git a/config/scripts/pr-code-change-scope.test.mjs b/config/scripts/pr-code-change-scope.test.mjs index 01cb077b179..fb49cb23cb9 100644 --- a/config/scripts/pr-code-change-scope.test.mjs +++ b/config/scripts/pr-code-change-scope.test.mjs @@ -132,6 +132,12 @@ describe('per-job path classification', () => { expectClassification(['.github/actions/prepare-git-compatibility/action.yml'], { git_compatibility: true }) + // The contract pins the local-main fast-forward's exact arguments. + expectClassification(['src/shared/worktree/local-base-branch-fast-forward.ts'], { + git_compatibility: true, + package: true, + package_windows: true + }) }) it('runs the Codex index-heal contract only when the heal or its transport changes', () => { diff --git a/src/main/git/exact-ref-probe.ts b/src/main/git/exact-ref-probe.ts index ce89fac3f69..b8857a3c1f8 100644 --- a/src/main/git/exact-ref-probe.ts +++ b/src/main/git/exact-ref-probe.ts @@ -1,4 +1,7 @@ import { isSafeGitRefName } from '../../shared/git-status-upstream-ref' +import { isShowRefNoMatchError } from '../../shared/git-show-ref-no-match' + +export { isShowRefNoMatchError } export type ExactRefProbeExecOptions = { maxBuffer?: number @@ -22,23 +25,6 @@ const EXACT_REF_PROBE_CONCURRENCY = 8 // SHA-1 and SHA-256 repositories both report a full object id here. const OBJECT_ID_PATTERN = /^[0-9a-f]{40}(?:[0-9a-f]{24})?$/ -export function isShowRefNoMatchError(error: unknown): boolean { - const record = error && typeof error === 'object' ? (error as Record) : undefined - // Git reports a missing ref as numeric exit status 1. Keep string-valued - // transport/error codes (including a relay that happens to use `"1"`) in - // the unknown bucket so SSH loss cannot look like an absent ref. - if (record?.code !== 1) { - return false - } - // `--quiet` makes Git print nothing for a missing ref, but a wrapper that - // also exits 1 always explains itself: `wsl.exe` on a dead distro, a relay - // transport error. Empty stderr is what separates proven absence from a - // probe that never ran. A runner that reports no stderr at all (the SSH - // provider) keeps its existing exit-code contract. - const stderr = record.stderr - return stderr === undefined || stderr === null || String(stderr).trim().length === 0 -} - function commandOptions(options: ExactRefProbeExecOptions): ExactRefProbeExecOptions | undefined { if (options.maxBuffer === undefined && options.timeoutMs === undefined) { return undefined diff --git a/src/main/git/worktree-add-local-base-refresh.test.ts b/src/main/git/worktree-add-local-base-refresh.test.ts index c5ad0418d27..e8ba08d98a0 100644 --- a/src/main/git/worktree-add-local-base-refresh.test.ts +++ b/src/main/git/worktree-add-local-base-refresh.test.ts @@ -1,13 +1,19 @@ -// addWorktree: fast-forwarding the local base ref (reset --hard / update-ref) and its safety bailouts. +// addWorktree: fast-forwarding the local base ref (merge --ff-only / update-ref) and its safety bailouts. import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -const { gitExecFileAsyncMock, gitExecFileSyncMock, translateWslOutputPathsMock } = vi.hoisted( - () => ({ - gitExecFileAsyncMock: vi.fn(), - gitExecFileSyncMock: vi.fn(), - translateWslOutputPathsMock: vi.fn((output: string) => output) - }) -) +const { + gitExecFileAsyncMock, + refreshGitMock, + checkoutGitMock, + gitExecFileSyncMock, + translateWslOutputPathsMock +} = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn(), + refreshGitMock: vi.fn(), + checkoutGitMock: vi.fn(), + gitExecFileSyncMock: vi.fn(), + translateWslOutputPathsMock: vi.fn((output: string) => output) +})) vi.mock('./runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock, @@ -22,53 +28,59 @@ registerWorktreeSuiteHooks() describe('addWorktree', () => { afterEach(() => vi.restoreAllMocks()) - const resolveCreationBaseConfigWrite = () => { - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base - } - beforeEach(() => { // These branch-safety assertions use POSIX argv; Windows flags have separate coverage. vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') - gitExecFileAsyncMock.mockReset() + // The refresh overlaps `worktree add`, so checkout calls get a fixed fake and the + // (sequential) base-resolution + refresh calls keep their own ordered queue. + refreshGitMock.mockReset() + checkoutGitMock.mockReset().mockImplementation(async (args: string[]) => { + if (args[0] === 'config' && args[1] === '--get') { + throw Object.assign(new Error('key unset'), { code: 1 }) + } + return { stdout: '' } + }) + gitExecFileAsyncMock + .mockReset() + .mockImplementation((args: string[], opts: unknown) => + (args[0] === 'worktree' && args[1] === 'add') || args[0] === 'config' + ? checkoutGitMock(args, opts) + : refreshGitMock(args, opts) + ) gitExecFileSyncMock.mockReset() translateWslOutputPathsMock.mockClear() }) - it('fast-forwards with reset --hard when localBranch is checked out in primary worktree', async () => { + it('fast-forwards with merge --ff-only when localBranch is checked out in primary worktree', async () => { const worktreeListOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n\nworktree /repo-other\nHEAD def456\nbranch refs/heads/feature\n' - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse refs/remotes/origin/main^{commit} - .mockResolvedValueOnce({ stdout: '0\t3\n' }) // rev-list --left-right --count - .mockResolvedValueOnce({ stdout: 'old-main\n' }) // rev-parse refs/heads/main^{commit} - .mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse remote tracking ref^{commit} - .mockResolvedValueOnce({ stdout: '' }) // merge-base captured OIDs + queueBehindInspection() .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list --porcelain .mockResolvedValueOnce({ stdout: '' }) // status --porcelain (in /repo) - .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list recheck - .mockResolvedValueOnce({ stdout: '' }) // status --porcelain recheck (in /repo) - .mockResolvedValueOnce({ stdout: '' }) // reset --hard (in /repo) - .mockResolvedValueOnce({ stdout: '' }) // worktree add - .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base - .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) - .mockResolvedValueOnce({ stdout: '' }) // config --local set push.autoSetupRemote + .mockResolvedValueOnce({ stdout: 'refs/heads/main\n' }) // symbolic-ref -q HEAD (in /repo) + .mockResolvedValueOnce({ stdout: '' }) // merge --ff-only (in /repo) + .mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse refs/heads/main after the merge - await addWorktree('/repo', '/repo-feature', 'feature/test', 'origin/main', true) + const result = await addWorktree('/repo', '/repo-feature', 'feature/test', 'origin/main', true) - expect(gitExecFileAsyncMock.mock.calls).toEqual([ + expect(result.localBaseRefRefresh).toEqual({ + status: 'updated', + baseRef: 'origin/main', + localBranch: 'main', + ownerWorktreePath: '/repo' + }) + expect(refreshGitMock.mock.calls).toEqual([ [['rev-parse', '--verify', '--quiet', 'refs/remotes/origin/main^{commit}'], { cwd: '/repo' }], - [ - ['rev-list', '--left-right', '--count', 'refs/heads/main...refs/remotes/origin/main'], - { cwd: '/repo' } - ], [['rev-parse', '--verify', 'refs/heads/main^{commit}'], { cwd: '/repo' }], [['rev-parse', '--verify', 'refs/remotes/origin/main^{commit}'], { cwd: '/repo' }], - [['merge-base', '--is-ancestor', 'old-main', 'remote-main'], { cwd: '/repo' }], + [['rev-list', '--left-right', '--count', 'old-main...remote-main'], { cwd: '/repo' }], [['worktree', 'list', '--porcelain'], { cwd: '/repo' }], - [['status', '--porcelain', '--untracked-files=no'], { cwd: '/repo' }], - [['worktree', 'list', '--porcelain'], { cwd: '/repo' }], - [['status', '--porcelain', '--untracked-files=no'], { cwd: '/repo' }], - [['reset', '--hard', 'remote-main'], { cwd: '/repo' }], + [['--no-optional-locks', 'status', '--porcelain', '--untracked-files=no'], { cwd: '/repo' }], + [['symbolic-ref', '-q', 'HEAD'], { cwd: '/repo' }], + [ownerFastForwardArgs('remote-main'), { cwd: '/repo' }], + [['rev-parse', '--verify', 'refs/heads/main^{commit}'], { cwd: '/repo' }] + ]) + expect(checkoutGitMock.mock.calls).toEqual([ [ [ 'worktree', @@ -96,55 +108,75 @@ describe('addWorktree', () => { ]) }) - it('fast-forwards with reset --hard in sibling worktree when localBranch is checked out there', async () => { + it('runs worktree add while the local base refresh is still fast-forwarding', async () => { + const worktreeListOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' + let finishMerge!: () => void + let markMergeStarted!: () => void + const mergeStarted = new Promise((resolve) => { + markMergeStarted = resolve + }) + queueBehindInspection() + .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list --porcelain + .mockResolvedValueOnce({ stdout: '' }) // status --porcelain + .mockResolvedValueOnce({ stdout: 'refs/heads/main\n' }) // symbolic-ref -q HEAD + .mockImplementationOnce( + () => + new Promise((resolve) => { + finishMerge = () => resolve({ stdout: '' }) + markMergeStarted() + }) + ) // merge --ff-only, held until worktree add is running + .mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse refs/heads/main after the merge + // Would deadlock if the create awaited the refresh before starting the add. + checkoutGitMock.mockImplementationOnce(async () => { + await mergeStarted + finishMerge() + return { stdout: '' } + }) + + const result = await addWorktree('/repo', '/repo-feature', 'feature/test', 'origin/main', true) + + expect(result.localBaseRefRefresh).toEqual({ + status: 'updated', + baseRef: 'origin/main', + localBranch: 'main', + ownerWorktreePath: '/repo' + }) + expect(checkoutGitMock.mock.calls[0]?.[0]).toContain('add') + }) + + it('fast-forwards in the sibling worktree when localBranch is checked out there', async () => { const worktreeListOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/develop\n\nworktree /repo-main-wt\nHEAD def456\nbranch refs/heads/main\n' - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse refs/remotes/origin/main^{commit} - .mockResolvedValueOnce({ stdout: '0\t3\n' }) // rev-list --left-right --count - .mockResolvedValueOnce({ stdout: 'old-main\n' }) // rev-parse refs/heads/main^{commit} - .mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse remote tracking ref^{commit} - .mockResolvedValueOnce({ stdout: '' }) // merge-base captured OIDs + queueBehindInspection() .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list --porcelain .mockResolvedValueOnce({ stdout: '' }) // status --porcelain (in /repo-main-wt) - .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list recheck - .mockResolvedValueOnce({ stdout: '' }) // status --porcelain recheck (in /repo-main-wt) - .mockResolvedValueOnce({ stdout: '' }) // reset --hard (in /repo-main-wt) - .mockResolvedValueOnce({ stdout: '' }) // worktree add - .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base - .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) - .mockResolvedValueOnce({ stdout: '' }) // config --local set push.autoSetupRemote + .mockResolvedValueOnce({ stdout: 'refs/heads/main\n' }) // symbolic-ref (in /repo-main-wt) + .mockResolvedValueOnce({ stdout: '' }) // merge --ff-only (in /repo-main-wt) + .mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse refs/heads/main after the merge await addWorktree('/repo', '/repo-feature', 'feature/test', 'origin/main', true) - expect(gitExecFileAsyncMock.mock.calls[6]).toEqual([ - ['status', '--porcelain', '--untracked-files=no'], - expect.objectContaining({ cwd: '/repo-main-wt' }) - ]) - expect(gitExecFileAsyncMock.mock.calls[8]).toEqual([ - ['status', '--porcelain', '--untracked-files=no'], - expect.objectContaining({ cwd: '/repo-main-wt' }) - ]) - expect(gitExecFileAsyncMock.mock.calls[9]).toEqual([ - ['reset', '--hard', 'remote-main'], - expect.objectContaining({ cwd: '/repo-main-wt' }) + expect(refreshGitMock.mock.calls.slice(5)).toEqual([ + [ + ['--no-optional-locks', 'status', '--porcelain', '--untracked-files=no'], + expect.objectContaining({ cwd: '/repo-main-wt' }) + ], + [['symbolic-ref', '-q', 'HEAD'], expect.objectContaining({ cwd: '/repo-main-wt' })], + [ownerFastForwardArgs('remote-main'), expect.objectContaining({ cwd: '/repo-main-wt' })], + // Confirms local landed exactly on the target, read from the repo like the inspection. + [ + ['rev-parse', '--verify', 'refs/heads/main^{commit}'], + expect.objectContaining({ cwd: '/repo' }) + ] ]) }) it('fast-forwards local base via update-ref when localBranch is not checked out', async () => { const worktreeListOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/develop\n' - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse refs/remotes/origin/main^{commit} - .mockResolvedValueOnce({ stdout: '0\t3\n' }) // rev-list --left-right --count - .mockResolvedValueOnce({ stdout: 'old-main\n' }) // rev-parse refs/heads/main^{commit} - .mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse remote tracking ref^{commit} - .mockResolvedValueOnce({ stdout: '' }) // merge-base captured OIDs + queueBehindInspection() .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list --porcelain .mockResolvedValueOnce({ stdout: '' }) // update-ref refs/heads/main remote-main old-main - .mockResolvedValueOnce({ stdout: '' }) // worktree add - .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base - .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) - .mockResolvedValueOnce({ stdout: '' }) // config --local set push.autoSetupRemote const result = await addWorktree('/repo', '/repo-feature', 'feature/test', 'origin/main', true) @@ -154,36 +186,36 @@ describe('addWorktree', () => { localBranch: 'main' }) // Compare-and-swap form (expected old OID) so a concurrent ref move is a no-op. - expect(gitExecFileAsyncMock.mock.calls.map((call) => call[0])).toContainEqual([ - 'update-ref', - 'refs/heads/main', - 'remote-main', - 'old-main' - ]) - // No worktree owns the branch, so no working tree is reset. - expect(gitExecFileAsyncMock.mock.calls.map((call) => call[0])).not.toContainEqual([ - 'reset', - '--hard', - 'remote-main' + expect(refreshGitMock.mock.calls.at(-1)).toEqual([ + [ + 'update-ref', + '-m', + 'orca: fast-forward to refs/remotes/origin/main', + 'refs/heads/main', + 'remote-main', + 'old-main' + ], + { cwd: '/repo' } ]) + // No worktree owns the branch, so no working tree is touched. + expect(refreshGitMock.mock.calls.map(([args]) => args)).not.toContainEqual( + ownerFastForwardArgs('remote-main') + ) }) - it('skips local base refresh when the owner worktree becomes dirty before mutation', async () => { + it('reports the owner dirty when the fast-forward refuses to overwrite an edit made after inspection', async () => { const worktreeListOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse refs/remotes/origin/main^{commit} - .mockResolvedValueOnce({ stdout: '0\t3\n' }) // rev-list --left-right --count - .mockResolvedValueOnce({ stdout: 'old-main\n' }) // rev-parse refs/heads/main^{commit} - .mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse remote tracking ref^{commit} - .mockResolvedValueOnce({ stdout: '' }) // merge-base captured OIDs + queueBehindInspection() .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list --porcelain - .mockResolvedValueOnce({ stdout: '' }) // status --porcelain during evaluation - .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list before mutation - .mockResolvedValueOnce({ stdout: ' M package.json\n' }) // status --porcelain before mutation - .mockResolvedValueOnce({ stdout: '' }) // worktree add - .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base - .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) - .mockResolvedValueOnce({ stdout: '' }) // config --local set push.autoSetupRemote + .mockResolvedValueOnce({ stdout: '' }) // status --porcelain during inspection + .mockResolvedValueOnce({ stdout: 'refs/heads/main\n' }) // symbolic-ref -q HEAD + .mockRejectedValueOnce( + Object.assign(new Error('Command failed: git merge'), { + stderr: + 'error: Your local changes to the following files would be overwritten by merge:\n\tpackage.json' + }) + ) // merge --ff-only refuses + .mockRejectedValueOnce(Object.assign(new Error('not ancestor'), { code: 1 })) // merge-base const result = await addWorktree('/repo', '/repo-feature', 'feature/test', 'origin/main', true) @@ -193,71 +225,38 @@ describe('addWorktree', () => { localBranch: 'main', ownerWorktreePath: '/repo' }) - expect(gitExecFileAsyncMock.mock.calls.map((call) => call[0])).not.toContainEqual([ - 'update-ref', - 'refs/heads/main', - 'remote-main', - 'old-main' - ]) - expect(gitExecFileAsyncMock.mock.calls.map((call) => call[0])).not.toContainEqual([ - 'reset', - '--hard', - 'refs/heads/main' + expect(refreshGitMock.mock.calls.at(-1)).toEqual([ + ['merge-base', '--is-ancestor', 'remote-main', 'refs/heads/main'], + expect.objectContaining({ cwd: '/repo' }) ]) + expect(gitExecFileAsyncMock.mock.calls.map(([args]) => args[0])).not.toContain('update-ref') + expect(gitExecFileAsyncMock.mock.calls.map(([args]) => args[0])).not.toContain('reset') }) it('skips local base refresh when the owner worktree switches branches before mutation', async () => { - const firstWorktreeListOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' - const secondWorktreeListOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/develop\n' - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse refs/remotes/origin/main^{commit} - .mockResolvedValueOnce({ stdout: '0\t3\n' }) // rev-list --left-right --count - .mockResolvedValueOnce({ stdout: 'old-main\n' }) // rev-parse refs/heads/main^{commit} - .mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse refs/remotes/origin/main^{commit} - .mockResolvedValueOnce({ stdout: '' }) // merge-base captured OIDs - .mockResolvedValueOnce({ stdout: firstWorktreeListOutput }) // worktree list during evaluation - .mockResolvedValueOnce({ stdout: '' }) // status --porcelain during evaluation - .mockResolvedValueOnce({ stdout: secondWorktreeListOutput }) // worktree list before mutation - .mockResolvedValueOnce({ stdout: '' }) // worktree add - .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base - .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) - .mockResolvedValueOnce({ stdout: '' }) // config --local set push.autoSetupRemote + const worktreeListOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' + queueBehindInspection() + .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list during inspection + .mockResolvedValueOnce({ stdout: '' }) // status --porcelain during inspection + .mockResolvedValueOnce({ stdout: 'refs/heads/develop\n' }) // symbolic-ref: owner switched + .mockRejectedValueOnce(Object.assign(new Error('not ancestor'), { code: 1 })) // merge-base const result = await addWorktree('/repo', '/repo-feature', 'feature/test', 'origin/main', true) expect(result.localBaseRefRefresh).toEqual({ status: 'skipped_error', baseRef: 'origin/main', - localBranch: 'main' + localBranch: 'main', + ownerWorktreePath: '/repo' }) - expect(gitExecFileAsyncMock.mock.calls.map((call) => call[0])).not.toContainEqual([ - 'update-ref', - 'refs/heads/main', - 'remote-main', - 'old-main' - ]) - expect(gitExecFileAsyncMock.mock.calls.map((call) => call[0])).not.toContainEqual([ - 'reset', - '--hard', - 'refs/heads/main' - ]) + expect(gitExecFileAsyncMock.mock.calls.map(([args]) => args)).not.toContainEqual( + ownerFastForwardArgs('remote-main') + ) + expect(gitExecFileAsyncMock.mock.calls.map(([args]) => args[0])).not.toContain('update-ref') }) - it('skips local base refresh when owner revalidation cannot list worktrees', async () => { - const worktreeListOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse refs/remotes/origin/main^{commit} - .mockResolvedValueOnce({ stdout: '0\t3\n' }) // rev-list --left-right --count - .mockResolvedValueOnce({ stdout: 'old-main\n' }) // rev-parse refs/heads/main^{commit} - .mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse refs/remotes/origin/main^{commit} - .mockResolvedValueOnce({ stdout: '' }) // merge-base captured OIDs - .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list during evaluation - .mockResolvedValueOnce({ stdout: '' }) // status --porcelain during evaluation - .mockRejectedValueOnce(new Error('worktree list failed')) // worktree list before mutation - .mockResolvedValueOnce({ stdout: '' }) // worktree add - .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base - .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) - .mockResolvedValueOnce({ stdout: '' }) // config --local set push.autoSetupRemote + it('skips local base refresh when worktree ownership cannot be listed', async () => { + queueBehindInspection().mockRejectedValueOnce(new Error('worktree list failed')) const result = await addWorktree('/repo', '/repo-feature', 'feature/test', 'origin/main', true) @@ -266,28 +265,20 @@ describe('addWorktree', () => { baseRef: 'origin/main', localBranch: 'main' }) - expect(gitExecFileAsyncMock.mock.calls.map((call) => call[0])).not.toContainEqual([ - 'update-ref', - 'refs/heads/main', - 'remote-main', - 'old-main' + expect(refreshGitMock.mock.calls.map(([args]) => args[0])).toEqual([ + 'rev-parse', + 'rev-parse', + 'rev-parse', + 'rev-list', + 'worktree' ]) }) it('skips update when the owning worktree is dirty', async () => { const worktreeListOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse refs/remotes/origin/main^{commit} - .mockResolvedValueOnce({ stdout: '0\t3\n' }) // rev-list --left-right --count - .mockResolvedValueOnce({ stdout: 'old-main\n' }) // rev-parse refs/heads/main^{commit} - .mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse remote tracking ref^{commit} - .mockResolvedValueOnce({ stdout: '' }) // merge-base captured OIDs + queueBehindInspection() .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list --porcelain .mockResolvedValueOnce({ stdout: ' M package.json\n' }) // status --porcelain (dirty) - .mockResolvedValueOnce({ stdout: '' }) // worktree add - .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base - .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) - .mockResolvedValueOnce({ stdout: '' }) // config --local set push.autoSetupRemote const result = await addWorktree('/repo', '/repo-feature', 'feature/test', 'origin/main', true) @@ -298,52 +289,41 @@ describe('addWorktree', () => { ownerWorktreePath: '/repo' }) - // No reset --hard or update-ref — just base resolution, drift check, local/remote - // OIDs, ancestry check, worktree list, status, worktree add, and config writes. - expect(gitExecFileAsyncMock.mock.calls).toHaveLength(11) - expect(gitExecFileAsyncMock.mock.calls[0]?.[0]).toEqual([ + // No merge or update-ref: just base resolution, local/remote OIDs, drift count, worktree + // list and the owner status. + expect(refreshGitMock.mock.calls).toHaveLength(6) + expect(refreshGitMock.mock.calls[0]?.[0]).toEqual([ 'rev-parse', '--verify', '--quiet', 'refs/remotes/origin/main^{commit}' ]) - expect(gitExecFileAsyncMock.mock.calls[7]?.[0]).toEqual([ - 'worktree', - 'add', - '--no-track', - '-b', - 'feature/test', - '/repo-feature', - 'refs/remotes/origin/main' - ]) - expect(gitExecFileAsyncMock.mock.calls[8]?.[0]).toEqual([ - 'config', - '--local', - '--replace-all', - 'branch.feature/test.base', - 'refs/remotes/origin/main' - ]) - expect(gitExecFileAsyncMock.mock.calls[9]?.[0]).toEqual([ - 'config', - '--get', - 'push.autoSetupRemote' - ]) - expect(gitExecFileAsyncMock.mock.calls[10]?.[0]).toEqual([ - 'config', - '--local', - 'push.autoSetupRemote', - 'true' + expect(checkoutGitMock.mock.calls.map((call) => call[0])).toEqual([ + [ + 'worktree', + 'add', + '--no-track', + '-b', + 'feature/test', + '/repo-feature', + 'refs/remotes/origin/main' + ], + [ + 'config', + '--local', + '--replace-all', + 'branch.feature/test.base', + 'refs/remotes/origin/main' + ], + ['config', '--get', 'push.autoSetupRemote'], + ['config', '--local', 'push.autoSetupRemote', 'true'] ]) }) - it('skips updating the local branch when it has diverged', async () => { - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse refs/remotes/origin/main^{commit} - gitExecFileAsyncMock.mockRejectedValueOnce(new Error('not a fast-forward')) - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // show-ref refs/heads/main (exists) - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // worktree add - resolveCreationBaseConfigWrite() - gitExecFileAsyncMock.mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // config --local set push.autoSetupRemote + it('skips updating the local branch when its drift probe fails but the branch exists', async () => { + queueBehindInspection({ counts: new Error('not a fast-forward') }).mockResolvedValueOnce({ + stdout: '' + }) // show-ref refs/heads/main (exists) const result = await addWorktree('/repo', '/repo-feature', 'feature/test', 'origin/main', true) @@ -352,66 +332,47 @@ describe('addWorktree', () => { baseRef: 'origin/main', localBranch: 'main' }) - expect(gitExecFileAsyncMock.mock.calls).toEqual([ + expect(refreshGitMock.mock.calls).toEqual([ [ ['rev-parse', '--verify', '--quiet', 'refs/remotes/origin/main^{commit}'], expect.objectContaining({ cwd: '/repo' }) ], [ - ['rev-list', '--left-right', '--count', 'refs/heads/main...refs/remotes/origin/main'], + ['rev-parse', '--verify', 'refs/heads/main^{commit}'], + expect.objectContaining({ cwd: '/repo' }) + ], + [ + ['rev-parse', '--verify', 'refs/remotes/origin/main^{commit}'], + expect.objectContaining({ cwd: '/repo' }) + ], + [ + ['rev-list', '--left-right', '--count', 'old-main...remote-main'], expect.objectContaining({ cwd: '/repo' }) ], [ ['show-ref', '--verify', '--quiet', '--', 'refs/heads/main'], expect.objectContaining({ cwd: '/repo' }) - ], - [ - [ - 'worktree', - 'add', - '--no-track', - '-b', - 'feature/test', - '/repo-feature', - 'refs/remotes/origin/main' - ], - expect.objectContaining({ cwd: '/repo' }) - ], - [ - [ - 'config', - '--local', - '--replace-all', - 'branch.feature/test.base', - 'refs/remotes/origin/main' - ], - expect.objectContaining({ cwd: '/repo-feature' }) - ], - [ - ['config', '--get', 'push.autoSetupRemote'], - expect.objectContaining({ cwd: '/repo-feature' }) - ], - [ - ['config', '--local', 'push.autoSetupRemote', 'true'], - expect.objectContaining({ cwd: '/repo-feature' }) ] ]) + expect(checkoutGitMock.mock.calls.map((call) => call[0][0])).toEqual([ + 'worktree', + 'config', + 'config', + 'config' + ]) }) - // #15331: evaluation runs before `-b ` exists, so rev-list fails on the missing local ref. + // #15331: `-b feature-x` proves there was no local feature-x to refresh. The add overlaps the + // refresh, so a probe could see the branch absent and then present (the add just wrote it). it('does not warn when worktree add itself creates the local base branch', async () => { - gitExecFileAsyncMock + refreshGitMock .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse --verify --quiet refs/remotes/origin/feature-x^{commit} .mockRejectedValueOnce( new Error( "fatal: ambiguous argument 'refs/heads/feature-x...refs/remotes/origin/feature-x': unknown revision or path not in the working tree." ) - ) // rev-list: refs/heads/feature-x does not exist yet - .mockRejectedValueOnce(Object.assign(new Error('missing ref'), { code: 1 })) // show-ref refs/heads/feature-x (missing) - .mockResolvedValueOnce({ stdout: '' }) // worktree add - .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base - .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) - .mockResolvedValueOnce({ stdout: '' }) // config --local set push.autoSetupRemote + ) // rev-list, if it ran: refs/heads/feature-x not written yet + .mockResolvedValueOnce({ stdout: '' }) // show-ref, if it ran: the concurrent add has written it const result = await addWorktree( '/repo', @@ -431,26 +392,23 @@ describe('addWorktree', () => { '/repo-feature-x', 'refs/remotes/origin/feature-x' ]) - // Nothing was refreshed, so no ref mutation. + // No refresh probe raced the add, and nothing was mutated. + expect(refreshGitMock.mock.calls.map((call) => call[0][0])).toEqual(['rev-parse']) expect(gitExecFileAsyncMock.mock.calls.map((call) => call[0][0])).not.toContain('update-ref') expect(gitExecFileAsyncMock.mock.calls.map((call) => call[0][0])).not.toContain('reset') }) // #15331: same missing-local-branch class, but the new branch name differs from the base's. it('does not warn when the local base branch does not exist in a fetch-only clone', async () => { - gitExecFileAsyncMock + refreshGitMock .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse --verify --quiet refs/remotes/origin/main^{commit} - .mockRejectedValueOnce(new Error('unknown revision refs/heads/main')) // rev-list: no local main + .mockRejectedValueOnce(new Error('unknown revision refs/heads/main')) // rev-parse: no local main .mockRejectedValueOnce(Object.assign(new Error('missing ref'), { code: 1 })) // show-ref refs/heads/main (missing) - .mockResolvedValueOnce({ stdout: '' }) // worktree add - .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base - .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) - .mockResolvedValueOnce({ stdout: '' }) // config --local set push.autoSetupRemote const result = await addWorktree('/repo', '/repo-feature', 'my-feature', 'origin/main', true) expect(result.localBaseRefRefresh).toBeUndefined() - expect(gitExecFileAsyncMock.mock.calls.map((call) => call[0])).toContainEqual([ + expect(refreshGitMock.mock.calls.at(-1)?.[0]).toEqual([ 'show-ref', '--verify', '--quiet', @@ -461,14 +419,10 @@ describe('addWorktree', () => { // A failed probe is not proof of absence, so the warning must survive it. it('keeps the warning when the local base ref probe itself fails', async () => { - gitExecFileAsyncMock + refreshGitMock .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse --verify --quiet refs/remotes/origin/main^{commit} - .mockRejectedValueOnce(new Error('rev-list failed')) // drift probe + .mockRejectedValueOnce(new Error('rev-parse failed')) // local oid probe .mockRejectedValueOnce(new Error('fatal: not a git repository')) // show-ref probe could not run - .mockResolvedValueOnce({ stdout: '' }) // worktree add - .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base - .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) - .mockResolvedValueOnce({ stdout: '' }) // config --local set push.autoSetupRemote const result = await addWorktree('/repo', '/repo-feature', 'my-feature', 'origin/main', true) @@ -480,131 +434,103 @@ describe('addWorktree', () => { }) it('still suggests nothing but keeps the warning when the local base ref exists and diverged', async () => { - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse --verify --quiet refs/remotes/origin/main^{commit} - .mockResolvedValueOnce({ stdout: '2\t3\n' }) // rev-list: 2 local-only commits - .mockResolvedValueOnce({ stdout: '' }) // worktree add - .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base - .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) - .mockResolvedValueOnce({ stdout: '' }) // config --local set push.autoSetupRemote + queueBehindInspection({ counts: { stdout: '2\t3\n' } }) // rev-list: 2 local-only commits - const result = await addWorktree('/repo', '/repo-feature', 'main', 'origin/main', true) + const result = await addWorktree('/repo', '/repo-feature', 'my-feature', 'origin/main', true) - // Same branch name as the base, but rev-list succeeded: real divergence must still warn. + // Local main exists with local-only commits: real divergence must still warn. expect(result.localBaseRefRefresh).toEqual({ status: 'skipped_not_fast_forward', baseRef: 'origin/main', localBranch: 'main' }) + expect(refreshGitMock.mock.calls).toHaveLength(4) }) - it('skips local base refresh when captured OIDs are no longer ancestor-safe', async () => { - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse refs/remotes/origin/main^{commit} - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '0\t2\n' }) // stale rev-list result - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'new-local\n' }) // rev-parse refs/heads/main^{commit} - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse refs/remotes/origin/main^{commit} - gitExecFileAsyncMock.mockRejectedValueOnce(new Error('not an ancestor')) // merge-base captured OIDs - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // show-ref refs/heads/main (exists) - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // worktree add - resolveCreationBaseConfigWrite() - gitExecFileAsyncMock.mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // config --local set push.autoSetupRemote + it('reports not-fast-forward when local gained a commit after the inspection', async () => { + const worktreeListOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' + queueBehindInspection() + .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list --porcelain + .mockResolvedValueOnce({ stdout: '' }) // status --porcelain + .mockResolvedValueOnce({ stdout: 'refs/heads/main\n' }) // symbolic-ref -q HEAD + .mockRejectedValueOnce( + Object.assign(new Error('Command failed: git merge'), { + stderr: 'fatal: Not possible to fast-forward, aborting.' + }) + ) // merge --ff-only refuses to drop the new commit + .mockRejectedValueOnce(Object.assign(new Error('not ancestor'), { code: 1 })) // merge-base const result = await addWorktree('/repo', '/repo-feature', 'feature/test', 'origin/main', true) expect(result.localBaseRefRefresh).toEqual({ status: 'skipped_not_fast_forward', baseRef: 'origin/main', - localBranch: 'main' + localBranch: 'main', + ownerWorktreePath: '/repo' }) - expect(gitExecFileAsyncMock.mock.calls).toEqual([ - [ - ['rev-parse', '--verify', '--quiet', 'refs/remotes/origin/main^{commit}'], - expect.objectContaining({ cwd: '/repo' }) - ], - [ - ['rev-list', '--left-right', '--count', 'refs/heads/main...refs/remotes/origin/main'], - expect.objectContaining({ cwd: '/repo' }) - ], - [ - ['rev-parse', '--verify', 'refs/heads/main^{commit}'], - expect.objectContaining({ cwd: '/repo' }) - ], - [ - ['rev-parse', '--verify', 'refs/remotes/origin/main^{commit}'], - expect.objectContaining({ cwd: '/repo' }) - ], - [ - ['merge-base', '--is-ancestor', 'new-local', 'remote-main'], - expect.objectContaining({ cwd: '/repo' }) - ], - [ - ['show-ref', '--verify', '--quiet', '--', 'refs/heads/main'], - expect.objectContaining({ cwd: '/repo' }) - ], - [ - [ - 'worktree', - 'add', - '--no-track', - '-b', - 'feature/test', - '/repo-feature', - 'refs/remotes/origin/main' - ], - expect.objectContaining({ cwd: '/repo' }) - ], - [ - [ - 'config', - '--local', - '--replace-all', - 'branch.feature/test.base', - 'refs/remotes/origin/main' - ], - expect.objectContaining({ cwd: '/repo-feature' }) - ], - [ - ['config', '--get', 'push.autoSetupRemote'], - expect.objectContaining({ cwd: '/repo-feature' }) - ], - [ - ['config', '--local', 'push.autoSetupRemote', 'true'], - expect.objectContaining({ cwd: '/repo-feature' }) - ] - ]) + expect(gitExecFileAsyncMock.mock.calls.map(([args]) => args[0])).not.toContain('reset') }) it('uses the remote name from the base ref instead of hardcoding origin', async () => { const worktreeListOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' - gitExecFileAsyncMock - .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse refs/remotes/upstream/main^{commit} - .mockResolvedValueOnce({ stdout: '0\t3\n' }) // rev-list --left-right --count - .mockResolvedValueOnce({ stdout: 'old-main\n' }) // rev-parse refs/heads/main^{commit} - .mockResolvedValueOnce({ stdout: 'remote-upstream-main\n' }) // rev-parse refs/remotes/upstream/main^{commit} - .mockResolvedValueOnce({ stdout: '' }) // merge-base captured OIDs + queueBehindInspection({ remoteOid: 'remote-upstream-main' }) .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list --porcelain .mockResolvedValueOnce({ stdout: '' }) // status --porcelain - .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list recheck - .mockResolvedValueOnce({ stdout: '' }) // status --porcelain recheck - .mockResolvedValueOnce({ stdout: '' }) // reset --hard - .mockResolvedValueOnce({ stdout: '' }) // worktree add - .mockResolvedValueOnce({ stdout: '' }) // config --local --replace-all branch..base - .mockRejectedValueOnce(Object.assign(new Error('key unset'), { code: 1 })) // config --get push.autoSetupRemote (unset) - .mockResolvedValueOnce({ stdout: '' }) // config --local set push.autoSetupRemote + .mockResolvedValueOnce({ stdout: 'refs/heads/main\n' }) // symbolic-ref -q HEAD + .mockResolvedValueOnce({ stdout: '' }) // merge --ff-only + .mockResolvedValueOnce({ stdout: 'remote-upstream-main\n' }) // rev-parse refs/heads/main after the merge await addWorktree('/repo', '/repo-feature', 'feature/test', 'upstream/main', true) - expect(gitExecFileAsyncMock.mock.calls[1]?.[0]).toEqual([ + expect(refreshGitMock.mock.calls[2]?.[0]).toEqual([ + 'rev-parse', + '--verify', + 'refs/remotes/upstream/main^{commit}' + ]) + expect(refreshGitMock.mock.calls[3]?.[0]).toEqual([ 'rev-list', '--left-right', '--count', - 'refs/heads/main...refs/remotes/upstream/main' - ]) - expect(gitExecFileAsyncMock.mock.calls[9]?.[0]).toEqual([ - 'reset', - '--hard', - 'remote-upstream-main' + 'old-main...remote-upstream-main' ]) + expect(refreshGitMock.mock.calls[7]?.[0]).toEqual(ownerFastForwardArgs('remote-upstream-main')) }) }) + +function ownerFastForwardArgs(remoteOid: string): string[] { + return [ + '-c', + 'core.hooksPath=/dev/null', + '-c', + 'gc.auto=0', + '-c', + 'maintenance.auto=false', + '-c', + 'merge.autoStash=false', + '-c', + 'branch.main.mergeOptions=', + 'merge', + '--ff-only', + '-s', + 'recursive', + '--no-verify-signatures', + '--no-overwrite-ignore', + '--no-stat', + '-q', + remoteOid + ] +} + +/** Queues base resolution plus the inspection's local oid, remote oid and drift count. */ +function queueBehindInspection( + options: { remoteOid?: string; counts?: { stdout: string } | Error } = {} +) { + const counts = options.counts ?? { stdout: '0\t3\n' } + refreshGitMock + .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse --verify --quiet ^{commit} + .mockResolvedValueOnce({ stdout: 'old-main\n' }) // rev-parse --verify refs/heads/main^{commit} + .mockResolvedValueOnce({ stdout: `${options.remoteOid ?? 'remote-main'}\n` }) // rev-parse --verify ^{commit} + return counts instanceof Error + ? refreshGitMock.mockRejectedValueOnce(counts) + : refreshGitMock.mockResolvedValueOnce(counts) +} diff --git a/src/main/git/worktree-add-local-base-suggestion.test.ts b/src/main/git/worktree-add-local-base-suggestion.test.ts index e17581a55f6..3489a4d05b4 100644 --- a/src/main/git/worktree-add-local-base-suggestion.test.ts +++ b/src/main/git/worktree-add-local-base-suggestion.test.ts @@ -34,10 +34,9 @@ describe('addWorktree', () => { const worktreeListOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse refs/remotes/origin/main^{commit} - .mockResolvedValueOnce({ stdout: '0\t2\n' }) // rev-list --left-right --count .mockResolvedValueOnce({ stdout: 'old-main\n' }) // rev-parse refs/heads/main^{commit} .mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse refs/remotes/origin/main^{commit} - .mockResolvedValueOnce({ stdout: '' }) // merge-base captured OIDs + .mockResolvedValueOnce({ stdout: '0\t2\n' }) // rev-list --left-right --count .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list --porcelain .mockResolvedValueOnce({ stdout: '' }) // status --porcelain .mockResolvedValueOnce({ stdout: '' }) // worktree add @@ -60,16 +59,23 @@ describe('addWorktree', () => { localBranch: 'main', behind: 2 }) - expect(gitExecFileAsyncMock.mock.calls[1]).toEqual([ - ['rev-list', '--left-right', '--count', 'refs/heads/main...refs/remotes/origin/main'], - { cwd: '/repo' } + expect(gitExecFileAsyncMock.mock.calls.slice(1, 6)).toEqual([ + [['rev-parse', '--verify', 'refs/heads/main^{commit}'], { cwd: '/repo' }], + [['rev-parse', '--verify', 'refs/remotes/origin/main^{commit}'], { cwd: '/repo' }], + [['rev-list', '--left-right', '--count', 'old-main...remote-main'], { cwd: '/repo' }], + [['worktree', 'list', '--porcelain'], { cwd: '/repo' }], + [['--no-optional-locks', 'status', '--porcelain', '--untracked-files=no'], { cwd: '/repo' }] ]) + // Advisory only: nothing moves the local branch. + const commands = gitExecFileAsyncMock.mock.calls.map(([args]) => args) + expect(commands.some((args) => args.includes('merge') || args[0] === 'update-ref')).toBe(false) }) it('skips advisory owner probes when the local base is already current', async () => { gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'abc123\n' }) // resolve creation base - .mockResolvedValueOnce({ stdout: '0\t0\n' }) // local base is current + .mockResolvedValueOnce({ stdout: 'same\n' }) // rev-parse refs/heads/main^{commit} + .mockResolvedValueOnce({ stdout: 'same\n' }) // rev-parse refs/remotes/origin/main^{commit}: current .mockResolvedValueOnce({ stdout: '' }) // worktree add .mockResolvedValueOnce({ stdout: '' }) // persist branch base .mockResolvedValueOnce({ stdout: 'true\n' }) // push.autoSetupRemote already set @@ -82,7 +88,8 @@ describe('addWorktree', () => { expect(gitExecFileAsyncMock.mock.calls.map(([args]) => args)).toEqual([ ['rev-parse', '--verify', '--quiet', 'refs/remotes/origin/main^{commit}'], - ['rev-list', '--left-right', '--count', 'refs/heads/main...refs/remotes/origin/main'], + ['rev-parse', '--verify', 'refs/heads/main^{commit}'], + ['rev-parse', '--verify', 'refs/remotes/origin/main^{commit}'], [ 'worktree', 'add', @@ -107,10 +114,9 @@ describe('addWorktree', () => { const worktreeListOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse refs/remotes/foo/bar/main^{commit} - .mockResolvedValueOnce({ stdout: '0\t2\n' }) // rev-list --left-right --count .mockResolvedValueOnce({ stdout: 'old-main\n' }) // rev-parse refs/heads/main^{commit} .mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse refs/remotes/foo/bar/main^{commit} - .mockResolvedValueOnce({ stdout: '' }) // merge-base captured OIDs + .mockResolvedValueOnce({ stdout: '0\t2\n' }) // rev-list --left-right --count .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list --porcelain .mockResolvedValueOnce({ stdout: '' }) // status --porcelain .mockResolvedValueOnce({ stdout: '' }) // worktree add @@ -140,8 +146,8 @@ describe('addWorktree', () => { localBranch: 'main', behind: 2 }) - expect(gitExecFileAsyncMock.mock.calls[1]).toEqual([ - ['rev-list', '--left-right', '--count', 'refs/heads/main...refs/remotes/foo/bar/main'], + expect(gitExecFileAsyncMock.mock.calls[2]).toEqual([ + ['rev-parse', '--verify', 'refs/remotes/foo/bar/main^{commit}'], { cwd: '/repo' } ]) }) @@ -150,10 +156,9 @@ describe('addWorktree', () => { const worktreeListOutput = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n' gitExecFileAsyncMock .mockResolvedValueOnce({ stdout: 'abc123\n' }) // rev-parse refs/remotes/origin/main^{commit} - .mockResolvedValueOnce({ stdout: '0\t2\n' }) // rev-list --left-right --count .mockResolvedValueOnce({ stdout: 'old-main\n' }) // rev-parse refs/heads/main^{commit} - .mockResolvedValueOnce({ stdout: 'remote-upstream-main\n' }) // rev-parse refs/remotes/upstream/main^{commit} - .mockResolvedValueOnce({ stdout: '' }) // merge-base captured OIDs + .mockResolvedValueOnce({ stdout: 'remote-main\n' }) // rev-parse refs/remotes/origin/main^{commit} + .mockResolvedValueOnce({ stdout: '0\t2\n' }) // rev-list --left-right --count .mockResolvedValueOnce({ stdout: worktreeListOutput }) // worktree list --porcelain .mockResolvedValueOnce({ stdout: ' M package.json\n' }) // status --porcelain .mockResolvedValueOnce({ stdout: '' }) // worktree add diff --git a/src/main/git/worktree-add.ts b/src/main/git/worktree-add.ts index f7658f9c47f..acce67288ba 100644 --- a/src/main/git/worktree-add.ts +++ b/src/main/git/worktree-add.ts @@ -10,6 +10,7 @@ import { runWithGitReadCacheInvalidation } from './status' import { invalidateWslLinkedWorktreeGitRouting } from './wsl-linked-worktree-git-routing' import { getLocalBaseRefUpdateSuggestionForWorktreeCreate, + parseRemoteTrackingLocalBaseRef, refreshLocalBaseRefForWorktreeCreate } from './worktree-base-refresh' import { resolveWorktreeBaseCommitOid } from './worktree-base-ref-probe' @@ -22,31 +23,43 @@ import { gitExecOptions, resolveWorktreeAddTimeoutMs } from './worktree-operatio import { bumpWorktreeScanGeneration } from './worktree-scan-cache' import { assertNoPendingWorktreeRemovalConflict } from '../worktree-background-removal' -export type WorktreeAddBaseContext = AddWorktreeResult & { +export type WorktreeAddBaseContext = Pick & { effectiveBase: string effectiveBaseOid?: string + /** Started, not awaited: the worktree is created from the remote-tracking commit, so the refresh can overlap the checkout. Never rejects. */ + pendingLocalBaseRefRefresh?: Promise } export async function resolveWorktreeAddBaseContext( repoPath: string, baseBranch: string, refreshLocalBaseRef: boolean, - options: AddWorktreeOptions + options: AddWorktreeOptions, + createdBranch: string ): Promise { let effectiveBaseOid: string | null = null const effectiveBase = await resolveWorktreeAddBaseRef(baseBranch, async (qualifiedRef) => { effectiveBaseOid = await resolveWorktreeBaseCommitOid(repoPath, qualifiedRef, options) return effectiveBaseOid !== null }) - const localBaseRefRefresh = refreshLocalBaseRef - ? await refreshLocalBaseRefForWorktreeCreate( - repoPath, - baseBranch, - effectiveBase, - options.remoteTrackingBase, - options - ) - : undefined + // Why: `-b` refuses an existing branch, so creating the base's own local branch leaves nothing to refresh; probing it would race the overlapped add's branch write into a false "not fast-forward" warning. + const createsLocalBaseBranch = + parseRemoteTrackingLocalBaseRef(baseBranch, effectiveBase, options.remoteTrackingBase) + ?.localBranch === createdBranch + const pendingLocalBaseRefRefresh = + refreshLocalBaseRef && !createsLocalBaseBranch + ? refreshLocalBaseRefForWorktreeCreate( + repoPath, + baseBranch, + effectiveBase, + options.remoteTrackingBase, + options + ).catch((error: unknown) => { + // Why: the create may already have succeeded by the time this settles; a refresh bug must not fail it. + console.warn('addWorktree: local base ref refresh failed unexpectedly', error) + return undefined + }) + : undefined const localBaseRefUpdateSuggestion = !refreshLocalBaseRef && options.suggestLocalBaseRefUpdate ? await getLocalBaseRefUpdateSuggestionForWorktreeCreate( @@ -63,7 +76,7 @@ export async function resolveWorktreeAddBaseContext( ...(!refreshLocalBaseRef && !options.suggestLocalBaseRefUpdate && effectiveBaseOid ? { effectiveBaseOid } : {}), - ...(localBaseRefRefresh ? { localBaseRefRefresh } : {}), + ...(pendingLocalBaseRefRefresh ? { pendingLocalBaseRefRefresh } : {}), ...(localBaseRefUpdateSuggestion ? { localBaseRefUpdateSuggestion } : {}) } } @@ -188,7 +201,7 @@ async function performAddWorktree( // Why: Git still owns that path and branch until the background delete finishes; a create now // would race it, and the branch cleanup that follows would find the branch checked out again. assertNoPendingWorktreeRemovalConflict(repoPath, { worktreePath, branch }) - let localBaseRefRefresh: LocalBaseRefRefreshResult | undefined + let pendingLocalBaseRefRefresh: Promise | undefined let localBaseRefUpdateSuggestion: LocalBaseRefUpdateSuggestion | undefined // Why: enable long paths for this Windows checkout without changing user Git config. const args = [...windowsLongPathGitArgs(repoPath), 'worktree', 'add'] @@ -207,10 +220,11 @@ async function performAddWorktree( repoPath, baseBranch, refreshLocalBaseRef, - options + options, + branch ) effectiveBase = baseContext.effectiveBase - localBaseRefRefresh = baseContext.localBaseRefRefresh + pendingLocalBaseRefRefresh = baseContext.pendingLocalBaseRefRefresh localBaseRefUpdateSuggestion = baseContext.localBaseRefUpdateSuggestion args.push(effectiveBase) } @@ -221,6 +235,10 @@ async function performAddWorktree( // Why: resolve per call — hoisting this to a module const would freeze the override at import. timeout: resolveWorktreeAddTimeoutMs() }) + } catch (error) { + // Why: settle the overlapped refresh inside the caller's ref-maintenance pause before reporting the failure. + await pendingLocalBaseRefRefresh + throw error } finally { // Git may have written the target's `.git` marker even when it reports a late // failure, so drop any pre-create route before the follow-up commands route. @@ -228,7 +246,7 @@ async function performAddWorktree( } if (options.checkoutExistingBranch) { - return localBaseRefRefresh ? { localBaseRefRefresh } : {} + return {} } if (effectiveBase) { @@ -241,6 +259,7 @@ async function performAddWorktree( // linked worktree writes the shared common-dir config (whole repo) — intentional and idempotent, // so it's warn-only and not rolled back on failure. await configurePushAutoSetupRemote(worktreePath, options) + const localBaseRefRefresh = await pendingLocalBaseRefRefresh return { ...(localBaseRefRefresh ? { localBaseRefRefresh } : {}), ...(localBaseRefUpdateSuggestion ? { localBaseRefUpdateSuggestion } : {}) diff --git a/src/main/git/worktree-base-refresh-analysis.ts b/src/main/git/worktree-base-refresh-analysis.ts deleted file mode 100644 index f79f6f82096..00000000000 --- a/src/main/git/worktree-base-refresh-analysis.ts +++ /dev/null @@ -1,210 +0,0 @@ -import { parseGitRevListAheadBehindCounts } from '../../shared/git-rev-list-output' -import type { - LocalBaseRefRefreshResult, - LocalBaseRefUpdateSuggestion -} from '../../shared/worktree/base-ref-drift-types' -import { gitExecFileAsync, translateWslOutputPaths } from './runner' -import { probeWorktreeBaseRefPresence } from './worktree-base-ref-probe' -import { parseWorktreeList } from '../../shared/git-worktree-porcelain-parser' -import type { AddWorktreeOptions, GitWorktreeExecOptions } from './worktree-operation-options' -import { gitExecOptions } from './worktree-operation-options' - -type LocalBaseRefRefreshability = - | { - refreshable: true - baseRef: string - localBranch: string - fullRef: string - remoteTrackingRef: string - localOid: string - remoteOid: string - behind: number - ownerWorktreePath?: string - } - | { - refreshable: false - result: LocalBaseRefRefreshResult - } - -function parseRemoteTrackingLocalBaseRef( - baseBranch: string, - remoteTrackingRef: string, - remoteTrackingBase?: AddWorktreeOptions['remoteTrackingBase'] -): { baseRef: string; localBranch: string; fullRef: string } | undefined { - if (remoteTrackingBase?.ref === remoteTrackingRef) { - return { - baseRef: remoteTrackingBase.base, - localBranch: remoteTrackingBase.branch, - fullRef: `refs/heads/${remoteTrackingBase.branch}` - } - } - - const remoteRefPrefix = 'refs/remotes/' - if (!remoteTrackingRef.startsWith(remoteRefPrefix)) { - return undefined - } - - // Why: only proven remote-tracking refs get refresh status; slash-containing local branches (release/2026) must not fake a "not refreshed" warning. - const shortRemoteRef = remoteTrackingRef.slice(remoteRefPrefix.length) - const slashIndex = shortRemoteRef.indexOf('/') - if (slashIndex <= 0) { - return undefined - } - - const localBranch = shortRemoteRef.slice(slashIndex + 1) - return { - baseRef: baseBranch, - localBranch, - fullRef: `refs/heads/${localBranch}` - } -} - -function parseRevListDrift(output: string): { ahead: number; behind: number } | null { - const counts = parseGitRevListAheadBehindCounts(output) - return counts.status === 'ok' ? { ahead: counts.ahead, behind: counts.behind } : null -} - -export async function evaluateLocalBaseRefRefreshability( - repoPath: string, - baseBranch: string, - remoteTrackingRef: string, - remoteTrackingBase?: AddWorktreeOptions['remoteTrackingBase'], - options: GitWorktreeExecOptions = {}, - shouldInspectOwner: (behind: number) => boolean = () => true -): Promise { - const parsed = parseRemoteTrackingLocalBaseRef(baseBranch, remoteTrackingRef, remoteTrackingBase) - if (!parsed) { - return undefined - } - - const resultBase = { baseRef: parsed.baseRef, localBranch: parsed.localBranch } - - let drift: { ahead: number; behind: number } - let localOid = '' - let remoteOid = '' - try { - // Why: advisory and mutating paths must agree on "safe to fast-forward"; `rev-list A...B` proves no local-only commits and how far behind. - const { stdout } = await gitExecFileAsync( - ['rev-list', '--left-right', '--count', `${parsed.fullRef}...${remoteTrackingRef}`], - gitExecOptions(repoPath, options) - ) - const parsedDrift = parseRevListDrift(stdout) - if (!parsedDrift || parsedDrift.ahead !== 0) { - return { refreshable: false, result: { ...resultBase, status: 'skipped_not_fast_forward' } } - } - if (!shouldInspectOwner(parsedDrift.behind)) { - // Why: a current local ref yields no update suggestion, so the advisory path skips OID resolution and owner inspection. - return undefined - } - const { stdout: localOidOutput } = await gitExecFileAsync( - ['rev-parse', '--verify', `${parsed.fullRef}^{commit}`], - gitExecOptions(repoPath, options) - ) - localOid = localOidOutput.trim() - if (!localOid) { - return { refreshable: false, result: { ...resultBase, status: 'skipped_not_fast_forward' } } - } - const { stdout: remoteOidOutput } = await gitExecFileAsync( - ['rev-parse', '--verify', `${remoteTrackingRef}^{commit}`], - gitExecOptions(repoPath, options) - ) - remoteOid = remoteOidOutput.trim() - if (!remoteOid) { - return { refreshable: false, result: { ...resultBase, status: 'skipped_not_fast_forward' } } - } - await gitExecFileAsync( - ['merge-base', '--is-ancestor', localOid, remoteOid], - gitExecOptions(repoPath, options) - ) - drift = parsedDrift - } catch { - // Why (#15331): the probes above also fail when refs/heads/ is simply absent; a branch that - // does not exist yet cannot be stale, so report nothing instead of a bogus divergence warning. - // Only a proven absence suppresses: an unusable repo leaves the warning alone. - const presence = await probeWorktreeBaseRefPresence( - (args) => gitExecFileAsync(args, gitExecOptions(repoPath, options)), - parsed.fullRef - ) - if (presence === 'absent') { - return undefined - } - return { refreshable: false, result: { ...resultBase, status: 'skipped_not_fast_forward' } } - } - - try { - // Why: if the local base branch is checked out, only update it when that owner worktree is clean. - const { stdout: worktreeListOutput } = await gitExecFileAsync( - ['worktree', 'list', '--porcelain'], - gitExecOptions(repoPath, options) - ) - const worktrees = parseWorktreeList( - translateWslOutputPaths(worktreeListOutput, repoPath, options) - ) - const ownerWorktree = worktrees.find((wt) => wt.branch === parsed.fullRef) - - if (ownerWorktree) { - const { stdout: status } = await gitExecFileAsync( - ['status', '--porcelain', '--untracked-files=no'], - gitExecOptions(ownerWorktree.path, options) - ) - if (status.trim()) { - return { - refreshable: false, - result: { - ...resultBase, - status: 'skipped_dirty_worktree', - ownerWorktreePath: ownerWorktree.path - } - } - } - return { - refreshable: true, - ...resultBase, - fullRef: parsed.fullRef, - remoteTrackingRef, - localOid, - remoteOid, - behind: drift.behind, - ownerWorktreePath: ownerWorktree.path - } - } - - // Why: localBranch isn't checked out anywhere, so a bare-ref fast-forward is safe; omitting ownerWorktreePath signals the mutating path to take it. - return { - refreshable: true, - ...resultBase, - fullRef: parsed.fullRef, - remoteTrackingRef, - localOid, - remoteOid, - behind: drift.behind - } - } catch { - return { refreshable: false, result: { ...resultBase, status: 'skipped_error' } } - } -} - -export async function getLocalBaseRefUpdateSuggestionForWorktreeCreate( - repoPath: string, - baseBranch: string, - remoteTrackingRef: string, - remoteTrackingBase?: AddWorktreeOptions['remoteTrackingBase'], - options: GitWorktreeExecOptions = {} -): Promise { - const evaluation = await evaluateLocalBaseRefRefreshability( - repoPath, - baseBranch, - remoteTrackingRef, - remoteTrackingBase, - options, - (behind) => behind > 0 - ) - if (!evaluation?.refreshable || evaluation.behind <= 0) { - return undefined - } - return { - baseRef: evaluation.baseRef, - localBranch: evaluation.localBranch, - behind: evaluation.behind - } -} diff --git a/src/main/git/worktree-base-refresh-contention.test.ts b/src/main/git/worktree-base-refresh-contention.test.ts new file mode 100644 index 00000000000..b71e13ba081 --- /dev/null +++ b/src/main/git/worktree-base-refresh-contention.test.ts @@ -0,0 +1,322 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { gitExecFileAsyncMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: + vi.fn<(args: string[], opts: { cwd: string }) => Promise<{ stdout: string }>>() +})) + +vi.mock('./runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock, + translateWslOutputPaths: (output: string) => output +})) + +import { + LOCAL_BASE_REF_REFRESH_WAIT_MS, + refreshLocalBaseRefForWorktreeCreate +} from './worktree-base-refresh' + +const INDEX_LOCK_ERROR = Object.assign(new Error('Command failed: git merge --ff-only'), { + stderr: + "fatal: Unable to create '/repo/.git/index.lock': File exists.\n\nAnother git process seems to be running in this repository" +}) +const OWNER_WORKTREE_LIST = 'worktree /repo\nHEAD old-main\nbranch refs/heads/main\n' + +type GitFake = { + owner?: boolean + mutate: () => Promise<{ stdout: string }> + /** Whether local already contains the target when a failed move is re-checked. */ + localContainsTarget?: () => boolean + ownerHead?: () => string +} + +/** The git subcommand, past leading `-c key=value` and `--flag` globals. */ +function commandOf(args: readonly string[]): string { + for (let index = 0; index < args.length; index += 1) { + if (args[index] === '-c') { + index += 1 + } else if (!args[index].startsWith('-')) { + return args[index] + } + } + return '' +} + +function installGitFake(fake: GitFake): void { + let localMain = 'old-main' + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + const command = commandOf(args) + if (command === 'rev-parse') { + return { stdout: args[2] === 'refs/heads/main^{commit}' ? `${localMain}\n` : 'remote-main\n' } + } + if (command === 'rev-list') { + return { stdout: '0\t3\n' } + } + if (command === 'status') { + return { stdout: '' } + } + if (command === 'worktree') { + return { stdout: fake.owner ? OWNER_WORKTREE_LIST : '' } + } + if (command === 'symbolic-ref') { + return { stdout: `${fake.ownerHead?.() ?? 'refs/heads/main'}\n` } + } + if (command === 'merge-base') { + if (fake.localContainsTarget?.()) { + return { stdout: '' } + } + throw Object.assign(new Error('not an ancestor'), { code: 1 }) + } + if (command === 'merge' || command === 'update-ref') { + const result = await fake.mutate() + localMain = 'remote-main' + return result + } + throw new Error(`unexpected git ${args.join(' ')}`) + }) +} + +function refresh(repoPath = '/repo') { + return refreshLocalBaseRefForWorktreeCreate(repoPath, 'origin/main', 'refs/remotes/origin/main') +} + +function mutationCalls(): string[][] { + return gitExecFileAsyncMock.mock.calls + .map(([args]) => args) + .filter((args) => ['merge', 'update-ref', 'reset'].includes(commandOf(args))) +} + +describe('refreshLocalBaseRefForWorktreeCreate lock contention', () => { + beforeEach(() => { + vi.useFakeTimers() + gitExecFileAsyncMock.mockReset() + }) + afterEach(() => vi.useRealTimers()) + + it('retries a fast-forward that lost the index.lock race and reports updated', async () => { + const mutate = vi + .fn() + .mockRejectedValueOnce(INDEX_LOCK_ERROR) + .mockResolvedValueOnce({ stdout: '' }) + installGitFake({ owner: true, mutate }) + + const pending = refresh() + await vi.runAllTimersAsync() + + await expect(pending).resolves.toEqual({ + baseRef: 'origin/main', + localBranch: 'main', + status: 'updated', + ownerWorktreePath: '/repo' + }) + expect(mutationCalls().map(commandOf)).toEqual(['merge', 'merge']) + expect(mutationCalls().map((args) => args.at(-1))).toEqual(['remote-main', 'remote-main']) + }) + + it('re-confirms the owner has the branch checked out before each retry', async () => { + let symbolicRefReads = 0 + installGitFake({ + owner: true, + mutate: () => Promise.reject(INDEX_LOCK_ERROR), + // The user switches the owner checkout to another branch while the first attempt waits. + ownerHead: () => (++symbolicRefReads >= 2 ? 'refs/heads/develop' : 'refs/heads/main') + }) + + const pending = refresh() + await vi.runAllTimersAsync() + + await expect(pending).resolves.toMatchObject({ status: 'skipped_error' }) + expect(mutationCalls()).toHaveLength(1) + }) + + it('reports updated without a warning when the lock never clears but local already reached the target', async () => { + installGitFake({ + owner: true, + mutate: () => Promise.reject(INDEX_LOCK_ERROR), + // The concurrent lock holder fast-forwarded local itself. + localContainsTarget: () => true + }) + + const pending = refresh() + await vi.runAllTimersAsync() + + await expect(pending).resolves.toMatchObject({ status: 'updated' }) + expect(mutationCalls()).toHaveLength(4) + }) + + it('reports skipped_error when the lock never clears and local is still behind', async () => { + installGitFake({ owner: true, mutate: () => Promise.reject(INDEX_LOCK_ERROR) }) + + const pending = refresh() + await vi.runAllTimersAsync() + + await expect(pending).resolves.toMatchObject({ status: 'skipped_error' }) + expect(mutationCalls()).toHaveLength(4) + }) + + it('does not retry a failure that is not lock contention', async () => { + const casMismatch = Object.assign(new Error('Command failed: git update-ref'), { + stderr: "fatal: cannot lock ref 'refs/heads/main': is at other-oid but expected old-main" + }) + installGitFake({ mutate: () => Promise.reject(casMismatch) }) + + const pending = refresh() + await vi.runAllTimersAsync() + + await expect(pending).resolves.toMatchObject({ status: 'skipped_error' }) + expect(mutationCalls()).toEqual([ + [ + 'update-ref', + '-m', + 'orca: fast-forward to refs/remotes/origin/main', + 'refs/heads/main', + 'remote-main', + 'old-main' + ] + ]) + }) + + it('treats a lost update-ref race as success when someone else moved local to the target', async () => { + const casMismatch = Object.assign(new Error('Command failed: git update-ref'), { + stderr: "fatal: cannot lock ref 'refs/heads/main': is at remote-main but expected old-main" + }) + installGitFake({ mutate: () => Promise.reject(casMismatch), localContainsTarget: () => true }) + + await expect(refresh()).resolves.toEqual({ + baseRef: 'origin/main', + localBranch: 'main', + status: 'updated' + }) + expect(mutationCalls()).toHaveLength(1) + }) + + it('skips the owner check and the move when local is already current', async () => { + installGitFake({ owner: true, mutate: () => Promise.resolve({ stdout: '' }) }) + const base = gitExecFileAsyncMock.getMockImplementation()! + gitExecFileAsyncMock.mockImplementation(async (args: string[], opts: { cwd: string }) => + args[0] === 'rev-parse' ? { stdout: 'remote-main\n' } : base(args, opts) + ) + + await expect(refresh()).resolves.toBeUndefined() + expect(gitExecFileAsyncMock.mock.calls.map(([args]) => args[0])).toEqual([ + 'rev-parse', + 'rev-parse' + ]) + }) + + it("runs the shared refresh without one create's abort signal or timeout", async () => { + installGitFake({ owner: true, mutate: () => Promise.resolve({ stdout: '' }) }) + + await refreshLocalBaseRefForWorktreeCreate( + '/repo', + 'origin/main', + 'refs/remotes/origin/main', + undefined, + { wslDistro: 'Ubuntu', signal: new AbortController().signal, timeout: 8000 } + ) + + for (const [, options] of gitExecFileAsyncMock.mock.calls) { + expect(options).toEqual({ cwd: expect.any(String), wslDistro: 'Ubuntu' }) + } + }) +}) + +describe('refreshLocalBaseRefForWorktreeCreate runs one refresh at a time per branch', () => { + beforeEach(() => { + gitExecFileAsyncMock.mockReset() + }) + afterEach(() => vi.useRealTimers()) + + // Local moves to the target once the (held) owner fast-forward finishes. + function installRepoFake(repoPath: string) { + let localOid = 'old-main' + let finishMerge!: () => void + const mergeStarted = new Promise((resolve) => { + gitExecFileAsyncMock.mockImplementation(async (args: string[], opts: { cwd: string }) => { + const command = commandOf(args) + if (opts.cwd.replace(/\/+$/, '') !== repoPath) { + return { stdout: command === 'rev-parse' ? 'current\n' : '' } + } + if (command === 'rev-parse') { + return { stdout: args[2]?.startsWith('refs/heads/') ? `${localOid}\n` : 'remote-main\n' } + } + if (command === 'rev-list') { + return { stdout: '0\t3\n' } + } + if (command === 'worktree') { + return { stdout: `worktree ${repoPath}\nHEAD ${localOid}\nbranch refs/heads/main\n` } + } + if (command === 'symbolic-ref') { + return { stdout: 'refs/heads/main\n' } + } + if (command === 'merge') { + resolve() + await new Promise((release) => { + finishMerge = release + }) + localOid = 'remote-main' + } + return { stdout: '' } + }) + }) + return { mergeStarted, finishMerge: () => finishMerge() } + } + + it('folds creates that arrive mid-refresh into one follow-up that finds local current', async () => { + const repo = installRepoFake('/repo') + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + const first = refresh('/repo') + const joiners = [refresh('/repo/'), refresh('/repo')] + await repo.mergeStarted + const callsWhileFirstMerges = gitExecFileAsyncMock.mock.calls.length + repo.finishMerge() + + await expect(first).resolves.toMatchObject({ status: 'updated' }) + await expect(Promise.all(joiners)).resolves.toEqual([undefined, undefined]) + const commands = gitExecFileAsyncMock.mock.calls.map(([args]) => commandOf(args)) + // Nothing ran for the joiners while the first held the checkout; after its post-move check, one inspection for all. + expect(commands.slice(0, callsWhileFirstMerges).filter((c) => c === 'rev-list')).toHaveLength(1) + expect(commands.slice(callsWhileFirstMerges)).toEqual(['rev-parse', 'rev-parse', 'rev-parse']) + expect(mutationCalls()).toHaveLength(1) + expect(warn).not.toHaveBeenCalled() + warn.mockRestore() + }) + + it('does not queue refreshes of different repos behind each other', async () => { + const repo = installRepoFake('/repo') + + const first = refresh('/repo') + await repo.mergeStarted + await expect(refresh('/other')).resolves.toBeUndefined() + repo.finishMerge() + await expect(first).resolves.toMatchObject({ status: 'updated' }) + }) + + it('stops waiting on a wedged refresh after the bound without starting a second move', async () => { + vi.useFakeTimers() + const repo = installRepoFake('/repo') + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + let secondResult: unknown = 'pending' + + const first = refresh('/repo') + await repo.mergeStarted + await vi.advanceTimersByTimeAsync(10_000) + const second = refresh('/repo').then((result) => (secondResult = result)) + + await vi.advanceTimersByTimeAsync(LOCAL_BASE_REF_REFRESH_WAIT_MS - 1) + expect(secondResult).toBe('pending') + await vi.advanceTimersByTimeAsync(1) + await second + expect(secondResult).toBeUndefined() + await expect(first).resolves.toBeUndefined() + expect(mutationCalls()).toHaveLength(1) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('stopped waiting')) + + // Settle the wedged run so the follow-up finds local current and frees the slot. + repo.finishMerge() + await vi.runAllTimersAsync() + await vi.waitFor(() => expect(gitExecFileAsyncMock.mock.calls.at(-1)?.[0][0]).toBe('rev-parse')) + expect(mutationCalls()).toHaveLength(1) + warn.mockRestore() + }) +}) diff --git a/src/main/git/worktree-base-refresh-lock-real-git.test.ts b/src/main/git/worktree-base-refresh-lock-real-git.test.ts new file mode 100644 index 00000000000..aaf292810b4 --- /dev/null +++ b/src/main/git/worktree-base-refresh-lock-real-git.test.ts @@ -0,0 +1,212 @@ +import { execFileSync } from 'node:child_process' +import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import * as gitRunner from './runner' +import { refreshLocalBaseRefForWorktreeCreate } from './worktree-base-refresh' + +const tempRoots: string[] = [] + +function git(cwd: string, args: string[]): string { + return execFileSync('git', args, { + cwd, + encoding: 'utf8', + stdio: ['pipe', 'pipe', 'pipe'] + }).trim() +} + +// Primary checkout on `main` one commit behind `refs/remotes/origin/main`, with its index locked. +async function createBehindRepoWithIndexLock(): Promise<{ + repoPath: string + lockPath: string + localOid: string + remoteOid: string +}> { + const root = await mkdtemp(join(tmpdir(), 'orca-base-refresh-lock-')) + tempRoots.push(root) + const repoPath = join(root, 'repo') + execFileSync('git', ['init', '--quiet', repoPath]) + git(repoPath, ['symbolic-ref', 'HEAD', 'refs/heads/main']) + git(repoPath, ['config', 'user.email', 'test@example.com']) + git(repoPath, ['config', 'user.name', 'Test User']) + git(repoPath, ['config', 'core.autocrlf', 'false']) + await writeFile(join(repoPath, 'version.txt'), 'one\n') + git(repoPath, ['add', 'version.txt']) + git(repoPath, ['commit', '--quiet', '-m', 'one']) + const localOid = git(repoPath, ['rev-parse', 'HEAD']) + git(repoPath, ['checkout', '--quiet', '-b', 'upstream']) + await writeFile(join(repoPath, 'version.txt'), 'two\n') + git(repoPath, ['commit', '--quiet', '-am', 'two']) + const remoteOid = git(repoPath, ['rev-parse', 'HEAD']) + git(repoPath, ['checkout', '--quiet', 'main']) + git(repoPath, ['update-ref', 'refs/remotes/origin/main', remoteOid]) + git(repoPath, ['branch', '--quiet', '-D', 'upstream']) + const lockPath = join(repoPath, '.git', 'index.lock') + await writeFile(lockPath, '') + return { repoPath, lockPath, localOid, remoteOid } +} + +function isOwnerFastForward(args: readonly string[]): boolean { + return args.includes('merge') && args.includes('--ff-only') +} + +/** Counts owner fast-forwards; `onFailure` runs after each failed one, before it is reported. */ +function spyOnFastForwards(onFailure: () => Promise | void = () => {}): { + merges: () => number + maxConcurrent: () => number +} { + const original = gitRunner.gitExecFileAsync + let merges = 0 + let active = 0 + let maxConcurrent = 0 + vi.spyOn(gitRunner, 'gitExecFileAsync').mockImplementation(async (args, options) => { + if (!isOwnerFastForward(args)) { + return original(args, options) + } + merges += 1 + active += 1 + maxConcurrent = Math.max(maxConcurrent, active) + try { + return await original(args, options) + } catch (error) { + await onFailure() + throw error + } finally { + active -= 1 + } + }) + return { merges: () => merges, maxConcurrent: () => maxConcurrent } +} + +function refresh(repoPath: string, remote: 'origin' | 'upstream' = 'origin') { + return refreshLocalBaseRefForWorktreeCreate( + repoPath, + `${remote}/main`, + `refs/remotes/${remote}/main` + ) +} + +// A fork: checked-out `main` behind `origin/main`, which is one commit behind `upstream/main`. +async function createForkRepo(): Promise<{ + repoPath: string + originOid: string + upstreamOid: string +}> { + const { repoPath, lockPath, remoteOid: originOid } = await createBehindRepoWithIndexLock() + await rm(lockPath, { force: true }) + git(repoPath, ['checkout', '--quiet', '-b', 'fork-upstream', originOid]) + await writeFile(join(repoPath, 'version.txt'), 'three\n') + git(repoPath, ['commit', '--quiet', '-am', 'three']) + const upstreamOid = git(repoPath, ['rev-parse', 'HEAD']) + git(repoPath, ['checkout', '--quiet', 'main']) + git(repoPath, ['update-ref', 'refs/remotes/upstream/main', upstreamOid]) + git(repoPath, ['branch', '--quiet', '-D', 'fork-upstream']) + return { repoPath, originOid, upstreamOid } +} + +beforeEach(() => { + // Why: a developer's global config (e.g. merge.verifySignatures) must not change what these repos do. + vi.stubEnv('GIT_CONFIG_NOSYSTEM', '1') + vi.stubEnv('GIT_CONFIG_GLOBAL', join(tmpdir(), `orca-no-global-gitconfig-${process.pid}`)) +}) + +afterEach(async () => { + vi.unstubAllEnvs() + vi.restoreAllMocks() + await Promise.all(tempRoots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) +}) + +describe('local base refresh against a held index.lock with real Git', () => { + it('retries once the other git process releases the lock', async () => { + const { repoPath, lockPath, remoteOid } = await createBehindRepoWithIndexLock() + const spy = spyOnFastForwards(() => rm(lockPath, { force: true })) + + const result = await refresh(repoPath) + + expect(result).toMatchObject({ status: 'updated' }) + expect(spy.merges()).toBe(2) + expect(git(repoPath, ['rev-parse', 'main'])).toBe(remoteOid) + expect(await readFile(join(repoPath, 'version.txt'), 'utf8')).toBe('two\n') + }) + + it('reports updated when another process fast-forwarded local while holding the lock', async () => { + const { repoPath, remoteOid } = await createBehindRepoWithIndexLock() + spyOnFastForwards(() => { + git(repoPath, ['update-ref', 'refs/heads/main', remoteOid]) + }) + + const result = await refresh(repoPath) + + expect(result).toMatchObject({ status: 'updated' }) + expect(git(repoPath, ['rev-parse', 'main'])).toBe(remoteOid) + }) + + it('reports skipped_error when the lock outlives every retry', async () => { + const { repoPath, localOid } = await createBehindRepoWithIndexLock() + const spy = spyOnFastForwards() + + const result = await refresh(repoPath) + + expect(result).toMatchObject({ status: 'skipped_error' }) + expect(spy.merges()).toBe(4) + expect(git(repoPath, ['rev-parse', 'main'])).toBe(localOid) + }) +}) + +describe('concurrent local base refreshes of one repo with real Git', () => { + it('runs one fast-forward and resolves every create without a warning', async () => { + const { repoPath, lockPath, remoteOid } = await createBehindRepoWithIndexLock() + await rm(lockPath, { force: true }) + const spy = spyOnFastForwards() + const warn = vi.spyOn(console, 'warn') + + const results = await Promise.all([refresh(repoPath), refresh(repoPath), refresh(repoPath)]) + + expect(results[0]).toMatchObject({ status: 'updated', ownerWorktreePath: expect.any(String) }) + // The joiners share one follow-up run, which finds local already current. + expect(results.slice(1)).toEqual([undefined, undefined]) + expect(spy.merges()).toBe(1) + expect(warn).not.toHaveBeenCalled() + expect(git(repoPath, ['rev-parse', 'main'])).toBe(remoteOid) + }) +}) + +describe('concurrent local base refreshes toward different remotes with real Git', () => { + it('moves local to a target a later create from another remote asked for', async () => { + const { repoPath, upstreamOid } = await createForkRepo() + const spy = spyOnFastForwards() + + const results = await Promise.all([ + refresh(repoPath, 'origin'), + refresh(repoPath, 'upstream'), + refresh(repoPath, 'origin') + ]) + + expect(results[0]).toMatchObject({ baseRef: 'origin/main', status: 'updated' }) + expect(results[1]).toMatchObject({ baseRef: 'upstream/main', status: 'updated' }) + // Local is now ahead of origin/main: the existing ahead-of-requested-remote rule, not a sharing artifact. + expect(results[2]).toMatchObject({ baseRef: 'origin/main', status: 'skipped_not_fast_forward' }) + expect(git(repoPath, ['rev-parse', 'main'])).toBe(upstreamOid) + expect(spy.maxConcurrent()).toBe(1) + }) + + it('never answers a create with the outcome of another remote target', async () => { + const { repoPath, upstreamOid } = await createForkRepo() + const spy = spyOnFastForwards() + + const results = await Promise.all([ + refresh(repoPath, 'upstream'), + refresh(repoPath, 'upstream'), + refresh(repoPath, 'origin') + ]) + + expect(results[0]).toMatchObject({ baseRef: 'upstream/main', status: 'updated' }) + // Local already equals upstream/main, so the second upstream create has nothing to report. + expect(results[1]).toBeUndefined() + // Local is now ahead of origin/main: the existing ahead-of-requested-remote rule, not a sharing artifact. + expect(results[2]).toMatchObject({ baseRef: 'origin/main', status: 'skipped_not_fast_forward' }) + expect(git(repoPath, ['rev-parse', 'main'])).toBe(upstreamOid) + expect(spy.maxConcurrent()).toBe(1) + }) +}) diff --git a/src/main/git/worktree-base-refresh.ts b/src/main/git/worktree-base-refresh.ts index 862bfc1dedc..ba0fe4ffc33 100644 --- a/src/main/git/worktree-base-refresh.ts +++ b/src/main/git/worktree-base-refresh.ts @@ -1,14 +1,61 @@ -import type { LocalBaseRefRefreshResult } from '../../shared/worktree/base-ref-drift-types' -import { gitExecFileAsync, translateWslOutputPaths } from './runner' -import { - evaluateLocalBaseRefRefreshability, - getLocalBaseRefUpdateSuggestionForWorktreeCreate -} from './worktree-base-refresh-analysis' +import type { + LocalBaseRefRefreshResult, + LocalBaseRefUpdateSuggestion +} from '../../shared/worktree/base-ref-drift-types' +import { normalizeRuntimePathForComparison } from '../../shared/cross-platform-path' +import { createCoalescingKeyedRunner } from '../../shared/coalescing-keyed-runner' import { parseWorktreeList } from '../../shared/git-worktree-porcelain-parser' +import { + fastForwardLocalBaseBranch, + inspectLocalBaseBranch, + toLocalBaseRefRefreshResult, + type LocalBaseBranchFastForwardOutcome, + type LocalBaseBranchGit +} from '../../shared/worktree/local-base-branch-fast-forward' +import { gitExecFileAsync, translateWslOutputPaths } from './runner' +import { runWithGitReadCacheInvalidation } from './status' import type { AddWorktreeOptions, GitWorktreeExecOptions } from './worktree-operation-options' -import { gitExecOptions } from './worktree-operation-options' -export { getLocalBaseRefUpdateSuggestionForWorktreeCreate } +// Why: the create reports the refresh result, and a mutating git process has no timeout; past this +// a create stops waiting and reports nothing, while the one in-flight refresh keeps its checkout. +export const LOCAL_BASE_REF_REFRESH_WAIT_MS = 30_000 + +// Why: two runs moving one checkout at once collide on index.lock and misread each other's half-done +// merge; one run per branch at a time, shared only by creates toward the same target, leaves nothing to race. +const runPerLocalBaseBranch = createCoalescingKeyedRunner() + +export function parseRemoteTrackingLocalBaseRef( + baseBranch: string, + remoteTrackingRef: string, + remoteTrackingBase?: AddWorktreeOptions['remoteTrackingBase'] +): { baseRef: string; localBranch: string; fullRef: string } | undefined { + if (remoteTrackingBase?.ref === remoteTrackingRef) { + return { + baseRef: remoteTrackingBase.base, + localBranch: remoteTrackingBase.branch, + fullRef: `refs/heads/${remoteTrackingBase.branch}` + } + } + + const remoteRefPrefix = 'refs/remotes/' + if (!remoteTrackingRef.startsWith(remoteRefPrefix)) { + return undefined + } + + // Why: only proven remote-tracking refs get refresh status; slash-containing local branches (release/2026) must not fake a "not refreshed" warning. + const shortRemoteRef = remoteTrackingRef.slice(remoteRefPrefix.length) + const slashIndex = shortRemoteRef.indexOf('/') + if (slashIndex <= 0) { + return undefined + } + + const localBranch = shortRemoteRef.slice(slashIndex + 1) + return { + baseRef: baseBranch, + localBranch, + fullRef: `refs/heads/${localBranch}` + } +} export async function refreshLocalBaseRefForWorktreeCreate( repoPath: string, @@ -17,60 +64,75 @@ export async function refreshLocalBaseRefForWorktreeCreate( remoteTrackingBase?: AddWorktreeOptions['remoteTrackingBase'], options: GitWorktreeExecOptions = {} ): Promise { - const evaluation = await evaluateLocalBaseRefRefreshability( - repoPath, - baseBranch, - remoteTrackingRef, - remoteTrackingBase, - options - ) - if (!evaluation) { + const parsed = parseRemoteTrackingLocalBaseRef(baseBranch, remoteTrackingRef, remoteTrackingBase) + if (!parsed) { return undefined } - if (!evaluation.refreshable) { - return evaluation.result - } - - const resultBase = { baseRef: evaluation.baseRef, localBranch: evaluation.localBranch } - try { - if (evaluation.ownerWorktreePath) { - const { stdout: worktreeListOutput } = await gitExecFileAsync( - ['worktree', 'list', '--porcelain'], - gitExecOptions(repoPath, options) + const key = `${options.wslDistro ?? ''}\0${normalizeRuntimePathForComparison(repoPath)}\0${parsed.fullRef}` + const git = localBaseBranchGit(options) + const outcome = await waitAtMost( + // Why: the run can outlive this create's wait, so it clears git read caches itself when it moves main. + runPerLocalBaseBranch(key, remoteTrackingRef, () => + runWithGitReadCacheInvalidation(() => + fastForwardLocalBaseBranch(git, { repoPath, fullRef: parsed.fullRef, remoteTrackingRef }) ) - const worktrees = parseWorktreeList( - translateWslOutputPaths(worktreeListOutput, repoPath, options) - ) - const currentOwner = worktrees.find((wt) => wt.branch === evaluation.fullRef) - if (!currentOwner || currentOwner.path !== evaluation.ownerWorktreePath) { - return { ...resultBase, status: 'skipped_error' } - } - const { stdout: status } = await gitExecFileAsync( - ['status', '--porcelain', '--untracked-files=no'], - gitExecOptions(currentOwner.path, options) - ) - if (status.trim()) { - return { - ...resultBase, - status: 'skipped_dirty_worktree', - ownerWorktreePath: currentOwner.path - } - } - await gitExecFileAsync( - ['reset', '--hard', evaluation.remoteOid], - gitExecOptions(currentOwner.path, options) - ) - return { ...resultBase, status: 'updated', ownerWorktreePath: currentOwner.path } - } - - // Why: no owner worktree — fast-forward the bare ref; the expected-old-OID form is a no-op-safe CAS if the ref moved since evaluation. - await gitExecFileAsync( - ['update-ref', evaluation.fullRef, evaluation.remoteOid, evaluation.localOid], - gitExecOptions(repoPath, options) + ), + LOCAL_BASE_REF_REFRESH_WAIT_MS + ) + if (!outcome) { + console.warn( + `addWorktree: stopped waiting for the local ${parsed.localBranch} refresh after ${LOCAL_BASE_REF_REFRESH_WAIT_MS}ms` ) - return { ...resultBase, status: 'updated' } - } catch { - // update-ref/reset can fail on locked refs or odd worktree states; worktree creation should still proceed. - return { ...resultBase, status: 'skipped_error' } + } + return toLocalBaseRefRefreshResult(parsed, outcome) +} + +export async function getLocalBaseRefUpdateSuggestionForWorktreeCreate( + repoPath: string, + baseBranch: string, + remoteTrackingRef: string, + remoteTrackingBase?: AddWorktreeOptions['remoteTrackingBase'], + options: GitWorktreeExecOptions = {} +): Promise { + const parsed = parseRemoteTrackingLocalBaseRef(baseBranch, remoteTrackingRef, remoteTrackingBase) + if (!parsed) { + return undefined + } + const inspection = await inspectLocalBaseBranch(localBaseBranchGit(options), { + repoPath, + fullRef: parsed.fullRef, + remoteTrackingRef + }) + return inspection.status === 'behind' + ? { baseRef: parsed.baseRef, localBranch: parsed.localBranch, behind: inspection.behind } + : undefined +} + +function localBaseBranchGit(options: GitWorktreeExecOptions): LocalBaseBranchGit { + // Why: the run is shared by every create that joins it, so no one create's abort signal or timeout may cut it short. + const execOptions = (cwd: string) => ({ + cwd, + ...(options.wslDistro ? { wslDistro: options.wslDistro } : {}), + ...(options.admissionTier ? { admissionTier: options.admissionTier } : {}) + }) + return { + exec: (args, cwd) => gitExecFileAsync(args, execOptions(cwd)), + listWorktrees: async (repoPath) => { + const { stdout } = await gitExecFileAsync( + ['worktree', 'list', '--porcelain'], + execOptions(repoPath) + ) + return parseWorktreeList(translateWslOutputPaths(stdout, repoPath, options)) + } } } + +function waitAtMost(promise: Promise, timeoutMs: number): Promise { + let timer: ReturnType | undefined + return Promise.race([ + promise, + new Promise((resolve) => { + timer = setTimeout(() => resolve(undefined), timeoutMs) + }) + ]).finally(() => clearTimeout(timer)) +} diff --git a/src/main/git/worktree-create-preparation.ts b/src/main/git/worktree-create-preparation.ts index 28330e64e4f..5a4f65314af 100644 --- a/src/main/git/worktree-create-preparation.ts +++ b/src/main/git/worktree-create-preparation.ts @@ -190,7 +190,8 @@ export async function finalizePreparedWorktree( repoPath, baseBranch, refreshLocalBaseRef, - finalizeGitOptions + finalizeGitOptions, + branch ) const targetHead = baseContext.effectiveBaseOid ?? @@ -211,10 +212,11 @@ export async function finalizePreparedWorktree( if (targetResult.status === 'rejected') { throw targetResult.reason } + const { baseContext, targetHead } = targetResult.value if (preparedResult.status === 'rejected') { + await baseContext.pendingLocalBaseRefRefresh throw preparedResult.reason } - const { baseContext, targetHead } = targetResult.value const preparedHeadOutput = preparedResult.value.stdout if (preparedHeadOutput.trim() !== targetHead) { await gitExecFileAsync( @@ -275,12 +277,13 @@ export async function finalizePreparedWorktree( moved, finalizeGitOptions ) + await baseContext.pendingLocalBaseRefRefresh throw error } + // Why: the refresh overlapped the finalize above; it has no bearing on the checkout's content. + const localBaseRefRefresh = await baseContext.pendingLocalBaseRefRefresh return { - ...(baseContext.localBaseRefRefresh - ? { localBaseRefRefresh: baseContext.localBaseRefRefresh } - : {}), + ...(localBaseRefRefresh ? { localBaseRefRefresh } : {}), ...(baseContext.localBaseRefUpdateSuggestion ? { localBaseRefUpdateSuggestion: baseContext.localBaseRefUpdateSuggestion } : {}) diff --git a/src/main/git/worktree-preparation-base-oid.test.ts b/src/main/git/worktree-preparation-base-oid.test.ts index 5864b9d305a..ecd6f4eab19 100644 --- a/src/main/git/worktree-preparation-base-oid.test.ts +++ b/src/main/git/worktree-preparation-base-oid.test.ts @@ -1,9 +1,11 @@ import { beforeEach, expect, it, vi } from 'vitest' +import type * as WorktreeBaseRefresh from './worktree-base-refresh' const gitExec = vi.hoisted(() => vi.fn()) vi.mock('./runner', () => ({ gitExecFileAsync: gitExec })) -vi.mock('./worktree-base-refresh', () => ({ - refreshLocalBaseRefForWorktreeCreate: vi.fn(), +vi.mock('./worktree-base-refresh', async (importOriginal) => ({ + ...(await importOriginal()), + refreshLocalBaseRefForWorktreeCreate: vi.fn(async () => undefined), getLocalBaseRefUpdateSuggestionForWorktreeCreate: vi.fn() })) vi.mock('./status', () => ({ runWithGitReadCacheInvalidation: (run: () => unknown) => run() })) diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index c360243694d..256cb87fe9f 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -45,10 +45,7 @@ import { getBranchConflictKindViaExec } from '../git/repo-branch-conflict' import { WorktreeCreateCollisionError } from '../../shared/new-workspace/worktree-create-collision' import { resolveLocalGitUsername, getSshGitUsername } from '../git/git-username' import { hasCommitObjectViaGitExec } from '../git/commit-object-ref' -import { - hasLocalWorktreeBaseRef, - probeWorktreeBaseRefPresence -} from '../git/worktree-base-ref-probe' +import { hasLocalWorktreeBaseRef } from '../git/worktree-base-ref-probe' import { resolveWorktreeCreateBase } from '../worktree-create-base' import { resolveWorktreeAddBaseRef } from '../../shared/worktree/base-ref' import { getHostedReviewForBranch } from '../source-control/hosted-review' @@ -170,6 +167,8 @@ import { retireGeneratedWorktreeName } from '../worktree-name-retirement' import { createRetiredNameLookup } from '../../shared/worktree/retired-name-registry' +import { toLocalBaseRefRefreshResult } from '../../shared/worktree/local-base-branch-fast-forward' +import { isSshRequestOutcomeUnverifiable } from '../ssh/ssh-channel-multiplexer' import { findPendingWorktreeRemovalConflict } from '../worktree-background-removal' const SSH_WORKTREE_CREATE_FETCH_FRESHNESS_MS = 30_000 @@ -223,22 +222,6 @@ type StagedStartupResult = { warning?: string } -type RemoteLocalBaseRefRefreshability = - | { - refreshable: true - baseRef: string - localBranch: string - fullRef: string - remoteTrackingRef: string - behind: number - ownerWorktreePath?: string - } - | { - refreshable: false - // undefined = nothing to refresh (no local branch yet), so the caller reports no status at all. - result: LocalBaseRefRefreshResult | undefined - } - function appendWorktreeCreateWarning(current: string | undefined, next: string): string { return current ? `${current} Also ${next[0]?.toLowerCase() ?? ''}${next.slice(1)}` : next } @@ -386,10 +369,6 @@ export function recordWorkspaceLineageForCreatedWorktree( return { lineage, workspaceLineage } } -function countNonEmptyGitOutputLines(output: string): number { - return output.split(/\r?\n/).filter((line) => line.trim().length > 0).length -} - async function spawnLocalStartupAndSetupTerminals(args: { runtime: OrcaRuntimeService | undefined worktree: Pick @@ -1638,121 +1617,28 @@ export async function prefetchRemoteWorktreeCreateBase( await fetchRemoteForWorktreeCreate(provider, repo, 'origin') } +/** Never rejects: the create may already have succeeded when this settles. */ async function refreshLocalBaseRefForRemoteWorktreeCreate( provider: SshGitProvider, repoPath: string, remoteTrackingBase: RemoteTrackingBase ): Promise { - const evaluation = await evaluateRemoteLocalBaseRefRefreshability( - provider, - repoPath, - remoteTrackingBase - ) - if (!evaluation.refreshable) { - return evaluation.result - } - - const resultBase = { baseRef: evaluation.baseRef, localBranch: evaluation.localBranch } + const names = { baseRef: remoteTrackingBase.base, localBranch: remoteTrackingBase.branch } try { - await provider.refreshLocalBaseRefForWorktreeCreate({ + // Why: the relay owns the whole refresh on the execution host, one run per branch at a time. + const outcome = await provider.refreshLocalBaseRefForWorktreeCreate({ repoPath, - fullRef: evaluation.fullRef, - remoteTrackingRef: evaluation.remoteTrackingRef, - ...(evaluation.ownerWorktreePath ? { ownerWorktreePath: evaluation.ownerWorktreePath } : {}) + fullRef: `refs/heads/${remoteTrackingBase.branch}`, + remoteTrackingRef: remoteTrackingBase.ref }) - return { - ...resultBase, - status: 'updated', - ...(evaluation.ownerWorktreePath ? { ownerWorktreePath: evaluation.ownerWorktreePath } : {}) + return toLocalBaseRefRefreshResult(names, outcome) + } catch (error) { + if (isSshRequestOutcomeUnverifiable(error)) { + // Why: the host may still finish it; claiming it failed would be a guess. + console.warn('[worktree-create] local base ref refresh outcome unknown', error) + return undefined } - } catch { - return { ...resultBase, status: 'skipped_error' } - } -} - -async function evaluateRemoteLocalBaseRefRefreshability( - provider: SshGitProvider, - repoPath: string, - remoteTrackingBase: RemoteTrackingBase, - shouldInspectOwner: (behind: number) => boolean = () => true -): Promise { - const resultBase = { - baseRef: remoteTrackingBase.base, - localBranch: remoteTrackingBase.branch - } - const fullRef = `refs/heads/${remoteTrackingBase.branch}` - - let behind = 0 - try { - // Why: SSH generic git.exec is allowlisted — merge-base and log are permitted read-only probes; rev-list is intentionally not exposed. - await provider.exec(['merge-base', '--is-ancestor', fullRef, remoteTrackingBase.ref], repoPath) - const { stdout } = await provider.exec( - ['log', '--format=%H', `${fullRef}..${remoteTrackingBase.ref}`], - repoPath - ) - behind = countNonEmptyGitOutputLines(stdout) - if (!shouldInspectOwner(behind)) { - // Why: no behind commits means no update to advise; skip remote worktree/status round trips. - return { - refreshable: true, - ...resultBase, - fullRef, - remoteTrackingRef: remoteTrackingBase.ref, - behind - } - } - } catch { - // Why (#15331): the probes above also fail when refs/heads/ is simply absent; the relay's - // `worktree add -b` is about to create it, so there is nothing stale to warn about. Only a proven - // absence suppresses: a dropped relay connection is not evidence the branch is missing. - const presence = await probeWorktreeBaseRefPresence( - (args) => provider.exec(args, repoPath), - fullRef - ) - if (presence === 'absent') { - return { refreshable: false, result: undefined } - } - return { refreshable: false, result: { ...resultBase, status: 'skipped_not_fast_forward' } } - } - - try { - const worktrees = await provider.listWorktrees(repoPath) - const ownerWorktree = worktrees.find((wt) => wt.branch === fullRef) - - if (ownerWorktree) { - const status = await provider.worktreeIsClean(ownerWorktree.path, { - includeUntracked: false - }) - if (!status.clean) { - return { - refreshable: false, - result: { - ...resultBase, - status: 'skipped_dirty_worktree', - ownerWorktreePath: ownerWorktree.path - } - } - } - return { - refreshable: true, - ...resultBase, - fullRef, - remoteTrackingRef: remoteTrackingBase.ref, - behind, - ownerWorktreePath: ownerWorktree.path - } - } - - // Why: not checked out anywhere, so a bare-ref fast-forward is safe; omitting ownerWorktreePath tells the relay to update-ref, not reset --hard. - return { - refreshable: true, - ...resultBase, - fullRef, - remoteTrackingRef: remoteTrackingBase.ref, - behind - } - } catch { - return { refreshable: false, result: { ...resultBase, status: 'skipped_error' } } + return toLocalBaseRefRefreshResult(names, { status: 'skipped_error' }) } } @@ -1761,31 +1647,18 @@ async function getRemoteLocalBaseRefUpdateSuggestionForWorktreeCreate( repoPath: string, remoteTrackingBase: RemoteTrackingBase ): Promise { - const evaluation = await evaluateRemoteLocalBaseRefRefreshability( - provider, - repoPath, - remoteTrackingBase, - (behind) => behind > 0 - ) - if (!evaluation.refreshable || evaluation.behind <= 0) { - return undefined - } try { - await provider.refreshLocalBaseRefForWorktreeCreate({ + const behind = await provider.getLocalBaseRefFastForwardableBehind({ repoPath, - fullRef: evaluation.fullRef, - remoteTrackingRef: evaluation.remoteTrackingRef, - ...(evaluation.ownerWorktreePath ? { ownerWorktreePath: evaluation.ownerWorktreePath } : {}), - checkOnly: true + fullRef: `refs/heads/${remoteTrackingBase.branch}`, + remoteTrackingRef: remoteTrackingBase.ref }) + return behind === undefined + ? undefined + : { baseRef: remoteTrackingBase.base, localBranch: remoteTrackingBase.branch, behind } } catch { return undefined } - return { - baseRef: evaluation.baseRef, - localBranch: evaluation.localBranch, - behind: evaluation.behind - } } export function notifyWorktreesChanged(mainWindow: BrowserWindow, repoId: string): void { @@ -2039,9 +1912,14 @@ export async function createRemoteWorktree( } } - const localBaseRefRefresh = - settings.refreshLocalBaseRefOnWorktreeCreate && !checkoutExistingBranch && remoteTrackingBase - ? await refreshLocalBaseRefForRemoteWorktreeCreate(provider, repo.path, remoteTrackingBase) + // Why: started, not awaited — the relay adds from the remote-tracking ref, so the refresh overlaps the checkout. + // A create of the base's own local branch is skipped: `-b` proves it did not exist, and probing it would race the add. + const pendingLocalBaseRefRefresh = + settings.refreshLocalBaseRefOnWorktreeCreate && + !checkoutExistingBranch && + remoteTrackingBase && + remoteTrackingBase.branch !== branchName + ? refreshLocalBaseRefForRemoteWorktreeCreate(provider, repo.path, remoteTrackingBase) : undefined const localBaseRefUpdateSuggestion = !settings.refreshLocalBaseRefOnWorktreeCreate && @@ -2066,6 +1944,7 @@ export async function createRemoteWorktree( // (#17828) instead of paying it at create time for a read-only review. const preparedPushTarget: GitPushTarget | undefined = args.pushTarget + let localBaseRefRefresh: LocalBaseRefRefreshResult | undefined try { await timing.time('git_worktree_add', async () => provider.addWorktree( @@ -2089,6 +1968,9 @@ export async function createRemoteWorktree( ) } throw err + } finally { + // Why: a sparse rollback below must not race a refresh still moving the owner checkout. + localBaseRefRefresh = await pendingLocalBaseRefRefresh } // Why: the worktree is listable from here on; a scan that began before it appeared is overtaken. runWorktreeChangeInvalidators(repo.id) diff --git a/src/main/ipc/worktrees-ssh-local-base-refresh-overlap.test.ts b/src/main/ipc/worktrees-ssh-local-base-refresh-overlap.test.ts new file mode 100644 index 00000000000..59794f82ba7 --- /dev/null +++ b/src/main/ipc/worktrees-ssh-local-base-refresh-overlap.test.ts @@ -0,0 +1,309 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { getSshGitProviderMock, getActiveMultiplexerMock } from './worktrees-test-module-mocks' +import { handlers, setupWorktreeHandlers, store } from './worktrees-test-harness' +import { SSH_MUX_REQUEST_TIMEOUT_CODE } from '../ssh/ssh-channel-multiplexer' + +vi.mock('electron', async () => + (await import('./worktrees-test-module-mocks')).electronModuleMock() +) +vi.mock('../git/worktree', async () => + (await import('./worktrees-test-module-mocks')).gitWorktreeModuleMock() +) +vi.mock('../git/runner', async () => + (await import('./worktrees-test-module-mocks')).gitRunnerModuleMock() +) +vi.mock('../git/repo', async () => + (await import('./worktrees-test-module-mocks')).gitRepoModuleMock() +) +vi.mock('../git/git-username', async (importOriginal) => ({ + ...(await importOriginal>()), + resolveLocalGitUsername: (await import('./worktrees-test-module-mocks')) + .resolveLocalGitUsernameMock +})) +vi.mock('../github/client', async () => + (await import('./worktrees-test-module-mocks')).githubClientModuleMock() +) +vi.mock('../source-control/hosted-review', async () => + (await import('./worktrees-test-module-mocks')).hostedReviewModuleMock() +) +vi.mock('../providers/ssh-git-dispatch', async () => + (await import('./worktrees-test-module-mocks')).sshGitDispatchModuleMock() +) +vi.mock('../providers/ssh-filesystem-dispatch', async () => + (await import('./worktrees-test-module-mocks')).sshFilesystemDispatchModuleMock() +) +vi.mock('./worktree-symlinks', async () => + (await import('./worktrees-test-module-mocks')).worktreeSymlinksModuleMock() +) +vi.mock('./ssh', async () => (await import('./worktrees-test-module-mocks')).sshModuleMock()) +vi.mock('../ssh/ssh-target-registry', async () => + (await import('./worktrees-test-module-mocks')).sshTargetRegistryModuleMock() +) +vi.mock('../hooks', async () => (await import('./worktrees-test-module-mocks')).hooksModuleMock()) +vi.mock('../setup-runner-script-text', async (importOriginal) => + (await import('./worktrees-test-module-mocks')).setupRunnerScriptTextModuleMock( + await importOriginal>() + ) +) +vi.mock('../worktree-runner-script', async (importOriginal) => + (await import('./worktrees-test-module-mocks')).worktreeRunnerScriptModuleMock( + await importOriginal>() + ) +) +vi.mock('../effective-hook-config', async (importOriginal) => + (await import('./worktrees-test-module-mocks')).effectiveHookConfigModuleMock( + await importOriginal>() + ) +) +vi.mock('../setup-hook-env-vars', async (importOriginal) => + (await import('./worktrees-test-module-mocks')).setupHookEnvVarsModuleMock( + await importOriginal>() + ) +) +vi.mock('./worktree-logic', async (importOriginal) => + (await import('./worktrees-test-module-mocks')).worktreeLogicModuleMock( + await importOriginal>() + ) +) +vi.mock('../terminal-history-deletion', async () => + (await import('./worktrees-test-module-mocks')).terminalHistoryDeletionModuleMock() +) +vi.mock('../ports/advertised-url-watcher', async () => + (await import('./worktrees-test-module-mocks')).advertisedUrlWatcherModuleMock() +) +vi.mock('../workspace-cleanup-scan-snapshot', async () => + (await import('./worktrees-test-module-mocks')).workspaceCleanupScanSnapshotModuleMock() +) +vi.mock('../workspace-space-analysis-snapshot', async () => + (await import('./worktrees-test-module-mocks')).workspaceSpaceAnalysisSnapshotModuleMock() +) +vi.mock('../workspace-cleanup-removal-snapshot-prune', async () => + (await import('./worktrees-test-module-mocks')).workspaceCleanupRemovalSnapshotPruneModuleMock() +) +vi.mock('../runtime/worktree-teardown', async () => + (await import('./worktrees-test-module-mocks')).worktreeTeardownModuleMock() +) +vi.mock('./pty', async () => (await import('./worktrees-test-module-mocks')).ptyModuleMock()) + +const REPO = { + id: 'repo-ssh', + path: '/remote/repo', + displayName: 'ssh', + badgeColor: '#000', + addedAt: 0, + connectionId: 'conn-1', + worktreeBaseRef: 'origin/main' +} +const REFRESH_UPDATED = { status: 'updated', ownerWorktreePath: '/remote/repo' } +const UPDATED = { + status: 'updated', + baseRef: 'origin/main', + localBranch: 'main', + ownerWorktreePath: '/remote/repo' +} + +function createProvider(overrides: { + refreshLocalBaseRefForWorktreeCreate: ReturnType + addWorktree?: ReturnType +}) { + return { + exec: vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'origin\n', stderr: '' } + } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing remote ref'), { code: 1 }) + } + return { stdout: '', stderr: '' } + }), + fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined), + addWorktree: overrides.addWorktree ?? vi.fn().mockResolvedValue(undefined), + listWorktrees: vi.fn().mockResolvedValue([ + { + path: '/remote/repo-improve-dashboard', + head: 'remote-main', + branch: 'refs/heads/improve-dashboard', + isBare: false, + isMainWorktree: false + } + ]), + worktreeIsClean: vi.fn().mockResolvedValue({ clean: true }), + refreshLocalBaseRefForWorktreeCreate: overrides.refreshLocalBaseRefForWorktreeCreate + } +} + +async function createWith( + provider: ReturnType, + request: { repo?: typeof REPO; name?: string } = {} +) { + const repo = request.repo ?? REPO + store.getSettings.mockReturnValue({ + branchPrefix: 'none', + nestWorkspaces: false, + refreshLocalBaseRefOnWorktreeCreate: true, + workspaceDir: '/workspace' + }) + store.getRepos.mockReturnValue([repo]) + store.getRepo.mockReturnValue(repo) + getSshGitProviderMock.mockReturnValue(provider) + getActiveMultiplexerMock.mockReturnValue({ + request: vi.fn().mockResolvedValue(undefined), + notify: vi.fn() + }) + store.setWorktreeMeta.mockImplementation((_worktreeId, meta) => meta) + const result: unknown = await handlers['worktrees:create'](null, { + repoId: 'repo-ssh', + name: request.name ?? 'improve-dashboard' + }) + return result +} + +describe('SSH local base refresh failures', () => { + beforeEach(() => { + setupWorktreeHandlers() + }) + + // The relay may still finish the refresh; reporting it failed would be a guess. + it.each([ + ['the request timed out', { code: SSH_MUX_REQUEST_TIMEOUT_CODE }], + ['the connection was lost', { code: 'CONNECTION_LOST' }] + ])('reports no refresh status when %s', async (_case, fields) => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const refresh = vi.fn(async () => { + throw Object.assign(new Error('relay did not answer'), fields) + }) + + const result = await createWith( + createProvider({ refreshLocalBaseRefForWorktreeCreate: refresh }) + ) + + expect(refresh).toHaveBeenCalledTimes(1) + expect(result).toMatchObject({ worktree: expect.anything() }) + expect(result).not.toHaveProperty('localBaseRefRefresh') + warn.mockRestore() + }) + + it('reports an error once, without retrying, when the relay rejects the refresh', async () => { + const refresh = vi.fn(async () => { + throw new Error('Invalid local base ref refresh refs.') + }) + + const result = await createWith( + createProvider({ refreshLocalBaseRefForWorktreeCreate: refresh }) + ) + + expect(refresh).toHaveBeenCalledTimes(1) + expect(result).toMatchObject({ + localBaseRefRefresh: { status: 'skipped_error', baseRef: 'origin/main', localBranch: 'main' } + }) + }) +}) + +describe('SSH local base refresh overlap', () => { + beforeEach(() => { + setupWorktreeHandlers() + }) + + // #15331: `-b feature-x` proves there was no local feature-x to refresh; probing it would race the relay add. + it('does not refresh or warn when the create makes the local base branch itself', async () => { + const provider = createProvider({ refreshLocalBaseRefForWorktreeCreate: vi.fn() }) + provider.exec.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'origin\n', stderr: '' } + } + // Only the remote-tracking base resolves; refs/heads/feature-x does not exist yet. + const ref = args.at(-1) ?? '' + return { + stdout: args[0] === 'rev-parse' && ref.startsWith('refs/remotes/') ? 'remote-x\n' : '', + stderr: '' + } + }) + provider.listWorktrees + .mockReset() + .mockResolvedValue([ + { path: '/remote/repo-feature-x', head: 'remote-x', branch: 'refs/heads/feature-x' } + ]) + + const result = await createWith(provider, { + repo: { ...REPO, worktreeBaseRef: 'origin/feature-x' }, + name: 'feature-x' + }) + + expect(provider.addWorktree.mock.calls[0]?.[3]).toMatchObject({ base: 'origin/feature-x' }) + expect(provider.addWorktree).toHaveBeenCalledTimes(1) + expect(provider.refreshLocalBaseRefForWorktreeCreate).not.toHaveBeenCalled() + expect(result).not.toHaveProperty('localBaseRefRefresh') + }) + + it('starts the relay worktree add while the refresh is still running', async () => { + let finishRefresh!: () => void + let markRefreshStarted!: () => void + const refreshStarted = new Promise((resolve) => { + markRefreshStarted = resolve + }) + const refresh = vi.fn( + () => + new Promise((resolve) => { + finishRefresh = () => resolve(REFRESH_UPDATED) + markRefreshStarted() + }) + ) + // Would deadlock if create awaited the refresh before starting the add. + const addWorktree = vi.fn(async () => { + await refreshStarted + finishRefresh() + }) + + const result = await createWith( + createProvider({ refreshLocalBaseRefForWorktreeCreate: refresh, addWorktree }) + ) + + expect(addWorktree).toHaveBeenCalledTimes(1) + expect(result).toMatchObject({ localBaseRefRefresh: UPDATED }) + }) + + // The relay runs one refresh per branch host-side, so the app adds no queue of its own. + it('hands every concurrent create straight to the relay', async () => { + const settlers: (() => void)[] = [] + const refresh = vi.fn( + () => new Promise((resolve) => settlers.push(() => resolve(REFRESH_UPDATED))) + ) + const repos = [ + { repo: REPO, name: 'improve-dashboard' }, + { repo: REPO, name: 'fix-login' }, + { repo: { ...REPO, id: 'repo-ssh-other', path: '/remote/other' }, name: 'add-search' } + ] + const provider = createProvider({ refreshLocalBaseRefForWorktreeCreate: refresh }) + provider.listWorktrees.mockResolvedValue( + repos.map(({ repo, name }) => ({ + path: `${repo.path}-${name}`, + head: 'remote-main', + branch: `refs/heads/${name}` + })) + ) + store.getSettings.mockReturnValue({ + branchPrefix: 'none', + nestWorkspaces: false, + refreshLocalBaseRefOnWorktreeCreate: true, + workspaceDir: '/workspace' + }) + store.getRepos.mockReturnValue(repos.map(({ repo }) => repo)) + store.getRepo.mockImplementation((id: string) => repos.find((r) => r.repo.id === id)?.repo) + getSshGitProviderMock.mockReturnValue(provider) + getActiveMultiplexerMock.mockReturnValue({ + request: vi.fn().mockResolvedValue(undefined), + notify: vi.fn() + }) + store.setWorktreeMeta.mockImplementation((_worktreeId, meta) => meta) + + const results = repos.map(({ repo, name }) => + handlers['worktrees:create'](null, { repoId: repo.id, name }) + ) + + await vi.waitFor(() => expect(refresh).toHaveBeenCalledTimes(3)) + settlers.forEach((settle) => settle()) + for (const result of await Promise.all(results)) { + expect(result).toMatchObject({ localBaseRefRefresh: { status: 'updated' } }) + } + }) +}) diff --git a/src/main/ipc/worktrees-ssh-local-base-refresh.test.ts b/src/main/ipc/worktrees-ssh-local-base-refresh.test.ts index c8a4e5088b1..2d8023c9a8e 100644 --- a/src/main/ipc/worktrees-ssh-local-base-refresh.test.ts +++ b/src/main/ipc/worktrees-ssh-local-base-refresh.test.ts @@ -1,5 +1,4 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -import type { CreateWorktreeResult } from '../../shared/worktree/create-types' import { getSshGitProviderMock, getActiveMultiplexerMock } from './worktrees-test-module-mocks' import { handlers, setupWorktreeHandlers, store } from './worktrees-test-harness' @@ -42,27 +41,27 @@ vi.mock('../ssh/ssh-target-registry', async () => vi.mock('../hooks', async () => (await import('./worktrees-test-module-mocks')).hooksModuleMock()) vi.mock('../setup-runner-script-text', async (importOriginal) => (await import('./worktrees-test-module-mocks')).setupRunnerScriptTextModuleMock( - (await importOriginal()) as Record + await importOriginal>() ) ) vi.mock('../worktree-runner-script', async (importOriginal) => (await import('./worktrees-test-module-mocks')).worktreeRunnerScriptModuleMock( - (await importOriginal()) as Record + await importOriginal>() ) ) vi.mock('../effective-hook-config', async (importOriginal) => (await import('./worktrees-test-module-mocks')).effectiveHookConfigModuleMock( - (await importOriginal()) as Record + await importOriginal>() ) ) vi.mock('../setup-hook-env-vars', async (importOriginal) => (await import('./worktrees-test-module-mocks')).setupHookEnvVarsModuleMock( - (await importOriginal()) as Record + await importOriginal>() ) ) vi.mock('./worktree-logic', async (importOriginal) => (await import('./worktrees-test-module-mocks')).worktreeLogicModuleMock( - (await importOriginal()) as Record + await importOriginal>() ) ) vi.mock('../terminal-history-deletion', async () => @@ -85,528 +84,220 @@ vi.mock('../runtime/worktree-teardown', async () => ) vi.mock('./pty', async () => (await import('./worktrees-test-module-mocks')).ptyModuleMock()) -describe('registerWorktreeHandlers', () => { +const REPO = { + id: 'repo-ssh', + path: '/remote/repo', + displayName: 'ssh', + badgeColor: '#000', + addedAt: 0, + connectionId: 'conn-1', + worktreeBaseRef: 'origin/main' +} +const REFS = { + repoPath: '/remote/repo', + fullRef: 'refs/heads/main', + remoteTrackingRef: 'refs/remotes/origin/main' +} + +function createProvider(relay: { + refresh?: ReturnType + behind?: ReturnType +}) { + return { + exec: vi.fn().mockImplementation(async (args: string[]) => { + if (args[0] === 'remote') { + return { stdout: 'origin\n', stderr: '' } + } + if (args[0] === 'show-ref') { + throw Object.assign(new Error('missing remote ref'), { code: 1 }) + } + return { stdout: '', stderr: '' } + }), + fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined), + addWorktree: vi.fn().mockResolvedValue(undefined), + listWorktrees: vi.fn().mockResolvedValue([ + { path: '/remote/repo', head: 'base123', branch: 'refs/heads/main', isMainWorktree: true }, + { + path: '/remote/repo-improve-dashboard', + head: 'abc123', + branch: 'refs/heads/improve-dashboard', + isMainWorktree: false + } + ]), + worktreeIsClean: vi.fn().mockResolvedValue({ clean: true }), + refreshLocalBaseRefForWorktreeCreate: relay.refresh ?? vi.fn(), + getLocalBaseRefFastForwardableBehind: relay.behind ?? vi.fn() + } +} + +async function createWith( + provider: ReturnType, + options: { refreshSetting?: boolean; worktreeBaseRef?: string; registerRoot?: () => void } = {} +) { + const repo = { ...REPO, worktreeBaseRef: options.worktreeBaseRef ?? REPO.worktreeBaseRef } + if (options.refreshSetting !== false) { + store.getSettings.mockReturnValue({ + branchPrefix: 'none', + nestWorkspaces: false, + refreshLocalBaseRefOnWorktreeCreate: true, + workspaceDir: '/workspace' + }) + } + store.getRepos.mockReturnValue([repo]) + store.getRepo.mockReturnValue(repo) + getSshGitProviderMock.mockReturnValue(provider) + getActiveMultiplexerMock.mockReturnValue({ + request: vi.fn().mockImplementation(async (method: string) => { + if (method === 'session.registerRoot') { + options.registerRoot?.() + } + }), + notify: vi.fn() + }) + store.setWorktreeMeta.mockImplementation((_worktreeId, meta) => meta) + const result: unknown = await handlers['worktrees:create'](null, { + repoId: 'repo-ssh', + name: 'improve-dashboard' + }) + return result +} + +/** Refresh git the app used to run itself; the relay now owns all of it on the host. */ +const APP_SIDE_REFRESH_COMMANDS = ['merge-base', 'log', 'rev-list', 'reset', 'update-ref', 'merge'] + +describe('SSH local base refresh on worktree create', () => { beforeEach(() => { setupWorktreeHandlers() }) - it('returns SSH local base refresh skip status when the owning worktree is dirty', async () => { - const repo = { - id: 'repo-ssh', - path: '/remote/repo', - displayName: 'ssh', - badgeColor: '#000', - addedAt: 0, - connectionId: 'conn-1', - worktreeBaseRef: 'origin/main' - } - const provider = { - exec: vi.fn().mockImplementation(async (args: string[]) => { - if (args[0] === 'remote') { - return { stdout: 'origin\n', stderr: '' } - } - if (args[0] === 'show-ref') { - throw Object.assign(new Error('missing remote ref'), { code: 1 }) - } - if (args[0] === 'merge-base') { - return { stdout: '', stderr: '' } - } - if (args[0] === 'log') { - return { stdout: 'commit-a\ncommit-b\ncommit-c\n', stderr: '' } - } - return { stdout: '', stderr: '' } - }), - fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined), - addWorktree: vi.fn().mockResolvedValue(undefined), - listWorktrees: vi - .fn() - .mockResolvedValueOnce([ - { - path: '/remote/repo', - head: 'base123', - branch: 'refs/heads/main', - isBare: false, - isMainWorktree: true - } - ]) - .mockResolvedValueOnce([ - { - path: '/remote/repo-improve-dashboard', - head: 'abc123', - branch: 'refs/heads/improve-dashboard', - isBare: false, - isMainWorktree: false - } - ]), - worktreeIsClean: vi.fn().mockResolvedValue({ clean: false, stdout: ' M package.json\n' }), - refreshLocalBaseRefForWorktreeCreate: vi.fn().mockResolvedValue(undefined) - } - const mux = { - request: vi.fn().mockResolvedValue(undefined), - notify: vi.fn() - } - store.getSettings.mockReturnValue({ - branchPrefix: 'none', - nestWorkspaces: false, - refreshLocalBaseRefOnWorktreeCreate: true, - workspaceDir: '/workspace' - }) - store.getRepos.mockReturnValue([repo]) - store.getRepo.mockReturnValue(repo) - getSshGitProviderMock.mockReturnValue(provider) - getActiveMultiplexerMock.mockReturnValue(mux) - store.setWorktreeMeta.mockImplementation((_worktreeId, meta) => meta) + it('reports the dirty owner the relay found, without inspecting anything itself', async () => { + const refresh = vi + .fn() + .mockResolvedValue({ status: 'skipped_dirty_worktree', ownerWorktreePath: '/remote/repo' }) + const provider = createProvider({ refresh }) - const result = (await handlers['worktrees:create'](null, { - repoId: 'repo-ssh', - name: 'improve-dashboard' - })) as CreateWorktreeResult + const result = await createWith(provider) - expect(provider.exec).toHaveBeenCalledWith( - ['merge-base', '--is-ancestor', 'refs/heads/main', 'refs/remotes/origin/main'], - '/remote/repo' - ) - expect(provider.exec).toHaveBeenCalledWith( - ['log', '--format=%H', 'refs/heads/main..refs/remotes/origin/main'], - '/remote/repo' - ) - expect(provider.worktreeIsClean).toHaveBeenCalledWith('/remote/repo', { - includeUntracked: false + expect(result).toMatchObject({ + localBaseRefRefresh: { + status: 'skipped_dirty_worktree', + baseRef: 'origin/main', + localBranch: 'main', + ownerWorktreePath: '/remote/repo' + } }) - expect(provider.exec).not.toHaveBeenCalledWith( - ['reset', '--hard', 'refs/remotes/origin/main'], - expect.any(String) - ) - expect(provider.refreshLocalBaseRefForWorktreeCreate).not.toHaveBeenCalled() - expect(result).toEqual( - expect.objectContaining({ - localBaseRefRefresh: { - status: 'skipped_dirty_worktree', - baseRef: 'origin/main', - localBranch: 'main', - ownerWorktreePath: '/remote/repo' - } - }) - ) + const execCommands = provider.exec.mock.calls.map(([args]) => args[0]) + expect(execCommands.filter((c) => APP_SIDE_REFRESH_COMMANDS.includes(c))).toEqual([]) + expect(provider.worktreeIsClean).not.toHaveBeenCalled() }) - it('refreshes SSH local base through the narrow relay RPC when the setting is on', async () => { - const repo = { - id: 'repo-ssh', - path: '/remote/repo', - displayName: 'ssh', - badgeColor: '#000', - addedAt: 0, - connectionId: 'conn-1', - worktreeBaseRef: 'origin/main' - } - const provider = { - exec: vi.fn().mockImplementation(async (args: string[]) => { - if (args[0] === 'remote') { - return { stdout: 'origin\n', stderr: '' } - } - if (args[0] === 'show-ref') { - throw Object.assign(new Error('missing remote ref'), { code: 1 }) - } - if (args[0] === 'merge-base') { - return { stdout: '', stderr: '' } - } - if (args[0] === 'log') { - return { stdout: 'commit-a\ncommit-b\n', stderr: '' } - } - throw new Error(`unexpected generic exec: ${args.join(' ')}`) - }), - fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined), - addWorktree: vi.fn().mockResolvedValue(undefined), - listWorktrees: vi - .fn() - .mockResolvedValueOnce([ - { - path: '/remote/repo', - head: 'base123', - branch: 'refs/heads/main', - isBare: false, - isMainWorktree: true - } - ]) - .mockResolvedValueOnce([ - { - path: '/remote/repo-improve-dashboard', - head: 'abc123', - branch: 'refs/heads/improve-dashboard', - isBare: false, - isMainWorktree: false - } - ]), - worktreeIsClean: vi.fn().mockResolvedValue({ clean: true }), - refreshLocalBaseRefForWorktreeCreate: vi.fn().mockResolvedValue(undefined) - } - const mux = { - request: vi.fn().mockResolvedValue(undefined), - notify: vi.fn() - } - store.getSettings.mockReturnValue({ - branchPrefix: 'none', - nestWorkspaces: false, - refreshLocalBaseRefOnWorktreeCreate: true, - workspaceDir: '/workspace' - }) - store.getRepos.mockReturnValue([repo]) - store.getRepo.mockReturnValue(repo) - getSshGitProviderMock.mockReturnValue(provider) - getActiveMultiplexerMock.mockReturnValue(mux) - store.setWorktreeMeta.mockImplementation((_worktreeId, meta) => meta) + it('refreshes through the narrow relay RPC with only the refs to move', async () => { + const refresh = vi + .fn() + .mockResolvedValue({ status: 'updated', ownerWorktreePath: '/remote/repo' }) + const provider = createProvider({ refresh }) - const result = (await handlers['worktrees:create'](null, { - repoId: 'repo-ssh', - name: 'improve-dashboard' - })) as CreateWorktreeResult + const result = await createWith(provider) - expect(provider.exec).toHaveBeenCalledWith( - ['merge-base', '--is-ancestor', 'refs/heads/main', 'refs/remotes/origin/main'], - '/remote/repo' - ) - expect(provider.exec).toHaveBeenCalledWith( - ['log', '--format=%H', 'refs/heads/main..refs/remotes/origin/main'], - '/remote/repo' - ) - expect(provider.refreshLocalBaseRefForWorktreeCreate).toHaveBeenCalledWith({ - repoPath: '/remote/repo', - fullRef: 'refs/heads/main', - remoteTrackingRef: 'refs/remotes/origin/main', - ownerWorktreePath: '/remote/repo' + expect(refresh).toHaveBeenCalledTimes(1) + expect(refresh).toHaveBeenCalledWith(REFS) + expect(provider.getLocalBaseRefFastForwardableBehind).not.toHaveBeenCalled() + expect(result).toMatchObject({ + localBaseRefRefresh: { + status: 'updated', + baseRef: 'origin/main', + localBranch: 'main', + ownerWorktreePath: '/remote/repo' + } }) - expect(provider.exec).not.toHaveBeenCalledWith( - ['reset', '--hard', 'refs/remotes/origin/main'], - expect.any(String) - ) - expect(provider.exec).not.toHaveBeenCalledWith( - ['update-ref', 'refs/heads/main', 'refs/remotes/origin/main'], - expect.any(String) - ) - expect(result).toEqual( - expect.objectContaining({ - localBaseRefRefresh: { - status: 'updated', - baseRef: 'origin/main', - localBranch: 'main', - ownerWorktreePath: '/remote/repo' - } - }) - ) }) - it('returns SSH local base update suggestion when a full local base ref is safely behind', async () => { - const repo = { - id: 'repo-ssh', - path: '/remote/repo', - displayName: 'ssh', - badgeColor: '#000', - addedAt: 0, - connectionId: 'conn-1', - worktreeBaseRef: 'refs/remotes/origin/main' - } - let registeredRoots = false - const provider = { - exec: vi.fn().mockImplementation(async (args: string[]) => { - if (args[0] === 'remote') { - return { stdout: 'origin\n', stderr: '' } - } - if (args[0] === 'show-ref') { - throw Object.assign(new Error('missing remote ref'), { code: 1 }) - } - if (args[0] === 'merge-base' || args[0] === 'log') { - if (!registeredRoots) { - throw new Error('Path outside authorized workspace') - } - return { - stdout: args[0] === 'log' ? 'commit-a\ncommit-b\ncommit-c\ncommit-d\n' : '', - stderr: '' - } - } - return { stdout: '', stderr: '' } - }), - fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined), - addWorktree: vi.fn().mockResolvedValue(undefined), - listWorktrees: vi.fn().mockImplementation(async () => { - if (!registeredRoots) { - throw new Error('No workspace roots registered yet') - } - return [ - { - path: '/remote/repo', - head: 'base123', - branch: 'refs/heads/main', - isBare: false, - isMainWorktree: true - }, - { - path: '/remote/repo-improve-dashboard', - head: 'abc123', - branch: 'refs/heads/improve-dashboard', - isBare: false, - isMainWorktree: false - } - ] - }), - worktreeIsClean: vi.fn().mockImplementation(async () => { - if (!registeredRoots) { - throw new Error('Path outside authorized workspace') - } - return { clean: true } - }), - refreshLocalBaseRefForWorktreeCreate: vi.fn().mockResolvedValue(undefined) - } - const mux = { - request: vi.fn().mockImplementation(async (method: string) => { - if (method === 'session.registerRoot') { - registeredRoots = true - } - }), - notify: vi.fn() - } - store.getRepos.mockReturnValue([repo]) - store.getRepo.mockReturnValue(repo) - getSshGitProviderMock.mockReturnValue(provider) - getActiveMultiplexerMock.mockReturnValue(mux) - store.setWorktreeMeta.mockImplementation((_worktreeId, meta) => meta) + it('reports a branch the relay moved without a checkout as updated with no owner', async () => { + const provider = createProvider({ refresh: vi.fn().mockResolvedValue({ status: 'updated' }) }) - const result = await handlers['worktrees:create'](null, { - repoId: 'repo-ssh', - name: 'improve-dashboard' - }) + const result = await createWith(provider) - expect(provider.exec).toHaveBeenCalledWith( - ['merge-base', '--is-ancestor', 'refs/heads/main', 'refs/remotes/origin/main'], - '/remote/repo' - ) - expect(provider.exec).toHaveBeenCalledWith( - ['log', '--format=%H', 'refs/heads/main..refs/remotes/origin/main'], - '/remote/repo' - ) - expect(provider.listWorktrees).toHaveBeenCalledWith('/remote/repo') - expect(provider.worktreeIsClean).toHaveBeenCalledWith('/remote/repo', { - includeUntracked: false + expect(result).toMatchObject({ + localBaseRefRefresh: { status: 'updated', baseRef: 'origin/main', localBranch: 'main' } }) - expect(provider.refreshLocalBaseRefForWorktreeCreate).toHaveBeenCalledWith({ - repoPath: '/remote/repo', - fullRef: 'refs/heads/main', - remoteTrackingRef: 'refs/remotes/origin/main', - ownerWorktreePath: '/remote/repo', - checkOnly: true - }) - expect(provider.exec).not.toHaveBeenCalledWith( - ['reset', '--hard', 'refs/remotes/origin/main'], - expect.any(String) - ) - expect(result).toEqual( - expect.objectContaining({ - localBaseRefUpdateSuggestion: { - baseRef: 'origin/main', - localBranch: 'main', - behind: 4 - } - }) - ) + expect(result).not.toHaveProperty('localBaseRefRefresh.ownerWorktreePath') }) - it('does not suggest SSH local base updates when the relay cannot refresh local refs', async () => { - const repo = { - id: 'repo-ssh', - path: '/remote/repo', - displayName: 'ssh', - badgeColor: '#000', - addedAt: 0, - connectionId: 'conn-1', - worktreeBaseRef: 'refs/remotes/origin/main' - } - const methodNotFound = Object.assign( - new Error('Method not found: git.refreshLocalBaseRefForWorktreeCreate'), - { code: -32601 } - ) - const provider = { - exec: vi.fn().mockImplementation(async (args: string[]) => { - if (args[0] === 'remote') { - return { stdout: 'origin\n', stderr: '' } - } - if (args[0] === 'show-ref') { - throw Object.assign(new Error('missing remote ref'), { code: 1 }) - } - if (args[0] === 'merge-base') { - return { stdout: '', stderr: '' } - } - if (args[0] === 'log') { - return { stdout: 'commit-a\n', stderr: '' } - } - return { stdout: '', stderr: '' } - }), - fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined), - addWorktree: vi.fn().mockResolvedValue(undefined), - listWorktrees: vi - .fn() - .mockResolvedValueOnce([ - { - path: '/remote/repo', - head: 'base123', - branch: 'refs/heads/main', - isBare: false, - isMainWorktree: true - } - ]) - .mockResolvedValueOnce([ - { - path: '/remote/repo-improve-dashboard', - head: 'abc123', - branch: 'refs/heads/improve-dashboard', - isBare: false, - isMainWorktree: false - } - ]), - worktreeIsClean: vi.fn().mockResolvedValue({ clean: true }), - refreshLocalBaseRefForWorktreeCreate: vi.fn().mockRejectedValue(methodNotFound) - } - const mux = { - request: vi.fn().mockResolvedValue(undefined), - notify: vi.fn() - } - store.getRepos.mockReturnValue([repo]) - store.getRepo.mockReturnValue(repo) - getSshGitProviderMock.mockReturnValue(provider) - getActiveMultiplexerMock.mockReturnValue(mux) - store.setWorktreeMeta.mockImplementation((_worktreeId, meta) => meta) - - const result = (await handlers['worktrees:create'](null, { - repoId: 'repo-ssh', - name: 'improve-dashboard' - })) as CreateWorktreeResult - - expect(provider.refreshLocalBaseRefForWorktreeCreate).toHaveBeenCalledWith({ - repoPath: '/remote/repo', - fullRef: 'refs/heads/main', - remoteTrackingRef: 'refs/remotes/origin/main', - ownerWorktreePath: '/remote/repo', - checkOnly: true + // #15331: the relay proves the local branch absent (or current); there is nothing to report. + it('reports no refresh status when the relay had nothing to do', async () => { + const provider = createProvider({ + refresh: vi.fn().mockResolvedValue({ status: 'nothing_to_do' }) }) - expect(result.localBaseRefUpdateSuggestion).toBeUndefined() - }) - // #15331: the pre-create merge-base probe fails when refs/heads/ does not exist yet. - const buildMissingLocalBaseSshCase = (presence: 'absent' | 'present' | 'probe-failed') => { - const repo = { - id: 'repo-ssh', - path: '/remote/repo', - displayName: 'ssh', - badgeColor: '#000', - addedAt: 0, - connectionId: 'conn-1', - worktreeBaseRef: 'origin/main' - } - const provider = { - exec: vi.fn().mockImplementation(async (args: string[]) => { - if (args[0] === 'remote') { - return { stdout: 'origin\n', stderr: '' } - } - if (args[0] === 'show-ref' && args.at(-1) === 'refs/heads/main') { - if (presence === 'probe-failed') { - throw new Error('ssh: connection closed by remote host') - } - if (presence === 'present') { - return { stdout: '', stderr: '' } - } - throw Object.assign(new Error('missing ref'), { code: 1 }) - } - if (args[0] === 'show-ref') { - throw Object.assign(new Error('missing ref'), { code: 1 }) - } - if (args[0] === 'rev-parse') { - const ref = args.at(-1) ?? '' - if (ref.startsWith('refs/heads/main')) { - return { stdout: presence === 'present' ? 'local-main\n' : '', stderr: '' } - } - // Other refs/heads probes are the new-branch conflict check; it must stay unresolvable. - return { stdout: ref.startsWith('refs/heads/') ? '' : 'remote-main\n', stderr: '' } - } - if (args[0] === 'merge-base') { - throw new Error('fatal: Not a valid object name refs/heads/main') - } - return { stdout: '', stderr: '' } - }), - fetchRemoteTrackingRef: vi.fn().mockResolvedValue(undefined), - addWorktree: vi.fn().mockResolvedValue(undefined), - listWorktrees: vi.fn().mockResolvedValue([ - { - path: '/remote/repo-improve-dashboard', - head: 'abc123', - branch: 'refs/heads/improve-dashboard', - isBare: false, - isMainWorktree: false - } - ]), - worktreeIsClean: vi.fn().mockResolvedValue({ clean: true }), - refreshLocalBaseRefForWorktreeCreate: vi.fn().mockResolvedValue(undefined) - } - store.getSettings.mockReturnValue({ - branchPrefix: 'none', - nestWorkspaces: false, - refreshLocalBaseRefOnWorktreeCreate: true, - workspaceDir: '/workspace' - }) - store.getRepos.mockReturnValue([repo]) - store.getRepo.mockReturnValue(repo) - getSshGitProviderMock.mockReturnValue(provider) - getActiveMultiplexerMock.mockReturnValue({ - request: vi.fn().mockResolvedValue(undefined), - notify: vi.fn() - }) - store.setWorktreeMeta.mockImplementation((_worktreeId, meta) => meta) - return provider - } - it('does not report an SSH local base refresh when the local base branch does not exist', async () => { - const provider = buildMissingLocalBaseSshCase('absent') + const result = await createWith(provider) - const result = (await handlers['worktrees:create'](null, { - repoId: 'repo-ssh', - name: 'improve-dashboard' - })) as CreateWorktreeResult - - expect(provider.exec).toHaveBeenCalledWith( - ['show-ref', '--verify', '--quiet', '--', 'refs/heads/main'], - '/remote/repo' - ) - expect(result.localBaseRefRefresh).toBeUndefined() - expect(provider.refreshLocalBaseRefForWorktreeCreate).not.toHaveBeenCalled() + expect(provider.refreshLocalBaseRefForWorktreeCreate).toHaveBeenCalledTimes(1) + expect(result).not.toHaveProperty('localBaseRefRefresh') }) - it('keeps the SSH not-fast-forward status when the local base branch exists and diverged', async () => { - const provider = buildMissingLocalBaseSshCase('present') - - const result = (await handlers['worktrees:create'](null, { - repoId: 'repo-ssh', - name: 'improve-dashboard' - })) as CreateWorktreeResult - - expect(result.localBaseRefRefresh).toEqual({ - status: 'skipped_not_fast_forward', - baseRef: 'origin/main', - localBranch: 'main' + it('keeps the not-fast-forward status the relay reports', async () => { + const provider = createProvider({ + refresh: vi.fn().mockResolvedValue({ status: 'skipped_not_fast_forward' }) }) - expect(provider.refreshLocalBaseRefForWorktreeCreate).not.toHaveBeenCalled() - }) - // Losing the relay mid-probe is not evidence the branch is missing. - it('keeps the SSH not-fast-forward status when the local base ref probe fails', async () => { - const provider = buildMissingLocalBaseSshCase('probe-failed') + const result = await createWith(provider) - const result = (await handlers['worktrees:create'](null, { - repoId: 'repo-ssh', - name: 'improve-dashboard' - })) as CreateWorktreeResult - - expect(result.localBaseRefRefresh).toEqual({ - status: 'skipped_not_fast_forward', - baseRef: 'origin/main', - localBranch: 'main' + expect(result).toMatchObject({ + localBaseRefRefresh: { + status: 'skipped_not_fast_forward', + baseRef: 'origin/main', + localBranch: 'main' + } }) + }) +}) + +describe('SSH local base update suggestion on worktree create', () => { + beforeEach(() => { + setupWorktreeHandlers() + }) + + it('suggests an update from the relay inspection once the workspace root is registered', async () => { + let registeredRoots = false + const behind = vi.fn().mockImplementation(async () => { + if (!registeredRoots) { + throw new Error('Path outside authorized workspace') + } + return 4 + }) + const provider = createProvider({ behind }) + + const result = await createWith(provider, { + refreshSetting: false, + worktreeBaseRef: 'refs/remotes/origin/main', + registerRoot: () => (registeredRoots = true) + }) + + expect(behind).toHaveBeenCalledWith(REFS) expect(provider.refreshLocalBaseRefForWorktreeCreate).not.toHaveBeenCalled() + expect(result).toMatchObject({ + localBaseRefUpdateSuggestion: { baseRef: 'origin/main', localBranch: 'main', behind: 4 } + }) + }) + + it.each([ + ['cannot be fast-forwarded', async () => undefined], + [ + 'cannot be inspected', + async () => { + throw Object.assign(new Error('Method not found'), { code: -32601 }) + } + ] + ])('does not suggest an update when the local base %s', async (_case, inspect) => { + const behind = vi.fn(inspect) + const provider = createProvider({ behind }) + + const result = await createWith(provider, { + refreshSetting: false, + worktreeBaseRef: 'refs/remotes/origin/main' + }) + + expect(behind).toHaveBeenCalledTimes(1) + expect(result).not.toHaveProperty('localBaseRefUpdateSuggestion') }) }) diff --git a/src/main/providers/ssh-git-provider-worktree.test.ts b/src/main/providers/ssh-git-provider-worktree.test.ts index 03722c93a52..cc9a8024104 100644 --- a/src/main/providers/ssh-git-provider-worktree.test.ts +++ b/src/main/providers/ssh-git-provider-worktree.test.ts @@ -140,20 +140,53 @@ describe('SshGitProvider', () => { expect(result).toEqual({ clean: false }) }) - it('refreshLocalBaseRefForWorktreeCreate sends the narrow refresh request', async () => { - await provider.refreshLocalBaseRefForWorktreeCreate({ + it('refreshLocalBaseRefForWorktreeCreate sends the narrow refresh request and returns its outcome', async () => { + const refs = { repoPath: '/home/user/repo', fullRef: 'refs/heads/main', - remoteTrackingRef: 'refs/remotes/origin/main', + remoteTrackingRef: 'refs/remotes/origin/main' + } + mux.request.mockResolvedValueOnce({ + status: 'skipped_dirty_worktree', ownerWorktreePath: '/home/user/repo' }) - expect(mux.request).toHaveBeenCalledWith('git.refreshLocalBaseRefForWorktreeCreate', { - repoPath: '/home/user/repo', - fullRef: 'refs/heads/main', - remoteTrackingRef: 'refs/remotes/origin/main', + await expect(provider.refreshLocalBaseRefForWorktreeCreate(refs)).resolves.toEqual({ + status: 'skipped_dirty_worktree', ownerWorktreePath: '/home/user/repo' }) + expect(mux.request).toHaveBeenCalledWith('git.refreshLocalBaseRefForWorktreeCreate', refs) + }) + + it('refreshLocalBaseRefForWorktreeCreate reads a malformed relay reply as an error', async () => { + mux.request.mockResolvedValueOnce(undefined) + + await expect( + provider.refreshLocalBaseRefForWorktreeCreate({ + repoPath: '/home/user/repo', + fullRef: 'refs/heads/main', + remoteTrackingRef: 'refs/remotes/origin/main' + }) + ).resolves.toEqual({ status: 'skipped_error' }) + }) + + it('getLocalBaseRefFastForwardableBehind asks the relay to inspect without moving anything', async () => { + const refs = { + repoPath: '/home/user/repo', + fullRef: 'refs/heads/main', + remoteTrackingRef: 'refs/remotes/origin/main' + } + mux.request + .mockResolvedValueOnce({ status: 'behind', behind: 4, localOid: 'a', remoteOid: 'b' }) + .mockResolvedValueOnce({ status: 'skipped_dirty_worktree', ownerWorktreePath: '/x' }) + + await expect(provider.getLocalBaseRefFastForwardableBehind(refs)).resolves.toBe(4) + await expect(provider.getLocalBaseRefFastForwardableBehind(refs)).resolves.toBeUndefined() + expect(mux.request).toHaveBeenCalledWith('git.inspectLocalBaseRefForWorktreeCreate', refs) + expect(mux.request).not.toHaveBeenCalledWith( + 'git.refreshLocalBaseRefForWorktreeCreate', + expect.anything() + ) }) it('worktreeIsClean falls back to git.status for old relays', async () => { diff --git a/src/main/providers/ssh-git-worktree-provider.ts b/src/main/providers/ssh-git-worktree-provider.ts index 17e5b273de7..4e1e0f3a057 100644 --- a/src/main/providers/ssh-git-worktree-provider.ts +++ b/src/main/providers/ssh-git-worktree-provider.ts @@ -5,6 +5,12 @@ import { CapabilityProbeCache } from '../../shared/capability-probe-cache' import { InFlightPromiseDedupe, stableInFlightKey } from '../../shared/in-flight-promise-dedupe' import { assertAuthoritativeWorktreeCatalog } from '../../shared/worktree/worktree-catalog-availability' import { isJsonRpcMethodNotFoundError } from './ssh-git-relay-errors' +import { + parseLocalBaseBranchFastForwardOutcome, + readFastForwardableBehindCount, + type LocalBaseBranchFastForwardOutcome, + type LocalBaseBranchRefs +} from '../../shared/worktree/local-base-branch-fast-forward' import { SshGitReviewHeadProvider } from './ssh-git-review-head-provider' const WORKTREE_IS_CLEAN_CAPABILITY = 'git.worktreeIsClean' as const @@ -134,16 +140,24 @@ export class SshGitWorktreeProvider extends SshGitReviewHeadProvider { ) } - async refreshLocalBaseRefForWorktreeCreate(args: { - repoPath: string - fullRef: string - remoteTrackingRef: string - ownerWorktreePath?: string - checkOnly?: boolean - }): Promise { - await this.runWithGitReadInvalidation(async () => { - await this.mux.request('git.refreshLocalBaseRefForWorktreeCreate', args) - }) + /** The relay inspects and moves the branch host-side, one run per branch at a time. */ + async refreshLocalBaseRefForWorktreeCreate( + args: LocalBaseBranchRefs + ): Promise { + return this.runWithGitReadInvalidation(async () => + parseLocalBaseBranchFastForwardOutcome( + await this.mux.request('git.refreshLocalBaseRefForWorktreeCreate', args) + ) + ) + } + + /** Commits local is behind when the relay could fast-forward it; read-only. */ + async getLocalBaseRefFastForwardableBehind( + args: LocalBaseBranchRefs + ): Promise { + return readFastForwardableBehindCount( + await this.mux.request('git.inspectLocalBaseRefForWorktreeCreate', args) + ) } async renameCurrentBranch(worktreePath: string, newBranch: string): Promise { diff --git a/src/relay/git-handler-local-base-ref-refresh.ts b/src/relay/git-handler-local-base-ref-refresh.ts index 299780799f0..3dd9b7db302 100644 --- a/src/relay/git-handler-local-base-ref-refresh.ts +++ b/src/relay/git-handler-local-base-ref-refresh.ts @@ -1,88 +1,66 @@ import type { GitExec } from './git-handler-ops' -import { areRelayWorktreePathsEqual, readRelayWorktreeList } from './git-handler-worktree-ops' +import { readRelayWorktreeList } from './git-handler-worktree-ops' import type { GitCapabilityCache } from '../shared/git-capability-cache' +import { createCoalescingKeyedRunner } from '../shared/coalescing-keyed-runner' +import { + fastForwardLocalBaseBranch, + inspectLocalBaseBranch, + type LocalBaseBranchFastForwardOutcome, + type LocalBaseBranchGit, + type LocalBaseBranchInspection, + type LocalBaseBranchRefs +} from '../shared/worktree/local-base-branch-fast-forward' + +// Why: the host owns this; one run per branch at a time here also serializes creates from every client of this relay. +const runPerLocalBaseBranch = createCoalescingKeyedRunner() export async function refreshLocalBaseRefForWorktreeCreateOp( git: GitExec, params: Record, capabilities: GitCapabilityCache -): Promise { - const repoPath = params.repoPath as string - const fullRef = params.fullRef as string - const remoteTrackingRef = params.remoteTrackingRef as string - const ownerWorktreePath = params.ownerWorktreePath as string | undefined - const checkOnly = params.checkOnly === true +): Promise { + const refs = await readLocalBaseBranchRefs(git, params) + const host = relayLocalBaseBranchGit(git, capabilities) + return runPerLocalBaseBranch(`${refs.repoPath}\0${refs.fullRef}`, refs.remoteTrackingRef, () => + fastForwardLocalBaseBranch(host, refs) + ) +} +export async function inspectLocalBaseRefForWorktreeCreateOp( + git: GitExec, + params: Record, + capabilities: GitCapabilityCache +): Promise { + const refs = await readLocalBaseBranchRefs(git, params) + return inspectLocalBaseBranch(relayLocalBaseBranchGit(git, capabilities), refs) +} + +async function readLocalBaseBranchRefs( + git: GitExec, + params: Record +): Promise { + const { repoPath, fullRef, remoteTrackingRef } = params if ( typeof repoPath !== 'string' || typeof fullRef !== 'string' || - typeof remoteTrackingRef !== 'string' || - (ownerWorktreePath !== undefined && typeof ownerWorktreePath !== 'string') + typeof remoteTrackingRef !== 'string' ) { throw new Error('Invalid local base ref refresh request.') } if (!fullRef.startsWith('refs/heads/') || !remoteTrackingRef.startsWith('refs/remotes/')) { throw new Error('Invalid local base ref refresh refs.') } - await git(['check-ref-format', fullRef], repoPath) await git(['check-ref-format', remoteTrackingRef], repoPath) - - const localOid = await revParseCommit(git, repoPath, fullRef, 'Local base ref is missing.') - const remoteOid = await revParseCommit( - git, - repoPath, - remoteTrackingRef, - 'Remote-tracking base ref is missing.' - ) - - // Why: this RPC mutates refs/worktrees, so the relay repeats main-process - // safety checks at mutation time to close stale-preflight and direct-call gaps. - try { - await git(['merge-base', '--is-ancestor', localOid, remoteOid], repoPath) - } catch { - throw new Error('Local base ref is not a fast-forward update.') - } - - const worktrees = await readRelayWorktreeList(git, repoPath, capabilities) - const ownerWorktree = worktrees.find((worktree) => worktree.branch === fullRef) - if (ownerWorktree) { - if (ownerWorktreePath && !areRelayWorktreePathsEqual(ownerWorktree.path, ownerWorktreePath)) { - throw new Error('Local base ref is checked out in a different worktree.') - } - const { stdout } = await git( - ['status', '--porcelain', '--untracked-files=no'], - ownerWorktree.path - ) - if (stdout.trim()) { - throw new Error('Local base ref worktree has tracked changes.') - } - if (checkOnly) { - return - } - await git(['reset', '--hard', remoteOid], ownerWorktree.path) - return - } - - // Why: not checked out anywhere — fast-forward the bare ref. The - // expected-old-OID form is a no-op-safe compare-and-swap if the ref moved - // since the caller's evaluation snapshot. - if (checkOnly) { - return - } - await git(['update-ref', fullRef, remoteOid, localOid], repoPath) + return { repoPath, fullRef, remoteTrackingRef } } -async function revParseCommit( +function relayLocalBaseBranchGit( git: GitExec, - repoPath: string, - ref: string, - missingMessage: string -): Promise { - const { stdout } = await git(['rev-parse', '--verify', `${ref}^{commit}`], repoPath) - const oid = stdout.trim() - if (!oid) { - throw new Error(missingMessage) + capabilities: GitCapabilityCache +): LocalBaseBranchGit { + return { + exec: (args, cwd) => git(args, cwd), + listWorktrees: (repoPath) => readRelayWorktreeList(git, repoPath, capabilities) } - return oid } diff --git a/src/relay/git-handler-registration.ts b/src/relay/git-handler-registration.ts index 6462327c416..02690b202c8 100644 --- a/src/relay/git-handler-registration.ts +++ b/src/relay/git-handler-registration.ts @@ -65,6 +65,9 @@ export function registerGitHandlers( dispatcher.onRequest('git.refreshLocalBaseRefForWorktreeCreate', (p) => handlers.worktree.refreshLocalBaseRefForWorktreeCreate(p) ) + dispatcher.onRequest('git.inspectLocalBaseRefForWorktreeCreate', (p) => + handlers.worktree.inspectLocalBaseRefForWorktreeCreate(p) + ) dispatcher.onRequest('git.markRemoteOrcaCreated', (p) => handlers.exec.markRemoteOrcaCreated(p)) dispatcher.onRequest('git.renameCurrentBranch', (p) => handlers.exec.renameCurrentBranch(p)) dispatcher.onRequest('git.forceDeletePreservedBranch', (p) => diff --git a/src/relay/git-handler-worktree-operations.ts b/src/relay/git-handler-worktree-operations.ts index 966030c4562..9303db71964 100644 --- a/src/relay/git-handler-worktree-operations.ts +++ b/src/relay/git-handler-worktree-operations.ts @@ -12,7 +12,10 @@ import { worktreeIsCleanOp } from './git-handler-worktree-ops' import { annotatePrunableWorktreesByExistence } from './git-handler-worktree-list' -import { refreshLocalBaseRefForWorktreeCreateOp } from './git-handler-local-base-ref-refresh' +import { + inspectLocalBaseRefForWorktreeCreateOp, + refreshLocalBaseRefForWorktreeCreateOp +} from './git-handler-local-base-ref-refresh' import { hasUnsupportedRevParsePathFormatEcho, isUnsupportedRevParsePathFormatError @@ -171,4 +174,8 @@ export class GitHandlerWorktreeOperations extends GitHandlerOperationContext { refreshLocalBaseRefForWorktreeCreateOp(this.git.bind(this), params, this.gitCapabilities) ) } + + async inspectLocalBaseRefForWorktreeCreate(params: Record) { + return inspectLocalBaseRefForWorktreeCreateOp(this.git.bind(this), params, this.gitCapabilities) + } } diff --git a/src/relay/git-handler-worktree-provisioning.test.ts b/src/relay/git-handler-worktree-provisioning.test.ts index 804799103bd..9764f9b8b9b 100644 --- a/src/relay/git-handler-worktree-provisioning.test.ts +++ b/src/relay/git-handler-worktree-provisioning.test.ts @@ -20,7 +20,8 @@ import { import { createGitHandlerRelay, createGitTempDir, - removeGitTempDir + removeGitTempDir, + type GitSpyTarget } from './git-handler-test-harness' describe('GitHandler', () => { @@ -79,251 +80,208 @@ describe('GitHandler', () => { return { localDispatcher, gitMock } } - it('resets the owning worktree to the remote-tracking ref', async () => { + function revParse(ref: string): string { + return execFileSync('git', ['rev-parse', ref], { cwd: tmpDir, encoding: 'utf-8' }).trim() + } + + // The checked-out branch is one commit behind refs/remotes/origin/main, which changes base.txt. + function initBehindRepo(): { branchRef: string; localSha: string; remoteSha: string } { gitInit(tmpDir) + execFileSync('git', ['config', 'core.autocrlf', 'false'], { cwd: tmpDir, stdio: 'pipe' }) writeFileSync(path.join(tmpDir, 'base.txt'), 'base') gitCommit(tmpDir, 'initial') - const branchRef = currentBranchFullRef(tmpDir) - const ownerPath = reportedWorktreePath(tmpDir) - const firstSha = execFileSync('git', ['rev-parse', 'HEAD'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() + const localSha = revParse('HEAD') writeFileSync(path.join(tmpDir, 'base.txt'), 'remote') gitCommit(tmpDir, 'remote update') - const remoteSha = execFileSync('git', ['rev-parse', 'HEAD'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() + const remoteSha = revParse('HEAD') execFileSync('git', ['update-ref', 'refs/remotes/origin/main', remoteSha], { cwd: tmpDir, stdio: 'pipe' }) - execFileSync('git', ['reset', '--hard', firstSha], { cwd: tmpDir, stdio: 'pipe' }) + execFileSync('git', ['reset', '--hard', localSha], { cwd: tmpDir, stdio: 'pipe' }) + return { branchRef: currentBranchFullRef(tmpDir), localSha, remoteSha } + } - await dispatcher.callRequest('git.refreshLocalBaseRefForWorktreeCreate', { - repoPath: tmpDir, - fullRef: branchRef, - remoteTrackingRef: 'refs/remotes/origin/main', - ownerWorktreePath: ownerPath - }) + it('fast-forwards the owning worktree to the remote-tracking ref on the host', async () => { + const { branchRef, remoteSha } = initBehindRepo() - const actual = execFileSync('git', ['rev-parse', 'HEAD'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() - expect(actual).toBe(remoteSha) + await expect( + dispatcher.callRequest('git.refreshLocalBaseRefForWorktreeCreate', { + repoPath: tmpDir, + fullRef: branchRef, + remoteTrackingRef: 'refs/remotes/origin/main' + }) + ).resolves.toEqual({ status: 'updated', ownerWorktreePath: reportedWorktreePath(tmpDir) }) + + expect(revParse('HEAD')).toBe(remoteSha) await expect(fs.readFile(path.join(tmpDir, 'base.txt'), 'utf-8')).resolves.toBe('remote') }) it('fast-forwards a non-checked-out local branch via update-ref', async () => { - gitInit(tmpDir) - writeFileSync(path.join(tmpDir, 'base.txt'), 'base') - gitCommit(tmpDir, 'initial') - execFileSync('git', ['branch', 'main-copy'], { cwd: tmpDir, stdio: 'pipe' }) - writeFileSync(path.join(tmpDir, 'base.txt'), 'remote') - gitCommit(tmpDir, 'remote update') - const remoteSha = execFileSync('git', ['rev-parse', 'HEAD'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() - execFileSync('git', ['update-ref', 'refs/remotes/origin/main', remoteSha], { - cwd: tmpDir, - stdio: 'pipe' - }) + const { localSha, remoteSha } = initBehindRepo() + execFileSync('git', ['branch', 'main-copy', localSha], { cwd: tmpDir, stdio: 'pipe' }) - await dispatcher.callRequest('git.refreshLocalBaseRefForWorktreeCreate', { - repoPath: tmpDir, - fullRef: 'refs/heads/main-copy', - remoteTrackingRef: 'refs/remotes/origin/main' - }) + await expect( + dispatcher.callRequest('git.refreshLocalBaseRefForWorktreeCreate', { + repoPath: tmpDir, + fullRef: 'refs/heads/main-copy', + remoteTrackingRef: 'refs/remotes/origin/main' + }) + ).resolves.toEqual({ status: 'updated' }) - // No working tree owns main-copy, so the bare ref fast-forwards. - const actual = execFileSync('git', ['rev-parse', 'refs/heads/main-copy'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() - expect(actual).toBe(remoteSha) + expect(revParse('refs/heads/main-copy')).toBe(remoteSha) }) - it('does not move a non-checked-out local branch when checkOnly is set', async () => { - gitInit(tmpDir) - writeFileSync(path.join(tmpDir, 'base.txt'), 'base') - gitCommit(tmpDir, 'initial') - execFileSync('git', ['branch', 'main-copy'], { cwd: tmpDir, stdio: 'pipe' }) - const originalSha = execFileSync('git', ['rev-parse', 'refs/heads/main-copy'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() - writeFileSync(path.join(tmpDir, 'base.txt'), 'remote') - gitCommit(tmpDir, 'remote update') - const remoteSha = execFileSync('git', ['rev-parse', 'HEAD'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() - execFileSync('git', ['update-ref', 'refs/remotes/origin/main', remoteSha], { - cwd: tmpDir, - stdio: 'pipe' - }) + it('reports nothing to do for a local branch that does not exist yet', async () => { + initBehindRepo() - await dispatcher.callRequest('git.refreshLocalBaseRefForWorktreeCreate', { - repoPath: tmpDir, - fullRef: 'refs/heads/main-copy', - remoteTrackingRef: 'refs/remotes/origin/main', - checkOnly: true - }) - - const actual = execFileSync('git', ['rev-parse', 'refs/heads/main-copy'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() - expect(actual).toBe(originalSha) + await expect( + dispatcher.callRequest('git.refreshLocalBaseRefForWorktreeCreate', { + repoPath: tmpDir, + fullRef: 'refs/heads/not-created-yet', + remoteTrackingRef: 'refs/remotes/origin/main' + }) + ).resolves.toEqual({ status: 'nothing_to_do' }) }) - it('rejects invalid local base ref refresh refs', async () => { + it.each([ + [ + 'refs outside heads/remotes', + { fullRef: 'refs/tags/main', remoteTrackingRef: 'refs/remotes/origin/main' }, + 'Invalid local base ref refresh refs.' + ], + [ + 'a non-string ref', + { fullRef: 42, remoteTrackingRef: 'refs/remotes/origin/main' }, + 'Invalid local base ref refresh request.' + ], + [ + 'a missing remote-tracking ref', + { fullRef: 'refs/heads/main' }, + 'Invalid local base ref refresh request.' + ] + ])('rejects %s without touching git', async (_case, params, message) => { + const { localDispatcher, gitMock } = setupMockedRefreshHandler() + + for (const method of [ + 'git.refreshLocalBaseRefForWorktreeCreate', + 'git.inspectLocalBaseRefForWorktreeCreate' + ]) { + await expect( + localDispatcher.callRequest(method, { repoPath: '/repo', ...params }) + ).rejects.toThrow(message) + } + expect(gitMock).not.toHaveBeenCalled() + }) + + it('rejects a ref name git itself refuses', async () => { gitInit(tmpDir) await expect( dispatcher.callRequest('git.refreshLocalBaseRefForWorktreeCreate', { repoPath: tmpDir, - fullRef: 'refs/tags/main', + fullRef: 'refs/heads/bad..name', remoteTrackingRef: 'refs/remotes/origin/main' }) - ).rejects.toThrow('Invalid local base ref refresh refs.') + ).rejects.toThrow() }) - it('rejects a dirty owner worktree before resetting', async () => { - gitInit(tmpDir) - writeFileSync(path.join(tmpDir, 'base.txt'), 'base') - gitCommit(tmpDir, 'initial') - const branchRef = currentBranchFullRef(tmpDir) - const ownerPath = reportedWorktreePath(tmpDir) - const firstSha = execFileSync('git', ['rev-parse', 'HEAD'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() - writeFileSync(path.join(tmpDir, 'base.txt'), 'remote') - gitCommit(tmpDir, 'remote update') - const remoteSha = execFileSync('git', ['rev-parse', 'HEAD'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() - execFileSync('git', ['update-ref', 'refs/remotes/origin/main', remoteSha], { - cwd: tmpDir, - stdio: 'pipe' - }) - execFileSync('git', ['reset', '--hard', firstSha], { cwd: tmpDir, stdio: 'pipe' }) + it('leaves a dirty owner worktree and its edit alone', async () => { + const { branchRef, localSha } = initBehindRepo() writeFileSync(path.join(tmpDir, 'base.txt'), 'local dirty') await expect( dispatcher.callRequest('git.refreshLocalBaseRefForWorktreeCreate', { repoPath: tmpDir, fullRef: branchRef, - remoteTrackingRef: 'refs/remotes/origin/main', - ownerWorktreePath: ownerPath + remoteTrackingRef: 'refs/remotes/origin/main' }) - ).rejects.toThrow('Local base ref worktree has tracked changes.') + ).resolves.toEqual({ + status: 'skipped_dirty_worktree', + ownerWorktreePath: reportedWorktreePath(tmpDir) + }) - const actual = execFileSync('git', ['rev-parse', 'HEAD'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() - expect(actual).toBe(firstSha) + expect(revParse('HEAD')).toBe(localSha) await expect(fs.readFile(path.join(tmpDir, 'base.txt'), 'utf-8')).resolves.toBe('local dirty') }) - it('rejects when the caller-supplied owner path is not the checked-out branch owner', async () => { - gitInit(tmpDir) - writeFileSync(path.join(tmpDir, 'base.txt'), 'base') - gitCommit(tmpDir, 'initial') - const branchRef = currentBranchFullRef(tmpDir) - const headSha = execFileSync('git', ['rev-parse', 'HEAD'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() - execFileSync('git', ['update-ref', 'refs/remotes/origin/main', headSha], { - cwd: tmpDir, - stdio: 'pipe' - }) - - await expect( - dispatcher.callRequest('git.refreshLocalBaseRefForWorktreeCreate', { - repoPath: tmpDir, - fullRef: branchRef, - remoteTrackingRef: 'refs/remotes/origin/main', - ownerWorktreePath: path.join(path.dirname(tmpDir), 'different-owner') - }) - ).rejects.toThrow('Local base ref is checked out in a different worktree.') - }) - - it('rejects diverged local refs before mutating', async () => { - gitInit(tmpDir) - writeFileSync(path.join(tmpDir, 'base.txt'), 'base') - gitCommit(tmpDir, 'initial') - execFileSync('git', ['branch', 'main-copy'], { cwd: tmpDir, stdio: 'pipe' }) - writeFileSync(path.join(tmpDir, 'remote.txt'), 'remote') - gitCommit(tmpDir, 'remote update') - const remoteSha = execFileSync('git', ['rev-parse', 'HEAD'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() - execFileSync('git', ['update-ref', 'refs/remotes/origin/main', remoteSha], { - cwd: tmpDir, - stdio: 'pipe' - }) - execFileSync('git', ['checkout', 'main-copy'], { cwd: tmpDir, stdio: 'pipe' }) + it('does not move diverged local refs', async () => { + const { localSha } = initBehindRepo() + execFileSync('git', ['checkout', '-q', '-b', 'main-copy', localSha], { cwd: tmpDir }) writeFileSync(path.join(tmpDir, 'local.txt'), 'local') gitCommit(tmpDir, 'local update') - const localSha = execFileSync('git', ['rev-parse', 'refs/heads/main-copy'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() + const divergedSha = revParse('refs/heads/main-copy') await expect( dispatcher.callRequest('git.refreshLocalBaseRefForWorktreeCreate', { repoPath: tmpDir, fullRef: 'refs/heads/main-copy', - remoteTrackingRef: 'refs/remotes/origin/main', - ownerWorktreePath: tmpDir + remoteTrackingRef: 'refs/remotes/origin/main' }) - ).rejects.toThrow('Local base ref is not a fast-forward update.') + ).resolves.toEqual({ status: 'skipped_not_fast_forward' }) - const actual = execFileSync('git', ['rev-parse', 'refs/heads/main-copy'], { - cwd: tmpDir, - encoding: 'utf-8' - }).trim() - expect(actual).toBe(localSha) + expect(revParse('refs/heads/main-copy')).toBe(divergedSha) }) - it('resets owner worktree to captured remote OID without update-ref', async () => { - const { localDispatcher, gitMock } = setupMockedRefreshHandler() + it('inspects how far local is behind without moving it', async () => { + const { branchRef, localSha, remoteSha } = initBehindRepo() + + await expect( + dispatcher.callRequest('git.inspectLocalBaseRefForWorktreeCreate', { + repoPath: tmpDir, + fullRef: branchRef, + remoteTrackingRef: 'refs/remotes/origin/main' + }) + ).resolves.toEqual({ + status: 'behind', + behind: 1, + localOid: localSha, + remoteOid: remoteSha, + ownerWorktreePath: reportedWorktreePath(tmpDir) + }) + + expect(revParse('HEAD')).toBe(localSha) + await expect(fs.readFile(path.join(tmpDir, 'base.txt'), 'utf-8')).resolves.toBe('base') + }) + + function mockBehindOwnerGit( + gitMock: ReturnType['gitMock'], + onMerge: () => Promise = async () => {} + ) { + let localOid = 'old-local-oid' gitMock.mockImplementation(async (args: string[]) => { - if (args[0] === 'check-ref-format') { + const command = args.find((arg, index) => !arg.startsWith('-') && args[index - 1] !== '-c') + if (command === 'check-ref-format' || command === 'status') { return { stdout: '', stderr: '' } } - if (args[0] === 'rev-parse' && args[2] === 'refs/remotes/origin/main^{commit}') { - return { stdout: 'remote-oid\n', stderr: '' } + if (command === 'rev-parse') { + const oid = args[2] === 'refs/remotes/origin/main^{commit}' ? 'remote-oid' : localOid + return { stdout: `${oid}\n`, stderr: '' } } - if (args[0] === 'rev-parse') { - return { stdout: 'old-local-oid\n', stderr: '' } + if (command === 'rev-list') { + return { stdout: '0\t2\n', stderr: '' } } - if (args[0] === 'merge-base') { - return { stdout: '', stderr: '' } - } - if (args[0] === 'worktree') { + if (command === 'worktree') { return { - stdout: 'worktree /repo\nHEAD old-local-oid\nbranch refs/heads/main\n', + stdout: 'worktree /repo\0HEAD old-local-oid\0branch refs/heads/main\0\0', stderr: '' } } - if (args[0] === 'status') { - return { stdout: '', stderr: '' } + if (command === 'symbolic-ref') { + return { stdout: 'refs/heads/main\n', stderr: '' } } - if (args[0] === 'reset') { + if (command === 'merge') { + await onMerge() + localOid = 'remote-oid' return { stdout: '', stderr: '' } } throw new Error(`unexpected git call: ${args.join(' ')}`) }) + } + + it('fast-forwards the owner with merge --ff-only and hooks disabled, never reset or update-ref', async () => { + const { localDispatcher, gitMock } = setupMockedRefreshHandler() + mockBehindOwnerGit(gitMock) await expect( localDispatcher.callRequest('git.refreshLocalBaseRefForWorktreeCreate', { @@ -331,40 +289,172 @@ describe('GitHandler', () => { fullRef: 'refs/heads/main', remoteTrackingRef: 'refs/remotes/origin/main' }) - ).resolves.toBeUndefined() + ).resolves.toEqual({ status: 'updated', ownerWorktreePath: '/repo' }) - expect(gitMock).toHaveBeenCalledWith( - ['merge-base', '--is-ancestor', 'old-local-oid', 'remote-oid'], - '/repo' + const merge = gitMock.mock.calls.find(([args]) => args.includes('merge')) + expect(merge?.[0]).toEqual( + expect.arrayContaining([ + 'core.hooksPath=/dev/null', + 'branch.main.mergeOptions=', + '--ff-only', + 'recursive', + '--no-verify-signatures', + 'remote-oid' + ]) ) - expect(gitMock).toHaveBeenCalledWith(['reset', '--hard', 'remote-oid'], '/repo') - expect(gitMock.mock.calls.map((call) => call[0])).not.toContainEqual([ - 'update-ref', - 'refs/heads/main', - 'remote-oid', - 'old-local-oid' - ]) + expect(merge?.[1]).toBe('/repo') + const commands = gitMock.mock.calls.map(([args]) => args[0]) + expect(commands).not.toContain('reset') + expect(commands).not.toContain('update-ref') }) - it('fails closed when worktree ownership cannot be listed', async () => { + it('runs one refresh per branch at a time for every client of the relay', async () => { const { localDispatcher, gitMock } = setupMockedRefreshHandler() - gitMock.mockImplementation(async (args: string[]) => { - if (args[0] === 'check-ref-format') { - return { stdout: '', stderr: '' } + let releaseMerge!: () => void + const mergeHeld = new Promise((resolve) => (releaseMerge = resolve)) + mockBehindOwnerGit(gitMock, () => mergeHeld) + const params = { + repoPath: '/repo', + fullRef: 'refs/heads/main', + remoteTrackingRef: 'refs/remotes/origin/main' + } + + const first = localDispatcher.callRequest('git.refreshLocalBaseRefForWorktreeCreate', params) + await vi.waitFor(() => + expect(gitMock.mock.calls.some(([args]) => args.includes('merge'))).toBe(true) + ) + const second = localDispatcher.callRequest('git.refreshLocalBaseRefForWorktreeCreate', params) + // Let the second request get past validation and as far as it can while the merge is held. + await vi.waitFor(() => + expect(gitMock.mock.calls.filter(([args]) => args[0] === 'check-ref-format').length).toBe(4) + ) + await new Promise((resolve) => setTimeout(resolve, 20)) + releaseMerge() + + await expect(first).resolves.toEqual({ status: 'updated', ownerWorktreePath: '/repo' }) + // The joiner's follow-up run finds local already at the target. + await expect(second).resolves.toEqual({ status: 'nothing_to_do' }) + expect(gitMock.mock.calls.filter(([args]) => args.includes('merge'))).toHaveLength(1) + }) + + // A fork on the real host: the checked-out branch behind origin/main, one commit behind upstream/main. + function initForkRepo(): { branchRef: string; upstreamSha: string } { + const { branchRef, localSha, remoteSha } = initBehindRepo() + execFileSync('git', ['checkout', '-q', '-b', 'fork-upstream', remoteSha], { cwd: tmpDir }) + writeFileSync(path.join(tmpDir, 'base.txt'), 'upstream') + gitCommit(tmpDir, 'upstream update') + const upstreamSha = revParse('HEAD') + execFileSync('git', ['checkout', '-q', '-'], { cwd: tmpDir }) + execFileSync('git', ['update-ref', 'refs/remotes/upstream/main', upstreamSha], { + cwd: tmpDir + }) + execFileSync('git', ['branch', '-q', '-D', 'fork-upstream'], { cwd: tmpDir }) + expect(revParse('HEAD')).toBe(localSha) + return { branchRef, upstreamSha } + } + + /** Sends refreshes in order, each reaching the relay's per-branch queue while the first merge is held. */ + async function refreshWhileFirstMergeHeld(branchRef: string, remotes: string[]) { + const { dispatcher: relay, handler } = createGitHandlerRelay() + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: GitHandler really has this private git runner; the spy wraps the real one. + const target = handler as unknown as GitSpyTarget + const realGit = target.git.bind(handler) + let releaseMerge!: () => void + const mergeHeld = new Promise((resolve) => (releaseMerge = resolve)) + let merges = 0 + let activeMerges = 0 + let maxConcurrentMerges = 0 + let refFormatChecks = 0 + vi.spyOn(target, 'git').mockImplementation(async (args, cwd, opts) => { + if (args.includes('check-ref-format')) { + const result = await realGit(args, cwd, opts) + refFormatChecks += 1 + return result } - if (args[0] === 'rev-parse' && args[2] === 'refs/remotes/origin/main^{commit}') { - return { stdout: 'remote-oid\n', stderr: '' } + if (!args.includes('merge')) { + return realGit(args, cwd, opts) } - if (args[0] === 'rev-parse') { - return { stdout: 'old-local-oid\n', stderr: '' } + merges += 1 + activeMerges += 1 + maxConcurrentMerges = Math.max(maxConcurrentMerges, activeMerges) + try { + if (merges === 1) { + await mergeHeld + } + return await realGit(args, cwd, opts) + } finally { + activeMerges -= 1 } - if (args[0] === 'merge-base') { - return { stdout: '', stderr: '' } + }) + + const results: Promise[] = [] + for (const [index, remote] of remotes.entries()) { + results.push( + relay.callRequest('git.refreshLocalBaseRefForWorktreeCreate', { + repoPath: tmpDir, + fullRef: branchRef, + remoteTrackingRef: `refs/remotes/${remote}/main` + }) + ) + if (index === 0) { + await vi.waitFor(() => expect(merges).toBe(1)) + } else { + await vi.waitFor(() => expect(refFormatChecks).toBe(2 * (index + 1))) + await new Promise((resolve) => setTimeout(resolve, 20)) } + } + releaseMerge() + return { results: await Promise.all(results), maxConcurrentMerges: () => maxConcurrentMerges } + } + + it('moves the branch to a target a later client asked for from another remote', async () => { + const { branchRef, upstreamSha } = initForkRepo() + + const { results, maxConcurrentMerges } = await refreshWhileFirstMergeHeld(branchRef, [ + 'origin', + 'upstream', + 'origin' + ]) + + const owner = reportedWorktreePath(tmpDir) + // The third is the existing ahead-of-requested-remote rule (local is past origin/main), not a sharing artifact. + expect(results).toEqual([ + { status: 'updated', ownerWorktreePath: owner }, + { status: 'updated', ownerWorktreePath: owner }, + { status: 'skipped_not_fast_forward' } + ]) + expect(revParse('HEAD')).toBe(upstreamSha) + expect(maxConcurrentMerges()).toBe(1) + }) + + it('never answers a client with the outcome of another remote target', async () => { + const { branchRef, upstreamSha } = initForkRepo() + + const { results, maxConcurrentMerges } = await refreshWhileFirstMergeHeld(branchRef, [ + 'upstream', + 'upstream', + 'origin' + ]) + + // The third is the existing ahead-of-requested-remote rule (local is past origin/main), not a sharing artifact. + expect(results).toEqual([ + { status: 'updated', ownerWorktreePath: reportedWorktreePath(tmpDir) }, + { status: 'nothing_to_do' }, + { status: 'skipped_not_fast_forward' } + ]) + expect(revParse('HEAD')).toBe(upstreamSha) + expect(maxConcurrentMerges()).toBe(1) + }) + + it('reports an error without mutating when worktree ownership cannot be listed', async () => { + const { localDispatcher, gitMock } = setupMockedRefreshHandler() + mockBehindOwnerGit(gitMock) + const base = gitMock.getMockImplementation()! + gitMock.mockImplementation(async (args, cwd) => { if (args[0] === 'worktree') { throw new Error('worktree list failed') } - throw new Error(`unexpected git call: ${args.join(' ')}`) + return base(args, cwd) }) await expect( @@ -373,19 +463,12 @@ describe('GitHandler', () => { fullRef: 'refs/heads/main', remoteTrackingRef: 'refs/remotes/origin/main' }) - ).rejects.toThrow('worktree list failed') + ).resolves.toEqual({ status: 'skipped_error' }) - expect(gitMock.mock.calls.map((call) => call[0])).not.toContainEqual([ - 'update-ref', - 'refs/heads/main', - 'refs/remotes/origin/main', - 'old-local-oid' - ]) - expect(gitMock.mock.calls.map((call) => call[0])).not.toContainEqual([ - 'reset', - '--hard', - 'refs/heads/main' - ]) + const commands = gitMock.mock.calls.map(([args]) => args) + expect(commands.some((args) => args.includes('merge') || args[0] === 'update-ref')).toBe( + false + ) }) }) diff --git a/src/relay/git-handler.test.ts b/src/relay/git-handler.test.ts index 590b22636ba..c6ba3142585 100644 --- a/src/relay/git-handler.test.ts +++ b/src/relay/git-handler.test.ts @@ -73,6 +73,7 @@ describe('GitHandler', () => { expect(methods).toContain('git.removeWorktree') expect(methods).toContain('git.worktreeIsClean') expect(methods).toContain('git.refreshLocalBaseRefForWorktreeCreate') + expect(methods).toContain('git.inspectLocalBaseRefForWorktreeCreate') expect(methods).toContain('git.markRemoteOrcaCreated') expect(methods).toContain('git.renameCurrentBranch') expect(methods).toContain('git.forceDeletePreservedBranch') diff --git a/src/renderer/src/store/slices/worktrees-create-base-status.test.ts b/src/renderer/src/store/slices/worktrees-create-base-status.test.ts index 0bfe28c86c8..fcee4b6a3c2 100644 --- a/src/renderer/src/store/slices/worktrees-create-base-status.test.ts +++ b/src/renderer/src/store/slices/worktrees-create-base-status.test.ts @@ -389,7 +389,7 @@ describe('createWorktree base status merge', () => { await store.getState().createWorktree('repo1', 'feature', 'origin/main') expect(toast.warning).toHaveBeenCalledWith('Local main was not refreshed for "feature-wt"', { - id: 'local-base-ref-refresh-failed:repo1::/path/wt1:main', + id: 'local-base-ref-refresh-failed:repo1:main', description: expect.stringContaining(expectedReason), duration: Infinity, dismissible: true @@ -457,11 +457,38 @@ describe('createWorktree base status merge', () => { expect(toast.warning).toHaveBeenCalledWith( 'Local main was not refreshed for "feature"', expect.objectContaining({ - id: 'local-base-ref-refresh-failed:repo1::/path/wt1:main' + id: 'local-base-ref-refresh-failed:repo1:main' }) ) }) + // Creates that joined one refresh report the same fact, so they share one toast per repo+branch. + it('reuses one toast id for every create of the same repo and base branch', async () => { + const store = createTestStore() + const refresh = { + status: 'skipped_error', + baseRef: 'origin/main', + localBranch: 'main' + } as const + for (const [id, repoId] of [ + ['repo1::/path/wt1', 'repo1'], + ['repo1::/path/wt2', 'repo1'], + ['repo2::/path/wt3', 'repo2'] + ]) { + mockApi.worktrees.create.mockResolvedValueOnce({ + worktree: makeWorktree({ id, repoId, path: id.split('::')[1] }), + localBaseRefRefresh: refresh + }) + await store.getState().createWorktree(repoId, 'feature', 'origin/main') + } + + expect(vi.mocked(toast.warning).mock.calls.map(([, options]) => options?.id)).toEqual([ + 'local-base-ref-refresh-failed:repo1:main', + 'local-base-ref-refresh-failed:repo1:main', + 'local-base-ref-refresh-failed:repo2:main' + ]) + }) + it('does not warn when the local base ref refresh succeeds', async () => { const store = createTestStore() const wt = makeWorktree({ diff --git a/src/renderer/src/store/slices/worktrees/create/local-base-ref-refresh-toast.ts b/src/renderer/src/store/slices/worktrees/create/local-base-ref-refresh-toast.ts index 5e7ab555c23..c239b82863d 100644 --- a/src/renderer/src/store/slices/worktrees/create/local-base-ref-refresh-toast.ts +++ b/src/renderer/src/store/slices/worktrees/create/local-base-ref-refresh-toast.ts @@ -39,7 +39,7 @@ export function localBaseRefRefreshFailureDetail(result: LocalBaseRefRefreshResu export function showLocalBaseRefRefreshToast( result: LocalBaseRefRefreshResult | undefined, - createdWorktree?: Pick + createdWorktree?: Pick ): void { if (!result || result.status === 'updated') { return @@ -48,7 +48,7 @@ export function showLocalBaseRefRefreshToast( const worktreeName = createdWorktree ? resolveWorktreeDisplayName(createdWorktree).trim() : '' const detail = localBaseRefRefreshFailureDetail(result) - // Why: Infinity so create-time failures aren't buried; id is per worktree so each create stays attributable. + // Why: Infinity so create-time failures aren't buried; one id per repo and branch because creates toward one target share a run, so its result is one fact, not one per create. toast.warning( worktreeName ? translate( @@ -60,7 +60,7 @@ export function showLocalBaseRefRefreshToast( value0: result.localBranch }), { - id: `local-base-ref-refresh-failed:${createdWorktree?.id ?? 'unknown'}:${result.localBranch}`, + id: `local-base-ref-refresh-failed:${createdWorktree?.repoId ?? 'unknown'}:${result.localBranch}`, description: worktreeName ? translate( 'auto.store.slices.worktrees.localBaseRefRefreshFailedDescriptionNamed', diff --git a/src/shared/coalescing-keyed-runner.test.ts b/src/shared/coalescing-keyed-runner.test.ts new file mode 100644 index 00000000000..42d7d0c4e2f --- /dev/null +++ b/src/shared/coalescing-keyed-runner.test.ts @@ -0,0 +1,155 @@ +import { describe, expect, it, vi } from 'vitest' +import { createCoalescingKeyedRunner } from './coalescing-keyed-runner' + +function deferred() { + let resolve!: (value: T) => void + let reject!: (reason: unknown) => void + const promise = new Promise((res, rej) => { + resolve = res + reject = rej + }) + return { promise, resolve, reject } +} + +describe('createCoalescingKeyedRunner', () => { + it('runs a lone call once and returns its result', async () => { + const run = createCoalescingKeyedRunner() + const work = vi.fn(async () => 'done') + + await expect(run('repo', 'target', work)).resolves.toBe('done') + expect(work).toHaveBeenCalledTimes(1) + }) + + it('folds a burst that arrives mid-run into one trailing run that every burst caller shares', async () => { + const run = createCoalescingKeyedRunner() + const first = deferred() + const firstWork = vi.fn(() => first.promise) + const leading = run('repo', 'target', firstWork) + await vi.waitFor(() => expect(firstWork).toHaveBeenCalledTimes(1)) + + const burstWork = Array.from({ length: 5 }, (_, index) => + vi.fn(async () => `trailing-${index}`) + ) + const burst = burstWork.map((work) => run('repo', 'target', work)) + expect(burstWork.every((work) => work.mock.calls.length === 0)).toBe(true) + + first.resolve('leading') + await expect(leading).resolves.toBe('leading') + // The trailing run uses the latest caller's work, and every burst caller gets its result. + await expect(Promise.all(burst)).resolves.toEqual(Array(5).fill('trailing-4')) + expect(burstWork.slice(0, 4).every((work) => work.mock.calls.length === 0)).toBe(true) + expect(burstWork[4]).toHaveBeenCalledTimes(1) + }) + + it('queues a caller with a different share key behind the run in flight, never beside it', async () => { + const run = createCoalescingKeyedRunner() + const first = deferred() + let active = 0 + let maxActive = 0 + const tracked = (work: () => Promise) => async () => { + active += 1 + maxActive = Math.max(maxActive, active) + try { + return await work() + } finally { + active -= 1 + } + } + const leadingWork = tracked(() => first.promise) + const leading = run('repo', 'origin', leadingWork) + const other = vi.fn(async () => 'upstream result') + const queued = run('repo', 'upstream', tracked(other)) + await Promise.resolve() + expect(other).not.toHaveBeenCalled() + + first.resolve('origin result') + await expect(Promise.all([leading, queued])).resolves.toEqual([ + 'origin result', + 'upstream result' + ]) + expect(maxActive).toBe(1) + }) + + it('answers every caller from a run of its own share key, in arrival order', async () => { + const run = createCoalescingKeyedRunner() + const first = deferred() + const order: string[] = [] + const work = (shareKey: string, caller: number) => async () => { + order.push(`${shareKey}:${caller}`) + return `${shareKey}:${caller}` + } + const leading = run('repo', 'origin', () => first.promise) + const callers = [ + run('repo', 'upstream', work('upstream', 1)), + run('repo', 'origin', work('origin', 2)), + run('repo', 'upstream', work('upstream', 3)) + ] + + first.resolve('origin:0') + await expect(leading).resolves.toBe('origin:0') + // Each queued run uses its latest joiner's work; the runs start in the order they were queued. + await expect(Promise.all(callers)).resolves.toEqual(['upstream:3', 'origin:2', 'upstream:3']) + expect(order).toEqual(['upstream:3', 'origin:2']) + }) + + it('bounds a mixed burst to one run per distinct share key beyond the one in flight', async () => { + const run = createCoalescingKeyedRunner() + const first = deferred() + const work = vi.fn(async () => 'done') + const leading = run('repo', 'origin', () => first.promise) + const burst = Array.from({ length: 12 }, (_, index) => + run('repo', ['origin', 'upstream', 'fork'][index % 3] ?? 'origin', work) + ) + + first.resolve('done') + await Promise.all([leading, ...burst]) + expect(work).toHaveBeenCalledTimes(3) + }) + + it('starts the trailing run even when the run in flight rejects', async () => { + const run = createCoalescingKeyedRunner() + const first = deferred() + const leading = run('repo', 'target', () => first.promise) + const trailing = run('repo', 'target', async () => 'after failure') + + first.reject(new Error('boom')) + await expect(leading).rejects.toThrow('boom') + await expect(trailing).resolves.toBe('after failure') + }) + + it('does not let a rejected run block the next call', async () => { + const run = createCoalescingKeyedRunner() + + const failing = async (): Promise => { + throw new Error('boom') + } + await expect(run('repo', 'target', failing)).rejects.toThrow('boom') + await expect(run('repo', 'target', async () => 'next')).resolves.toBe('next') + }) + + it('runs different keys concurrently', async () => { + const run = createCoalescingKeyedRunner() + const held = deferred() + const heldResult = run('repo-a', 'target', () => held.promise) + const other = vi.fn(async () => 'other') + + await expect(run('repo-b', 'target', other)).resolves.toBe('other') + expect(other).toHaveBeenCalledTimes(1) + held.resolve('held') + await expect(heldResult).resolves.toBe('held') + }) + + it('starts fresh once a key has settled, instead of joining a finished run', async () => { + const run = createCoalescingKeyedRunner() + let runs = 0 + const work = async () => ++runs + + await expect(run('repo', 'target', work)).resolves.toBe(1) + await expect(run('repo', 'target', work)).resolves.toBe(2) + + const leading = run('repo', 'target', work) + const trailing = run('repo', 'target', work) + await expect(Promise.all([leading, trailing])).resolves.toEqual([3, 4]) + await expect(run('repo', 'target', work)).resolves.toBe(5) + }) +}) diff --git a/src/shared/coalescing-keyed-runner.ts b/src/shared/coalescing-keyed-runner.ts new file mode 100644 index 00000000000..e6436ff88dd --- /dev/null +++ b/src/shared/coalescing-keyed-runner.ts @@ -0,0 +1,64 @@ +export type CoalescingKeyedRunner = ( + key: string, + shareKey: string, + work: () => Promise +) => Promise + +type QueuedRun = { + shareKey: string + work: () => Promise + start: () => void + result: Promise +} + +/** + * At most one run per key at a time, in arrival order. A caller joins a queued, not yet started + * run with the same `shareKey`, which then uses the latest joiner's `work`; otherwise it queues a + * new run. So every result comes from a run of the caller's own `shareKey`, and a burst costs at + * most one run per distinct `shareKey` beyond the one in flight. + */ +export function createCoalescingKeyedRunner(): CoalescingKeyedRunner { + const queues = new Map[]>() + + const drain = async (key: string, queue: QueuedRun[]): Promise => { + for (let run = queue.shift(); run; run = queue.shift()) { + run.start() + await settled(run.result) + } + queues.delete(key) + } + + return (key, shareKey, work) => { + const queue = queues.get(key) + const joinable = queue?.find((run) => run.shareKey === shareKey) + if (joinable) { + joinable.work = work + return joinable.result + } + const run = createQueuedRun(shareKey, work) + if (queue) { + queue.push(run) + } else { + const fresh = [run] + queues.set(key, fresh) + void drain(key, fresh) + } + return run.result + } +} + +function createQueuedRun(shareKey: string, work: () => Promise): QueuedRun { + let start!: () => void + const started = new Promise((resolve) => { + start = resolve + }) + const run: QueuedRun = { shareKey, work, start, result: started.then(() => run.work()) } + return run +} + +function settled(promise: Promise): Promise { + return promise.then( + () => undefined, + () => undefined + ) +} diff --git a/src/shared/git-binary-compatibility.test.ts b/src/shared/git-binary-compatibility.test.ts index af9dcb9d2af..8575f0fddd2 100644 --- a/src/shared/git-binary-compatibility.test.ts +++ b/src/shared/git-binary-compatibility.test.ts @@ -1,7 +1,7 @@ import { execFile } from 'node:child_process' import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' -import { join } from 'node:path' +import { dirname, join } from 'node:path' import { promisify } from 'node:util' import { afterAll, beforeAll, describe, expect, it } from 'vitest' import { @@ -24,6 +24,8 @@ import { gitlabMergeRequestHeadLocalRef, reviewHeadRemoteRefComponent } from './review-head-tracking-ref' +import { parseWorktreeList } from './git-worktree-porcelain-parser' +import { fastForwardLocalBaseBranch } from './worktree/local-base-branch-fast-forward' const execFileAsync = promisify(execFile) const image = process.env.ORCA_GIT_COMPAT_IMAGE @@ -581,4 +583,67 @@ describeBinaryCompatibility('real Git binary compatibility', () => { const included = await listFiles({ includePattern: 'vendored' }) expect(included).toEqual(['vendored/inner.txt']) }) + + // Why pin this: the owner-checkout fast-forward overrides the user's merge settings with flags + // and `-c` keys; every one must parse on the baseline, and a branch-level `-s ours` must not win. + it('fast-forwards a checked-out base branch with the exact owner arguments', async () => { + const worktree = 'compat-ff-wt' + const branch = 'compat-ff-main' + const marker = join(repoPath, worktree, 'compat-ff-hook-ran') + const hookPath = join(repoPath, '.git', 'hooks', 'post-merge') + await runGit(['worktree', 'add', '-q', '-b', branch, worktree]) + const localOid = (await runGit(['-C', worktree, 'rev-parse', 'HEAD'])).stdout.trim() + await writeFile(join(repoPath, worktree, 'compat-ff-added.txt'), 'upstream\n') + await runGit(['-C', worktree, 'add', 'compat-ff-added.txt']) + await runGit(['-C', worktree, 'commit', '-qm', 'upstream']) + const remoteOid = (await runGit(['-C', worktree, 'rev-parse', 'HEAD'])).stdout.trim() + await runGit(['update-ref', `refs/remotes/origin/${branch}`, remoteOid]) + await runGit(['-C', worktree, 'reset', '-q', '--hard', localOid]) + await runGit(['config', `branch.${branch}.mergeOptions`, '-s ours']) + // Why: an uninstalled source-built Git (the CI baseline) has no templates, so no hooks dir. + await mkdir(dirname(hookPath), { recursive: true }) + await writeFile(hookPath, '#!/bin/sh\necho ran > compat-ff-hook-ran\n', { mode: 0o755 }) + const merges: string[][] = [] + // Why `-C`: in the Docker lane, paths Git reports are container paths, not host ones. + const git = { + exec: (args: string[], cwd: string) => { + if (args.includes('merge')) { + merges.push(args) + } + return runGit(['-C', cwd, ...args]) + }, + listWorktrees: async (path: string) => + parseWorktreeList((await runGit(['-C', path, 'worktree', 'list', '--porcelain'])).stdout) + } + + try { + const outcome = await fastForwardLocalBaseBranch(git, { + repoPath: image ? '/repo' : repoPath, + fullRef: `refs/heads/${branch}`, + remoteTrackingRef: `refs/remotes/origin/${branch}` + }) + + expect(outcome).toMatchObject({ status: 'updated' }) + expect(merges).toHaveLength(1) + await expect( + runGit(['rev-list', '--parents', '-1', `refs/heads/${branch}`]) + ).resolves.toMatchObject({ stdout: `${remoteOid} ${localOid}\n` }) + await expect( + readFile(join(repoPath, worktree, 'compat-ff-added.txt'), 'utf-8') + ).resolves.toBe('upstream\n') + await expect(readFile(marker, 'utf-8')).rejects.toMatchObject({ code: 'ENOENT' }) + + // Control: the hook and the branch setting are both live for a plain fast-forward. + await runGit(['-C', worktree, 'reset', '-q', '--hard', localOid]) + await runGit(['-C', worktree, 'merge', '--ff-only', '-q', remoteOid]) + await expect(readFile(marker, 'utf-8')).resolves.toBe('ran\n') + await expect(runGit(['rev-parse', `refs/heads/${branch}`])).resolves.not.toMatchObject({ + stdout: `${remoteOid}\n` + }) + } finally { + await rm(hookPath, { force: true }) + await runGit(['config', '--unset', `branch.${branch}.mergeOptions`]) + await runGit(['worktree', 'remove', '--force', worktree]) + } + }) }) diff --git a/src/shared/git-lock-contention.test.ts b/src/shared/git-lock-contention.test.ts new file mode 100644 index 00000000000..088f79b27ab --- /dev/null +++ b/src/shared/git-lock-contention.test.ts @@ -0,0 +1,34 @@ +import { describe, expect, it } from 'vitest' +import { isGitLockContentionFailure } from './git-lock-contention' + +describe('isGitLockContentionFailure', () => { + it.each([ + "fatal: Unable to create '/repo/.git/index.lock': File exists.", + "fatal: Unable to create 'C:/repo/.git/worktrees/wt/index.lock': File exists.", + "fatal: cannot lock ref 'refs/heads/main': Unable to create '/repo/.git/refs/heads/main.lock': File exists.", + "error: Unable to create '/repo/.git/packed-refs.lock': File exists.", + 'error: could not lock config file .git/config: File exists' + ])('matches %s', (stderr) => { + expect(isGitLockContentionFailure(Object.assign(new Error('Command failed'), { stderr }))).toBe( + true + ) + }) + + it.each([ + "fatal: cannot lock ref 'refs/heads/main': is at abc but expected def", + 'fatal: ambiguous argument', + 'error: Your local changes to the following files would be overwritten by checkout' + ])('does not match %s', (message) => { + expect(isGitLockContentionFailure(new Error(message))).toBe(false) + }) + + it('reads the relay error message, which carries git stderr', () => { + expect( + isGitLockContentionFailure( + new Error( + "Command failed: git reset --hard abc\nfatal: Unable to create '/r/.git/index.lock': File exists." + ) + ) + ).toBe(true) + }) +}) diff --git a/src/shared/git-lock-contention.ts b/src/shared/git-lock-contention.ts new file mode 100644 index 00000000000..01780b104a1 --- /dev/null +++ b/src/shared/git-lock-contention.ts @@ -0,0 +1,32 @@ +import { readGitCommandFailureText } from './git-command-failure-text' + +// Why: only "another git process holds the lock" is transient; a CAS mismatch (`cannot lock ref ...: is at X but expected Y`) is not. +const GIT_LOCK_CONTENTION_PATTERNS = [ + /Unable to create '[^'\n]*\.lock': File exists/i, + /could not lock config file [^\n]*: File exists/i +] + +export const GIT_LOCK_CONTENTION_RETRY_DELAYS_MS: readonly number[] = [50, 150, 400] + +export function isGitLockContentionFailure(error: unknown): boolean { + const text = readGitCommandFailureText(error) + return GIT_LOCK_CONTENTION_PATTERNS.some((pattern) => pattern.test(text)) +} + +/** Re-runs `attempt` after each lock-contention failure; any other failure, or the last one, is thrown. */ +export async function retryOnGitLockContention( + attempt: () => Promise, + delaysMs: readonly number[] = GIT_LOCK_CONTENTION_RETRY_DELAYS_MS +): Promise { + for (let retry = 0; ; retry += 1) { + try { + return await attempt() + } catch (error) { + const delayMs = delaysMs[retry] + if (delayMs === undefined || !isGitLockContentionFailure(error)) { + throw error + } + await new Promise((resolve) => setTimeout(resolve, delayMs)) + } + } +} diff --git a/src/shared/git-show-ref-no-match.ts b/src/shared/git-show-ref-no-match.ts new file mode 100644 index 00000000000..e35f98405a1 --- /dev/null +++ b/src/shared/git-show-ref-no-match.ts @@ -0,0 +1,16 @@ +/** True when `git show-ref --verify --quiet` proved the ref absent, as opposed to the probe itself failing. */ +export function isShowRefNoMatchError(error: unknown): boolean { + // Git reports a missing ref as numeric exit status 1. Keep string-valued + // transport/error codes (including a relay that happens to use `"1"`) in + // the unknown bucket so SSH loss cannot look like an absent ref. + if (typeof error !== 'object' || error === null || !('code' in error) || error.code !== 1) { + return false + } + // `--quiet` makes Git print nothing for a missing ref, but a wrapper that + // also exits 1 always explains itself: `wsl.exe` on a dead distro, a relay + // transport error. Empty stderr is what separates proven absence from a + // probe that never ran. A runner that reports no stderr at all (the SSH + // provider) keeps its existing exit-code contract. + const stderr = 'stderr' in error ? error.stderr : undefined + return stderr === undefined || stderr === null || String(stderr).trim().length === 0 +} diff --git a/src/shared/worktree/local-base-branch-fast-forward-real-git.test.ts b/src/shared/worktree/local-base-branch-fast-forward-real-git.test.ts new file mode 100644 index 00000000000..4976836fe13 --- /dev/null +++ b/src/shared/worktree/local-base-branch-fast-forward-real-git.test.ts @@ -0,0 +1,240 @@ +import { execFile, execFileSync } from 'node:child_process' +import { existsSync } from 'node:fs' +import { chmod, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { parseWorktreeList } from '../git-worktree-porcelain-parser' +import { + fastForwardLocalBaseBranch, + type LocalBaseBranchGit +} from './local-base-branch-fast-forward' + +const tempRoots: string[] = [] + +// Why: keep the user's global/system config (hooksPath, autocrlf, signing) out of these repos. +function isolatedGitEnv(root: string): NodeJS.ProcessEnv { + return { + ...process.env, + HOME: root, + USERPROFILE: root, + XDG_CONFIG_HOME: root, + GIT_CONFIG_NOSYSTEM: '1', + GIT_CONFIG_GLOBAL: join(root, 'empty-gitconfig') + } +} + +type Fixture = { + repoPath: string + env: NodeJS.ProcessEnv + localOid: string + remoteOid: string + git: (args: string[], cwd?: string) => string +} + +// `main` is checked out one commit behind `refs/remotes/origin/main`, which edits version.txt and adds added.txt. +async function createBehindRepo(): Promise { + const root = await mkdtemp(join(tmpdir(), 'orca-local-base-ff-')) + tempRoots.push(root) + await writeFile(join(root, 'empty-gitconfig'), '') + const env = isolatedGitEnv(root) + const repoPath = join(root, 'repo') + const git = (args: string[], cwd = repoPath) => + execFileSync('git', args, { cwd, env, encoding: 'utf8', stdio: 'pipe' }).trim() + git(['init', '--quiet', repoPath], root) + git(['symbolic-ref', 'HEAD', 'refs/heads/main']) + git(['config', 'user.email', 'test@example.com']) + git(['config', 'user.name', 'Test User']) + git(['config', 'core.autocrlf', 'false']) + git(['config', 'commit.gpgsign', 'false']) + await writeFile(join(repoPath, 'version.txt'), 'one\n') + git(['add', 'version.txt']) + git(['commit', '--quiet', '-m', 'one']) + const localOid = git(['rev-parse', 'HEAD']) + git(['checkout', '--quiet', '-b', 'upstream']) + await writeFile(join(repoPath, 'version.txt'), 'two\n') + await writeFile(join(repoPath, 'added.txt'), 'from upstream\n') + git(['add', 'version.txt', 'added.txt']) + git(['commit', '--quiet', '-m', 'two']) + const remoteOid = git(['rev-parse', 'HEAD']) + git(['checkout', '--quiet', 'main']) + git(['update-ref', 'refs/remotes/origin/main', remoteOid]) + git(['branch', '--quiet', '-D', 'upstream']) + return { repoPath, env, localOid, remoteOid, git } +} + +/** Real git; `beforeMerge` runs just before the owner fast-forward, after the inspection passed. */ +function realGit(fixture: Fixture, beforeMerge?: () => Promise | void) { + let merges = 0 + const run = (args: string[], cwd: string) => + new Promise<{ stdout: string }>((resolve, reject) => { + execFile('git', args, { cwd, env: fixture.env, encoding: 'utf8' }, (error, stdout, stderr) => + error ? reject(Object.assign(error, { stdout, stderr })) : resolve({ stdout }) + ) + }) + const git: LocalBaseBranchGit = { + exec: async (args, cwd) => { + if (args.includes('merge')) { + merges += 1 + await beforeMerge?.() + } + return run(args, cwd) + }, + listWorktrees: async (repoPath) => + parseWorktreeList((await run(['worktree', 'list', '--porcelain'], repoPath)).stdout) + } + return { git, merges: () => merges } +} + +function fastForward(fixture: Fixture, git: LocalBaseBranchGit) { + return fastForwardLocalBaseBranch(git, { + repoPath: fixture.repoPath, + fullRef: 'refs/heads/main', + remoteTrackingRef: 'refs/remotes/origin/main' + }) +} + +afterEach(async () => { + await Promise.all(tempRoots.splice(0).map((root) => rm(root, { recursive: true, force: true }))) +}) + +describe('fastForwardLocalBaseBranch against real Git', () => { + it('leaves an untracked file at a path the new commit adds, and does not move main', async () => { + const fixture = await createBehindRepo() + await writeFile(join(fixture.repoPath, 'added.txt'), 'my notes\n') + + const outcome = await fastForward(fixture, realGit(fixture).git) + + expect(outcome.status).toBe('skipped_dirty_worktree') + expect(await readFile(join(fixture.repoPath, 'added.txt'), 'utf8')).toBe('my notes\n') + expect(fixture.git(['rev-parse', 'main'])).toBe(fixture.localOid) + }) + + it('leaves an ignored file at a path the new commit adds, and does not move main', async () => { + const fixture = await createBehindRepo() + await writeFile(join(fixture.repoPath, '.git', 'info', 'exclude'), 'added.txt\n') + await writeFile(join(fixture.repoPath, 'added.txt'), 'SECRET=mine\n') + + const outcome = await fastForward(fixture, realGit(fixture).git) + + expect(outcome.status).toBe('skipped_dirty_worktree') + expect(await readFile(join(fixture.repoPath, 'added.txt'), 'utf8')).toBe('SECRET=mine\n') + expect(fixture.git(['rev-parse', 'main'])).toBe(fixture.localOid) + // Control: a plain fast-forward silently replaces the ignored file. + fixture.git(['merge', '--ff-only', '--quiet', fixture.remoteOid]) + expect(await readFile(join(fixture.repoPath, 'added.txt'), 'utf8')).toBe('from upstream\n') + }) + + it('moves main without running the post-merge hook of the checkout', async () => { + const fixture = await createBehindRepo() + const marker = join(fixture.repoPath, '..', 'hook-ran') + const hookPath = join(fixture.repoPath, '.git', 'hooks', 'post-merge') + await writeFile(hookPath, `#!/bin/sh\necho ran > "${marker.replace(/\\/g, '/')}"\n`) + await chmod(hookPath, 0o755) + + const outcome = await fastForward(fixture, realGit(fixture).git) + + expect(outcome).toMatchObject({ status: 'updated' }) + expect(fixture.git(['rev-parse', 'main'])).toBe(fixture.remoteOid) + expect(await readFile(join(fixture.repoPath, 'version.txt'), 'utf8')).toBe('two\n') + expect(existsSync(marker)).toBe(false) + // Control: the hook is live, so only the override kept it from running. + fixture.git(['reset', '--quiet', '--hard', fixture.localOid]) + fixture.git(['merge', '--ff-only', '--quiet', fixture.remoteOid]) + expect(existsSync(marker)).toBe(true) + }) + + // Why: each setting is live (the control shows a plain `merge --ff-only` obeying it), yet the owner + // update must still land main exactly on the target as a fast-forward. + it.each([ + ['merge.verifySignatures', 'true', 'refuses'], + ['branch.main.mergeOptions', '--verify-signatures', 'refuses'], + ['branch.main.mergeOptions', '-s ours', 'merge-commit'], + ['pull.twohead', 'ours', 'merge-commit'], + ['branch.main.mergeOptions', '--squash', 'stages-only'] + ] as const)( + 'fast-forwards main exactly to the target despite %s=%s', + async (key, value, control) => { + const fixture = await createBehindRepo() + fixture.git(['config', key, value]) + + const outcome = await fastForward(fixture, realGit(fixture).git) + + expect(outcome).toMatchObject({ status: 'updated' }) + expect(fixture.git(['rev-parse', 'main'])).toBe(fixture.remoteOid) + expect(fixture.git(['rev-list', '--parents', '-1', 'main'])).toBe( + `${fixture.remoteOid} ${fixture.localOid}` + ) + expect(await readFile(join(fixture.repoPath, 'added.txt'), 'utf8')).toBe('from upstream\n') + expect(fixture.git(['status', '--porcelain'])).toBe('') + + fixture.git(['reset', '--quiet', '--hard', fixture.localOid]) + const plainMerge = () => fixture.git(['merge', '--ff-only', '--quiet', fixture.remoteOid]) + if (control === 'refuses') { + expect(plainMerge).toThrow(/does not have a GPG signature/) + return + } + plainMerge() + if (control === 'merge-commit') { + // A merge commit that keeps local's tree and drops upstream's changes. + expect(fixture.git(['rev-list', '--parents', '-1', 'main'])).toMatch( + new RegExp(` ${fixture.localOid} ${fixture.remoteOid}$`) + ) + expect(existsSync(join(fixture.repoPath, 'added.txt'))).toBe(false) + } else { + expect(fixture.git(['rev-parse', 'main'])).toBe(fixture.localOid) + expect(fixture.git(['status', '--porcelain'])).not.toBe('') + } + } + ) + + it('keeps a tracked edit made after the inspection instead of overwriting it', async () => { + const fixture = await createBehindRepo() + const edited = join(fixture.repoPath, 'version.txt') + + const outcome = await fastForward( + fixture, + realGit(fixture, () => writeFile(edited, 'my edit\n')).git + ) + + expect(outcome.status).not.toBe('updated') + expect(outcome.status).toBe('skipped_dirty_worktree') + expect(await readFile(edited, 'utf8')).toBe('my edit\n') + expect(fixture.git(['rev-parse', 'main'])).toBe(fixture.localOid) + }) + + it('keeps a commit made on main after the inspection', async () => { + const fixture = await createBehindRepo() + let userCommit = '' + const commitOnMain = async () => { + await writeFile(join(fixture.repoPath, 'local.txt'), 'local work\n') + fixture.git(['add', 'local.txt']) + fixture.git(['commit', '--quiet', '-m', 'local work']) + userCommit = fixture.git(['rev-parse', 'HEAD']) + } + + const outcome = await fastForward(fixture, realGit(fixture, commitOnMain).git) + + expect(outcome.status).not.toBe('updated') + expect(outcome.status).toBe('skipped_not_fast_forward') + expect(fixture.git(['rev-parse', 'main'])).toBe(userCommit) + }) + + it('moves a branch no worktree has checked out and records why in its reflog', async () => { + const fixture = await createBehindRepo() + fixture.git(['checkout', '--quiet', '-b', 'develop']) + const real = realGit(fixture) + + const outcome = await fastForward(fixture, real.git) + + expect(outcome).toEqual({ status: 'updated' }) + expect(real.merges()).toBe(0) + expect(fixture.git(['rev-parse', 'main'])).toBe(fixture.remoteOid) + expect(fixture.git(['reflog', 'show', '--format=%gs', '-1', 'main'])).toBe( + 'orca: fast-forward to refs/remotes/origin/main' + ) + // The checked-out branch and its files are untouched. + expect(fixture.git(['symbolic-ref', 'HEAD'])).toBe('refs/heads/develop') + expect(await readFile(join(fixture.repoPath, 'version.txt'), 'utf8')).toBe('one\n') + }) +}) diff --git a/src/shared/worktree/local-base-branch-fast-forward.test.ts b/src/shared/worktree/local-base-branch-fast-forward.test.ts new file mode 100644 index 00000000000..b3dfaa1ed01 --- /dev/null +++ b/src/shared/worktree/local-base-branch-fast-forward.test.ts @@ -0,0 +1,475 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + fastForwardLocalBaseBranch, + inspectLocalBaseBranch, + parseLocalBaseBranchFastForwardOutcome, + readFastForwardableBehindCount, + toLocalBaseRefRefreshResult, + type LocalBaseBranchGit +} from './local-base-branch-fast-forward' + +const REFS = { + repoPath: '/repo', + fullRef: 'refs/heads/main', + remoteTrackingRef: 'refs/remotes/origin/main' +} +const OWNER_MERGE_ARGS = [ + '-c', + 'core.hooksPath=/dev/null', + '-c', + 'gc.auto=0', + '-c', + 'maintenance.auto=false', + '-c', + 'merge.autoStash=false', + '-c', + 'branch.main.mergeOptions=', + 'merge', + '--ff-only', + '-s', + 'recursive', + '--no-verify-signatures', + '--no-overwrite-ignore', + '--no-stat', + '-q', + 'remote-main' +] +const INDEX_LOCK_ERROR = Object.assign(new Error('Command failed: git merge'), { + stderr: "fatal: Unable to create '/repo-main/.git/index.lock': File exists." +}) +const MUTATIONS = new Set(['merge', 'update-ref', 'reset', 'checkout']) + +type Reply = { stdout: string } | Error +type Handler = (args: string[], cwd: string) => Reply | Promise + +/** The git subcommand, past leading global options such as `-c key=value`. */ +function commandOf(args: readonly string[]): string { + for (let index = 0; index < args.length; index += 1) { + if (args[index] === '-c') { + index += 1 + } else if (!args[index].startsWith('-')) { + return args[index] + } + } + return '' +} + +function createFakeGit( + overrides: Record = {}, + worktrees: () => { path: string; branch?: string | null }[] | Promise = () => [ + { path: '/repo', branch: 'refs/heads/develop' }, + { path: '/repo-main', branch: 'refs/heads/main' } + ] +) { + // Where the local branch points; a successful merge moves it to `afterMerge`, else its target. + const local: { oid: string; afterMerge?: string } = { oid: 'old-main' } + const defaults: Record = { + 'rev-parse': (args) => ({ + stdout: args[2]?.startsWith('refs/heads/') ? `${local.oid}\n` : 'remote-main\n' + }), + 'rev-list': () => ({ stdout: '0\t3\n' }), + status: () => ({ stdout: '' }), + 'symbolic-ref': () => ({ stdout: 'refs/heads/main\n' }), + merge: () => ({ stdout: '' }), + 'update-ref': () => ({ stdout: '' }), + 'show-ref': () => ({ stdout: '' }), + // Default: local does not contain the target, so a failed move stays a failure. + 'merge-base': () => Object.assign(new Error('not an ancestor'), { code: 1 }) + } + const calls: { args: string[]; cwd: string }[] = [] + const git: LocalBaseBranchGit = { + exec: async (args, cwd) => { + calls.push({ args, cwd }) + const handler = overrides[commandOf(args)] ?? defaults[commandOf(args)] + if (!handler) { + throw new Error(`unexpected git ${args.join(' ')}`) + } + const reply = await handler(args, cwd) + if (reply instanceof Error) { + throw reply + } + if (commandOf(args) === 'merge') { + local.oid = local.afterMerge ?? args.at(-1) ?? '' + } + return reply + }, + listWorktrees: vi.fn(async () => worktrees()) + } + const commands = () => calls.map(({ args }) => commandOf(args)) + const mutations = () => calls.filter(({ args }) => MUTATIONS.has(commandOf(args))) + return { git, calls, commands, mutations, local } +} + +afterEach(() => { + vi.useRealTimers() +}) + +describe('inspectLocalBaseBranch', () => { + it('has nothing to do when local already matches, without reading worktrees', async () => { + const fake = createFakeGit({ 'rev-parse': () => ({ stdout: 'same\n' }) }) + + await expect(inspectLocalBaseBranch(fake.git, REFS)).resolves.toEqual({ + status: 'nothing_to_do' + }) + expect(fake.commands()).toEqual(['rev-parse', 'rev-parse']) + expect(fake.git.listWorktrees).not.toHaveBeenCalled() + }) + + // #15331: a branch that does not exist yet cannot be stale. + it('has nothing to do when the local branch is proven absent', async () => { + const fake = createFakeGit({ + 'rev-parse': (args) => + args[2] === 'refs/heads/main^{commit}' ? new Error('unknown revision') : { stdout: 'x\n' }, + 'show-ref': () => Object.assign(new Error('missing'), { code: 1, stderr: '' }) + }) + + await expect(inspectLocalBaseBranch(fake.git, REFS)).resolves.toEqual({ + status: 'nothing_to_do' + }) + expect(fake.calls.at(-1)).toEqual({ + args: ['show-ref', '--verify', '--quiet', '--', 'refs/heads/main'], + cwd: '/repo' + }) + }) + + it.each([ + ['local is ahead', { 'rev-list': () => ({ stdout: '2\t0\n' }) }], + ['local diverged', { 'rev-list': () => ({ stdout: '1\t3\n' }) }], + ['the counts are unreadable', { 'rev-list': () => ({ stdout: 'garbage\n' }) }], + [ + 'the probe fails but the branch exists', + { 'rev-list': () => new Error('rev-list failed'), 'show-ref': () => ({ stdout: '' }) } + ], + [ + 'the presence probe itself cannot run', + { + 'rev-parse': () => new Error('unknown revision'), + 'show-ref': () => + Object.assign(new Error('wsl failed'), { code: 1, stderr: 'distro not running' }) + } + ], + [ + 'the remote-tracking ref resolves to nothing', + { + 'rev-parse': (args: string[]) => ({ + stdout: args[2] === 'refs/heads/main^{commit}' ? 'old-main\n' : '\n' + }) + } + ] + ] satisfies [string, Record][])( + 'is not a fast-forward when %s', + async (_case, overrides) => { + const fake = createFakeGit(overrides) + + await expect(inspectLocalBaseBranch(fake.git, REFS)).resolves.toEqual({ + status: 'skipped_not_fast_forward' + }) + expect(fake.mutations()).toEqual([]) + } + ) + + it('compares the resolved oids, not the ref names', async () => { + const fake = createFakeGit() + + await expect(inspectLocalBaseBranch(fake.git, REFS)).resolves.toEqual({ + status: 'behind', + behind: 3, + localOid: 'old-main', + remoteOid: 'remote-main', + ownerWorktreePath: '/repo-main' + }) + expect(fake.calls.slice(0, 3)).toEqual([ + { args: ['rev-parse', '--verify', 'refs/heads/main^{commit}'], cwd: '/repo' }, + { args: ['rev-parse', '--verify', 'refs/remotes/origin/main^{commit}'], cwd: '/repo' }, + { args: ['rev-list', '--left-right', '--count', 'old-main...remote-main'], cwd: '/repo' } + ]) + expect(fake.mutations()).toEqual([]) + }) + + it('reads the owner status without taking optional locks and ignores untracked files', async () => { + const fake = createFakeGit() + + await inspectLocalBaseBranch(fake.git, REFS) + + expect(fake.calls.find(({ args }) => commandOf(args) === 'status')).toEqual({ + args: ['--no-optional-locks', 'status', '--porcelain', '--untracked-files=no'], + cwd: '/repo-main' + }) + }) + + it('reports an error when worktree ownership cannot be listed', async () => { + const fake = createFakeGit({}, () => Promise.reject(new Error('worktree list failed'))) + + await expect(inspectLocalBaseBranch(fake.git, REFS)).resolves.toEqual({ + status: 'skipped_error' + }) + }) +}) + +describe('fastForwardLocalBaseBranch', () => { + it('leaves a dirty owner checkout alone', async () => { + const fake = createFakeGit({ status: () => ({ stdout: ' M package.json\n' }) }) + + await expect(fastForwardLocalBaseBranch(fake.git, REFS)).resolves.toEqual({ + status: 'skipped_dirty_worktree', + ownerWorktreePath: '/repo-main' + }) + expect(fake.mutations()).toEqual([]) + }) + + it('moves a branch no worktree has checked out with a compare-and-swap update-ref', async () => { + const fake = createFakeGit({}, () => [{ path: '/repo', branch: 'refs/heads/develop' }]) + + await expect(fastForwardLocalBaseBranch(fake.git, REFS)).resolves.toEqual({ + status: 'updated' + }) + expect(fake.mutations()).toEqual([ + { + args: [ + 'update-ref', + '-m', + 'orca: fast-forward to refs/remotes/origin/main', + 'refs/heads/main', + 'remote-main', + 'old-main' + ], + cwd: '/repo' + } + ]) + expect(fake.commands()).not.toContain('status') + }) + + it('confirms the owner still has the branch out, then fast-forwards it without hooks', async () => { + const fake = createFakeGit() + + await expect(fastForwardLocalBaseBranch(fake.git, REFS)).resolves.toEqual({ + status: 'updated', + ownerWorktreePath: '/repo-main' + }) + expect(fake.calls.slice(-3)).toEqual([ + { args: ['symbolic-ref', '-q', 'HEAD'], cwd: '/repo-main' }, + { args: OWNER_MERGE_ARGS, cwd: '/repo-main' }, + { args: ['rev-parse', '--verify', 'refs/heads/main^{commit}'], cwd: '/repo' } + ]) + }) + + it('pins the merge options of the branch being moved', async () => { + const refs = { + repoPath: '/repo', + fullRef: 'refs/heads/release/2.0', + remoteTrackingRef: 'refs/remotes/origin/release/2.0' + } + const fake = createFakeGit( + { 'symbolic-ref': () => ({ stdout: 'refs/heads/release/2.0\n' }) }, + () => [{ path: '/repo-rel', branch: 'refs/heads/release/2.0' }] + ) + + await expect(fastForwardLocalBaseBranch(fake.git, refs)).resolves.toEqual({ + status: 'updated', + ownerWorktreePath: '/repo-rel' + }) + expect(fake.mutations()[0]?.args).toContain('branch.release/2.0.mergeOptions=') + }) + + // A merge setting (`-s ours`, `--squash`) the flags failed to override must not read as updated. + it.each([ + ['created a merge commit instead', 'merge-commit'], + ['left local where it was', 'old-main'] + ])('reports an error when the merge exits cleanly but %s', async (_case, afterMerge) => { + const fake = createFakeGit() + fake.local.afterMerge = afterMerge + + await expect(fastForwardLocalBaseBranch(fake.git, REFS)).resolves.toEqual({ + status: 'skipped_error', + ownerWorktreePath: '/repo-main' + }) + expect(fake.calls.at(-1)).toEqual({ + args: ['rev-parse', '--verify', 'refs/heads/main^{commit}'], + cwd: '/repo' + }) + }) + + // `-c branch..mergeOptions=` splits at the first `=`, so the pin cannot name this branch. + it('does not merge into an owner checkout of a branch whose name contains =', async () => { + const refs = { + repoPath: '/repo', + fullRef: 'refs/heads/release=1', + remoteTrackingRef: 'refs/remotes/origin/release=1' + } + const fake = createFakeGit({}, () => [{ path: '/repo-rel', branch: 'refs/heads/release=1' }]) + + await expect(fastForwardLocalBaseBranch(fake.git, refs)).resolves.toEqual({ + status: 'skipped_error', + ownerWorktreePath: '/repo-rel' + }) + expect(fake.mutations()).toEqual([]) + expect(fake.commands()).not.toContain('symbolic-ref') + }) + + it('still moves a branch whose name contains = when no worktree has it checked out', async () => { + const refs = { + repoPath: '/repo', + fullRef: 'refs/heads/release=1', + remoteTrackingRef: 'refs/remotes/origin/release=1' + } + const fake = createFakeGit({}, () => []) + + await expect(fastForwardLocalBaseBranch(fake.git, refs)).resolves.toEqual({ + status: 'updated' + }) + expect(fake.mutations().map(({ args }) => args[0])).toEqual(['update-ref']) + }) + + it('does not merge into a checkout that switched branches after the inspection', async () => { + const fake = createFakeGit({ 'symbolic-ref': () => ({ stdout: 'refs/heads/develop\n' }) }) + + await expect(fastForwardLocalBaseBranch(fake.git, REFS)).resolves.toEqual({ + status: 'skipped_error', + ownerWorktreePath: '/repo-main' + }) + expect(fake.mutations()).toEqual([]) + }) + + it('retries a move that lost an index.lock race', async () => { + vi.useFakeTimers() + const merge = vi + .fn() + .mockReturnValueOnce(INDEX_LOCK_ERROR) + .mockReturnValueOnce({ stdout: '' }) + const fake = createFakeGit({ merge }) + + const pending = fastForwardLocalBaseBranch(fake.git, REFS) + await vi.runAllTimersAsync() + + await expect(pending).resolves.toEqual({ status: 'updated', ownerWorktreePath: '/repo-main' }) + // Each attempt re-confirms the owner's branch before merging. + expect(fake.commands().slice(-5)).toEqual([ + 'symbolic-ref', + 'merge', + 'symbolic-ref', + 'merge', + 'rev-parse' + ]) + }) + + it('gives up after the retries when the lock never clears and local is still behind', async () => { + vi.useFakeTimers() + const fake = createFakeGit({ merge: () => INDEX_LOCK_ERROR }) + + const pending = fastForwardLocalBaseBranch(fake.git, REFS) + await vi.runAllTimersAsync() + + await expect(pending).resolves.toEqual({ + status: 'skipped_error', + ownerWorktreePath: '/repo-main' + }) + expect(fake.mutations()).toHaveLength(4) + }) + + it('does not retry a failure that is not lock contention', async () => { + const casMismatch = Object.assign(new Error('Command failed: git update-ref'), { + stderr: "fatal: cannot lock ref 'refs/heads/main': is at other but expected old-main" + }) + const fake = createFakeGit({ 'update-ref': () => casMismatch }, () => []) + + await expect(fastForwardLocalBaseBranch(fake.git, REFS)).resolves.toEqual({ + status: 'skipped_error' + }) + expect(fake.mutations()).toHaveLength(1) + }) + + it('reports updated when the move failed but local already contains the target', async () => { + const fake = createFakeGit({ + merge: () => new Error('fatal: Not possible to fast-forward, aborting.'), + 'merge-base': () => ({ stdout: '' }) + }) + + await expect(fastForwardLocalBaseBranch(fake.git, REFS)).resolves.toEqual({ + status: 'updated', + ownerWorktreePath: '/repo-main' + }) + expect(fake.calls.at(-1)).toEqual({ + args: ['merge-base', '--is-ancestor', 'remote-main', 'refs/heads/main'], + cwd: '/repo' + }) + }) + + it.each([ + [ + 'error: Your local changes to the following files would be overwritten by merge:\n\tREADME.md', + 'skipped_dirty_worktree' + ], + [ + 'error: The following untracked working tree files would be overwritten by merge:\n\tnew.txt', + 'skipped_dirty_worktree' + ], + [ + 'error: Updating the following directories would lose untracked files in them:\n\tvendor', + 'skipped_dirty_worktree' + ], + ['fatal: Not possible to fast-forward, aborting.', 'skipped_not_fast_forward'], + ['fatal: unable to write new index file', 'skipped_error'] + ])('classifies a failed merge saying %j as %s', async (stderr, status) => { + const fake = createFakeGit({ + merge: () => Object.assign(new Error('Command failed: git merge'), { stderr }) + }) + + await expect(fastForwardLocalBaseBranch(fake.git, REFS)).resolves.toEqual({ + status, + ownerWorktreePath: '/repo-main' + }) + }) + + it('passes a non-behind inspection straight through without mutating', async () => { + const fake = createFakeGit({ 'rev-list': () => ({ stdout: '1\t3\n' }) }) + + await expect(fastForwardLocalBaseBranch(fake.git, REFS)).resolves.toEqual({ + status: 'skipped_not_fast_forward' + }) + expect(fake.mutations()).toEqual([]) + }) +}) + +describe('relay reply parsing', () => { + it.each([undefined, null, 'updated', {}, { status: 'bogus' }, { status: 'behind', behind: 2 }])( + 'reads the malformed outcome %j as an error', + (value) => { + expect(parseLocalBaseBranchFastForwardOutcome(value)).toEqual({ status: 'skipped_error' }) + } + ) + + it.each([ + [{ status: 'nothing_to_do', ownerWorktreePath: '/x' }, { status: 'nothing_to_do' }], + [{ status: 'updated' }, { status: 'updated' }], + [ + { status: 'skipped_dirty_worktree', ownerWorktreePath: '/repo-main' }, + { status: 'skipped_dirty_worktree', ownerWorktreePath: '/repo-main' } + ], + [{ status: 'updated', ownerWorktreePath: 42 }, { status: 'updated' }], + [{ status: 'skipped_error', ownerWorktreePath: '' }, { status: 'skipped_error' }] + ])('keeps only the recognized fields of %j', (value, expected) => { + expect(parseLocalBaseBranchFastForwardOutcome(value)).toEqual(expected) + }) + + it.each([ + [{ status: 'behind', behind: 3 }, 3], + [{ status: 'behind', behind: 0 }, undefined], + [{ status: 'behind', behind: '3' }, undefined], + [{ status: 'behind' }, undefined], + [{ status: 'skipped_dirty_worktree', behind: 3 }, undefined], + [null, undefined], + [4, undefined] + ])('reads the behind count of %j as %s', (value, expected) => { + expect(readFastForwardableBehindCount(value)).toBe(expected) + }) + + it('reports no status when there was nothing to refresh', () => { + const names = { baseRef: 'origin/main', localBranch: 'main' } + + expect(toLocalBaseRefRefreshResult(names, undefined)).toBeUndefined() + expect(toLocalBaseRefRefreshResult(names, { status: 'nothing_to_do' })).toBeUndefined() + expect( + toLocalBaseRefRefreshResult(names, { status: 'updated', ownerWorktreePath: '/repo-main' }) + ).toEqual({ ...names, status: 'updated', ownerWorktreePath: '/repo-main' }) + }) +}) diff --git a/src/shared/worktree/local-base-branch-fast-forward.ts b/src/shared/worktree/local-base-branch-fast-forward.ts new file mode 100644 index 00000000000..444d5ee5035 --- /dev/null +++ b/src/shared/worktree/local-base-branch-fast-forward.ts @@ -0,0 +1,287 @@ +import { readGitCommandFailureText } from '../git-command-failure-text' +import { retryOnGitLockContention } from '../git-lock-contention' +import { parseGitRevListAheadBehindCounts } from '../git-rev-list-output' +import { isShowRefNoMatchError } from '../git-show-ref-no-match' +import type { LocalBaseRefRefreshResult } from './base-ref-drift-types' + +/** + * The execution host's git, injected so the main process (local and WSL repos) and the SSH relay + * run this one policy. `exec` rejects on a non-zero exit with git's output on the error. + */ +export type LocalBaseBranchGit = { + exec: (args: string[], cwd: string) => Promise<{ stdout: string }> + /** Each worktree's path and checked-out branch as a full ref, as this host reports them. */ + listWorktrees: (repoPath: string) => Promise +} + +export type LocalBaseBranchRefs = { + repoPath: string + /** `refs/heads/` */ + fullRef: string + /** `refs/remotes//` */ + remoteTrackingRef: string +} + +type LocalBaseRefRefreshStatus = LocalBaseRefRefreshResult['status'] + +export type LocalBaseBranchInspection = + /** Nothing stale: local already matches, or does not exist yet (#15331). */ + | { status: 'nothing_to_do' } + | { + status: 'behind' + behind: number + localOid: string + remoteOid: string + ownerWorktreePath?: string + } + | { status: Exclude; ownerWorktreePath?: string } + +export type LocalBaseBranchFastForwardOutcome = + | { status: 'nothing_to_do' } + | { status: LocalBaseRefRefreshStatus; ownerWorktreePath?: string } + +// Why: an update Orca makes on the user's behalf must be a plain fast-forward whatever the user's +// merge settings say: no post-merge hooks (the create waits on it), auto-gc or autostash, no +// branch-level mergeOptions (`-s ours` or `--squash` there would drop or stage upstream), no +// signature refusal of the tip the workspace was created from, and no overwrite of an ignored file +// (a `.env`) at a path the new commit adds. Command-line flags beat config; keys older Git does not +// know are ignored; `/dev/null/` never exists. Git 2.25 has no `ort` or `--no-autostash`. +function ownerFastForwardArgs(branch: string, targetOid: string): string[] { + return [ + '-c', + 'core.hooksPath=/dev/null', + '-c', + 'gc.auto=0', + '-c', + 'maintenance.auto=false', + '-c', + 'merge.autoStash=false', + '-c', + `branch.${branch}.mergeOptions=`, + 'merge', + '--ff-only', + '-s', + 'recursive', + '--no-verify-signatures', + '--no-overwrite-ignore', + '--no-stat', + '-q', + targetOid + ] +} + +/** Read-only: how far local is behind, and whether its checkout would let it move. */ +export async function inspectLocalBaseBranch( + git: LocalBaseBranchGit, + refs: LocalBaseBranchRefs +): Promise { + const { repoPath, fullRef, remoteTrackingRef } = refs + let localOid: string + let remoteOid: string + let behind: number + try { + localOid = await revParseCommit(git, repoPath, fullRef) + remoteOid = await revParseCommit(git, repoPath, remoteTrackingRef) + if (localOid === remoteOid) { + return { status: 'nothing_to_do' } + } + const { stdout } = await git.exec( + ['rev-list', '--left-right', '--count', `${localOid}...${remoteOid}`], + repoPath + ) + const counts = parseGitRevListAheadBehindCounts(stdout) + if (counts.status !== 'ok' || counts.ahead !== 0 || counts.behind === 0) { + return { status: 'skipped_not_fast_forward' } + } + behind = counts.behind + } catch { + // Why (#15331): a branch that does not exist yet cannot be stale. Only a proven absence + // suppresses the warning; an unusable repo or a lost transport keeps it. + return (await isRefProvenAbsent(git, repoPath, fullRef)) + ? { status: 'nothing_to_do' } + : { status: 'skipped_not_fast_forward' } + } + + try { + const owner = (await git.listWorktrees(repoPath)).find((wt) => wt.branch === fullRef) + if (!owner) { + return { status: 'behind', behind, localOid, remoteOid } + } + if (await hasTrackedChanges(git, owner.path)) { + return { status: 'skipped_dirty_worktree', ownerWorktreePath: owner.path } + } + return { status: 'behind', behind, localOid, remoteOid, ownerWorktreePath: owner.path } + } catch { + return { status: 'skipped_error' } + } +} + +/** + * Fast-forwards local to its remote-tracking ref. A checked-out branch moves with + * `merge --ff-only`, which refuses under git's own index lock to overwrite an edit or untracked + * file, or to drop a commit made since the inspection; a free branch moves with a compare-and-swap + * `update-ref`. Never rejects. + */ +export async function fastForwardLocalBaseBranch( + git: LocalBaseBranchGit, + refs: LocalBaseBranchRefs +): Promise { + const inspection = await inspectLocalBaseBranch(git, refs) + if (inspection.status !== 'behind') { + return inspection + } + const { repoPath, fullRef, remoteTrackingRef } = refs + const { localOid, remoteOid, ownerWorktreePath } = inspection + const owner = ownerWorktreePath ? { ownerWorktreePath } : {} + const branch = fullRef.slice('refs/heads/'.length) + if (ownerWorktreePath && branch.includes('=')) { + // Why: `-c branch..mergeOptions=` splits at the first `=`, so such a name cannot be pinned. + return { status: 'skipped_error', ...owner } + } + try { + await retryOnGitLockContention(async () => { + if (!ownerWorktreePath) { + await git.exec( + [ + 'update-ref', + '-m', + `orca: fast-forward to ${remoteTrackingRef}`, + fullRef, + remoteOid, + localOid + ], + repoPath + ) + return + } + // Why: merge moves whatever HEAD is; this narrows, but cannot close, a branch-switch window, which the post-move check reports as a warning. + const { stdout: head } = await git.exec(['symbolic-ref', '-q', 'HEAD'], ownerWorktreePath) + if (head.trim() !== fullRef) { + throw new Error(`${fullRef} is no longer checked out at ${ownerWorktreePath}`) + } + await git.exec(ownerFastForwardArgs(branch, remoteOid), ownerWorktreePath) + }) + // Why: "updated" must mean local is exactly the target, whatever a merge setting did instead. + if (ownerWorktreePath && (await revParseCommit(git, repoPath, fullRef)) !== remoteOid) { + return { status: 'skipped_error', ...owner } + } + return { status: 'updated', ...owner } + } catch (error) { + // Why: a concurrent update (another Orca, the user's own pull) may already have moved local. + if (await localContains(git, repoPath, fullRef, remoteOid)) { + return { status: 'updated', ...owner } + } + return { status: classifyFastForwardFailure(error), ...owner } + } +} + +export function toLocalBaseRefRefreshResult( + names: { baseRef: string; localBranch: string }, + outcome: LocalBaseBranchFastForwardOutcome | undefined +): LocalBaseRefRefreshResult | undefined { + if (!outcome || outcome.status === 'nothing_to_do') { + return undefined + } + return { + baseRef: names.baseRef, + localBranch: names.localBranch, + status: outcome.status, + ...(outcome.ownerWorktreePath ? { ownerWorktreePath: outcome.ownerWorktreePath } : {}) + } +} + +function classifyFastForwardFailure(error: unknown): Exclude { + const text = readGitCommandFailureText(error) + if (/would be overwritten by merge|would lose untracked files/i.test(text)) { + return 'skipped_dirty_worktree' + } + if (/Not possible to fast-forward/i.test(text)) { + return 'skipped_not_fast_forward' + } + return 'skipped_error' +} + +async function revParseCommit( + git: LocalBaseBranchGit, + repoPath: string, + ref: string +): Promise { + const { stdout } = await git.exec(['rev-parse', '--verify', `${ref}^{commit}`], repoPath) + const oid = stdout.trim() + if (!oid) { + throw new Error(`${ref} did not resolve to a commit`) + } + return oid +} + +async function hasTrackedChanges(git: LocalBaseBranchGit, worktreePath: string): Promise { + // Why: a read must not take index.lock in the user's checkout, where it would fail their own git. + const { stdout } = await git.exec( + ['--no-optional-locks', 'status', '--porcelain', '--untracked-files=no'], + worktreePath + ) + return stdout.trim().length > 0 +} + +async function isRefProvenAbsent( + git: LocalBaseBranchGit, + repoPath: string, + fullRef: string +): Promise { + try { + await git.exec(['show-ref', '--verify', '--quiet', '--', fullRef], repoPath) + return false + } catch (error) { + return isShowRefNoMatchError(error) + } +} + +async function localContains( + git: LocalBaseBranchGit, + repoPath: string, + fullRef: string, + oid: string +): Promise { + try { + await git.exec(['merge-base', '--is-ancestor', oid, fullRef], repoPath) + return true + } catch { + return false + } +} + +const REFRESH_STATUSES: readonly LocalBaseRefRefreshStatus[] = [ + 'updated', + 'skipped_dirty_worktree', + 'skipped_not_fast_forward', + 'skipped_error' +] + +/** Validates a relay reply; anything unrecognized reads as an error rather than a silent success. */ +export function parseLocalBaseBranchFastForwardOutcome( + value: unknown +): LocalBaseBranchFastForwardOutcome { + if (typeof value !== 'object' || value === null || !('status' in value)) { + return { status: 'skipped_error' } + } + if (value.status === 'nothing_to_do') { + return { status: 'nothing_to_do' } + } + const status = REFRESH_STATUSES.find((candidate) => candidate === value.status) + if (!status) { + return { status: 'skipped_error' } + } + const ownerWorktreePath = 'ownerWorktreePath' in value ? value.ownerWorktreePath : undefined + return typeof ownerWorktreePath === 'string' && ownerWorktreePath + ? { status, ownerWorktreePath } + : { status } +} + +/** How many commits a relay's inspection says local is behind, when it may be fast-forwarded. */ +export function readFastForwardableBehindCount(value: unknown): number | undefined { + if (typeof value !== 'object' || value === null || !('status' in value) || !('behind' in value)) { + return undefined + } + const { behind } = value + return value.status === 'behind' && typeof behind === 'number' && behind > 0 ? behind : undefined +}