diff --git a/src/main/git/remote.ts b/src/main/git/remote.ts index 20cf8415d04..aa7932687ca 100644 --- a/src/main/git/remote.ts +++ b/src/main/git/remote.ts @@ -3,8 +3,7 @@ import { runPullWithDivergenceFallback } from '../../shared/git-remote-error' import { resolveEffectiveGitUpstream } from '../../shared/git-effective-upstream' -import { gitRefTargetsBranchOnRemote } from '../../shared/git-remote-branch-name' -import { findGitRemoteNameByFetchUrl } from '../../shared/git-remote-url-index' +import { resolveConfiguredGitPushTarget } from '../../shared/git-push-target-resolution' import type { GitPushTarget } from '../../shared/worktree/types' import type { GitRuntimeOptions } from './git-runtime-options' import { gitOptionsForWorktree } from './git-runtime-options' @@ -20,157 +19,6 @@ import { runWithGitWorktreeOperationLock } from '../../shared/git-worktree-opera export { gitPullRebaseFromBase } from './remote-rebase' -async function getConfiguredPushTarget( - worktreePath: string, - options: GitRuntimeOptions = {} -): Promise<{ remote: string; refspec: string } | null> { - try { - const { stdout: branchStdout } = await gitExecFileAsync( - ['symbolic-ref', '--quiet', '--short', 'HEAD'], - gitOptionsForWorktree(worktreePath, options) - ) - const branch = branchStdout.trim() - if (!branch) { - return null - } - - const [pushRemote, { stdout: mergeStdout }] = await Promise.all([ - getConfiguredPushRemote(worktreePath, branch, options), - gitExecFileAsync( - ['config', '--get', `branch.${branch}.merge`], - gitOptionsForWorktree(worktreePath, options) - ) - ]) - const remote = pushRemote?.remote - const mergeRef = mergeStdout.trim() - const branchRef = mergeRef.replace(/^refs\/heads\//, '') - if (!remote || !branchRef || remote === '.' || branchRef === mergeRef) { - return null - } - if (await branchMergeTargetsConfiguredBase(worktreePath, branch, remote, branchRef, options)) { - return null - } - if (!canPushConfiguredMergeBranch(pushRemote, branch, branchRef)) { - return null - } - return { remote, refspec: `HEAD:${branchRef}` } - } catch { - return null - } -} - -async function getConfigValue( - worktreePath: string, - key: string, - options: GitRuntimeOptions = {} -): Promise { - try { - const { stdout } = await gitExecFileAsync( - ['config', '--get', key], - gitOptionsForWorktree(worktreePath, options) - ) - const value = stdout.trim() - return value || null - } catch { - return null - } -} - -function isUrlValuedRemote(remote: string): boolean { - return /^[A-Za-z][A-Za-z0-9+.-]*:\/\//.test(remote) || /^[^@/:]+@[^:]+:.+/.test(remote) -} - -type ConfiguredPushRemote = { - remote: string - 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( - worktreePath: string, - remoteUrl: string, - options: GitRuntimeOptions = {} -): Promise { - try { - const { stdout } = await gitExecFileAsync( - ['remote', '-v'], - gitOptionsForWorktree(worktreePath, options) - ) - return findGitRemoteNameByFetchUrl(stdout, (candidateUrl) => candidateUrl === remoteUrl) - } catch { - return null - } -} - -async function normalizePushRemote( - worktreePath: string, - remote: string, - options: GitRuntimeOptions = {} -): Promise { - if (!isUrlValuedRemote(remote)) { - return remote - } - return (await findRemoteNameForUrl(worktreePath, remote, options)) ?? remote -} - -async function getConfiguredPushRemote( - worktreePath: string, - branch: string, - options: GitRuntimeOptions = {} -): Promise { - const branchRemote = await getConfigValue(worktreePath, `branch.${branch}.remote`, options) - const remote = - (await getConfigValue(worktreePath, `branch.${branch}.pushRemote`, options)) ?? - (await getConfigValue(worktreePath, 'remote.pushDefault', options)) ?? - branchRemote - if (!remote) { - return null - } - const normalizedRemote = await normalizePushRemote(worktreePath, remote, options) - // 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(worktreePath, branchRemote, options) - } -} - -async function branchMergeTargetsConfiguredBase( - worktreePath: string, - branch: string, - remote: string, - branchRef: string, - options: GitRuntimeOptions = {} -): Promise { - return gitRefTargetsBranchOnRemote( - await getConfigValue(worktreePath, `branch.${branch}.base`, options), - remote, - branchRef - ) -} - -function canPushConfiguredMergeBranch( - pushRemote: ConfiguredPushRemote | null, - branch: string, - branchRef: string -): boolean { - if (!pushRemote) { - return false - } - if (branchRef === branch) { - return true - } - // Why: branch.merge belongs to branch.remote. A pushDefault fork must not - // inherit origin/main as its destination branch. - return pushRemote.remote !== 'origin' && pushRemote.branchRemote === pushRemote.remote -} - function explicitPushTarget(target: GitPushTarget): { remote: string; refspec: string } { return { remote: target.remoteName, refspec: `HEAD:${target.branchName}` } } @@ -197,7 +45,9 @@ export async function gitPush( // from worktree config, not the upstream relationship. const target = pushTarget ? explicitPushTarget(pushTarget) - : await getConfiguredPushTarget(worktreePath, options) + : await resolveConfiguredGitPushTarget((args) => + gitExecFileAsync(args, gitOptionsForWorktree(worktreePath, options)) + ) const args = [ 'push', ...(options.forceWithLease ? ['--force-with-lease'] : []), diff --git a/src/relay/git-handler-push-target.ts b/src/relay/git-handler-push-target.ts index d81111d45d3..6663b5b3ad3 100644 --- a/src/relay/git-handler-push-target.ts +++ b/src/relay/git-handler-push-target.ts @@ -1,168 +1,24 @@ import { assertGitPushTargetShape } from '../shared/git-push-target-validation' -import { gitRefTargetsBranchOnRemote } from '../shared/git-remote-branch-name' -import { findGitRemoteNameByFetchUrl } from '../shared/git-remote-url-index' +import { + resolveConfiguredGitPushTarget, + type ResolvedGitPushTarget +} from '../shared/git-push-target-resolution' import type { GitPushTarget } from '../shared/worktree/types' type RelayGit = (args: string[], cwd: string) => Promise<{ stdout: string; stderr: string }> -export type ResolvedPushTarget = { - remote: string - refspec: string -} - -async function getConfiguredPushTarget( - git: RelayGit, - worktreePath: string -): Promise { - try { - const { stdout: branchStdout } = await git( - ['symbolic-ref', '--quiet', '--short', 'HEAD'], - worktreePath - ) - const branch = branchStdout.trim() - if (!branch) { - return null - } - const [pushRemote, { stdout: mergeStdout }] = await Promise.all([ - getConfiguredPushRemote(git, worktreePath, branch), - git(['config', '--get', `branch.${branch}.merge`], worktreePath) - ]) - const remote = pushRemote?.remote - const mergeRef = mergeStdout.trim() - const branchRef = mergeRef.replace(/^refs\/heads\//, '') - if (!remote || !branchRef || remote === '.' || branchRef === mergeRef) { - return null - } - if (await branchMergeTargetsConfiguredBase(git, worktreePath, branch, remote, branchRef)) { - return null - } - if (!canPushConfiguredMergeBranch(pushRemote, branch, branchRef)) { - return null - } - return { remote, refspec: `HEAD:${branchRef}` } - } catch { - return null - } -} - -async function getConfigValue( - git: RelayGit, - worktreePath: string, - key: string -): Promise { - try { - const { stdout } = await git(['config', '--get', key], worktreePath) - const value = stdout.trim() - return value || null - } catch { - return null - } -} - -function isUrlValuedRemote(remote: string): boolean { - return /^[A-Za-z][A-Za-z0-9+.-]*:\/\//.test(remote) || /^[^@/:]+@[^:]+:.+/.test(remote) -} - -type ConfiguredPushRemote = { - remote: string - branchRemote: string | null -} - -// Host-side twin of `src/main/git/remote.ts`: one `git remote -v` instead of -// `git remote` plus a serial `git remote get-url` per remote. -async function findRemoteNameForUrl( - git: RelayGit, - worktreePath: string, - remoteUrl: string -): Promise { - try { - const { stdout } = await git(['remote', '-v'], worktreePath) - return findGitRemoteNameByFetchUrl(stdout, (candidateUrl) => candidateUrl === remoteUrl) - } catch { - return null - } -} - -async function normalizePushRemote( - git: RelayGit, - worktreePath: string, - remote: string -): Promise { - if (!isUrlValuedRemote(remote)) { - return remote - } - return (await findRemoteNameForUrl(git, worktreePath, remote)) ?? remote -} - -async function getConfiguredPushRemote( - git: RelayGit, - worktreePath: string, - branch: string -): Promise { - // Why: mirror the local gitPush resolver so SSH worktrees do not drift to a - // different target when branch.pushRemote or remote.pushDefault is present. - const branchRemote = await getConfigValue(git, worktreePath, `branch.${branch}.remote`) - const remote = - (await getConfigValue(git, worktreePath, `branch.${branch}.pushRemote`)) ?? - (await getConfigValue(git, worktreePath, 'remote.pushDefault')) ?? - branchRemote - if (!remote) { - return null - } - const normalizedRemote = await normalizePushRemote(git, worktreePath, 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(git, worktreePath, branchRemote) - } -} - -async function branchMergeTargetsConfiguredBase( - git: RelayGit, - worktreePath: string, - branch: string, - remote: string, - branchRef: string -): Promise { - return gitRefTargetsBranchOnRemote( - await getConfigValue(git, worktreePath, `branch.${branch}.base`), - remote, - branchRef - ) -} - -function canPushConfiguredMergeBranch( - pushRemote: ConfiguredPushRemote | null, - branch: string, - branchRef: string -): boolean { - if (!pushRemote) { - return false - } - if (branchRef === branch) { - return true - } - // Why: branch.merge belongs to branch.remote. A pushDefault fork must not - // inherit origin/main as its destination branch. - return pushRemote.remote !== 'origin' && pushRemote.branchRemote === pushRemote.remote -} - export async function resolveRelayPushTarget( git: RelayGit, worktreePath: string, pushTarget: unknown -): Promise { +): Promise { if (pushTarget === undefined) { - return getConfiguredPushTarget(git, worktreePath) + return resolveConfiguredGitPushTarget((args) => git(args, worktreePath)) } assertGitPushTargetShape(pushTarget) const explicitTarget: GitPushTarget = pushTarget + // Why here and not in the shared resolver: an explicit target arrives over the wire, + // so the host re-validates its shape and asks Git to vet the branch name itself. await git(['check-ref-format', '--branch', explicitTarget.branchName], worktreePath) return { remote: explicitTarget.remoteName, diff --git a/src/relay/git-push-target-local-parity.test.ts b/src/relay/git-push-target-local-parity.test.ts new file mode 100644 index 00000000000..152b062d88e --- /dev/null +++ b/src/relay/git-push-target-local-parity.test.ts @@ -0,0 +1,209 @@ +/** + * Push-target resolution decides which remote a plain `git push` hits, and a wrong + * answer is not recoverable by retrying. The relay and the desktop used to carry + * identical ~160-line copies of it; they now share one implementation. + * + * These tests script one repository's Git config and require `git.push` over the real + * relay dispatcher and the desktop's `gitPush` to emit the *same push argv*, plus the + * argv each case is supposed to produce — so a second implementation on either side + * fails here even if it is wrong in the same direction on both. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { gitExecFileAsyncMock } = vi.hoisted(() => ({ gitExecFileAsyncMock: vi.fn() })) + +vi.mock('../main/git/runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock +})) + +import { gitPush } from '../main/git/remote' +import { RelayContext } from './context' +import { GitHandler } from './git-handler' +import { createMockDispatcher, type RelayDispatcher } from './git-handler-test-setup' + +const WORKTREE_PATH = '/worktree' + +type GitConfigFixture = { + /** Empty means detached HEAD: `symbolic-ref --quiet --short HEAD` prints nothing. */ + branch: string + merge?: string + branchRemote?: string + pushRemote?: string + pushDefault?: string + base?: string + /** remote name -> fetch URL, as `git remote -v` prints it. */ + remotes?: Record +} + +type GitSpyTarget = { + git(args: string[], cwd: string): Promise<{ stdout: string; stderr: string }> +} + +/** One scripted repository, driven identically by both hosts. */ +function scriptGit(fixture: GitConfigFixture) { + const configValues = new Map() + const put = (key: string, value: string | undefined): void => { + if (value !== undefined) { + configValues.set(key, value) + } + } + put(`branch.${fixture.branch}.merge`, fixture.merge) + put(`branch.${fixture.branch}.remote`, fixture.branchRemote) + put(`branch.${fixture.branch}.pushRemote`, fixture.pushRemote) + put(`branch.${fixture.branch}.base`, fixture.base) + put('remote.pushDefault', fixture.pushDefault) + + const calls: string[][] = [] + return { + calls, + run: async (args: string[]): Promise<{ stdout: string; stderr: string }> => { + calls.push(args) + if (args[0] === 'symbolic-ref') { + return { stdout: `${fixture.branch}\n`, stderr: '' } + } + if (args[0] === 'config' && args[1] === '--get') { + const value = configValues.get(args[2] ?? '') + // Why throw: `git config --get` exits 1 for a missing key, and the resolver's + // fallback chain reads that rejection, not an empty string. + if (value === undefined) { + throw Object.assign(new Error('missing config key'), { code: 1 }) + } + return { stdout: `${value}\n`, stderr: '' } + } + if (args[0] === 'remote' && args[1] === '-v') { + const lines = Object.entries(fixture.remotes ?? {}).flatMap(([name, url]) => [ + `${name}\t${url} (fetch)`, + `${name}\t${url} (push)` + ]) + return { stdout: `${lines.join('\n')}\n`, stderr: '' } + } + if (args[0] === 'push') { + return { stdout: '', stderr: '' } + } + throw new Error(`Unexpected git command: ${args.join(' ')}`) + } + } +} + +function pushArgv(calls: string[][]): string[] { + const push = calls.find((args) => args[0] === 'push') + if (!push) { + throw new Error('no push command was issued') + } + return push +} + +async function pushOverRelay(fixture: GitConfigFixture): Promise { + const dispatcher = createMockDispatcher() + const handler = new GitHandler(dispatcher as unknown as RelayDispatcher, new RelayContext()) + const script = scriptGit(fixture) + vi.spyOn(handler as unknown as GitSpyTarget, 'git').mockImplementation((args) => script.run(args)) + await dispatcher.callRequest('git.push', { worktreePath: WORKTREE_PATH }) + return pushArgv(script.calls) +} + +async function pushLocally(fixture: GitConfigFixture): Promise { + const script = scriptGit(fixture) + gitExecFileAsyncMock.mockImplementation((args: string[]) => script.run(args)) + await gitPush(WORKTREE_PATH) + return pushArgv(script.calls) +} + +async function expectSamePushArgv(fixture: GitConfigFixture, expected: string[]): Promise { + const relayArgv = await pushOverRelay(fixture) + const localArgv = await pushLocally(fixture) + expect(relayArgv).toEqual(localArgv) + expect(localArgv).toEqual(expected) +} + +const FIRST_PUBLISH = ['push', '--set-upstream', 'origin', 'HEAD'] + +beforeEach(() => { + gitExecFileAsyncMock.mockReset() +}) + +describe('relay/desktop push-target parity', () => { + it('sends a review branch to the fork its pushDefault names', async () => { + await expectSamePushArgv( + { + branch: 'review/pr-1738', + merge: 'refs/heads/contributor/fix', + branchRemote: 'fork', + pushDefault: 'fork' + }, + ['push', '--set-upstream', 'fork', 'HEAD:contributor/fix'] + ) + }) + + it('refuses to inherit origin/main as a destination for a differently named branch', async () => { + // branch.merge belongs to branch.remote; a branch tracking origin/main must + // first-publish under its own name rather than push onto main. + await expectSamePushArgv( + { + branch: 'feature/fix', + merge: 'refs/heads/main', + branchRemote: 'origin' + }, + FIRST_PUBLISH + ) + }) + + it('refuses a pushDefault fork whose branch.remote names a different remote', async () => { + await expectSamePushArgv( + { + branch: 'review/pr-1738', + merge: 'refs/heads/contributor/fix', + branchRemote: 'origin', + pushDefault: 'fork' + }, + FIRST_PUBLISH + ) + }) + + it('refuses when branch.base names the same remote branch as branch.merge', async () => { + await expectSamePushArgv( + { + branch: 'feature/fix', + merge: 'refs/heads/release', + branchRemote: 'fork', + pushRemote: 'fork', + base: 'fork/release' + }, + FIRST_PUBLISH + ) + }) + + it('resolves a URL-valued pushRemote back to its remote name', async () => { + await expectSamePushArgv( + { + branch: 'review/pr-1738', + merge: 'refs/heads/contributor/fix', + branchRemote: 'git@example.invalid:contributor/repo.git', + pushRemote: 'git@example.invalid:contributor/repo.git', + remotes: { + origin: 'git@example.invalid:upstream/repo.git', + fork: 'git@example.invalid:contributor/repo.git' + } + }, + ['push', '--set-upstream', 'fork', 'HEAD:contributor/fix'] + ) + }) + + it('treats a local-repository remote as no configured target', async () => { + await expectSamePushArgv( + { + branch: 'feature/fix', + merge: 'refs/heads/feature/fix', + branchRemote: '.' + }, + FIRST_PUBLISH + ) + }) + + it('first-publishes a branch with no configured remote at all', async () => { + await expectSamePushArgv( + { branch: 'feature/fix', merge: 'refs/heads/feature/fix' }, + FIRST_PUBLISH + ) + }) +}) diff --git a/src/shared/git-push-target-resolution.ts b/src/shared/git-push-target-resolution.ts new file mode 100644 index 00000000000..63fe7828d9e --- /dev/null +++ b/src/shared/git-push-target-resolution.ts @@ -0,0 +1,140 @@ +import type { GitCommandRunner } from './git-effective-upstream' +import { gitRefTargetsBranchOnRemote } from './git-remote-branch-name' +import { findGitRemoteNameByFetchUrl } from './git-remote-url-index' + +export type ResolvedGitPushTarget = { + remote: string + refspec: string +} + +async function getConfigValue(runGit: GitCommandRunner, key: string): Promise { + try { + const { stdout } = await runGit(['config', '--get', key]) + const value = stdout.trim() + return value || null + } catch { + return null + } +} + +function isUrlValuedRemote(remote: string): boolean { + return /^[A-Za-z][A-Za-z0-9+.-]*:\/\//.test(remote) || /^[^@/:]+@[^:]+:.+/.test(remote) +} + +type ConfiguredPushRemote = { + remote: string + 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 findGitRemoteNameByFetchUrl(stdout, (candidateUrl) => candidateUrl === remoteUrl) + } catch { + return null + } +} + +async function normalizePushRemote(runGit: GitCommandRunner, remote: string): Promise { + if (!isUrlValuedRemote(remote)) { + return remote + } + return (await findRemoteNameForUrl(runGit, remote)) ?? remote +} + +async function getConfiguredPushRemote( + runGit: GitCommandRunner, + branch: string +): Promise { + const branchRemote = await getConfigValue(runGit, `branch.${branch}.remote`) + const remote = + (await getConfigValue(runGit, `branch.${branch}.pushRemote`)) ?? + (await getConfigValue(runGit, 'remote.pushDefault')) ?? + branchRemote + 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) + } +} + +async function branchMergeTargetsConfiguredBase( + runGit: GitCommandRunner, + branch: string, + remote: string, + branchRef: string +): Promise { + return gitRefTargetsBranchOnRemote( + await getConfigValue(runGit, `branch.${branch}.base`), + remote, + branchRef + ) +} + +function canPushConfiguredMergeBranch( + pushRemote: ConfiguredPushRemote | null, + branch: string, + branchRef: string +): boolean { + if (!pushRemote) { + return false + } + if (branchRef === branch) { + return true + } + // Why: branch.merge belongs to branch.remote. A pushDefault fork must not + // inherit origin/main as its destination branch. + return pushRemote.remote !== 'origin' && pushRemote.branchRemote === pushRemote.remote +} + +/** + * Which remote and refspec a plain `git push` from this worktree should hit, or `null` + * to fall back to first-publish (`origin HEAD`). + * + * Why shared: this decides where commits land, and a wrong answer is not recoverable by + * retrying. The local runner and the SSH relay must never be able to answer differently + * for the same repository — they differ only in how `runGit` reaches the Git binary. + */ +export async function resolveConfiguredGitPushTarget( + runGit: GitCommandRunner +): Promise { + try { + const { stdout: branchStdout } = await runGit(['symbolic-ref', '--quiet', '--short', 'HEAD']) + const branch = branchStdout.trim() + if (!branch) { + return null + } + const [pushRemote, { stdout: mergeStdout }] = await Promise.all([ + getConfiguredPushRemote(runGit, branch), + runGit(['config', '--get', `branch.${branch}.merge`]) + ]) + const remote = pushRemote?.remote + const mergeRef = mergeStdout.trim() + const branchRef = mergeRef.replace(/^refs\/heads\//, '') + if (!remote || !branchRef || remote === '.' || branchRef === mergeRef) { + return null + } + if (await branchMergeTargetsConfiguredBase(runGit, branch, remote, branchRef)) { + return null + } + if (!canPushConfiguredMergeBranch(pushRemote, branch, branchRef)) { + return null + } + return { remote, refspec: `HEAD:${branchRef}` } + } catch { + return null + } +}