From 0179381efdc45980596aae7a2147ce13440ec483 Mon Sep 17 00:00:00 2001 From: Frederick Zhang Date: Fri, 14 Jun 2024 14:43:38 +1000 Subject: [PATCH 01/23] Find PRs using @{push} When using a push.default = current central workflow [1], we should use @{push} instead to locate the remote branch. In fact, @{push} covers most cases in push.default = upstream too. The branch..merge is probably only needed when using RemoteURL and different remote / local branch names. [1] https://github.com/tpope/vim-fugitive/issues/1172#issuecomment-522301607 --- git/client.go | 3 ++ git/client_test.go | 37 +++++++++++++++++------- git/objects.go | 1 + pkg/cmd/pr/create/create_test.go | 49 ++++++++++++++++++++++++++++++++ pkg/cmd/pr/shared/finder.go | 4 ++- pkg/cmd/pr/shared/finder_test.go | 10 +++++++ pkg/cmd/pr/status/status.go | 4 ++- pkg/cmd/pr/status/status_test.go | 1 + 8 files changed, 96 insertions(+), 13 deletions(-) diff --git a/git/client.go b/git/client.go index 1a6d9ae7f..99acb0ac9 100644 --- a/git/client.go +++ b/git/client.go @@ -412,6 +412,9 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg Branc cfg.MergeBase = parts[1] } } + if out, err = c.revParse(ctx, "--verify", "--quiet", "--abbrev-ref", branch+"@{push}"); err == nil { + cfg.Push = strings.TrimSuffix(string(out), "\n") + } return } diff --git a/git/client_test.go b/git/client_test.go index fff1397f3..c547d0335 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -727,28 +727,43 @@ func TestClientCommitBody(t *testing.T) { func TestClientReadBranchConfig(t *testing.T) { tests := []struct { name string - cmdExitStatus int - cmdStdout string - cmdStderr string - wantCmdArgs string + cmdExitStatus []int + cmdStdout []string + cmdStderr []string + wantCmdArgs []string wantBranchConfig BranchConfig }{ { name: "read branch config", - cmdStdout: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk\nbranch.trunk.gh-merge-base trunk", - wantCmdArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|gh-merge-base)$`, - wantBranchConfig: BranchConfig{RemoteName: "origin", MergeRef: "refs/heads/trunk", MergeBase: "trunk"}, + cmdExitStatus: []int{0, 0}, + cmdStdout: []string{"branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk\nbranch.trunk.gh-merge-base trunk", "origin/trunk"}, + cmdStderr: []string{"", ""}, + wantCmdArgs: []string{`path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|gh-merge-base)$`, `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`}, + wantBranchConfig: BranchConfig{RemoteName: "origin", MergeRef: "refs/heads/trunk", MergeBase: "trunk", Push: "origin/trunk"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus, tt.cmdStdout, tt.cmdStderr) + var cmds []*exec.Cmd + var cmdCtxs []commandCtx + for i := 0; i < len(tt.cmdExitStatus); i++ { + cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus[i], tt.cmdStdout[i], tt.cmdStderr[i]) + cmds = append(cmds, cmd) + cmdCtxs = append(cmdCtxs, cmdCtx) + } + i := -1 client := Client{ - GitPath: "path/to/git", - commandContext: cmdCtx, + GitPath: "path/to/git", + commandContext: func(ctx context.Context, name string, args ...string) *exec.Cmd { + i++ + cmdCtxs[i](ctx, name, args...) + return cmds[i] + }, } branchConfig := client.ReadBranchConfig(context.Background(), "trunk") - assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " ")) + for i := 0; i < len(tt.cmdExitStatus); i++ { + assert.Equal(t, tt.wantCmdArgs[i], strings.Join(cmds[i].Args[3:], " ")) + } assert.Equal(t, tt.wantBranchConfig, branchConfig) }) } diff --git a/git/objects.go b/git/objects.go index c09683042..3b23d20b7 100644 --- a/git/objects.go +++ b/git/objects.go @@ -66,4 +66,5 @@ type BranchConfig struct { // MergeBase is the optional base branch to target in a new PR if `--base` is not specified. MergeBase string MergeRef string + Push string } diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 6c0ff11e2..2446b16c0 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -354,6 +354,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") }, expectedBrowse: "https://github.com/OWNER/REPO/compare/master...feature?body=&expand=1", @@ -383,6 +384,9 @@ func Test_createRun(t *testing.T) { opts.HeadBranch = "feature" return func() {} }, + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") + }, expectedOut: "https://github.com/OWNER/REPO/pull/12\n", }, { @@ -397,6 +401,9 @@ func Test_createRun(t *testing.T) { opts.DryRun = true return func() {} }, + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") + }, expectedOutputs: []string{ "Would have created a Pull Request with:", `title: my title`, @@ -428,6 +435,9 @@ func Test_createRun(t *testing.T) { opts.DryRun = true return func() {} }, + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") + }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { reg.Register( httpmock.GraphQL(`query RepositoryResolveMetadataIDs\b`), @@ -488,6 +498,9 @@ func Test_createRun(t *testing.T) { opts.DryRun = true return func() {} }, + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") + }, expectedOutputs: []string{ `Would have created a Pull Request with:`, `Title: my title`, @@ -525,6 +538,9 @@ func Test_createRun(t *testing.T) { opts.DryRun = true return func() {} }, + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") + }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { reg.Register( httpmock.GraphQL(`query RepositoryResolveMetadataIDs\b`), @@ -591,6 +607,9 @@ func Test_createRun(t *testing.T) { opts.DryRun = true return func() {} }, + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") + }, expectedOut: heredoc.Doc(` Would have created a Pull Request with: Title: TITLE @@ -637,6 +656,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, @@ -700,6 +720,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, @@ -746,6 +767,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, @@ -795,6 +817,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register("git remote rename origin upstream", 0, "") cs.Register(`git remote add origin https://github.com/monalisa/REPO.git`, 0, "") @@ -853,6 +876,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register("git show-ref --verify", 0, heredoc.Doc(` deadbeef HEAD deadb00f refs/remotes/upstream/feature @@ -890,6 +914,7 @@ func Test_createRun(t *testing.T) { branch.feature.remote origin branch.feature.merge refs/heads/my-feat2 `)) // determineTrackingBranch + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 0, "origin/my-feat2") cs.Register("git show-ref --verify", 0, heredoc.Doc(` deadbeef HEAD deadbeef refs/remotes/origin/my-feat2 @@ -930,6 +955,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "d3476a1\u0000commit 0\u0000\u0000\n7a6ea13\u0000commit 1\u0000\u0000") }, promptStubs: func(pm *prompter.PrompterMock) { @@ -970,6 +996,9 @@ func Test_createRun(t *testing.T) { opts.Milestone = "big one.oh" return func() {} }, + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") + }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { reg.Register( httpmock.GraphQL(`query RepositoryResolveMetadataIDs\b`), @@ -1057,6 +1086,9 @@ func Test_createRun(t *testing.T) { opts.Finder = shared.NewMockFinder("feature", &api.PullRequest{URL: "https://github.com/OWNER/REPO/pull/123"}, ghrepo.New("OWNER", "REPO")) return func() {} }, + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") + }, wantErr: "a pull request for branch \"feature\" into branch \"master\" already exists:\nhttps://github.com/OWNER/REPO/pull/123", }, { @@ -1073,6 +1105,7 @@ func Test_createRun(t *testing.T) { httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") @@ -1105,6 +1138,7 @@ func Test_createRun(t *testing.T) { mockRetrieveProjects(t, reg) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") @@ -1152,6 +1186,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature`, 0, "") cs.Register(`git rev-parse --show-toplevel`, 0, "") }, @@ -1211,6 +1246,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") }, promptStubs: func(pm *prompter.PrompterMock) { @@ -1259,6 +1295,7 @@ func Test_createRun(t *testing.T) { { name: "web long URL", cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") }, setup: func(opts *CreateOptions, t *testing.T) func() { @@ -1285,6 +1322,9 @@ func Test_createRun(t *testing.T) { } return func() {} }, + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") + }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { reg.Register( httpmock.GraphQL(`mutation PullRequestCreate\b`), @@ -1304,6 +1344,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register( "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", 0, @@ -1379,6 +1420,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register( "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", 0, @@ -1415,6 +1457,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register( "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", 0, @@ -1465,6 +1508,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register("git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", 0, "") }, expectedOut: "https://github.com/OWNER/REPO/pull/12\n", @@ -1524,6 +1568,7 @@ func Test_createRun(t *testing.T) { deadbeef HEAD deadb00f refs/remotes/upstream/feature/feat2 deadbeef refs/remotes/origin/task1`)) // determineTrackingBranch + cs.Register(`git rev-parse --verify --quiet --abbrev-ref task1@\{push\}`, 1, "") }, expectedOut: "https://github.com/OWNER/REPO/pull/12\n", expectedErrOut: "\nCreating pull request for monalisa:task1 into feature/feat2 in OWNER/REPO\n\n", @@ -1634,6 +1679,7 @@ func Test_tryDetermineTrackingRef(t *testing.T) { name: "empty", cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD`, 0, "abc HEAD") }, expectedTrackingRef: trackingRef{}, @@ -1643,6 +1689,7 @@ func Test_tryDetermineTrackingRef(t *testing.T) { name: "no match", cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register("git show-ref --verify -- HEAD refs/remotes/upstream/feature refs/remotes/origin/feature", 0, "abc HEAD\nbca refs/remotes/upstream/feature") }, remotes: context.Remotes{ @@ -1662,6 +1709,7 @@ func Test_tryDetermineTrackingRef(t *testing.T) { name: "match", cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 0, "origin/feature") cs.Register(`git show-ref --verify -- HEAD refs/remotes/upstream/feature refs/remotes/origin/feature$`, 0, heredoc.Doc(` deadbeef HEAD deadb00f refs/remotes/upstream/feature @@ -1691,6 +1739,7 @@ func Test_tryDetermineTrackingRef(t *testing.T) { branch.feature.remote origin branch.feature.merge refs/heads/great-feat `)) + cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 0, "origin/great-feat") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/great-feat refs/remotes/origin/feature$`, 0, heredoc.Doc(` deadbeef HEAD deadb00f refs/remotes/origin/feature diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index e4a70eb22..23aee371c 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -261,7 +261,9 @@ func (f *finder) parseCurrentBranch() (string, int, error) { } if gitRemoteRepo != nil { - if strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { + if branchConfig.Push != "" { + prHeadRef = strings.TrimPrefix(branchConfig.Push, branchConfig.RemoteName+"/") + } else if 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 258ef01e8..88ee170a0 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -232,8 +232,17 @@ func TestFind(t *testing.T) { return "blueberries", nil }, branchConfig: func(branch string) (c git.BranchConfig) { + c.MergeRef = "refs/heads/blueberries" + c.RemoteName = "origin" + c.Push = "origin/blueberries" return }, + remotesFn: func() (context.Remotes, error) { + return context.Remotes{{ + Remote: &git.Remote{Name: "origin"}, + Repo: ghrepo.New("OWNER", "REPO"), + }}, nil + }, }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -318,6 +327,7 @@ func TestFind(t *testing.T) { branchConfig: func(branch string) (c git.BranchConfig) { c.MergeRef = "refs/heads/blue-upstream-berries" c.RemoteName = "origin" + c.Push = "origin/blue-upstream-berries" return }, remotesFn: func() (context.Remotes, error) { diff --git a/pkg/cmd/pr/status/status.go b/pkg/cmd/pr/status/status.go index 27666461d..ebba3313c 100644 --- a/pkg/cmd/pr/status/status.go +++ b/pkg/cmd/pr/status/status.go @@ -209,7 +209,9 @@ func prSelectorForCurrentBranch(gitClient *git.Client, baseRepo ghrepo.Interface } if branchOwner != "" { - if strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { + if branchConfig.Push != "" { + selector = strings.TrimPrefix(branchConfig.Push, branchConfig.RemoteName+"/") + } else if 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 e8e487559..9fbe484c5 100644 --- a/pkg/cmd/pr/status/status_test.go +++ b/pkg/cmd/pr/status/status_test.go @@ -383,6 +383,7 @@ func Test_prSelectorForCurrentBranch(t *testing.T) { 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, "") repo := ghrepo.NewWithHost("octocat", "playground", "github.com") rem := context.Remotes{ From 7fc35fd47d9482068a5c7666c5c67d4e5e001fce Mon Sep 17 00:00:00 2001 From: Frederick Zhang Date: Fri, 14 Jun 2024 15:10:53 +1000 Subject: [PATCH 02/23] 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{ From 4254818dbdfa59b4f51e524687bd6cd49e7b61c2 Mon Sep 17 00:00:00 2001 From: Frederick Zhang Date: Fri, 14 Jun 2024 19:00:31 +1000 Subject: [PATCH 03/23] Find push remote using branch..pushRemote and remote.pushDefault When using a push.default = current triangular workflow, apart from using @{push} to determine the remote branch name, we should also follow the 1. branch..pushRemote 2. remote.pushDefault 3. branch..remote ...list to determine which remote Git pushes to. --- git/client.go | 31 ++++-- git/client_test.go | 157 ++++++++++++++++++++++++++++--- git/objects.go | 8 +- pkg/cmd/pr/create/create_test.go | 35 ++++++- pkg/cmd/pr/shared/finder.go | 9 +- pkg/cmd/pr/shared/finder_test.go | 4 + pkg/cmd/pr/status/status.go | 6 +- pkg/cmd/pr/status/status_test.go | 2 + 8 files changed, 215 insertions(+), 37 deletions(-) diff --git a/git/client.go b/git/client.go index 99acb0ac9..d82fd7b78 100644 --- a/git/client.go +++ b/git/client.go @@ -379,7 +379,7 @@ func (c *Client) lookupCommit(ctx context.Context, sha, format string) ([]byte, // ReadBranchConfig parses the `branch.BRANCH.(remote|merge|gh-merge-base)` part of git config. func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg BranchConfig) { prefix := regexp.QuoteMeta(fmt.Sprintf("branch.%s.", branch)) - args := []string{"config", "--get-regexp", fmt.Sprintf("^%s(remote|merge|%s)$", prefix, MergeBaseConfig)} + args := []string{"config", "--get-regexp", fmt.Sprintf("^%s(remote|merge|pushremote|%s)$", prefix, MergeBaseConfig)} cmd, err := c.Command(ctx, args...) if err != nil { return @@ -397,21 +397,22 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg Branc keys := strings.Split(parts[0], ".") switch keys[len(keys)-1] { case "remote": - if strings.Contains(parts[1], ":") { - u, err := ParseURL(parts[1]) - if err != nil { - continue - } - cfg.RemoteURL = u - } else if !isFilesystemPath(parts[1]) { - cfg.RemoteName = parts[1] - } + parseRemoteURLOrName(parts[1], &cfg.RemoteURL, &cfg.RemoteName) + case "pushremote": + parseRemoteURLOrName(parts[1], &cfg.PushRemoteURL, &cfg.PushRemoteName) case "merge": cfg.MergeRef = parts[1] case MergeBaseConfig: cfg.MergeBase = parts[1] } } + if cfg.PushRemoteURL == nil && cfg.PushRemoteName == "" { + if conf, err := c.Config(ctx, "remote.pushDefault"); err == nil && conf != "" { + parseRemoteURLOrName(conf, &cfg.PushRemoteURL, &cfg.PushRemoteName) + } else { + cfg.PushRemoteName = cfg.RemoteName + } + } if out, err = c.revParse(ctx, "--verify", "--quiet", "--abbrev-ref", branch+"@{push}"); err == nil { cfg.Push = strings.TrimSuffix(string(out), "\n") } @@ -776,6 +777,16 @@ func parseRemotes(remotesStr []string) RemoteSet { return remotes } +func parseRemoteURLOrName(value string, remoteURL **url.URL, remoteName *string) { + if strings.Contains(value, ":") { + if u, err := ParseURL(value); err == nil { + *remoteURL = u + } + } else if !isFilesystemPath(value) { + *remoteName = value + } +} + func populateResolvedRemotes(remotes RemoteSet, resolved []string) { for _, l := range resolved { parts := strings.SplitN(l, " ", 2) diff --git a/git/client_test.go b/git/client_test.go index c547d0335..3136363c1 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -725,31 +725,160 @@ func TestClientCommitBody(t *testing.T) { } func TestClientReadBranchConfig(t *testing.T) { + type cmdTest struct { + exitStatus int + stdOut string + stdErr string + wantArgs string + } tests := []struct { name string - cmdExitStatus []int - cmdStdout []string - cmdStderr []string - wantCmdArgs []string + cmds []cmdTest wantBranchConfig BranchConfig }{ { - name: "read branch config", - cmdExitStatus: []int{0, 0}, - cmdStdout: []string{"branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk\nbranch.trunk.gh-merge-base trunk", "origin/trunk"}, - cmdStderr: []string{"", ""}, - wantCmdArgs: []string{`path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|gh-merge-base)$`, `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`}, - wantBranchConfig: BranchConfig{RemoteName: "origin", MergeRef: "refs/heads/trunk", MergeBase: "trunk", Push: "origin/trunk"}, + name: "read branch config, central", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk\nbranch.trunk.gh-merge-base trunk", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`, + }, + { + wantArgs: `path/to/git config remote.pushDefault`, + }, + { + stdOut: "origin/trunk", + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{RemoteName: "origin", MergeRef: "refs/heads/trunk", MergeBase: "trunk", PushRemoteName: "origin", Push: "origin/trunk"}, + }, + { + name: "read branch config, central, push.default = upstream", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk-remote", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`, + }, + { + wantArgs: `path/to/git config remote.pushDefault`, + }, + { + stdOut: "origin/trunk-remote", + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{RemoteName: "origin", MergeRef: "refs/heads/trunk-remote", PushRemoteName: "origin", Push: "origin/trunk-remote"}, + }, + { + name: "read branch config, central, push.default = current", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/main", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`, + }, + { + wantArgs: `path/to/git config remote.pushDefault`, + }, + { + stdOut: "origin/trunk", + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{RemoteName: "origin", MergeRef: "refs/heads/main", PushRemoteName: "origin", Push: "origin/trunk"}, + }, + { + name: "read branch config, central, push.default = current, target branch not pushed, no existing remote branch", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote .\nbranch.trunk.merge refs/heads/trunk-middle", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`, + }, + { + stdOut: "origin", + wantArgs: `path/to/git config remote.pushDefault`, + }, + { + exitStatus: 1, + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{MergeRef: "refs/heads/trunk-middle", PushRemoteName: "origin"}, + }, + { + name: "read branch config, triangular, push.default = current, has existing remote branch, branch.trunk.pushremote effective", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main\nbranch.trunk.pushremote origin", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`, + }, + { + stdOut: "origin/trunk", + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{RemoteName: "upstream", MergeRef: "refs/heads/main", PushRemoteName: "origin", Push: "origin/trunk"}, + }, + { + name: "read branch config, triangular, push.default = current, has existing remote branch, remote.pushDefault effective", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`, + }, + { + stdOut: "origin", + wantArgs: `path/to/git config remote.pushDefault`, + }, + { + stdOut: "origin/trunk", + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{RemoteName: "upstream", MergeRef: "refs/heads/main", PushRemoteName: "origin", Push: "origin/trunk"}, + }, + { + name: "read branch config, triangular, push.default = current, no existing remote branch, branch.trunk.pushremote effective", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main\nbranch.trunk.pushremote origin", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`, + }, + { + exitStatus: 1, + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{RemoteName: "upstream", MergeRef: "refs/heads/main", PushRemoteName: "origin"}, + }, + { + name: "read branch config, triangular, push.default = current, no existing remote branch, remote.pushDefault effective", + cmds: []cmdTest{ + { + stdOut: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main", + wantArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`, + }, + { + stdOut: "origin", + wantArgs: `path/to/git config remote.pushDefault`, + }, + { + exitStatus: 1, + wantArgs: `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`, + }, + }, + wantBranchConfig: BranchConfig{RemoteName: "upstream", MergeRef: "refs/heads/main", PushRemoteName: "origin"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var cmds []*exec.Cmd var cmdCtxs []commandCtx - for i := 0; i < len(tt.cmdExitStatus); i++ { - cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus[i], tt.cmdStdout[i], tt.cmdStderr[i]) + for _, c := range tt.cmds { + cmd, cmdCtx := createCommandContext(t, c.exitStatus, c.stdOut, c.stdErr) cmds = append(cmds, cmd) cmdCtxs = append(cmdCtxs, cmdCtx) + } i := -1 client := Client{ @@ -761,8 +890,8 @@ func TestClientReadBranchConfig(t *testing.T) { }, } branchConfig := client.ReadBranchConfig(context.Background(), "trunk") - for i := 0; i < len(tt.cmdExitStatus); i++ { - assert.Equal(t, tt.wantCmdArgs[i], strings.Join(cmds[i].Args[3:], " ")) + for i := 0; i < len(tt.cmds); i++ { + assert.Equal(t, tt.cmds[i].wantArgs, strings.Join(cmds[i].Args[3:], " ")) } assert.Equal(t, tt.wantBranchConfig, branchConfig) }) diff --git a/git/objects.go b/git/objects.go index 3b23d20b7..eff209be4 100644 --- a/git/objects.go +++ b/git/objects.go @@ -64,7 +64,9 @@ type BranchConfig struct { RemoteName string RemoteURL *url.URL // MergeBase is the optional base branch to target in a new PR if `--base` is not specified. - MergeBase string - MergeRef string - Push string + MergeBase string + MergeRef string + PushRemoteURL *url.URL + PushRemoteName string + Push string } diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 2446b16c0..073dfb879 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -354,6 +354,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") }, @@ -385,6 +386,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") }, expectedOut: "https://github.com/OWNER/REPO/pull/12\n", @@ -402,6 +404,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") }, expectedOutputs: []string{ @@ -436,6 +439,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { @@ -499,6 +503,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") }, expectedOutputs: []string{ @@ -539,6 +544,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { @@ -608,6 +614,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") }, expectedOut: heredoc.Doc(` @@ -656,6 +663,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") @@ -720,6 +728,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") @@ -767,6 +776,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") @@ -817,6 +827,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register("git remote rename origin upstream", 0, "") @@ -876,6 +887,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register("git show-ref --verify", 0, heredoc.Doc(` deadbeef HEAD @@ -914,6 +926,7 @@ func Test_createRun(t *testing.T) { branch.feature.remote origin branch.feature.merge refs/heads/my-feat2 `)) // determineTrackingBranch + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 0, "origin/my-feat2") cs.Register("git show-ref --verify", 0, heredoc.Doc(` deadbeef HEAD @@ -955,6 +968,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "d3476a1\u0000commit 0\u0000\u0000\n7a6ea13\u0000commit 1\u0000\u0000") }, @@ -997,6 +1011,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { @@ -1087,6 +1102,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") }, wantErr: "a pull request for branch \"feature\" into branch \"master\" already exists:\nhttps://github.com/OWNER/REPO/pull/123", @@ -1105,6 +1121,7 @@ func Test_createRun(t *testing.T) { httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") @@ -1138,6 +1155,7 @@ func Test_createRun(t *testing.T) { mockRetrieveProjects(t, reg) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") @@ -1186,6 +1204,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature`, 0, "") cs.Register(`git rev-parse --show-toplevel`, 0, "") @@ -1246,6 +1265,7 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") }, @@ -1295,6 +1315,7 @@ func Test_createRun(t *testing.T) { { name: "web long URL", cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") }, @@ -1323,6 +1344,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { @@ -1344,6 +1366,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register( "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", @@ -1420,6 +1443,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register( "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", @@ -1457,6 +1481,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register( "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", @@ -1508,6 +1533,7 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register("git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", 0, "") }, @@ -1560,7 +1586,7 @@ func Test_createRun(t *testing.T) { }, customBranchConfig: true, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp \^branch\\\.task1\\\.\(remote\|merge\|gh-merge-base\)\$`, 0, heredoc.Doc(` + cs.Register(`git config --get-regexp \^branch\\\.task1\\\.\(remote\|merge\|pushremote\|gh-merge-base\)\$`, 0, heredoc.Doc(` branch.task1.remote origin branch.task1.merge refs/heads/task1 branch.task1.gh-merge-base feature/feat2`)) // ReadBranchConfig @@ -1568,6 +1594,7 @@ func Test_createRun(t *testing.T) { deadbeef HEAD deadb00f refs/remotes/upstream/feature/feat2 deadbeef refs/remotes/origin/task1`)) // determineTrackingBranch + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref task1@\{push\}`, 1, "") }, expectedOut: "https://github.com/OWNER/REPO/pull/12\n", @@ -1595,7 +1622,7 @@ func Test_createRun(t *testing.T) { defer cmdTeardown(t) cs.Register(`git status --porcelain`, 0, "") if !tt.customBranchConfig { - cs.Register(`git config --get-regexp \^branch\\\..+\\\.\(remote\|merge\|gh-merge-base\)\$`, 0, "") + cs.Register(`git config --get-regexp \^branch\\\..+\\\.\(remote\|merge\|pushremote\|gh-merge-base\)\$`, 0, "") } if tt.cmdStubs != nil { @@ -1679,6 +1706,7 @@ func Test_tryDetermineTrackingRef(t *testing.T) { name: "empty", cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD`, 0, "abc HEAD") }, @@ -1689,6 +1717,7 @@ func Test_tryDetermineTrackingRef(t *testing.T) { name: "no match", cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register("git show-ref --verify -- HEAD refs/remotes/upstream/feature refs/remotes/origin/feature", 0, "abc HEAD\nbca refs/remotes/upstream/feature") }, @@ -1709,6 +1738,7 @@ func Test_tryDetermineTrackingRef(t *testing.T) { name: "match", cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 0, "origin/feature") cs.Register(`git show-ref --verify -- HEAD refs/remotes/upstream/feature refs/remotes/origin/feature$`, 0, heredoc.Doc(` deadbeef HEAD @@ -1739,6 +1769,7 @@ func Test_tryDetermineTrackingRef(t *testing.T) { branch.feature.remote origin branch.feature.merge refs/heads/great-feat `)) + cs.Register(`git config remote.pushDefault`, 1, "") cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 0, "origin/great-feat") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/great-feat refs/remotes/origin/feature$`, 0, heredoc.Doc(` deadbeef HEAD diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index 6e36b4dd6..f2c8f9f92 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -251,22 +251,21 @@ func (f *finder) parseCurrentBranch() (string, int, error) { } var gitRemoteRepo ghrepo.Interface - if branchConfig.RemoteURL != nil { + if branchConfig.PushRemoteURL != nil { // the branch merges from a remote specified by URL if r, err := ghrepo.FromURL(branchConfig.RemoteURL); err == nil { gitRemoteRepo = r } - } else if branchConfig.RemoteName != "" { - // the branch merges from a remote specified by name + } else if branchConfig.PushRemoteName != "" { rem, _ := f.remotesFn() - if r, err := rem.FindByName(branchConfig.RemoteName); err == nil { + if r, err := rem.FindByName(branchConfig.PushRemoteName); err == nil { gitRemoteRepo = r } } if gitRemoteRepo != nil { if branchConfig.Push != "" { - prHeadRef = strings.TrimPrefix(branchConfig.Push, branchConfig.RemoteName+"/") + prHeadRef = strings.TrimPrefix(branchConfig.Push, branchConfig.PushRemoteName+"/") } else if pushDefault, _ := f.pushDefault(); (pushDefault == "upstream" || pushDefault == "tracking") && strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { prHeadRef = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index 2c34833c8..7c9f1ab57 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -328,6 +328,7 @@ func TestFind(t *testing.T) { branchConfig: func(branch string) (c git.BranchConfig) { c.MergeRef = "refs/heads/blue-upstream-berries" c.RemoteName = "origin" + c.PushRemoteName = "origin" c.Push = "origin/blue-upstream-berries" return }, @@ -373,6 +374,7 @@ func TestFind(t *testing.T) { u, _ := url.Parse("https://github.com/UPSTREAMOWNER/REPO") c.MergeRef = "refs/heads/blue-upstream-berries" c.RemoteURL = u + c.PushRemoteURL = u return }, pushDefault: func() (string, error) { return "upstream", nil }, @@ -411,6 +413,7 @@ func TestFind(t *testing.T) { branchConfig: func(branch string) (c git.BranchConfig) { c.MergeRef = "refs/heads/blue-upstream-berries" c.RemoteName = "origin" + c.PushRemoteName = "origin" c.Push = "origin/blue-upstream-berries" return }, @@ -454,6 +457,7 @@ func TestFind(t *testing.T) { }, branchConfig: func(branch string) (c git.BranchConfig) { c.RemoteName = "origin" + c.PushRemoteName = "origin" c.Push = "origin/blueberries" return }, diff --git a/pkg/cmd/pr/status/status.go b/pkg/cmd/pr/status/status.go index 1b6da8427..33fc096c8 100644 --- a/pkg/cmd/pr/status/status.go +++ b/pkg/cmd/pr/status/status.go @@ -201,16 +201,16 @@ func prSelectorForCurrentBranch(gitClient *git.Client, baseRepo ghrepo.Interface if r, err := ghrepo.FromURL(branchConfig.RemoteURL); err == nil { branchOwner = r.RepoOwner() } - } else if branchConfig.RemoteName != "" { + } else if branchConfig.PushRemoteName != "" { // the branch merges from a remote specified by name - if r, err := rem.FindByName(branchConfig.RemoteName); err == nil { + if r, err := rem.FindByName(branchConfig.PushRemoteName); err == nil { branchOwner = r.RepoOwner() } } if branchOwner != "" { if branchConfig.Push != "" { - selector = strings.TrimPrefix(branchConfig.Push, branchConfig.RemoteName+"/") + selector = strings.TrimPrefix(branchConfig.Push, branchConfig.PushRemoteName+"/") } 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/") diff --git a/pkg/cmd/pr/status/status_test.go b/pkg/cmd/pr/status/status_test.go index d09abf5b9..e60764c4f 100644 --- a/pkg/cmd/pr/status/status_test.go +++ b/pkg/cmd/pr/status/status_test.go @@ -383,6 +383,7 @@ func Test_prSelectorForCurrentBranchPushDefaultUpstream(t *testing.T) { branch.Frederick888/main.remote git@github.com:Frederick888/playground.git branch.Frederick888/main.merge refs/heads/main `)) + rs.Register(`git config remote.pushDefault`, 1, "") rs.Register(`git rev-parse --verify --quiet --abbrev-ref Frederick888/main@\{push\}`, 1, "") rs.Register(`git config push\.default`, 0, "upstream") @@ -414,6 +415,7 @@ func Test_prSelectorForCurrentBranchPushDefaultTracking(t *testing.T) { branch.Frederick888/main.remote git@github.com:Frederick888/playground.git branch.Frederick888/main.merge refs/heads/main `)) + rs.Register(`git config remote.pushDefault`, 1, "") rs.Register(`git rev-parse --verify --quiet --abbrev-ref Frederick888/main@\{push\}`, 1, "") rs.Register(`git config push\.default`, 0, "tracking") From 2d1e4d625bc2a31c61c70452388188b327032b3c Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 16 Dec 2024 12:02:09 -0500 Subject: [PATCH 04/23] Add base gh pr view acceptance tests for changes These are acceptance tests based on @williammartin work within various issues associated with #9208. These need to be enhanced to account for `gh pr status`, however that should be quicker given these pass based on the updated branch and upstream fix for cross-repo same org changes. --- .../pr-view-respects-branch-pushremote.txtar | 42 +++++++++++++++++++ .../pr-view-respects-push-destination.txtar | 33 +++++++++++++++ .../pr-view-respects-remote-pushdefault.txtar | 42 +++++++++++++++++++ .../pr-view-respects-simple-pushdefault.txtar | 30 +++++++++++++ 4 files changed, 147 insertions(+) create mode 100644 acceptance/testdata/pr/pr-view-respects-branch-pushremote.txtar create mode 100644 acceptance/testdata/pr/pr-view-respects-push-destination.txtar create mode 100644 acceptance/testdata/pr/pr-view-respects-remote-pushdefault.txtar create mode 100644 acceptance/testdata/pr/pr-view-respects-simple-pushdefault.txtar diff --git a/acceptance/testdata/pr/pr-view-respects-branch-pushremote.txtar b/acceptance/testdata/pr/pr-view-respects-branch-pushremote.txtar new file mode 100644 index 000000000..3e79e755d --- /dev/null +++ b/acceptance/testdata/pr/pr-view-respects-branch-pushremote.txtar @@ -0,0 +1,42 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} +env FORK=${REPO}-fork + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository to act as upstream with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup of upstream +defer gh repo delete --yes ${ORG}/${REPO} +exec gh repo view ${ORG}/${REPO} --json id --jq '.id' +stdout2env REPO_ID + +# Create a user fork of repository as opposed to private organization fork +exec gh repo fork ${ORG}/${REPO} --org ${ORG} --fork-name ${FORK} + +# Defer repo cleanup of fork +defer gh repo delete --yes ${ORG}/${FORK} +sleep 5 +exec gh repo view ${ORG}/${FORK} --json id --jq '.id' +stdout2env FORK_ID + +# Clone the repo +exec gh repo clone ${ORG}/${FORK} +cd ${FORK} + +# Prepare a branch where changes are pulled from the upstream default branch but pushed to fork +exec git checkout -b feature-branch upstream/main +exec git config branch.feature-branch.pushRemote origin +exec git config --list +exec git commit --allow-empty -m 'Empty Commit' +exec git push + +# Create the PR spanning upstream and fork repositories, gh pr create does not support headRepositoryId needed for private forks +exec gh api graphql -F repositoryId="${REPO_ID}" -F headRepositoryId="${FORK_ID}" -F query='mutation CreatePullRequest($headRepositoryId: ID!, $repositoryId: ID!) { createPullRequest(input:{ baseRefName: "main", body: "Feature Body", draft: false, headRefName: "feature-branch", headRepositoryId: $headRepositoryId, repositoryId: $repositoryId, title:"Feature Title" }){ pullRequest{ id url } } }' + +# View the PR +env GH_DEBUG=api +exec gh pr view +stdout 'Feature Title' diff --git a/acceptance/testdata/pr/pr-view-respects-push-destination.txtar b/acceptance/testdata/pr/pr-view-respects-push-destination.txtar new file mode 100644 index 000000000..6772d6cfd --- /dev/null +++ b/acceptance/testdata/pr/pr-view-respects-push-destination.txtar @@ -0,0 +1,33 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes ${ORG}/${REPO} + +# Clone the repo +exec gh repo clone ${ORG}/${REPO} +cd ${REPO} + +# Configure default push behavior so local and remote branches will be the same +exec git config push.default current + +# Prepare a branch where changes are pulled from the default branch instead of remote branch of same name +exec git checkout -b test-feature +exec git branch --set-upstream-to origin/main +exec git rev-parse --abbrev-ref test-feature@{upstream} +stdout origin/main + +# Create the PR +exec git commit --allow-empty -m 'Empty Commit' +exec git push +exec gh pr create -B main -H test-feature --title 'Feature Title' --body 'Feature Body' + +# View the PR +exec gh pr view +stdout 'Feature Title' diff --git a/acceptance/testdata/pr/pr-view-respects-remote-pushdefault.txtar b/acceptance/testdata/pr/pr-view-respects-remote-pushdefault.txtar new file mode 100644 index 000000000..2a8a92826 --- /dev/null +++ b/acceptance/testdata/pr/pr-view-respects-remote-pushdefault.txtar @@ -0,0 +1,42 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} +env FORK=${REPO}-fork + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository to act as upstream with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup of upstream +defer gh repo delete --yes ${ORG}/${REPO} +exec gh repo view ${ORG}/${REPO} --json id --jq '.id' +stdout2env REPO_ID + +# Create a user fork of repository as opposed to private organization fork +exec gh repo fork ${ORG}/${REPO} --org ${ORG} --fork-name ${FORK} + +# Defer repo cleanup of fork +defer gh repo delete --yes ${ORG}/${FORK} +sleep 5 +exec gh repo view ${ORG}/${FORK} --json id --jq '.id' +stdout2env FORK_ID + +# Clone the repo +exec gh repo clone ${ORG}/${FORK} +cd ${FORK} + +# Prepare a branch where changes are pulled from the upstream default branch but pushed to fork +exec git checkout -b feature-branch upstream/main +exec git config remote.pushDefault origin +exec git config --list +exec git commit --allow-empty -m 'Empty Commit' +exec git push + +# Create the PR spanning upstream and fork repositories, gh pr create does not support headRepositoryId needed for private forks +exec gh api graphql -F repositoryId="${REPO_ID}" -F headRepositoryId="${FORK_ID}" -F query='mutation CreatePullRequest($headRepositoryId: ID!, $repositoryId: ID!) { createPullRequest(input:{ baseRefName: "main", body: "Feature Body", draft: false, headRefName: "feature-branch", headRepositoryId: $headRepositoryId, repositoryId: $repositoryId, title:"Feature Title" }){ pullRequest{ id url } } }' + +# View the PR +env GH_DEBUG=api +exec gh pr view +stdout 'Feature Title' diff --git a/acceptance/testdata/pr/pr-view-respects-simple-pushdefault.txtar b/acceptance/testdata/pr/pr-view-respects-simple-pushdefault.txtar new file mode 100644 index 000000000..55da849ac --- /dev/null +++ b/acceptance/testdata/pr/pr-view-respects-simple-pushdefault.txtar @@ -0,0 +1,30 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes ${ORG}/${REPO} + +# Clone the repo +exec gh repo clone ${ORG}/${REPO} +cd ${REPO} + +# Configure default push behavior so local and remote branches have to be the same +exec git config push.default simple + +# Prepare a branch where changes are pulled from the default branch instead of remote branch of same name +exec git checkout -b test-feature origin/main + +# Create the PR +exec git commit --allow-empty -m 'Empty Commit' +exec git push origin test-feature +exec gh pr create -H test-feature --title 'Feature Title' --body 'Feature Body' + +# View the PR +exec gh pr view +stdout 'Feature Title' From eb16a75ab1a02b7a7f309c8de4c03528fe83e1eb Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 16 Dec 2024 14:24:59 -0500 Subject: [PATCH 05/23] Expand with gh pr status --- .../pr/pr-view-respects-branch-pushremote.txtar | 7 +++++-- .../pr/pr-view-respects-push-destination.txtar | 11 ++++++++--- .../pr/pr-view-respects-remote-pushdefault.txtar | 7 +++++-- .../pr/pr-view-respects-simple-pushdefault.txtar | 11 ++++++++--- 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/acceptance/testdata/pr/pr-view-respects-branch-pushremote.txtar b/acceptance/testdata/pr/pr-view-respects-branch-pushremote.txtar index 3e79e755d..ef80cd8ba 100644 --- a/acceptance/testdata/pr/pr-view-respects-branch-pushremote.txtar +++ b/acceptance/testdata/pr/pr-view-respects-branch-pushremote.txtar @@ -29,7 +29,6 @@ cd ${FORK} # Prepare a branch where changes are pulled from the upstream default branch but pushed to fork exec git checkout -b feature-branch upstream/main exec git config branch.feature-branch.pushRemote origin -exec git config --list exec git commit --allow-empty -m 'Empty Commit' exec git push @@ -37,6 +36,10 @@ exec git push exec gh api graphql -F repositoryId="${REPO_ID}" -F headRepositoryId="${FORK_ID}" -F query='mutation CreatePullRequest($headRepositoryId: ID!, $repositoryId: ID!) { createPullRequest(input:{ baseRefName: "main", body: "Feature Body", draft: false, headRefName: "feature-branch", headRepositoryId: $headRepositoryId, repositoryId: $repositoryId, title:"Feature Title" }){ pullRequest{ id url } } }' # View the PR -env GH_DEBUG=api exec gh pr view stdout 'Feature Title' + +# Check the PR status +env PR_STATUS_BRANCH=#1 Feature Title [${ORG}:feature-branch] +exec gh pr status +stdout $PR_STATUS_BRANCH diff --git a/acceptance/testdata/pr/pr-view-respects-push-destination.txtar b/acceptance/testdata/pr/pr-view-respects-push-destination.txtar index 6772d6cfd..ff9db4037 100644 --- a/acceptance/testdata/pr/pr-view-respects-push-destination.txtar +++ b/acceptance/testdata/pr/pr-view-respects-push-destination.txtar @@ -18,16 +18,21 @@ cd ${REPO} exec git config push.default current # Prepare a branch where changes are pulled from the default branch instead of remote branch of same name -exec git checkout -b test-feature +exec git checkout -b feature-branch exec git branch --set-upstream-to origin/main -exec git rev-parse --abbrev-ref test-feature@{upstream} +exec git rev-parse --abbrev-ref feature-branch@{upstream} stdout origin/main # Create the PR exec git commit --allow-empty -m 'Empty Commit' exec git push -exec gh pr create -B main -H test-feature --title 'Feature Title' --body 'Feature Body' +exec gh pr create -B main -H feature-branch --title 'Feature Title' --body 'Feature Body' # View the PR exec gh pr view stdout 'Feature Title' + +# Check the PR status +env PR_STATUS_BRANCH=#1 Feature Title [feature-branch] +exec gh pr status +stdout $PR_STATUS_BRANCH diff --git a/acceptance/testdata/pr/pr-view-respects-remote-pushdefault.txtar b/acceptance/testdata/pr/pr-view-respects-remote-pushdefault.txtar index 2a8a92826..8bfac2837 100644 --- a/acceptance/testdata/pr/pr-view-respects-remote-pushdefault.txtar +++ b/acceptance/testdata/pr/pr-view-respects-remote-pushdefault.txtar @@ -29,7 +29,6 @@ cd ${FORK} # Prepare a branch where changes are pulled from the upstream default branch but pushed to fork exec git checkout -b feature-branch upstream/main exec git config remote.pushDefault origin -exec git config --list exec git commit --allow-empty -m 'Empty Commit' exec git push @@ -37,6 +36,10 @@ exec git push exec gh api graphql -F repositoryId="${REPO_ID}" -F headRepositoryId="${FORK_ID}" -F query='mutation CreatePullRequest($headRepositoryId: ID!, $repositoryId: ID!) { createPullRequest(input:{ baseRefName: "main", body: "Feature Body", draft: false, headRefName: "feature-branch", headRepositoryId: $headRepositoryId, repositoryId: $repositoryId, title:"Feature Title" }){ pullRequest{ id url } } }' # View the PR -env GH_DEBUG=api exec gh pr view stdout 'Feature Title' + +# Check the PR status +env PR_STATUS_BRANCH=#1 Feature Title [${ORG}:feature-branch] +exec gh pr status +stdout $PR_STATUS_BRANCH diff --git a/acceptance/testdata/pr/pr-view-respects-simple-pushdefault.txtar b/acceptance/testdata/pr/pr-view-respects-simple-pushdefault.txtar index 55da849ac..114f401ec 100644 --- a/acceptance/testdata/pr/pr-view-respects-simple-pushdefault.txtar +++ b/acceptance/testdata/pr/pr-view-respects-simple-pushdefault.txtar @@ -18,13 +18,18 @@ cd ${REPO} exec git config push.default simple # Prepare a branch where changes are pulled from the default branch instead of remote branch of same name -exec git checkout -b test-feature origin/main +exec git checkout -b feature-branch origin/main # Create the PR exec git commit --allow-empty -m 'Empty Commit' -exec git push origin test-feature -exec gh pr create -H test-feature --title 'Feature Title' --body 'Feature Body' +exec git push origin feature-branch +exec gh pr create -H feature-branch --title 'Feature Title' --body 'Feature Body' # View the PR exec gh pr view stdout 'Feature Title' + +# Check the PR status +env PR_STATUS_BRANCH=#1 Feature Title [feature-branch] +exec gh pr status +stdout $PR_STATUS_BRANCH From 8bb2879b87b9be2d104f6b3e6266d2058e117252 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 16 Dec 2024 14:26:13 -0500 Subject: [PATCH 06/23] Reflect coverage for view and status subcommands --- .../pr-view-respects-branch-pushremote.txtar | 45 ------------------- .../pr-view-respects-push-destination.txtar | 38 ---------------- .../pr-view-respects-remote-pushdefault.txtar | 45 ------------------- .../pr-view-respects-simple-pushdefault.txtar | 35 --------------- 4 files changed, 163 deletions(-) delete mode 100644 acceptance/testdata/pr/pr-view-respects-branch-pushremote.txtar delete mode 100644 acceptance/testdata/pr/pr-view-respects-push-destination.txtar delete mode 100644 acceptance/testdata/pr/pr-view-respects-remote-pushdefault.txtar delete mode 100644 acceptance/testdata/pr/pr-view-respects-simple-pushdefault.txtar diff --git a/acceptance/testdata/pr/pr-view-respects-branch-pushremote.txtar b/acceptance/testdata/pr/pr-view-respects-branch-pushremote.txtar deleted file mode 100644 index ef80cd8ba..000000000 --- a/acceptance/testdata/pr/pr-view-respects-branch-pushremote.txtar +++ /dev/null @@ -1,45 +0,0 @@ -# Setup environment variables used for testscript -env REPO=${SCRIPT_NAME}-${RANDOM_STRING} -env FORK=${REPO}-fork - -# Use gh as a credential helper -exec gh auth setup-git - -# Create a repository to act as upstream with a file so it has a default branch -exec gh repo create ${ORG}/${REPO} --add-readme --private - -# Defer repo cleanup of upstream -defer gh repo delete --yes ${ORG}/${REPO} -exec gh repo view ${ORG}/${REPO} --json id --jq '.id' -stdout2env REPO_ID - -# Create a user fork of repository as opposed to private organization fork -exec gh repo fork ${ORG}/${REPO} --org ${ORG} --fork-name ${FORK} - -# Defer repo cleanup of fork -defer gh repo delete --yes ${ORG}/${FORK} -sleep 5 -exec gh repo view ${ORG}/${FORK} --json id --jq '.id' -stdout2env FORK_ID - -# Clone the repo -exec gh repo clone ${ORG}/${FORK} -cd ${FORK} - -# Prepare a branch where changes are pulled from the upstream default branch but pushed to fork -exec git checkout -b feature-branch upstream/main -exec git config branch.feature-branch.pushRemote origin -exec git commit --allow-empty -m 'Empty Commit' -exec git push - -# Create the PR spanning upstream and fork repositories, gh pr create does not support headRepositoryId needed for private forks -exec gh api graphql -F repositoryId="${REPO_ID}" -F headRepositoryId="${FORK_ID}" -F query='mutation CreatePullRequest($headRepositoryId: ID!, $repositoryId: ID!) { createPullRequest(input:{ baseRefName: "main", body: "Feature Body", draft: false, headRefName: "feature-branch", headRepositoryId: $headRepositoryId, repositoryId: $repositoryId, title:"Feature Title" }){ pullRequest{ id url } } }' - -# View the PR -exec gh pr view -stdout 'Feature Title' - -# Check the PR status -env PR_STATUS_BRANCH=#1 Feature Title [${ORG}:feature-branch] -exec gh pr status -stdout $PR_STATUS_BRANCH diff --git a/acceptance/testdata/pr/pr-view-respects-push-destination.txtar b/acceptance/testdata/pr/pr-view-respects-push-destination.txtar deleted file mode 100644 index ff9db4037..000000000 --- a/acceptance/testdata/pr/pr-view-respects-push-destination.txtar +++ /dev/null @@ -1,38 +0,0 @@ -# Setup environment variables used for testscript -env REPO=${SCRIPT_NAME}-${RANDOM_STRING} - -# Use gh as a credential helper -exec gh auth setup-git - -# Create a repository with a file so it has a default branch -exec gh repo create ${ORG}/${REPO} --add-readme --private - -# Defer repo cleanup -defer gh repo delete --yes ${ORG}/${REPO} - -# Clone the repo -exec gh repo clone ${ORG}/${REPO} -cd ${REPO} - -# Configure default push behavior so local and remote branches will be the same -exec git config push.default current - -# Prepare a branch where changes are pulled from the default branch instead of remote branch of same name -exec git checkout -b feature-branch -exec git branch --set-upstream-to origin/main -exec git rev-parse --abbrev-ref feature-branch@{upstream} -stdout origin/main - -# Create the PR -exec git commit --allow-empty -m 'Empty Commit' -exec git push -exec gh pr create -B main -H feature-branch --title 'Feature Title' --body 'Feature Body' - -# View the PR -exec gh pr view -stdout 'Feature Title' - -# Check the PR status -env PR_STATUS_BRANCH=#1 Feature Title [feature-branch] -exec gh pr status -stdout $PR_STATUS_BRANCH diff --git a/acceptance/testdata/pr/pr-view-respects-remote-pushdefault.txtar b/acceptance/testdata/pr/pr-view-respects-remote-pushdefault.txtar deleted file mode 100644 index 8bfac2837..000000000 --- a/acceptance/testdata/pr/pr-view-respects-remote-pushdefault.txtar +++ /dev/null @@ -1,45 +0,0 @@ -# Setup environment variables used for testscript -env REPO=${SCRIPT_NAME}-${RANDOM_STRING} -env FORK=${REPO}-fork - -# Use gh as a credential helper -exec gh auth setup-git - -# Create a repository to act as upstream with a file so it has a default branch -exec gh repo create ${ORG}/${REPO} --add-readme --private - -# Defer repo cleanup of upstream -defer gh repo delete --yes ${ORG}/${REPO} -exec gh repo view ${ORG}/${REPO} --json id --jq '.id' -stdout2env REPO_ID - -# Create a user fork of repository as opposed to private organization fork -exec gh repo fork ${ORG}/${REPO} --org ${ORG} --fork-name ${FORK} - -# Defer repo cleanup of fork -defer gh repo delete --yes ${ORG}/${FORK} -sleep 5 -exec gh repo view ${ORG}/${FORK} --json id --jq '.id' -stdout2env FORK_ID - -# Clone the repo -exec gh repo clone ${ORG}/${FORK} -cd ${FORK} - -# Prepare a branch where changes are pulled from the upstream default branch but pushed to fork -exec git checkout -b feature-branch upstream/main -exec git config remote.pushDefault origin -exec git commit --allow-empty -m 'Empty Commit' -exec git push - -# Create the PR spanning upstream and fork repositories, gh pr create does not support headRepositoryId needed for private forks -exec gh api graphql -F repositoryId="${REPO_ID}" -F headRepositoryId="${FORK_ID}" -F query='mutation CreatePullRequest($headRepositoryId: ID!, $repositoryId: ID!) { createPullRequest(input:{ baseRefName: "main", body: "Feature Body", draft: false, headRefName: "feature-branch", headRepositoryId: $headRepositoryId, repositoryId: $repositoryId, title:"Feature Title" }){ pullRequest{ id url } } }' - -# View the PR -exec gh pr view -stdout 'Feature Title' - -# Check the PR status -env PR_STATUS_BRANCH=#1 Feature Title [${ORG}:feature-branch] -exec gh pr status -stdout $PR_STATUS_BRANCH diff --git a/acceptance/testdata/pr/pr-view-respects-simple-pushdefault.txtar b/acceptance/testdata/pr/pr-view-respects-simple-pushdefault.txtar deleted file mode 100644 index 114f401ec..000000000 --- a/acceptance/testdata/pr/pr-view-respects-simple-pushdefault.txtar +++ /dev/null @@ -1,35 +0,0 @@ -# Setup environment variables used for testscript -env REPO=${SCRIPT_NAME}-${RANDOM_STRING} - -# Use gh as a credential helper -exec gh auth setup-git - -# Create a repository with a file so it has a default branch -exec gh repo create ${ORG}/${REPO} --add-readme --private - -# Defer repo cleanup -defer gh repo delete --yes ${ORG}/${REPO} - -# Clone the repo -exec gh repo clone ${ORG}/${REPO} -cd ${REPO} - -# Configure default push behavior so local and remote branches have to be the same -exec git config push.default simple - -# Prepare a branch where changes are pulled from the default branch instead of remote branch of same name -exec git checkout -b feature-branch origin/main - -# Create the PR -exec git commit --allow-empty -m 'Empty Commit' -exec git push origin feature-branch -exec gh pr create -H feature-branch --title 'Feature Title' --body 'Feature Body' - -# View the PR -exec gh pr view -stdout 'Feature Title' - -# Check the PR status -env PR_STATUS_BRANCH=#1 Feature Title [feature-branch] -exec gh pr status -stdout $PR_STATUS_BRANCH From be250b3d332d42fe16bf2b420df2debfc2482a1c Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 16 Dec 2024 15:36:43 -0500 Subject: [PATCH 07/23] Add renamed acceptance tests --- ...ew-status-respects-branch-pushremote.txtar | 45 +++++++++++++++++++ ...iew-status-respects-push-destination.txtar | 38 ++++++++++++++++ ...w-status-respects-remote-pushdefault.txtar | 45 +++++++++++++++++++ ...w-status-respects-simple-pushdefault.txtar | 35 +++++++++++++++ 4 files changed, 163 insertions(+) create mode 100644 acceptance/testdata/pr/pr-view-status-respects-branch-pushremote.txtar create mode 100644 acceptance/testdata/pr/pr-view-status-respects-push-destination.txtar create mode 100644 acceptance/testdata/pr/pr-view-status-respects-remote-pushdefault.txtar create mode 100644 acceptance/testdata/pr/pr-view-status-respects-simple-pushdefault.txtar diff --git a/acceptance/testdata/pr/pr-view-status-respects-branch-pushremote.txtar b/acceptance/testdata/pr/pr-view-status-respects-branch-pushremote.txtar new file mode 100644 index 000000000..ef80cd8ba --- /dev/null +++ b/acceptance/testdata/pr/pr-view-status-respects-branch-pushremote.txtar @@ -0,0 +1,45 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} +env FORK=${REPO}-fork + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository to act as upstream with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup of upstream +defer gh repo delete --yes ${ORG}/${REPO} +exec gh repo view ${ORG}/${REPO} --json id --jq '.id' +stdout2env REPO_ID + +# Create a user fork of repository as opposed to private organization fork +exec gh repo fork ${ORG}/${REPO} --org ${ORG} --fork-name ${FORK} + +# Defer repo cleanup of fork +defer gh repo delete --yes ${ORG}/${FORK} +sleep 5 +exec gh repo view ${ORG}/${FORK} --json id --jq '.id' +stdout2env FORK_ID + +# Clone the repo +exec gh repo clone ${ORG}/${FORK} +cd ${FORK} + +# Prepare a branch where changes are pulled from the upstream default branch but pushed to fork +exec git checkout -b feature-branch upstream/main +exec git config branch.feature-branch.pushRemote origin +exec git commit --allow-empty -m 'Empty Commit' +exec git push + +# Create the PR spanning upstream and fork repositories, gh pr create does not support headRepositoryId needed for private forks +exec gh api graphql -F repositoryId="${REPO_ID}" -F headRepositoryId="${FORK_ID}" -F query='mutation CreatePullRequest($headRepositoryId: ID!, $repositoryId: ID!) { createPullRequest(input:{ baseRefName: "main", body: "Feature Body", draft: false, headRefName: "feature-branch", headRepositoryId: $headRepositoryId, repositoryId: $repositoryId, title:"Feature Title" }){ pullRequest{ id url } } }' + +# View the PR +exec gh pr view +stdout 'Feature Title' + +# Check the PR status +env PR_STATUS_BRANCH=#1 Feature Title [${ORG}:feature-branch] +exec gh pr status +stdout $PR_STATUS_BRANCH diff --git a/acceptance/testdata/pr/pr-view-status-respects-push-destination.txtar b/acceptance/testdata/pr/pr-view-status-respects-push-destination.txtar new file mode 100644 index 000000000..ff9db4037 --- /dev/null +++ b/acceptance/testdata/pr/pr-view-status-respects-push-destination.txtar @@ -0,0 +1,38 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes ${ORG}/${REPO} + +# Clone the repo +exec gh repo clone ${ORG}/${REPO} +cd ${REPO} + +# Configure default push behavior so local and remote branches will be the same +exec git config push.default current + +# Prepare a branch where changes are pulled from the default branch instead of remote branch of same name +exec git checkout -b feature-branch +exec git branch --set-upstream-to origin/main +exec git rev-parse --abbrev-ref feature-branch@{upstream} +stdout origin/main + +# Create the PR +exec git commit --allow-empty -m 'Empty Commit' +exec git push +exec gh pr create -B main -H feature-branch --title 'Feature Title' --body 'Feature Body' + +# View the PR +exec gh pr view +stdout 'Feature Title' + +# Check the PR status +env PR_STATUS_BRANCH=#1 Feature Title [feature-branch] +exec gh pr status +stdout $PR_STATUS_BRANCH diff --git a/acceptance/testdata/pr/pr-view-status-respects-remote-pushdefault.txtar b/acceptance/testdata/pr/pr-view-status-respects-remote-pushdefault.txtar new file mode 100644 index 000000000..8bfac2837 --- /dev/null +++ b/acceptance/testdata/pr/pr-view-status-respects-remote-pushdefault.txtar @@ -0,0 +1,45 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} +env FORK=${REPO}-fork + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository to act as upstream with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup of upstream +defer gh repo delete --yes ${ORG}/${REPO} +exec gh repo view ${ORG}/${REPO} --json id --jq '.id' +stdout2env REPO_ID + +# Create a user fork of repository as opposed to private organization fork +exec gh repo fork ${ORG}/${REPO} --org ${ORG} --fork-name ${FORK} + +# Defer repo cleanup of fork +defer gh repo delete --yes ${ORG}/${FORK} +sleep 5 +exec gh repo view ${ORG}/${FORK} --json id --jq '.id' +stdout2env FORK_ID + +# Clone the repo +exec gh repo clone ${ORG}/${FORK} +cd ${FORK} + +# Prepare a branch where changes are pulled from the upstream default branch but pushed to fork +exec git checkout -b feature-branch upstream/main +exec git config remote.pushDefault origin +exec git commit --allow-empty -m 'Empty Commit' +exec git push + +# Create the PR spanning upstream and fork repositories, gh pr create does not support headRepositoryId needed for private forks +exec gh api graphql -F repositoryId="${REPO_ID}" -F headRepositoryId="${FORK_ID}" -F query='mutation CreatePullRequest($headRepositoryId: ID!, $repositoryId: ID!) { createPullRequest(input:{ baseRefName: "main", body: "Feature Body", draft: false, headRefName: "feature-branch", headRepositoryId: $headRepositoryId, repositoryId: $repositoryId, title:"Feature Title" }){ pullRequest{ id url } } }' + +# View the PR +exec gh pr view +stdout 'Feature Title' + +# Check the PR status +env PR_STATUS_BRANCH=#1 Feature Title [${ORG}:feature-branch] +exec gh pr status +stdout $PR_STATUS_BRANCH diff --git a/acceptance/testdata/pr/pr-view-status-respects-simple-pushdefault.txtar b/acceptance/testdata/pr/pr-view-status-respects-simple-pushdefault.txtar new file mode 100644 index 000000000..114f401ec --- /dev/null +++ b/acceptance/testdata/pr/pr-view-status-respects-simple-pushdefault.txtar @@ -0,0 +1,35 @@ +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes ${ORG}/${REPO} + +# Clone the repo +exec gh repo clone ${ORG}/${REPO} +cd ${REPO} + +# Configure default push behavior so local and remote branches have to be the same +exec git config push.default simple + +# Prepare a branch where changes are pulled from the default branch instead of remote branch of same name +exec git checkout -b feature-branch origin/main + +# Create the PR +exec git commit --allow-empty -m 'Empty Commit' +exec git push origin feature-branch +exec gh pr create -H feature-branch --title 'Feature Title' --body 'Feature Body' + +# View the PR +exec gh pr view +stdout 'Feature Title' + +# Check the PR status +env PR_STATUS_BRANCH=#1 Feature Title [feature-branch] +exec gh pr status +stdout $PR_STATUS_BRANCH From 0006091d766ce3016ff3f2c84d3dc430bd2349bb Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 18 Dec 2024 12:26:43 +1100 Subject: [PATCH 08/23] Fix up intra-org fork test setup [1] https://github.com/cli/cli/commit/96ac8d6a2fb268a5825e155ae398e640d3e472fd --- pkg/cmd/pr/shared/finder_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index 7c9f1ab57..ffa0615a9 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -456,6 +456,7 @@ func TestFind(t *testing.T) { return "blueberries", nil }, branchConfig: func(branch string) (c git.BranchConfig) { + c.MergeRef = "refs/heads/main" c.RemoteName = "origin" c.PushRemoteName = "origin" c.Push = "origin/blueberries" @@ -465,6 +466,9 @@ func TestFind(t *testing.T) { return context.Remotes{{ Remote: &git.Remote{Name: "origin"}, Repo: ghrepo.New("OWNER", "REPO-FORK"), + }, { + Remote: &git.Remote{Name: "upstream"}, + Repo: ghrepo.New("OWNER", "REPO"), }}, nil }, }, From 018438088e111df9cdc1a36ec11141de85a0bbe6 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Tue, 14 Jan 2025 16:02:41 -0800 Subject: [PATCH 09/23] Add missing git stubs to tests --- pkg/cmd/pr/status/status_test.go | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/pr/status/status_test.go b/pkg/cmd/pr/status/status_test.go index 19a5a4519..97524acba 100644 --- a/pkg/cmd/pr/status/status_test.go +++ b/pkg/cmd/pr/status/status_test.go @@ -100,6 +100,8 @@ func TestPRStatus(t *testing.T) { rs, cleanup := run.Stub() defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") + rs.Register(`git config remote.pushDefault`, 0, "") + rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -130,6 +132,8 @@ func TestPRStatus_reviewsAndChecks(t *testing.T) { rs, cleanup := run.Stub() defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") + rs.Register(`git config remote.pushDefault`, 0, "") + rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -160,6 +164,8 @@ func TestPRStatus_reviewsAndChecksWithStatesByCount(t *testing.T) { rs, cleanup := run.Stub() defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") + rs.Register(`git config remote.pushDefault`, 0, "") + rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") output, err := runCommandWithDetector(http, "blueberries", true, "", &fd.EnabledDetectorMock{}) if err != nil { @@ -189,6 +195,8 @@ func TestPRStatus_currentBranch_showTheMostRecentPR(t *testing.T) { rs, cleanup := run.Stub() defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") + rs.Register(`git config remote.pushDefault`, 0, "") + rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -222,6 +230,8 @@ func TestPRStatus_currentBranch_defaultBranch(t *testing.T) { rs, cleanup := run.Stub() defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") + rs.Register(`git config remote.pushDefault`, 0, "") + rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -261,6 +271,8 @@ func TestPRStatus_currentBranch_Closed(t *testing.T) { rs, cleanup := run.Stub() defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") + rs.Register(`git config remote.pushDefault`, 0, "") + rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -283,6 +295,8 @@ func TestPRStatus_currentBranch_Closed_defaultBranch(t *testing.T) { rs, cleanup := run.Stub() defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") + rs.Register(`git config remote.pushDefault`, 0, "") + rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -305,6 +319,8 @@ func TestPRStatus_currentBranch_Merged(t *testing.T) { rs, cleanup := run.Stub() defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") + rs.Register(`git config remote.pushDefault`, 0, "") + rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -327,6 +343,8 @@ func TestPRStatus_currentBranch_Merged_defaultBranch(t *testing.T) { rs, cleanup := run.Stub() defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") + rs.Register(`git config remote.pushDefault`, 0, "") + rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -349,6 +367,8 @@ func TestPRStatus_blankSlate(t *testing.T) { rs, cleanup := run.Stub() defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") + rs.Register(`git config remote.pushDefault`, 0, "") + rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -407,6 +427,8 @@ func TestPRStatus_detachedHead(t *testing.T) { rs, cleanup := run.Stub() defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") + rs.Register(`git config remote.pushDefault`, 0, "") + rs.Register(`git rev-parse --verify --quiet --abbrev-ref @{push}`, 0, "") output, err := runCommand(http, "", true, "") if err != nil { @@ -434,7 +456,8 @@ Requesting a code review from you func TestPRStatus_error_ReadBranchConfig(t *testing.T) { rs, cleanup := run.Stub() defer cleanup(t) - rs.Register(`git config --get-regexp \^branch\\.`, 1, "") + // We only need the one stub because this fails early + rs.Register(`git config --get-regexp \^branch\\.`, 2, "") _, err := runCommand(initFakeHTTP(), "blueberries", true, "") assert.Error(t, err) } From 4a9fd9508f71abf4dd81d61a3e6b38088a9f775c Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Wed, 15 Jan 2025 16:13:03 -0800 Subject: [PATCH 10/23] Add comments and a bit of code cleanup --- git/client.go | 34 +++++++---- git/client_test.go | 97 +++++++++++++------------------- git/objects.go | 12 ++-- pkg/cmd/pr/status/status.go | 3 +- pkg/cmd/pr/status/status_test.go | 8 +-- 5 files changed, 73 insertions(+), 81 deletions(-) diff --git a/git/client.go b/git/client.go index 82ead58a4..956d13671 100644 --- a/git/client.go +++ b/git/client.go @@ -402,23 +402,25 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (BranchCon return BranchConfig{}, nil } - pushDefaultOut, err := c.Config(ctx, "remote.pushDefault") + // Check to see if there is a pushDefault ref set for the repo + remotePushDefaultOut, err := c.Config(ctx, "remote.pushDefault") if ok := errors.As(err, &gitError); ok && gitError.ExitCode != 1 { return BranchConfig{}, err } - revParseOut, err := c.revParse(ctx, "--verify", "--quiet", "--abbrev-ref", branch+"@{push}") - if ok := errors.As(err, &gitError); ok && gitError.ExitCode != 1 { - return BranchConfig{}, err - } + // Check to see if we can resolve the @{push} revision syntax. This is the easiest way to get + // the name of the push remote. + //We ignore errors resolving simple push.Default settings as these are handled downstream + revParseOut, _ := c.revParse(ctx, "--verify", "--quiet", "--abbrev-ref", branch+"@{push}") - return parseBranchConfig(outputLines(branchCfgOut), strings.TrimSuffix(pushDefaultOut, "\n"), firstLine(revParseOut)), nil + return parseBranchConfig(outputLines(branchCfgOut), strings.TrimSuffix(remotePushDefaultOut, "\n"), firstLine(revParseOut)), nil } -func parseBranchConfig(configLines []string, pushDefault string, revParse string) BranchConfig { +func parseBranchConfig(branchConfigLines []string, remotePushDefault string, revParse string) BranchConfig { var cfg BranchConfig - for _, line := range configLines { + // Read the config lines for the specific branch + for _, line := range branchConfigLines { parts := strings.SplitN(line, " ", 2) if len(parts) < 2 { continue @@ -429,6 +431,7 @@ func parseBranchConfig(configLines []string, pushDefault string, revParse string remoteURL, remoteName := parseRemoteURLOrName(parts[1]) cfg.RemoteURL = remoteURL cfg.RemoteName = remoteName + // pushremote usually indicates a "triangular" workflow case "pushremote": pushRemoteURL, pushRemoteName := parseRemoteURLOrName(parts[1]) cfg.PushRemoteURL = pushRemoteURL @@ -440,18 +443,27 @@ func parseBranchConfig(configLines []string, pushDefault string, revParse string } } + // PushRemote{URL|Name} takes precedence over remotePushDefault, so we'll only + // use remotePushDefault if we don't have a push remote. if cfg.PushRemoteURL == nil && cfg.PushRemoteName == "" { - if pushDefault != "" { - pushRemoteURL, pushRemoteName := parseRemoteURLOrName(pushDefault) + // remotePushDefault usually indicates a "triangular" workflow + if remotePushDefault != "" { + pushRemoteURL, pushRemoteName := parseRemoteURLOrName(remotePushDefault) cfg.PushRemoteURL = pushRemoteURL cfg.PushRemoteName = pushRemoteName } else { + // Without a PushRemote{URL|Name} or a remotePushDefault, we assume that the + // push remote ref is the same as the remote ref. This is likely + // a "centralized" workflow. cfg.PushRemoteName = cfg.RemoteName } } + // The value returned by revParse is the easiest way to get the name of the push ref cfg.Push = revParse - cfg.PushDefaultName = pushDefault + + // Some `gh pr` workflows don't work if this is set to 'simple' or 'current' + cfg.RemotePushDefault = remotePushDefault return cfg } diff --git a/git/client_test.go b/git/client_test.go index 801d7c025..ba13c889a 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -787,7 +787,7 @@ func TestClientReadBranchConfig(t *testing.T) { Stdout: "branch.trunk.remote origin\n", }, `path/to/git config remote.pushDefault`: { - Stdout: "pushdefault", + Stdout: "remotePushDefault", }, `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { Stdout: "origin/trunk", @@ -795,10 +795,10 @@ func TestClientReadBranchConfig(t *testing.T) { }, branch: "trunk", wantBranchConfig: BranchConfig{ - RemoteName: "origin", - PushRemoteName: "pushdefault", - Push: "origin/trunk", - PushDefaultName: "pushdefault", + RemoteName: "origin", + PushRemoteName: "remotePushDefault", + Push: "origin/trunk", + RemotePushDefault: "remotePushDefault", }, wantError: nil, }, @@ -809,7 +809,7 @@ func TestClientReadBranchConfig(t *testing.T) { Stdout: "branch.trunk.remote origin\n", }, `path/to/git config remote.pushDefault`: { - Stdout: "pushdefault", + Stdout: "remotePushDefault", }, `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { ExitStatus: 1, @@ -817,49 +817,28 @@ func TestClientReadBranchConfig(t *testing.T) { }, branch: "trunk", wantBranchConfig: BranchConfig{ - RemoteName: "origin", - PushRemoteName: "pushdefault", - PushDefaultName: "pushdefault", + RemoteName: "origin", + PushRemoteName: "remotePushDefault", + RemotePushDefault: "remotePushDefault", }, wantError: nil, }, { - name: "when git reads the config, pushDefault is set, and rev-parse fails, it should return an empty BranchConfig and the error", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote origin\n", - }, - `path/to/git config remote.pushDefault`: { - Stdout: "pushdefault", - }, - `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { - ExitStatus: 2, - Stderr: "rev-parse error", - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{}, - wantError: &GitError{ - ExitCode: 2, - Stderr: "rev-parse error", - }, - }, - { - name: "when git reads the config but pushdefault fails, it should return and empty BranchConfig and the error", + name: "when git reads the config but remotePushDefault fails, it should return and empty BranchConfig and the error", cmds: mockedCommands{ `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { Stdout: "branch.trunk.remote origin\n", }, `path/to/git config remote.pushDefault`: { ExitStatus: 2, - Stderr: "pushdefault error", + Stderr: "remotePushDefault error", }, }, branch: "trunk", wantBranchConfig: BranchConfig{}, wantError: &GitError{ ExitCode: 2, - Stderr: "pushdefault error", + Stderr: "remotePushDefault error", }, }, { @@ -963,11 +942,11 @@ func TestClientReadBranchConfig(t *testing.T) { }, branch: "trunk", wantBranchConfig: BranchConfig{ - RemoteName: "upstream", - MergeRef: "refs/heads/main", - PushRemoteName: "origin", - Push: "origin/trunk", - PushDefaultName: "origin", + RemoteName: "upstream", + MergeRef: "refs/heads/main", + PushRemoteName: "origin", + Push: "origin/trunk", + RemotePushDefault: "origin", }, }, { @@ -985,10 +964,10 @@ func TestClientReadBranchConfig(t *testing.T) { }, branch: "trunk", wantBranchConfig: BranchConfig{ - RemoteName: "upstream", - MergeRef: "refs/heads/main", - PushRemoteName: "origin", - PushDefaultName: "current", + RemoteName: "upstream", + MergeRef: "refs/heads/main", + PushRemoteName: "origin", + RemotePushDefault: "current", }, }, { @@ -1006,10 +985,10 @@ func TestClientReadBranchConfig(t *testing.T) { }, branch: "trunk", wantBranchConfig: BranchConfig{ - RemoteName: "upstream", - MergeRef: "refs/heads/main", - PushRemoteName: "origin", - PushDefaultName: "origin", + RemoteName: "upstream", + MergeRef: "refs/heads/main", + PushRemoteName: "origin", + RemotePushDefault: "origin", }, }, } @@ -1021,15 +1000,15 @@ func TestClientReadBranchConfig(t *testing.T) { commandContext: cmdCtx, } branchConfig, err := client.ReadBranchConfig(context.Background(), tt.branch) - assert.Equal(t, tt.wantBranchConfig, branchConfig) if tt.wantError != nil { var gitError *GitError require.ErrorAs(t, err, &gitError) assert.Equal(t, tt.wantError.ExitCode, gitError.ExitCode) assert.Equal(t, tt.wantError.Stderr, gitError.Stderr) } else { - assert.NoError(t, err) + require.NoError(t, err) } + assert.Equal(t, tt.wantBranchConfig, branchConfig) }) } } @@ -1082,10 +1061,10 @@ func Test_parseBranchConfig(t *testing.T) { { name: "push default specified", configLines: []string{}, - pushDefault: "pushdefault", + pushDefault: "remotePushDefault", wantBranchConfig: BranchConfig{ - PushRemoteName: "pushdefault", - PushDefaultName: "pushdefault", + PushRemoteName: "remotePushDefault", + RemotePushDefault: "remotePushDefault", }, }, { @@ -1128,15 +1107,15 @@ func Test_parseBranchConfig(t *testing.T) { "branch.trunk.gh-merge-base gh-merge-base", "branch.trunk.merge refs/heads/trunk", }, - pushDefault: "pushdefault", + pushDefault: "remotePushDefault", revParse: "origin/trunk", wantBranchConfig: BranchConfig{ - RemoteName: "remote", - PushRemoteName: "pushremote", - MergeBase: "gh-merge-base", - MergeRef: "refs/heads/trunk", - Push: "origin/trunk", - PushDefaultName: "pushdefault", + RemoteName: "remote", + PushRemoteName: "pushremote", + MergeBase: "gh-merge-base", + MergeRef: "refs/heads/trunk", + Push: "origin/trunk", + RemotePushDefault: "remotePushDefault", }, }, { @@ -1158,7 +1137,7 @@ func Test_parseBranchConfig(t *testing.T) { assert.Equalf(t, tt.wantBranchConfig.MergeBase, branchConfig.MergeBase, "unexpected MergeBase") assert.Equalf(t, tt.wantBranchConfig.PushRemoteName, branchConfig.PushRemoteName, "unexpected PushRemoteName") assert.Equalf(t, tt.wantBranchConfig.Push, branchConfig.Push, "unexpected Push") - assert.Equalf(t, tt.wantBranchConfig.PushDefaultName, branchConfig.PushDefaultName, "unexpected PushDefaultName") + assert.Equalf(t, tt.wantBranchConfig.RemotePushDefault, branchConfig.RemotePushDefault, "unexpected RemotePushDefault") if tt.wantBranchConfig.RemoteURL != nil { assert.Equalf(t, tt.wantBranchConfig.RemoteURL.String(), branchConfig.RemoteURL.String(), "unexpected RemoteURL") } diff --git a/git/objects.go b/git/objects.go index 8a9d086c6..ff4068173 100644 --- a/git/objects.go +++ b/git/objects.go @@ -64,10 +64,10 @@ type BranchConfig struct { RemoteName string RemoteURL *url.URL // MergeBase is the optional base branch to target in a new PR if `--base` is not specified. - MergeBase string - MergeRef string - PushDefaultName string - PushRemoteURL *url.URL - PushRemoteName string - Push string + MergeBase string + MergeRef string + RemotePushDefault string + PushRemoteURL *url.URL + PushRemoteName string + Push string } diff --git a/pkg/cmd/pr/status/status.go b/pkg/cmd/pr/status/status.go index 593f05e63..fe674ad6e 100644 --- a/pkg/cmd/pr/status/status.go +++ b/pkg/cmd/pr/status/status.go @@ -226,8 +226,9 @@ func prSelectorForCurrentBranch(branchConfig git.BranchConfig, baseRepo ghrepo.I if branchOwner != "" { selector := prHeadRef if branchConfig.Push != "" { + // If we have a resolved push revision ref, we defer to that selector = strings.TrimPrefix(branchConfig.Push, branchConfig.PushRemoteName+"/") - } else if (branchConfig.PushDefaultName == "upstream" || branchConfig.PushDefaultName == "tracking") && + } else if (branchConfig.RemotePushDefault == "upstream" || branchConfig.RemotePushDefault == "tracking") && strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { selector = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") } diff --git a/pkg/cmd/pr/status/status_test.go b/pkg/cmd/pr/status/status_test.go index 97524acba..586e22183 100644 --- a/pkg/cmd/pr/status/status_test.go +++ b/pkg/cmd/pr/status/status_test.go @@ -666,8 +666,8 @@ func Test_prSelectorForCurrentBranch(t *testing.T) { Host: "github.com", Path: "Frederick888/playground.git", }, - MergeRef: "refs/heads/main", - PushDefaultName: "upstream", + MergeRef: "refs/heads/main", + RemotePushDefault: "upstream", }, baseRepo: ghrepo.NewWithHost("octocat", "playground", "github.com"), prHeadRef: "Frederick888/main", @@ -690,8 +690,8 @@ func Test_prSelectorForCurrentBranch(t *testing.T) { Host: "github.com", Path: "Frederick888/playground.git", }, - MergeRef: "refs/heads/main", - PushDefaultName: "tracking", + MergeRef: "refs/heads/main", + RemotePushDefault: "tracking", }, baseRepo: ghrepo.NewWithHost("octocat", "playground", "github.com"), prHeadRef: "Frederick888/main", From d289ddd6178aa177f9c564dfc9b299c642b50cff Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Wed, 15 Jan 2025 16:21:06 -0800 Subject: [PATCH 11/23] Use PushRemoteURL instead of RemoteURL in prSelectorForCurrentBranch --- git/client.go | 1 + pkg/cmd/pr/status/status.go | 5 ++--- pkg/cmd/pr/status/status_test.go | 10 +++++----- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/git/client.go b/git/client.go index 956d13671..5e47098c2 100644 --- a/git/client.go +++ b/git/client.go @@ -456,6 +456,7 @@ func parseBranchConfig(branchConfigLines []string, remotePushDefault string, rev // push remote ref is the same as the remote ref. This is likely // a "centralized" workflow. cfg.PushRemoteName = cfg.RemoteName + cfg.PushRemoteURL = cfg.RemoteURL } } diff --git a/pkg/cmd/pr/status/status.go b/pkg/cmd/pr/status/status.go index fe674ad6e..89926bfa2 100644 --- a/pkg/cmd/pr/status/status.go +++ b/pkg/cmd/pr/status/status.go @@ -199,11 +199,10 @@ func prSelectorForCurrentBranch(branchConfig git.BranchConfig, baseRepo ghrepo.I } return prNumber, prHeadRef, nil } - var branchOwner string - if branchConfig.RemoteURL != nil { + if branchConfig.PushRemoteURL != nil { // the branch merges from a remote specified by URL - r, err := ghrepo.FromURL(branchConfig.RemoteURL) + r, err := ghrepo.FromURL(branchConfig.PushRemoteURL) if err != nil { // TODO: We aren't returning the error because we discovered that it was shadowed // before refactoring to its current return pattern. Thus, we aren't confident diff --git a/pkg/cmd/pr/status/status_test.go b/pkg/cmd/pr/status/status_test.go index 586e22183..4e9ca19b0 100644 --- a/pkg/cmd/pr/status/status_test.go +++ b/pkg/cmd/pr/status/status_test.go @@ -494,7 +494,7 @@ func Test_prSelectorForCurrentBranch(t *testing.T) { { name: "Branch merges from a remote specified by URL", branchConfig: git.BranchConfig{ - RemoteURL: &url.URL{ + PushRemoteURL: &url.URL{ Scheme: "ssh", User: url.User("git"), Host: "github.com", @@ -537,7 +537,7 @@ func Test_prSelectorForCurrentBranch(t *testing.T) { { name: "Branch is a fork and merges from a remote specified by URL", branchConfig: git.BranchConfig{ - RemoteURL: &url.URL{ + PushRemoteURL: &url.URL{ Scheme: "ssh", User: url.User("git"), Host: "github.com", @@ -625,7 +625,7 @@ func Test_prSelectorForCurrentBranch(t *testing.T) { { name: "Remote URL errors", branchConfig: git.BranchConfig{ - RemoteURL: &url.URL{ + PushRemoteURL: &url.URL{ Scheme: "ssh", User: url.User("git"), Host: "github.com", @@ -660,7 +660,7 @@ func Test_prSelectorForCurrentBranch(t *testing.T) { { name: "Current branch pushes to default upstream", branchConfig: git.BranchConfig{ - RemoteURL: &url.URL{ + PushRemoteURL: &url.URL{ Scheme: "ssh", User: url.User("git"), Host: "github.com", @@ -684,7 +684,7 @@ func Test_prSelectorForCurrentBranch(t *testing.T) { { name: "Current branch pushes to default tracking", branchConfig: git.BranchConfig{ - RemoteURL: &url.URL{ + PushRemoteURL: &url.URL{ Scheme: "ssh", User: url.User("git"), Host: "github.com", From aef2642581348e56b6031087288c40aae268cc8b Mon Sep 17 00:00:00 2001 From: Frederick Zhang Date: Thu, 16 Jan 2025 15:07:33 +1100 Subject: [PATCH 12/23] fixup! Add comments and a bit of code cleanup --- git/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git/client.go b/git/client.go index 5e47098c2..9ef6c152e 100644 --- a/git/client.go +++ b/git/client.go @@ -410,7 +410,7 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (BranchCon // Check to see if we can resolve the @{push} revision syntax. This is the easiest way to get // the name of the push remote. - //We ignore errors resolving simple push.Default settings as these are handled downstream + // We ignore errors resolving simple push.Default settings as these are handled downstream revParseOut, _ := c.revParse(ctx, "--verify", "--quiet", "--abbrev-ref", branch+"@{push}") return parseBranchConfig(outputLines(branchCfgOut), strings.TrimSuffix(remotePushDefaultOut, "\n"), firstLine(revParseOut)), nil From 41729b004def8e1d827f763a0f236afc5fa448b4 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Wed, 22 Jan 2025 15:39:43 -0800 Subject: [PATCH 13/23] Refactor finder.Find and replace parseCurrentBranch with parsePRRefs I've been struggling horribly to reason through all of this code, and after much mental gymnastics I identified the culprit as the overloaded "branch" string returned by parseCurrentBranch. This value was either the name of the branch that the PR we're looking for is associated with, or that name prepended with the owner's name and a : if we're on a branch, so: PR branch: featureBranch branch == "featureBranch" If on Fork belonging to "ForkOwner" branch == "ForkOwner:featureBranch" Since this extra information was bundled up into this single string, it complicated the responsibilities of parseCurrentBranch's "branch" return value. Thus, I've teased out "branch" into the new PRRefs struct: type PRRefs struct{ BranchName string HeadRepo ghrepo.Interface BaseRepo ghrepo.Interface } This allows the new parsePRRefs function to move all the previous "branch" string's information into structured data, and allows for a new method on PRRefs, GetPRLabel(), to create the string that "branch" previously held to pass into its downstream consumer, namely findForBranch. This also allowed for better test coverage, directly connecting the PRRefs fields to the values contained in the git config. Overall, I am now confident that this is doing what its supposed to do with respect to my understanding of the various central and triangular git workflows we are addressing. --- git/objects.go | 6 +- pkg/cmd/pr/shared/finder.go | 199 ++++++---- pkg/cmd/pr/shared/finder_test.go | 621 +++++++++++++++++++++---------- 3 files changed, 540 insertions(+), 286 deletions(-) diff --git a/git/objects.go b/git/objects.go index ff4068173..c6176fb73 100644 --- a/git/objects.go +++ b/git/objects.go @@ -64,8 +64,10 @@ type BranchConfig struct { RemoteName string RemoteURL *url.URL // MergeBase is the optional base branch to target in a new PR if `--base` is not specified. - MergeBase string - MergeRef string + MergeBase string + MergeRef string + // These are used to handle triangular workflows. They can be defined by either + // a remote.pushDefault or a branch..pushremote value set on the git config. RemotePushDefault string PushRemoteURL *url.URL PushRemoteName string diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index a707b034e..06a66e93d 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -41,9 +41,9 @@ type finder struct { branchConfig func(string) (git.BranchConfig, error) progress progressIndicator - repo ghrepo.Interface - prNumber int - branchName string + baseRefRepo ghrepo.Interface + prNumber int + branchName string } func NewFinder(factory *cmdutil.Factory) PRFinder { @@ -89,6 +89,22 @@ type FindOptions struct { States []string } +type PRRefs struct { + BranchName string + HeadRepo ghrepo.Interface + BaseRepo ghrepo.Interface +} + +// GetPRLabel returns the string that the GitHub API uses to identify the PR. This is +// either just the branch name or, if the PR is originating from a fork, the fork owner +// and the branch name, like :. +func (s *PRRefs) GetPRLabel() string { + if s.HeadRepo == s.BaseRepo { + return s.BranchName + } + return fmt.Sprintf("%s:%s", s.HeadRepo.RepoOwner(), s.BranchName) +} + func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, error) { if len(opts.Fields) == 0 { return nil, nil, errors.New("Find error: no fields specified") @@ -96,26 +112,18 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err if repo, prNumber, err := f.parseURL(opts.Selector); err == nil { f.prNumber = prNumber - f.repo = repo + f.baseRefRepo = repo } - if f.repo == nil { + if f.baseRefRepo == nil { repo, err := f.baseRepoFn() if err != nil { return nil, nil, err } - f.repo = repo + f.baseRefRepo = repo } - if opts.Selector == "" { - if branch, prNumber, err := f.parseCurrentBranch(); err != nil { - return nil, nil, err - } else if prNumber > 0 { - f.prNumber = prNumber - } else { - f.branchName = branch - } - } else if f.prNumber == 0 { + if f.prNumber == 0 && opts.Selector != "" { // If opts.Selector is a valid number then assume it is the // PR number unless opts.BaseBranch is specified. This is a // special case for PR create command which will always want @@ -127,8 +135,28 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err } else { f.branchName = opts.Selector } + } else { + currentBranchName, err := f.branchFn() + if err != nil { + return nil, nil, err + } + f.branchName = currentBranchName } + // Get the branch config for the current branchName + branchConfig, err := f.branchConfig(f.branchName) + if err != nil { + return nil, nil, err + } + + // Determine if the branch is configured to merge to a special PR ref + prHeadRE := regexp.MustCompile(`^refs/pull/(\d+)/head$`) + if m := prHeadRE.FindStringSubmatch(branchConfig.MergeRef); m != nil { + prNumber, _ := strconv.Atoi(m[1]) + f.prNumber = prNumber + } + + // Set up HTTP client httpClient, err := f.httpClient() if err != nil { return nil, nil, err @@ -147,7 +175,7 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err if fields.Contains("isInMergeQueue") || fields.Contains("isMergeQueueEnabled") { cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) - detector := fd.NewDetector(cachedClient, f.repo.RepoHost()) + detector := fd.NewDetector(cachedClient, f.baseRefRepo.RepoHost()) prFeatures, err := detector.PullRequestFeatures() if err != nil { return nil, nil, err @@ -168,36 +196,54 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err if f.prNumber > 0 { if numberFieldOnly { // avoid hitting the API if we already have all the information - return &api.PullRequest{Number: f.prNumber}, f.repo, nil + return &api.PullRequest{Number: f.prNumber}, f.baseRefRepo, nil + } + pr, err = findByNumber(httpClient, f.baseRefRepo, f.prNumber, fields.ToSlice()) + if err != nil { + return pr, f.baseRefRepo, err } - pr, err = findByNumber(httpClient, f.repo, f.prNumber, fields.ToSlice()) } else { - pr, err = findForBranch(httpClient, f.repo, opts.BaseBranch, f.branchName, opts.States, fields.ToSlice()) - } - if err != nil { - return pr, f.repo, err + rems, err := f.remotesFn() + if err != nil { + return nil, nil, err + } + + pushDefault, err := f.pushDefault() + if err != nil { + return nil, nil, err + } + + prRefs, err := parsePRRefs(f.branchName, branchConfig, pushDefault, f.baseRefRepo, rems) + if err != nil { + return nil, nil, err + } + + pr, err = findForBranch(httpClient, f.baseRefRepo, opts.BaseBranch, prRefs.GetPRLabel(), opts.States, fields.ToSlice()) + if err != nil { + return pr, f.baseRefRepo, err + } } g, _ := errgroup.WithContext(context.Background()) if fields.Contains("reviews") { g.Go(func() error { - return preloadPrReviews(httpClient, f.repo, pr) + return preloadPrReviews(httpClient, f.baseRefRepo, pr) }) } if fields.Contains("comments") { g.Go(func() error { - return preloadPrComments(httpClient, f.repo, pr) + return preloadPrComments(httpClient, f.baseRefRepo, pr) }) } if fields.Contains("statusCheckRollup") { g.Go(func() error { - return preloadPrChecks(httpClient, f.repo, pr) + return preloadPrChecks(httpClient, f.baseRefRepo, pr) }) } if getProjectItems { g.Go(func() error { apiClient := api.NewClientFromHTTP(httpClient) - err := api.ProjectsV2ItemsForPullRequest(apiClient, f.repo, pr) + err := api.ProjectsV2ItemsForPullRequest(apiClient, f.baseRefRepo, pr) if err != nil && !api.ProjectsV2IgnorableError(err) { return err } @@ -205,7 +251,7 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err }) } - return pr, f.repo, g.Wait() + return pr, f.baseRefRepo, g.Wait() } var pullURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/pull/(\d+)`) @@ -234,61 +280,53 @@ func (f *finder) parseURL(prURL string) (ghrepo.Interface, int, error) { return repo, prNumber, nil } -var prHeadRE = regexp.MustCompile(`^refs/pull/(\d+)/head$`) - -func (f *finder) parseCurrentBranch() (string, int, error) { - prHeadRef, err := f.branchFn() - if err != nil { - return "", 0, err +func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, pushDefault string, baseRefRepo ghrepo.Interface, rems remotes.Remotes) (PRRefs, error) { + prRefs := PRRefs{ + BaseRepo: baseRefRepo, } - branchConfig, err := f.branchConfig(prHeadRef) - if err != nil { - return "", 0, err - } - - // the branch is configured to merge a special PR head ref - if m := prHeadRE.FindStringSubmatch(branchConfig.MergeRef); m != nil { - prNumber, _ := strconv.Atoi(m[1]) - return "", prNumber, nil - } - - var gitRemoteRepo ghrepo.Interface - if branchConfig.PushRemoteURL != nil { - // the branch merges from a remote specified by URL - if r, err := ghrepo.FromURL(branchConfig.RemoteURL); err == nil { - gitRemoteRepo = r - } - } else if branchConfig.PushRemoteName != "" { - rem, _ := f.remotesFn() - if r, err := rem.FindByName(branchConfig.PushRemoteName); err == nil { - gitRemoteRepo = r + // If @{push} resolves, then we have all the information we need to determine the head repo + // and branch name. It is of the form /. + if branchConfig.Push != "" { + for _, r := range rems { + // Find the remote who's name matches the push prefix + if strings.HasPrefix(branchConfig.Push, r.Name+"/") { + prRefs.BranchName = strings.TrimPrefix(branchConfig.Push, r.Name+"/") + prRefs.HeadRepo = r.Repo + return prRefs, nil + } } } - if gitRemoteRepo != nil { - if branchConfig.Push != "" { - prHeadRef = strings.TrimPrefix(branchConfig.Push, branchConfig.PushRemoteName+"/") - } else if pushDefault, _ := f.pushDefault(); (pushDefault == "upstream" || pushDefault == "tracking") && - strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { - prHeadRef = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") + // To get the HeadRepo, we look to the git config. The PushRemote{Name | URL} comes from + // one of the following, in order of precedence: + // 1. branch..pushRemote + // 2. remote.pushDefault + // 3. branch..remote + if branchConfig.PushRemoteName != "" { + if r, err := rems.FindByName(branchConfig.PushRemoteName); err == nil { + prRefs.HeadRepo = r.Repo } - // prepend `OWNER:` if this branch is pushed to a fork - // This is determined by: - // - The repo having a different owner - // - The repo having the same owner but a different name (private org fork) - // I suspect that the implementation of the second case may be broken in the face - // of a repo rename, where the remote hasn't been updated locally. This is a - // frequent issue in commands that use SmartBaseRepoFunc. It's not any worse than not - // supporting this case at all though. - sameOwner := strings.EqualFold(gitRemoteRepo.RepoOwner(), f.repo.RepoOwner()) - sameOwnerDifferentRepoName := sameOwner && !strings.EqualFold(gitRemoteRepo.RepoName(), f.repo.RepoName()) - if !sameOwner || sameOwnerDifferentRepoName { - prHeadRef = fmt.Sprintf("%s:%s", gitRemoteRepo.RepoOwner(), prHeadRef) + } else if branchConfig.PushRemoteURL != nil { + if r, err := ghrepo.FromURL(branchConfig.PushRemoteURL); err == nil { + prRefs.HeadRepo = r } } - return prHeadRef, 0, nil + // We assume the PR's branch name is the same as whatever f.BranchFn() returned earlier. + // unless the user has specified push.default = upstream or tracking, then we use the + // branch name from the merge ref. + prRefs.BranchName = currentBranchName + if pushDefault == "upstream" || pushDefault == "tracking" { + prRefs.BranchName = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") + } + + // The PR merges from a branch in the same repo as the base branch (usually the default branch) + if prRefs.HeadRepo == nil { + prRefs.HeadRepo = baseRefRepo + } + + return prRefs, nil } func findByNumber(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.PullRequest, error) { @@ -321,7 +359,7 @@ func findByNumber(httpClient *http.Client, repo ghrepo.Interface, number int, fi return &resp.Repository.PullRequest, nil } -func findForBranch(httpClient *http.Client, repo ghrepo.Interface, baseBranch, headBranch string, stateFilters, fields []string) (*api.PullRequest, error) { +func findForBranch(httpClient *http.Client, repo ghrepo.Interface, baseBranch, headBranchWithOwnerIfFork string, stateFilters, fields []string) (*api.PullRequest, error) { type response struct { Repository struct { PullRequests struct { @@ -348,9 +386,9 @@ func findForBranch(httpClient *http.Client, repo ghrepo.Interface, baseBranch, h } }`, api.PullRequestGraphQL(fieldSet.ToSlice())) - branchWithoutOwner := headBranch - if idx := strings.Index(headBranch, ":"); idx >= 0 { - branchWithoutOwner = headBranch[idx+1:] + branchWithoutOwner := headBranchWithOwnerIfFork + if idx := strings.Index(headBranchWithOwnerIfFork, ":"); idx >= 0 { + branchWithoutOwner = headBranchWithOwnerIfFork[idx+1:] } variables := map[string]interface{}{ @@ -373,18 +411,17 @@ func findForBranch(httpClient *http.Client, repo ghrepo.Interface, baseBranch, h }) for _, pr := range prs { - headBranchMatches := pr.HeadLabel() == headBranch + headBranchMatches := pr.HeadLabel() == headBranchWithOwnerIfFork baseBranchEmptyOrMatches := baseBranch == "" || pr.BaseRefName == baseBranch // When the head is the default branch, it doesn't really make sense to show merged or closed PRs. // https://github.com/cli/cli/issues/4263 - isNotClosedOrMergedWhenHeadIsDefault := pr.State == "OPEN" || resp.Repository.DefaultBranchRef.Name != headBranch - + isNotClosedOrMergedWhenHeadIsDefault := pr.State == "OPEN" || resp.Repository.DefaultBranchRef.Name != headBranchWithOwnerIfFork if headBranchMatches && baseBranchEmptyOrMatches && isNotClosedOrMergedWhenHeadIsDefault { return &pr, nil } } - return nil, &NotFoundError{fmt.Errorf("no pull requests found for branch %q", headBranch)} + return nil, &NotFoundError{fmt.Errorf("no pull requests found for branch %q", headBranchWithOwnerIfFork)} } func preloadPrReviews(httpClient *http.Client, repo ghrepo.Interface, pr *api.PullRequest) error { diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index 340f69810..7a0140088 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -18,20 +18,45 @@ type args struct { baseRepoFn func() (ghrepo.Interface, error) branchFn func() (string, error) branchConfig func(string) (git.BranchConfig, error) - remotesFn func() (context.Remotes, error) + pushDefault func() (string, error) + selector string + fields []string + baseBranch string } func TestFind(t *testing.T) { - type args struct { - baseRepoFn func() (ghrepo.Interface, error) - branchFn func() (string, error) - branchConfig func(string) (git.BranchConfig, error) - pushDefault func() (string, error) - remotesFn func() (context.Remotes, error) - selector string - fields []string - baseBranch string + // TODO: Abstract these out meaningfully for reuse in parsePRRefs tests + originOwnerUrl, err := url.Parse("https://github.com/ORIGINOWNER/REPO.git") + if err != nil { + t.Fatal(err) } + remoteOrigin := context.Remote{ + Remote: &git.Remote{ + Name: "origin", + FetchURL: originOwnerUrl, + }, + Repo: ghrepo.New("ORIGINOWNER", "REPO"), + } + remoteOther := context.Remote{ + Remote: &git.Remote{ + Name: "other", + FetchURL: originOwnerUrl, + }, + Repo: ghrepo.New("ORIGINOWNER", "OTHER-REPO"), + } + + upstreamOwnerUrl, err := url.Parse("https://github.com/UPSTREAMOWNER/REPO.git") + if err != nil { + t.Fatal(err) + } + remoteUpstream := context.Remote{ + Remote: &git.Remote{ + Name: "upstream", + FetchURL: upstreamOwnerUrl, + }, + Repo: ghrepo.New("UPSTREAMOWNER", "REPO"), + } + tests := []struct { name string args args @@ -43,11 +68,13 @@ func TestFind(t *testing.T) { { name: "number argument", args: args{ - selector: "13", - fields: []string{"id", "number"}, - baseRepoFn: func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("OWNER/REPO") + selector: "13", + fields: []string{"id", "number"}, + baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), + branchFn: func() (string, error) { + return "blueberries", nil }, + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -57,7 +84,7 @@ func TestFind(t *testing.T) { }}}`)) }, wantPR: 13, - wantRepo: "https://github.com/OWNER/REPO", + wantRepo: "https://github.com/ORIGINOWNER/REPO", }, { name: "number argument with base branch", @@ -65,9 +92,14 @@ func TestFind(t *testing.T) { selector: "13", baseBranch: "main", fields: []string{"id", "number"}, - baseRepoFn: func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("OWNER/REPO") + baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), + branchFn: func() (string, error) { + return "blueberries", nil }, + branchConfig: stubBranchConfig(git.BranchConfig{ + PushRemoteName: remoteOrigin.Remote.Name, + }, nil), + pushDefault: stubPushDefault("simple", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -80,22 +112,25 @@ func TestFind(t *testing.T) { "baseRefName": "main", "headRefName": "13", "isCrossRepository": false, - "headRepositoryOwner": {"login":"OWNER"} + "headRepositoryOwner": {"login":"ORIGINOWNER"} } ]} }}}`)) }, wantPR: 123, - wantRepo: "https://github.com/OWNER/REPO", + wantRepo: "https://github.com/ORIGINOWNER/REPO", }, { name: "baseRepo is error", args: args{ - selector: "13", - fields: []string{"id", "number"}, - baseRepoFn: func() (ghrepo.Interface, error) { - return nil, errors.New("baseRepoErr") + selector: "13", + fields: []string{"id", "number"}, + baseRepoFn: stubBaseRepoFn(nil, errors.New("baseRepoErr")), + branchFn: func() (string, error) { + return "blueberries", nil }, + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), }, wantErr: true, }, @@ -110,24 +145,30 @@ func TestFind(t *testing.T) { { name: "number only", args: args{ - selector: "13", - fields: []string{"number"}, - baseRepoFn: func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("OWNER/REPO") + selector: "13", + fields: []string{"number"}, + baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), + branchFn: func() (string, error) { + return "blueberries", nil }, + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), }, httpStub: nil, wantPR: 13, - wantRepo: "https://github.com/OWNER/REPO", + wantRepo: "https://github.com/ORIGINOWNER/REPO", }, { name: "number with hash argument", args: args{ - selector: "#13", - fields: []string{"id", "number"}, - baseRepoFn: func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("OWNER/REPO") + selector: "#13", + fields: []string{"id", "number"}, + baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), + branchFn: func() (string, error) { + return "blueberries", nil }, + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -137,14 +178,19 @@ func TestFind(t *testing.T) { }}}`)) }, wantPR: 13, - wantRepo: "https://github.com/OWNER/REPO", + wantRepo: "https://github.com/ORIGINOWNER/REPO", }, { - name: "URL argument", + name: "PR URL argument", args: args{ selector: "https://example.org/OWNER/REPO/pull/13/files", fields: []string{"id", "number"}, baseRepoFn: nil, + branchFn: func() (string, error) { + return "blueberries", nil + }, + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -157,13 +203,16 @@ func TestFind(t *testing.T) { wantRepo: "https://example.org/OWNER/REPO", }, { - name: "branch argument", + name: "when provided branch argument with an open and closed PR for that branch name, it returns the open PR", args: args{ - selector: "blueberries", - fields: []string{"id", "number"}, - baseRepoFn: func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("OWNER/REPO") + selector: "blueberries", + fields: []string{"id", "number"}, + baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), + branchFn: func() (string, error) { + return "blueberries", nil }, + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -176,7 +225,7 @@ func TestFind(t *testing.T) { "baseRefName": "main", "headRefName": "blueberries", "isCrossRepository": false, - "headRepositoryOwner": {"login":"OWNER"} + "headRepositoryOwner": {"login":"ORIGINOWNER"} }, { "number": 13, @@ -184,13 +233,13 @@ func TestFind(t *testing.T) { "baseRefName": "main", "headRefName": "blueberries", "isCrossRepository": false, - "headRepositoryOwner": {"login":"OWNER"} + "headRepositoryOwner": {"login":"ORIGINOWNER"} } ]} }}}`)) }, wantPR: 13, - wantRepo: "https://github.com/OWNER/REPO", + wantRepo: "https://github.com/ORIGINOWNER/REPO", }, { name: "branch argument with base branch", @@ -201,6 +250,11 @@ func TestFind(t *testing.T) { baseRepoFn: func() (ghrepo.Interface, error) { return ghrepo.FromFullName("OWNER/REPO") }, + branchFn: func() (string, error) { + return "blueberries", nil + }, + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -240,17 +294,8 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: stubBranchConfig(git.BranchConfig{ - MergeRef: "refs/heads/blueberries", - RemoteName: "origin", - Push: "origin/blueberries", - }, nil), - remotesFn: func() (context.Remotes, error) { - return context.Remotes{{ - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.New("OWNER", "REPO"), - }}, nil - }, + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -283,6 +328,7 @@ func TestFind(t *testing.T) { return "blueberries", nil }, branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -320,114 +366,19 @@ func TestFind(t *testing.T) { wantErr: true, }, { - name: "current branch with upstream configuration", + name: "when the current branch is configured to push to and pull from 'upstream' and push.default = upstream but the repo push/pulls from 'origin', it finds the PR associated with the upstream repo and returns origin as the base repo", args: args{ - selector: "", - fields: []string{"id", "number"}, - baseRepoFn: func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("OWNER/REPO") - }, - branchFn: func() (string, error) { - return "blueberries", nil - }, - pushDefault: func() (string, error) { return "upstream", nil }, - branchConfig: stubBranchConfig(git.BranchConfig{ - MergeRef: "refs/heads/blue-upstream-berries", - RemoteName: "origin", - PushRemoteName: "origin", - Push: "origin/blue-upstream-berries", - }, 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( - 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 upstream RemoteURL 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) (git.BranchConfig, error) { - u, _ := url.Parse("https://github.com/UPSTREAMOWNER/REPO") - return stubBranchConfig(git.BranchConfig{ - MergeRef: "refs/heads/blue-upstream-berries", - RemoteURL: u, - PushRemoteURL: u, - }, nil)(branch) - }, - 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") - }, + selector: "", + fields: []string{"id", "number"}, + baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), branchFn: func() (string, error) { return "blueberries", nil }, branchConfig: stubBranchConfig(git.BranchConfig{ MergeRef: "refs/heads/blue-upstream-berries", - RemoteName: "origin", - PushRemoteName: "origin", - Push: "origin/blue-upstream-berries", + PushRemoteName: "upstream", }, nil), - 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 - }, + pushDefault: stubPushDefault("upstream", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -446,34 +397,55 @@ func TestFind(t *testing.T) { }}}`)) }, wantPR: 13, - wantRepo: "https://github.com/OWNER/REPO", + wantRepo: "https://github.com/ORIGINOWNER/REPO", + }, + { + name: "the current branch is configured to push to and pull from a URL (upstream, in this example) that is different from what the repo is configured to push to and pull from (origin, in this example) and push.default = upstream, it finds the PR associated with the upstream repo and returns origin as the base repo", + args: args{ + selector: "", + fields: []string{"id", "number"}, + baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), + branchFn: func() (string, error) { + return "blueberries", nil + }, + branchConfig: stubBranchConfig(git.BranchConfig{ + MergeRef: "refs/heads/blue-upstream-berries", + PushRemoteURL: remoteUpstream.Remote.FetchURL, + }, nil), + pushDefault: stubPushDefault("upstream", 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/ORIGINOWNER/REPO", }, { name: "current branch with upstream and fork in same org", args: args{ - selector: "", - fields: []string{"id", "number"}, - baseRepoFn: func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("OWNER/REPO") - }, + selector: "", + fields: []string{"id", "number"}, + baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), branchFn: func() (string, error) { return "blueberries", nil }, branchConfig: stubBranchConfig(git.BranchConfig{ - RemoteName: "origin", - MergeRef: "refs/heads/main", - PushRemoteName: "origin", - Push: "origin/blueberries", + Push: "other/blueberries", }, nil), - remotesFn: func() (context.Remotes, error) { - return context.Remotes{{ - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.New("OWNER", "REPO-FORK"), - }, { - Remote: &git.Remote{Name: "upstream"}, - Repo: ghrepo.New("OWNER", "REPO"), - }}, nil - }, + pushDefault: stubPushDefault("simple", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -486,13 +458,13 @@ func TestFind(t *testing.T) { "baseRefName": "main", "headRefName": "blueberries", "isCrossRepository": true, - "headRepositoryOwner": {"login":"OWNER"} + "headRepositoryOwner": {"login":"ORIGINOWNER"} } ]} }}}`)) }, wantPR: 13, - wantRepo: "https://github.com/OWNER/REPO", + wantRepo: "https://github.com/ORIGINOWNER/REPO", }, { name: "current branch made by pr checkout", @@ -533,6 +505,7 @@ func TestFind(t *testing.T) { branchConfig: stubBranchConfig(git.BranchConfig{ MergeRef: "refs/pull/13/head", }, nil), + pushDefault: stubPushDefault("simple", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -597,7 +570,11 @@ func TestFind(t *testing.T) { branchFn: tt.args.branchFn, branchConfig: tt.args.branchConfig, pushDefault: tt.args.pushDefault, - remotesFn: tt.args.remotesFn, + remotesFn: stubRemotes(context.Remotes{ + &remoteOrigin, + &remoteOther, + &remoteUpstream, + }, nil), } pr, repo, err := f.Find(FindOptions{ @@ -630,42 +607,262 @@ func TestFind(t *testing.T) { } } -func Test_parseCurrentBranch(t *testing.T) { +func Test_parsePRRefs(t *testing.T) { + originOwnerUrl, err := url.Parse("https://github.com/ORIGINOWNER/REPO.git") + if err != nil { + t.Fatal(err) + } + remoteOrigin := context.Remote{ + Remote: &git.Remote{ + Name: "origin", + FetchURL: originOwnerUrl, + }, + Repo: ghrepo.New("ORIGINOWNER", "REPO"), + } + remoteOther := context.Remote{ + Remote: &git.Remote{ + Name: "other", + FetchURL: originOwnerUrl, + }, + Repo: ghrepo.New("ORIGINOWNER", "REPO"), + } + + upstreamOwnerUrl, err := url.Parse("https://github.com/UPSTREAMOWNER/REPO.git") + if err != nil { + t.Fatal(err) + } + remoteUpstream := context.Remote{ + Remote: &git.Remote{ + Name: "upstream", + FetchURL: upstreamOwnerUrl, + }, + Repo: ghrepo.New("UPSTREAMOWNER", "REPO"), + } + tests := []struct { - name string - args args - wantSelector string - wantPR int - wantError error + name string + branchConfig git.BranchConfig + pushDefault string + currentBranchName string + baseRefRepo ghrepo.Interface + rems context.Remotes + wantPRRefs PRRefs + wantErr error }{ { - name: "failed branch config", - args: args{ - branchConfig: stubBranchConfig(git.BranchConfig{}, errors.New("branchConfigErr")), - branchFn: func() (string, error) { - return "blueberries", nil - }, + name: "When the branch is called 'blueberries' with an empty branch config, it returns the correct PRRefs", + branchConfig: git.BranchConfig{}, + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, + wantPRRefs: PRRefs{ + BranchName: "blueberries", + HeadRepo: remoteOrigin.Repo, + BaseRepo: remoteOrigin.Repo, }, - wantSelector: "", - wantPR: 0, - wantError: errors.New("branchConfigErr"), + wantErr: nil, + }, + { + name: "When the branch is called 'otherBranch' with an empty branch config, it returns the correct PRRefs", + branchConfig: git.BranchConfig{}, + currentBranchName: "otherBranch", + baseRefRepo: remoteOrigin.Repo, + wantPRRefs: PRRefs{ + BranchName: "otherBranch", + HeadRepo: remoteOrigin.Repo, + BaseRepo: remoteOrigin.Repo, + }, + wantErr: nil, + }, + { + name: "When the branch name doesn't match the branch name in BranchConfig.Push, it returns the BranchConfig.Push branch name", + branchConfig: git.BranchConfig{ + Push: "origin/pushBranch", + }, + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, + rems: context.Remotes{ + &remoteOrigin, + }, + wantPRRefs: PRRefs{ + BranchName: "pushBranch", + HeadRepo: remoteOrigin.Repo, + BaseRepo: remoteOrigin.Repo, + }, + wantErr: nil, + }, + { + name: "When the branch name doesn't match a different branch name in BranchConfig.Push, it returns the BranchConfig.Push branch name", + branchConfig: git.BranchConfig{ + Push: "origin/differentPushBranch", + }, + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, + rems: context.Remotes{ + &remoteOrigin, + }, + wantPRRefs: PRRefs{ + BranchName: "differentPushBranch", + HeadRepo: remoteOrigin.Repo, + BaseRepo: remoteOrigin.Repo, + }, + wantErr: nil, + }, + { + name: "When the branch name doesn't match a different branch name in BranchConfig.Push and the remote isn't 'origin', it returns the BranchConfig.Push branch name", + branchConfig: git.BranchConfig{ + Push: "other/pushBranch", + }, + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, + rems: context.Remotes{ + &remoteOther, + }, + wantPRRefs: PRRefs{ + BranchName: "pushBranch", + HeadRepo: remoteOther.Repo, + BaseRepo: remoteOrigin.Repo, + }, + wantErr: nil, + }, + { + name: "When the push remote is the same as the baseRepo, it returns the baseRepo as the PRRefs HeadRepo", + branchConfig: git.BranchConfig{ + PushRemoteName: remoteOrigin.Remote.Name, + }, + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, + rems: context.Remotes{ + &remoteOrigin, + &remoteUpstream, + }, + wantPRRefs: PRRefs{ + BranchName: "blueberries", + HeadRepo: remoteOrigin.Repo, + BaseRepo: remoteOrigin.Repo, + }, + wantErr: nil, + }, + { + name: "When the push remote is different from the baseRepo, it returns the push remote repo as the PRRefs HeadRepo", + branchConfig: git.BranchConfig{ + PushRemoteName: remoteOrigin.Remote.Name, + }, + currentBranchName: "blueberries", + baseRefRepo: remoteUpstream.Repo, + rems: context.Remotes{ + &remoteOrigin, + &remoteUpstream, + }, + wantPRRefs: PRRefs{ + BranchName: "blueberries", + HeadRepo: remoteOrigin.Repo, + BaseRepo: remoteUpstream.Repo, + }, + wantErr: nil, + }, + { + name: "When the push remote defined by a URL and the baseRepo is different from the push remote, it returns the push remote repo as the PRRefs HeadRepo", + branchConfig: git.BranchConfig{ + PushRemoteURL: remoteOrigin.Remote.FetchURL, + }, + currentBranchName: "blueberries", + baseRefRepo: remoteUpstream.Repo, + rems: context.Remotes{ + &remoteOrigin, + &remoteUpstream, + }, + wantPRRefs: PRRefs{ + BranchName: "blueberries", + HeadRepo: remoteOrigin.Repo, + BaseRepo: remoteUpstream.Repo, + }, + wantErr: nil, + }, + { + name: "When the push remote and merge ref are configured to a different repo and push.default = upstream, it should return the branch name from the other repo", + branchConfig: git.BranchConfig{ + PushRemoteName: remoteUpstream.Remote.Name, + MergeRef: "refs/heads/blue-upstream-berries", + }, + pushDefault: "upstream", + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, + rems: context.Remotes{ + &remoteOrigin, + &remoteUpstream, + }, + wantPRRefs: PRRefs{ + BranchName: "blue-upstream-berries", + HeadRepo: remoteUpstream.Repo, + BaseRepo: remoteOrigin.Repo, + }, + wantErr: nil, + }, + { + name: "When the push remote and merge ref are configured to a different repo and push.default = tracking, it should return the branch name from the other repo", + branchConfig: git.BranchConfig{ + PushRemoteName: remoteUpstream.Remote.Name, + MergeRef: "refs/heads/blue-upstream-berries", + }, + pushDefault: "tracking", + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, + rems: context.Remotes{ + &remoteOrigin, + &remoteUpstream, + }, + wantPRRefs: PRRefs{ + BranchName: "blue-upstream-berries", + HeadRepo: remoteUpstream.Repo, + BaseRepo: remoteOrigin.Repo, + }, + wantErr: nil, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - f := finder{ - httpClient: func() (*http.Client, error) { - return &http.Client{}, nil - }, - baseRepoFn: tt.args.baseRepoFn, - branchFn: tt.args.branchFn, - branchConfig: tt.args.branchConfig, - remotesFn: tt.args.remotesFn, + prRefs, err := parsePRRefs(tt.currentBranchName, tt.branchConfig, tt.pushDefault, tt.baseRefRepo, tt.rems) + if tt.wantErr != nil { + require.Error(t, err) + assert.Equal(t, tt.wantErr, err) + } else { + require.NoError(t, err) } - selector, pr, err := f.parseCurrentBranch() - assert.Equal(t, tt.wantSelector, selector) - assert.Equal(t, tt.wantPR, pr) - assert.Equal(t, tt.wantError, err) + assert.Equal(t, tt.wantPRRefs, prRefs) + }) + } +} + +func TestPRRefs_GetPRLabel(t *testing.T) { + originRepo := ghrepo.New("ORIGINOWNER", "REPO") + upstreamRepo := ghrepo.New("UPSTREAMOWNER", "REPO") + tests := []struct { + name string + prRefs PRRefs + want string + }{ + { + name: "When the HeadRepo and BaseRepo match, it returns the branch name", + prRefs: PRRefs{ + BranchName: "blueberries", + HeadRepo: originRepo, + BaseRepo: originRepo, + }, + want: "blueberries", + }, + { + name: "When the HeadRepo and BaseRepo do not match, it returns the prepended HeadRepo owner to the branch name", + prRefs: PRRefs{ + BranchName: "blueberries", + HeadRepo: originRepo, + BaseRepo: upstreamRepo, + }, + want: "ORIGINOWNER:blueberries", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, tt.prRefs.GetPRLabel()) }) } } @@ -675,3 +872,21 @@ func stubBranchConfig(branchConfig git.BranchConfig, err error) func(string) (gi return branchConfig, err } } + +func stubRemotes(remotes context.Remotes, err error) func() (context.Remotes, error) { + return func() (context.Remotes, error) { + return remotes, err + } +} + +func stubBaseRepoFn(baseRepo ghrepo.Interface, err error) func() (ghrepo.Interface, error) { + return func() (ghrepo.Interface, error) { + return baseRepo, err + } +} + +func stubPushDefault(pushDefault string, err error) func() (string, error) { + return func() (string, error) { + return pushDefault, err + } +} From a72bef9b42e29667d00f0f2032ee9f4336709d9b Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 24 Jan 2025 17:07:24 +0100 Subject: [PATCH 14/23] Error if push revision doesn't match a remote --- pkg/cmd/pr/shared/finder.go | 8 +++++++- pkg/cmd/pr/shared/finder_test.go | 35 +++++++++++++++----------------- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index 06a66e93d..4aa58af96 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -99,7 +99,7 @@ type PRRefs struct { // either just the branch name or, if the PR is originating from a fork, the fork owner // and the branch name, like :. func (s *PRRefs) GetPRLabel() string { - if s.HeadRepo == s.BaseRepo { + if ghrepo.IsSame(s.HeadRepo, s.BaseRepo) { return s.BranchName } return fmt.Sprintf("%s:%s", s.HeadRepo.RepoOwner(), s.BranchName) @@ -296,6 +296,12 @@ func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, pushDe return prRefs, nil } } + + remoteNames := make([]string, len(rems)) + for i, r := range rems { + remoteNames[i] = r.Name + } + return PRRefs{}, fmt.Errorf("no remote for %q found in %q", branchConfig.Push, strings.Join(remoteNames, ", ")) } // To get the HeadRepo, we look to the git config. The PushRemote{Name | URL} comes from diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index 7a0140088..fe01e6b6e 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -2,6 +2,7 @@ package shared import ( "errors" + "fmt" "net/http" "net/url" "testing" @@ -70,7 +71,7 @@ func TestFind(t *testing.T) { args: args{ selector: "13", fields: []string{"id", "number"}, - baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), + baseRepoFn: stubBaseRepoFn(ghrepo.New("ORIGINOWNER", "REPO"), nil), branchFn: func() (string, error) { return "blueberries", nil }, @@ -92,7 +93,7 @@ func TestFind(t *testing.T) { selector: "13", baseBranch: "main", fields: []string{"id", "number"}, - baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), + baseRepoFn: stubBaseRepoFn(ghrepo.New("ORIGINOWNER", "REPO"), nil), branchFn: func() (string, error) { return "blueberries", nil }, @@ -147,7 +148,7 @@ func TestFind(t *testing.T) { args: args{ selector: "13", fields: []string{"number"}, - baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), + baseRepoFn: stubBaseRepoFn(ghrepo.New("ORIGINOWNER", "REPO"), nil), branchFn: func() (string, error) { return "blueberries", nil }, @@ -163,7 +164,7 @@ func TestFind(t *testing.T) { args: args{ selector: "#13", fields: []string{"id", "number"}, - baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), + baseRepoFn: stubBaseRepoFn(ghrepo.New("ORIGINOWNER", "REPO"), nil), branchFn: func() (string, error) { return "blueberries", nil }, @@ -207,7 +208,7 @@ func TestFind(t *testing.T) { args: args{ selector: "blueberries", fields: []string{"id", "number"}, - baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), + baseRepoFn: stubBaseRepoFn(ghrepo.New("ORIGINOWNER", "REPO"), nil), branchFn: func() (string, error) { return "blueberries", nil }, @@ -370,7 +371,7 @@ func TestFind(t *testing.T) { args: args{ selector: "", fields: []string{"id", "number"}, - baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), + baseRepoFn: stubBaseRepoFn(ghrepo.New("ORIGINOWNER", "REPO"), nil), branchFn: func() (string, error) { return "blueberries", nil }, @@ -404,7 +405,7 @@ func TestFind(t *testing.T) { args: args{ selector: "", fields: []string{"id", "number"}, - baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), + baseRepoFn: stubBaseRepoFn(ghrepo.New("ORIGINOWNER", "REPO"), nil), branchFn: func() (string, error) { return "blueberries", nil }, @@ -438,7 +439,7 @@ func TestFind(t *testing.T) { args: args{ selector: "", fields: []string{"id", "number"}, - baseRepoFn: stubBaseRepoFn(remoteOrigin.Repo, nil), + baseRepoFn: stubBaseRepoFn(ghrepo.New("ORIGINOWNER", "REPO"), nil), branchFn: func() (string, error) { return "blueberries", nil }, @@ -691,21 +692,18 @@ func Test_parsePRRefs(t *testing.T) { wantErr: nil, }, { - name: "When the branch name doesn't match a different branch name in BranchConfig.Push, it returns the BranchConfig.Push branch name", + name: "When the push revision doesn't match a remote, it returns an error", branchConfig: git.BranchConfig{ Push: "origin/differentPushBranch", }, currentBranchName: "blueberries", baseRefRepo: remoteOrigin.Repo, rems: context.Remotes{ - &remoteOrigin, + &remoteUpstream, + &remoteOther, }, - wantPRRefs: PRRefs{ - BranchName: "differentPushBranch", - HeadRepo: remoteOrigin.Repo, - BaseRepo: remoteOrigin.Repo, - }, - wantErr: nil, + wantPRRefs: PRRefs{}, + wantErr: fmt.Errorf("no remote for %q found in %q", "origin/differentPushBranch", "upstream, other"), }, { name: "When the branch name doesn't match a different branch name in BranchConfig.Push and the remote isn't 'origin', it returns the BranchConfig.Push branch name", @@ -823,12 +821,11 @@ func Test_parsePRRefs(t *testing.T) { t.Run(tt.name, func(t *testing.T) { prRefs, err := parsePRRefs(tt.currentBranchName, tt.branchConfig, tt.pushDefault, tt.baseRefRepo, tt.rems) if tt.wantErr != nil { - require.Error(t, err) - assert.Equal(t, tt.wantErr, err) + require.Equal(t, tt.wantErr, err) } else { require.NoError(t, err) } - assert.Equal(t, tt.wantPRRefs, prRefs) + require.Equal(t, tt.wantPRRefs, prRefs) }) } } From 6355ed7c08e1a6a76cb10b5b461750cdee034dcb Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 24 Jan 2025 17:25:38 +0100 Subject: [PATCH 15/23] WIP: push default defaults to simple --- pkg/cmd/pr/shared/finder.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index 4aa58af96..57db40a85 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -59,7 +59,17 @@ func NewFinder(factory *cmdutil.Factory) PRFinder { remotesFn: factory.Remotes, httpClient: factory.HttpClient, pushDefault: func() (string, error) { - return factory.GitClient.Config(context.Background(), "push.default") + pushDefault, err := factory.GitClient.Config(context.Background(), "push.default") + if err == nil { + return pushDefault, nil + } + + var gitErr *git.GitError + if ok := errors.As(err, &gitErr); ok && gitErr.ExitCode == 1 { + return "simple", nil + } + + return "", err }, progress: factory.IOStreams, branchConfig: func(s string) (git.BranchConfig, error) { From 5a8dd35ba7e99600b43ffa37fd8cace328ce9ab2 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 24 Jan 2025 09:40:02 -0800 Subject: [PATCH 16/23] Add PushDefault method to git client --- git/client.go | 15 +++++++++ git/client_test.go | 62 +++++++++++++++++++++++++++++++++++++ pkg/cmd/pr/shared/finder.go | 12 +------ 3 files changed, 78 insertions(+), 11 deletions(-) diff --git a/git/client.go b/git/client.go index 9ef6c152e..c1951f07b 100644 --- a/git/client.go +++ b/git/client.go @@ -482,6 +482,21 @@ func (c *Client) SetBranchConfig(ctx context.Context, branch, name, value string return err } +// PushDefault returns the value of push.default in the config. If the value +// is not set, it returns "simple" (the default value). +func (c *Client) PushDefault(ctx context.Context) (string, error) { + pushDefault, err := c.Config(ctx, "push.default") + if err == nil { + return pushDefault, nil + } + + var gitError *GitError + if ok := errors.As(err, &gitError); ok && gitError.ExitCode == 1 { + return "simple", nil + } + return "", err +} + func (c *Client) DeleteLocalTag(ctx context.Context, tag string) error { args := []string{"tag", "-d", tag} cmd, err := c.Command(ctx, args...) diff --git a/git/client_test.go b/git/client_test.go index ba13c889a..4e35879d9 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -1191,6 +1191,68 @@ func Test_parseRemoteURLOrName(t *testing.T) { } } +func TestClientPushDefault(t *testing.T) { + tests := []struct { + name string + commandResult commandResult + wantPushDefault string + wantError *GitError + }{ + { + name: "push default is not set", + commandResult: commandResult{ + ExitStatus: 1, + Stderr: "error: key does not contain a section: remote.pushDefault", + }, + wantPushDefault: "simple", + wantError: nil, + }, + { + name: "push default is set to current", + commandResult: commandResult{ + ExitStatus: 0, + Stdout: "current", + }, + wantPushDefault: "current", + wantError: nil, + }, + { + name: "push default errors", + commandResult: commandResult{ + ExitStatus: 128, + Stderr: "fatal: git error", + }, + wantPushDefault: "", + wantError: &GitError{ + ExitCode: 128, + Stderr: "fatal: git error", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmdCtx := createMockedCommandContext(t, mockedCommands{ + `path/to/git config push.default`: tt.commandResult, + }, + ) + client := Client{ + GitPath: "path/to/git", + commandContext: cmdCtx, + } + pushDefault, err := client.PushDefault(context.Background()) + if tt.wantError != nil { + var gitError *GitError + require.ErrorAs(t, err, &gitError) + assert.Equal(t, tt.wantError.ExitCode, gitError.ExitCode) + assert.Equal(t, tt.wantError.Stderr, gitError.Stderr) + } else { + require.NoError(t, err) + } + assert.Equal(t, tt.wantPushDefault, pushDefault) + }) + } +} + func TestClientDeleteLocalTag(t *testing.T) { tests := []struct { name string diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index 57db40a85..ce98a9363 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -59,17 +59,7 @@ func NewFinder(factory *cmdutil.Factory) PRFinder { remotesFn: factory.Remotes, httpClient: factory.HttpClient, pushDefault: func() (string, error) { - pushDefault, err := factory.GitClient.Config(context.Background(), "push.default") - if err == nil { - return pushDefault, nil - } - - var gitErr *git.GitError - if ok := errors.As(err, &gitErr); ok && gitErr.ExitCode == 1 { - return "simple", nil - } - - return "", err + return factory.GitClient.PushDefault(context.Background()) }, progress: factory.IOStreams, branchConfig: func(s string) (git.BranchConfig, error) { From e4d8ed0e6091ef4cc5961d3ffe08a293a2a8420c Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 24 Jan 2025 10:20:04 -0800 Subject: [PATCH 17/23] Remove @{push} from branch config --- git/client.go | 21 +++--- git/client_test.go | 109 +++++++++++++++-------------- git/objects.go | 3 +- pkg/cmd/pr/shared/finder.go | 33 +++++---- pkg/cmd/pr/shared/finder_test.go | 115 +++++++++++++++++-------------- 5 files changed, 151 insertions(+), 130 deletions(-) diff --git a/git/client.go b/git/client.go index c1951f07b..c188b27bb 100644 --- a/git/client.go +++ b/git/client.go @@ -408,15 +408,10 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (BranchCon return BranchConfig{}, err } - // Check to see if we can resolve the @{push} revision syntax. This is the easiest way to get - // the name of the push remote. - // We ignore errors resolving simple push.Default settings as these are handled downstream - revParseOut, _ := c.revParse(ctx, "--verify", "--quiet", "--abbrev-ref", branch+"@{push}") - - return parseBranchConfig(outputLines(branchCfgOut), strings.TrimSuffix(remotePushDefaultOut, "\n"), firstLine(revParseOut)), nil + return parseBranchConfig(outputLines(branchCfgOut), strings.TrimSuffix(remotePushDefaultOut, "\n")), nil } -func parseBranchConfig(branchConfigLines []string, remotePushDefault string, revParse string) BranchConfig { +func parseBranchConfig(branchConfigLines []string, remotePushDefault string) BranchConfig { var cfg BranchConfig // Read the config lines for the specific branch @@ -460,9 +455,6 @@ func parseBranchConfig(branchConfigLines []string, remotePushDefault string, rev } } - // The value returned by revParse is the easiest way to get the name of the push ref - cfg.Push = revParse - // Some `gh pr` workflows don't work if this is set to 'simple' or 'current' cfg.RemotePushDefault = remotePushDefault @@ -497,6 +489,15 @@ func (c *Client) PushDefault(ctx context.Context) (string, error) { return "", err } +// ParsePushRevision gets the value of the @{push} revision syntax +// An error here doesn't necessarily mean something are broke, but may mean that the @{push} +// revision syntax couldn't be resolved, such as in non-centralized workflows with +// push.default = simple. Downstream consumers should consider how to handle this error. +func (c *Client) ParsePushRevision(ctx context.Context, branch string) (string, error) { + revParseOut, err := c.revParse(ctx, "--abbrev-ref", branch+"@{push}") + return firstLine(revParseOut), err +} + func (c *Client) DeleteLocalTag(ctx context.Context, tag string) error { args := []string{"tag", "-d", tag} cmd, err := c.Command(ctx, args...) diff --git a/git/client_test.go b/git/client_test.go index 4e35879d9..5d8ee9d65 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -760,7 +760,7 @@ func TestClientReadBranchConfig(t *testing.T) { }, }, { - name: "when the git reads the config, pushDefault isn't set, and rev-parse succeeds, it should return the correct BranchConfig", + name: "when the git reads the config, pushDefault isn't set, it should return the correct BranchConfig", cmds: mockedCommands{ `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { Stdout: "branch.trunk.remote origin\n", @@ -768,20 +768,16 @@ func TestClientReadBranchConfig(t *testing.T) { `path/to/git config remote.pushDefault`: { ExitStatus: 1, }, - `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { - Stdout: "origin/trunk", - }, }, branch: "trunk", wantBranchConfig: BranchConfig{ RemoteName: "origin", PushRemoteName: "origin", - Push: "origin/trunk", }, wantError: nil, }, { - name: "when the git reads the config, pushDefault is set, and rev-parse succeeds, it should return the correct BranchConfig", + name: "when the git reads the config, pushDefault is set, it should return the correct BranchConfig", cmds: mockedCommands{ `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { Stdout: "branch.trunk.remote origin\n", @@ -789,15 +785,11 @@ func TestClientReadBranchConfig(t *testing.T) { `path/to/git config remote.pushDefault`: { Stdout: "remotePushDefault", }, - `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { - Stdout: "origin/trunk", - }, }, branch: "trunk", wantBranchConfig: BranchConfig{ RemoteName: "origin", PushRemoteName: "remotePushDefault", - Push: "origin/trunk", RemotePushDefault: "remotePushDefault", }, wantError: nil, @@ -811,9 +803,6 @@ func TestClientReadBranchConfig(t *testing.T) { `path/to/git config remote.pushDefault`: { Stdout: "remotePushDefault", }, - `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { - ExitStatus: 1, - }, }, branch: "trunk", wantBranchConfig: BranchConfig{ @@ -850,9 +839,6 @@ func TestClientReadBranchConfig(t *testing.T) { `path/to/git config remote.pushDefault`: { ExitStatus: 1, }, - `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { - Stdout: "origin/trunk", - }, }, branch: "trunk", wantBranchConfig: BranchConfig{ @@ -860,7 +846,6 @@ func TestClientReadBranchConfig(t *testing.T) { MergeRef: "refs/heads/trunk", MergeBase: "merge-base", PushRemoteName: "origin", - Push: "origin/trunk", }, wantError: nil, }, @@ -873,16 +858,12 @@ func TestClientReadBranchConfig(t *testing.T) { `path/to/git config remote.pushDefault`: { ExitStatus: 1, }, - `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { - Stdout: "origin/trunk-remote", - }, }, branch: "trunk", wantBranchConfig: BranchConfig{ RemoteName: "origin", MergeRef: "refs/heads/trunk-remote", PushRemoteName: "origin", - Push: "origin/trunk-remote", }, }, { @@ -894,16 +875,12 @@ func TestClientReadBranchConfig(t *testing.T) { `path/to/git config remote.pushDefault`: { ExitStatus: 1, }, - `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { - Stdout: "origin/trunk", - }, }, branch: "trunk", wantBranchConfig: BranchConfig{ RemoteName: "origin", MergeRef: "refs/heads/main", PushRemoteName: "origin", - Push: "origin/trunk", }, }, { @@ -915,16 +892,12 @@ func TestClientReadBranchConfig(t *testing.T) { `path/to/git config remote.pushDefault`: { ExitStatus: 1, }, - `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { - Stdout: "origin/trunk", - }, }, branch: "trunk", wantBranchConfig: BranchConfig{ RemoteName: "upstream", MergeRef: "refs/heads/main", PushRemoteName: "origin", - Push: "origin/trunk", }, }, { @@ -936,16 +909,12 @@ func TestClientReadBranchConfig(t *testing.T) { `path/to/git config remote.pushDefault`: { Stdout: "origin", }, - `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { - Stdout: "origin/trunk", - }, }, branch: "trunk", wantBranchConfig: BranchConfig{ RemoteName: "upstream", MergeRef: "refs/heads/main", PushRemoteName: "origin", - Push: "origin/trunk", RemotePushDefault: "origin", }, }, @@ -958,9 +927,6 @@ func TestClientReadBranchConfig(t *testing.T) { `path/to/git config remote.pushDefault`: { Stdout: "current", }, - `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { - ExitStatus: 1, - }, }, branch: "trunk", wantBranchConfig: BranchConfig{ @@ -979,9 +945,6 @@ func TestClientReadBranchConfig(t *testing.T) { `path/to/git config remote.pushDefault`: { Stdout: "origin", }, - `path/to/git rev-parse --verify --quiet --abbrev-ref trunk@{push}`: { - ExitStatus: 1, - }, }, branch: "trunk", wantBranchConfig: BranchConfig{ @@ -1018,7 +981,6 @@ func Test_parseBranchConfig(t *testing.T) { name string configLines []string pushDefault string - revParse string wantBranchConfig BranchConfig }{ { @@ -1050,14 +1012,6 @@ func Test_parseBranchConfig(t *testing.T) { PushRemoteName: "pushremote", }, }, - { - name: "rev parse specified", - configLines: []string{}, - revParse: "origin/trunk", - wantBranchConfig: BranchConfig{ - Push: "origin/trunk", - }, - }, { name: "push default specified", configLines: []string{}, @@ -1108,13 +1062,11 @@ func Test_parseBranchConfig(t *testing.T) { "branch.trunk.merge refs/heads/trunk", }, pushDefault: "remotePushDefault", - revParse: "origin/trunk", wantBranchConfig: BranchConfig{ RemoteName: "remote", PushRemoteName: "pushremote", MergeBase: "gh-merge-base", MergeRef: "refs/heads/trunk", - Push: "origin/trunk", RemotePushDefault: "remotePushDefault", }, }, @@ -1131,12 +1083,11 @@ func Test_parseBranchConfig(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - branchConfig := parseBranchConfig(tt.configLines, tt.pushDefault, tt.revParse) + branchConfig := parseBranchConfig(tt.configLines, tt.pushDefault) assert.Equalf(t, tt.wantBranchConfig.RemoteName, branchConfig.RemoteName, "unexpected RemoteName") assert.Equalf(t, tt.wantBranchConfig.MergeRef, branchConfig.MergeRef, "unexpected MergeRef") assert.Equalf(t, tt.wantBranchConfig.MergeBase, branchConfig.MergeBase, "unexpected MergeBase") assert.Equalf(t, tt.wantBranchConfig.PushRemoteName, branchConfig.PushRemoteName, "unexpected PushRemoteName") - assert.Equalf(t, tt.wantBranchConfig.Push, branchConfig.Push, "unexpected Push") assert.Equalf(t, tt.wantBranchConfig.RemotePushDefault, branchConfig.RemotePushDefault, "unexpected RemotePushDefault") if tt.wantBranchConfig.RemoteURL != nil { assert.Equalf(t, tt.wantBranchConfig.RemoteURL.String(), branchConfig.RemoteURL.String(), "unexpected RemoteURL") @@ -1253,6 +1204,60 @@ func TestClientPushDefault(t *testing.T) { } } +func TestClientParsePushRevision(t *testing.T) { + tests := []struct { + name string + branch string + commandResult commandResult + wantParsedPushRevision string + wantError *GitError + }{ + { + name: "@{push} resolves to origin/branchName", + branch: "branchName", + commandResult: commandResult{ + ExitStatus: 0, + Stdout: "origin/branchName", + }, + wantParsedPushRevision: "origin/branchName", + }, + { + name: "@{push} doesn't resolve", + commandResult: commandResult{ + ExitStatus: 128, + Stderr: "fatal: git error", + }, + wantParsedPushRevision: "", + wantError: &GitError{ + ExitCode: 128, + Stderr: "fatal: git error", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd := fmt.Sprintf("path/to/git rev-parse --abbrev-ref %s@{push}", tt.branch) + cmdCtx := createMockedCommandContext(t, mockedCommands{ + args(cmd): tt.commandResult, + }) + client := Client{ + GitPath: "path/to/git", + commandContext: cmdCtx, + } + pushDefault, err := client.ParsePushRevision(context.Background(), tt.branch) + if tt.wantError != nil { + var gitError *GitError + require.ErrorAs(t, err, &gitError) + assert.Equal(t, tt.wantError.ExitCode, gitError.ExitCode) + assert.Equal(t, tt.wantError.Stderr, gitError.Stderr) + } else { + require.NoError(t, err) + } + assert.Equal(t, tt.wantParsedPushRevision, pushDefault) + }) + } +} + func TestClientDeleteLocalTag(t *testing.T) { tests := []struct { name string diff --git a/git/objects.go b/git/objects.go index c6176fb73..eab7583ae 100644 --- a/git/objects.go +++ b/git/objects.go @@ -68,8 +68,7 @@ type BranchConfig struct { MergeRef string // These are used to handle triangular workflows. They can be defined by either // a remote.pushDefault or a branch..pushremote value set on the git config. - RemotePushDefault string PushRemoteURL *url.URL PushRemoteName string - Push string + RemotePushDefault string } diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index ce98a9363..d76a62140 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -33,13 +33,14 @@ type progressIndicator interface { } type finder struct { - baseRepoFn func() (ghrepo.Interface, error) - branchFn func() (string, error) - remotesFn func() (remotes.Remotes, error) - httpClient func() (*http.Client, error) - pushDefault func() (string, error) - branchConfig func(string) (git.BranchConfig, error) - progress progressIndicator + baseRepoFn func() (ghrepo.Interface, error) + branchFn func() (string, error) + remotesFn func() (remotes.Remotes, error) + httpClient func() (*http.Client, error) + pushDefault func() (string, error) + parsePushRevision func(string) (string, error) + branchConfig func(string) (git.BranchConfig, error) + progress progressIndicator baseRefRepo ghrepo.Interface prNumber int @@ -61,6 +62,9 @@ func NewFinder(factory *cmdutil.Factory) PRFinder { pushDefault: func() (string, error) { return factory.GitClient.PushDefault(context.Background()) }, + parsePushRevision: func(branch string) (string, error) { + return factory.GitClient.ParsePushRevision(context.Background(), branch) + }, progress: factory.IOStreams, branchConfig: func(s string) (git.BranchConfig, error) { return factory.GitClient.ReadBranchConfig(context.Background(), s) @@ -213,7 +217,10 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err return nil, nil, err } - prRefs, err := parsePRRefs(f.branchName, branchConfig, pushDefault, f.baseRefRepo, rems) + // Suppressing the error as we have other means of computing the PRRefs if this fails. + parsedPushRevision, _ := f.parsePushRevision(f.branchName) + + prRefs, err := parsePRRefs(f.branchName, branchConfig, parsedPushRevision, pushDefault, f.baseRefRepo, rems) if err != nil { return nil, nil, err } @@ -280,18 +287,18 @@ func (f *finder) parseURL(prURL string) (ghrepo.Interface, int, error) { return repo, prNumber, nil } -func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, pushDefault string, baseRefRepo ghrepo.Interface, rems remotes.Remotes) (PRRefs, error) { +func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, parsedPushRevision string, pushDefault string, baseRefRepo ghrepo.Interface, rems remotes.Remotes) (PRRefs, error) { prRefs := PRRefs{ BaseRepo: baseRefRepo, } // If @{push} resolves, then we have all the information we need to determine the head repo // and branch name. It is of the form /. - if branchConfig.Push != "" { + if parsedPushRevision != "" { for _, r := range rems { // Find the remote who's name matches the push prefix - if strings.HasPrefix(branchConfig.Push, r.Name+"/") { - prRefs.BranchName = strings.TrimPrefix(branchConfig.Push, r.Name+"/") + if strings.HasPrefix(parsedPushRevision, r.Name+"/") { + prRefs.BranchName = strings.TrimPrefix(parsedPushRevision, r.Name+"/") prRefs.HeadRepo = r.Repo return prRefs, nil } @@ -301,7 +308,7 @@ func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, pushDe for i, r := range rems { remoteNames[i] = r.Name } - return PRRefs{}, fmt.Errorf("no remote for %q found in %q", branchConfig.Push, strings.Join(remoteNames, ", ")) + return PRRefs{}, fmt.Errorf("no remote for %q found in %q", parsedPushRevision, strings.Join(remoteNames, ", ")) } // To get the HeadRepo, we look to the git config. The PushRemote{Name | URL} comes from diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index fe01e6b6e..db56ce775 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -16,13 +16,14 @@ import ( ) type args struct { - baseRepoFn func() (ghrepo.Interface, error) - branchFn func() (string, error) - branchConfig func(string) (git.BranchConfig, error) - pushDefault func() (string, error) - selector string - fields []string - baseBranch string + baseRepoFn func() (ghrepo.Interface, error) + branchFn func() (string, error) + branchConfig func(string) (git.BranchConfig, error) + pushDefault func() (string, error) + parsePushRevision func(string) (string, error) + selector string + fields []string + baseBranch string } func TestFind(t *testing.T) { @@ -100,7 +101,8 @@ func TestFind(t *testing.T) { branchConfig: stubBranchConfig(git.BranchConfig{ PushRemoteName: remoteOrigin.Remote.Name, }, nil), - pushDefault: stubPushDefault("simple", nil), + pushDefault: stubPushDefault("simple", nil), + parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -212,8 +214,9 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: stubBranchConfig(git.BranchConfig{}, nil), - pushDefault: stubPushDefault("simple", nil), + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), + parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -254,8 +257,9 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: stubBranchConfig(git.BranchConfig{}, nil), - pushDefault: stubPushDefault("simple", nil), + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), + parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -295,8 +299,9 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: stubBranchConfig(git.BranchConfig{}, nil), - pushDefault: stubPushDefault("simple", nil), + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), + parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -328,8 +333,9 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: stubBranchConfig(git.BranchConfig{}, nil), - pushDefault: stubPushDefault("simple", nil), + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), + parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -379,7 +385,8 @@ func TestFind(t *testing.T) { MergeRef: "refs/heads/blue-upstream-berries", PushRemoteName: "upstream", }, nil), - pushDefault: stubPushDefault("upstream", nil), + pushDefault: stubPushDefault("upstream", nil), + parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -413,7 +420,8 @@ func TestFind(t *testing.T) { MergeRef: "refs/heads/blue-upstream-berries", PushRemoteURL: remoteUpstream.Remote.FetchURL, }, nil), - pushDefault: stubPushDefault("upstream", nil), + pushDefault: stubPushDefault("upstream", nil), + parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -443,10 +451,9 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: stubBranchConfig(git.BranchConfig{ - Push: "other/blueberries", - }, nil), - pushDefault: stubPushDefault("simple", nil), + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), + parsePushRevision: stubParsedPushRevision("other/blueberries", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -567,10 +574,11 @@ func TestFind(t *testing.T) { httpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - baseRepoFn: tt.args.baseRepoFn, - branchFn: tt.args.branchFn, - branchConfig: tt.args.branchConfig, - pushDefault: tt.args.pushDefault, + baseRepoFn: tt.args.baseRepoFn, + branchFn: tt.args.branchFn, + branchConfig: tt.args.branchConfig, + pushDefault: tt.args.pushDefault, + parsePushRevision: tt.args.parsePushRevision, remotesFn: stubRemotes(context.Remotes{ &remoteOrigin, &remoteOther, @@ -641,14 +649,15 @@ func Test_parsePRRefs(t *testing.T) { } tests := []struct { - name string - branchConfig git.BranchConfig - pushDefault string - currentBranchName string - baseRefRepo ghrepo.Interface - rems context.Remotes - wantPRRefs PRRefs - wantErr error + name string + branchConfig git.BranchConfig + pushDefault string + parsedPushRevision string + currentBranchName string + baseRefRepo ghrepo.Interface + rems context.Remotes + wantPRRefs PRRefs + wantErr error }{ { name: "When the branch is called 'blueberries' with an empty branch config, it returns the correct PRRefs", @@ -675,12 +684,10 @@ func Test_parsePRRefs(t *testing.T) { wantErr: nil, }, { - name: "When the branch name doesn't match the branch name in BranchConfig.Push, it returns the BranchConfig.Push branch name", - branchConfig: git.BranchConfig{ - Push: "origin/pushBranch", - }, - currentBranchName: "blueberries", - baseRefRepo: remoteOrigin.Repo, + name: "When the branch name doesn't match the branch name in BranchConfig.Push, it returns the BranchConfig.Push branch name", + parsedPushRevision: "origin/pushBranch", + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, rems: context.Remotes{ &remoteOrigin, }, @@ -692,12 +699,10 @@ func Test_parsePRRefs(t *testing.T) { wantErr: nil, }, { - name: "When the push revision doesn't match a remote, it returns an error", - branchConfig: git.BranchConfig{ - Push: "origin/differentPushBranch", - }, - currentBranchName: "blueberries", - baseRefRepo: remoteOrigin.Repo, + name: "When the push revision doesn't match a remote, it returns an error", + parsedPushRevision: "origin/differentPushBranch", + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, rems: context.Remotes{ &remoteUpstream, &remoteOther, @@ -706,12 +711,10 @@ func Test_parsePRRefs(t *testing.T) { wantErr: fmt.Errorf("no remote for %q found in %q", "origin/differentPushBranch", "upstream, other"), }, { - name: "When the branch name doesn't match a different branch name in BranchConfig.Push and the remote isn't 'origin', it returns the BranchConfig.Push branch name", - branchConfig: git.BranchConfig{ - Push: "other/pushBranch", - }, - currentBranchName: "blueberries", - baseRefRepo: remoteOrigin.Repo, + name: "When the branch name doesn't match a different branch name in BranchConfig.Push and the remote isn't 'origin', it returns the BranchConfig.Push branch name", + parsedPushRevision: "other/pushBranch", + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, rems: context.Remotes{ &remoteOther, }, @@ -819,7 +822,7 @@ func Test_parsePRRefs(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - prRefs, err := parsePRRefs(tt.currentBranchName, tt.branchConfig, tt.pushDefault, tt.baseRefRepo, tt.rems) + prRefs, err := parsePRRefs(tt.currentBranchName, tt.branchConfig, tt.parsedPushRevision, tt.pushDefault, tt.baseRefRepo, tt.rems) if tt.wantErr != nil { require.Equal(t, tt.wantErr, err) } else { @@ -887,3 +890,9 @@ func stubPushDefault(pushDefault string, err error) func() (string, error) { return pushDefault, err } } + +func stubParsedPushRevision(parsedPushRevision string, err error) func(string) (string, error) { + return func(_ string) (string, error) { + return parsedPushRevision, err + } +} From cdead50d572c4b9f830873f18724ac07afe4d3e4 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 24 Jan 2025 11:05:15 -0800 Subject: [PATCH 18/23] Moved remote.pushDefault out of ReadBranchConfig and into finder --- git/client.go | 46 ++--- git/client_test.go | 308 +++++++++---------------------- git/objects.go | 5 +- pkg/cmd/pr/shared/finder.go | 46 +++-- pkg/cmd/pr/shared/finder_test.go | 95 +++++++++- 5 files changed, 223 insertions(+), 277 deletions(-) diff --git a/git/client.go b/git/client.go index c188b27bb..be747e962 100644 --- a/git/client.go +++ b/git/client.go @@ -402,16 +402,10 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (BranchCon return BranchConfig{}, nil } - // Check to see if there is a pushDefault ref set for the repo - remotePushDefaultOut, err := c.Config(ctx, "remote.pushDefault") - if ok := errors.As(err, &gitError); ok && gitError.ExitCode != 1 { - return BranchConfig{}, err - } - - return parseBranchConfig(outputLines(branchCfgOut), strings.TrimSuffix(remotePushDefaultOut, "\n")), nil + return parseBranchConfig(outputLines(branchCfgOut)), nil } -func parseBranchConfig(branchConfigLines []string, remotePushDefault string) BranchConfig { +func parseBranchConfig(branchConfigLines []string) BranchConfig { var cfg BranchConfig // Read the config lines for the specific branch @@ -438,26 +432,6 @@ func parseBranchConfig(branchConfigLines []string, remotePushDefault string) Bra } } - // PushRemote{URL|Name} takes precedence over remotePushDefault, so we'll only - // use remotePushDefault if we don't have a push remote. - if cfg.PushRemoteURL == nil && cfg.PushRemoteName == "" { - // remotePushDefault usually indicates a "triangular" workflow - if remotePushDefault != "" { - pushRemoteURL, pushRemoteName := parseRemoteURLOrName(remotePushDefault) - cfg.PushRemoteURL = pushRemoteURL - cfg.PushRemoteName = pushRemoteName - } else { - // Without a PushRemote{URL|Name} or a remotePushDefault, we assume that the - // push remote ref is the same as the remote ref. This is likely - // a "centralized" workflow. - cfg.PushRemoteName = cfg.RemoteName - cfg.PushRemoteURL = cfg.RemoteURL - } - } - - // Some `gh pr` workflows don't work if this is set to 'simple' or 'current' - cfg.RemotePushDefault = remotePushDefault - return cfg } @@ -489,6 +463,22 @@ func (c *Client) PushDefault(ctx context.Context) (string, error) { return "", err } +// RemotePushDefault returns the value of remote.pushDefault in the config. If +// the value is not set, it returns an empty string. +func (c *Client) RemotePushDefault(ctx context.Context) (string, error) { + remotePushDefault, err := c.Config(ctx, "remote.pushDefault") + if err == nil { + return remotePushDefault, nil + } + + var gitError *GitError + if ok := errors.As(err, &gitError); ok && gitError.ExitCode == 1 { + return "", nil + } + + return "", err +} + // ParsePushRevision gets the value of the @{push} revision syntax // An error here doesn't necessarily mean something are broke, but may mean that the @{push} // revision syntax couldn't be resolved, such as in non-centralized workflows with diff --git a/git/client_test.go b/git/client_test.go index 5d8ee9d65..99f783881 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -760,199 +760,20 @@ func TestClientReadBranchConfig(t *testing.T) { }, }, { - name: "when the git reads the config, pushDefault isn't set, it should return the correct BranchConfig", + name: "when the config is read, it should return the correct BranchConfig", cmds: mockedCommands{ `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote origin\n", - }, - `path/to/git config remote.pushDefault`: { - ExitStatus: 1, - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "origin", - PushRemoteName: "origin", - }, - wantError: nil, - }, - { - name: "when the git reads the config, pushDefault is set, it should return the correct BranchConfig", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote origin\n", - }, - `path/to/git config remote.pushDefault`: { - Stdout: "remotePushDefault", - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "origin", - PushRemoteName: "remotePushDefault", - RemotePushDefault: "remotePushDefault", - }, - wantError: nil, - }, - { - name: "when git reads the config, pushDefault is set, an the branch hasn't been pushed, it should return the correct BranchConfig", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote origin\n", - }, - `path/to/git config remote.pushDefault`: { - Stdout: "remotePushDefault", - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "origin", - PushRemoteName: "remotePushDefault", - RemotePushDefault: "remotePushDefault", - }, - wantError: nil, - }, - { - name: "when git reads the config but remotePushDefault fails, it should return and empty BranchConfig and the error", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote origin\n", - }, - `path/to/git config remote.pushDefault`: { - ExitStatus: 2, - Stderr: "remotePushDefault error", - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{}, - wantError: &GitError{ - ExitCode: 2, - Stderr: "remotePushDefault error", - }, - }, - { - name: "read branch config, central", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk\nbranch.trunk.gh-merge-base merge-base", - }, - `path/to/git config remote.pushDefault`: { - ExitStatus: 1, - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "origin", - MergeRef: "refs/heads/trunk", - MergeBase: "merge-base", - PushRemoteName: "origin", - }, - wantError: nil, - }, - { - name: "read branch config, central, push.default = upstream", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk-remote", - }, - `path/to/git config remote.pushDefault`: { - ExitStatus: 1, - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "origin", - MergeRef: "refs/heads/trunk-remote", - PushRemoteName: "origin", - }, - }, - { - name: "read branch config, central, push.default = upstream, no existing remote branch", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/main", - }, - `path/to/git config remote.pushDefault`: { - ExitStatus: 1, - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "origin", - MergeRef: "refs/heads/main", - PushRemoteName: "origin", - }, - }, - { - name: "read branch config, triangular, push.default = current, has existing remote branch, branch.trunk.pushremote effective", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main\nbranch.trunk.pushremote origin", - }, - `path/to/git config remote.pushDefault`: { - ExitStatus: 1, + Stdout: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/trunk\nbranch.trunk.pushremote origin\nbranch.trunk.gh-merge-base gh-merge-base\n", }, }, branch: "trunk", wantBranchConfig: BranchConfig{ RemoteName: "upstream", - MergeRef: "refs/heads/main", PushRemoteName: "origin", + MergeRef: "refs/heads/trunk", + MergeBase: "gh-merge-base", }, - }, - { - name: "read branch config, triangular, push.default = current, has existing remote branch, remote.pushDefault effective", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main", - }, - `path/to/git config remote.pushDefault`: { - Stdout: "origin", - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "upstream", - MergeRef: "refs/heads/main", - PushRemoteName: "origin", - RemotePushDefault: "origin", - }, - }, - { - name: "read branch config, triangular, push.default = current, no existing remote branch, branch.trunk.pushremote effective", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main\nbranch.trunk.pushremote origin", - }, - `path/to/git config remote.pushDefault`: { - Stdout: "current", - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "upstream", - MergeRef: "refs/heads/main", - PushRemoteName: "origin", - RemotePushDefault: "current", - }, - }, - { - name: "read branch config, triangular, push.default = current, no existing remote branch, remote.pushDefault effective", - cmds: mockedCommands{ - `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/main", - }, - `path/to/git config remote.pushDefault`: { - Stdout: "origin", - }, - }, - branch: "trunk", - wantBranchConfig: BranchConfig{ - RemoteName: "upstream", - MergeRef: "refs/heads/main", - PushRemoteName: "origin", - RemotePushDefault: "origin", - }, + wantError: nil, }, } for _, tt := range tests { @@ -980,15 +801,13 @@ func Test_parseBranchConfig(t *testing.T) { tests := []struct { name string configLines []string - pushDefault string wantBranchConfig BranchConfig }{ { name: "remote branch", configLines: []string{"branch.trunk.remote origin"}, wantBranchConfig: BranchConfig{ - RemoteName: "origin", - PushRemoteName: "origin", + RemoteName: "origin", }, }, { @@ -1006,92 +825,73 @@ func Test_parseBranchConfig(t *testing.T) { }, }, { - name: "push remote", + name: "pushremote", configLines: []string{"branch.trunk.pushremote pushremote"}, wantBranchConfig: BranchConfig{ PushRemoteName: "pushremote", }, }, - { - name: "push default specified", - configLines: []string{}, - pushDefault: "remotePushDefault", - wantBranchConfig: BranchConfig{ - PushRemoteName: "remotePushDefault", - RemotePushDefault: "remotePushDefault", - }, - }, { name: "remote and pushremote are specified by name", configLines: []string{ - "branch.trunk.remote origin", - "branch.trunk.pushremote pushremote", + "branch.trunk.remote upstream", + "branch.trunk.pushremote origin", }, wantBranchConfig: BranchConfig{ - RemoteName: "origin", - PushRemoteName: "pushremote", + RemoteName: "upstream", + PushRemoteName: "origin", }, }, { name: "remote and pushremote are specified by url", configLines: []string{ - "branch.Frederick888/main.remote git@github.com:Frederick888/remote.git", - "branch.Frederick888/main.pushremote git@github.com:Frederick888/pushremote.git", + "branch.trunk.remote git@github.com:UPSTREAMOWNER/REPO.git", + "branch.trunk.pushremote git@github.com:ORIGINOWNER/REPO.git", }, wantBranchConfig: BranchConfig{ RemoteURL: &url.URL{ Scheme: "ssh", User: url.User("git"), Host: "github.com", - Path: "/Frederick888/remote.git", + Path: "/UPSTREAMOWNER/REPO.git", }, PushRemoteURL: &url.URL{ Scheme: "ssh", User: url.User("git"), Host: "github.com", - Path: "/Frederick888/pushremote.git", + Path: "/ORIGINOWNER/REPO.git", }, }, }, { - name: "remote, pushremote, gh-merge-base, merge ref, push default, and rev parse all specified", + name: "remote, pushremote, gh-merge-base, and merge ref all specified", configLines: []string{ "branch.trunk.remote remote", "branch.trunk.pushremote pushremote", "branch.trunk.gh-merge-base gh-merge-base", "branch.trunk.merge refs/heads/trunk", }, - pushDefault: "remotePushDefault", - wantBranchConfig: BranchConfig{ - RemoteName: "remote", - PushRemoteName: "pushremote", - MergeBase: "gh-merge-base", - MergeRef: "refs/heads/trunk", - RemotePushDefault: "remotePushDefault", - }, - }, - { - name: "pushremote and pushDefault are not specified, but a remoteName is provided", - configLines: []string{ - "branch.trunk.remote remote", - }, wantBranchConfig: BranchConfig{ RemoteName: "remote", - PushRemoteName: "remote", + PushRemoteName: "pushremote", + MergeBase: "gh-merge-base", + MergeRef: "refs/heads/trunk", }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - branchConfig := parseBranchConfig(tt.configLines, tt.pushDefault) + branchConfig := parseBranchConfig(tt.configLines) assert.Equalf(t, tt.wantBranchConfig.RemoteName, branchConfig.RemoteName, "unexpected RemoteName") assert.Equalf(t, tt.wantBranchConfig.MergeRef, branchConfig.MergeRef, "unexpected MergeRef") assert.Equalf(t, tt.wantBranchConfig.MergeBase, branchConfig.MergeBase, "unexpected MergeBase") assert.Equalf(t, tt.wantBranchConfig.PushRemoteName, branchConfig.PushRemoteName, "unexpected PushRemoteName") - assert.Equalf(t, tt.wantBranchConfig.RemotePushDefault, branchConfig.RemotePushDefault, "unexpected RemotePushDefault") if tt.wantBranchConfig.RemoteURL != nil { assert.Equalf(t, tt.wantBranchConfig.RemoteURL.String(), branchConfig.RemoteURL.String(), "unexpected RemoteURL") } + if tt.wantBranchConfig.PushRemoteURL != nil { + assert.Equalf(t, tt.wantBranchConfig.PushRemoteURL.String(), branchConfig.PushRemoteURL.String(), "unexpected PushRemoteURL") + } }) } } @@ -1204,6 +1004,68 @@ func TestClientPushDefault(t *testing.T) { } } +func TestClientRemotePushDefault(t *testing.T) { + tests := []struct { + name string + commandResult commandResult + wantRemotePushDefault string + wantError *GitError + }{ + { + name: "remote.pushDefault is not set", + commandResult: commandResult{ + ExitStatus: 1, + Stderr: "error: key does not contain a section: remote.pushDefault", + }, + wantRemotePushDefault: "", + wantError: nil, + }, + { + name: "remote.pushDefault is set to origin", + commandResult: commandResult{ + ExitStatus: 0, + Stdout: "origin", + }, + wantRemotePushDefault: "origin", + wantError: nil, + }, + { + name: "remote.pushDefault errors", + commandResult: commandResult{ + ExitStatus: 128, + Stderr: "fatal: git error", + }, + wantRemotePushDefault: "", + wantError: &GitError{ + ExitCode: 128, + Stderr: "fatal: git error", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmdCtx := createMockedCommandContext(t, mockedCommands{ + `path/to/git config remote.pushDefault`: tt.commandResult, + }, + ) + client := Client{ + GitPath: "path/to/git", + commandContext: cmdCtx, + } + pushDefault, err := client.RemotePushDefault(context.Background()) + if tt.wantError != nil { + var gitError *GitError + require.ErrorAs(t, err, &gitError) + assert.Equal(t, tt.wantError.ExitCode, gitError.ExitCode) + assert.Equal(t, tt.wantError.Stderr, gitError.Stderr) + } else { + require.NoError(t, err) + } + assert.Equal(t, tt.wantRemotePushDefault, pushDefault) + }) + } +} + func TestClientParsePushRevision(t *testing.T) { tests := []struct { name string diff --git a/git/objects.go b/git/objects.go index eab7583ae..d66d5b9bb 100644 --- a/git/objects.go +++ b/git/objects.go @@ -68,7 +68,6 @@ type BranchConfig struct { MergeRef string // These are used to handle triangular workflows. They can be defined by either // a remote.pushDefault or a branch..pushremote value set on the git config. - PushRemoteURL *url.URL - PushRemoteName string - RemotePushDefault string + PushRemoteURL *url.URL + PushRemoteName string } diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index d76a62140..8589840a9 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) pushDefault func() (string, error) + remotePushDefault func() (string, error) parsePushRevision func(string) (string, error) branchConfig func(string) (git.BranchConfig, error) progress progressIndicator @@ -62,6 +63,9 @@ func NewFinder(factory *cmdutil.Factory) PRFinder { pushDefault: func() (string, error) { return factory.GitClient.PushDefault(context.Background()) }, + remotePushDefault: func() (string, error) { + return factory.GitClient.RemotePushDefault(context.Background()) + }, parsePushRevision: func(branch string) (string, error) { return factory.GitClient.ParsePushRevision(context.Background(), branch) }, @@ -217,10 +221,15 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err return nil, nil, err } - // Suppressing the error as we have other means of computing the PRRefs if this fails. + // Suppressing these errors as we have other means of computing the PRRefs when these fail. parsedPushRevision, _ := f.parsePushRevision(f.branchName) - prRefs, err := parsePRRefs(f.branchName, branchConfig, parsedPushRevision, pushDefault, f.baseRefRepo, rems) + remotePushDefault, err := f.remotePushDefault() + if err != nil { + return nil, nil, err + } + + prRefs, err := parsePRRefs(f.branchName, branchConfig, parsedPushRevision, pushDefault, remotePushDefault, f.baseRefRepo, rems) if err != nil { return nil, nil, err } @@ -287,7 +296,7 @@ func (f *finder) parseURL(prURL string) (ghrepo.Interface, int, error) { return repo, prNumber, nil } -func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, parsedPushRevision string, pushDefault string, baseRefRepo ghrepo.Interface, rems remotes.Remotes) (PRRefs, error) { +func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, parsedPushRevision string, pushDefault string, remotePushDefault string, baseRefRepo ghrepo.Interface, rems remotes.Remotes) (PRRefs, error) { prRefs := PRRefs{ BaseRepo: baseRefRepo, } @@ -311,8 +320,15 @@ func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, parsed return PRRefs{}, fmt.Errorf("no remote for %q found in %q", parsedPushRevision, strings.Join(remoteNames, ", ")) } - // To get the HeadRepo, we look to the git config. The PushRemote{Name | URL} comes from - // one of the following, in order of precedence: + // We assume the PR's branch name is the same as whatever f.BranchFn() returned earlier + // unless the user has specified push.default = upstream or tracking, then we use the + // branch name from the merge ref. + prRefs.BranchName = currentBranchName + if pushDefault == "upstream" || pushDefault == "tracking" { + prRefs.BranchName = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") + } + + // To get the HeadRepo, we look to the git config. The HeadRepo comes from one of the following, in order of precedence: // 1. branch..pushRemote // 2. remote.pushDefault // 3. branch..remote @@ -324,14 +340,18 @@ func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, parsed if r, err := ghrepo.FromURL(branchConfig.PushRemoteURL); err == nil { prRefs.HeadRepo = r } - } - - // We assume the PR's branch name is the same as whatever f.BranchFn() returned earlier. - // unless the user has specified push.default = upstream or tracking, then we use the - // branch name from the merge ref. - prRefs.BranchName = currentBranchName - if pushDefault == "upstream" || pushDefault == "tracking" { - prRefs.BranchName = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") + } else if remotePushDefault != "" { + if r, err := rems.FindByName(remotePushDefault); err == nil { + prRefs.HeadRepo = r.Repo + } + } else if branchConfig.RemoteName != "" { + if r, err := rems.FindByName(branchConfig.RemoteName); err == nil { + prRefs.HeadRepo = r.Repo + } + } else if branchConfig.RemoteURL != nil { + if r, err := ghrepo.FromURL(branchConfig.RemoteURL); err == nil { + prRefs.HeadRepo = r + } } // The PR merges from a branch in the same repo as the base branch (usually the default branch) diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index db56ce775..89835b028 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -20,6 +20,7 @@ type args struct { branchFn func() (string, error) branchConfig func(string) (git.BranchConfig, error) pushDefault func() (string, error) + remotePushDefault func() (string, error) parsePushRevision func(string) (string, error) selector string fields []string @@ -102,6 +103,7 @@ func TestFind(t *testing.T) { PushRemoteName: remoteOrigin.Remote.Name, }, nil), pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { @@ -132,8 +134,9 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: stubBranchConfig(git.BranchConfig{}, nil), - pushDefault: stubPushDefault("simple", nil), + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), }, wantErr: true, }, @@ -154,8 +157,9 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: stubBranchConfig(git.BranchConfig{}, nil), - pushDefault: stubPushDefault("simple", nil), + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), }, httpStub: nil, wantPR: 13, @@ -170,8 +174,9 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: stubBranchConfig(git.BranchConfig{}, nil), - pushDefault: stubPushDefault("simple", nil), + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -192,8 +197,9 @@ func TestFind(t *testing.T) { branchFn: func() (string, error) { return "blueberries", nil }, - branchConfig: stubBranchConfig(git.BranchConfig{}, nil), - pushDefault: stubPushDefault("simple", nil), + branchConfig: stubBranchConfig(git.BranchConfig{}, nil), + pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -217,6 +223,7 @@ func TestFind(t *testing.T) { branchConfig: stubBranchConfig(git.BranchConfig{}, nil), pushDefault: stubPushDefault("simple", nil), parsePushRevision: stubParsedPushRevision("", nil), + remotePushDefault: stubRemotePushDefault("", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -259,6 +266,7 @@ func TestFind(t *testing.T) { }, branchConfig: stubBranchConfig(git.BranchConfig{}, nil), pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { @@ -301,6 +309,7 @@ func TestFind(t *testing.T) { }, branchConfig: stubBranchConfig(git.BranchConfig{}, nil), pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { @@ -335,6 +344,7 @@ func TestFind(t *testing.T) { }, branchConfig: stubBranchConfig(git.BranchConfig{}, nil), pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { @@ -386,6 +396,7 @@ func TestFind(t *testing.T) { PushRemoteName: "upstream", }, nil), pushDefault: stubPushDefault("upstream", nil), + remotePushDefault: stubRemotePushDefault("", nil), parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { @@ -421,6 +432,7 @@ func TestFind(t *testing.T) { PushRemoteURL: remoteUpstream.Remote.FetchURL, }, nil), pushDefault: stubPushDefault("upstream", nil), + remotePushDefault: stubRemotePushDefault("", nil), parsePushRevision: stubParsedPushRevision("", nil), }, httpStub: func(r *httpmock.Registry) { @@ -453,6 +465,7 @@ func TestFind(t *testing.T) { }, branchConfig: stubBranchConfig(git.BranchConfig{}, nil), pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), parsePushRevision: stubParsedPushRevision("other/blueberries", nil), }, httpStub: func(r *httpmock.Registry) { @@ -513,7 +526,8 @@ func TestFind(t *testing.T) { branchConfig: stubBranchConfig(git.BranchConfig{ MergeRef: "refs/pull/13/head", }, nil), - pushDefault: stubPushDefault("simple", nil), + pushDefault: stubPushDefault("simple", nil), + remotePushDefault: stubRemotePushDefault("", nil), }, httpStub: func(r *httpmock.Registry) { r.Register( @@ -578,6 +592,7 @@ func TestFind(t *testing.T) { branchFn: tt.args.branchFn, branchConfig: tt.args.branchConfig, pushDefault: tt.args.pushDefault, + remotePushDefault: tt.args.remotePushDefault, parsePushRevision: tt.args.parsePushRevision, remotesFn: stubRemotes(context.Remotes{ &remoteOrigin, @@ -653,6 +668,7 @@ func Test_parsePRRefs(t *testing.T) { branchConfig git.BranchConfig pushDefault string parsedPushRevision string + remotePushDefault string currentBranchName string baseRefRepo ghrepo.Interface rems context.Remotes @@ -819,10 +835,63 @@ func Test_parsePRRefs(t *testing.T) { }, wantErr: nil, }, + { + name: "When remote.pushDefault is set, it returns the correct PRRefs", + branchConfig: git.BranchConfig{}, + remotePushDefault: remoteUpstream.Remote.Name, + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, + rems: context.Remotes{ + &remoteOrigin, + &remoteUpstream, + }, + wantPRRefs: PRRefs{ + BranchName: "blueberries", + HeadRepo: remoteUpstream.Repo, + BaseRepo: remoteOrigin.Repo, + }, + wantErr: nil, + }, + { + name: "When the remote name is set on the branch, it returns the correct PRRefs", + branchConfig: git.BranchConfig{ + RemoteName: remoteUpstream.Remote.Name, + }, + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, + rems: context.Remotes{ + &remoteOrigin, + &remoteUpstream, + }, + wantPRRefs: PRRefs{ + BranchName: "blueberries", + HeadRepo: remoteUpstream.Repo, + BaseRepo: remoteOrigin.Repo, + }, + wantErr: nil, + }, + { + name: "When the remote URL is set on the branch, it returns the correct PRRefs", + branchConfig: git.BranchConfig{ + RemoteURL: remoteUpstream.Remote.FetchURL, + }, + currentBranchName: "blueberries", + baseRefRepo: remoteOrigin.Repo, + rems: context.Remotes{ + &remoteOrigin, + &remoteUpstream, + }, + wantPRRefs: PRRefs{ + BranchName: "blueberries", + HeadRepo: remoteUpstream.Repo, + BaseRepo: remoteOrigin.Repo, + }, + wantErr: nil, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - prRefs, err := parsePRRefs(tt.currentBranchName, tt.branchConfig, tt.parsedPushRevision, tt.pushDefault, tt.baseRefRepo, tt.rems) + prRefs, err := parsePRRefs(tt.currentBranchName, tt.branchConfig, tt.parsedPushRevision, tt.pushDefault, tt.remotePushDefault, tt.baseRefRepo, tt.rems) if tt.wantErr != nil { require.Equal(t, tt.wantErr, err) } else { @@ -891,6 +960,12 @@ func stubPushDefault(pushDefault string, err error) func() (string, error) { } } +func stubRemotePushDefault(remotePushDefault string, err error) func() (string, error) { + return func() (string, error) { + return remotePushDefault, err + } +} + func stubParsedPushRevision(parsedPushRevision string, err error) func(string) (string, error) { return func(_ string) (string, error) { return parsedPushRevision, err From d684834ad9728ff5e8bb5db57d869864c6188170 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 24 Jan 2025 11:51:49 -0800 Subject: [PATCH 19/23] Refactor pr status to use the ParsePRRefs helper on the Finder There was a lot of copy-pasta code between the finder and pr status. After some investigation it was clear that the prSelectorForCurrentBranch code was really just a duplicate of the finder's code without actually making the API call for the PR. Since the ParsePRRefs helper had already extracted much of the logic for determining a PR's head ref branch, I was able to reuse it in gh pr status with a small refactor. --- pkg/cmd/pr/shared/finder.go | 4 +- pkg/cmd/pr/shared/finder_test.go | 4 +- pkg/cmd/pr/status/status.go | 110 +++++------- pkg/cmd/pr/status/status_test.go | 292 +++---------------------------- 4 files changed, 70 insertions(+), 340 deletions(-) diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index 8589840a9..ae0ea4553 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -229,7 +229,7 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err return nil, nil, err } - prRefs, err := parsePRRefs(f.branchName, branchConfig, parsedPushRevision, pushDefault, remotePushDefault, f.baseRefRepo, rems) + prRefs, err := ParsePRRefs(f.branchName, branchConfig, parsedPushRevision, pushDefault, remotePushDefault, f.baseRefRepo, rems) if err != nil { return nil, nil, err } @@ -296,7 +296,7 @@ func (f *finder) parseURL(prURL string) (ghrepo.Interface, int, error) { return repo, prNumber, nil } -func parsePRRefs(currentBranchName string, branchConfig git.BranchConfig, parsedPushRevision string, pushDefault string, remotePushDefault string, baseRefRepo ghrepo.Interface, rems remotes.Remotes) (PRRefs, error) { +func ParsePRRefs(currentBranchName string, branchConfig git.BranchConfig, parsedPushRevision string, pushDefault string, remotePushDefault string, baseRefRepo ghrepo.Interface, rems remotes.Remotes) (PRRefs, error) { prRefs := PRRefs{ BaseRepo: baseRefRepo, } diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index 89835b028..1d5c1e3ac 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -631,7 +631,7 @@ func TestFind(t *testing.T) { } } -func Test_parsePRRefs(t *testing.T) { +func TestParsePRRefs(t *testing.T) { originOwnerUrl, err := url.Parse("https://github.com/ORIGINOWNER/REPO.git") if err != nil { t.Fatal(err) @@ -891,7 +891,7 @@ func Test_parsePRRefs(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - prRefs, err := parsePRRefs(tt.currentBranchName, tt.branchConfig, tt.parsedPushRevision, tt.pushDefault, tt.remotePushDefault, tt.baseRefRepo, tt.rems) + prRefs, err := ParsePRRefs(tt.currentBranchName, tt.branchConfig, tt.parsedPushRevision, tt.pushDefault, tt.remotePushDefault, tt.baseRefRepo, tt.rems) if tt.wantErr != nil { require.Equal(t, tt.wantErr, err) } else { diff --git a/pkg/cmd/pr/status/status.go b/pkg/cmd/pr/status/status.go index 89926bfa2..996a178e1 100644 --- a/pkg/cmd/pr/status/status.go +++ b/pkg/cmd/pr/status/status.go @@ -7,7 +7,6 @@ import ( "net/http" "regexp" "strconv" - "strings" "time" "github.com/cli/cli/v2/api" @@ -78,27 +77,56 @@ func statusRun(opts *StatusOptions) error { return err } - baseRepo, err := opts.BaseRepo() + baseRefRepo, err := opts.BaseRepo() if err != nil { return err } - var currentBranch string + var currentBranchName string var currentPRNumber int - var currentPRHeadRef string + var currentHeadRefBranchName string if !opts.HasRepoOverride { - currentBranch, err = opts.Branch() + currentBranchName, err = opts.Branch() if err != nil && !errors.Is(err, git.ErrNotOnAnyBranch) { return fmt.Errorf("could not query for pull request for current branch: %w", err) } - remotes, _ := opts.Remotes() - branchConfig, err := opts.GitClient.ReadBranchConfig(ctx, currentBranch) + branchConfig, err := opts.GitClient.ReadBranchConfig(ctx, currentBranchName) if err != nil { return err } - currentPRNumber, currentPRHeadRef, err = prSelectorForCurrentBranch(branchConfig, baseRepo, currentBranch, remotes) + // Determine if the branch is configured to merge to a special PR ref + prHeadRE := regexp.MustCompile(`^refs/pull/(\d+)/head$`) + if m := prHeadRE.FindStringSubmatch(branchConfig.MergeRef); m != nil { + currentPRNumber, _ = strconv.Atoi(m[1]) + } + + if currentPRNumber == 0 { + remotes, err := opts.Remotes() + if err != nil { + return err + } + // Suppressing these errors as we have other means of computing the PRRefs when these fail. + parsedPushRevision, _ := opts.GitClient.ParsePushRevision(ctx, currentBranchName) + + remotePushDefault, err := opts.GitClient.RemotePushDefault(ctx) + if err != nil { + return err + } + + pushDefault, err := opts.GitClient.PushDefault(ctx) + if err != nil { + return err + } + + prRefs, err := shared.ParsePRRefs(currentBranchName, branchConfig, parsedPushRevision, pushDefault, remotePushDefault, baseRefRepo, remotes) + if err != nil { + return err + } + currentHeadRefBranchName = prRefs.BranchName + } + if err != nil { return fmt.Errorf("could not query for pull request for current branch: %w", err) } @@ -107,7 +135,7 @@ func statusRun(opts *StatusOptions) error { options := requestOptions{ Username: "@me", CurrentPR: currentPRNumber, - HeadRef: currentPRHeadRef, + HeadRef: currentHeadRefBranchName, ConflictStatus: opts.ConflictStatus, } if opts.Exporter != nil { @@ -116,7 +144,7 @@ func statusRun(opts *StatusOptions) error { if opts.Detector == nil { cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) - opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost()) + opts.Detector = fd.NewDetector(cachedClient, baseRefRepo.RepoHost()) } prFeatures, err := opts.Detector.PullRequestFeatures() if err != nil { @@ -124,7 +152,7 @@ func statusRun(opts *StatusOptions) error { } options.CheckRunAndStatusContextCountsSupported = prFeatures.CheckRunAndStatusContextCounts - prPayload, err := pullRequestStatus(httpClient, baseRepo, options) + prPayload, err := pullRequestStatus(httpClient, baseRefRepo, options) if err != nil { return err } @@ -151,21 +179,21 @@ func statusRun(opts *StatusOptions) error { cs := opts.IO.ColorScheme() fmt.Fprintln(out, "") - fmt.Fprintf(out, "Relevant pull requests in %s\n", ghrepo.FullName(baseRepo)) + fmt.Fprintf(out, "Relevant pull requests in %s\n", ghrepo.FullName(baseRefRepo)) fmt.Fprintln(out, "") if !opts.HasRepoOverride { shared.PrintHeader(opts.IO, "Current branch") currentPR := prPayload.CurrentPR - if currentPR != nil && currentPR.State != "OPEN" && prPayload.DefaultBranch == currentBranch { + if currentPR != nil && currentPR.State != "OPEN" && prPayload.DefaultBranch == currentBranchName { currentPR = nil } if currentPR != nil { printPrs(opts.IO, 1, *currentPR) - } else if currentPRHeadRef == "" { + } else if currentHeadRefBranchName == "" { shared.PrintMessage(opts.IO, " There is no current branch") } else { - shared.PrintMessage(opts.IO, fmt.Sprintf(" There is no pull request associated with %s", cs.Cyan("["+currentPRHeadRef+"]"))) + shared.PrintMessage(opts.IO, fmt.Sprintf(" There is no pull request associated with %s", cs.Cyan("["+currentHeadRefBranchName+"]"))) } fmt.Fprintln(out) } @@ -189,58 +217,6 @@ func statusRun(opts *StatusOptions) error { return nil } -func prSelectorForCurrentBranch(branchConfig git.BranchConfig, baseRepo ghrepo.Interface, prHeadRef string, rem ghContext.Remotes) (int, string, error) { - // the branch is configured to merge a special PR head ref - prHeadRE := regexp.MustCompile(`^refs/pull/(\d+)/head$`) - if m := prHeadRE.FindStringSubmatch(branchConfig.MergeRef); m != nil { - prNumber, err := strconv.Atoi(m[1]) - if err != nil { - return 0, "", err - } - return prNumber, prHeadRef, nil - } - var branchOwner string - if branchConfig.PushRemoteURL != nil { - // the branch merges from a remote specified by URL - r, err := ghrepo.FromURL(branchConfig.PushRemoteURL) - if err != nil { - // TODO: We aren't returning the error because we discovered that it was shadowed - // before refactoring to its current return pattern. Thus, we aren't confident - // that returning the error won't break existing behavior. - return 0, prHeadRef, nil - } - branchOwner = r.RepoOwner() - } else if branchConfig.PushRemoteName != "" { - // the branch merges from a remote specified by name - r, err := rem.FindByName(branchConfig.PushRemoteName) - if err != nil { - // TODO: We aren't returning the error because we discovered that it was shadowed - // before refactoring to its current return pattern. Thus, we aren't confident - // that returning the error won't break existing behavior. - return 0, prHeadRef, nil - } - branchOwner = r.RepoOwner() - } - - if branchOwner != "" { - selector := prHeadRef - if branchConfig.Push != "" { - // If we have a resolved push revision ref, we defer to that - selector = strings.TrimPrefix(branchConfig.Push, branchConfig.PushRemoteName+"/") - } else if (branchConfig.RemotePushDefault == "upstream" || branchConfig.RemotePushDefault == "tracking") && - strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { - selector = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") - } - // prepend `OWNER:` if this branch is pushed to a fork - if !strings.EqualFold(branchOwner, baseRepo.RepoOwner()) { - selector = fmt.Sprintf("%s:%s", branchOwner, selector) - } - return 0, selector, nil - } - - return 0, prHeadRef, nil -} - func totalApprovals(pr *api.PullRequest) int { approvals := 0 for _, review := range pr.LatestReviews.Nodes { diff --git a/pkg/cmd/pr/status/status_test.go b/pkg/cmd/pr/status/status_test.go index 4e9ca19b0..c55604c28 100644 --- a/pkg/cmd/pr/status/status_test.go +++ b/pkg/cmd/pr/status/status_test.go @@ -4,7 +4,6 @@ import ( "bytes" "io" "net/http" - "net/url" "regexp" "strings" "testing" @@ -96,12 +95,13 @@ func TestPRStatus(t *testing.T) { defer http.Verify(t) http.Register(httpmock.GraphQL(`query PullRequestStatus\b`), httpmock.FileResponse("./fixtures/prStatus.json")) - // stub successful git command + // stub successful git commands rs, cleanup := run.Stub() defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") rs.Register(`git config remote.pushDefault`, 0, "") - rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git rev-parse --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git config push.default`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -133,7 +133,8 @@ func TestPRStatus_reviewsAndChecks(t *testing.T) { defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") rs.Register(`git config remote.pushDefault`, 0, "") - rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git rev-parse --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git config push.default`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -165,7 +166,8 @@ func TestPRStatus_reviewsAndChecksWithStatesByCount(t *testing.T) { defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") rs.Register(`git config remote.pushDefault`, 0, "") - rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git rev-parse --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git config push.default`, 0, "") output, err := runCommandWithDetector(http, "blueberries", true, "", &fd.EnabledDetectorMock{}) if err != nil { @@ -196,7 +198,8 @@ func TestPRStatus_currentBranch_showTheMostRecentPR(t *testing.T) { defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") rs.Register(`git config remote.pushDefault`, 0, "") - rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git rev-parse --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git config push.default`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -231,7 +234,8 @@ func TestPRStatus_currentBranch_defaultBranch(t *testing.T) { defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") rs.Register(`git config remote.pushDefault`, 0, "") - rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git rev-parse --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git config push.default`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -272,7 +276,8 @@ func TestPRStatus_currentBranch_Closed(t *testing.T) { defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") rs.Register(`git config remote.pushDefault`, 0, "") - rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git rev-parse --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git config push.default`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -296,7 +301,8 @@ func TestPRStatus_currentBranch_Closed_defaultBranch(t *testing.T) { defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") rs.Register(`git config remote.pushDefault`, 0, "") - rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git rev-parse --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git config push.default`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -320,7 +326,8 @@ func TestPRStatus_currentBranch_Merged(t *testing.T) { defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") rs.Register(`git config remote.pushDefault`, 0, "") - rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git rev-parse --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git config push.default`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -344,7 +351,8 @@ func TestPRStatus_currentBranch_Merged_defaultBranch(t *testing.T) { defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") rs.Register(`git config remote.pushDefault`, 0, "") - rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git rev-parse --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git config push.default`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -368,7 +376,8 @@ func TestPRStatus_blankSlate(t *testing.T) { defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") rs.Register(`git config remote.pushDefault`, 0, "") - rs.Register(`git rev-parse --verify --quiet --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git rev-parse --abbrev-ref blueberries@{push}`, 0, "") + rs.Register(`git config push.default`, 0, "") output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -428,7 +437,8 @@ func TestPRStatus_detachedHead(t *testing.T) { defer cleanup(t) rs.Register(`git config --get-regexp \^branch\\.`, 0, "") rs.Register(`git config remote.pushDefault`, 0, "") - rs.Register(`git rev-parse --verify --quiet --abbrev-ref @{push}`, 0, "") + rs.Register(`git rev-parse --abbrev-ref @{push}`, 0, "") + rs.Register(`git config push.default`, 0, "") output, err := runCommand(http, "", true, "") if err != nil { @@ -461,259 +471,3 @@ func TestPRStatus_error_ReadBranchConfig(t *testing.T) { _, err := runCommand(initFakeHTTP(), "blueberries", true, "") assert.Error(t, err) } - -func Test_prSelectorForCurrentBranch(t *testing.T) { - tests := []struct { - name string - branchConfig git.BranchConfig - baseRepo ghrepo.Interface - prHeadRef string - remotes context.Remotes - wantPrNumber int - wantSelector string - wantError error - }{ - { - name: "Empty branch config", - branchConfig: git.BranchConfig{}, - prHeadRef: "monalisa/main", - wantPrNumber: 0, - wantSelector: "monalisa/main", - wantError: nil, - }, - { - name: "The branch is configured to merge a special PR head ref", - branchConfig: git.BranchConfig{ - MergeRef: "refs/pull/42/head", - }, - prHeadRef: "monalisa/main", - wantPrNumber: 42, - wantSelector: "monalisa/main", - wantError: nil, - }, - { - name: "Branch merges from a remote specified by URL", - branchConfig: git.BranchConfig{ - PushRemoteURL: &url.URL{ - Scheme: "ssh", - User: url.User("git"), - Host: "github.com", - Path: "monalisa/playground.git", - }, - }, - baseRepo: ghrepo.NewWithHost("monalisa", "playground", "github.com"), - prHeadRef: "monalisa/main", - remotes: context.Remotes{ - &context.Remote{ - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.NewWithHost("monalisa", "playground", "github.com"), - }, - }, - wantPrNumber: 0, - wantSelector: "monalisa/main", - wantError: nil, - }, - { - name: "Branch merges from a remote specified by name", - branchConfig: git.BranchConfig{ - RemoteName: "upstream", - }, - baseRepo: ghrepo.NewWithHost("monalisa", "playground", "github.com"), - prHeadRef: "monalisa/main", - remotes: context.Remotes{ - &context.Remote{ - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.NewWithHost("forkName", "playground", "github.com"), - }, - &context.Remote{ - Remote: &git.Remote{Name: "upstream"}, - Repo: ghrepo.NewWithHost("monalisa", "playground", "github.com"), - }, - }, - wantPrNumber: 0, - wantSelector: "monalisa/main", - wantError: nil, - }, - { - name: "Branch is a fork and merges from a remote specified by URL", - branchConfig: git.BranchConfig{ - PushRemoteURL: &url.URL{ - Scheme: "ssh", - User: url.User("git"), - Host: "github.com", - Path: "forkName/playground.git", - }, - MergeRef: "refs/heads/main", - }, - baseRepo: ghrepo.NewWithHost("monalisa", "playground", "github.com"), - prHeadRef: "monalisa/main", - remotes: context.Remotes{ - &context.Remote{ - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.NewWithHost("forkName", "playground", "github.com"), - }, - }, - wantPrNumber: 0, - wantSelector: "forkName:main", - wantError: nil, - }, - { - name: "Branch is a fork and merges from a remote specified by name", - branchConfig: git.BranchConfig{ - RemoteName: "origin", - }, - baseRepo: ghrepo.NewWithHost("monalisa", "playground", "github.com"), - prHeadRef: "monalisa/main", - remotes: context.Remotes{ - &context.Remote{ - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.NewWithHost("forkName", "playground", "github.com"), - }, - &context.Remote{ - Remote: &git.Remote{Name: "upstream"}, - Repo: ghrepo.NewWithHost("monalisa", "playground", "github.com"), - }, - }, - wantPrNumber: 0, - wantSelector: "forkName:monalisa/main", - wantError: nil, - }, - { - name: "Branch specifies a mergeRef and merges from a remote specified by name", - branchConfig: git.BranchConfig{ - RemoteName: "upstream", - MergeRef: "refs/heads/main", - }, - baseRepo: ghrepo.NewWithHost("monalisa", "playground", "github.com"), - prHeadRef: "monalisa/main", - remotes: context.Remotes{ - &context.Remote{ - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.NewWithHost("forkName", "playground", "github.com"), - }, - &context.Remote{ - Remote: &git.Remote{Name: "upstream"}, - Repo: ghrepo.NewWithHost("monalisa", "playground", "github.com"), - }, - }, - wantPrNumber: 0, - wantSelector: "main", - wantError: nil, - }, - { - name: "Branch is a fork, specifies a mergeRef, and merges from a remote specified by name", - branchConfig: git.BranchConfig{ - RemoteName: "origin", - MergeRef: "refs/heads/main", - }, - baseRepo: ghrepo.NewWithHost("monalisa", "playground", "github.com"), - prHeadRef: "monalisa/main", - remotes: context.Remotes{ - &context.Remote{ - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.NewWithHost("forkName", "playground", "github.com"), - }, - &context.Remote{ - Remote: &git.Remote{Name: "upstream"}, - Repo: ghrepo.NewWithHost("monalisa", "playground", "github.com"), - }, - }, - wantPrNumber: 0, - wantSelector: "forkName:main", - wantError: nil, - }, - { - name: "Remote URL errors", - branchConfig: git.BranchConfig{ - PushRemoteURL: &url.URL{ - Scheme: "ssh", - User: url.User("git"), - Host: "github.com", - Path: "/\\invalid?Path/", - }, - }, - prHeadRef: "monalisa/main", - wantPrNumber: 0, - wantSelector: "monalisa/main", - wantError: nil, - }, - { - name: "Remote Name errors", - branchConfig: git.BranchConfig{ - RemoteName: "nonexistentRemote", - }, - prHeadRef: "monalisa/main", - remotes: context.Remotes{ - &context.Remote{ - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.NewWithHost("forkName", "playground", "github.com"), - }, - &context.Remote{ - Remote: &git.Remote{Name: "upstream"}, - Repo: ghrepo.NewWithHost("monalisa", "playground", "github.com"), - }, - }, - wantPrNumber: 0, - wantSelector: "monalisa/main", - wantError: nil, - }, - { - name: "Current branch pushes to default upstream", - branchConfig: git.BranchConfig{ - PushRemoteURL: &url.URL{ - Scheme: "ssh", - User: url.User("git"), - Host: "github.com", - Path: "Frederick888/playground.git", - }, - MergeRef: "refs/heads/main", - RemotePushDefault: "upstream", - }, - baseRepo: ghrepo.NewWithHost("octocat", "playground", "github.com"), - prHeadRef: "Frederick888/main", - remotes: context.Remotes{ - &context.Remote{ - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.NewWithHost("octocat", "playground", "github.com"), - }, - }, - wantPrNumber: 0, - wantSelector: "Frederick888:main", - wantError: nil, - }, - { - name: "Current branch pushes to default tracking", - branchConfig: git.BranchConfig{ - PushRemoteURL: &url.URL{ - Scheme: "ssh", - User: url.User("git"), - Host: "github.com", - Path: "Frederick888/playground.git", - }, - MergeRef: "refs/heads/main", - RemotePushDefault: "tracking", - }, - baseRepo: ghrepo.NewWithHost("octocat", "playground", "github.com"), - prHeadRef: "Frederick888/main", - remotes: context.Remotes{ - &context.Remote{ - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.NewWithHost("octocat", "playground", "github.com"), - }, - }, - wantPrNumber: 0, - wantSelector: "Frederick888:main", - wantError: nil, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - - prNum, headRef, err := prSelectorForCurrentBranch(tt.branchConfig, tt.baseRepo, tt.prHeadRef, tt.remotes) - assert.Equal(t, tt.wantPrNumber, prNum) - assert.Equal(t, tt.wantSelector, headRef) - assert.Equal(t, tt.wantError, err) - }) - } -} From 4382bdf072b558790358abda47b8cf3d802cd895 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 24 Jan 2025 14:01:44 -0800 Subject: [PATCH 20/23] Fix pr create tests Due to the refactor of BranchConfig, the tests for `pr create` no longer require a handful of stubbed git commands. Additionally, I noticed some overlap between `pr create`'s existing logic with the new finder code. I suspect that the finder's new ParsePRRefs helper could be leveraged here as well, which would allow us to respect non-centralized workflows pr create in the same way we are respecting them in `gh pr view` and `gh pr status` --- pkg/cmd/pr/create/create.go | 5 +- pkg/cmd/pr/create/create_test.go | 87 -------------------------------- pkg/cmd/pr/shared/finder.go | 6 +++ 3 files changed, 10 insertions(+), 88 deletions(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 124bd4b07..6c9d2a2af 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -518,6 +518,7 @@ func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState, u return nil } +// TODO: Replace with the finder's PRRefs struct // trackingRef represents a ref for a remote tracking branch. type trackingRef struct { remoteName string @@ -685,7 +686,9 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) { return nil, err } if isPushEnabled { - // determine whether the head branch is already pushed to a remote + // TODO: This doesn't respect the @{push} revision resolution or triagular workflows assembled with + // remote.pushDefault, or branch..pushremote config settings. The finder's ParsePRRefs + // may be able to replace this function entirely. if trackingRef, found := tryDetermineTrackingRef(gitClient, remotes, headBranch, headBranchConfig); found { isPushEnabled = false if r, err := remotes.FindByName(trackingRef.remoteName); err == nil { diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 6629b2fa1..c486997ad 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -353,8 +353,6 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") }, expectedBrowse: "https://github.com/OWNER/REPO/compare/master...feature?body=&expand=1", @@ -384,10 +382,6 @@ func Test_createRun(t *testing.T) { opts.HeadBranch = "feature" return func() {} }, - cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") - }, expectedOut: "https://github.com/OWNER/REPO/pull/12\n", }, { @@ -402,10 +396,6 @@ func Test_createRun(t *testing.T) { opts.DryRun = true return func() {} }, - cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") - }, expectedOutputs: []string{ "Would have created a Pull Request with:", `title: my title`, @@ -437,10 +427,6 @@ func Test_createRun(t *testing.T) { opts.DryRun = true return func() {} }, - cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") - }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { reg.Register( httpmock.GraphQL(`query RepositoryResolveMetadataIDs\b`), @@ -501,10 +487,6 @@ func Test_createRun(t *testing.T) { opts.DryRun = true return func() {} }, - cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") - }, expectedOutputs: []string{ `Would have created a Pull Request with:`, `Title: my title`, @@ -542,10 +524,6 @@ func Test_createRun(t *testing.T) { opts.DryRun = true return func() {} }, - cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") - }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { reg.Register( httpmock.GraphQL(`query RepositoryResolveMetadataIDs\b`), @@ -612,10 +590,6 @@ func Test_createRun(t *testing.T) { opts.DryRun = true return func() {} }, - cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") - }, expectedOut: heredoc.Doc(` Would have created a Pull Request with: Title: TITLE @@ -662,8 +636,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, @@ -727,8 +699,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, @@ -775,8 +745,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, @@ -826,8 +794,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register("git remote rename origin upstream", 0, "") cs.Register(`git remote add origin https://github.com/monalisa/REPO.git`, 0, "") @@ -886,8 +852,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register("git show-ref --verify", 0, heredoc.Doc(` deadbeef HEAD deadb00f refs/remotes/upstream/feature @@ -925,8 +889,6 @@ func Test_createRun(t *testing.T) { branch.feature.remote origin branch.feature.merge refs/heads/my-feat2 `)) // determineTrackingBranch - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 0, "origin/my-feat2") cs.Register("git show-ref --verify", 0, heredoc.Doc(` deadbeef HEAD deadbeef refs/remotes/origin/my-feat2 @@ -967,8 +929,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "d3476a1\u0000commit 0\u0000\u0000\n7a6ea13\u0000commit 1\u0000\u0000") }, promptStubs: func(pm *prompter.PrompterMock) { @@ -1009,10 +969,6 @@ func Test_createRun(t *testing.T) { opts.Milestone = "big one.oh" return func() {} }, - cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") - }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { reg.Register( httpmock.GraphQL(`query RepositoryResolveMetadataIDs\b`), @@ -1100,10 +1056,6 @@ func Test_createRun(t *testing.T) { opts.Finder = shared.NewMockFinder("feature", &api.PullRequest{URL: "https://github.com/OWNER/REPO/pull/123"}, ghrepo.New("OWNER", "REPO")) return func() {} }, - cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") - }, wantErr: "a pull request for branch \"feature\" into branch \"master\" already exists:\nhttps://github.com/OWNER/REPO/pull/123", }, { @@ -1120,8 +1072,6 @@ func Test_createRun(t *testing.T) { httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") @@ -1154,8 +1104,6 @@ func Test_createRun(t *testing.T) { mockRetrieveProjects(t, reg) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") @@ -1203,8 +1151,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature`, 0, "") cs.Register(`git rev-parse --show-toplevel`, 0, "") }, @@ -1264,8 +1210,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") }, promptStubs: func(pm *prompter.PrompterMock) { @@ -1314,8 +1258,6 @@ func Test_createRun(t *testing.T) { { name: "web long URL", cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git( .+)? log( .+)? origin/master\.\.\.feature`, 0, "") }, setup: func(opts *CreateOptions, t *testing.T) func() { @@ -1342,10 +1284,6 @@ func Test_createRun(t *testing.T) { } return func() {} }, - cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") - }, httpStubs: func(reg *httpmock.Registry, t *testing.T) { reg.Register( httpmock.GraphQL(`mutation PullRequestCreate\b`), @@ -1365,8 +1303,6 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register( "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", 0, @@ -1442,8 +1378,6 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register( "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", 0, @@ -1480,8 +1414,6 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register( "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", 0, @@ -1532,8 +1464,6 @@ func Test_createRun(t *testing.T) { return func() {} }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register("git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry origin/master...feature", 0, "") }, expectedOut: "https://github.com/OWNER/REPO/pull/12\n", @@ -1593,8 +1523,6 @@ func Test_createRun(t *testing.T) { deadbeef HEAD deadb00f refs/remotes/upstream/feature/feat2 deadbeef refs/remotes/origin/task1`)) // determineTrackingBranch - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref task1@\{push\}`, 1, "") }, expectedOut: "https://github.com/OWNER/REPO/pull/12\n", expectedErrOut: "\nCreating pull request for monalisa:task1 into feature/feat2 in OWNER/REPO\n\n", @@ -1705,9 +1633,6 @@ func Test_tryDetermineTrackingRef(t *testing.T) { { name: "empty", cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register(`git show-ref --verify -- HEAD`, 0, "abc HEAD") }, headBranchConfig: git.BranchConfig{}, @@ -1717,9 +1642,6 @@ func Test_tryDetermineTrackingRef(t *testing.T) { { name: "no match", cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 1, "") cs.Register("git show-ref --verify -- HEAD refs/remotes/upstream/feature refs/remotes/origin/feature", 0, "abc HEAD\nbca refs/remotes/upstream/feature") }, headBranchConfig: git.BranchConfig{}, @@ -1739,9 +1661,6 @@ func Test_tryDetermineTrackingRef(t *testing.T) { { name: "match", cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 0, "origin/feature") cs.Register(`git show-ref --verify -- HEAD refs/remotes/upstream/feature refs/remotes/origin/feature$`, 0, heredoc.Doc(` deadbeef HEAD deadb00f refs/remotes/upstream/feature @@ -1768,12 +1687,6 @@ func Test_tryDetermineTrackingRef(t *testing.T) { { name: "respect tracking config", cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, heredoc.Doc(` - branch.feature.remote origin - branch.feature.merge refs/heads/great-feat - `)) - cs.Register(`git config remote.pushDefault`, 1, "") - cs.Register(`git rev-parse --verify --quiet --abbrev-ref feature@\{push\}`, 0, "origin/great-feat") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/great-feat refs/remotes/origin/feature$`, 0, heredoc.Doc(` deadbeef HEAD deadb00f refs/remotes/origin/feature diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index ae0ea4553..205939931 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -97,6 +97,12 @@ type FindOptions struct { States []string } +// TODO: Does this also need the BaseBranchName? +// PR's are represented by the following: +// baseRef -----PR-----> headRef +// +// A ref is described as "remoteName/branchName", so +// baseRepoName/baseBranchName -----PR-----> headRepoName/headBranchName type PRRefs struct { BranchName string HeadRepo ghrepo.Interface From 62106dc800ada6ed8d4c8deab6270c3c0a008d44 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 24 Jan 2025 14:18:20 -0800 Subject: [PATCH 21/23] Cleanup comment --- git/objects.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/git/objects.go b/git/objects.go index d66d5b9bb..b57d79bec 100644 --- a/git/objects.go +++ b/git/objects.go @@ -66,8 +66,7 @@ type BranchConfig struct { // MergeBase is the optional base branch to target in a new PR if `--base` is not specified. MergeBase string MergeRef string - // These are used to handle triangular workflows. They can be defined by either - // a remote.pushDefault or a branch..pushremote value set on the git config. + // These are usually used to handle triangular workflows. PushRemoteURL *url.URL PushRemoteName string } From e31bfd0092e5482247c6a6a90ca1d8c4c6c7a966 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Mon, 27 Jan 2025 16:40:47 -0800 Subject: [PATCH 22/23] Cleaned up some naming and comments --- git/client.go | 20 +++++++------------- git/client_test.go | 20 +++++++++++++------- git/objects.go | 13 +++++++------ pkg/cmd/pr/shared/finder.go | 6 +++--- pkg/cmd/pr/shared/finder_test.go | 10 +++++++--- 5 files changed, 37 insertions(+), 32 deletions(-) diff --git a/git/client.go b/git/client.go index be747e962..30cb4306d 100644 --- a/git/client.go +++ b/git/client.go @@ -389,13 +389,11 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (BranchCon return BranchConfig{}, err } - // This is the error we expect if a git command does not run successfully. - // If the ExitCode is 1, then we just didn't find any config for the branch. - // We will use this error to check against the commands that are allowed to - // return an empty result. - var gitError *GitError branchCfgOut, err := cmd.Output() if err != nil { + // This is the error we expect if the git command does not run successfully. + // If the ExitCode is 1, then we just didn't find any config for the branch. + var gitError *GitError if ok := errors.As(err, &gitError); ok && gitError.ExitCode != 1 { return BranchConfig{}, err } @@ -417,14 +415,9 @@ func parseBranchConfig(branchConfigLines []string) BranchConfig { keys := strings.Split(parts[0], ".") switch keys[len(keys)-1] { case "remote": - remoteURL, remoteName := parseRemoteURLOrName(parts[1]) - cfg.RemoteURL = remoteURL - cfg.RemoteName = remoteName - // pushremote usually indicates a "triangular" workflow + cfg.RemoteURL, cfg.RemoteName = parseRemoteURLOrName(parts[1]) case "pushremote": - pushRemoteURL, pushRemoteName := parseRemoteURLOrName(parts[1]) - cfg.PushRemoteURL = pushRemoteURL - cfg.PushRemoteName = pushRemoteName + cfg.PushRemoteURL, cfg.PushRemoteName = parseRemoteURLOrName(parts[1]) case "merge": cfg.MergeRef = parts[1] case MergeBaseConfig: @@ -449,7 +442,8 @@ func (c *Client) SetBranchConfig(ctx context.Context, branch, name, value string } // PushDefault returns the value of push.default in the config. If the value -// is not set, it returns "simple" (the default value). +// is not set, it returns "simple" (the default git value). See +// https://git-scm.com/docs/git-config#Documentation/git-config.txt-pushdefault func (c *Client) PushDefault(ctx context.Context) (string, error) { pushDefault, err := c.Config(ctx, "push.default") if err == nil { diff --git a/git/client_test.go b/git/client_test.go index 99f783881..872c8595b 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -14,6 +14,7 @@ import ( "strings" "testing" + "github.com/MakeNowJust/heredoc" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -763,7 +764,12 @@ func TestClientReadBranchConfig(t *testing.T) { name: "when the config is read, it should return the correct BranchConfig", cmds: mockedCommands{ `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|pushremote|gh-merge-base)$`: { - Stdout: "branch.trunk.remote upstream\nbranch.trunk.merge refs/heads/trunk\nbranch.trunk.pushremote origin\nbranch.trunk.gh-merge-base gh-merge-base\n", + Stdout: heredoc.Doc(` + branch.trunk.remote upstream + branch.trunk.merge refs/heads/trunk + branch.trunk.pushremote origin + branch.trunk.gh-merge-base gh-merge-base + `), }, }, branch: "trunk", @@ -1869,16 +1875,16 @@ func TestCommandMocking(t *testing.T) { jsonVar, ok := os.LookupEnv("GH_HELPER_PROCESS_RICH_COMMANDS") if !ok { fmt.Fprint(os.Stderr, "missing GH_HELPER_PROCESS_RICH_COMMANDS") - // Exit 1 is reserved for empty command responses by git. This can be desirable in some cases, - // so returning an arbitrary exit code to avoid suppressing this if an exit code 1 is allowed. + // Exit 1 is used for empty key values in the git config. This is non-breaking in those use cases, + // so this is returning a non-zero exit code to avoid suppressing this error for those use cases. os.Exit(16) } var commands mockedCommands if err := json.Unmarshal([]byte(jsonVar), &commands); err != nil { fmt.Fprint(os.Stderr, "failed to unmarshal GH_HELPER_PROCESS_RICH_COMMANDS") - // Exit 1 is reserved for empty command responses by git. This can be desirable in some cases, - // so returning an arbitrary exit code to avoid suppressing this if an exit code 1 is allowed. + // Exit 1 is used for empty key values in the git config. This is non-breaking in those use cases, + // so this is returning a non-zero exit code to avoid suppressing this error for those use cases. os.Exit(16) } @@ -1888,8 +1894,8 @@ func TestCommandMocking(t *testing.T) { commandResult, ok := commands[args(strings.Join(realArgs, " "))] if !ok { fmt.Fprintf(os.Stderr, "unexpected command: %s\n", strings.Join(realArgs, " ")) - // Exit 1 is reserved for empty command responses by git. This can be desirable in some cases, - // so returning an arbitrary exit code to avoid suppressing this if an exit code 1 is allowed. + // Exit 1 is used for empty key values in the git config. This is non-breaking in those use cases, + // so this is returning a non-zero exit code to avoid suppressing this error for those use cases. os.Exit(16) } diff --git a/git/objects.go b/git/objects.go index b57d79bec..9db528b8c 100644 --- a/git/objects.go +++ b/git/objects.go @@ -60,13 +60,14 @@ type Commit struct { Body string } +// These are the keys we read from the git branch. config. type BranchConfig struct { - RemoteName string - RemoteURL *url.URL + RemoteName string // .remote if string + RemoteURL *url.URL // .remote if url + MergeRef string // .merge + PushRemoteName string // .pushremote if string + PushRemoteURL *url.URL // .pushremote if url + // MergeBase is the optional base branch to target in a new PR if `--base` is not specified. MergeBase string - MergeRef string - // These are usually used to handle triangular workflows. - PushRemoteURL *url.URL - PushRemoteName string } diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index 205939931..05ec24c9a 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -109,10 +109,10 @@ type PRRefs struct { BaseRepo ghrepo.Interface } -// GetPRLabel returns the string that the GitHub API uses to identify the PR. This is +// GetPRHeadLabel returns the string that the GitHub API uses to identify the PR. This is // either just the branch name or, if the PR is originating from a fork, the fork owner // and the branch name, like :. -func (s *PRRefs) GetPRLabel() string { +func (s *PRRefs) GetPRHeadLabel() string { if ghrepo.IsSame(s.HeadRepo, s.BaseRepo) { return s.BranchName } @@ -240,7 +240,7 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err return nil, nil, err } - pr, err = findForBranch(httpClient, f.baseRefRepo, opts.BaseBranch, prRefs.GetPRLabel(), opts.States, fields.ToSlice()) + pr, err = findForBranch(httpClient, f.baseRefRepo, opts.BaseBranch, prRefs.GetPRHeadLabel(), opts.States, fields.ToSlice()) if err != nil { return pr, f.baseRefRepo, err } diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index 1d5c1e3ac..a54412db5 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -419,7 +419,11 @@ func TestFind(t *testing.T) { wantRepo: "https://github.com/ORIGINOWNER/REPO", }, { - name: "the current branch is configured to push to and pull from a URL (upstream, in this example) that is different from what the repo is configured to push to and pull from (origin, in this example) and push.default = upstream, it finds the PR associated with the upstream repo and returns origin as the base repo", + // The current BRANCH is configured to push to and pull from a URL (upstream, in this example) + // which is different from what the REPO is configured to push to and pull from (origin, in this example) + // and push.default = upstream. It should find the PR associated with the upstream repo and return + // origin as the base repo + name: "when push.default = upstream and the current branch is configured to push/pull from a different remote than the repo", args: args{ selector: "", fields: []string{"id", "number"}, @@ -902,7 +906,7 @@ func TestParsePRRefs(t *testing.T) { } } -func TestPRRefs_GetPRLabel(t *testing.T) { +func TestPRRefs_GetPRHeadLabel(t *testing.T) { originRepo := ghrepo.New("ORIGINOWNER", "REPO") upstreamRepo := ghrepo.New("UPSTREAMOWNER", "REPO") tests := []struct { @@ -931,7 +935,7 @@ func TestPRRefs_GetPRLabel(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, tt.prRefs.GetPRLabel()) + assert.Equal(t, tt.want, tt.prRefs.GetPRHeadLabel()) }) } } From e0f624ba240546f980f1190a93dd0264a84fa2c8 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Wed, 29 Jan 2025 11:41:23 -0800 Subject: [PATCH 23/23] Rename PRRefs to PullRequestRefs and PR comment cleanup --- git/client.go | 2 +- git/client_test.go | 2 +- pkg/cmd/pr/create/create.go | 2 +- pkg/cmd/pr/shared/finder.go | 12 ++++---- pkg/cmd/pr/shared/finder_test.go | 50 ++++++++++++++++---------------- pkg/cmd/pr/status/status.go | 2 +- 6 files changed, 35 insertions(+), 35 deletions(-) diff --git a/git/client.go b/git/client.go index 30cb4306d..11a2e2e20 100644 --- a/git/client.go +++ b/git/client.go @@ -474,7 +474,7 @@ func (c *Client) RemotePushDefault(ctx context.Context) (string, error) { } // ParsePushRevision gets the value of the @{push} revision syntax -// An error here doesn't necessarily mean something are broke, but may mean that the @{push} +// An error here doesn't necessarily mean something is broken, but may mean that the @{push} // revision syntax couldn't be resolved, such as in non-centralized workflows with // push.default = simple. Downstream consumers should consider how to handle this error. func (c *Client) ParsePushRevision(ctx context.Context, branch string) (string, error) { diff --git a/git/client_test.go b/git/client_test.go index 872c8595b..9fa076199 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -769,7 +769,7 @@ func TestClientReadBranchConfig(t *testing.T) { branch.trunk.merge refs/heads/trunk branch.trunk.pushremote origin branch.trunk.gh-merge-base gh-merge-base - `), + `), }, }, branch: "trunk", diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 6c9d2a2af..b2abe0938 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -518,7 +518,7 @@ func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState, u return nil } -// TODO: Replace with the finder's PRRefs struct +// TODO: Replace with the finder's PullRequestRefs struct // trackingRef represents a ref for a remote tracking branch. type trackingRef struct { remoteName string diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index 05ec24c9a..e4f89502c 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -103,7 +103,7 @@ type FindOptions struct { // // A ref is described as "remoteName/branchName", so // baseRepoName/baseBranchName -----PR-----> headRepoName/headBranchName -type PRRefs struct { +type PullRequestRefs struct { BranchName string HeadRepo ghrepo.Interface BaseRepo ghrepo.Interface @@ -112,7 +112,7 @@ type PRRefs struct { // GetPRHeadLabel returns the string that the GitHub API uses to identify the PR. This is // either just the branch name or, if the PR is originating from a fork, the fork owner // and the branch name, like :. -func (s *PRRefs) GetPRHeadLabel() string { +func (s *PullRequestRefs) GetPRHeadLabel() string { if ghrepo.IsSame(s.HeadRepo, s.BaseRepo) { return s.BranchName } @@ -227,7 +227,7 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err return nil, nil, err } - // Suppressing these errors as we have other means of computing the PRRefs when these fail. + // Suppressing these errors as we have other means of computing the PullRequestRefs when these fail. parsedPushRevision, _ := f.parsePushRevision(f.branchName) remotePushDefault, err := f.remotePushDefault() @@ -302,8 +302,8 @@ func (f *finder) parseURL(prURL string) (ghrepo.Interface, int, error) { return repo, prNumber, nil } -func ParsePRRefs(currentBranchName string, branchConfig git.BranchConfig, parsedPushRevision string, pushDefault string, remotePushDefault string, baseRefRepo ghrepo.Interface, rems remotes.Remotes) (PRRefs, error) { - prRefs := PRRefs{ +func ParsePRRefs(currentBranchName string, branchConfig git.BranchConfig, parsedPushRevision string, pushDefault string, remotePushDefault string, baseRefRepo ghrepo.Interface, rems remotes.Remotes) (PullRequestRefs, error) { + prRefs := PullRequestRefs{ BaseRepo: baseRefRepo, } @@ -323,7 +323,7 @@ func ParsePRRefs(currentBranchName string, branchConfig git.BranchConfig, parsed for i, r := range rems { remoteNames[i] = r.Name } - return PRRefs{}, fmt.Errorf("no remote for %q found in %q", parsedPushRevision, strings.Join(remoteNames, ", ")) + return PullRequestRefs{}, fmt.Errorf("no remote for %q found in %q", parsedPushRevision, strings.Join(remoteNames, ", ")) } // We assume the PR's branch name is the same as whatever f.BranchFn() returned earlier diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index a54412db5..694e0c20d 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -676,15 +676,15 @@ func TestParsePRRefs(t *testing.T) { currentBranchName string baseRefRepo ghrepo.Interface rems context.Remotes - wantPRRefs PRRefs + wantPRRefs PullRequestRefs wantErr error }{ { - name: "When the branch is called 'blueberries' with an empty branch config, it returns the correct PRRefs", + name: "When the branch is called 'blueberries' with an empty branch config, it returns the correct PullRequestRefs", branchConfig: git.BranchConfig{}, currentBranchName: "blueberries", baseRefRepo: remoteOrigin.Repo, - wantPRRefs: PRRefs{ + wantPRRefs: PullRequestRefs{ BranchName: "blueberries", HeadRepo: remoteOrigin.Repo, BaseRepo: remoteOrigin.Repo, @@ -692,11 +692,11 @@ func TestParsePRRefs(t *testing.T) { wantErr: nil, }, { - name: "When the branch is called 'otherBranch' with an empty branch config, it returns the correct PRRefs", + name: "When the branch is called 'otherBranch' with an empty branch config, it returns the correct PullRequestRefs", branchConfig: git.BranchConfig{}, currentBranchName: "otherBranch", baseRefRepo: remoteOrigin.Repo, - wantPRRefs: PRRefs{ + wantPRRefs: PullRequestRefs{ BranchName: "otherBranch", HeadRepo: remoteOrigin.Repo, BaseRepo: remoteOrigin.Repo, @@ -711,7 +711,7 @@ func TestParsePRRefs(t *testing.T) { rems: context.Remotes{ &remoteOrigin, }, - wantPRRefs: PRRefs{ + wantPRRefs: PullRequestRefs{ BranchName: "pushBranch", HeadRepo: remoteOrigin.Repo, BaseRepo: remoteOrigin.Repo, @@ -727,7 +727,7 @@ func TestParsePRRefs(t *testing.T) { &remoteUpstream, &remoteOther, }, - wantPRRefs: PRRefs{}, + wantPRRefs: PullRequestRefs{}, wantErr: fmt.Errorf("no remote for %q found in %q", "origin/differentPushBranch", "upstream, other"), }, { @@ -738,7 +738,7 @@ func TestParsePRRefs(t *testing.T) { rems: context.Remotes{ &remoteOther, }, - wantPRRefs: PRRefs{ + wantPRRefs: PullRequestRefs{ BranchName: "pushBranch", HeadRepo: remoteOther.Repo, BaseRepo: remoteOrigin.Repo, @@ -746,7 +746,7 @@ func TestParsePRRefs(t *testing.T) { wantErr: nil, }, { - name: "When the push remote is the same as the baseRepo, it returns the baseRepo as the PRRefs HeadRepo", + name: "When the push remote is the same as the baseRepo, it returns the baseRepo as the PullRequestRefs HeadRepo", branchConfig: git.BranchConfig{ PushRemoteName: remoteOrigin.Remote.Name, }, @@ -756,7 +756,7 @@ func TestParsePRRefs(t *testing.T) { &remoteOrigin, &remoteUpstream, }, - wantPRRefs: PRRefs{ + wantPRRefs: PullRequestRefs{ BranchName: "blueberries", HeadRepo: remoteOrigin.Repo, BaseRepo: remoteOrigin.Repo, @@ -764,7 +764,7 @@ func TestParsePRRefs(t *testing.T) { wantErr: nil, }, { - name: "When the push remote is different from the baseRepo, it returns the push remote repo as the PRRefs HeadRepo", + name: "When the push remote is different from the baseRepo, it returns the push remote repo as the PullRequestRefs HeadRepo", branchConfig: git.BranchConfig{ PushRemoteName: remoteOrigin.Remote.Name, }, @@ -774,7 +774,7 @@ func TestParsePRRefs(t *testing.T) { &remoteOrigin, &remoteUpstream, }, - wantPRRefs: PRRefs{ + wantPRRefs: PullRequestRefs{ BranchName: "blueberries", HeadRepo: remoteOrigin.Repo, BaseRepo: remoteUpstream.Repo, @@ -782,7 +782,7 @@ func TestParsePRRefs(t *testing.T) { wantErr: nil, }, { - name: "When the push remote defined by a URL and the baseRepo is different from the push remote, it returns the push remote repo as the PRRefs HeadRepo", + name: "When the push remote defined by a URL and the baseRepo is different from the push remote, it returns the push remote repo as the PullRequestRefs HeadRepo", branchConfig: git.BranchConfig{ PushRemoteURL: remoteOrigin.Remote.FetchURL, }, @@ -792,7 +792,7 @@ func TestParsePRRefs(t *testing.T) { &remoteOrigin, &remoteUpstream, }, - wantPRRefs: PRRefs{ + wantPRRefs: PullRequestRefs{ BranchName: "blueberries", HeadRepo: remoteOrigin.Repo, BaseRepo: remoteUpstream.Repo, @@ -812,7 +812,7 @@ func TestParsePRRefs(t *testing.T) { &remoteOrigin, &remoteUpstream, }, - wantPRRefs: PRRefs{ + wantPRRefs: PullRequestRefs{ BranchName: "blue-upstream-berries", HeadRepo: remoteUpstream.Repo, BaseRepo: remoteOrigin.Repo, @@ -832,7 +832,7 @@ func TestParsePRRefs(t *testing.T) { &remoteOrigin, &remoteUpstream, }, - wantPRRefs: PRRefs{ + wantPRRefs: PullRequestRefs{ BranchName: "blue-upstream-berries", HeadRepo: remoteUpstream.Repo, BaseRepo: remoteOrigin.Repo, @@ -840,7 +840,7 @@ func TestParsePRRefs(t *testing.T) { wantErr: nil, }, { - name: "When remote.pushDefault is set, it returns the correct PRRefs", + name: "When remote.pushDefault is set, it returns the correct PullRequestRefs", branchConfig: git.BranchConfig{}, remotePushDefault: remoteUpstream.Remote.Name, currentBranchName: "blueberries", @@ -849,7 +849,7 @@ func TestParsePRRefs(t *testing.T) { &remoteOrigin, &remoteUpstream, }, - wantPRRefs: PRRefs{ + wantPRRefs: PullRequestRefs{ BranchName: "blueberries", HeadRepo: remoteUpstream.Repo, BaseRepo: remoteOrigin.Repo, @@ -857,7 +857,7 @@ func TestParsePRRefs(t *testing.T) { wantErr: nil, }, { - name: "When the remote name is set on the branch, it returns the correct PRRefs", + name: "When the remote name is set on the branch, it returns the correct PullRequestRefs", branchConfig: git.BranchConfig{ RemoteName: remoteUpstream.Remote.Name, }, @@ -867,7 +867,7 @@ func TestParsePRRefs(t *testing.T) { &remoteOrigin, &remoteUpstream, }, - wantPRRefs: PRRefs{ + wantPRRefs: PullRequestRefs{ BranchName: "blueberries", HeadRepo: remoteUpstream.Repo, BaseRepo: remoteOrigin.Repo, @@ -875,7 +875,7 @@ func TestParsePRRefs(t *testing.T) { wantErr: nil, }, { - name: "When the remote URL is set on the branch, it returns the correct PRRefs", + name: "When the remote URL is set on the branch, it returns the correct PullRequestRefs", branchConfig: git.BranchConfig{ RemoteURL: remoteUpstream.Remote.FetchURL, }, @@ -885,7 +885,7 @@ func TestParsePRRefs(t *testing.T) { &remoteOrigin, &remoteUpstream, }, - wantPRRefs: PRRefs{ + wantPRRefs: PullRequestRefs{ BranchName: "blueberries", HeadRepo: remoteUpstream.Repo, BaseRepo: remoteOrigin.Repo, @@ -911,12 +911,12 @@ func TestPRRefs_GetPRHeadLabel(t *testing.T) { upstreamRepo := ghrepo.New("UPSTREAMOWNER", "REPO") tests := []struct { name string - prRefs PRRefs + prRefs PullRequestRefs want string }{ { name: "When the HeadRepo and BaseRepo match, it returns the branch name", - prRefs: PRRefs{ + prRefs: PullRequestRefs{ BranchName: "blueberries", HeadRepo: originRepo, BaseRepo: originRepo, @@ -925,7 +925,7 @@ func TestPRRefs_GetPRHeadLabel(t *testing.T) { }, { name: "When the HeadRepo and BaseRepo do not match, it returns the prepended HeadRepo owner to the branch name", - prRefs: PRRefs{ + prRefs: PullRequestRefs{ BranchName: "blueberries", HeadRepo: originRepo, BaseRepo: upstreamRepo, diff --git a/pkg/cmd/pr/status/status.go b/pkg/cmd/pr/status/status.go index 996a178e1..3877fb1cf 100644 --- a/pkg/cmd/pr/status/status.go +++ b/pkg/cmd/pr/status/status.go @@ -107,7 +107,7 @@ func statusRun(opts *StatusOptions) error { if err != nil { return err } - // Suppressing these errors as we have other means of computing the PRRefs when these fail. + // Suppressing these errors as we have other means of computing the PullRequestRefs when these fail. parsedPushRevision, _ := opts.GitClient.ParsePushRevision(ctx, currentBranchName) remotePushDefault, err := opts.GitClient.RemotePushDefault(ctx)