mirror of
https://github.com/stablyai/orca.git
synced 2026-11-05 19:47:27 +00:00
fix(github): compare a tracked upstream with its own remote's default branch
A fork's upstream remote can default to a different branch than origin. Read refs/remotes/<remote>/HEAD for the tracked remote and fall back to origin's default only when git never recorded it. Reuses the default-branch resolver's host routing and cache.
This commit is contained in:
@@ -46,13 +46,23 @@ const {
|
||||
|
||||
const ACME = { owner: 'acme', repo: 'widgets' }
|
||||
|
||||
function primeGit(trackedUpstream: string): void {
|
||||
/** remoteHeads maps a remote to the branch its `refs/remotes/<remote>/HEAD` points at. */
|
||||
function primeGit(
|
||||
trackedUpstream: string,
|
||||
remoteHeads: Record<string, string> = { origin: 'develop' }
|
||||
): void {
|
||||
gitExecFileAsyncMock.mockImplementation(async (args: string[]) => {
|
||||
if (args[0] === 'for-each-ref' && args.includes('--format=%(refname)%00%(upstream)')) {
|
||||
return { stdout: `refs/heads/feature/my-change\0${trackedUpstream}\n`, stderr: '' }
|
||||
}
|
||||
if (args[0] === 'for-each-ref' && args.includes('--format=%(refname)%00%(symref)')) {
|
||||
return { stdout: 'refs/remotes/origin/HEAD\0refs/remotes/origin/develop\n', stderr: '' }
|
||||
const stdout = Object.entries(remoteHeads)
|
||||
.filter(([remote]) => args.includes(`refs/remotes/${remote}/HEA[D]`))
|
||||
.map(
|
||||
([remote, branch]) => `refs/remotes/${remote}/HEAD\0refs/remotes/${remote}/${branch}\n`
|
||||
)
|
||||
.join('')
|
||||
return { stdout, stderr: '' }
|
||||
}
|
||||
if (args[0] === 'rev-parse' && args[1] === 'HEAD') {
|
||||
return { stdout: 'feature-head-oid\n', stderr: '' }
|
||||
@@ -105,6 +115,18 @@ function primeGh(listsByHead: Record<string, Record<string, unknown>[]>): void {
|
||||
})
|
||||
}
|
||||
|
||||
/** A fork clone: `origin` is the fork, `upstream` is the repo PRs target. */
|
||||
function primeForkWithUpstream(): void {
|
||||
const UPSTREAM = { owner: 'stablyai', repo: 'widgets' }
|
||||
resolvePRRepositoryCandidatesMock.mockResolvedValue({
|
||||
candidates: [UPSTREAM, ACME],
|
||||
headRepo: ACME
|
||||
})
|
||||
getOwnerRepoForRemoteMock.mockImplementation(async (_repoPath: string, remoteName: string) =>
|
||||
remoteName === 'upstream' ? UPSTREAM : remoteName === 'origin' ? ACME : null
|
||||
)
|
||||
}
|
||||
|
||||
describe('issue #26948: a branch tracking the default branch', () => {
|
||||
beforeEach(() => {
|
||||
resetPRForBranchMocks(clientMocks)
|
||||
@@ -130,14 +152,7 @@ describe('issue #26948: a branch tracking the default branch', () => {
|
||||
})
|
||||
|
||||
it('skips the default branch on a second remote that PRs target (fork checkout off upstream)', async () => {
|
||||
const UPSTREAM = { owner: 'stablyai', repo: 'widgets' }
|
||||
resolvePRRepositoryCandidatesMock.mockResolvedValue({
|
||||
candidates: [UPSTREAM, ACME],
|
||||
headRepo: ACME
|
||||
})
|
||||
getOwnerRepoForRemoteMock.mockImplementation(async (_repoPath: string, remoteName: string) =>
|
||||
remoteName === 'upstream' ? UPSTREAM : remoteName === 'origin' ? ACME : null
|
||||
)
|
||||
primeForkWithUpstream()
|
||||
primeGit('refs/remotes/upstream/develop')
|
||||
primeGh({ 'stablyai:develop': [restPR('develop')] })
|
||||
|
||||
@@ -146,6 +161,26 @@ describe('issue #26948: a branch tracking the default branch', () => {
|
||||
expect(pr).toBeNull()
|
||||
})
|
||||
|
||||
it("uses the tracked remote's own default branch when it differs from origin's", async () => {
|
||||
primeForkWithUpstream()
|
||||
primeGit('refs/remotes/upstream/develop', { origin: 'main', upstream: 'develop' })
|
||||
primeGh({ 'stablyai:develop': [restPR('develop')] })
|
||||
|
||||
const pr = await getPRForBranch('/repo-root', 'feature/my-change')
|
||||
|
||||
expect(pr).toBeNull()
|
||||
})
|
||||
|
||||
it("keeps a PR headed by a tracked branch that is only origin's default", async () => {
|
||||
primeForkWithUpstream()
|
||||
primeGit('refs/remotes/upstream/develop', { origin: 'develop', upstream: 'main' })
|
||||
primeGh({ 'stablyai:develop': [restPR('develop')] })
|
||||
|
||||
const pr = await getPRForBranch('/repo-root', 'feature/my-change')
|
||||
|
||||
expect(pr).toMatchObject({ number: 7 })
|
||||
})
|
||||
|
||||
it("still follows a contributor fork's branch that shares the default branch's name", async () => {
|
||||
getOwnerRepoForRemoteMock.mockImplementation(async (_repoPath: string, remoteName: string) =>
|
||||
remoteName === 'contributor' ? { owner: 'contributor', repo: 'widgets' } : ACME
|
||||
|
||||
@@ -1,6 +1,9 @@
|
||||
import type { OwnerRepo } from '../../gh-utils'
|
||||
import { githubRepoIdentityKey } from '../../../../shared/github/repository-identity-key'
|
||||
import { getRepoDefaultBranchName } from '../../../source-control/repo-default-branch'
|
||||
import {
|
||||
getRemoteHeadBranchName,
|
||||
getRepoDefaultBranchName
|
||||
} from '../../../source-control/repo-default-branch'
|
||||
import type { HostedReviewLocalGitOptions } from './../github-exec-scope'
|
||||
import type { TrackedUpstreamBranch } from './tracked-upstream-cache'
|
||||
|
||||
@@ -21,10 +24,19 @@ export async function isTrackedUpstreamDefaultBranch(input: {
|
||||
if (!input.candidates.some((candidate) => githubRepoIdentityKey(candidate) === upstreamRepoKey)) {
|
||||
return false
|
||||
}
|
||||
const defaultBranchName = await getRepoDefaultBranchName(
|
||||
input.repoPath,
|
||||
input.connectionId,
|
||||
input.localGitOptions
|
||||
)
|
||||
const { remoteName } = input.upstreamBranch
|
||||
// Why: a fork's `upstream` can default to a different branch than `origin`; use origin's only when the remote never recorded its HEAD.
|
||||
const remoteDefaultBranchName =
|
||||
remoteName === 'origin'
|
||||
? null
|
||||
: await getRemoteHeadBranchName(
|
||||
input.repoPath,
|
||||
remoteName,
|
||||
input.connectionId,
|
||||
input.localGitOptions
|
||||
)
|
||||
const defaultBranchName =
|
||||
remoteDefaultBranchName ??
|
||||
(await getRepoDefaultBranchName(input.repoPath, input.connectionId, input.localGitOptions))
|
||||
return defaultBranchName === input.upstreamBranch.branchName
|
||||
}
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import { resolveDefaultBaseRefViaExec } from '../git/repo'
|
||||
import { resolveRemoteHeadBranchViaExec, type GitExec } from '../../shared/git-default-base-ref'
|
||||
import { gitExecFileAsync } from '../git/runner'
|
||||
import { getSshGitProvider } from '../providers/ssh-git-dispatch'
|
||||
import type { HostedReviewLocalGitOptions } from './hosted-review-git-options'
|
||||
@@ -45,17 +46,80 @@ function pruneRepoDefaultBranchCache(now: number): void {
|
||||
}
|
||||
}
|
||||
|
||||
/** Git over the transport the hosted-review call uses; null when the SSH provider is gone. */
|
||||
function createRepoGitExec(
|
||||
repoPath: string,
|
||||
connectionId: string | null | undefined,
|
||||
localGitOptions: HostedReviewLocalGitOptions
|
||||
): GitExec | null {
|
||||
const provider = connectionId ? getSshGitProvider(connectionId) : null
|
||||
if (connectionId && !provider) {
|
||||
// Why: a dropped SSH provider must not fall back to local git — the
|
||||
// repoPath is remote, so a local run could answer for the wrong repo.
|
||||
return null
|
||||
}
|
||||
const resolutionDeadline = Date.now() + REPO_DEFAULT_BRANCH_RESOLUTION_BUDGET_MS
|
||||
// Why: the resolver can try five refs; share one deadline so an unhealthy
|
||||
// local/WSL/SSH host cannot multiply the refresh delay per fallback probe.
|
||||
return (argv) => {
|
||||
const timeoutMs = resolutionDeadline - Date.now()
|
||||
if (timeoutMs <= 0) {
|
||||
return Promise.reject(new Error('Default branch resolution timed out.'))
|
||||
}
|
||||
return provider
|
||||
? provider.exec(argv, repoPath, { timeoutMs })
|
||||
: gitExecFileAsync(argv, {
|
||||
cwd: repoPath,
|
||||
...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}),
|
||||
...(localGitOptions.admissionTier
|
||||
? { admissionTier: localGitOptions.admissionTier }
|
||||
: {}),
|
||||
timeout: timeoutMs
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve the repository's default branch NAME (`main`, `master`, …) over the
|
||||
* transport the surrounding hosted-review call already uses. Returns null when
|
||||
* unresolvable so callers fail open (#9171 guard skipped, current behavior).
|
||||
*/
|
||||
export async function getRepoDefaultBranchName(
|
||||
export function getRepoDefaultBranchName(
|
||||
repoPath: string,
|
||||
connectionId?: string | null,
|
||||
localGitOptions: HostedReviewLocalGitOptions = {}
|
||||
): Promise<string | null> {
|
||||
const cacheKey = getRepoDefaultBranchCacheKey(repoPath, connectionId, localGitOptions)
|
||||
return resolveCachedBranchName(
|
||||
getRepoDefaultBranchCacheKey(repoPath, connectionId, localGitOptions),
|
||||
async () => {
|
||||
const exec = createRepoGitExec(repoPath, connectionId, localGitOptions)
|
||||
const baseRef = exec ? await resolveDefaultBaseRefViaExec(exec) : null
|
||||
// Same base-ref → branch-name normalization as git/repo.ts getRemoteFileUrl.
|
||||
return baseRef ? baseRef.replace(/^origin\//, '') : null
|
||||
}
|
||||
)
|
||||
}
|
||||
|
||||
/** Branch the remote's own HEAD names; null on older Git or a remote added without it. */
|
||||
export function getRemoteHeadBranchName(
|
||||
repoPath: string,
|
||||
remoteName: string,
|
||||
connectionId?: string | null,
|
||||
localGitOptions: HostedReviewLocalGitOptions = {}
|
||||
): Promise<string | null> {
|
||||
return resolveCachedBranchName(
|
||||
[getRepoDefaultBranchCacheKey(repoPath, connectionId, localGitOptions), remoteName].join('\0'),
|
||||
async () => {
|
||||
const exec = createRepoGitExec(repoPath, connectionId, localGitOptions)
|
||||
return exec ? resolveRemoteHeadBranchViaExec(exec, remoteName) : null
|
||||
}
|
||||
)
|
||||
}
|
||||
|
||||
async function resolveCachedBranchName(
|
||||
cacheKey: string,
|
||||
resolve: () => Promise<string | null>
|
||||
): Promise<string | null> {
|
||||
const now = Date.now()
|
||||
const cached = repoDefaultBranchCache.get(cacheKey)
|
||||
if (cached && cached.expiresAt > now) {
|
||||
@@ -71,33 +135,7 @@ export async function getRepoDefaultBranchName(
|
||||
const resolution = (async (): Promise<string | null> => {
|
||||
let branchName: string | null = null
|
||||
try {
|
||||
const provider = connectionId ? getSshGitProvider(connectionId) : null
|
||||
if (connectionId && !provider) {
|
||||
// Why: a dropped SSH provider must not fall back to local git — the
|
||||
// repoPath is remote, so a local run could answer for the wrong repo.
|
||||
return null
|
||||
}
|
||||
const resolutionDeadline = Date.now() + REPO_DEFAULT_BRANCH_RESOLUTION_BUDGET_MS
|
||||
// Why: the resolver can try five refs; share one deadline so an unhealthy
|
||||
// local/WSL/SSH host cannot multiply the refresh delay per fallback probe.
|
||||
const baseRef = await resolveDefaultBaseRefViaExec((argv) => {
|
||||
const timeoutMs = resolutionDeadline - Date.now()
|
||||
if (timeoutMs <= 0) {
|
||||
return Promise.reject(new Error('Default branch resolution timed out.'))
|
||||
}
|
||||
return provider
|
||||
? provider.exec(argv, repoPath, { timeoutMs })
|
||||
: gitExecFileAsync(argv, {
|
||||
cwd: repoPath,
|
||||
...(localGitOptions.wslDistro ? { wslDistro: localGitOptions.wslDistro } : {}),
|
||||
...(localGitOptions.admissionTier
|
||||
? { admissionTier: localGitOptions.admissionTier }
|
||||
: {}),
|
||||
timeout: timeoutMs
|
||||
})
|
||||
})
|
||||
// Same base-ref → branch-name normalization as git/repo.ts getRemoteFileUrl.
|
||||
branchName = baseRef ? baseRef.replace(/^origin\//, '') : null
|
||||
branchName = await resolve()
|
||||
} catch {
|
||||
branchName = null
|
||||
}
|
||||
|
||||
@@ -35,3 +35,28 @@ export async function resolveDefaultBaseRefViaExec(exec: GitExec): Promise<strin
|
||||
return null
|
||||
}
|
||||
}
|
||||
|
||||
/** Branch a remote's `refs/remotes/<remote>/HEAD` points at, or null when git never recorded it. */
|
||||
export async function resolveRemoteHeadBranchViaExec(
|
||||
exec: GitExec,
|
||||
remoteName: string
|
||||
): Promise<string | null> {
|
||||
const headRef = `refs/remotes/${remoteName}/HEAD`
|
||||
const branchPrefix = `refs/remotes/${remoteName}/`
|
||||
try {
|
||||
const { stdout } = await exec([
|
||||
'for-each-ref',
|
||||
'--format=%(refname)%00%(symref)',
|
||||
`${headRef.slice(0, -1)}[${headRef.slice(-1)}]`
|
||||
])
|
||||
for (const line of iterateProcessOutputLines(stdout)) {
|
||||
const [ref, target] = line.split('\0')
|
||||
if (ref === headRef && target?.startsWith(branchPrefix)) {
|
||||
return target.slice(branchPrefix.length)
|
||||
}
|
||||
}
|
||||
return null
|
||||
} catch {
|
||||
return null
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user