Skip to content

Implement forge pr view for current branch#109

Open
untitaker wants to merge 2 commits into
git-pkgs:mainfrom
untitaker:pr-view
Open

Implement forge pr view for current branch#109
untitaker wants to merge 2 commits into
git-pkgs:mainfrom
untitaker:pr-view

Conversation

@untitaker
Copy link
Copy Markdown
Contributor

@untitaker untitaker commented May 30, 2026

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:

  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 (which is slow, and for Gitea, also wrong because the results are truncated arbitrarily)
  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.

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.
@untitaker untitaker marked this pull request as ready for review May 30, 2026 04:59
@untitaker
Copy link
Copy Markdown
Contributor Author

I think i'd also be fine to do either of those things:

  1. remove API usage entirely, relying entirely on local caches
  2. implement all of this in a wrapper script since it's more about git workflows than integrating with forges

though if option 2 is preferred, I think there needs to be a better way to get the PR number back out of forge pr checkout (or the URL parsing stuff needs to be exposed as commands)

Copy link
Copy Markdown
Contributor

@andrew andrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
  • storePRForBranch in prCheckoutCmd runs before the checkout itself, so a failed checkout still writes the cache entry. Harmless given the key is the branch name, just noting it.

Copy link
Copy Markdown
Contributor

@andrew andrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 closed and one open PR — assert the open one wins and gets cached.
  • A list containing only a closed PR — assert it's returned and that loadPRForBranch finds nothing in the cache afterwards.

Nits (not blocking)

  • Limit: 100 with no pagination at internal/cli/pr.go:789. Given the PR body notes Gitea ignores the Head filter, 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 a TODO.
  • OwnerForBranch in internal/resolve/resolve.go:219,228 uses exec.Command rather than exec.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>.merge fallback in loadPRForBranch is fine because the regex rejects refs/heads/*, but a one-line comment to that effect would save the next reader a double-take.

Otherwise this is ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support for forge pr view without PR number

2 participants