diff --git a/packages/server/src/server/resolve-worktree-creation-intent.test.ts b/packages/server/src/server/resolve-worktree-creation-intent.test.ts index 82895e302b..8d44b9ba80 100644 --- a/packages/server/src/server/resolve-worktree-creation-intent.test.ts +++ b/packages/server/src/server/resolve-worktree-creation-intent.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from "vitest"; import type { ForgeService } from "../services/forge-service.js"; +import { buildForkLocalBranchName } from "../utils/change-request-checkout.js"; import { CheckoutSourceForgeMismatchError, MissingCheckoutTargetError, @@ -32,14 +33,8 @@ function createResolverHarness(overrides?: { defaultCheckoutRefs: ({ changeRequestNumber }) => [ { remoteName: "origin", remoteRef: `refs/pull/${changeRequestNumber}/head` }, ], - buildPrLocalBranchName: ({ headRef, checkoutTarget }) => { - const normalized = checkoutTarget.headOwnerLogin?.trim().toLowerCase() ?? ""; - const owner = - checkoutTarget.isCrossRepository && /^[a-z0-9-]+$/.test(normalized) - ? normalized - : null; - return owner ? `${owner}/${headRef}` : headRef; - }, + buildPrLocalBranchName: ({ headRef, checkoutTarget }) => + buildForkLocalBranchName({ headRef, ...checkoutTarget }), supportsCrossRepoCheckoutWithoutRefs: true, } : {}; @@ -182,6 +177,28 @@ describe("resolveWorktreeCreationIntent", () => { expect(deps.headRefLookups).toEqual([]); }); + test("does not track origin for a same-repo head with no origin branch", async () => { + const deps = createResolverHarness(); + deps.forgeService.getPullRequestCheckoutTarget = async () => ({ + number: 42, + baseRefName: "main", + headRefName: "ford/towel", + headOwnerLogin: null, + headRepositorySshUrl: null, + headRepositoryUrl: null, + isCrossRepository: false, + headRefKind: "pull-ref", + }); + + const intent = await resolveWorktreeCreationIntent( + { action: "checkout", githubPrNumber: 42 }, + repoRoot, + deps, + ); + + expect(intent).not.toHaveProperty("trackOriginHead"); + }); + test("configures the contributor remote for fork PR targets", async () => { const deps = createResolverHarness(); deps.forgeService.getPullRequestCheckoutTarget = async () => ({ @@ -211,6 +228,29 @@ describe("resolveWorktreeCreationIntent", () => { expect(deps.headRefLookups).toEqual([]); }); + test("prefixes the pr number when the fork was deleted", async () => { + const deps = createResolverHarness(); + deps.forgeService.getPullRequestCheckoutTarget = async () => ({ + number: 42, + baseRefName: "main", + headRefName: "towel", + headOwnerLogin: null, + headRepositorySshUrl: null, + headRepositoryUrl: null, + isCrossRepository: true, + }); + + const intent = await resolveWorktreeCreationIntent( + { action: "checkout", githubPrNumber: 42 }, + repoRoot, + deps, + ); + + expect(intent).toMatchObject({ headRef: "towel", localBranchName: "pr-42/towel" }); + expect(intent).not.toHaveProperty("pushRemoteUrl"); + expect(intent).not.toHaveProperty("trackOriginHead"); + }); + test("uses an explicit PR head ref without calling GitHub", async () => { const deps = createResolverHarness(); @@ -293,6 +333,105 @@ describe("resolveWorktreeCreationIntent", () => { expect(deps.headRefLookups).toEqual([]); }); + test("names the local branch pr- for a gitea pull-ref-only head", async () => { + const deps = createResolverHarness({ + forge: "gitea", + forgeService: { + defaultCheckoutRefs: ({ changeRequestNumber }) => [ + { remoteName: "origin", remoteRef: `refs/pull/${changeRequestNumber}/head` }, + { remoteName: "upstream", remoteRef: `refs/pull/${changeRequestNumber}/head` }, + ], + buildPrLocalBranchName: ({ headRef, checkoutTarget }) => + buildForkLocalBranchName({ headRef, ...checkoutTarget }), + getPullRequestCheckoutTarget: async ({ number }) => ({ + number, + baseRefName: "main", + headRefName: `refs/pull/${number}/head`, + headOwnerLogin: null, + headRepositorySshUrl: null, + headRepositoryUrl: null, + isCrossRepository: false, + headRefKind: "pull-ref", + }), + }, + }); + + const intent = await resolveWorktreeCreationIntent( + { action: "checkout", checkoutSource: { kind: "change_request", number: 3 } }, + repoRoot, + deps, + ); + + expect(intent).toMatchObject({ localBranchName: "pr-3" }); + expect(intent).not.toHaveProperty("trackOriginHead"); + }); + + test("names the local branch owner/pr- for a gitea fork pull-ref-only head", async () => { + const deps = createResolverHarness({ + forge: "gitea", + forgeService: { + defaultCheckoutRefs: ({ changeRequestNumber }) => [ + { remoteName: "origin", remoteRef: `refs/pull/${changeRequestNumber}/head` }, + { remoteName: "upstream", remoteRef: `refs/pull/${changeRequestNumber}/head` }, + ], + buildPrLocalBranchName: ({ headRef, checkoutTarget }) => + buildForkLocalBranchName({ headRef, ...checkoutTarget }), + supportsCrossRepoCheckoutWithoutRefs: true, + getPullRequestCheckoutTarget: async ({ number }) => ({ + number, + baseRefName: "main", + headRefName: `refs/pull/${number}/head`, + headOwnerLogin: "contributor", + headRepositorySshUrl: "git@gitea.test:contributor/repo.git", + headRepositoryUrl: "https://gitea.test/contributor/repo", + isCrossRepository: true, + headRefKind: "pull-ref", + }), + }, + }); + + const intent = await resolveWorktreeCreationIntent( + { action: "checkout", checkoutSource: { kind: "change_request", number: 3 } }, + repoRoot, + deps, + ); + + expect(intent).toMatchObject({ localBranchName: "contributor/pr-3" }); + }); + + test("does not double the pr number for a gitea deleted fork's pull-ref-only head", async () => { + const deps = createResolverHarness({ + forge: "gitea", + forgeService: { + defaultCheckoutRefs: ({ changeRequestNumber }) => [ + { remoteName: "origin", remoteRef: `refs/pull/${changeRequestNumber}/head` }, + { remoteName: "upstream", remoteRef: `refs/pull/${changeRequestNumber}/head` }, + ], + buildPrLocalBranchName: ({ headRef, checkoutTarget }) => + buildForkLocalBranchName({ headRef, ...checkoutTarget }), + supportsCrossRepoCheckoutWithoutRefs: true, + getPullRequestCheckoutTarget: async ({ number }) => ({ + number, + baseRefName: "main", + headRefName: `refs/pull/${number}/head`, + headOwnerLogin: null, + headRepositorySshUrl: null, + headRepositoryUrl: null, + isCrossRepository: true, + headRefKind: "pull-ref", + }), + }, + }); + + const intent = await resolveWorktreeCreationIntent( + { action: "checkout", checkoutSource: { kind: "change_request", number: 3 } }, + repoRoot, + deps, + ); + + expect(intent).toMatchObject({ localBranchName: "pr-3" }); + }); + test("reports unsupported cross-repository checkout targets without a checkout ref", async () => { const deps = createResolverHarness({ forge: "gitea", diff --git a/packages/server/src/server/resolve-worktree-creation-intent.ts b/packages/server/src/server/resolve-worktree-creation-intent.ts index 73403f5908..5eddc0ef7b 100644 --- a/packages/server/src/server/resolve-worktree-creation-intent.ts +++ b/packages/server/src/server/resolve-worktree-creation-intent.ts @@ -191,7 +191,8 @@ async function resolvePrCheckoutIntent( }) ?? [{ remoteName: "origin", remoteRef: `refs/heads/${headRef}` }]; const localBranchName = service.buildPrLocalBranchName?.({ headRef, checkoutTarget }); const crossRepository = resolveCrossRepositoryFields(checkoutTarget); - const trackOriginHead = !checkoutTarget.isCrossRepository; + const trackOriginHead = + !checkoutTarget.isCrossRepository && checkoutTarget.headRefKind !== "pull-ref"; return { kind: "checkout-change-request", diff --git a/packages/server/src/services/forge-service.ts b/packages/server/src/services/forge-service.ts index fba9833cd8..8499cc8756 100644 --- a/packages/server/src/services/forge-service.ts +++ b/packages/server/src/services/forge-service.ts @@ -39,6 +39,8 @@ export interface PullRequestCheckoutTarget { headRepositorySshUrl: string | null; headRepositoryUrl: string | null; isCrossRepository: boolean; + /** "pull-ref" when headRefName is a synthetic pull ref, not a real branch name (agit PRs, deleted branches); undefined means "branch" */ + headRefKind?: "branch" | "pull-ref"; } export interface PullRequestCheckoutRef { @@ -441,9 +443,9 @@ export interface ForgeService { getPullRequestCheckoutTarget(options: GetPullRequestOptions): Promise; /** * Refs to fetch for a change-request checkout when the resolved checkout - * target carries none. Adapters that expose a universal change-request head - * ref (GitHub's refs/pull/N/head) return it here; others let the shell fall - * back to the head branch. + * target carries none. Adapters with a universal change-request head ref + * (refs/pull/N/head) return it here; others let the shell fall back to the + * head branch. */ defaultCheckoutRefs?(params: { changeRequestNumber: number; @@ -451,7 +453,7 @@ export interface ForgeService { }): PullRequestCheckoutRef[]; /** * Local branch name for a checked-out change request when the adapter - * disambiguates cross-repository heads (GitHub prefixes the fork owner). + * disambiguates cross-repository heads (prefixes the fork owner). * Returns undefined to keep the head ref name as-is. */ buildPrLocalBranchName?(params: { diff --git a/packages/server/src/services/gitea-service.test.ts b/packages/server/src/services/gitea-service.test.ts index 768f1a7f05..9e3a3ecc62 100644 --- a/packages/server/src/services/gitea-service.test.ts +++ b/packages/server/src/services/gitea-service.test.ts @@ -2297,11 +2297,13 @@ describe("createGiteaService", () => { checkoutRefs: [ { remoteName: "origin", remoteRef: "refs/pull/5/head" }, { remoteName: "origin", remoteRef: "refs/heads/feat/sample-change" }, + { remoteName: "upstream", remoteRef: "refs/pull/5/head" }, ], headOwnerLogin: null, headRepositorySshUrl: null, headRepositoryUrl: null, isCrossRepository: false, + headRefKind: "branch", }); }); @@ -2332,17 +2334,215 @@ describe("createGiteaService", () => { headRefName: "feat/sample-change", checkoutRefs: [ { remoteName: "origin", remoteRef: "refs/pull/5/head" }, - { remoteName: "origin", remoteRef: "refs/heads/feat/sample-change" }, + { remoteName: "upstream", remoteRef: "refs/pull/5/head" }, ], headOwnerLogin: "contributor", headRepositorySshUrl: "git@gitea.com:contributor/sample-repo.git", headRepositoryUrl: "https://gitea.com/contributor/sample-repo", isCrossRepository: true, + headRefKind: "branch", }); expect(calls).toContainEqual(["api", "repos/example-user/sample-repo/pulls/5"]); }); + it("skips the origin branch fallback and tracking for an AGit pull request", async () => { + const { service } = makeService((args) => + args[0] === "api" + ? ok( + JSON.stringify({ + flow: 1, + head: { ref: "refs/pull/5/head", repo: { id: 1 } }, + base: { repo: { id: 1 } }, + }), + ) + : ok(JSON.stringify(STATUS_PR_VIEW)), + ); + + const target = await service.getPullRequestCheckoutTarget({ cwd: "/repo", number: 5 }); + + expect(target).toMatchObject({ + checkoutRefs: [ + { remoteName: "origin", remoteRef: "refs/pull/5/head" }, + { remoteName: "upstream", remoteRef: "refs/pull/5/head" }, + ], + isCrossRepository: false, + headRefKind: "pull-ref", + }); + }); + + it("names the local branch pr- end to end for an agit pull request", async () => { + // the tea CLI reports head as the raw pull ref too when there's no real + // branch, same as the Forgejo API's head.ref + const { service } = makeService((args) => + args[0] === "api" + ? ok( + JSON.stringify({ + flow: 1, + head: { ref: "refs/pull/5/head", repo: { id: 1 } }, + base: { repo: { id: 1 } }, + }), + ) + : ok(JSON.stringify({ ...STATUS_PR_VIEW, head: "refs/pull/5/head" })), + ); + + const target = await service.getPullRequestCheckoutTarget({ cwd: "/repo", number: 5 }); + const localBranchName = service.buildPrLocalBranchName?.({ + headRef: target.headRefName, + checkoutTarget: target, + }); + + expect(localBranchName).toBe("pr-5"); + }); + + it("skips the origin branch fallback when the head branch was deleted", async () => { + const { service } = makeService((args) => + args[0] === "api" + ? ok( + JSON.stringify({ + head: { ref: "refs/pull/5/head", repo: { id: 1 } }, + base: { repo: { id: 1 } }, + }), + ) + : ok(JSON.stringify(STATUS_PR_VIEW)), + ); + + const target = await service.getPullRequestCheckoutTarget({ cwd: "/repo", number: 5 }); + + expect(target).toMatchObject({ + checkoutRefs: [ + { remoteName: "origin", remoteRef: "refs/pull/5/head" }, + { remoteName: "upstream", remoteRef: "refs/pull/5/head" }, + ], + isCrossRepository: false, + headRefKind: "pull-ref", + }); + }); + + it("keeps a pull request from a deleted fork cross-repository", async () => { + const { service } = makeService((args) => + args[0] === "api" + ? ok(JSON.stringify({ head: { repo: null }, base: { repo: { id: 1 } } })) + : ok(JSON.stringify(STATUS_PR_VIEW)), + ); + + const target = await service.getPullRequestCheckoutTarget({ cwd: "/repo", number: 5 }); + + expect(target).toMatchObject({ + checkoutRefs: [ + { remoteName: "origin", remoteRef: "refs/pull/5/head" }, + { remoteName: "upstream", remoteRef: "refs/pull/5/head" }, + ], + headOwnerLogin: null, + isCrossRepository: true, + headRefKind: "branch", + }); + }); + + it("keeps the origin branch fallback when the server sends no head ref", async () => { + const { service } = makeService((args) => + args[0] === "api" + ? ok(JSON.stringify({ head: { repo: { id: 1 } }, base: { repo: { id: 1 } } })) + : ok(JSON.stringify(STATUS_PR_VIEW)), + ); + + const target = await service.getPullRequestCheckoutTarget({ cwd: "/repo", number: 5 }); + + expect(target.checkoutRefs).toContainEqual({ + remoteName: "origin", + remoteRef: "refs/heads/feat/sample-change", + }); + expect(target.headRefKind).toBe("branch"); + }); + + it("prefixes the local branch name with the fork owner for a cross-repository checkout", () => { + const { service } = makeService(() => ok("")); + + const localBranchName = service.buildPrLocalBranchName?.({ + headRef: "patch-1", + checkoutTarget: { + number: 5, + baseRefName: "main", + headRefName: "patch-1", + headOwnerLogin: "contributor", + headRepositorySshUrl: null, + headRepositoryUrl: null, + isCrossRepository: true, + }, + }); + + expect(localBranchName).toBe("contributor/patch-1"); + }); + + it("names the local branch pr- for a same-repo pull-ref-only head", () => { + const { service } = makeService(() => ok("")); + + const localBranchName = service.buildPrLocalBranchName?.({ + headRef: "refs/pull/5/head", + checkoutTarget: { + number: 5, + baseRefName: "main", + headRefName: "refs/pull/5/head", + headOwnerLogin: null, + headRepositorySshUrl: null, + headRepositoryUrl: null, + isCrossRepository: false, + headRefKind: "pull-ref", + }, + }); + + expect(localBranchName).toBe("pr-5"); + }); + + it("prefixes pr- with the fork owner for a pull-ref-only head", () => { + const { service } = makeService(() => ok("")); + + const localBranchName = service.buildPrLocalBranchName?.({ + headRef: "refs/pull/5/head", + checkoutTarget: { + number: 5, + baseRefName: "main", + headRefName: "refs/pull/5/head", + headOwnerLogin: "contributor", + headRepositorySshUrl: null, + headRepositoryUrl: null, + isCrossRepository: true, + headRefKind: "pull-ref", + }, + }); + + expect(localBranchName).toBe("contributor/pr-5"); + }); + + it("does not double the pr number for a deleted fork's pull-ref-only head", () => { + const { service } = makeService(() => ok("")); + + const localBranchName = service.buildPrLocalBranchName?.({ + headRef: "refs/pull/5/head", + checkoutTarget: { + number: 5, + baseRefName: "main", + headRefName: "refs/pull/5/head", + headOwnerLogin: null, + headRepositorySshUrl: null, + headRepositoryUrl: null, + isCrossRepository: true, + headRefKind: "pull-ref", + }, + }); + + expect(localBranchName).toBe("pr-5"); + }); + + it("returns the pull head ref for default checkout refs", () => { + const { service } = makeService(() => ok("")); + + expect(service.defaultCheckoutRefs?.({ changeRequestNumber: 5, headRef: "patch-1" })).toEqual([ + { remoteName: "origin", remoteRef: "refs/pull/5/head" }, + { remoteName: "upstream", remoteRef: "refs/pull/5/head" }, + ]); + }); + it("creates a pull request and parses the resulting URL and index", async () => { const { service, calls } = makeService((args) => { if (args[0] === "pr" && args[1] === "create") { diff --git a/packages/server/src/services/gitea-service.ts b/packages/server/src/services/gitea-service.ts index 88b5caa004..892f9cc33a 100644 --- a/packages/server/src/services/gitea-service.ts +++ b/packages/server/src/services/gitea-service.ts @@ -5,6 +5,10 @@ import pLimit from "p-limit"; import { parseGitHubRemoteIdentity, parseGitRemoteLocation } from "@getpaseo/protocol/git-remote"; import { findExecutable } from "../executable-resolution/executable-resolution.js"; import { runGitCommand } from "../utils/run-git-command.js"; +import { + buildForkLocalBranchName, + buildPullHeadCheckoutRefs, +} from "../utils/change-request-checkout.js"; import { execCommand } from "../utils/spawn.js"; import { createCachedCliPathResolver, @@ -75,6 +79,8 @@ const CURRENT_PR_HEAD_SHA_CACHE_TTL_MS = 30_000; // the sweep to the 250 most recent Actions tasks. const GITEA_ACTION_PAGE_LIMIT = 50; const GITEA_ACTION_MAX_PAGES = 5; +// PullRequestFlowAGit in forgejo models/issues/pull.go +const GITEA_PULL_REQUEST_FLOW_AGIT = 1; /** * Fields requested from `tea pr list -o json`. tea's default field set omits the @@ -225,8 +231,12 @@ const GiteaPullRequestRepoSchema = z const GiteaPullRequestApiSchema = z .object({ + flow: z.number().optional(), head: z - .object({ repo: GiteaPullRequestRepoSchema.nullable().optional() }) + .object({ + ref: z.string().optional(), + repo: GiteaPullRequestRepoSchema.nullable().optional(), + }) .passthrough() .optional(), base: z @@ -1435,17 +1445,30 @@ export function createGiteaService(options: CreateGiteaServiceOptions = {}): For return runJsonParse(args, runOptions, stdout, schema); } + function resolveHeadRefKind( + pr: z.infer, + number: number, + ): "branch" | "pull-ref" { + // head.ref only becomes the branch name when the branch still exists, + // agit PRs and deleted branches keep the pull ref + const isPullRefOnly = + pr.flow === GITEA_PULL_REQUEST_FLOW_AGIT || pr.head?.ref === `refs/pull/${number}/head`; + return isPullRefOnly ? "pull-ref" : "branch"; + } + async function resolveForkCheckoutFacts( cwd: string, summary: PullRequestSummary, ): Promise<{ isCrossRepository: boolean; + headRefKind: "branch" | "pull-ref"; headOwnerLogin: string | null; headRepositorySshUrl: string | null; headRepositoryUrl: string | null; }> { const sameRepo = { isCrossRepository: false, + headRefKind: "branch" as const, headOwnerLogin: null, headRepositorySshUrl: null, headRepositoryUrl: null, @@ -1464,14 +1487,17 @@ export function createGiteaService(options: CreateGiteaServiceOptions = {}): For ); const headRepo = pr.head?.repo ?? null; const baseRepo = pr.base?.repo ?? null; - if (headRepo?.id == null || baseRepo?.id == null || headRepo.id === baseRepo.id) { - return sameRepo; + const headRefKind = resolveHeadRefKind(pr, summary.number); + if (baseRepo?.id == null || headRepo?.id === baseRepo.id) { + return { ...sameRepo, headRefKind }; } + // headRepo is null when the fork was deleted return { isCrossRepository: true, - headOwnerLogin: headRepo.owner?.login ?? null, - headRepositorySshUrl: headRepo.ssh_url ?? null, - headRepositoryUrl: headRepo.html_url ?? null, + headRefKind, + headOwnerLogin: headRepo?.owner?.login ?? null, + headRepositorySshUrl: headRepo?.ssh_url ?? null, + headRepositoryUrl: headRepo?.html_url ?? null, }; } @@ -1997,17 +2023,29 @@ export function createGiteaService(options: CreateGiteaServiceOptions = {}): For return summary.headRefName; }, + defaultCheckoutRefs({ changeRequestNumber }) { + return buildPullHeadCheckoutRefs(changeRequestNumber); + }, + + buildPrLocalBranchName({ headRef, checkoutTarget }) { + return buildForkLocalBranchName({ headRef, ...checkoutTarget }); + }, + async getPullRequestCheckoutTarget( input: GetPullRequestOptions, ): Promise { const summary = await this.getPullRequest(input); - const checkoutRefs = [ - { remoteName: "origin", remoteRef: `refs/pull/${summary.number}/head` }, - ]; - if (summary.headRefName) { + const fork = await resolveForkCheckoutFacts(input.cwd, summary); + const [originPullRef, upstreamPullRef] = buildPullHeadCheckoutRefs(summary.number); + const checkoutRefs = [originPullRef]; + // for a fork PR a branch with that name on origin belongs to the base repo + const headBranchOnOrigin = + !fork.isCrossRepository && fork.headRefKind === "branch" && Boolean(summary.headRefName); + if (headBranchOnOrigin) { checkoutRefs.push({ remoteName: "origin", remoteRef: `refs/heads/${summary.headRefName}` }); } - const fork = await resolveForkCheckoutFacts(input.cwd, summary); + // upstream can hold a different PR with the same number, so it goes last + checkoutRefs.push(upstreamPullRef); return { number: summary.number, baseRefName: summary.baseRefName, @@ -2017,6 +2055,7 @@ export function createGiteaService(options: CreateGiteaServiceOptions = {}): For headRepositorySshUrl: fork.headRepositorySshUrl, headRepositoryUrl: fork.headRepositoryUrl, isCrossRepository: fork.isCrossRepository, + headRefKind: fork.headRefKind, }; }, diff --git a/packages/server/src/services/github-service.test.ts b/packages/server/src/services/github-service.test.ts index ab71115f36..3109bcc63c 100644 --- a/packages/server/src/services/github-service.test.ts +++ b/packages/server/src/services/github-service.test.ts @@ -797,6 +797,7 @@ describe("ForgeService", () => { headRepositorySshUrl: "git@github.com:therainisme/paseo.git", headRepositoryUrl: "https://github.com/therainisme/paseo", isCrossRepository: true, + headRefKind: "branch", }); expect(runner.calls).toHaveLength(2); diff --git a/packages/server/src/services/github-service.ts b/packages/server/src/services/github-service.ts index b6de0cbd6e..2dd3fdc269 100644 --- a/packages/server/src/services/github-service.ts +++ b/packages/server/src/services/github-service.ts @@ -6,6 +6,10 @@ import { } from "@getpaseo/protocol/git-remote"; import { findExecutable } from "../executable-resolution/executable-resolution.js"; import { runGitCommand } from "../utils/run-git-command.js"; +import { + buildForkLocalBranchName, + buildPullHeadCheckoutRefs, +} from "../utils/change-request-checkout.js"; import { execCommand } from "../utils/spawn.js"; import { resolveSshHostname } from "../utils/ssh-hostname.js"; import { @@ -2104,17 +2108,11 @@ export function createGitHubService(options: CreateGitHubServiceOptions = {}): G }, defaultCheckoutRefs({ changeRequestNumber }) { - return [ - { remoteName: "origin", remoteRef: `refs/pull/${changeRequestNumber}/head` }, - { remoteName: "upstream", remoteRef: `refs/pull/${changeRequestNumber}/head` }, - ]; + return buildPullHeadCheckoutRefs(changeRequestNumber); }, buildPrLocalBranchName({ headRef, checkoutTarget }) { - const owner = checkoutTarget.isCrossRepository - ? normalizeGitHubOwnerForBranch(checkoutTarget.headOwnerLogin) - : null; - return owner ? `${owner}/${headRef}` : headRef; + return buildForkLocalBranchName({ headRef, ...checkoutTarget }); }, supportsCrossRepoCheckoutWithoutRefs: true, @@ -2663,11 +2661,6 @@ export function createGitHubService(options: CreateGitHubServiceOptions = {}): G return api; } -function normalizeGitHubOwnerForBranch(owner: string | null): string | null { - const normalized = owner?.trim().toLowerCase() ?? ""; - return /^[a-z0-9-]+$/.test(normalized) ? normalized : null; -} - function getGithubStatusFacts( status: PullRequestCommandStatus | null | undefined, ): GitHubPullRequestStatusFacts | null { @@ -3529,6 +3522,7 @@ function toPullRequestCheckoutTarget( headRepositorySshUrl: pullRequest.headRepository?.sshUrl || null, headRepositoryUrl: pullRequest.headRepository?.url || null, isCrossRepository: pullRequest.isCrossRepository, + headRefKind: "branch", }; } diff --git a/packages/server/src/services/gitlab-service.test.ts b/packages/server/src/services/gitlab-service.test.ts index 371198e447..b9b6d447d4 100644 --- a/packages/server/src/services/gitlab-service.test.ts +++ b/packages/server/src/services/gitlab-service.test.ts @@ -477,6 +477,7 @@ describe("createGitLabService", () => { headRepositorySshUrl: null, headRepositoryUrl: null, isCrossRepository: false, + headRefKind: "branch", }); expect(calls[0]).toEqual(["mr", "view", "14", "-F", "json"]); }); diff --git a/packages/server/src/services/gitlab-service.ts b/packages/server/src/services/gitlab-service.ts index 1fff0e2a02..c5f54be2cb 100644 --- a/packages/server/src/services/gitlab-service.ts +++ b/packages/server/src/services/gitlab-service.ts @@ -1204,6 +1204,7 @@ export function createGitLabService(options: CreateGitLabServiceOptions = {}): F mr.target_project_id !== undefined && mr.target_project_id !== null && mr.source_project_id !== mr.target_project_id, + headRefKind: "branch", }; }, diff --git a/packages/server/src/utils/change-request-checkout.test.ts b/packages/server/src/utils/change-request-checkout.test.ts new file mode 100644 index 0000000000..74a57c2445 --- /dev/null +++ b/packages/server/src/utils/change-request-checkout.test.ts @@ -0,0 +1,89 @@ +import { describe, expect, it } from "vitest"; + +import { + buildForkLocalBranchName, + normalizeForgeOwnerForBranch, +} from "./change-request-checkout.js"; + +describe("normalizeForgeOwnerForBranch", () => { + it("lowercases a valid login", () => { + expect(normalizeForgeOwnerForBranch("Contributor")).toBe("contributor"); + }); + + it("allows dots and underscores", () => { + expect(normalizeForgeOwnerForBranch("a.b_c")).toBe("a.b_c"); + }); + + it("rejects a login starting with a dash", () => { + expect(normalizeForgeOwnerForBranch("-abc")).toBeNull(); + }); + + it("rejects a login starting with a dot", () => { + expect(normalizeForgeOwnerForBranch(".hidden")).toBeNull(); + }); + + it("rejects a login containing '..'", () => { + expect(normalizeForgeOwnerForBranch("a..b")).toBeNull(); + }); + + it("rejects a login ending in .lock", () => { + expect(normalizeForgeOwnerForBranch("contributor.lock")).toBeNull(); + }); + + it("rejects a login with a path traversal segment", () => { + expect(normalizeForgeOwnerForBranch("../evil")).toBeNull(); + }); + + it("rejects an empty or null login", () => { + expect(normalizeForgeOwnerForBranch(null)).toBeNull(); + expect(normalizeForgeOwnerForBranch("")).toBeNull(); + expect(normalizeForgeOwnerForBranch(" ")).toBeNull(); + }); +}); + +describe("buildForkLocalBranchName", () => { + const fork = { headRef: "patch-1", number: 42, isCrossRepository: true }; + + it("keeps the head ref for a same-repository head", () => { + expect( + buildForkLocalBranchName({ ...fork, isCrossRepository: false, headOwnerLogin: "arthur" }), + ).toBe("patch-1"); + }); + + it("prefixes a fork head with the lowercased owner", () => { + expect(buildForkLocalBranchName({ ...fork, headOwnerLogin: "Arthur" })).toBe("arthur/patch-1"); + }); + + it("falls back to the pr number when the owner is unknown", () => { + expect(buildForkLocalBranchName({ ...fork, headOwnerLogin: null })).toBe("pr-42/patch-1"); + }); + + it("falls back to the pr number when the owner is not a safe ref segment", () => { + expect(buildForkLocalBranchName({ ...fork, headOwnerLogin: "-zaphod" })).toBe("pr-42/patch-1"); + }); + + // pull-ref-only heads (agit PRs, deleted branches) report headRef as the + // literal pull ref, e.g. refs/pull/42/head, not a real branch name + it("collapses a same-repository pull-ref-only head to pr-", () => { + expect( + buildForkLocalBranchName({ + ...fork, + isCrossRepository: false, + headOwnerLogin: "arthur", + headRefKind: "pull-ref", + }), + ).toBe("pr-42"); + }); + + it("prefixes pr- with the owner for a fork pull-ref-only head", () => { + expect( + buildForkLocalBranchName({ ...fork, headOwnerLogin: "Arthur", headRefKind: "pull-ref" }), + ).toBe("arthur/pr-42"); + }); + + it("does not double the pr number when the fork owner is unknown and there is no head branch", () => { + expect( + buildForkLocalBranchName({ ...fork, headOwnerLogin: null, headRefKind: "pull-ref" }), + ).toBe("pr-42"); + }); +}); diff --git a/packages/server/src/utils/change-request-checkout.ts b/packages/server/src/utils/change-request-checkout.ts new file mode 100644 index 0000000000..12c779571b --- /dev/null +++ b/packages/server/src/utils/change-request-checkout.ts @@ -0,0 +1,47 @@ +import type { PullRequestCheckoutRef } from "../services/forge-service.js"; + +// upstream for clones where origin is your fork +export function buildPullHeadCheckoutRefs(number: number): PullRequestCheckoutRef[] { + return [ + { remoteName: "origin", remoteRef: `refs/pull/${number}/head` }, + { remoteName: "upstream", remoteRef: `refs/pull/${number}/head` }, + ]; +} + +// forgejo username pattern, covers github logins too +export function normalizeForgeOwnerForBranch(owner: string | null): string | null { + const normalized = owner?.trim().toLowerCase() ?? ""; + if (!/^[a-z0-9][a-z0-9_.-]*$/.test(normalized)) { + return null; + } + // git refuses .. and .lock, trailing dot too + if (normalized.includes("..") || normalized.endsWith(".") || normalized.endsWith(".lock")) { + return null; + } + return normalized; +} + +export interface ForkLocalBranchNameInput { + headRef: string; + number: number; + isCrossRepository: boolean; + headOwnerLogin: string | null; + /** "pull-ref" when headRef is a synthetic pull ref, not a real branch name (agit PRs, deleted branches); undefined means "branch" */ + headRefKind?: "branch" | "pull-ref"; +} + +export function buildForkLocalBranchName(params: ForkLocalBranchNameInput): string { + const pullRefFallback = `pr-${params.number}`; + // headRef is the literal pull ref (e.g. refs/pull/42/head) when there is no + // real branch to name after, so use the pr number instead + const headSegment = params.headRefKind === "pull-ref" ? pullRefFallback : params.headRef; + if (!params.isCrossRepository) { + return headSegment; + } + const owner = normalizeForgeOwnerForBranch(params.headOwnerLogin); + if (!owner) { + // don't double up pr-/pr- when the owner is also unknown + return headSegment === pullRefFallback ? headSegment : `${pullRefFallback}/${headSegment}`; + } + return `${owner}/${headSegment}`; +} diff --git a/packages/server/src/utils/worktree.test.ts b/packages/server/src/utils/worktree.test.ts index 9b0cba2643..82505dc800 100644 --- a/packages/server/src/utils/worktree.test.ts +++ b/packages/server/src/utils/worktree.test.ts @@ -16,6 +16,7 @@ import { mkdirSync, rmSync, existsSync, + readFileSync, realpathSync, symlinkSync, writeFileSync, @@ -94,6 +95,64 @@ describe("paseo worktree manager", () => { await rm(tempDir, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }); }); + it.each([ + { existing: [], preferred: "main/patch-1", expected: "main-1/patch-1" }, + { existing: ["main-1"], preferred: "main/patch-1", expected: "main-2/patch-1" }, + { + existing: ["main-1/patch-1"], + preferred: "main/patch-1", + expected: "main-1/patch-1-2", + }, + { + existing: ["bob/patch-1/child"], + preferred: "bob/patch-1", + expected: "bob/patch-1-1", + }, + ])( + "checks out a fork when $preferred conflicts with existing branch paths", + async ({ existing, preferred, expected }) => { + const git = (...args: string[]) => + execFileSync("git", args, { cwd: repoDir, encoding: "utf8", stdio: "pipe" }).trim(); + git("config", "core.autocrlf", "false"); + const baseHead = git("rev-parse", "main"); + for (const branch of existing) git("branch", branch); + git("checkout", "-b", "fixture-pr"); + writeFileSync(join(repoDir, "file.txt"), "from-pr\n"); + git("add", "file.txt"); + git("-c", "commit.gpgsign=false", "commit", "-m", "fork patch"); + const prHead = git("rev-parse", "HEAD"); + git("checkout", "main"); + const remote = join(tempDir, "origin.git"); + git("clone", "--bare", repoDir, remote); + git("remote", "add", "origin", remote); + git(`--git-dir=${remote}`, "update-ref", "refs/pull/1/head", prHead); + git("pack-refs", "--all"); + + const result = await createWorktreePrimitive({ + cwd: repoDir, + paseoHome, + worktreeSlug: "fork-review", + runSetup: false, + source: { + kind: "checkout-change-request", + forge: "gitea", + changeRequestNumber: 1, + headRef: "patch-1", + localBranchName: preferred, + baseRefName: "main", + checkoutRefs: [{ remoteName: "origin", remoteRef: "refs/pull/1/head" }], + }, + }); + + expect(result.branchName).toBe(expected); + expect(git("rev-parse", result.branchName)).toBe(prHead); + expect(readFileSync(join(result.worktreePath, "file.txt"), "utf8")).toBe("from-pr\n"); + for (const branch of ["main", ...existing]) { + expect(git("rev-parse", branch)).toBe(baseHead); + } + }, + ); + it("treats a worktree as paseo-owned even when its .git admin is missing", async () => { const created = await createLegacyWorktreeForTest({ branchName: "orphan-admin-branch", diff --git a/packages/server/src/utils/worktree.ts b/packages/server/src/utils/worktree.ts index f454a7225a..39b64d8137 100644 --- a/packages/server/src/utils/worktree.ts +++ b/packages/server/src/utils/worktree.ts @@ -1778,13 +1778,30 @@ async function localBranchExists(cwd: string, branchName: string): Promise { - let newBranchName = candidateBranch; + const { stdout } = await runGitCommand( + ["for-each-ref", "--format=%(refname:strip=2)", "refs/heads/"], + { cwd, envOverlay: READ_ONLY_GIT_ENV }, + ); + const existingBranches = new Set(stdout.trim().split("\n").filter(Boolean)); + const candidateSegments = candidateBranch.split("/"); + const segments = [...candidateSegments]; let suffix = 1; - while (await localBranchExists(cwd, newBranchName)) { - newBranchName = `${candidateBranch}-${suffix}`; - suffix++; + while (true) { + const branchName = segments.join("/"); + const parentConflict = segments.findIndex((_, index) => + existingBranches.has(segments.slice(0, index + 1).join("/")), + ); + const childConflict = [...existingBranches].some((branch) => + branch.startsWith(`${branchName}/`), + ); + if (parentConflict === -1 && !childConflict) { + return branchName; + } + // Git refs form a namespace: a branch cannot also be another branch's + // parent directory. Rename the conflicting segment, preserving the rest. + const conflictIndex = parentConflict === -1 ? segments.length - 1 : parentConflict; + segments[conflictIndex] = `${candidateSegments[conflictIndex]}-${suffix++}`; } - return newBranchName; } async function isBranchCheckedOut(cwd: string, branchName: string): Promise {