Preserve literal Git operation selectors through endpoint resolution

This commit is contained in:
Merge Sim
2026-09-07 19:34:37 -07:00
parent 2040e8764f
commit 19afeb02c3
19 changed files with 534 additions and 121 deletions
+35 -11
View File
@@ -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' } }
})
})
})
+63 -25
View File
@@ -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' }
}
+13 -1
View File
@@ -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<string, GitSelectorEndpoints>
config: Map<string, string>
localBranchOids: Map<string, string>
remoteBranchOids: Map<string, string>
@@ -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,
+26 -8
View File
@@ -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<T> =
| {
kind: 'resolved'
repository: T
remoteName: string
selector: GitOperationSelector
direction: 'fetch' | 'push'
provenance: GitRemoteRoleProvenance
confidence: 'tracked' | 'inferred'
@@ -20,6 +21,7 @@ export type GitRepositoryRole<T> =
| { kind: 'unresolved' }
export type GitRemoteRepositories<T> = {
selectors: Map<string, { fetch: GitRepositoryEvidence<T>; push: GitRepositoryEvidence<T>[] }>
fetch: Map<string, GitRepositoryEvidence<T>>
push: Map<string, GitRepositoryEvidence<T>[]>
}
@@ -47,7 +49,14 @@ export async function resolveSnapshotRepositories<T>(
push.set(name, await Promise.all((snapshot.pushUrls.get(name) ?? []).map(resolve)))
})
)
return { fetch, push }
const selectors: GitRemoteRepositories<T>['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<T>(
@@ -64,16 +73,25 @@ export function bindRepositoryRole<T>(
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' }
@@ -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)
})
+55
View File
@@ -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<string, string>,
remoteNames: readonly string[],
runGit: GitCommandRunner,
urlBudget: number
): Promise<Map<string, GitSelectorEndpoints>> {
const selectors = new Set<string>()
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<string, GitSelectorEndpoints>()
// 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
}
+15 -5
View File
@@ -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' }
)
})
+6 -1
View File
@@ -82,7 +82,12 @@ async function gitPullWithArgs(
// Why: legacy Orca branches may still track origin/main while pushes
// target origin/<branch>. 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
@@ -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<typeof GitRunner>()),
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' })
})
})
+2 -2
View File
@@ -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'
})
})
+6 -1
View File
@@ -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/<branch>; 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
@@ -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<string, string> = {
'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'])
})
@@ -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']
)
})
@@ -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,
+7 -16
View File
@@ -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<boolean>
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
+2
View File
@@ -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 {
+14
View File
@@ -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
}
}
+1 -36
View File
@@ -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<string | null> {
try {
const { stdout } = await runGit(['remote', '-v'])
return normalizeConfiguredGitRemote(remoteUrl, parseGitRemoteFetchUrls(stdout))
} catch {
return null
}
}
async function normalizePushRemote(runGit: GitCommandRunner, remote: string): Promise<string> {
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(
-10
View File
@@ -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, string>
): string {
if (!isUrlValuedGitRemote(remote)) {
return remote
}
return [...fetchUrls].find(([, url]) => url === remote)?.[0] ?? remote
}