From 4aaaed2153017caa308e1a559347a054e558aee5 Mon Sep 17 00:00:00 2001 From: Nicolas Medda Date: Wed, 22 Apr 2026 09:32:50 +0000 Subject: [PATCH] BBC2-13 add bb pr checkout (same-repo PRs only) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fetches the PR's source branch via `git fetch origin `, then either creates a local tracking branch or fast-forwards an existing one. Fork-based PRs are detected by comparing source vs destination repository.full_name on the PR response and rejected with a clear error — full fork support is out of scope for this ticket. Extends the GitRunner interface with fetchRef, hasLocalBranch, checkout (create-tracking / existing), and mergeFastForwardOnly primitives. Writes throw a new GitError whose message carries git's stderr, so the user sees git's own diagnostics for dirty working trees, diverged branches, etc. Extends PullRequestDetail with sourceRepositoryFullName and destinationRepositoryFullName so the fork check reads from the same typed response rather than a separate call. --- src/backend/pullrequests/index.test.ts | 58 +++++++- src/backend/pullrequests/index.ts | 12 ++ src/commands/pullrequest/checkout.ts | 118 ++++++++++++++++ src/commands/pullrequest/index.ts | 12 ++ src/shared/repository/git.integration.test.ts | 128 +++++++++++++++++- src/shared/repository/git.ts | 95 +++++++++++++ src/shared/repository/index.ts | 2 +- src/shared/repository/resolve.test.ts | 40 +++++- 8 files changed, 454 insertions(+), 11 deletions(-) create mode 100644 src/commands/pullrequest/checkout.ts diff --git a/src/backend/pullrequests/index.test.ts b/src/backend/pullrequests/index.test.ts index e30b5f4..11b4a15 100644 --- a/src/backend/pullrequests/index.test.ts +++ b/src/backend/pullrequests/index.test.ts @@ -55,8 +55,14 @@ function makePrDetail( summary: { raw: "A detailed PR description.\n\n- fix thing\n- fix other thing", }, - source: { branch: { name: "feature/auth" } }, - destination: { branch: { name: "main" } }, + source: { + branch: { name: "feature/auth" }, + repository: { full_name: "ws/repo" }, + }, + destination: { + branch: { name: "main" }, + repository: { full_name: "ws/repo" }, + }, links: { html: { href: "https://bitbucket.org/ws/repo/pull-requests/42" } }, participants: [ { @@ -380,6 +386,8 @@ describe("getPullRequest", () => { "A detailed PR description.\n\n- fix thing\n- fix other thing", sourceBranch: "feature/auth", destinationBranch: "main", + sourceRepositoryFullName: "ws/repo", + destinationRepositoryFullName: "ws/repo", reviewers: [ { account: { uuid: "{bob}", displayName: "Bob", nickname: "bob" }, @@ -415,6 +423,52 @@ describe("getPullRequest", () => { expect((err as PullRequestError).status).toBe(404); }); + test("surfaces source vs destination repository full_name for fork detection", async () => { + // Fork-based PRs carry a different `source.repository.full_name` from + // the destination. Consumers (bb pr checkout) rely on this distinction + // to refuse fork checkouts cleanly. + server.use( + http.get(PR_DETAIL_PATH(42), () => + HttpResponse.json( + makePrDetail({ + source: { + branch: { name: "patch-1" }, + repository: { full_name: "fork-ws/repo" }, + }, + destination: { + branch: { name: "main" }, + repository: { full_name: "ws/repo" }, + }, + }), + ), + ), + ); + + const result = await getPullRequest(creds, ref, 42); + expect(result.sourceRepositoryFullName).toBe("fork-ws/repo"); + expect(result.destinationRepositoryFullName).toBe("ws/repo"); + }); + + test("falls back to empty strings when repository metadata is missing", async () => { + // Bitbucket occasionally omits the repository reference on same-repo + // PRs. Treating the fields as optional (empty) keeps `bb pr checkout`'s + // fork check from producing false positives. + server.use( + http.get(PR_DETAIL_PATH(42), () => + HttpResponse.json( + makePrDetail({ + source: { branch: { name: "feature/auth" } }, + destination: { branch: { name: "main" } }, + }), + ), + ), + ); + + const result = await getPullRequest(creds, ref, 42); + expect(result.sourceRepositoryFullName).toBe(""); + expect(result.destinationRepositoryFullName).toBe(""); + }); + test("handles a PR with no participants", async () => { server.use( http.get(PR_DETAIL_PATH(42), () => diff --git a/src/backend/pullrequests/index.ts b/src/backend/pullrequests/index.ts index 0d7236f..3e54b04 100644 --- a/src/backend/pullrequests/index.ts +++ b/src/backend/pullrequests/index.ts @@ -65,6 +65,14 @@ export type PullRequestDetail = PullRequest & { description: string; sourceBranch: string; destinationBranch: string; + /** + * `workspace/slug` of the source repo. Differs from the destination when + * the PR is fork-based. Consumers that care about cross-repo semantics + * (e.g. `bb pr checkout`, which only supports same-repo PRs) compare + * against `destinationRepositoryFullName`. + */ + sourceRepositoryFullName: string; + destinationRepositoryFullName: string; reviewers: Reviewer[]; participants: Participant[]; }; @@ -615,6 +623,10 @@ function toPullRequestDetail(pr: RawPullRequest): PullRequestDetail { description: String(raw.summary?.raw ?? raw.description ?? ""), sourceBranch: String(raw.source?.branch?.name ?? ""), destinationBranch: String(raw.destination?.branch?.name ?? ""), + sourceRepositoryFullName: String(raw.source?.repository?.full_name ?? ""), + destinationRepositoryFullName: String( + raw.destination?.repository?.full_name ?? "", + ), reviewers: toReviewers(raw.participants), participants: toParticipants(raw.participants), }; diff --git a/src/commands/pullrequest/checkout.ts b/src/commands/pullrequest/checkout.ts new file mode 100644 index 0000000..94330bd --- /dev/null +++ b/src/commands/pullrequest/checkout.ts @@ -0,0 +1,118 @@ +import { + getPullRequest, + type PullRequestDetail, + PullRequestError, +} from "../../backend/pullrequests/index.ts"; +import { loadConfigOrExit } from "../../shared/config/index.ts"; +import type { Renderer } from "../../shared/renderer/index.ts"; +import { + defaultGitRunner, + GitError, + type GitRunner, + RepositoryResolutionError, + resolveRepository, +} from "../../shared/repository/index.ts"; + +export type PullRequestCheckoutOptions = { + repository?: string; +}; + +/** + * Fetches the PR's source branch and either creates a local branch tracking + * it, or fast-forwards an existing local branch. Same-repo PRs only — fork + * checkouts are out of scope (and meaningfully harder: need a temporary + * remote, branch rename to avoid collisions, and cleanup semantics). + */ +export async function runPullRequestCheckout( + renderer: Renderer, + idArg: string, + options: PullRequestCheckoutOptions, + git: GitRunner = defaultGitRunner, +): Promise { + const id = parseId(idArg); + if (id === null) { + renderer.error(`Invalid PR id '${idArg}'. Expected a positive integer.`); + process.exit(1); + } + + const config = await loadConfigOrExit(renderer); + + try { + const ref = await resolveRepository({ override: options.repository }); + const pr = await getPullRequest(config, ref, id); + + ensureSameRepo(pr, ref); + + const branch = pr.sourceBranch; + if (!branch) { + renderer.error( + `Pull request #${id} has no source branch — nothing to check out.`, + ); + process.exit(1); + } + + const cwd = process.cwd(); + const remote = "origin"; + const remoteRef = `${remote}/${branch}`; + + await git.fetchRef(cwd, remote, branch); + + const existed = await git.hasLocalBranch(cwd, branch); + if (existed) { + await git.checkoutExistingBranch(cwd, branch); + await git.mergeFastForwardOnly(cwd, remoteRef); + } else { + await git.checkoutCreateTracking(cwd, branch, remoteRef); + } + + const action = existed ? "fast-forwarded" : "created"; + if (renderer.json) { + renderer.detail({ id, branch, action }, []); + return; + } + renderer.message( + existed + ? `Checked out pull request #${id}: ${branch} (fast-forwarded).` + : `Checked out pull request #${id}: ${branch}.`, + ); + } catch (err) { + if ( + err instanceof RepositoryResolutionError || + err instanceof PullRequestError + ) { + renderer.error(err.message); + process.exit(1); + } + if (err instanceof GitError) { + // Surface git's own stderr verbatim — it's almost always clearer + // than anything we could paraphrase (dirty working tree, diverged + // branches, missing remote ref, etc.). + renderer.error(err.message); + process.exit(1); + } + throw err; + } +} + +function ensureSameRepo( + pr: PullRequestDetail, + ref: { workspace: string; slug: string }, +): void { + const source = pr.sourceRepositoryFullName; + const destination = pr.destinationRepositoryFullName; + // Empty strings mean Bitbucket didn't send the field — treat as same-repo + // (the common case for non-fork PRs; the server sometimes omits the + // repository reference when source == destination). + if (!source || !destination) return; + if (source.toLowerCase() === destination.toLowerCase()) return; + throw new PullRequestError( + `Pull request source repo '${source}' differs from '${ref.workspace}/${ref.slug}'. ` + + "Fork-based PR checkout is not supported yet — open a follow-up if you need this.", + ); +} + +function parseId(raw: string): number | null { + const n = Number(raw); + if (!Number.isInteger(n) || n <= 0) return null; + return n; +} diff --git a/src/commands/pullrequest/index.ts b/src/commands/pullrequest/index.ts index eb4197f..c6ae62b 100644 --- a/src/commands/pullrequest/index.ts +++ b/src/commands/pullrequest/index.ts @@ -1,5 +1,6 @@ import type { Command } from "commander"; import { withRenderer } from "../../shared/renderer/commander.ts"; +import { runPullRequestCheckout } from "./checkout.ts"; import { runPullRequestComment } from "./comment.ts"; import { runPullRequestCreate } from "./create.ts"; import { runPullRequestDiff } from "./diff.ts"; @@ -86,6 +87,17 @@ export function registerPullRequestCommands(program: Command): void { ) .action(withRenderer(runPullRequestDiff)); + pr.command("checkout") + .description( + "Fetch and check out a pull request's source branch locally (same-repo PRs only)", + ) + .argument("", "Pull request number") + .option( + "-R, --repository ", + "Override repository detection", + ) + .action(withRenderer(runPullRequestCheckout)); + pr.command("review") .description( "Submit a review on a pull request (defaults to the PR for the current branch)", diff --git a/src/shared/repository/git.integration.test.ts b/src/shared/repository/git.integration.test.ts index b7b572b..5687d0a 100644 --- a/src/shared/repository/git.integration.test.ts +++ b/src/shared/repository/git.integration.test.ts @@ -3,7 +3,7 @@ import { mkdtemp, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { $ } from "bun"; -import { defaultGitRunner } from "./git.ts"; +import { defaultGitRunner, GitError } from "./git.ts"; import { RepositoryResolutionError, resolveRepository } from "./resolve.ts"; /** @@ -164,6 +164,132 @@ describe("defaultGitRunner against a local bare remote", () => { await defaultGitRunner.getDefaultBranchFromRemote(repoDir, "origin"), ).toBeUndefined(); }); + + test("fetchRef pulls a branch that was pushed to the remote after clone", async () => { + // Push a new branch to the bare remote from a side seed, then verify + // the clone can fetch it by name and ends up with the remote-tracking + // ref. + const sideDir = join(cloneDir, "..", "side"); + await $`git clone -q ${bareDir} ${sideDir}`.quiet(); + await $`git -C ${sideDir} -c user.email=s@b.co -c user.name=Side checkout -b feature/fetch-me`.quiet(); + await $`git -C ${sideDir} -c user.email=s@b.co -c user.name=Side commit --allow-empty -m "side work"`.quiet(); + await $`git -C ${sideDir} push -q origin feature/fetch-me`.quiet(); + + await defaultGitRunner.fetchRef(cloneDir, "origin", "feature/fetch-me"); + + const remoteSha = await defaultGitRunner.getSha( + cloneDir, + "refs/remotes/origin/feature/fetch-me", + ); + expect(remoteSha).toBeDefined(); + }); + + test("fetchRef throws GitError for a branch that does not exist on the remote", async () => { + const err = await defaultGitRunner + .fetchRef(cloneDir, "origin", "no-such-branch") + .catch((e: unknown) => e); + expect(err).toBeInstanceOf(GitError); + }); + + test("hasLocalBranch reports the presence of local branches", async () => { + expect(await defaultGitRunner.hasLocalBranch(cloneDir, "main")).toBe(true); + expect( + await defaultGitRunner.hasLocalBranch(cloneDir, "never-created"), + ).toBe(false); + }); + + test("checkoutCreateTracking creates a local branch tracking the remote ref", async () => { + // Seed a branch on the remote, fetch it, then use checkoutCreateTracking + // to create a local copy. + const seedDir = join(cloneDir, "..", "seed2"); + await $`git clone -q ${bareDir} ${seedDir}`.quiet(); + await $`git -C ${seedDir} -c user.email=s@b.co -c user.name=Side checkout -b feature/track-me`.quiet(); + await $`git -C ${seedDir} -c user.email=s@b.co -c user.name=Side commit --allow-empty -m "track"`.quiet(); + await $`git -C ${seedDir} push -q origin feature/track-me`.quiet(); + + await defaultGitRunner.fetchRef(cloneDir, "origin", "feature/track-me"); + await defaultGitRunner.checkoutCreateTracking( + cloneDir, + "feature/track-me", + "origin/feature/track-me", + ); + + expect( + await defaultGitRunner.hasLocalBranch(cloneDir, "feature/track-me"), + ).toBe(true); + // Current branch is now feature/track-me + expect(await defaultGitRunner.getCurrentBranch(cloneDir)).toBe( + "feature/track-me", + ); + + // Clean up: switch back to main for subsequent tests. + await defaultGitRunner.checkoutExistingBranch(cloneDir, "main"); + }); + + test("checkoutExistingBranch switches to an existing local branch", async () => { + // We should currently be on main (reset at the end of the previous test). + expect(await defaultGitRunner.getCurrentBranch(cloneDir)).toBe("main"); + + await defaultGitRunner.checkoutExistingBranch(cloneDir, "feature/track-me"); + expect(await defaultGitRunner.getCurrentBranch(cloneDir)).toBe( + "feature/track-me", + ); + + await defaultGitRunner.checkoutExistingBranch(cloneDir, "main"); + }); + + test("checkoutCreateTracking throws GitError when the branch already exists", async () => { + const err = await defaultGitRunner + .checkoutCreateTracking( + cloneDir, + "feature/track-me", + "origin/feature/track-me", + ) + .catch((e: unknown) => e); + expect(err).toBeInstanceOf(GitError); + }); + + test("mergeFastForwardOnly fast-forwards when the branch is strictly behind", async () => { + // Advance the remote's main by one commit via a side clone, then + // fetch + ff in our main clone. + const ahead = join(cloneDir, "..", "ahead"); + await $`git clone -q ${bareDir} ${ahead}`.quiet(); + await $`git -C ${ahead} -c user.email=a@b.co -c user.name=Ahead commit --allow-empty -m "ahead"`.quiet(); + await $`git -C ${ahead} push -q origin main`.quiet(); + + await defaultGitRunner.fetchRef(cloneDir, "origin", "main"); + await defaultGitRunner.mergeFastForwardOnly(cloneDir, "origin/main"); + + const localSha = await defaultGitRunner.getSha(cloneDir, "HEAD"); + const remoteSha = await defaultGitRunner.getSha( + cloneDir, + "refs/remotes/origin/main", + ); + expect(localSha).toBe(remoteSha); + }); + + test("mergeFastForwardOnly throws GitError when the branches have diverged", async () => { + // Real divergence: make a fresh clone, commit locally (one side of + // the diverge), then push a different commit to origin/main from a + // second clone (the other side). Now the common ancestor is in the + // past but neither branch is an ancestor of the other → git refuses + // to ff. + const divergeDir = join(cloneDir, "..", "diverge"); + await $`git clone -q ${bareDir} ${divergeDir}`.quiet(); + await $`git -C ${divergeDir} -c user.email=d@b.co -c user.name=Diverge commit --allow-empty -m "local diverge"`.quiet(); + + const remoteSide = join(cloneDir, "..", "remote-side"); + await $`git clone -q ${bareDir} ${remoteSide}`.quiet(); + await $`git -C ${remoteSide} -c user.email=r@b.co -c user.name=Remote commit --allow-empty -m "remote diverge"`.quiet(); + await $`git -C ${remoteSide} push -q origin main`.quiet(); + + await defaultGitRunner.fetchRef(divergeDir, "origin", "main"); + + const err = await defaultGitRunner + .mergeFastForwardOnly(divergeDir, "origin/main") + .catch((e: unknown) => e); + expect(err).toBeInstanceOf(GitError); + }); }); describe("resolveRepository with real git", () => { diff --git a/src/shared/repository/git.ts b/src/shared/repository/git.ts index 628d10d..a0075fb 100644 --- a/src/shared/repository/git.ts +++ b/src/shared/repository/git.ts @@ -1,5 +1,22 @@ import { $ } from "bun"; +/** + * Error from a git command that was expected to succeed. Carries git's stderr + * so callers can surface the underlying message (which is usually more + * actionable than anything bbcli could synthesize) to the user. + */ +export class GitError extends Error { + override name = "GitError"; + readonly stderr: string; + readonly exitCode: number; + + constructor(message: string, exitCode: number, stderr: string) { + super(message); + this.exitCode = exitCode; + this.stderr = stderr; + } +} + /** * Thin wrapper around the git CLI. Injected into resolveRepository so tests * can simulate every failure mode without shelling out. Production code uses @@ -42,6 +59,34 @@ export interface GitRunner { cwd: string, remote: string, ): Promise; + /** + * Fetches a single ref from the remote, updating the remote-tracking + * branch. Throws GitError on non-zero exit (network failure, no such + * branch on remote, etc.). + */ + fetchRef(cwd: string, remote: string, ref: string): Promise; + /** Returns true iff a local branch with the given name exists. */ + hasLocalBranch(cwd: string, branch: string): Promise; + /** + * Switches to an existing local branch. Throws GitError on non-zero + * exit (most commonly: a dirty working tree that would be clobbered). + */ + checkoutExistingBranch(cwd: string, branch: string): Promise; + /** + * Creates a new local branch tracking a remote ref (`/`) + * and checks it out. Throws GitError on non-zero exit. + */ + checkoutCreateTracking( + cwd: string, + branch: string, + remoteRef: string, + ): Promise; + /** + * Fast-forwards the current branch to the given ref, refusing any + * non-fast-forward update. Throws GitError on non-zero exit (most + * commonly: the branches have diverged). + */ + mergeFastForwardOnly(cwd: string, ref: string): Promise; } export const defaultGitRunner: GitRunner = { @@ -120,4 +165,54 @@ export const defaultGitRunner: GitRunner = { const prefix = `${remote}/`; return full.startsWith(prefix) ? full.slice(prefix.length) : full; }, + + async fetchRef(cwd, remote, ref) { + const result = await $`git -C ${cwd} fetch ${remote} ${ref}` + .nothrow() + .quiet(); + throwIfFailed(result, `git fetch ${remote} ${ref}`); + }, + + async hasLocalBranch(cwd, branch) { + // `show-ref --verify --quiet` exits 0 iff the ref exists; no output. + const result = + await $`git -C ${cwd} show-ref --verify --quiet ${`refs/heads/${branch}`}` + .nothrow() + .quiet(); + return result.exitCode === 0; + }, + + async checkoutExistingBranch(cwd, branch) { + const result = await $`git -C ${cwd} checkout ${branch}`.nothrow().quiet(); + throwIfFailed(result, `git checkout ${branch}`); + }, + + async checkoutCreateTracking(cwd, branch, remoteRef) { + const result = + await $`git -C ${cwd} checkout -b ${branch} --track ${remoteRef}` + .nothrow() + .quiet(); + throwIfFailed(result, `git checkout -b ${branch} --track ${remoteRef}`); + }, + + async mergeFastForwardOnly(cwd, ref) { + const result = await $`git -C ${cwd} merge --ff-only ${ref}` + .nothrow() + .quiet(); + throwIfFailed(result, `git merge --ff-only ${ref}`); + }, }; + +function throwIfFailed( + result: { exitCode: number; stderr: Buffer; stdout: Buffer }, + command: string, +): void { + if (result.exitCode === 0) return; + const stderr = result.stderr.toString().trim(); + const stdout = result.stdout.toString().trim(); + // Git writes most diagnostics to stderr, but a few to stdout; prefer + // stderr, fall back to stdout so we never lose the actual reason. + const message = + stderr || stdout || `${command} exited with code ${result.exitCode}`; + throw new GitError(message, result.exitCode, stderr); +} diff --git a/src/shared/repository/index.ts b/src/shared/repository/index.ts index f6bb27a..4f18d37 100644 --- a/src/shared/repository/index.ts +++ b/src/shared/repository/index.ts @@ -1,5 +1,5 @@ export type { GitRunner } from "./git.ts"; -export { defaultGitRunner } from "./git.ts"; +export { defaultGitRunner, GitError } from "./git.ts"; export type { RepositoryRef } from "./parse-url.ts"; export { parseBitbucketRemoteUrl } from "./parse-url.ts"; export { diff --git a/src/shared/repository/resolve.test.ts b/src/shared/repository/resolve.test.ts index dd78455..f03f914 100644 --- a/src/shared/repository/resolve.test.ts +++ b/src/shared/repository/resolve.test.ts @@ -32,6 +32,13 @@ function fakeGit(s: Setup = {}): GitRunner { async getDefaultBranchFromRemote() { return undefined; }, + async fetchRef() {}, + async hasLocalBranch() { + return false; + }, + async checkoutExistingBranch() {}, + async checkoutCreateTracking() {}, + async mergeFastForwardOnly() {}, }; } @@ -57,35 +64,54 @@ describe("resolveRepository — override", () => { test("does not consult git when override is given", async () => { let called = false; + const markCalled = () => { + called = true; + }; const git: GitRunner = { async isInsideWorkTree() { - called = true; + markCalled(); return true; }, async listRemotes() { - called = true; + markCalled(); return []; }, async getRemoteUrl() { - called = true; + markCalled(); return undefined; }, async getCurrentBranch() { - called = true; + markCalled(); return undefined; }, async getSha() { - called = true; + markCalled(); return undefined; }, async getRemoteBranchSha() { - called = true; + markCalled(); return undefined; }, async getDefaultBranchFromRemote() { - called = true; + markCalled(); return undefined; }, + async fetchRef() { + markCalled(); + }, + async hasLocalBranch() { + markCalled(); + return false; + }, + async checkoutExistingBranch() { + markCalled(); + }, + async checkoutCreateTracking() { + markCalled(); + }, + async mergeFastForwardOnly() { + markCalled(); + }, }; await resolveRepository({ override: "ws/repo" }, git); expect(called).toBe(false);