Implement forge pr view for current branch#109
Conversation
Fix git-pkgs#100 There is no unified way in the API packages to query a PR by its head's owner:branch. Gitea doesn't provide this API at all, and just returns unfiltered PRs, GitHub does it just fine. If we had that consistently, we would be done. Instead we read from a bunch of caches for figuring out the PR number: 1. For compat with gh CLI checked out PRs, we parse `refs/pull/XYZ` to extract the PR number 2. We store it in our own cache 3. If that fails, we actually use the API 4. We write back the result into our own cache The cache is also populated during `forge pr checkout`. This way `forge pr view --web` is as fast as it can be. It's faster than `gh pr view --web`. Even if Gitea had the right API, the API calls to Codeberg are atrociously slow in general so I would like that cache no matter what.
|
I think i'd also be fine to do either of those things:
though if option 2 is preferred, I think there needs to be a better way to get the PR number back out of |
andrew
left a comment
There was a problem hiding this comment.
The cache-in-git-config approach is nice, and the gh refs/pull/N/head fallback is a good touch.
One logic issue in findPRForCurrentBranch:
if len(matching) < 1 { ... }
if len(matching) > 1 {
return 0, fmt.Errorf("multiple pull request found for branch %q, might be a bug?", localBranch)
}
for _, pr := range matching {
if pr.State == "open" {
_ = storePRForBranch(ctx, localBranch, pr.Number)
return pr.Number, nil
}
}
return 0, fmt.Errorf("no matching pull request found for branch %q", localBranch)After the two length checks, len(matching) == 1. If that single PR is closed or merged, the loop falls through and we report "no matching pull request found" — but there is one. Sitting on a branch whose PR was just merged and being told there's no PR is confusing.
The > 1 case also isn't really a bug — a branch reused for a closed PR and then a new open one is normal. gh handles it by preferring the open PR.
I'd suggest: among matching, return the open one if any, otherwise return matching[0]. Only cache when the result is open (so a stale closed PR doesn't stick).
Nits:
- "multiple pull request" → "multiple pull requests"
storePRForBranchinprCheckoutCmdruns before the checkout itself, so a failed checkout still writes the cache entry. Harmless given the key is the branch name, just noting it.
andrew
left a comment
There was a problem hiding this comment.
Thanks for the iteration — the closed-PR fallback at internal/cli/pr.go:822-823 reads correctly now, and gating the cache write on State == "open" is the right call.
One blocker before merging, plus a few nits.
Blocker
No test covers the closed-PR path that this round was meant to fix. TestFindPRForCurrentBranch only exercises the open case, so the very behavior that regressed last time isn't pinned. A couple of additional cases would do it:
- A list containing one
closedand oneopenPR — assert the open one wins and gets cached. - A list containing only a
closedPR — assert it's returned and thatloadPRForBranchfinds nothing in the cache afterwards.
Nits (not blocking)
Limit: 100with no pagination atinternal/cli/pr.go:789. Given the PR body notes Gitea ignores theHeadfilter, a repo with >100 PRs will silently miss the match on a fresh checkout. The cache hides this in normal use, but worth at least aTODO.OwnerForBranchininternal/resolve/resolve.go:219,228usesexec.Commandrather thanexec.CommandContext, inconsistent with the rest of the PR. Practical risk is nil, just drift.matching[0]is non-deterministic when several closed PRs share the head ref. Picking the most recently updated would be nicer; not load-bearing.- The
branch.<name>.mergefallback inloadPRForBranchis fine because the regex rejectsrefs/heads/*, but a one-line comment to that effect would save the next reader a double-take.
Otherwise this is ready.
Fix #100
There is no unified way in the API packages to query a PR by its head's
owner:branch. Gitea doesn't provide this API at all, and just returns
unfiltered PRs, GitHub does it just fine. If we had that consistently,
we would be done.
Instead we read from a bunch of caches for figuring out the PR number:
refs/pull/XYZtoextract the PR number
The cache is also populated during
forge pr checkout.This way
forge pr view --webis as fast as it can be. It's faster thangh pr view --web.Even if Gitea had the right API, the API calls to Codeberg are
atrociously slow in general so I would like that cache no matter what.