From 7d34d715accf537cd7bd892747098f148a41bfa8 Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Mon, 8 Jun 2026 00:00:10 +0200 Subject: [PATCH 1/4] Fix forge pr checkout for AGit-flow I encountered this error: ``` $ git cd https://codeberg.org/forgejo/website/pulls/926/files fatal: couldn't find remote ref refs/heads/refs/pull/926/head Error: fetching origin/refs/pull/926/head: exit status 128 ``` It turned out that the gitea module returns a full git ref `refs/pull/123` for PRs that don't have a real git branch to check out to. This case wasn't properly handled by the generic checkout code. --- gitea/prs.go | 4 ++ internal/cli/pr.go | 45 ++++++++++++++---- internal/cli/pr_checkout_test.go | 80 ++++++++++++++++++++++++++++++++ 3 files changed, 121 insertions(+), 8 deletions(-) diff --git a/gitea/prs.go b/gitea/prs.go index a0920ae..8cc0106 100644 --- a/gitea/prs.go +++ b/gitea/prs.go @@ -58,6 +58,10 @@ func convertGiteaPR(pr *gitea.PullRequest) forge.PullRequest { } if pr.Head != nil { result.Head = forge.PRBranch{ + // Ref is the head branch name when one exists, but Gitea/Forgejo + // fall back to a refs/pull//head ref when there is no head + // branch: AGit-flow PRs (pushed to refs/for/*, never branched) or + // PRs whose branch was deleted. Callers must tolerate a full ref. Ref: pr.Head.Ref, SHA: pr.Head.Sha, } diff --git a/internal/cli/pr.go b/internal/cli/pr.go index 31c7da7..a5bd7e2 100644 --- a/internal/cli/pr.go +++ b/internal/cli/pr.go @@ -601,16 +601,22 @@ The argument can be a PR number or a full URL: return fmt.Errorf("getting PR #%d: %w", number, err) } - // remoteRef is the branch name on the remote (PR's head branch) + // remoteRef is usually the PR's head branch name, but Gitea/Forgejo + // report a refs/pull//head ref when there is no head branch + // (AGit-flow PRs, or PRs whose branch was deleted). Such a ref only + // lives on the base repo. remoteRef := pr.Head.Ref - // localBranch is what we'll name the local branch (defaults to remote ref) - localBranch := remoteRef - if flagBranch != "" { - localBranch = flagBranch + // localBranch is what we'll name the local branch (defaults to the + // head branch name, or pr- when only a pull ref is known). + localBranch := flagBranch + if localBranch == "" { + localBranch = defaultLocalBranch(pr) } - if pr.Head.Fork != nil { + // A pull ref isn't present on the fork remote, only on origin, so + // route it through the same-repo path even for fork PRs. + if pr.Head.Fork != nil && !isFullRef(remoteRef) { return checkoutForkPR(ctx, domain, pr, remoteRef, localBranch, flagRemoteName, flagDetach, flagForce) } @@ -699,8 +705,31 @@ func remoteMatches(existingURL, wantURL string) bool { return domain == wantDomain && owner == wantOwner && repo == wantRepo } +// isFullRef reports whether ref is a fully-qualified git ref (e.g. +// refs/pull//head) rather than a bare branch name. +func isFullRef(ref string) bool { + return strings.HasPrefix(ref, "refs/") +} + +// defaultLocalBranch picks the local branch name for a checked-out PR. It uses +// the head branch name when available, but falls back to pr- when only +// a pull ref is known (Gitea/Forgejo PRs with no head branch, e.g. AGit flow). +func defaultLocalBranch(pr *forges.PullRequest) string { + if isFullRef(pr.Head.Ref) { + return fmt.Sprintf("pr-%d", pr.Number) + } + return pr.Head.Ref +} + func gitCheckout(ctx context.Context, remote, remoteRef, localBranch string, detach, force bool) error { - refspec := fmt.Sprintf("+refs/heads/%s:refs/remotes/%s/%s", remoteRef, remote, remoteRef) + // A bare branch name needs the refs/heads/ prefix; a full ref (e.g. + // refs/pull//head) is fetched as-is. The remote-tracking ref is named + // after localBranch so a pull ref doesn't leak into refs/remotes/. + src := remoteRef + if !isFullRef(src) { + src = "refs/heads/" + src + } + refspec := fmt.Sprintf("+%s:refs/remotes/%s/%s", src, remote, localBranch) fetchCmd := exec.CommandContext(ctx, "git", "fetch", "--", remote, refspec) fetchCmd.Stdout = os.Stdout fetchCmd.Stderr = os.Stderr @@ -708,7 +737,7 @@ func gitCheckout(ctx context.Context, remote, remoteRef, localBranch string, det return fmt.Errorf("fetching %s/%s: %w", remote, remoteRef, err) } - ref := remote + "/" + remoteRef + ref := remote + "/" + localBranch if detach { cmd := exec.CommandContext(ctx, "git", "checkout", "--detach", ref) diff --git a/internal/cli/pr_checkout_test.go b/internal/cli/pr_checkout_test.go index f3b37ac..933a67f 100644 --- a/internal/cli/pr_checkout_test.go +++ b/internal/cli/pr_checkout_test.go @@ -188,6 +188,86 @@ func pushBranchToRemote(t *testing.T, repoDir, remoteName, branchName string) { } } +// pushToRemoteRef creates a commit and pushes it to an arbitrary ref on the +// remote (e.g. refs/pull/42/head), mimicking Gitea/Forgejo PRs whose head +// branch is gone and only the pull ref remains. +func pushToRemoteRef(t *testing.T, repoDir, remoteName, ref string) { + t.Helper() + + testFile := filepath.Join(repoDir, "pullref.txt") + if err := os.WriteFile(testFile, []byte("content for "+ref+"\n"), 0644); err != nil { + t.Fatalf("writing test file: %v", err) + } + + commands := [][]string{ + {"git", "checkout", "-b", "tmp-pushref"}, + {"git", "add", "."}, + {"git", "commit", "-m", "commit for " + ref}, + {"git", "push", remoteName, "HEAD:" + ref}, + {"git", "checkout", "-"}, + {"git", "branch", "-D", "tmp-pushref"}, + } + + for _, args := range commands { + cmd := exec.Command(args[0], args[1:]...) + cmd.Dir = repoDir + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git command %v failed: %v\n%s", args, err, out) + } + } +} + +// TestPRCheckoutPullRef covers Gitea/Forgejo PRs whose head.ref is a full +// refs/pull//head ref rather than a branch name. The ref must be fetched +// as-is (not under refs/heads/) and the local branch falls back to pr-. +func TestPRCheckoutPullRef(t *testing.T) { + if testing.Short() { + t.Skip("skipping git integration test in short mode") + } + + checkoutCmd, _, _ := rootCmd.Find([]string{"pr", "checkout"}) + if checkoutCmd != nil { + _ = checkoutCmd.Flags().Set("detach", "false") + _ = checkoutCmd.Flags().Set("force", "false") + _ = checkoutCmd.Flags().Set("branch", "") + _ = checkoutCmd.Flags().Set("remote-name", "") + } + + originDir := setupBareRepo(t) + workDir := setupTestRepo(t, originDir) + pushToRemoteRef(t, workDir, "origin", "refs/pull/42/head") + t.Chdir(workDir) + + pr := &forges.PullRequest{ + Number: 42, + Head: forges.PRBranch{Ref: "refs/pull/42/head", SHA: "abc123"}, + } + resolve.SetTestForge( + &mockForge{prService: &mockPRService{pr: pr}}, + "testowner", "testrepo", "github.com", + ) + t.Cleanup(resolve.ResetTestForge) + + var buf bytes.Buffer + rootCmd.SetOut(&buf) + rootCmd.SetErr(&buf) + rootCmd.SetArgs([]string{"pr", "checkout", "42"}) + + if err := rootCmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v\noutput: %s", err, buf.String()) + } + + cmd := exec.Command("git", "branch", "--show-current") + cmd.Dir = workDir + out, err := cmd.Output() + if err != nil { + t.Fatalf("getting current branch: %v", err) + } + if got := strings.TrimSpace(string(out)); got != "pr-42" { + t.Errorf("branch: want %q, got %q", "pr-42", got) + } +} + func TestEnsureRemote(t *testing.T) { tests := []struct { name string From c8a10709e5133b12775b96cde5936b89398d4ad6 Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Mon, 8 Jun 2026 00:46:29 +0200 Subject: [PATCH 2/4] Introduce PullRef --- gitea/gitea_test.go | 51 ++++++++++++++++++++++ gitea/prs.go | 20 ++++++--- github/prs.go | 5 ++- github/prs_test.go | 1 + gitlab/gitlab_test.go | 27 ++++++++++++ gitlab/prs.go | 4 +- internal/cli/pr.go | 74 ++++++++++++++++---------------- internal/cli/pr_checkout_test.go | 6 +-- types.go | 9 +++- 9 files changed, 145 insertions(+), 52 deletions(-) diff --git a/gitea/gitea_test.go b/gitea/gitea_test.go index 9320bdd..b7e43b6 100644 --- a/gitea/gitea_test.go +++ b/gitea/gitea_test.go @@ -284,6 +284,57 @@ func TestGiteaListReposFallbackToUser(t *testing.T) { assertEqual(t, "repos[0].FullName", "someuser/personal", repos[0].FullName) } +func TestGiteaGetPullRequestHeadRef(t *testing.T) { + // Gitea/Forgejo put the head branch name in "ref" while it exists, but fall + // back to refs/pull//head for branchless PRs (AGit flow / deleted + // branch). The converter must surface a clean branch name (empty when + // absent) plus the always-present pull ref. + tests := []struct { + name string + headRef string + wantRef string + wantPullRef string + }{ + { + name: "live branch", + headRef: "feature-branch", + wantRef: "feature-branch", + wantPullRef: "refs/pull/42/head", + }, + { + name: "branchless (AGit) PR", + headRef: "refs/pull/42/head", + wantRef: "", + wantPullRef: "refs/pull/42/head", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("GET /api/v1/version", giteaVersionHandler) + mux.HandleFunc("GET /api/v1/repos/testorg/testrepo/pulls/42", func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{ + "number": 42, + "head": map[string]any{"ref": tt.headRef, "sha": "headsha"}, + "base": map[string]any{"ref": "main", "sha": "basesha"}, + }) + }) + + srv := httptest.NewServer(mux) + defer srv.Close() + + pr, err := New(srv.URL, "", nil).PullRequests().Get(context.Background(), "testorg", "testrepo", 42) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + assertEqual(t, "Head.Ref", tt.wantRef, pr.Head.Ref) + assertEqual(t, "Head.PullRef", tt.wantPullRef, pr.Head.PullRef) + }) + } +} + func TestGiteaListTags(t *testing.T) { mux := http.NewServeMux() mux.HandleFunc("GET /api/v1/version", giteaVersionHandler) diff --git a/gitea/prs.go b/gitea/prs.go index 8cc0106..90cc1cc 100644 --- a/gitea/prs.go +++ b/gitea/prs.go @@ -5,6 +5,7 @@ import ( "fmt" forge "github.com/git-pkgs/forge" "net/http" + "strings" "code.gitea.io/sdk/gitea" ) @@ -57,13 +58,20 @@ func convertGiteaPR(pr *gitea.PullRequest) forge.PullRequest { baseRepoID = pr.Base.RepoID } if pr.Head != nil { + // Gitea/Forgejo report the head branch name in Ref while it exists, but + // fall back to the refs/pull//head ref when the PR has no head branch + // (AGit flow, pushed to refs/for/* and never branched; or a deleted + // branch). Normalize that into a clean branch name (empty when absent) + // plus the always-present pull ref, so callers never see a raw ref in + // Ref. + headBranch := pr.Head.Ref + if strings.HasPrefix(headBranch, "refs/") { + headBranch = "" + } result.Head = forge.PRBranch{ - // Ref is the head branch name when one exists, but Gitea/Forgejo - // fall back to a refs/pull//head ref when there is no head - // branch: AGit-flow PRs (pushed to refs/for/*, never branched) or - // PRs whose branch was deleted. Callers must tolerate a full ref. - Ref: pr.Head.Ref, - SHA: pr.Head.Sha, + Ref: headBranch, + SHA: pr.Head.Sha, + PullRef: fmt.Sprintf("refs/pull/%d/head", pr.Index), } if pr.Head.RepoID != baseRepoID && pr.Head.Repository != nil && pr.Head.Repository.Owner != nil { result.Head.Fork = &forge.ForkInfo{ diff --git a/github/prs.go b/github/prs.go index b4f97f3..23fccc4 100644 --- a/github/prs.go +++ b/github/prs.go @@ -93,8 +93,9 @@ func convertGitHubPR(pr *github.PullRequest) forge.PullRequest { } if h := pr.GetHead(); h != nil { result.Head = forge.PRBranch{ - Ref: h.GetRef(), - SHA: h.GetSHA(), + Ref: h.GetRef(), + SHA: h.GetSHA(), + PullRef: fmt.Sprintf("refs/pull/%d/head", pr.GetNumber()), } if repo := h.GetRepo(); repo != nil { if repo.GetFullName() != baseFullName { diff --git a/github/prs_test.go b/github/prs_test.go index 8ff6a49..4615d71 100644 --- a/github/prs_test.go +++ b/github/prs_test.go @@ -70,6 +70,7 @@ func TestGitHubGetPR(t *testing.T) { assertEqualBool(t, "Merged", false, pr.Merged) assertEqualBool(t, "Mergeable", true, pr.Mergeable) assertEqual(t, "Head", "feature-branch", pr.Head.Ref) + assertEqual(t, "Head.PullRef", "refs/pull/1/head", pr.Head.PullRef) assertEqual(t, "Base", "main", pr.Base.Ref) assertEqual(t, "Author.Login", "octocat", pr.Author.Login) assertEqualInt(t, "Comments", 2, pr.Comments) diff --git a/gitlab/gitlab_test.go b/gitlab/gitlab_test.go index 0513a9a..7dab16b 100644 --- a/gitlab/gitlab_test.go +++ b/gitlab/gitlab_test.go @@ -203,3 +203,30 @@ func TestGitLabListTags(t *testing.T) { assertEqual(t, "Tag[1].Name", "v1.0.0", tags[1].Name) assertEqual(t, "Tag[1].Commit", "bbb222", tags[1].Commit) } + +func TestGitLabGetMergeRequestPullRef(t *testing.T) { + // GitLab exposes a PR head via refs/merge-requests//head, which + // checkout uses to fetch branchless MRs (e.g. a deleted source branch). + mux := http.NewServeMux() + mux.HandleFunc("GET /api/v4/projects/mygroup%2Fmyrepo/merge_requests/7", func(w http.ResponseWriter, r *http.Request) { + _ = json.NewEncoder(w).Encode(map[string]any{ + "iid": 7, + "title": "Add feature", + "state": "opened", + "source_branch": "feature-branch", + "target_branch": "main", + "web_url": "https://gitlab.com/mygroup/myrepo/-/merge_requests/7", + }) + }) + + srv := httptest.NewServer(mux) + defer srv.Close() + + pr, err := New(srv.URL, "test-token", nil).PullRequests().Get(context.Background(), "mygroup", "myrepo", 7) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + assertEqual(t, "Head.Ref", "feature-branch", pr.Head.Ref) + assertEqual(t, "Head.PullRef", "refs/merge-requests/7/head", pr.Head.PullRef) +} diff --git a/gitlab/prs.go b/gitlab/prs.go index 6251ab9..c8dc7d9 100644 --- a/gitlab/prs.go +++ b/gitlab/prs.go @@ -29,7 +29,7 @@ func convertGitLabMR(mr *gitlab.MergeRequest) forge.PullRequest { Body: mr.Description, State: mr.State, // "opened", "closed", "merged" Draft: mr.Draft, - Head: forge.PRBranch{Ref: mr.SourceBranch, SHA: mr.SHA}, + Head: forge.PRBranch{Ref: mr.SourceBranch, SHA: mr.SHA, PullRef: fmt.Sprintf("refs/merge-requests/%d/head", mr.IID)}, Base: forge.PRBranch{Ref: mr.TargetBranch}, Merged: mr.State == "merged", Comments: int(mr.UserNotesCount), @@ -118,7 +118,7 @@ func convertBasicGitLabMR(mr *gitlab.BasicMergeRequest) forge.PullRequest { Body: mr.Description, State: mr.State, Draft: mr.Draft, - Head: forge.PRBranch{Ref: mr.SourceBranch}, + Head: forge.PRBranch{Ref: mr.SourceBranch, PullRef: fmt.Sprintf("refs/merge-requests/%d/head", mr.IID)}, Base: forge.PRBranch{Ref: mr.TargetBranch}, Merged: mr.State == "merged", HTMLURL: mr.WebURL, diff --git a/internal/cli/pr.go b/internal/cli/pr.go index a5bd7e2..25e5af8 100644 --- a/internal/cli/pr.go +++ b/internal/cli/pr.go @@ -99,11 +99,20 @@ func prViewCmd() *cobra.Command { return cmd } +// prHeadLabel is a display label for a PR's head: the branch name, or the pull +// ref when the PR has no head branch (AGit flow / deleted branch). +func prHeadLabel(h forges.PRBranch) string { + if h.Ref != "" { + return h.Ref + } + return h.PullRef +} + func printPRDetails(pr *forges.PullRequest) { _, _ = fmt.Fprintf(os.Stdout, "#%d %s\n", pr.Number, output.Sanitize(pr.Title)) _, _ = fmt.Fprintf(os.Stdout, "State: %s\n", pr.State) _, _ = fmt.Fprintf(os.Stdout, "Author: %s\n", output.Sanitize(pr.Author.Login)) - _, _ = fmt.Fprintf(os.Stdout, "Branch: %s -> %s\n", pr.Head.Ref, pr.Base.Ref) + _, _ = fmt.Fprintf(os.Stdout, "Branch: %s -> %s\n", prHeadLabel(pr.Head), pr.Base.Ref) if pr.Draft { _, _ = fmt.Fprintln(os.Stdout, "Draft: yes") @@ -211,7 +220,7 @@ func prListCmd() *cobra.Command { strconv.Itoa(pr.Number), title, output.Sanitize(pr.Author.Login), - pr.Head.Ref, + prHeadLabel(pr.Head), pr.UpdatedAt.Format("2006-01-02"), } } @@ -601,26 +610,31 @@ The argument can be a PR number or a full URL: return fmt.Errorf("getting PR #%d: %w", number, err) } - // remoteRef is usually the PR's head branch name, but Gitea/Forgejo - // report a refs/pull//head ref when there is no head branch - // (AGit-flow PRs, or PRs whose branch was deleted). Such a ref only - // lives on the base repo. - remoteRef := pr.Head.Ref + head := pr.Head - // localBranch is what we'll name the local branch (defaults to the - // head branch name, or pr- when only a pull ref is known). - localBranch := flagBranch - if localBranch == "" { - localBranch = defaultLocalBranch(pr) + // A PR with a head branch is fetched by branch name (from the fork + // remote when it lives on a fork). A branchless PR (e.g. AGit flow) + // has no branch to fetch, so fall back to its pull ref, which the + // forge exposes on the base repo. + if head.Ref != "" { + localBranch := flagBranch + if localBranch == "" { + localBranch = head.Ref + } + if head.Fork != nil { + return checkoutForkPR(ctx, domain, pr, head.Ref, localBranch, flagRemoteName, flagDetach, flagForce) + } + return checkoutSameRepoPR(ctx, head.Ref, localBranch, flagDetach, flagForce) } - // A pull ref isn't present on the fork remote, only on origin, so - // route it through the same-repo path even for fork PRs. - if pr.Head.Fork != nil && !isFullRef(remoteRef) { - return checkoutForkPR(ctx, domain, pr, remoteRef, localBranch, flagRemoteName, flagDetach, flagForce) + if head.PullRef == "" { + return fmt.Errorf("PR #%d has no head branch to check out", pr.Number) } - - return checkoutSameRepoPR(ctx, remoteRef, localBranch, flagDetach, flagForce) + localBranch := flagBranch + if localBranch == "" { + localBranch = fmt.Sprintf("pr-%d", pr.Number) + } + return checkoutSameRepoPR(ctx, head.PullRef, localBranch, flagDetach, flagForce) }, } @@ -705,28 +719,12 @@ func remoteMatches(existingURL, wantURL string) bool { return domain == wantDomain && owner == wantOwner && repo == wantRepo } -// isFullRef reports whether ref is a fully-qualified git ref (e.g. -// refs/pull//head) rather than a bare branch name. -func isFullRef(ref string) bool { - return strings.HasPrefix(ref, "refs/") -} - -// defaultLocalBranch picks the local branch name for a checked-out PR. It uses -// the head branch name when available, but falls back to pr- when only -// a pull ref is known (Gitea/Forgejo PRs with no head branch, e.g. AGit flow). -func defaultLocalBranch(pr *forges.PullRequest) string { - if isFullRef(pr.Head.Ref) { - return fmt.Sprintf("pr-%d", pr.Number) - } - return pr.Head.Ref -} - func gitCheckout(ctx context.Context, remote, remoteRef, localBranch string, detach, force bool) error { - // A bare branch name needs the refs/heads/ prefix; a full ref (e.g. - // refs/pull//head) is fetched as-is. The remote-tracking ref is named - // after localBranch so a pull ref doesn't leak into refs/remotes/. + // A bare branch name needs the refs/heads/ prefix; an already-qualified ref + // (e.g. a refs/pull//head pull ref) is fetched as-is. The remote-tracking + // ref is named after localBranch so a pull ref doesn't leak into refs/remotes/. src := remoteRef - if !isFullRef(src) { + if !strings.HasPrefix(src, "refs/") { src = "refs/heads/" + src } refspec := fmt.Sprintf("+%s:refs/remotes/%s/%s", src, remote, localBranch) diff --git a/internal/cli/pr_checkout_test.go b/internal/cli/pr_checkout_test.go index 933a67f..8d02be0 100644 --- a/internal/cli/pr_checkout_test.go +++ b/internal/cli/pr_checkout_test.go @@ -217,8 +217,8 @@ func pushToRemoteRef(t *testing.T, repoDir, remoteName, ref string) { } } -// TestPRCheckoutPullRef covers Gitea/Forgejo PRs whose head.ref is a full -// refs/pull//head ref rather than a branch name. The ref must be fetched +// TestPRCheckoutPullRef covers branchless PRs (e.g. Gitea/Forgejo AGit flow): +// Head.Ref is empty and only Head.PullRef is set. The pull ref must be fetched // as-is (not under refs/heads/) and the local branch falls back to pr-. func TestPRCheckoutPullRef(t *testing.T) { if testing.Short() { @@ -240,7 +240,7 @@ func TestPRCheckoutPullRef(t *testing.T) { pr := &forges.PullRequest{ Number: 42, - Head: forges.PRBranch{Ref: "refs/pull/42/head", SHA: "abc123"}, + Head: forges.PRBranch{PullRef: "refs/pull/42/head", SHA: "abc123"}, } resolve.SetTestForge( &mockForge{prService: &mockPRService{pr: pr}}, diff --git a/types.go b/types.go index bc04c18..ce59c0b 100644 --- a/types.go +++ b/types.go @@ -240,9 +240,16 @@ type ForkInfo struct { // PRBranch holds branch info including the repository it belongs to. // For same-repo PRs, Fork is nil. For fork PRs, Fork points to the source repo. type PRBranch struct { - Ref string `json:"ref"` // branch name + Ref string `json:"ref"` // branch name; empty if the PR has no head branch SHA string `json:"sha,omitempty"` // commit SHA Fork *ForkInfo `json:"fork,omitempty"` // nil if same repo as target + // PullRef is a read-only ref on the base repo that always resolves the head + // commit: refs/pull//head on GitHub and Gitea/Forgejo, + // refs/merge-requests//head on GitLab. It is the only way to fetch a + // branchless PR (e.g. one created via AGit flow, or whose head branch was + // deleted), so checkout uses it as a fallback when Ref is empty. Empty when + // the forge exposes no such ref (e.g. Bitbucket Cloud). + PullRef string `json:"pull_ref,omitempty"` } // PullRequest holds normalized metadata about a pull request (or merge request). From 0ec85d48b2ede3dfe01f3cbca5c4221c7cf8409d Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Mon, 8 Jun 2026 00:58:33 +0200 Subject: [PATCH 3/4] Revert "Introduce PullRef" This reverts commit c8a10709e5133b12775b96cde5936b89398d4ad6. --- gitea/gitea_test.go | 51 ---------------------- gitea/prs.go | 20 +++------ github/prs.go | 5 +-- github/prs_test.go | 1 - gitlab/gitlab_test.go | 27 ------------ gitlab/prs.go | 4 +- internal/cli/pr.go | 74 ++++++++++++++++---------------- internal/cli/pr_checkout_test.go | 6 +-- types.go | 9 +--- 9 files changed, 52 insertions(+), 145 deletions(-) diff --git a/gitea/gitea_test.go b/gitea/gitea_test.go index b7e43b6..9320bdd 100644 --- a/gitea/gitea_test.go +++ b/gitea/gitea_test.go @@ -284,57 +284,6 @@ func TestGiteaListReposFallbackToUser(t *testing.T) { assertEqual(t, "repos[0].FullName", "someuser/personal", repos[0].FullName) } -func TestGiteaGetPullRequestHeadRef(t *testing.T) { - // Gitea/Forgejo put the head branch name in "ref" while it exists, but fall - // back to refs/pull//head for branchless PRs (AGit flow / deleted - // branch). The converter must surface a clean branch name (empty when - // absent) plus the always-present pull ref. - tests := []struct { - name string - headRef string - wantRef string - wantPullRef string - }{ - { - name: "live branch", - headRef: "feature-branch", - wantRef: "feature-branch", - wantPullRef: "refs/pull/42/head", - }, - { - name: "branchless (AGit) PR", - headRef: "refs/pull/42/head", - wantRef: "", - wantPullRef: "refs/pull/42/head", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - mux := http.NewServeMux() - mux.HandleFunc("GET /api/v1/version", giteaVersionHandler) - mux.HandleFunc("GET /api/v1/repos/testorg/testrepo/pulls/42", func(w http.ResponseWriter, r *http.Request) { - _ = json.NewEncoder(w).Encode(map[string]any{ - "number": 42, - "head": map[string]any{"ref": tt.headRef, "sha": "headsha"}, - "base": map[string]any{"ref": "main", "sha": "basesha"}, - }) - }) - - srv := httptest.NewServer(mux) - defer srv.Close() - - pr, err := New(srv.URL, "", nil).PullRequests().Get(context.Background(), "testorg", "testrepo", 42) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - - assertEqual(t, "Head.Ref", tt.wantRef, pr.Head.Ref) - assertEqual(t, "Head.PullRef", tt.wantPullRef, pr.Head.PullRef) - }) - } -} - func TestGiteaListTags(t *testing.T) { mux := http.NewServeMux() mux.HandleFunc("GET /api/v1/version", giteaVersionHandler) diff --git a/gitea/prs.go b/gitea/prs.go index 90cc1cc..8cc0106 100644 --- a/gitea/prs.go +++ b/gitea/prs.go @@ -5,7 +5,6 @@ import ( "fmt" forge "github.com/git-pkgs/forge" "net/http" - "strings" "code.gitea.io/sdk/gitea" ) @@ -58,20 +57,13 @@ func convertGiteaPR(pr *gitea.PullRequest) forge.PullRequest { baseRepoID = pr.Base.RepoID } if pr.Head != nil { - // Gitea/Forgejo report the head branch name in Ref while it exists, but - // fall back to the refs/pull//head ref when the PR has no head branch - // (AGit flow, pushed to refs/for/* and never branched; or a deleted - // branch). Normalize that into a clean branch name (empty when absent) - // plus the always-present pull ref, so callers never see a raw ref in - // Ref. - headBranch := pr.Head.Ref - if strings.HasPrefix(headBranch, "refs/") { - headBranch = "" - } result.Head = forge.PRBranch{ - Ref: headBranch, - SHA: pr.Head.Sha, - PullRef: fmt.Sprintf("refs/pull/%d/head", pr.Index), + // Ref is the head branch name when one exists, but Gitea/Forgejo + // fall back to a refs/pull//head ref when there is no head + // branch: AGit-flow PRs (pushed to refs/for/*, never branched) or + // PRs whose branch was deleted. Callers must tolerate a full ref. + Ref: pr.Head.Ref, + SHA: pr.Head.Sha, } if pr.Head.RepoID != baseRepoID && pr.Head.Repository != nil && pr.Head.Repository.Owner != nil { result.Head.Fork = &forge.ForkInfo{ diff --git a/github/prs.go b/github/prs.go index 23fccc4..b4f97f3 100644 --- a/github/prs.go +++ b/github/prs.go @@ -93,9 +93,8 @@ func convertGitHubPR(pr *github.PullRequest) forge.PullRequest { } if h := pr.GetHead(); h != nil { result.Head = forge.PRBranch{ - Ref: h.GetRef(), - SHA: h.GetSHA(), - PullRef: fmt.Sprintf("refs/pull/%d/head", pr.GetNumber()), + Ref: h.GetRef(), + SHA: h.GetSHA(), } if repo := h.GetRepo(); repo != nil { if repo.GetFullName() != baseFullName { diff --git a/github/prs_test.go b/github/prs_test.go index 4615d71..8ff6a49 100644 --- a/github/prs_test.go +++ b/github/prs_test.go @@ -70,7 +70,6 @@ func TestGitHubGetPR(t *testing.T) { assertEqualBool(t, "Merged", false, pr.Merged) assertEqualBool(t, "Mergeable", true, pr.Mergeable) assertEqual(t, "Head", "feature-branch", pr.Head.Ref) - assertEqual(t, "Head.PullRef", "refs/pull/1/head", pr.Head.PullRef) assertEqual(t, "Base", "main", pr.Base.Ref) assertEqual(t, "Author.Login", "octocat", pr.Author.Login) assertEqualInt(t, "Comments", 2, pr.Comments) diff --git a/gitlab/gitlab_test.go b/gitlab/gitlab_test.go index 7dab16b..0513a9a 100644 --- a/gitlab/gitlab_test.go +++ b/gitlab/gitlab_test.go @@ -203,30 +203,3 @@ func TestGitLabListTags(t *testing.T) { assertEqual(t, "Tag[1].Name", "v1.0.0", tags[1].Name) assertEqual(t, "Tag[1].Commit", "bbb222", tags[1].Commit) } - -func TestGitLabGetMergeRequestPullRef(t *testing.T) { - // GitLab exposes a PR head via refs/merge-requests//head, which - // checkout uses to fetch branchless MRs (e.g. a deleted source branch). - mux := http.NewServeMux() - mux.HandleFunc("GET /api/v4/projects/mygroup%2Fmyrepo/merge_requests/7", func(w http.ResponseWriter, r *http.Request) { - _ = json.NewEncoder(w).Encode(map[string]any{ - "iid": 7, - "title": "Add feature", - "state": "opened", - "source_branch": "feature-branch", - "target_branch": "main", - "web_url": "https://gitlab.com/mygroup/myrepo/-/merge_requests/7", - }) - }) - - srv := httptest.NewServer(mux) - defer srv.Close() - - pr, err := New(srv.URL, "test-token", nil).PullRequests().Get(context.Background(), "mygroup", "myrepo", 7) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - - assertEqual(t, "Head.Ref", "feature-branch", pr.Head.Ref) - assertEqual(t, "Head.PullRef", "refs/merge-requests/7/head", pr.Head.PullRef) -} diff --git a/gitlab/prs.go b/gitlab/prs.go index c8dc7d9..6251ab9 100644 --- a/gitlab/prs.go +++ b/gitlab/prs.go @@ -29,7 +29,7 @@ func convertGitLabMR(mr *gitlab.MergeRequest) forge.PullRequest { Body: mr.Description, State: mr.State, // "opened", "closed", "merged" Draft: mr.Draft, - Head: forge.PRBranch{Ref: mr.SourceBranch, SHA: mr.SHA, PullRef: fmt.Sprintf("refs/merge-requests/%d/head", mr.IID)}, + Head: forge.PRBranch{Ref: mr.SourceBranch, SHA: mr.SHA}, Base: forge.PRBranch{Ref: mr.TargetBranch}, Merged: mr.State == "merged", Comments: int(mr.UserNotesCount), @@ -118,7 +118,7 @@ func convertBasicGitLabMR(mr *gitlab.BasicMergeRequest) forge.PullRequest { Body: mr.Description, State: mr.State, Draft: mr.Draft, - Head: forge.PRBranch{Ref: mr.SourceBranch, PullRef: fmt.Sprintf("refs/merge-requests/%d/head", mr.IID)}, + Head: forge.PRBranch{Ref: mr.SourceBranch}, Base: forge.PRBranch{Ref: mr.TargetBranch}, Merged: mr.State == "merged", HTMLURL: mr.WebURL, diff --git a/internal/cli/pr.go b/internal/cli/pr.go index 25e5af8..a5bd7e2 100644 --- a/internal/cli/pr.go +++ b/internal/cli/pr.go @@ -99,20 +99,11 @@ func prViewCmd() *cobra.Command { return cmd } -// prHeadLabel is a display label for a PR's head: the branch name, or the pull -// ref when the PR has no head branch (AGit flow / deleted branch). -func prHeadLabel(h forges.PRBranch) string { - if h.Ref != "" { - return h.Ref - } - return h.PullRef -} - func printPRDetails(pr *forges.PullRequest) { _, _ = fmt.Fprintf(os.Stdout, "#%d %s\n", pr.Number, output.Sanitize(pr.Title)) _, _ = fmt.Fprintf(os.Stdout, "State: %s\n", pr.State) _, _ = fmt.Fprintf(os.Stdout, "Author: %s\n", output.Sanitize(pr.Author.Login)) - _, _ = fmt.Fprintf(os.Stdout, "Branch: %s -> %s\n", prHeadLabel(pr.Head), pr.Base.Ref) + _, _ = fmt.Fprintf(os.Stdout, "Branch: %s -> %s\n", pr.Head.Ref, pr.Base.Ref) if pr.Draft { _, _ = fmt.Fprintln(os.Stdout, "Draft: yes") @@ -220,7 +211,7 @@ func prListCmd() *cobra.Command { strconv.Itoa(pr.Number), title, output.Sanitize(pr.Author.Login), - prHeadLabel(pr.Head), + pr.Head.Ref, pr.UpdatedAt.Format("2006-01-02"), } } @@ -610,31 +601,26 @@ The argument can be a PR number or a full URL: return fmt.Errorf("getting PR #%d: %w", number, err) } - head := pr.Head - - // A PR with a head branch is fetched by branch name (from the fork - // remote when it lives on a fork). A branchless PR (e.g. AGit flow) - // has no branch to fetch, so fall back to its pull ref, which the - // forge exposes on the base repo. - if head.Ref != "" { - localBranch := flagBranch - if localBranch == "" { - localBranch = head.Ref - } - if head.Fork != nil { - return checkoutForkPR(ctx, domain, pr, head.Ref, localBranch, flagRemoteName, flagDetach, flagForce) - } - return checkoutSameRepoPR(ctx, head.Ref, localBranch, flagDetach, flagForce) - } + // remoteRef is usually the PR's head branch name, but Gitea/Forgejo + // report a refs/pull//head ref when there is no head branch + // (AGit-flow PRs, or PRs whose branch was deleted). Such a ref only + // lives on the base repo. + remoteRef := pr.Head.Ref - if head.PullRef == "" { - return fmt.Errorf("PR #%d has no head branch to check out", pr.Number) - } + // localBranch is what we'll name the local branch (defaults to the + // head branch name, or pr- when only a pull ref is known). localBranch := flagBranch if localBranch == "" { - localBranch = fmt.Sprintf("pr-%d", pr.Number) + localBranch = defaultLocalBranch(pr) } - return checkoutSameRepoPR(ctx, head.PullRef, localBranch, flagDetach, flagForce) + + // A pull ref isn't present on the fork remote, only on origin, so + // route it through the same-repo path even for fork PRs. + if pr.Head.Fork != nil && !isFullRef(remoteRef) { + return checkoutForkPR(ctx, domain, pr, remoteRef, localBranch, flagRemoteName, flagDetach, flagForce) + } + + return checkoutSameRepoPR(ctx, remoteRef, localBranch, flagDetach, flagForce) }, } @@ -719,12 +705,28 @@ func remoteMatches(existingURL, wantURL string) bool { return domain == wantDomain && owner == wantOwner && repo == wantRepo } +// isFullRef reports whether ref is a fully-qualified git ref (e.g. +// refs/pull//head) rather than a bare branch name. +func isFullRef(ref string) bool { + return strings.HasPrefix(ref, "refs/") +} + +// defaultLocalBranch picks the local branch name for a checked-out PR. It uses +// the head branch name when available, but falls back to pr- when only +// a pull ref is known (Gitea/Forgejo PRs with no head branch, e.g. AGit flow). +func defaultLocalBranch(pr *forges.PullRequest) string { + if isFullRef(pr.Head.Ref) { + return fmt.Sprintf("pr-%d", pr.Number) + } + return pr.Head.Ref +} + func gitCheckout(ctx context.Context, remote, remoteRef, localBranch string, detach, force bool) error { - // A bare branch name needs the refs/heads/ prefix; an already-qualified ref - // (e.g. a refs/pull//head pull ref) is fetched as-is. The remote-tracking - // ref is named after localBranch so a pull ref doesn't leak into refs/remotes/. + // A bare branch name needs the refs/heads/ prefix; a full ref (e.g. + // refs/pull//head) is fetched as-is. The remote-tracking ref is named + // after localBranch so a pull ref doesn't leak into refs/remotes/. src := remoteRef - if !strings.HasPrefix(src, "refs/") { + if !isFullRef(src) { src = "refs/heads/" + src } refspec := fmt.Sprintf("+%s:refs/remotes/%s/%s", src, remote, localBranch) diff --git a/internal/cli/pr_checkout_test.go b/internal/cli/pr_checkout_test.go index 8d02be0..933a67f 100644 --- a/internal/cli/pr_checkout_test.go +++ b/internal/cli/pr_checkout_test.go @@ -217,8 +217,8 @@ func pushToRemoteRef(t *testing.T, repoDir, remoteName, ref string) { } } -// TestPRCheckoutPullRef covers branchless PRs (e.g. Gitea/Forgejo AGit flow): -// Head.Ref is empty and only Head.PullRef is set. The pull ref must be fetched +// TestPRCheckoutPullRef covers Gitea/Forgejo PRs whose head.ref is a full +// refs/pull//head ref rather than a branch name. The ref must be fetched // as-is (not under refs/heads/) and the local branch falls back to pr-. func TestPRCheckoutPullRef(t *testing.T) { if testing.Short() { @@ -240,7 +240,7 @@ func TestPRCheckoutPullRef(t *testing.T) { pr := &forges.PullRequest{ Number: 42, - Head: forges.PRBranch{PullRef: "refs/pull/42/head", SHA: "abc123"}, + Head: forges.PRBranch{Ref: "refs/pull/42/head", SHA: "abc123"}, } resolve.SetTestForge( &mockForge{prService: &mockPRService{pr: pr}}, diff --git a/types.go b/types.go index ce59c0b..bc04c18 100644 --- a/types.go +++ b/types.go @@ -240,16 +240,9 @@ type ForkInfo struct { // PRBranch holds branch info including the repository it belongs to. // For same-repo PRs, Fork is nil. For fork PRs, Fork points to the source repo. type PRBranch struct { - Ref string `json:"ref"` // branch name; empty if the PR has no head branch + Ref string `json:"ref"` // branch name SHA string `json:"sha,omitempty"` // commit SHA Fork *ForkInfo `json:"fork,omitempty"` // nil if same repo as target - // PullRef is a read-only ref on the base repo that always resolves the head - // commit: refs/pull//head on GitHub and Gitea/Forgejo, - // refs/merge-requests//head on GitLab. It is the only way to fetch a - // branchless PR (e.g. one created via AGit flow, or whose head branch was - // deleted), so checkout uses it as a fallback when Ref is empty. Empty when - // the forge exposes no such ref (e.g. Bitbucket Cloud). - PullRef string `json:"pull_ref,omitempty"` } // PullRequest holds normalized metadata about a pull request (or merge request). From 060dc2bc571c180c212c439c2674dca972beeafc Mon Sep 17 00:00:00 2001 From: Markus Unterwaditzer Date: Mon, 8 Jun 2026 01:03:28 +0200 Subject: [PATCH 4/4] fix doccomment --- types.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/types.go b/types.go index bc04c18..8e55e19 100644 --- a/types.go +++ b/types.go @@ -240,7 +240,7 @@ type ForkInfo struct { // PRBranch holds branch info including the repository it belongs to. // For same-repo PRs, Fork is nil. For fork PRs, Fork points to the source repo. type PRBranch struct { - Ref string `json:"ref"` // branch name + Ref string `json:"ref"` // branch name, or a full ref (e.g. refs/pull//head) for a branchless PR SHA string `json:"sha,omitempty"` // commit SHA Fork *ForkInfo `json:"fork,omitempty"` // nil if same repo as target }