diff --git a/src/main/git/git-operation-remote-roles.test.ts b/src/main/git/git-operation-remote-roles.test.ts index 307357c67cf..7351504fe70 100644 --- a/src/main/git/git-operation-remote-roles.test.ts +++ b/src/main/git/git-operation-remote-roles.test.ts @@ -76,7 +76,9 @@ function localProbe(args: { const remoteRefs = (args.matchingRemotes ?? []).map( (remote) => `refs/remotes/${remote}/${branch}\0${oid}` ) - return { stdout: [`refs/heads/${branch}\0${oid}`, ...remoteRefs].join('\n') } + return { + stdout: [`refs/heads/${branch}\0${oid}`, ...remoteRefs].join('\n') + } }) } @@ -104,13 +106,20 @@ describe('git operation remote roles', () => { localProbe({ remotes: ['origin', remote], matchingRemotes: [remote] }) await expect(resolve(['origin', remote])).resolves.toMatchObject({ - head: { kind: 'resolved', remoteName: remote, provenance: 'matching-remote-branch' } + head: { + kind: 'resolved', + selector: { kind: 'named-remote', value: remote }, + provenance: 'matching-remote-branch' + } }) } ) it('keeps two matching head remotes explicitly ambiguous', async () => { - localProbe({ remotes: ['fork-a', 'fork-b'], matchingRemotes: ['fork-a', 'fork-b'] }) + localProbe({ + remotes: ['fork-a', 'fork-b'], + matchingRemotes: ['fork-a', 'fork-b'] + }) await expect(resolve(['fork-a', 'fork-b'])).resolves.toMatchObject({ head: { @@ -133,7 +142,11 @@ describe('git operation remote roles', () => { }) await expect(resolve(['origin', 'fork'])).resolves.toMatchObject({ - head: { kind: 'resolved', remoteName: 'fork', provenance: 'matching-remote-branch' } + head: { + kind: 'resolved', + selector: { kind: 'named-remote', value: 'fork' }, + provenance: 'matching-remote-branch' + } }) }) @@ -151,7 +164,7 @@ describe('git operation remote roles', () => { ).resolves.toMatchObject({ issueSource: { kind: 'resolved', - remoteName: 'company', + selector: { kind: 'named-remote', value: 'company' }, provenance: 'persisted-exact-remote' } }) @@ -162,7 +175,7 @@ describe('git operation remote roles', () => { await expect(resolve(['gitlab-only'])).resolves.toMatchObject({ issueSource: { kind: 'resolved', - remoteName: 'gitlab-only', + selector: { kind: 'named-remote', value: 'gitlab-only' }, provenance: 'sole-provider-remote' } }) @@ -223,7 +236,10 @@ describe('git operation remote roles', () => { localProbe({ remotes: ['fork'], matchingRemotes: ['fork'] }) await expect(resolve(['fork'])).resolves.toMatchObject({ - head: { kind: 'resolved', remoteName: 'fork' } + head: { + kind: 'resolved', + selector: { kind: 'named-remote', value: 'fork' } + } }) }) @@ -236,7 +252,9 @@ describe('git operation remote roles', () => { if (command[0] === 'config') { return { stdout: '' } } - return { stdout: 'refs/heads/feature\0one\nrefs/remotes/old/feature\0one' } + return { + stdout: 'refs/heads/feature\0one\nrefs/remotes/old/feature\0one' + } }) } const secondProvider = { @@ -247,7 +265,9 @@ describe('git operation remote roles', () => { if (command[0] === 'config') { return { stdout: '' } } - return { stdout: 'refs/heads/feature\0two\nrefs/remotes/new/feature\0two' } + return { + stdout: 'refs/heads/feature\0two\nrefs/remotes/new/feature\0two' + } }) } getSshGitProviderMock.mockReturnValue(firstProvider) @@ -258,7 +278,9 @@ describe('git operation remote roles', () => { connectionId: 'ssh-1', eligibleRemotes: async (names) => names }) - ).resolves.toMatchObject({ head: { remoteName: 'old' } }) + ).resolves.toMatchObject({ + head: { selector: { kind: 'named-remote', value: 'old' } } + }) getSshGitProviderGenerationMock.mockReturnValue(1) getSshGitProviderMock.mockReturnValue(secondProvider) @@ -269,6 +291,8 @@ describe('git operation remote roles', () => { connectionId: 'ssh-1', eligibleRemotes: async (names) => names }) - ).resolves.toMatchObject({ head: { remoteName: 'new' } }) + ).resolves.toMatchObject({ + head: { selector: { kind: 'named-remote', value: 'new' } } + }) }) }) diff --git a/src/main/git/git-operation-remote-roles.ts b/src/main/git/git-operation-remote-roles.ts index 756e6915f68..3ecbc0b2447 100644 --- a/src/main/git/git-operation-remote-roles.ts +++ b/src/main/git/git-operation-remote-roles.ts @@ -1,13 +1,24 @@ import { gitRefTargetsBranchOnRemote } from '../../shared/git-remote-branch-name' -import { normalizeConfiguredGitRemote } from '../../shared/git-remote-url-index' +import { + gitOperationSelector, + type GitOperationSelector +} from '../../shared/git-operation-selector' import { normalizeGitConfigKey, type GitRemoteTopologySnapshot } from './git-remote-topology-snapshot' export type GitRemoteRoleResolution = - | { kind: 'resolved'; remoteName: string; provenance: GitRemoteRoleProvenance } - | { kind: 'ambiguous'; remoteNames: string[]; provenance: GitRemoteRoleProvenance } + | { + kind: 'resolved' + selector: GitOperationSelector + provenance: GitRemoteRoleProvenance + } + | { + kind: 'ambiguous' + remoteNames: string[] + provenance: GitRemoteRoleProvenance + } | { kind: 'unresolved' } export type GitRemoteRoleProvenance = @@ -18,22 +29,21 @@ export type GitRemoteRoleProvenance = | 'persisted-exact-remote' | 'sole-provider-remote' -function configuredRemote(snapshot: GitRemoteTopologySnapshot, key: string): string | null { +function configuredRemote( + snapshot: GitRemoteTopologySnapshot, + key: string +): GitOperationSelector | null { const value = snapshot.config.get(normalizeGitConfigKey(key))?.trim() if (!value) { return null } - if (snapshot.remoteNames.includes(value)) { - return value - } - const remote = normalizeConfiguredGitRemote(value, snapshot.fetchUrls) - return snapshot.remoteNames.includes(remote) ? remote : null + return gitOperationSelector(value, snapshot.remoteNames) } function configuredUpstreamRemote( snapshot: GitRemoteTopologySnapshot, branchName: string -): string | null { +): GitOperationSelector | null { const remoteName = configuredRemote(snapshot, `branch.${branchName}.remote`) const mergeRef = snapshot.config.get(normalizeGitConfigKey(`branch.${branchName}.merge`))?.trim() const mergeBranchName = mergeRef?.replace(/^refs\/heads\//, '') @@ -41,7 +51,7 @@ function configuredUpstreamRemote( return null } const baseRef = snapshot.config.get(normalizeGitConfigKey(`branch.${branchName}.base`)) - return gitRefTargetsBranchOnRemote(baseRef, remoteName, mergeBranchName) ? null : remoteName + return gitRefTargetsBranchOnRemote(baseRef, remoteName.value, mergeBranchName) ? null : remoteName } export function resolveHeadRole( @@ -51,16 +61,28 @@ export function resolveHeadRole( persistedExactRemoteName?: string ): GitRemoteRoleResolution { const pushRemote = configuredRemote(snapshot, `branch.${branchName}.pushRemote`) - if (pushRemote && eligibleRemotes.includes(pushRemote)) { - return { kind: 'resolved', remoteName: pushRemote, provenance: 'branch-push-remote' } + if (pushRemote) { + return { + kind: 'resolved', + selector: pushRemote, + provenance: 'branch-push-remote' + } } const pushDefault = configuredRemote(snapshot, 'remote.pushDefault') - if (pushDefault && eligibleRemotes.includes(pushDefault)) { - return { kind: 'resolved', remoteName: pushDefault, provenance: 'remote-push-default' } + if (pushDefault) { + return { + kind: 'resolved', + selector: pushDefault, + provenance: 'remote-push-default' + } } const upstream = configuredUpstreamRemote(snapshot, branchName) - if (upstream && eligibleRemotes.includes(upstream)) { - return { kind: 'resolved', remoteName: upstream, provenance: 'configured-upstream' } + if (upstream) { + return { + kind: 'resolved', + selector: upstream, + provenance: 'configured-upstream' + } } const localOid = snapshot.localBranchOids.get(branchName) const matching = localOid @@ -69,27 +91,39 @@ export function resolveHeadRole( ) : [] if (matching.length === 1) { - return { kind: 'resolved', remoteName: matching[0]!, provenance: 'matching-remote-branch' } + return { + kind: 'resolved', + selector: gitOperationSelector(matching[0]!, snapshot.remoteNames), + provenance: 'matching-remote-branch' + } } if (matching.length > 1) { - return { kind: 'ambiguous', remoteNames: matching, provenance: 'matching-remote-branch' } + return { + kind: 'ambiguous', + remoteNames: matching, + provenance: 'matching-remote-branch' + } } if (persistedExactRemoteName && eligibleRemotes.includes(persistedExactRemoteName)) { return { kind: 'resolved', - remoteName: persistedExactRemoteName, + selector: { kind: 'named-remote', value: persistedExactRemoteName }, provenance: 'persisted-exact-remote' } } if (eligibleRemotes.length === 1) { return { kind: 'resolved', - remoteName: eligibleRemotes[0]!, + selector: { kind: 'named-remote', value: eligibleRemotes[0]! }, provenance: 'sole-provider-remote' } } return eligibleRemotes.length > 1 - ? { kind: 'ambiguous', remoteNames: [...eligibleRemotes], provenance: 'sole-provider-remote' } + ? { + kind: 'ambiguous', + remoteNames: [...eligibleRemotes], + provenance: 'sole-provider-remote' + } : { kind: 'unresolved' } } @@ -100,19 +134,23 @@ export function resolveIssueSourceRole( if (persistedExactRemoteName && eligibleRemotes.includes(persistedExactRemoteName)) { return { kind: 'resolved', - remoteName: persistedExactRemoteName, + selector: { kind: 'named-remote', value: persistedExactRemoteName }, provenance: 'persisted-exact-remote' } } if (eligibleRemotes.length === 1) { return { kind: 'resolved', - remoteName: eligibleRemotes[0]!, + selector: { kind: 'named-remote', value: eligibleRemotes[0]! }, provenance: 'sole-provider-remote' } } return eligibleRemotes.length > 1 - ? { kind: 'ambiguous', remoteNames: [...eligibleRemotes], provenance: 'sole-provider-remote' } + ? { + kind: 'ambiguous', + remoteNames: [...eligibleRemotes], + provenance: 'sole-provider-remote' + } : { kind: 'unresolved' } } diff --git a/src/main/git/git-remote-topology-snapshot.ts b/src/main/git/git-remote-topology-snapshot.ts index 832bc72d41f..4b70eabed20 100644 --- a/src/main/git/git-remote-topology-snapshot.ts +++ b/src/main/git/git-remote-topology-snapshot.ts @@ -1,3 +1,4 @@ +import { captureGitSelectorEndpoints, type GitSelectorEndpoints } from './git-selector-endpoints' import { parseGitRemoteFetchUrls, parseGitRemoteVerboseLine @@ -12,6 +13,7 @@ import type { GitAdmissionTier } from './command-runner/git-exec-options' import { gitExecFileAsync } from './runner' export type GitRemoteTopologySnapshot = { + selectorEndpoints?: Map config: Map localBranchOids: Map remoteBranchOids: Map @@ -151,8 +153,18 @@ async function probeSnapshot( if (remoteNames.length > SNAPSHOT_MAX_REMOTES) { throw new Error('Git remote topology has too many remotes to resolve safely.') } + const config = parseConfigSnapshot(configResult.stdout) + const selectorEndpoints = await captureGitSelectorEndpoints( + config, + remoteNames, + runGit, + SNAPSHOT_MAX_URLS - + fetchUrls.size - + [...pushUrls.values()].reduce((sum, urls) => sum + urls.length, 0) + ) return { - config: parseConfigSnapshot(configResult.stdout), + config, + selectorEndpoints, ...refs, remoteNames, fetchUrls, diff --git a/src/main/git/git-repository-evidence.ts b/src/main/git/git-repository-evidence.ts index 9065e622906..cd0d12aaf6b 100644 --- a/src/main/git/git-repository-evidence.ts +++ b/src/main/git/git-repository-evidence.ts @@ -1,3 +1,4 @@ +import type { GitOperationSelector } from '../../shared/git-operation-selector' import type { GitRemoteTopologySnapshot } from './git-remote-topology-snapshot' import type { GitRemoteRoleProvenance, GitRemoteRoleResolution } from './git-operation-remote-roles' @@ -10,7 +11,7 @@ export type GitRepositoryRole = | { kind: 'resolved' repository: T - remoteName: string + selector: GitOperationSelector direction: 'fetch' | 'push' provenance: GitRemoteRoleProvenance confidence: 'tracked' | 'inferred' @@ -20,6 +21,7 @@ export type GitRepositoryRole = | { kind: 'unresolved' } export type GitRemoteRepositories = { + selectors: Map; push: GitRepositoryEvidence[] }> fetch: Map> push: Map[]> } @@ -47,7 +49,14 @@ export async function resolveSnapshotRepositories( push.set(name, await Promise.all((snapshot.pushUrls.get(name) ?? []).map(resolve))) }) ) - return { fetch, push } + const selectors: GitRemoteRepositories['selectors'] = new Map() + for (const [value, endpoints] of snapshot.selectorEndpoints ?? []) { + selectors.set(value, { + fetch: await resolve(endpoints.fetch), + push: await Promise.all(endpoints.push.map(resolve)) + }) + } + return { fetch, push, selectors } } export function plausibleRepositoryRemotes( @@ -64,16 +73,25 @@ export function bindRepositoryRole( if (role.kind !== 'resolved') { return role } - const evidence = - direction === 'fetch' - ? [repositories.fetch.get(role.remoteName) ?? { kind: 'unverifiable' as const }] - : (repositories.push.get(role.remoteName) ?? []) + const value = role.selector.value + const literal = role.selector.kind === 'literal-url' + const evidence = literal + ? direction === 'fetch' + ? [ + repositories.selectors.get(value)?.fetch ?? { + kind: 'unverifiable' as const + } + ] + : (repositories.selectors.get(value)?.push ?? []) + : direction === 'fetch' + ? [repositories.fetch.get(value) ?? { kind: 'unverifiable' as const }] + : (repositories.push.get(value) ?? []) if (evidence.length > 1) { - return { kind: 'ambiguous', remoteNames: [role.remoteName] } + return { kind: 'ambiguous', remoteNames: [value] } } const identity = evidence[0] if (!identity || identity.kind === 'unverifiable') { - return { kind: 'unverifiable', remoteNames: [role.remoteName] } + return { kind: 'unverifiable', remoteNames: [value] } } if (identity.kind === 'non-provider') { return { kind: 'unresolved' } diff --git a/src/main/git/git-selector-endpoints.test.ts b/src/main/git/git-selector-endpoints.test.ts new file mode 100644 index 00000000000..1675d730fdc --- /dev/null +++ b/src/main/git/git-selector-endpoints.test.ts @@ -0,0 +1,59 @@ +import { expect, it, vi } from 'vitest' +import { captureGitSelectorEndpoints } from './git-selector-endpoints' + +it('rejects excessive literal selectors before issuing host commands', async () => { + const run = vi.fn() + await expect( + captureGitSelectorEndpoints( + new Map([ + ['branch.a.pushremote', 'https://github.com/a/repo'], + ['branch.b.remote', 'https://github.com/b/repo'] + ]), + [], + run, + 2 + ) + ).rejects.toThrow('too many URLs') + expect(run).not.toHaveBeenCalled() +}) + +it('deduplicates literals and avoids configured synthetic-name collisions', async () => { + const url = 'https://github.com/canonical/repo' + const run = vi.fn(async () => ({ + stdout: [ + 'orca-operation-selector--\thttps://github.com/canonical/repo (fetch)', + 'orca-operation-selector--\thttps://github.com/canonical/repo (push)', + 'orca-operation-selector\thttps://github.com/wrong/repo (push)' + ].join('\n') + })) + const result = await captureGitSelectorEndpoints( + new Map([ + ['branch.a.pushremote', url], + ['remote.pushdefault', url], + ['remote.orca-operation-selector-.pushurl', 'https://github.com/wrong/repo'] + ]), + ['orca-operation-selector'], + run, + 2 + ) + expect(run).toHaveBeenCalledExactlyOnceWith([ + '-c', + `remote.orca-operation-selector--.url=${url}`, + 'remote', + '-v' + ]) + expect(result.get(url)).toEqual({ fetch: url, push: [url] }) +}) + +it('propagates execution-host failures without inventing local endpoint evidence', async () => { + const run = vi.fn().mockRejectedValue(new Error('Remote connection dropped.')) + await expect( + captureGitSelectorEndpoints( + new Map([['branch.feature.pushremote', 'ssh://example.invalid/repo']]), + [], + run, + 2 + ) + ).rejects.toThrow('Remote connection dropped.') + expect(run).toHaveBeenCalledTimes(1) +}) diff --git a/src/main/git/git-selector-endpoints.ts b/src/main/git/git-selector-endpoints.ts new file mode 100644 index 00000000000..39929af21e9 --- /dev/null +++ b/src/main/git/git-selector-endpoints.ts @@ -0,0 +1,55 @@ +import type { GitCommandRunner } from '../../shared/git-effective-upstream' +import { gitOperationSelector } from '../../shared/git-operation-selector' +import { parseGitRemoteVerboseLine } from '../../shared/git-remote-url-index' + +export type GitSelectorEndpoints = { fetch: string; push: string[] } + +export async function captureGitSelectorEndpoints( + config: Map, + remoteNames: readonly string[], + runGit: GitCommandRunner, + urlBudget: number +): Promise> { + const selectors = new Set() + for (const [key, raw] of config) { + if (key === 'remote.pushdefault' || /^branch\..+\.(remote|pushremote)$/.test(key)) { + const value = raw.trim() + if ( + value && + value !== '.' && + gitOperationSelector(value, remoteNames).kind === 'literal-url' + ) { + selectors.add(value) + } + } + } + if (selectors.size * 2 > urlBudget) { + throw new Error('Git remote topology has too many URLs to resolve safely.') + } + const endpoints = new Map() + // Command-local configuration lets the execution host apply all Git URL rewrites. + for (const value of selectors) { + let name = 'orca-operation-selector' + while ( + remoteNames.includes(name) || + [...config.keys()].some((key) => key.startsWith(`remote.${name}.`)) + ) { + name += '-' + } + const { stdout } = await runGit(['-c', `remote.${name}.url=${value}`, 'remote', '-v']) + const result: GitSelectorEndpoints = { fetch: '', push: [] } + for (const line of stdout.split(/\r?\n/)) { + const entry = parseGitRemoteVerboseLine(line) + if (entry?.name !== name) { + continue + } + if (entry.direction === 'fetch') { + result.fetch = entry.url + } else { + result.push.push(entry.url) + } + } + endpoints.set(value, result) + } + return endpoints +} diff --git a/src/main/git/remote.test.ts b/src/main/git/remote.test.ts index feb237eb18a..77415334aa3 100644 --- a/src/main/git/remote.test.ts +++ b/src/main/git/remote.test.ts @@ -170,7 +170,7 @@ describe('git remote operations', () => { ) }) - it('normalizes a URL-valued branch remote to a matching named remote before pushing', async () => { + it('preserves a URL-valued branch remote before pushing', async () => { gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { if (args[0] === 'symbolic-ref') { return { stdout: 'imp/chinese-translation\n', stderr: '' } @@ -213,14 +213,19 @@ describe('git remote operations', () => { await gitPush('/repo', false) expect(gitExecFileAsyncMock).toHaveBeenLastCalledWith( - ['push', '--set-upstream', 'pr-pynickle-orca', 'HEAD:imp/chinese-translation'], + [ + 'push', + '--set-upstream', + 'https://github.com/pynickle/orca.git', + 'HEAD:imp/chinese-translation' + ], { cwd: '/repo' } ) }) // Regression: normalizing a URL-valued push remote used to run `git remote` and then a // serial `git remote get-url` per remote -- 59 subprocesses on a 58-remote repo. - it('normalizes a URL-valued push remote from one remote table read at 58 remotes', async () => { + it('preserves a URL-valued push remote without scanning 58 named remotes', async () => { const remotes = [ { name: 'origin', url: 'https://github.com/stablyai/orca.git' }, ...Array.from({ length: 56 }, (_, index) => ({ @@ -259,9 +264,14 @@ describe('git remote operations', () => { await gitPush('/repo', false) const remoteReads = gitExecFileAsyncMock.mock.calls.filter(([args]) => args[0] === 'remote') - expect(remoteReads.map(([args]) => args)).toEqual([['remote', '-v']]) + expect(remoteReads.map(([args]) => args)).toEqual([]) expect(gitExecFileAsyncMock).toHaveBeenLastCalledWith( - ['push', '--set-upstream', 'pr-pynickle-orca', 'HEAD:imp/chinese-translation'], + [ + 'push', + '--set-upstream', + 'https://github.com/pynickle/orca.git', + 'HEAD:imp/chinese-translation' + ], { cwd: '/repo' } ) }) diff --git a/src/main/git/remote.ts b/src/main/git/remote.ts index aa7932687ca..976950733f9 100644 --- a/src/main/git/remote.ts +++ b/src/main/git/remote.ts @@ -82,7 +82,12 @@ async function gitPullWithArgs( // Why: legacy Orca branches may still track origin/main while pushes // target origin/. Pull the same effective branch the UI reports. await gitExecFileAsync( - ['pull', ...effectiveArgs, upstream.remoteName, upstream.branchName], + [ + 'pull', + ...effectiveArgs, + upstream.operationSelector?.value ?? upstream.remoteName, + upstream.branchName + ], gitOptionsForWorktree(worktreePath, options) ) return diff --git a/src/main/github/client-pr-operation-selector.test.ts b/src/main/github/client-pr-operation-selector.test.ts new file mode 100644 index 00000000000..9c0091ff64b --- /dev/null +++ b/src/main/github/client-pr-operation-selector.test.ts @@ -0,0 +1,166 @@ +import type * as GitRunner from '../git/runner' +import { execFileSync } from 'node:child_process' +import { mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { gh } = vi.hoisted(() => ({ gh: vi.fn() })) +vi.mock('../git/runner', async (importOriginal) => ({ + ...(await importOriginal()), + ghExecFileAsync: gh +})) +import { getPRForBranchOutcome } from './client/lookup/pr-for-branch-outcome' +import { resolveGitHubApiRepositoryCandidates } from './github-api-repository' +import { _resetGitRemoteTopologySnapshotCache } from '../git/git-remote-topology-snapshot' + +const canonical = 'https://github.com/canonical/repo.git' +const contributor = 'https://github.com/contributor/repo.git' + +describe('literal Git operation selectors through shipping review discovery', () => { + let repo = '' + function git(...args: string[]) { + return execFileSync('git', args, { + cwd: repo, + encoding: 'utf8', + timeout: 10_000 + }).trim() + } + beforeEach(() => { + repo = mkdtempSync(join(tmpdir(), 'orca-selector-')) + vi.stubEnv('HOME', repo) + vi.stubEnv('ORCA_E2E_HOME_DIR', repo) + vi.stubEnv('ORCA_E2E_USER_DATA_DIR', join(repo, 'user-data')) + vi.stubEnv('GIT_CONFIG_NOSYSTEM', '1') + vi.stubEnv('GIT_CONFIG_GLOBAL', join(repo, 'empty-config')) + git('init', '-q') + git( + '-c', + 'user.name=Fixture', + '-c', + 'user.email=fixture@example.invalid', + 'commit', + '--allow-empty', + '-qm', + 'fixture' + ) + git('checkout', '-qb', 'feature') + git('remote', 'add', 'origin', canonical) + git('remote', 'set-url', '--push', 'origin', contributor) + git('config', 'branch.feature.merge', 'refs/heads/feature') + _resetGitRemoteTopologySnapshotCache() + gh.mockReset() + }) + afterEach(() => { + vi.unstubAllEnvs() + rmSync(repo, { recursive: true, force: true }) + }) + function forge(owner: string) { + const identity = { + name: 'repo', + nameWithOwner: `${owner}/repo`, + owner: { login: owner }, + html_url: `https://github.com/${owner}/repo` + } + gh.mockImplementation(async (args: string[]) => { + if (args[0] === 'pr' && args[1] === 'view') { + return { + stdout: JSON.stringify({ + number: 91, + title: `Hydrated ${owner}`, + state: 'OPEN', + url: 'https://github.com/canonical/repo/pull/91', + headRefName: 'feature', + headRefOid: 'sha', + headRepository: identity, + headRepositoryOwner: { login: owner }, + statusCheckRollup: [], + updatedAt: '', + mergeable: 'UNKNOWN', + baseRefName: 'main' + }) + } + } + if (args[0] === 'api' && args[1].includes(`head=${owner}%3Afeature`)) { + return { + stdout: JSON.stringify([ + { + number: 91, + title: owner, + state: 'open', + html_url: 'https://github.com/canonical/repo/pull/91', + head: { ref: 'feature', sha: 'sha', repo: identity }, + base: { ref: 'main' } + } + ]) + } + } + return { stdout: '[]' } + }) + } + it.each(['branch.feature.pushRemote', 'remote.pushDefault', 'branch.feature.remote'])( + 'preserves literal %s through both discovery and successful hydration', + async (key) => { + git('config', key, canonical) + forge('contributor') + await expect(getPRForBranchOutcome(repo, 'feature')).resolves.toMatchObject({ + kind: key === 'branch.feature.remote' ? 'no-pr' : 'upstream-error' + }) + expect(gh.mock.calls.some(([args]) => args[1] === 'view')).toBe(false) + forge('canonical') + await expect(getPRForBranchOutcome(repo, 'feature')).resolves.toMatchObject({ + kind: 'found', + pr: { + title: 'Hydrated canonical', + headRepo: { owner: 'canonical', repo: 'repo' } + } + }) + } + ) + it.each(['branch.feature.pushRemote', 'remote.pushDefault'])( + 'retains named %s pushurl semantics', + async (key) => { + git('config', key, 'origin') + forge('contributor') + await expect(getPRForBranchOutcome(repo, 'feature')).resolves.toMatchObject({ + kind: 'found', + pr: { headRepo: { owner: 'contributor' } } + }) + } + ) + it.each(['insteadOf', 'pushInsteadOf'])( + 'lets execution-host Git expand literal %s without borrowing pushurl', + async (rewrite) => { + git('config', `url.${canonical}.${rewrite}`, 'shortcut:repo') + git('config', 'branch.feature.pushRemote', 'shortcut:repo') + forge('canonical') + await expect(getPRForBranchOutcome(repo, 'feature')).resolves.toMatchObject({ + kind: 'found', + pr: { headRepo: { owner: 'canonical' } } + }) + } + ) + it('retains repeated rewrite values and longest-prefix selection', async () => { + git('config', '--add', `url.${canonical}.pushInsteadOf`, 'shortcut:repo') + git('config', '--add', `url.${canonical}.pushInsteadOf`, 'another:repo') + git('config', `url.https://github.com/wrong/.pushInsteadOf`, 'shortcut:') + git('config', 'branch.feature.pushRemote', 'shortcut:repo') + forge('canonical') + await expect(getPRForBranchOutcome(repo, 'feature')).resolves.toMatchObject({ + kind: 'found', + pr: { headRepo: { owner: 'canonical' } } + }) + }) + it('does not fall through an explicit non-provider selector to origin', async () => { + git('config', 'branch.feature.pushRemote', './local-repository') + forge('contributor') + await expect(getPRForBranchOutcome(repo, 'feature')).resolves.toMatchObject({ + kind: 'upstream-error' + }) + }) + it('keeps upstream selection in the fetch direction', async () => { + git('config', 'branch.feature.remote', 'origin') + const result = await resolveGitHubApiRepositoryCandidates(repo, null, {}, 'feature') + expect(result.headRepo).toMatchObject({ owner: 'canonical' }) + }) +}) diff --git a/src/relay/git-handler-push-target.test.ts b/src/relay/git-handler-push-target.test.ts index b6fe5e96ad0..82b768ad14f 100644 --- a/src/relay/git-handler-push-target.test.ts +++ b/src/relay/git-handler-push-target.test.ts @@ -126,7 +126,7 @@ describe('resolveRelayPushTarget', () => { }) }) - it('normalizes a URL-valued branch remote to a matching named remote', async () => { + it('preserves a URL-valued branch remote despite a matching named remote', async () => { const forkUrl = 'https://github.com/contributor/orca.git' const git = gitForConfig({ pushRemote: new Error('missing pushRemote'), @@ -140,7 +140,7 @@ describe('resolveRelayPushTarget', () => { }) await expect(resolveRelayPushTarget(git, '/repo', undefined)).resolves.toEqual({ - remote: 'pr-contributor-orca', + remote: forkUrl, refspec: 'HEAD:feature/fix' }) }) diff --git a/src/relay/git-handler-sync-operations.ts b/src/relay/git-handler-sync-operations.ts index 262517b33cc..ecdf7786b0c 100644 --- a/src/relay/git-handler-sync-operations.ts +++ b/src/relay/git-handler-sync-operations.ts @@ -76,7 +76,12 @@ export class GitHandlerSyncOperations extends GitHandlerOperationContext { if (upstream && !upstream.isConfiguredUpstream) { // Why: legacy Orca branches may track origin/main while pushes target origin/; pull the same effective branch the UI reports. await this.git( - ['pull', ...effectiveArgs, upstream.remoteName, upstream.branchName], + [ + 'pull', + ...effectiveArgs, + upstream.operationSelector?.value ?? upstream.remoteName, + upstream.branchName + ], worktreePath ) return diff --git a/src/relay/git-pull-selector-local-parity.test.ts b/src/relay/git-pull-selector-local-parity.test.ts new file mode 100644 index 00000000000..28b525485b6 --- /dev/null +++ b/src/relay/git-pull-selector-local-parity.test.ts @@ -0,0 +1,56 @@ +import { expect, it, vi } from 'vitest' + +const { run } = vi.hoisted(() => ({ run: vi.fn() })) +vi.mock('../main/git/runner', () => ({ gitExecFileAsync: run })) +vi.mock('../main/git/status', () => ({ + runWithGitReadCacheInvalidation: (fn: () => unknown) => fn() +})) +vi.mock('../main/git/local-repo-ref-maintenance', () => ({ + postponeRepoRefMaintenance: () => {}, + withRepoRefMaintenancePaused: (_key: string, fn: () => unknown) => fn() +})) +import { gitPull } from '../main/git/remote' +import { RelayContext } from './context' +import { GitHandler } from './git-handler' +import { createMockDispatcher, type RelayDispatcher } from './git-handler-test-setup' + +it('keeps the literal pull selector separate from its matching status tracking ref on both hosts', async () => { + const url = 'https://github.com/canonical/repo.git' + const calls: string[][] = [] + const script = async (args: string[]) => { + calls.push(args) + if (args[0] === 'symbolic-ref') { + return { stdout: 'feature', stderr: '' } + } + if (args[0] === 'rev-parse' && args.includes('HEAD@{u}')) { + throw new Error("fatal: no upstream configured for branch 'feature'") + } + if (args[0] === 'config') { + const values: Record = { + 'branch.feature.remote': url, + 'branch.feature.merge': 'refs/heads/feature' + } + if (!(args[2] in values)) { + throw new Error('missing config') + } + return { stdout: values[args[2]], stderr: '' } + } + if (args[0] === 'remote') { + return { + stdout: `origin\t${url} (fetch)\norigin\thttps://github.com/contributor/repo.git (push)`, + stderr: '' + } + } + return { stdout: '', stderr: '' } + } + run.mockImplementation(script) + await gitPull('/repo') + expect(calls.find((args) => args[0] === 'pull')).toEqual(['pull', url, 'feature']) + expect(calls.some((args) => args.includes('refs/remotes/origin/feature'))).toBe(true) + calls.length = 0 + const dispatcher = createMockDispatcher() + const handler = new GitHandler(dispatcher as unknown as RelayDispatcher, new RelayContext()) + vi.spyOn(handler as unknown as { git: typeof script }, 'git').mockImplementation(script) + await dispatcher.callRequest('git.pull', { worktreePath: '/repo' }) + expect(calls.find((args) => args[0] === 'pull')).toEqual(['pull', url, 'feature']) +}) diff --git a/src/relay/git-push-target-local-parity.test.ts b/src/relay/git-push-target-local-parity.test.ts index 152b062d88e..f513ffac4d5 100644 --- a/src/relay/git-push-target-local-parity.test.ts +++ b/src/relay/git-push-target-local-parity.test.ts @@ -10,7 +10,9 @@ */ import { beforeEach, describe, expect, it, vi } from 'vitest' -const { gitExecFileAsyncMock } = vi.hoisted(() => ({ gitExecFileAsyncMock: vi.fn() })) +const { gitExecFileAsyncMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn() +})) vi.mock('../main/git/runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock @@ -173,7 +175,7 @@ describe('relay/desktop push-target parity', () => { ) }) - it('resolves a URL-valued pushRemote back to its remote name', async () => { + it('preserves a URL-valued pushRemote through local and relay execution', async () => { await expectSamePushArgv( { branch: 'review/pr-1738', @@ -185,7 +187,7 @@ describe('relay/desktop push-target parity', () => { fork: 'git@example.invalid:contributor/repo.git' } }, - ['push', '--set-upstream', 'fork', 'HEAD:contributor/fix'] + ['push', '--set-upstream', 'git@example.invalid:contributor/repo.git', 'HEAD:contributor/fix'] ) }) diff --git a/src/shared/git-configured-branch-target.test.ts b/src/shared/git-configured-branch-target.test.ts index a599a53bbb8..277c2e53116 100644 --- a/src/shared/git-configured-branch-target.test.ts +++ b/src/shared/git-configured-branch-target.test.ts @@ -69,7 +69,7 @@ const fiftyEightRemotes: RemoteRow[] = [ ] describe('hasConfiguredBranchPushTarget', () => { - it('resolves both URL-valued remotes from one remote table read at 58 remotes', async () => { + it('preserves both URL-valued selectors without reading the remote table', async () => { const { runGit, spawns } = makeRunner({ remotes: fiftyEightRemotes, config: { @@ -82,7 +82,7 @@ describe('hasConfiguredBranchPushTarget', () => { await expect(hasConfiguredBranchPushTarget(runGit, BRANCH)).resolves.toBe(true) // Both the push remote and the branch remote name the same URL, so one table read answers. - expect(spawns.filter((args) => args[0] === 'remote')).toEqual([['remote', '-v']]) + expect(spawns.filter((args) => args[0] === 'remote')).toEqual([]) expect(spawns.filter((args) => args[1] === 'get-url')).toEqual([]) }) @@ -125,6 +125,7 @@ describe('getConfiguredBranchRemoteUpstream', () => { await expect( getConfiguredBranchRemoteUpstream(runGit, BRANCH, remoteTrackingRefExists) ).resolves.toEqual({ + operationSelector: { kind: 'literal-url', value: FORK_URL }, upstreamName: `fork-a/${BRANCH}`, remoteName: 'fork-a', branchName: BRANCH, diff --git a/src/shared/git-configured-branch-target.ts b/src/shared/git-configured-branch-target.ts index 9faca91dce7..8529ced1a72 100644 --- a/src/shared/git-configured-branch-target.ts +++ b/src/shared/git-configured-branch-target.ts @@ -1,3 +1,4 @@ +import type { GitOperationSelector } from './git-operation-selector' import { gitRefTargetsBranchOnRemote } from './git-remote-branch-name' import { findGitRemoteNameByFetchUrl } from './git-remote-url-index' @@ -6,6 +7,7 @@ type GitCommandRunner = (args: string[]) => Promise<{ stdout: string }> type RemoteTrackingRefExists = (remoteName: string, branchName: string) => Promise export type ConfiguredBranchRemoteUpstream = { + operationSelector?: GitOperationSelector upstreamName: string remoteName: string branchName: string @@ -64,6 +66,9 @@ export async function getConfiguredBranchRemoteUpstream( return null } return { + ...(remote !== remoteName + ? { operationSelector: { kind: 'literal-url' as const, value: remote } } + : {}), upstreamName: `${remoteName}/${branchName}`, remoteName, branchName, @@ -87,26 +92,12 @@ export async function hasConfiguredBranchPushTarget( if (!remote || remote === '.' || !branchName || branchName === mergeRef) { return false } - const pushRemoteName = isUrlValuedRemote(remote) - ? ((await findRemoteNameForUrl(runGit, remote)) ?? remote) - : remote - // The two usually name the same URL; resolving it twice reads the remote table twice. - const branchRemoteName = !branchRemote - ? null - : branchRemote === remote - ? pushRemoteName - : isUrlValuedRemote(branchRemote) - ? ((await findRemoteNameForUrl(runGit, branchRemote)) ?? branchRemote) - : branchRemote - if (gitRefTargetsBranchOnRemote(baseRef, pushRemoteName, branchName)) { + if (gitRefTargetsBranchOnRemote(baseRef, remote, branchName)) { return false } // Why: branch.merge belongs to branch.remote. Do not combine a user's // pushDefault fork with an origin/main merge target and call it pushable. - if ( - branchName !== currentBranchName && - (pushRemoteName === 'origin' || branchRemoteName !== pushRemoteName) - ) { + if (branchName !== currentBranchName && (remote === 'origin' || branchRemote !== remote)) { return false } return true diff --git a/src/shared/git-effective-upstream.ts b/src/shared/git-effective-upstream.ts index db364c53b20..25337151089 100644 --- a/src/shared/git-effective-upstream.ts +++ b/src/shared/git-effective-upstream.ts @@ -1,3 +1,4 @@ +import type { GitOperationSelector } from './git-operation-selector' import { isNoUpstreamError } from './git-remote-error' import type { GitUpstreamStatus } from './git-status-types' import { @@ -24,6 +25,7 @@ export type EffectiveGitUpstream = remoteName: string branchName: string isConfiguredUpstream: false + operationSelector?: GitOperationSelector } function hasMultipleSlashSegments(refName: string): boolean { diff --git a/src/shared/git-operation-selector.ts b/src/shared/git-operation-selector.ts new file mode 100644 index 00000000000..5db160f6020 --- /dev/null +++ b/src/shared/git-operation-selector.ts @@ -0,0 +1,14 @@ +export type GitOperationSelector = + | { kind: 'named-remote'; value: string } + | { kind: 'literal-url'; value: string } + +// Git treats an unregistered selector as a URL/path, even when it matches a remote URL. +export function gitOperationSelector( + value: string, + remoteNames: readonly string[] +): GitOperationSelector { + return { + kind: remoteNames.includes(value) ? 'named-remote' : 'literal-url', + value + } +} diff --git a/src/shared/git-push-target-resolution.ts b/src/shared/git-push-target-resolution.ts index 90da4998012..b836e4db462 100644 --- a/src/shared/git-push-target-resolution.ts +++ b/src/shared/git-push-target-resolution.ts @@ -1,10 +1,5 @@ import type { GitCommandRunner } from './git-effective-upstream' import { gitRefTargetsBranchOnRemote } from './git-remote-branch-name' -import { - isUrlValuedGitRemote, - normalizeConfiguredGitRemote, - parseGitRemoteFetchUrls -} from './git-remote-url-index' export type ResolvedGitPushTarget = { remote: string @@ -26,27 +21,6 @@ type ConfiguredPushRemote = { branchRemote: string | null } -// One `git remote -v` instead of `git remote` plus a serial `git remote get-url` -// per remote; both print the same insteadOf-expanded fetch URL. -async function findRemoteNameForUrl( - runGit: GitCommandRunner, - remoteUrl: string -): Promise { - try { - const { stdout } = await runGit(['remote', '-v']) - return normalizeConfiguredGitRemote(remoteUrl, parseGitRemoteFetchUrls(stdout)) - } catch { - return null - } -} - -async function normalizePushRemote(runGit: GitCommandRunner, remote: string): Promise { - if (!isUrlValuedGitRemote(remote)) { - return remote - } - return (await findRemoteNameForUrl(runGit, remote)) ?? remote -} - async function getConfiguredPushRemote( runGit: GitCommandRunner, branch: string @@ -59,16 +33,7 @@ async function getConfiguredPushRemote( if (!remote) { return null } - const normalizedRemote = await normalizePushRemote(runGit, remote) - // The two usually name the same URL; resolving it twice reads the remote table twice. - if (!branchRemote) { - return { remote: normalizedRemote, branchRemote: null } - } - return { - remote: normalizedRemote, - branchRemote: - branchRemote === remote ? normalizedRemote : await normalizePushRemote(runGit, branchRemote) - } + return { remote, branchRemote } } async function branchMergeTargetsConfiguredBase( diff --git a/src/shared/git-remote-url-index.ts b/src/shared/git-remote-url-index.ts index 2af2dff88c6..0c8b5a95aec 100644 --- a/src/shared/git-remote-url-index.ts +++ b/src/shared/git-remote-url-index.ts @@ -66,13 +66,3 @@ export function findGitRemoteNameByFetchUrl( export function isUrlValuedGitRemote(remote: string): boolean { return /^[A-Za-z][A-Za-z0-9+.-]*:\/\//.test(remote) || /^[^@/:]+@[^:]+:.+/.test(remote) } - -export function normalizeConfiguredGitRemote( - remote: string, - fetchUrls: Map -): string { - if (!isUrlValuedGitRemote(remote)) { - return remote - } - return [...fetchUrls].find(([, url]) => url === remote)?.[0] ?? remote -}