diff --git a/src/main/runtime/runtime-local-git-worktree-create.ts b/src/main/runtime/runtime-local-git-worktree-create.ts index 9e5d4cdfac3..8977efb0dda 100644 --- a/src/main/runtime/runtime-local-git-worktree-create.ts +++ b/src/main/runtime/runtime-local-git-worktree-create.ts @@ -26,6 +26,54 @@ import { } from '../worktree-name-retirement' import type { WorktreeCreateTimingRecorder } from '../worktree-create-timing' +export type RuntimeLocalBaseRefRefresh = { + /** Awaits the deferred refresh started before naming, if any. */ + finish: () => Promise +} + +// Why: the deferred refresh can change neither the base nor the name, so start it before naming +// and await it before the add, as the desktop create does; naming's PR lookup overlaps it. +export function startRuntimeLocalBaseRefRefresh(args: { + repoPath: string + base: RuntimeLocalWorktreeCreateBase + localWorktreeGitOptions: LocalGitExecOptions + refreshRemoteTrackingBase: ( + repoPath: string, + base: RemoteTrackingBase, + options?: LocalGitExecOptions + ) => Promise + fetchRemote: (repoPath: string, remote: string, options?: LocalGitExecOptions) => Promise + timing: WorktreeCreateTimingRecorder +}): RuntimeLocalBaseRefRefresh { + const { remoteTrackingBase, deferredRefresh } = args.base + if (remoteTrackingBase && deferredRefresh === 'tracking_ref') { + const refresh = args.refreshRemoteTrackingBase( + args.repoPath, + remoteTrackingBase, + args.localWorktreeGitOptions + ) + // Why: naming can throw before finish() awaits the refresh. + void refresh.catch(() => undefined) + return { + finish: async () => { + await args.timing.time('refresh_base_ref', () => refresh) + } + } + } + if (deferredRefresh === 'origin') { + const originFetch = args.fetchRemote(args.repoPath, 'origin', args.localWorktreeGitOptions) + void originFetch.catch(() => undefined) + return { + finish: async () => { + try { + await args.timing.time('refresh_base_ref', () => originFetch) + } catch {} + } + } + } + return { finish: async () => {} } +} + export async function createRuntimeLocalGitWorktree(args: { request: RuntimeManagedWorktreeCreateArgs repo: Repo @@ -43,12 +91,7 @@ export async function createRuntimeLocalGitWorktree(args: { effectiveSanitizedName?: string checkoutExistingBranch: boolean localWorktreeGitOptions: LocalGitExecOptions - refreshRemoteTrackingBase: ( - repoPath: string, - base: RemoteTrackingBase, - options?: LocalGitExecOptions - ) => Promise - fetchRemote: (repoPath: string, remote: string, options?: LocalGitExecOptions) => Promise + baseRefRefresh: RuntimeLocalBaseRefRefresh rearm: PreparationRearmHolder timing: WorktreeCreateTimingRecorder }): Promise<{ @@ -57,22 +100,8 @@ export async function createRuntimeLocalGitWorktree(args: { created: GitWorktreeInfo addResult: AddWorktreeResult }> { - const { baseBranch, remoteTrackingBase, deferredRefresh } = args.base - if (remoteTrackingBase && deferredRefresh === 'tracking_ref') { - await args.timing.time('refresh_base_ref', () => - args.refreshRemoteTrackingBase( - args.repo.path, - remoteTrackingBase, - args.localWorktreeGitOptions - ) - ) - } else if (deferredRefresh === 'origin') { - try { - await args.timing.time('refresh_base_ref', () => - args.fetchRemote(args.repo.path, 'origin', args.localWorktreeGitOptions) - ) - } catch {} - } + const { baseBranch, remoteTrackingBase } = args.base + await args.baseRefRefresh.finish() const sparseDirectories = args.request.sparseCheckout ? normalizeSparseDirectories(args.request.sparseCheckout.directories) : [] diff --git a/src/main/runtime/runtime-local-worktree-create.test.ts b/src/main/runtime/runtime-local-worktree-create.test.ts index b6de7eb4f76..0bef4694b4d 100644 --- a/src/main/runtime/runtime-local-worktree-create.test.ts +++ b/src/main/runtime/runtime-local-worktree-create.test.ts @@ -140,8 +140,10 @@ function createWorktree( createdWithAgent: undefined, resolveRemoteTrackingBase: mocks.remoteBase, hasRemoteTrackingRef: mocks.hasRemoteRef, - refreshRemoteTrackingBase: mocks.refresh, - fetchRemote: mocks.fetch, + // Why: a vi.fn marks the promise it returns as handled; a fresh promise keeps + // an unawaited fetch rejection visible to the unhandled-rejection checks. + refreshRemoteTrackingBase: async (...refreshArgs) => await mocks.refresh(...refreshArgs), + fetchRemote: async (...fetchArgs) => await mocks.fetch(...fetchArgs), onWorktreeMetadataPersisted: () => undefined, rearm, timing @@ -403,6 +405,135 @@ describe('runtime create Git priority', () => { }) }) +describe('runtime create PR conflict probe', () => { + const remoteTrackingBase = { + remote: 'origin', + branch: 'main', + base: 'origin/main', + ref: 'refs/remotes/origin/main' + } + + beforeEach(() => { + mocks.branchName.mockImplementation(async (_path, _override, name: string) => name) + }) + + it('runs the first name PR lookup while the base fetch is still in flight', async () => { + mocks.remoteBase.mockResolvedValue(remoteTrackingBase) + let finishFetch!: () => void + mocks.refresh.mockReturnValue( + new Promise((resolve) => { + finishFetch = () => resolve({ ok: true }) + }) + ) + let fetchPendingAtLookup = false + mocks.githubPr.mockImplementation(async () => { + fetchPendingAtLookup = mocks.refresh.mock.calls.length === 1 + finishFetch?.() + return null + }) + + await createWorktree() + + expect(fetchPendingAtLookup).toBe(true) + expect(mocks.githubPr).toHaveBeenCalledWith('/repo', 'app', {}) + expect(mocks.consume).toHaveBeenCalledWith( + expect.objectContaining({ + branch: 'app', + options: expect.objectContaining({ remoteTrackingBase }) + }) + ) + }) + + it('suffixes past a PR on the first name', async () => { + mocks.githubPr.mockResolvedValueOnce({ number: 7, state: 'closed' }).mockResolvedValue(null) + + await createWorktree() + + expect(mocks.githubPr.mock.calls.map(([, branch]) => branch)).toEqual(['app', 'app-2']) + expect(mocks.consume).toHaveBeenCalledWith(expect.objectContaining({ branch: 'app-2' })) + }) + + it.each([undefined, 'Ubuntu'])( + 'probes the next name for a PR once the first name collides, routed for %s', + async (wslDistro) => { + const routing = wslDistro ? { wslDistro } : {} + mocks.routing.mockReturnValue(routing) + mocks.branchConflict.mockResolvedValueOnce('local').mockResolvedValue(null) + + await createWorktree() + + expect(mocks.githubPr).toHaveBeenCalledOnce() + expect(mocks.githubPr).toHaveBeenCalledWith('/repo', 'app-2', routing) + } + ) + + it('skips a later name that already has a PR', async () => { + mocks.branchConflict.mockResolvedValueOnce('local').mockResolvedValue(null) + mocks.githubPr.mockResolvedValueOnce({ number: 7, state: 'closed' }).mockResolvedValue(null) + + await createWorktree() + + expect(mocks.githubPr.mock.calls.map(([, branch]) => branch)).toEqual(['app-2', 'app-3']) + expect(mocks.consume).toHaveBeenCalledWith(expect.objectContaining({ branch: 'app-3' })) + }) + + it('reports the naming failure when the started base fetch also fails', async () => { + mocks.remoteBase.mockResolvedValue(remoteTrackingBase) + mocks.refresh.mockRejectedValue(new Error('offline')) + mocks.branchConflict.mockResolvedValue('local') + const unhandled = vi.fn() + process.on('unhandledRejection', unhandled) + try { + await expect(createWorktree()).rejects.toThrow('Branch "app-') + await new Promise((resolve) => setImmediate(resolve)) + expect(mocks.refresh).toHaveBeenCalledOnce() + expect(unhandled).not.toHaveBeenCalled() + expect(mocks.consume).not.toHaveBeenCalled() + } finally { + process.off('unhandledRejection', unhandled) + } + }) + + it('runs the first name PR lookup while the origin fetch is still in flight', async () => { + mocks.hasBase.mockResolvedValue(false) + let finishFetch!: () => void + mocks.fetch.mockReturnValue( + new Promise((resolve) => { + finishFetch = resolve + }) + ) + let fetchPendingAtLookup = false + mocks.githubPr.mockImplementation(async () => { + fetchPendingAtLookup = mocks.fetch.mock.calls.length === 1 + finishFetch?.() + return null + }) + + await createWorktree() + + expect(fetchPendingAtLookup).toBe(true) + expect(mocks.fetch).toHaveBeenCalledWith('/repo', 'origin', {}) + expect(mocks.consume).toHaveBeenCalledWith(expect.objectContaining({ branch: 'app' })) + }) + + it('reports the naming failure when the started origin fetch also fails', async () => { + mocks.hasBase.mockResolvedValue(false) + mocks.fetch.mockRejectedValue(new Error('offline')) + mocks.branchConflict.mockResolvedValue('local') + const unhandled = vi.fn() + process.on('unhandledRejection', unhandled) + try { + await expect(createWorktree()).rejects.toThrow('Branch "app-') + await new Promise((resolve) => setImmediate(resolve)) + expect(mocks.fetch).toHaveBeenCalledOnce() + expect(unhandled).not.toHaveBeenCalled() + expect(mocks.consume).not.toHaveBeenCalled() + } finally { + process.off('unhandledRejection', unhandled) + } + }) +}) + describe('runtime create base without a tracking ref', () => { const originMain = { remote: 'origin', diff --git a/src/main/runtime/runtime-local-worktree-create.ts b/src/main/runtime/runtime-local-worktree-create.ts index 6e50b3ce309..f16f7caf669 100644 --- a/src/main/runtime/runtime-local-worktree-create.ts +++ b/src/main/runtime/runtime-local-worktree-create.ts @@ -19,7 +19,10 @@ import type { RemoteFetchResult, RemoteTrackingBase } from './runtime-remote-fet import type { HostedReviewExecutionOptions } from '../source-control/hosted-review-git-options' import { hasLocalWorktreeBaseRef } from '../git/worktree-base-ref-probe' import { resolveRuntimeLocalWorktreeCreateCandidate } from './runtime-local-worktree-create-candidate' -import { createRuntimeLocalGitWorktree } from './runtime-local-git-worktree-create' +import { + createRuntimeLocalGitWorktree, + startRuntimeLocalBaseRefRefresh +} from './runtime-local-git-worktree-create' import { resolveRuntimeLocalWorktreeCreateBase } from './runtime-local-worktree-create-base' import { materializeRuntimeLocalWorktree } from './runtime-local-worktree-materialization' import { resolveRuntimeSetupDecision } from './runtime-local-worktree-setup' @@ -124,6 +127,14 @@ async function performRuntimeLocalWorktreeCreate(args: RuntimeLocalWorktreeCr refreshRemoteTrackingBase: args.refreshRemoteTrackingBase, timing: args.timing }) + const baseRefRefresh = startRuntimeLocalBaseRefRefresh({ + repoPath: repo.path, + base, + localWorktreeGitOptions: worktreeGitOptions, + refreshRemoteTrackingBase: args.refreshRemoteTrackingBase, + fetchRemote: args.fetchRemote, + timing: args.timing + }) const candidate = await args.timing.time('resolve_name', () => resolveRuntimeLocalWorktreeCreateCandidate({ request, @@ -150,8 +161,7 @@ async function performRuntimeLocalWorktreeCreate(args: RuntimeLocalWorktreeCr effectiveSanitizedName: candidate.effectiveSanitizedName, checkoutExistingBranch: candidate.checkoutExistingBranch, localWorktreeGitOptions: worktreeGitOptions, - refreshRemoteTrackingBase: args.refreshRemoteTrackingBase, - fetchRemote: args.fetchRemote, + baseRefRefresh, rearm: args.rearm, timing: args.timing }) diff --git a/src/main/runtime/runtime-worktree-create-git.test.ts b/src/main/runtime/runtime-worktree-create-git.test.ts new file mode 100644 index 00000000000..8fe1027b2bd --- /dev/null +++ b/src/main/runtime/runtime-worktree-create-git.test.ts @@ -0,0 +1,32 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as GitHubClient from '../github/client' + +const getPRForBranchMock = vi.hoisted(() => vi.fn()) + +vi.mock('../github/client', async (importOriginal) => ({ + ...(await importOriginal()), + getPRForBranch: getPRForBranchMock +})) + +import { getLocalGitHubPrForBranch } from './runtime-worktree-create-git' + +beforeEach(() => { + getPRForBranchMock.mockReset() + getPRForBranchMock.mockResolvedValue(null) +}) + +describe('getLocalGitHubPrForBranch', () => { + it('routes the PR lookup through the WSL distro', async () => { + await getLocalGitHubPrForBranch('/repo', 'app', { wslDistro: 'Ubuntu' }) + + expect(getPRForBranchMock).toHaveBeenCalledWith('/repo', 'app', null, null, null, { + localGitExecOptions: { wslDistro: 'Ubuntu' } + }) + }) + + it('uses the plain lookup without project git options', async () => { + await getLocalGitHubPrForBranch('/repo', 'app', {}) + + expect(getPRForBranchMock).toHaveBeenCalledWith('/repo', 'app') + }) +})