From 3d139019f83782503b8184cf570680946cfa082f Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Sat, 5 Oct 2024 22:40:40 -0700 Subject: [PATCH] Open PR against gh-merge-base Partly resolves issue #8979 by checking for a `gh-merge-base` branch tag and using that as though it were passed to `gh pr create --base`. --- git/client.go | 6 +- git/client_test.go | 6 +- git/objects.go | 6 +- pkg/cmd/pr/create/create.go | 11 ++-- pkg/cmd/pr/create/create_test.go | 106 +++++++++++++++++++++++++------ 5 files changed, 107 insertions(+), 28 deletions(-) diff --git a/git/client.go b/git/client.go index 1dea7a6d6..908ee8c4f 100644 --- a/git/client.go +++ b/git/client.go @@ -376,7 +376,7 @@ func (c *Client) lookupCommit(ctx context.Context, sha, format string) ([]byte, // ReadBranchConfig parses the `branch.BRANCH.(remote|merge)` 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)$", prefix)} + args := []string{"config", "--get-regexp", fmt.Sprintf("^%s(remote|merge|gh-merge-base)$", prefix)} cmd, err := c.Command(ctx, args...) if err != nil { return @@ -385,6 +385,8 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg Branc if err != nil { return } + + cfg.LocalName = branch for _, line := range outputLines(out) { parts := strings.SplitN(line, " ", 2) if len(parts) < 2 { @@ -404,6 +406,8 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg Branc } case "merge": cfg.MergeRef = parts[1] + case "gh-merge-base": + cfg.MergeBase = parts[1] } } return diff --git a/git/client_test.go b/git/client_test.go index 0fb7953bc..0ec4f7334 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -735,9 +735,9 @@ func TestClientReadBranchConfig(t *testing.T) { }{ { name: "read branch config", - cmdStdout: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk", - wantCmdArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge)$`, - wantBranchConfig: BranchConfig{RemoteName: "origin", MergeRef: "refs/heads/trunk"}, + 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{LocalName: "trunk", RemoteName: "origin", MergeRef: "refs/heads/trunk", MergeBase: "trunk"}, }, } for _, tt := range tests { diff --git a/git/objects.go b/git/objects.go index c33d92b7c..f371429dd 100644 --- a/git/objects.go +++ b/git/objects.go @@ -71,7 +71,11 @@ type Commit struct { } type BranchConfig struct { + // LocalName of the branch. + LocalName string RemoteName string RemoteURL *url.URL - MergeRef string + // MergeBase is the optional base branch to target in a new PR if `--base` is not specified. + MergeBase string + MergeRef string } diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 06f4e1e89..2074a6f59 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -513,11 +513,10 @@ func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState, u return nil } -func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, headBranch string) *git.TrackingRef { +func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, headBranchConfig *git.BranchConfig) *git.TrackingRef { refsForLookup := []string{"HEAD"} var trackingRefs []git.TrackingRef - headBranchConfig := gitClient.ReadBranchConfig(context.Background(), headBranch) if headBranchConfig.RemoteName != "" { tr := git.TrackingRef{ RemoteName: headBranchConfig.RemoteName, @@ -530,7 +529,7 @@ func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, h for _, remote := range remotes { tr := git.TrackingRef{ RemoteName: remote.Name, - BranchName: headBranch, + BranchName: headBranchConfig.LocalName, } trackingRefs = append(trackingRefs, tr) refsForLookup = append(refsForLookup, tr.String()) @@ -640,9 +639,10 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) { var headRepo ghrepo.Interface var headRemote *ghContext.Remote + headBranchConfig := gitClient.ReadBranchConfig(context.Background(), headBranch) if isPushEnabled { // determine whether the head branch is already pushed to a remote - if pushedTo := determineTrackingBranch(gitClient, remotes, headBranch); pushedTo != nil { + if pushedTo := determineTrackingBranch(gitClient, remotes, &headBranchConfig); pushedTo != nil { isPushEnabled = false if r, err := remotes.FindByName(pushedTo.RemoteName); err == nil { headRepo = r @@ -715,6 +715,9 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) { } baseBranch := opts.BaseBranch + if baseBranch == "" { + baseBranch = headBranchConfig.MergeBase + } if baseBranch == "" { baseBranch = baseRepo.DefaultBranchRef.Name } diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index d31174999..27220d052 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1,6 +1,7 @@ package create import ( + ctx "context" "encoding/json" "errors" "fmt" @@ -261,6 +262,15 @@ func TestNewCmdCreate(t *testing.T) { cli: "--editor", wantsErr: true, }, + { + name: "fill and base", + cli: "--fill --base trunk", + wantsOpts: CreateOptions{ + Autofill: true, + BaseBranch: "trunk", + MaintainerCanModify: true, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -323,17 +333,18 @@ func TestNewCmdCreate(t *testing.T) { func Test_createRun(t *testing.T) { tests := []struct { - name string - setup func(*CreateOptions, *testing.T) func() - cmdStubs func(*run.CommandStubber) - promptStubs func(*prompter.PrompterMock) - httpStubs func(*httpmock.Registry, *testing.T) - expectedOutputs []string - expectedOut string - expectedErrOut string - expectedBrowse string - wantErr string - tty bool + name string + setup func(*CreateOptions, *testing.T) func() + cmdStubs func(*run.CommandStubber) + promptStubs func(*prompter.PrompterMock) + httpStubs func(*httpmock.Registry, *testing.T) + expectedOutputs []string + expectedOut string + expectedErrOut string + expectedBrowse string + wantErr string + tty bool + customBranchConfig bool }{ { name: "nontty web", @@ -626,7 +637,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, @@ -690,7 +700,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, @@ -737,7 +746,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature`, 0, "") cs.Register(`git push --set-upstream origin HEAD:refs/heads/feature`, 0, "") }, @@ -787,7 +795,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") 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, "") @@ -846,7 +853,6 @@ func Test_createRun(t *testing.T) { })) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp \^branch\\\.feature\\\.`, 1, "") // determineTrackingBranch cs.Register("git show-ref --verify", 0, heredoc.Doc(` deadbeef HEAD deadb00f refs/remotes/upstream/feature @@ -878,6 +884,7 @@ func Test_createRun(t *testing.T) { assert.Equal(t, "my-feat2", input["headRefName"].(string)) })) }, + customBranchConfig: true, cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp \^branch\\\.feature\\\.`, 0, heredoc.Doc(` branch.feature.remote origin @@ -1066,7 +1073,6 @@ func Test_createRun(t *testing.T) { httpmock.StringResponse(`{"data": {"viewer": {"login": "OWNER"} } }`)) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") 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, "") @@ -1099,7 +1105,6 @@ func Test_createRun(t *testing.T) { mockRetrieveProjects(t, reg) }, cmdStubs: func(cs *run.CommandStubber) { - cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") 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, "") @@ -1464,6 +1469,65 @@ func Test_createRun(t *testing.T) { }, expectedOut: "https://github.com/OWNER/REPO/pull/12\n", }, + { + name: "gh-merge-base", + tty: true, + setup: func(opts *CreateOptions, t *testing.T) func() { + opts.TitleProvided = true + opts.BodyProvided = true + opts.Title = "my title" + opts.Body = "my body" + opts.Branch = func() (string, error) { + return "task1", nil + } + opts.Remotes = func() (context.Remotes, error) { + return context.Remotes{ + { + Remote: &git.Remote{ + Name: "upstream", + Resolved: "base", + }, + Repo: ghrepo.New("OWNER", "REPO"), + }, + { + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("monalisa", "REPO"), + }, + }, nil + } + return func() {} + }, + httpStubs: func(reg *httpmock.Registry, t *testing.T) { + reg.Register( + httpmock.GraphQL(`mutation PullRequestCreate\b`), + httpmock.GraphQLMutation(` + { "data": { "createPullRequest": { "pullRequest": { + "URL": "https://github.com/OWNER/REPO/pull/12" + } } } } + `, func(input map[string]interface{}) { + assert.Equal(t, "REPOID", input["repositoryId"].(string)) + assert.Equal(t, "my title", input["title"].(string)) + assert.Equal(t, "my body", input["body"].(string)) + assert.Equal(t, "feature/feat2", input["baseRefName"].(string)) + assert.Equal(t, "monalisa:task1", input["headRefName"].(string)) + })) + }, + customBranchConfig: true, + cmdStubs: func(cs *run.CommandStubber) { + cs.Register(`git config --get-regexp \^branch\\\.task1\\\.\(remote\|merge\|gh-merge-base\)\$`, 0, heredoc.Doc(` + branch.task1.remote origin + branch.task1.merge refs/heads/task1 + branch.task1.gh-merge-base feature/feat2`)) // ReadBranchConfig + cs.Register(`git show-ref --verify`, 0, heredoc.Doc(` + deadbeef HEAD + deadb00f refs/remotes/upstream/feature/feat2 + deadbeef refs/remotes/origin/task1`)) // determineTrackingBranch + }, + expectedOut: "https://github.com/OWNER/REPO/pull/12\n", + expectedErrOut: "\nCreating pull request for monalisa:task1 into feature/feat2 in OWNER/REPO\n\n", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -1485,6 +1549,9 @@ func Test_createRun(t *testing.T) { cs, cmdTeardown := run.Stub() 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, "") + } if tt.cmdStubs != nil { tt.cmdStubs(cs) @@ -1651,7 +1718,8 @@ func Test_determineTrackingBranch(t *testing.T) { GhPath: "some/path/gh", GitPath: "some/path/git", } - ref := determineTrackingBranch(gitClient, tt.remotes, "feature") + headBranchConfig := gitClient.ReadBranchConfig(ctx.Background(), "feature") + ref := determineTrackingBranch(gitClient, tt.remotes, &headBranchConfig) tt.assert(ref, t) }) }