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);