From 7fc35fd47d9482068a5c7666c5c67d4e5e001fce Mon Sep 17 00:00:00 2001 From: Frederick Zhang Date: Fri, 14 Jun 2024 15:10:53 +1000 Subject: [PATCH] Only find PRs w/ branch..merge if push.default = upstream/tracking When push.default is not 'upstream' / 'tracking' (or 'nothing'), we can expect local and remote branch names to be the same and solely rely on @{push} or RemoteURL. This fixes the wrong error message 'no pull requests found for branch ""' when the local branch is not pushed in the push.default = simple / current and upstream = setup. --- pkg/cmd/pr/shared/finder.go | 9 ++++-- pkg/cmd/pr/shared/finder_test.go | 51 +++++++++++++++++++++++++++++++- pkg/cmd/pr/status/status.go | 3 +- pkg/cmd/pr/status/status_test.go | 34 ++++++++++++++++++++- 4 files changed, 92 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index 23aee371c..6e36b4dd6 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -38,6 +38,7 @@ type finder struct { remotesFn func() (remotes.Remotes, error) httpClient func() (*http.Client, error) branchConfig func(string) git.BranchConfig + pushDefault func() (string, error) progress progressIndicator repo ghrepo.Interface @@ -57,7 +58,10 @@ func NewFinder(factory *cmdutil.Factory) PRFinder { branchFn: factory.Branch, remotesFn: factory.Remotes, httpClient: factory.HttpClient, - progress: factory.IOStreams, + pushDefault: func() (string, error) { + return factory.GitClient.Config(context.Background(), "push.default") + }, + progress: factory.IOStreams, branchConfig: func(s string) git.BranchConfig { return factory.GitClient.ReadBranchConfig(context.Background(), s) }, @@ -263,7 +267,8 @@ func (f *finder) parseCurrentBranch() (string, int, error) { if gitRemoteRepo != nil { if branchConfig.Push != "" { prHeadRef = strings.TrimPrefix(branchConfig.Push, branchConfig.RemoteName+"/") - } else if strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { + } else if pushDefault, _ := f.pushDefault(); (pushDefault == "upstream" || pushDefault == "tracking") && + strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { prHeadRef = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") } // prepend `OWNER:` if this branch is pushed to a fork diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index 88ee170a0..2c34833c8 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -18,6 +18,7 @@ func TestFind(t *testing.T) { baseRepoFn func() (ghrepo.Interface, error) branchFn func() (string, error) branchConfig func(string) git.BranchConfig + pushDefault func() (string, error) remotesFn func() (context.Remotes, error) selector string fields []string @@ -330,6 +331,7 @@ func TestFind(t *testing.T) { c.Push = "origin/blue-upstream-berries" return }, + pushDefault: func() (string, error) { return "upstream", nil }, remotesFn: func() (context.Remotes, error) { return context.Remotes{{ Remote: &git.Remote{Name: "origin"}, @@ -373,7 +375,52 @@ func TestFind(t *testing.T) { c.RemoteURL = u return }, - remotesFn: nil, + pushDefault: func() (string, error) { return "upstream", nil }, + remotesFn: nil, + }, + httpStub: func(r *httpmock.Registry) { + r.Register( + httpmock.GraphQL(`query PullRequestForBranch\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "pullRequests":{"nodes":[ + { + "number": 13, + "state": "OPEN", + "baseRefName": "main", + "headRefName": "blue-upstream-berries", + "isCrossRepository": true, + "headRepositoryOwner": {"login":"UPSTREAMOWNER"} + } + ]} + }}}`)) + }, + wantPR: 13, + wantRepo: "https://github.com/OWNER/REPO", + }, + { + name: "current branch with tracking (deprecated synonym of upstream) configuration", + args: args{ + selector: "", + fields: []string{"id", "number"}, + baseRepoFn: func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("OWNER/REPO") + }, + branchFn: func() (string, error) { + return "blueberries", nil + }, + branchConfig: func(branch string) (c git.BranchConfig) { + c.MergeRef = "refs/heads/blue-upstream-berries" + c.RemoteName = "origin" + c.Push = "origin/blue-upstream-berries" + return + }, + pushDefault: func() (string, error) { return "tracking", nil }, + remotesFn: func() (context.Remotes, error) { + return context.Remotes{{ + Remote: &git.Remote{Name: "origin"}, + Repo: ghrepo.New("UPSTREAMOWNER", "REPO"), + }}, nil + }, }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -407,6 +454,7 @@ func TestFind(t *testing.T) { }, branchConfig: func(branch string) (c git.BranchConfig) { c.RemoteName = "origin" + c.Push = "origin/blueberries" return }, remotesFn: func() (context.Remotes, error) { @@ -539,6 +587,7 @@ func TestFind(t *testing.T) { baseRepoFn: tt.args.baseRepoFn, branchFn: tt.args.branchFn, branchConfig: tt.args.branchConfig, + pushDefault: tt.args.pushDefault, remotesFn: tt.args.remotesFn, } diff --git a/pkg/cmd/pr/status/status.go b/pkg/cmd/pr/status/status.go index ebba3313c..1b6da8427 100644 --- a/pkg/cmd/pr/status/status.go +++ b/pkg/cmd/pr/status/status.go @@ -211,7 +211,8 @@ func prSelectorForCurrentBranch(gitClient *git.Client, baseRepo ghrepo.Interface if branchOwner != "" { if branchConfig.Push != "" { selector = strings.TrimPrefix(branchConfig.Push, branchConfig.RemoteName+"/") - } else if strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { + } else if pushDefault, _ := gitClient.Config(context.Background(), "push.default"); (pushDefault == "upstream" || pushDefault == "tracking") && + strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { selector = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") } // prepend `OWNER:` if this branch is pushed to a fork diff --git a/pkg/cmd/pr/status/status_test.go b/pkg/cmd/pr/status/status_test.go index 9fbe484c5..d09abf5b9 100644 --- a/pkg/cmd/pr/status/status_test.go +++ b/pkg/cmd/pr/status/status_test.go @@ -375,7 +375,7 @@ Requesting a code review from you } } -func Test_prSelectorForCurrentBranch(t *testing.T) { +func Test_prSelectorForCurrentBranchPushDefaultUpstream(t *testing.T) { rs, cleanup := run.Stub() defer cleanup(t) @@ -384,6 +384,38 @@ func Test_prSelectorForCurrentBranch(t *testing.T) { branch.Frederick888/main.merge refs/heads/main `)) rs.Register(`git rev-parse --verify --quiet --abbrev-ref Frederick888/main@\{push\}`, 1, "") + rs.Register(`git config push\.default`, 0, "upstream") + + repo := ghrepo.NewWithHost("octocat", "playground", "github.com") + rem := context.Remotes{ + &context.Remote{ + Remote: &git.Remote{Name: "origin"}, + Repo: repo, + }, + } + gitClient := &git.Client{GitPath: "some/path/git"} + prNum, headRef, err := prSelectorForCurrentBranch(gitClient, repo, "Frederick888/main", rem) + if err != nil { + t.Fatalf("prSelectorForCurrentBranch error: %v", err) + } + if prNum != 0 { + t.Errorf("expected prNum to be 0, got %q", prNum) + } + if headRef != "Frederick888:main" { + t.Errorf("expected headRef to be \"Frederick888:main\", got %q", headRef) + } +} + +func Test_prSelectorForCurrentBranchPushDefaultTracking(t *testing.T) { + rs, cleanup := run.Stub() + defer cleanup(t) + + rs.Register(`git config --get-regexp \^branch\\.`, 0, heredoc.Doc(` + branch.Frederick888/main.remote git@github.com:Frederick888/playground.git + branch.Frederick888/main.merge refs/heads/main + `)) + rs.Register(`git rev-parse --verify --quiet --abbrev-ref Frederick888/main@\{push\}`, 1, "") + rs.Register(`git config push\.default`, 0, "tracking") repo := ghrepo.NewWithHost("octocat", "playground", "github.com") rem := context.Remotes{