Initial implementation of PR checkout#97
Conversation
`forge pr checkout 123` will now check out PR/MR locally: 1. Bunch of API calls (forge-specific) 2. Add fork as remote if necessary 3. Make new branch with upstream from fork I tested this manually on Codeberg and GitHub. An agent also tested it on GitLab and Bitbucket. Since there are no e2e tests in this repo, I mainly constrained myself to testing the CLI with a mock forge. There are no CLI tests that spawn HTTP servers and run CLI, so I refrained from that. Existing fields on the structs were preserved for backwards compat, let me know if you want me to remove them since this library is young, or whether PRBranch should be flattened into its parent structs.
andrew
left a comment
There was a problem hiding this comment.
Thanks for this, the backend plumbing is the hard part and it looks right across all four forges. A few things to sort before merging.
Should fix
checkoutSameRepoPR hardcodes "origin" (internal/cli/pr.go). The CLI has a global --remote flag wired through resolve.SetRemote, so forge --remote upstream pr checkout 42 resolves the PR against upstream but then fetches from origin. It should use whichever remote resolve.Repo actually read. resolve doesn't currently export that name; a small resolve.RemoteName() accessor would do.
GitLab silently drops the fork lookup error (gitlab/prs.go). When GetProject(mr.SourceProjectID) fails (private fork, deleted project) the error is discarded and Fork stays nil, so checkout falls through to the same-repo path and git fails with "couldn't find remote ref" instead of telling the user the source project isn't accessible. Returning the error would be better.
Gitea: pr.Head.Repository.Owner is *User in the SDK and can be nil (deleted owner), so Owner.UserName will panic. Needs a guard.
On the Head/Base question
Drop the old string fields rather than keep both. We're pre-1.0 and nothing imports this yet; having pr.Head and pr.HeadBranch.Ref hold the same value will drift. I'd make Head and Base into PRBranch values (non-pointer, they're always populated) and update the callers.
Smaller stuff
--force only actually gates the dirty-tree check. An existing local branch just prints a warning and gets reset via -B regardless. Either make existing-branch a hard error without --force, or drop "or existing branch" from the flag help.
ensureRemote does exact-string URL matching, so a fork already added via SSH won't match the HTTPS URL the API returns (and Bitbucket's https://user@... form won't match either). Fine to leave for now but it'll come up.
Tests: use t.Chdir(dir) instead of os.Getwd/os.Chdir with manual cleanup, to match resolve_test.go and repo_test.go.
deadcode flags SetTestForge/ResetTestForge because they're test-only exports. Same situation as setBitbucketAPI so not new noise, just FYI.
|
I addressed everything but the
I got rid of that behavior, the code now roughly matches what the gh cli does. Unfortunately I find the gh cli's behavior somewhat confusing: It doesn't refuse to check out PRs if the worktree is dirty, it just sort of fails halfway through if any files conflict. But I think unless we can show that this behavior can be improved without fundamentally breaking people's existing gh cli workflow, it's better to be bug-compatible. |
forge pr checkout 123will now check out PR/MR locally:I tested this manually on Codeberg and GitHub. An agent also tested it on GitLab and Bitbucket.
Since there are no e2e tests in this repo, I mainly constrained myself to testing the CLI with a mock forge. There are no CLI tests that spawn HTTP servers and run CLI, so I refrained from that.
Existing fields on the structs were preserved for backwards compat, let me know if you want me to remove them since this library is young, or whether PRBranch should be flattened into its parent structs.
Fix #92