From 0179381efdc45980596aae7a2147ce13440ec483 Mon Sep 17 00:00:00 2001 From: Frederick Zhang Date: Fri, 14 Jun 2024 14:43:38 +1000 Subject: [PATCH] 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{