diff --git a/internal/app/checks.go b/internal/app/checks.go index 1fda2449..61168f62 100644 --- a/internal/app/checks.go +++ b/internal/app/checks.go @@ -29,7 +29,7 @@ tea exits with 8 while checks are pending and with 1 when a check failed, like g if interval <= 0 || timeout <= 0 { return clierr.Wrap(clierr.Usage, errors.New("--interval and --timeout must be positive")) } - t, n, err := a.pullSelector(cmd, args) + t, n, err := a.pullSelectorAny(cmd, args) if err != nil { return err } diff --git a/internal/app/pull_clean.go b/internal/app/pull_clean.go index 4349a79d..4bbff505 100644 --- a/internal/app/pull_clean.go +++ b/internal/app/pull_clean.go @@ -20,7 +20,7 @@ import ( func (a *application) pullCleanCommand() *cobra.Command { var ignoreSHA bool cmd := &cobra.Command{Use: "clean [ | | ]", Short: "Delete the branches of a merged or closed pull request", Annotations: pullArgumentsAnnotation(map[string]string{"tea.confirm": "true"}), Args: usageArgs(cobra.MaximumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) error { - target, n, err := a.pullSelector(cmd, args) + target, n, err := a.pullSelectorAny(cmd, args) if err != nil { return err } diff --git a/internal/app/pull_closed_lookup_test.go b/internal/app/pull_closed_lookup_test.go new file mode 100644 index 00000000..305ded3a --- /dev/null +++ b/internal/app/pull_closed_lookup_test.go @@ -0,0 +1,133 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package app + +import ( + "context" + "fmt" + "net/http" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// closedLookupServer has one open pull request, for feat, and closed ones for gone, listed +// newest first like sort=recentupdate. +func closedLookupServer(t *testing.T, requests *[]string) string { + t.Helper() + server := newAuthenticatedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + *requests = append(*requests, r.Method+" "+r.URL.Path+"?state="+r.URL.Query().Get("state")) + if number, ok := strings.CutPrefix(r.URL.Path, "/api/v1/repos/alice/project/pulls/"); ok { + _, _ = fmt.Fprintf(w, `{"number":%s,"title":"Old","state":"closed","merged":true,"head":{"ref":"gone","sha":"abc","repo":{"full_name":"alice/project"}}}`, number) + return + } + if r.URL.Path == "/api/v1/repos/alice/project" { + _, _ = fmt.Fprint(w, `{"full_name":"alice/project","default_branch":"main"}`) + return + } + if r.URL.Path != "/api/v1/repos/alice/project/pulls" { + http.NotFound(w, r) + return + } + if r.URL.Query().Get("state") == "open" { + writeFirstPage(w, r, `[{"number":3,"head":{"ref":"feat","repo":{"full_name":"alice/project"}}}]`) + return + } + require.Equal(t, "closed", r.URL.Query().Get("state")) + require.Equal(t, "recentupdate", r.URL.Query().Get("sort")) + writeFirstPage(w, r, `[ + {"number":40,"head":{"ref":"gone","repo":{"full_name":"bob/project"}}}, + {"number":31,"head":{"ref":"gone","repo":{"full_name":"alice/project"}}}, + {"number":30,"head":{"ref":"gone","repo":{"full_name":"alice/project"}}} + ]`) + })) + t.Cleanup(server.Close) + return server.URL +} + +func TestPullSelectorAnyFallsBackToTheNewestClosedPull(t *testing.T) { + var requests []string + serverURL := closedLookupServer(t, &requests) + for _, tc := range []struct { + name string + arg string + want int64 + err string + }{ + {"open one wins", "feat", 3, ""}, + {"newest closed", "gone", 40, ""}, + {"owner qualified", "alice:gone", 31, ""}, + {"unknown", "nothing", 0, `no open pull request for branch "nothing"`}, + } { + t.Run(tc.name, func(t *testing.T) { + a, _ := refApplicationWith(t, serverURL, nil) + _, n, err := a.pullSelectorAny(commandContext(), []string{tc.arg}) + if tc.err != "" { + require.ErrorContains(t, err, tc.err) + return + } + require.NoError(t, err) + require.Equal(t, tc.want, n) + }) + } + // Commands that need an open pull request still do not see closed ones. + requests = nil + a, _ := refApplicationWith(t, serverURL, nil) + _, _, err := a.pullSelector(commandContext(), []string{"gone"}) + require.ErrorContains(t, err, `no open pull request for branch "gone"`) + require.NotContains(t, strings.Join(requests, "\n"), "state=closed") +} + +func TestPullSelectorAnyFallsBackForTheCheckedOutBranch(t *testing.T) { + var requests []string + serverURL := closedLookupServer(t, &requests) + a, _ := refApplicationWith(t, serverURL, map[string]string{"symbolic-ref --quiet HEAD": "gone"}) + // The fork's newer pull request for a branch named like the local one is not this repository's. + _, n, err := a.pullSelectorAny(commandContext(), nil) + require.NoError(t, err) + require.Equal(t, int64(31), n) + _, _, err = a.pullSelector(commandContext(), nil) + require.ErrorContains(t, err, `no open pull request for branch "gone"`) +} + +func TestPullCommandsFindClosedPullsByBranch(t *testing.T) { + for _, tc := range []struct { + name string + args []string + err string + }{ + {"view", []string{"pr", "view", "alice:gone", "--json=number"}, ""}, + {"checks", []string{"pr", "checks", "alice:gone", "--web", "--no-input"}, ""}, + {"reopen", []string{"pr", "reopen", "alice:gone"}, "already merged"}, + {"clean", []string{"pr", "clean", "alice:gone", "--no-input"}, ""}, + } { + t.Run(tc.name, func(t *testing.T) { + var requests []string + serverURL := closedLookupServer(t, &requests) + deps, _, _ := serverDeps(t, serverURL) + deps.Getenv = serverEnv(serverURL, map[string]string{"TEA_REPO": "alice/project"}) + deps.Git = &fakeGit{results: map[string]string{}} + err := Execute(context.Background(), deps, tc.args) + if tc.err != "" { + require.ErrorContains(t, err, tc.err) + } + if err != nil { + require.NotContains(t, err.Error(), "no open pull request") + } + require.Contains(t, requests, "GET /api/v1/repos/alice/project/pulls/31?state=", "err: %v", err) + }) + } +} + +func TestPullMergeKeepsOpenOnlyLookup(t *testing.T) { + var requests []string + serverURL := closedLookupServer(t, &requests) + deps, _, _ := serverDeps(t, serverURL) + deps.Getenv = serverEnv(serverURL, map[string]string{"TEA_REPO": "alice/project"}) + deps.Git = &fakeGit{results: map[string]string{}} + err := Execute(context.Background(), deps, []string{"pr", "merge", "gone", "--yes", "--no-input"}) + require.ErrorContains(t, err, `no open pull request for branch "gone"`) + require.NotContains(t, strings.Join(requests, "\n"), "state=closed") +} diff --git a/internal/app/pull_mutations.go b/internal/app/pull_mutations.go index 85bd185a..83abbfeb 100644 --- a/internal/app/pull_mutations.go +++ b/internal/app/pull_mutations.go @@ -79,7 +79,7 @@ func (a *application) pullStateCommand(closing bool) *cobra.Command { var comment string var deleteBranch bool cmd := &cobra.Command{Use: verb + " [ | | ]", Short: titleCase(verb) + " a pull request", Annotations: pullArgumentsAnnotation(nil), Args: usageArgs(cobra.MaximumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) error { - t, n, err := a.pullSelector(cmd, args) + t, n, err := a.pullSelectorAny(cmd, args) if err != nil { return err } @@ -176,7 +176,7 @@ func (a *application) pullDiffCommand() *cobra.Command { On a terminal the diff is colored and paged. Piped, it is written unchanged, so that tea pr diff 13 | git apply works.`, Annotations: pullArgumentsAnnotation(textOutput("diff returns patch text")), Args: usageArgs(cobra.MaximumNArgs(1)), RunE: func(cmd *cobra.Command, args []string) error { - t, n, err := a.pullSelector(cmd, args) + t, n, err := a.pullSelectorAny(cmd, args) if err != nil { return err } diff --git a/internal/app/pulls.go b/internal/app/pulls.go index ffa6d3d3..e78ee2e5 100644 --- a/internal/app/pulls.go +++ b/internal/app/pulls.go @@ -150,7 +150,7 @@ func (a *application) pullViewCommand() *cobra.Command { if comments && a.options.json != "" { return clierr.Wrap(clierr.Usage, errors.New("--comments cannot be combined with --json or --jq; use tea pr comment list --json or tea pr review list --json")) } - t, n, err := a.pullSelector(cmd, args) + t, n, err := a.pullSelectorAny(cmd, args) if err != nil { return err } diff --git a/internal/app/ref.go b/internal/app/ref.go index bce86f51..1d3506f9 100644 --- a/internal/app/ref.go +++ b/internal/app/ref.go @@ -196,13 +196,26 @@ func (a *application) sameRepository(first, second string) bool { // pullSelector resolves the pull request a pr command addresses: N, #N, OWNER/REPO#N, a pull // request URL, a head branch, OWNER:BRANCH, or, without an argument, the pull request of the // checked-out branch. +// +// A branch names an open pull request only; commands that mutate open pull requests use it +// as is, and the others use pullSelectorAny. func (a *application) pullSelector(cmd *cobra.Command, args []string) (target, int64, error) { + return a.selectPull(cmd, args, false) +} + +// pullSelectorAny is pullSelector that, when no open pull request has the branch, falls back +// to the most recently updated closed or merged one, like gh. +func (a *application) pullSelectorAny(cmd *cobra.Command, args []string) (target, int64, error) { + return a.selectPull(cmd, args, true) +} + +func (a *application) selectPull(cmd *cobra.Command, args []string, closed bool) (target, int64, error) { if len(args) == 0 || args[0] == "" { // The checked-out branch says nothing about a repository chosen with --repo. if a.options.repo != "" { return target{}, 0, clierr.Wrap(clierr.Usage, errors.New("pass a pull request number, URL or branch with --repo")) } - return a.currentPull(cmd.Context()) + return a.currentPull(cmd.Context(), closed) } raw := args[0] if _, err := parseRef(raw, pullRef); !errors.Is(err, errNotRef) { @@ -216,7 +229,8 @@ func (a *application) pullSelector(cmd *cobra.Command, args []string) (target, i if !qualified { owner, branch = "", raw } - pulls, err := (services.Pulls{Client: t.client, Repository: t.Repo}).ListOpen(cmd.Context()) + service := services.Pulls{Client: t.client, Repository: t.Repo} + pulls, err := service.ListOpen(cmd.Context()) if err != nil { return t, 0, err } @@ -224,6 +238,14 @@ func (a *application) pullSelector(cmd *cobra.Command, args []string) (target, i if err != nil { return t, 0, err } + if n == 0 && closed { + if n, err = newestClosedForBranch(cmd.Context(), service, owner, branch, false, t.Repo); err != nil { + return t, 0, err + } + if n > 0 { + return t, n, nil + } + } if n == 0 { return t, 0, fmt.Errorf("no open pull request for branch %q in %s; pass a number, URL or branch", raw, t.Repo) } @@ -233,8 +255,9 @@ func (a *application) pullSelector(cmd *cobra.Command, args []string) (target, i // currentPull finds the pull request of the checked-out branch: the one pr checkout recorded // while it is open, else the open pull request whose head is the branch's upstream (and the // upstream remote's owner), else one of this repository whose head has the local branch's name, else N from a pr/N -// branch, else the recorded pull request even though it is no longer open. -func (a *application) currentPull(ctx context.Context) (target, int64, error) { +// branch, else the recorded pull request even though it is no longer open. With closed, the +// newest closed or merged pull request for the branch comes last. +func (a *application) currentPull(ctx context.Context, closed bool) (target, int64, error) { branch, err := git.CurrentBranchConfig(ctx, a.gitProbe(), a.options.cwd) if err != nil { return target{}, 0, clierr.Wrap(clierr.Usage, errors.New("not on a branch; pass a pull request number, URL or branch")) @@ -256,6 +279,7 @@ func (a *application) currentPull(ctx context.Context) (target, int64, error) { if err != nil { return t, 0, err } + upstreamOwner := "" if branch.Merge != "" { owner := "" if remoteURL, err := a.gitProbe().Output(ctx, a.options.cwd, false, "remote", "get-url", branch.Remote); err == nil { @@ -266,6 +290,7 @@ func (a *application) currentPull(ctx context.Context) (target, int64, error) { if n, err := pullForBranch(pulls, owner, branch.Merge, t.Repo); owner != "" && (n > 0 || err != nil) { return t, n, err } + upstreamOwner = owner } // A bare branch name only identifies the repository's own pull requests: a contributor's // fork may use main or fix as its head branch too. @@ -281,9 +306,34 @@ func (a *application) currentPull(ctx context.Context) (target, int64, error) { if recorded > 0 { return t, recorded, nil } + if closed { + // Same matching as above: the upstream's owner first, then the repository's own branches. + if upstreamOwner != "" { + if n, err := newestClosedForBranch(ctx, service, upstreamOwner, branch.Merge, false, t.Repo); n > 0 || err != nil { + return t, n, err + } + } + if n, err := newestClosedForBranch(ctx, service, "", branch.Name, true, t.Repo); n > 0 || err != nil { + return t, n, err + } + } return t, 0, fmt.Errorf("no open pull request for branch %q in %s; pass a number, URL or branch", branch.Name, t.Repo) } +// newestClosedForBranch returns the number of the most recently updated closed or merged pull +// request whose head is branch in a repository of owner (any owner when empty), or 0. With own, +// only pull requests from the repository itself count, as a fork may reuse the branch name. +func newestClosedForBranch(ctx context.Context, service services.Pulls, owner, branch string, own bool, repo string) (int64, error) { + pull, found, err := service.NewestClosed(ctx, func(pull services.Pull) bool { + headOwner, _, _ := strings.Cut(pull.HeadRepo, "/") + return pull.Head == branch && (owner == "" || strings.EqualFold(headOwner, owner)) && (!own || strings.EqualFold(pull.HeadRepo, repo)) + }) + if err != nil || !found { + return 0, err + } + return pull.Number, nil +} + // remoteOwns reports whether the Git remote named remote points at the target's repository. func (a *application) remoteOwns(ctx context.Context, t target, remote string) bool { raw, err := a.gitProbe().Output(ctx, a.options.cwd, false, "remote", "get-url", remote) diff --git a/internal/app/ref_test.go b/internal/app/ref_test.go index ef70dc93..ffafe458 100644 --- a/internal/app/ref_test.go +++ b/internal/app/ref_test.go @@ -82,19 +82,25 @@ func refApplication(t *testing.T, gitResults map[string]string) (*application, s ]`) })) t.Cleanup(server.Close) + return refApplicationWith(t, server.URL, gitResults) +} + +// refApplicationWith is refApplication for an existing server. +func refApplicationWith(t *testing.T, serverURL string, gitResults map[string]string) (*application, string) { + t.Helper() deps, _, _ := testDeps(t) - deps.Getenv = serverEnv(server.URL, map[string]string{"TEA_REPO": "alice/project"}) + deps.Getenv = serverEnv(serverURL, map[string]string{"TEA_REPO": "alice/project"}) results := map[string]string{ - "remote get-url origin": server.URL + "/alice/project.git", - "remote get-url pulls/bob/project": server.URL + "/bob/project.git", - "remote get-url fork": server.URL + "/bob/project.git", - "remote get-url pulls/carol/project": server.URL + "/carol/project.git", + "remote get-url origin": serverURL + "/alice/project.git", + "remote get-url pulls/bob/project": serverURL + "/bob/project.git", + "remote get-url fork": serverURL + "/bob/project.git", + "remote get-url pulls/carol/project": serverURL + "/carol/project.git", } for key, value := range gitResults { results[key] = value } deps.Git = &fakeGit{results: results} - return &application{deps: deps}, server.URL + return &application{deps: deps}, serverURL } func commandContext() *cobra.Command { diff --git a/internal/app/status.go b/internal/app/status.go index 7db61915..7fac6510 100644 --- a/internal/app/status.go +++ b/internal/app/status.go @@ -96,7 +96,7 @@ func (a *application) currentPullStatus(cmd *cobra.Command) (*services.Pull, pul if err != nil { return nil, details, "" } - t, n, err := a.pullSelector(cmd, nil) + t, n, err := a.pullSelectorAny(cmd, nil) if err != nil { return nil, details, branch } diff --git a/internal/services/pulls.go b/internal/services/pulls.go index f291655c..78c0bb11 100644 --- a/internal/services/pulls.go +++ b/internal/services/pulls.go @@ -276,6 +276,26 @@ func (s Pulls) ListOpen(ctx context.Context) ([]Pull, error) { }) } +// NewestClosed returns the most recently updated closed or merged pull request that keep +// accepts, and whether there is one. It reads pages newest first and stops at the first match. +func (s Pulls) NewestClosed(ctx context.Context, keep func(Pull) bool) (Pull, bool, error) { + for page := 1; page <= maxListPages; page++ { + items, _, err := s.List(ctx, PullListOptions{State: "closed", Sort: "recentupdate", Limit: 50, Page: page}) + if err != nil { + return Pull{}, false, err + } + if len(items) == 0 { + return Pull{}, false, nil + } + for _, item := range items { + if keep(item) { + return item, true, nil + } + } + } + return Pull{}, false, nil +} + // Create opens a pull request. func (s Pulls) Create(ctx context.Context, opts PullOptions) (Pull, error) { // TODO(sdk): CreatePullRequestOption lacks allow_maintainer_edit.