diff --git a/src/main/git/fork-remote-refspec.test.ts b/src/main/git/fork-remote-refspec.test.ts index 3ac0f2fc3ef2..09c9e5a9717c 100644 --- a/src/main/git/fork-remote-refspec.test.ts +++ b/src/main/git/fork-remote-refspec.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi, type Mock } from 'vitest' import { buildNarrowForkFetchRefspec, + ensureRemoteAlsoTracksBranch, ensureRemoteTracksBranchNarrowly, getRemoteFetchRefspecs, pruneUntrackedForkRemoteRefs, @@ -134,6 +135,42 @@ describe('ensureRemoteTracksBranchNarrowly', () => { }) }) +describe('ensureRemoteAlsoTracksBranch', () => { + it('appends the narrow refspec without dropping the user-configured wide default (#25703)', async () => { + const { exec } = makeConfigExec({ fork: [wildcardForkFetchRefspec('fork')] }) + + await ensureRemoteAlsoTracksBranch(exec, REPO, 'fork', 'main') + + await expect(getRemoteFetchRefspecs(exec, REPO, 'fork')).resolves.toEqual([ + wildcardForkFetchRefspec('fork'), + '+refs/heads/main*:refs/remotes/fork/main*' + ]) + }) + + it('is a no-op when the branch is already tracked', async () => { + const { exec } = makeConfigExec({ fork: ['+refs/heads/main*:refs/remotes/fork/main*'] }) + + await ensureRemoteAlsoTracksBranch(exec, REPO, 'fork', 'main') + + const addCalls = exec.mock.calls.filter(([args]) => args[1] === '--add') + expect(addCalls).toEqual([]) + }) + + it("keeps other branches' entries and never writes tagOpt", async () => { + const { exec, tagOptByRemote } = makeConfigExec({ + fork: ['+refs/heads/other*:refs/remotes/fork/other*'] + }) + + await ensureRemoteAlsoTracksBranch(exec, REPO, 'fork', 'main') + + await expect(getRemoteFetchRefspecs(exec, REPO, 'fork')).resolves.toEqual([ + '+refs/heads/other*:refs/remotes/fork/other*', + '+refs/heads/main*:refs/remotes/fork/main*' + ]) + expect(tagOptByRemote.fork).toBeUndefined() + }) +}) + describe('removeStaleForkFetchRefspec', () => { it('drops only the refspec whose source matches the stale branch', async () => { const { exec } = makeConfigExec({ diff --git a/src/main/git/fork-remote-refspec.ts b/src/main/git/fork-remote-refspec.ts index c924fdb2cb26..a98b3c22b50f 100644 --- a/src/main/git/fork-remote-refspec.ts +++ b/src/main/git/fork-remote-refspec.ts @@ -135,6 +135,25 @@ export async function ensureRemoteTracksBranchNarrowly( await execGit(['config', `remote.${remoteName}.tagOpt`, '--no-tags'], repoPath) } +/** + * Reuse-path twin of `ensureRemoteTracksBranchNarrowly` for remotes Orca did not create + * (#25703): appends the branch's narrow refspec when absent and never drops or rewrites + * anything already configured -- the user's wide default and other branches' entries stay + * intact, and `tagOpt` is left alone. + */ +export async function ensureRemoteAlsoTracksBranch( + execGit: GitExecFn, + repoPath: string, + remoteName: string, + branchName: string +): Promise { + const desired = buildNarrowForkFetchRefspec(remoteName, branchName) + const existing = await getRemoteFetchRefspecs(execGit, repoPath, remoteName) + if (!existing.includes(desired)) { + await execGit(['config', '--add', `remote.${remoteName}.fetch`, desired], repoPath) + } +} + /** * Removes every `remote..fetch` entry, leaving the remote pushable but importing * nothing on a plain fetch. For a fork remote with no branch pinning it at all (no diff --git a/src/main/ipc/worktree-push-target-cleanup.ts b/src/main/ipc/worktree-push-target-cleanup.ts index 09918ffe8f76..2d4ddcdfe220 100644 --- a/src/main/ipc/worktree-push-target-cleanup.ts +++ b/src/main/ipc/worktree-push-target-cleanup.ts @@ -188,8 +188,9 @@ function isBranchConfigSeparator(code: number): boolean { // Why: on-demand materialization (push/pull/fetch/fast-forward, #17828) never // updates the store's `pushTarget.remoteCreated` flag, so ownership must also be // readable from the repo-local `remote..orca-created` config Orca writes -// when it creates the remote (see `worktree-push-target-setup.ts`). -async function remoteHasOrcaProvenance( +// when it creates the remote (see `worktree-push-target-setup.ts`). Also gates +// which reused remotes the refspec narrowing may rewrite (#25703). +export async function remoteHasOrcaProvenance( execGit: GitRemoteExec, repoPath: string, remoteName: string diff --git a/src/main/ipc/worktree-push-target-setup.test.ts b/src/main/ipc/worktree-push-target-setup.test.ts index ef1e44aa9237..44646a30a206 100644 --- a/src/main/ipc/worktree-push-target-setup.test.ts +++ b/src/main/ipc/worktree-push-target-setup.test.ts @@ -67,6 +67,63 @@ function callsMatching(exec: ExecMock, head: string[]): string[][] { .filter((args) => head.every((part, i) => args[i] === part)) } +// `makeRepoExec` plus in-memory `remote..fetch` / `.orca-created` / `.tagOpt` config, +// mutated the way real `git config` would be (#25703's reuse-with-refspec matrix). +function makeConfigAwareRepoExec( + remotes: Record, + fetchRefspecsByRemote: Record = {}, + orcaCreatedRemotes: string[] = [] +): { + exec: ExecMock + fetchRefspecs: Record + tagOptByRemote: Record +} { + const fetchRefspecs: Record = { ...fetchRefspecsByRemote } + const tagOptByRemote: Record = {} + const orcaCreated = new Set(orcaCreatedRemotes) + const exec = vi.fn(async (args: string[]) => { + if (args[0] === 'config') { + const configKey = + args[1] === '--get-all' || + args[1] === '--unset-all' || + args[1] === '--add' || + args[1] === '--get' + ? args[2] + : args[1] + const fetchMatch = /^remote\.(.+)\.fetch$/.exec(configKey ?? '') + const markerMatch = /^remote\.(.+)\.orca-created$/.exec(configKey ?? '') + if (args[1] === '--get-all' && fetchMatch) { + const values = fetchRefspecs[fetchMatch[1]!] ?? [] + if (values.length === 0) { + throw new Error('key not found') + } + return { stdout: `${values.join('\n')}\n`, stderr: '' } + } + if (args[1] === '--unset-all' && fetchMatch) { + fetchRefspecs[fetchMatch[1]!] = [] + return { stdout: '', stderr: '' } + } + if (args[1] === '--add' && fetchMatch) { + fetchRefspecs[fetchMatch[1]!] = [...(fetchRefspecs[fetchMatch[1]!] ?? []), args[3]!] + return { stdout: '', stderr: '' } + } + if (args[1] === '--get' && markerMatch) { + if (!orcaCreated.has(markerMatch[1]!)) { + throw new Error('key not found') + } + return { stdout: 'true\n', stderr: '' } + } + if (args[1]?.startsWith('remote.') && args[1]?.endsWith('.tagOpt')) { + tagOptByRemote[args[1]!.slice('remote.'.length, -'.tagOpt'.length)] = args[2]! + return { stdout: '', stderr: '' } + } + return { stdout: '', stderr: '' } + } + return makeRepoExec(remotes)(args, REPO) + }) + return { exec, fetchRefspecs, tagOptByRemote } +} + function forkTarget(overrides: Partial = {}): GitPushTarget { return { remoteName: 'pr-contributor-orca', @@ -156,6 +213,79 @@ describe('prepareWorktreePushTargetWithExec', () => { expect(result.remoteCreated).toBe(true) }) + // #25703: a remote the user added by hand (wide default refspec, no provenance marker) + // must keep its own config -- only Orca's per-branch line is appended. + it('preserves a reused user-created remote with no marker and only appends its branch (#25703)', async () => { + const WIDE = '+refs/heads/*:refs/remotes/fork/*' + const { exec, fetchRefspecs, tagOptByRemote } = makeConfigAwareRepoExec( + { + origin: 'git@github.com:stablyai/orca.git', + fork: FORK_HTTPS + }, + { fork: [WIDE] } + ) + + await prepareWorktreePushTargetWithExec( + exec, + REPO, + { remoteName: 'pr-contributor-orca', branchName: 'contributor/fix', remoteUrl: FORK_SSH }, + () => false + ) + + expect(fetchRefspecs.fork).toEqual([ + WIDE, + '+refs/heads/contributor/fix*:refs/remotes/fork/contributor/fix*' + ]) + // The remote's fetch behavior is the user's; no tagOpt rewrite either. + expect(tagOptByRemote.fork).toBeUndefined() + }) + + // The issue's workaround (re-add the wide refspec next to Orca's line) used to be + // undone by the next branch's reuse, because a present wide refspec dropped everything. + it('keeps the workaround wide refspec when a second branch reuses the same user remote (#25703)', async () => { + const WIDE = '+refs/heads/*:refs/remotes/fork/*' + const { exec, fetchRefspecs } = makeConfigAwareRepoExec( + { + origin: 'git@github.com:stablyai/orca.git', + fork: FORK_HTTPS + }, + { fork: [WIDE, '+refs/heads/a*:refs/remotes/fork/a*'] } + ) + + await prepareWorktreePushTargetWithExec( + exec, + REPO, + { remoteName: 'pr-contributor-orca', branchName: 'b', remoteUrl: FORK_SSH }, + () => false + ) + + expect(fetchRefspecs.fork).toEqual([ + WIDE, + '+refs/heads/a*:refs/remotes/fork/a*', + '+refs/heads/b*:refs/remotes/fork/b*' + ]) + }) + + // Green guard: a remote carrying the `orca-created` marker is Orca's own, so the + // narrow rewrite (wide dropped, tagOpt pinned) still applies on reuse. + it('still narrows a reused remote Orca created (orca-created marker set)', async () => { + const { exec, fetchRefspecs, tagOptByRemote } = makeConfigAwareRepoExec( + { + origin: 'git@github.com:stablyai/orca.git', + 'pr-contributor-orca': FORK_HTTPS + }, + { 'pr-contributor-orca': ['+refs/heads/*:refs/remotes/pr-contributor-orca/*'] }, + ['pr-contributor-orca'] + ) + + await prepareWorktreePushTargetWithExec(exec, REPO, forkTarget(), () => false) + + expect(fetchRefspecs['pr-contributor-orca']).toEqual([ + '+refs/heads/contributor/fix*:refs/remotes/pr-contributor-orca/contributor/fix*' + ]) + expect(tagOptByRemote['pr-contributor-orca']).toBe('--no-tags') + }) + it('disambiguates with a numeric suffix when the preferred remote name is taken by a different URL', async () => { const exec = makeRepoExec({ 'pr-contributor-orca': 'git@github.com:someone-else/orca.git' }) diff --git a/src/main/ipc/worktree-push-target-setup.ts b/src/main/ipc/worktree-push-target-setup.ts index c4a82d949333..248a3d41df86 100644 --- a/src/main/ipc/worktree-push-target-setup.ts +++ b/src/main/ipc/worktree-push-target-setup.ts @@ -6,11 +6,16 @@ import type { GitPushTarget } from '../../shared/worktree/types' import { findGitRemoteNameByFetchUrl } from '../../shared/git-remote-url-index' -import { sameGitHubRemoteUrl, type GitRemoteExec } from './worktree-push-target-cleanup' import { buildNarrowForkFetchRefspec, + ensureRemoteAlsoTracksBranch, ensureRemoteTracksBranchNarrowly } from '../git/fork-remote-refspec' +import { + remoteHasOrcaProvenance, + sameGitHubRemoteUrl, + type GitRemoteExec +} from './worktree-push-target-cleanup' // One `git remote -v` replaces `git remote` plus a serial `git remote get-url` per // remote -- 59 subprocesses at 58 remotes, on every push-target resolution (#17914). @@ -112,10 +117,15 @@ export async function prepareWorktreePushTargetWithExec( // Why: if a later PR worktree reuses an Orca-created fork remote, it // must inherit ownership so deleting the final user can remove it. remoteCreated = isRemoteCreatedByKnownWorktree(existingRemote) - // Why: a remote created before this fix (or reused for a second branch on the - // same fork) may still carry the wide default refspec; widen-but-bound it to - // cover this branch too rather than trusting whatever is already configured. - await ensureRemoteTracksBranchNarrowly(execGit, repoPath, remoteName, target.branchName) + // Why (#25703): only a remote Orca minted (marker set) may have its refspec + // rewritten -- narrowing a user-created remote breaks tracking for every other + // branch they push (`@{u}`, ahead/behind, PR lookup). For those, widen add-only: + // append this branch's narrow refspec, never drop or rewrite their entries. + const orcaCreated = await remoteHasOrcaProvenance(execGit, repoPath, remoteName) + const widenRefspec = orcaCreated + ? ensureRemoteTracksBranchNarrowly + : ensureRemoteAlsoTracksBranch + await widenRefspec(execGit, repoPath, remoteName, target.branchName) } else { remoteName = await ensureUniqueRemoteName(execGit, repoPath, target.remoteName) // Why: `-t --no-tags` means this remote is never, even transiently, diff --git a/src/main/ipc/worktree-remote-push-target-materialization.test.ts b/src/main/ipc/worktree-remote-push-target-materialization.test.ts index 695baa10decf..65e302efeb9c 100644 --- a/src/main/ipc/worktree-remote-push-target-materialization.test.ts +++ b/src/main/ipc/worktree-remote-push-target-materialization.test.ts @@ -58,6 +58,15 @@ describe('materializeWorktreePushTargetRemote', () => { if (args[0] === 'config' && args[1] === '--get-all') { throw new Error('no such section') } + // Orca's own minted remote: provenance marker reads true, so the narrow + // rewrite (not the add-only reuse path) applies. + if ( + args[0] === 'config' && + args[1] === '--get' && + String(args[2]).endsWith('.orca-created') + ) { + return { stdout: 'true\n', stderr: '' } + } if (args[0] === 'symbolic-ref') { return { stdout: 'contributor/fix\n', stderr: '' } } @@ -86,6 +95,47 @@ describe('materializeWorktreePushTargetRemote', () => { ]) }) + // #25703: the deferred materialize path reaches user-created remotes too (the store may + // have persisted a reused remote's name). Without the provenance marker it must widen + // add-only, never rewrite the user's refspec config. + it('widens add-only on the short-circuit path when the remote has no orca-created marker', async () => { + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'remote' && args[1] === 'get-url') { + return { stdout: `${FORK_URL}\n`, stderr: '' } + } + if (args[0] === 'config' && args[1] === '--get-all') { + return { stdout: `+refs/heads/*:refs/remotes/${FORK_REMOTE}/*\n`, stderr: '' } + } + if ( + args[0] === 'config' && + args[1] === '--get' && + String(args[2]).endsWith('.orca-created') + ) { + throw new Error('no marker') + } + if (args[0] === 'rev-parse') { + throw new Error('unknown revision') + } + if (args[0] === 'symbolic-ref') { + return { stdout: 'contributor/fix\n', stderr: '' } + } + return { stdout: '', stderr: '' } + }) + const target = forkTarget() + + await materializeWorktreePushTargetRemote(REPO_PATH, target) + + const calls = gitExecFileAsyncMock.mock.calls.map((call) => call[0] as string[]) + expect(calls).not.toContainEqual(['config', '--unset-all', `remote.${FORK_REMOTE}.fetch`]) + expect(calls).not.toContainEqual(['config', `remote.${FORK_REMOTE}.tagOpt`, '--no-tags']) + expect(calls).toContainEqual([ + 'config', + '--add', + `remote.${FORK_REMOTE}.fetch`, + `+refs/heads/${target.branchName}*:refs/remotes/${FORK_REMOTE}/${target.branchName}*` + ]) + }) + it('materializes the remote (add + provenance + fetch) when the probe misses', async () => { gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { if (args[0] === 'remote' && args[1] === 'get-url') { diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index 902f9203b55b..e61dcd73680d 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -104,6 +104,7 @@ import { parseWorkspaceKey, worktreeWorkspaceKey } from '../../shared/workspace- import { sharesWorktreeLineageBoundary } from '../../shared/resolved-worktree-lineage' import { cleanupUnusedWorktreePushTargetRemoteWithExec, + remoteHasOrcaProvenance, sameGitHubRemoteUrl, type GitRemoteExec, type WorktreePushTargetStore @@ -122,6 +123,7 @@ import { } from './worktree-push-target-setup' import { buildNarrowForkFetchRefspec, + ensureRemoteAlsoTracksBranch, ensureRemoteTracksBranchNarrowly, forkRemoteTrackingRefExists } from '../git/fork-remote-refspec' @@ -1107,7 +1109,11 @@ async function adoptExistingForkRemoteForBranch( repoId: string | undefined, worktreeId: string | undefined ): Promise { - await ensureRemoteTracksBranchNarrowly(execGit, repoPath, target.remoteName, target.branchName) + // Why (#25703): the target's name can be a user-created remote the store persisted after + // a reuse. Without the provenance marker, widen add-only -- never rewrite their refspec. + const orcaCreated = await remoteHasOrcaProvenance(execGit, repoPath, target.remoteName) + const widenRefspec = orcaCreated ? ensureRemoteTracksBranchNarrowly : ensureRemoteAlsoTracksBranch + await widenRefspec(execGit, repoPath, target.remoteName, target.branchName) // Why: widening only rewrites config -- it never imports anything. For a sibling worktree's // first materialize of a *new* branch on an already-existing remote, the branch's tracking // ref doesn't exist yet, and `--set-upstream-to` below hard-fails with "the requested