From 3d139019f83782503b8184cf570680946cfa082f Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Sat, 5 Oct 2024 22:40:40 -0700 Subject: [PATCH 1/5] 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) }) } From 88b96f411ca56b24e56942ba5792b363e1627f42 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Sat, 5 Oct 2024 23:20:49 -0700 Subject: [PATCH 2/5] Set gh-merge-base from `issue develop` --- git/client.go | 20 ++++++++++++++++++-- pkg/cmd/issue/develop/develop.go | 8 ++++++++ pkg/cmd/issue/develop/develop_test.go | 1 + 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/git/client.go b/git/client.go index 908ee8c4f..43568839a 100644 --- a/git/client.go +++ b/git/client.go @@ -20,6 +20,9 @@ import ( "github.com/cli/safeexec" ) +// MergeBaseConfig is the configuration setting to keep track of the PR target branch. +const MergeBaseConfig = "gh-merge-base" + var remoteRE = regexp.MustCompile(`(.+)\s+(.+)\s+\((push|fetch)\)`) // This regexp exists to match lines of the following form: @@ -376,7 +379,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|gh-merge-base)$", prefix)} + args := []string{"config", "--get-regexp", fmt.Sprintf("^%s(remote|merge|%s)$", prefix, MergeBaseConfig)} cmd, err := c.Command(ctx, args...) if err != nil { return @@ -406,13 +409,26 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg Branc } case "merge": cfg.MergeRef = parts[1] - case "gh-merge-base": + case MergeBaseConfig: cfg.MergeBase = parts[1] } } return } +// SetBranchConfig sets the named value on the given branch. +func (c *Client) SetBranchConfig(ctx context.Context, branch, name, value string) error { + name = fmt.Sprintf("branch.%s.%s", branch, name) + args := []string{"config", name, value} + cmd, err := c.Command(ctx, args...) + if err != nil { + return err + } + // No output expected but check for any printed git error. + _, err = cmd.Output() + 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/pkg/cmd/issue/develop/develop.go b/pkg/cmd/issue/develop/develop.go index fa01a3dac..bbffa60ac 100644 --- a/pkg/cmd/issue/develop/develop.go +++ b/pkg/cmd/issue/develop/develop.go @@ -171,6 +171,14 @@ func developRunCreate(opts *DevelopOptions, apiClient *api.Client, issueRepo ghr return err } + // Remember which branch to target when creating a PR. + if opts.BaseBranch != "" { + err = opts.GitClient.SetBranchConfig(ctx.Background(), branchName, git.MergeBaseConfig, opts.BaseBranch) + if err != nil { + return err + } + } + fmt.Fprintf(opts.IO.Out, "%s/%s/tree/%s\n", branchRepo.RepoHost(), ghrepo.FullName(branchRepo), branchName) return checkoutBranch(opts, branchRepo, branchName) diff --git a/pkg/cmd/issue/develop/develop_test.go b/pkg/cmd/issue/develop/develop_test.go index abdebf0c8..831f03fc3 100644 --- a/pkg/cmd/issue/develop/develop_test.go +++ b/pkg/cmd/issue/develop/develop_test.go @@ -399,6 +399,7 @@ func TestDevelopRun(t *testing.T) { }, runStubs: func(cs *run.CommandStubber) { cs.Register(`git fetch origin \+refs/heads/my-branch:refs/remotes/origin/my-branch`, 0, "") + cs.Register(`git config branch\.my-branch\.gh-merge-base main`, 0, "") }, expectedOut: "github.com/OWNER/REPO/tree/my-branch\n", }, From 5da86e07e7536db79890040778c7be6fc3b1ac04 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Sun, 8 Dec 2024 17:52:58 -0800 Subject: [PATCH 3/5] Merge changes from #10004 Merges changes from @williammartin including acceptance tests and word changes. Co-authored-by: William Martin --- .../pr-create-from-issue-develop-base.txtar | 37 +++++++++++++++++++ .../pr/pr-create-from-manual-merge-base.txtar | 34 +++++++++++++++++ git/client.go | 2 +- pkg/cmd/issue/develop/develop.go | 7 ++++ pkg/cmd/pr/create/create.go | 4 ++ 5 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 acceptance/testdata/pr/pr-create-from-issue-develop-base.txtar create mode 100644 acceptance/testdata/pr/pr-create-from-manual-merge-base.txtar diff --git a/acceptance/testdata/pr/pr-create-from-issue-develop-base.txtar b/acceptance/testdata/pr/pr-create-from-issue-develop-base.txtar new file mode 100644 index 000000000..f0619940e --- /dev/null +++ b/acceptance/testdata/pr/pr-create-from-issue-develop-base.txtar @@ -0,0 +1,37 @@ +# Set up env vars +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} + +# Create a branch to act as the merge base branch +cd ${REPO} +exec git checkout -b long-lived-feature-branch +exec git push -u origin long-lived-feature-branch + +# Create an issue to develop against +exec gh issue create --title 'Feature Request' --body 'Request Body' +stdout2env ISSUE_URL + +# Create a new branch using issue develop with the long lived branch as the base +exec gh issue develop --name 'feature-branch' --base 'long-lived-feature-branch' --checkout ${ISSUE_URL} + +# Prepare a PR on the develop branch +exec git commit --allow-empty -m 'Empty Commit' +exec git push -u origin feature-branch + +# Create the PR +exec gh pr create --title 'Feature Title' --body 'Feature Body' + +# Check the PR is created against the base branch we specified +exec gh pr view --json 'baseRefName' --jq '.baseRefName' +stdout 'long-lived-feature-branch' diff --git a/acceptance/testdata/pr/pr-create-from-manual-merge-base.txtar b/acceptance/testdata/pr/pr-create-from-manual-merge-base.txtar new file mode 100644 index 000000000..97ae168f5 --- /dev/null +++ b/acceptance/testdata/pr/pr-create-from-manual-merge-base.txtar @@ -0,0 +1,34 @@ +# Set up env vars +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} + +# Create a branch to act as the merge base branch +cd ${REPO} +exec git checkout -b long-lived-feature-branch +exec git push -u origin long-lived-feature-branch + +# Prepare a branch from the merge base to PR +exec git checkout -b feature-branch +exec git commit --allow-empty -m 'Empty Commit' +exec git push -u origin feature-branch + +# Set the merge-base branch config +exec git config 'branch.feature-branch.gh-merge-base' 'long-lived-feature-branch' + +# Create the PR +exec gh pr create --title 'Feature Title' --body 'Feature Body' + +# Check the PR is created against the merge base branch +exec gh pr view --json 'baseRefName' --jq '.baseRefName' +stdout 'long-lived-feature-branch' diff --git a/git/client.go b/git/client.go index 43568839a..35b9cf16c 100644 --- a/git/client.go +++ b/git/client.go @@ -376,7 +376,7 @@ func (c *Client) lookupCommit(ctx context.Context, sha, format string) ([]byte, return out, nil } -// ReadBranchConfig parses the `branch.BRANCH.(remote|merge)` part of git config. +// 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)} diff --git a/pkg/cmd/issue/develop/develop.go b/pkg/cmd/issue/develop/develop.go index bbffa60ac..1536800f0 100644 --- a/pkg/cmd/issue/develop/develop.go +++ b/pkg/cmd/issue/develop/develop.go @@ -44,6 +44,13 @@ func NewCmdDevelop(f *cmdutil.Factory, runF func(*DevelopOptions) error) *cobra. cmd := &cobra.Command{ Use: "develop { | }", Short: "Manage linked branches for an issue", + Long: heredoc.Docf(` + Manage linked branches for an issue. + + When using the %[1]s--base%[1]s flag, the new development branch will be created from the specified + remote branch. The new branch will be configured as the base branch for pull requests created using + %[1]sgh pr create%[1]s. + `, "`"), Example: heredoc.Doc(` # List branches for issue 123 $ gh issue develop --list 123 diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 2074a6f59..f3bd12870 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -119,6 +119,10 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co alongside %[1]s--fill%[1]s, the values specified by %[1]s--title%[1]s and/or %[1]s--body%[1]s will take precedence and overwrite any autofilled content. + The base branch for the created PR can be specified using the %[1]s--base%[1]s flag. If not provided, + the value of %[1]sgh-merge-base%[1]s git branch config will be used. If not configured, the repository's + default branch will be used. + Link an issue to the pull request by referencing the issue in the body of the pull request. If the body text mentions %[1]sFixes #123%[1]s or %[1]sCloses #123%[1]s, the referenced issue will automatically get closed when the pull request gets merged. From e021a072850dac409ce011b01238cd6b3931f7ac Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Sun, 8 Dec 2024 22:27:00 -0800 Subject: [PATCH 4/5] Confirm auto-detected base branch If interactive, confirm the automatically configured gh-merge-branch or, if not configured, the default branch. Based on PR feedback. --- pkg/cmd/pr/create/create.go | 24 +++++++- pkg/cmd/pr/create/create_test.go | 94 ++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index f3bd12870..ac747c36b 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -41,8 +41,9 @@ type CreateOptions struct { Finder shared.PRFinder TitledEditSurvey func(string, string) (string, string, error) - TitleProvided bool - BodyProvided bool + TitleProvided bool + BodyProvided bool + BaseBranchProvided bool RootDirOverride string RepoOverride string @@ -147,6 +148,7 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co opts.TitleProvided = cmd.Flags().Changed("title") opts.RepoOverride, _ = cmd.Flags().GetString("repo") + opts.BaseBranchProvided = cmd.Flags().Changed("base") // Workaround: Due to the way this command is implemented, we need to manually check GH_REPO. // Commands should use the standard BaseRepoOverride functionality to handle this behavior instead. if opts.RepoOverride == "" { @@ -340,7 +342,7 @@ func createRun(opts *CreateOptions) (err error) { ghrepo.FullName(ctx.BaseRepo)) } - if !opts.EditorMode && (opts.FillVerbose || opts.Autofill || opts.FillFirst || (opts.TitleProvided && opts.BodyProvided)) { + if !opts.EditorMode && (opts.FillVerbose || opts.Autofill || opts.FillFirst || (opts.TitleProvided && opts.BodyProvided && ctx.BaseBranch != "")) { err = handlePush(*opts, *ctx) if err != nil { return @@ -422,6 +424,14 @@ func createRun(opts *CreateOptions) (err error) { } } + // Confirm the automatically-selected base branch. + if !opts.BaseBranchProvided { + err = confirmTrackingBranch(opts, ctx) + if err != nil { + return + } + } + openURL, err = generateCompareURL(*ctx, *state) if err != nil { return @@ -557,6 +567,14 @@ func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, h return nil } +func confirmTrackingBranch(opts *CreateOptions, ctx *CreateContext) (err error) { + ctx.BaseBranch, err = opts.Prompter.Input("Base branch", ctx.BaseBranch) + if err != nil { + return + } + return nil +} + func NewIssueState(ctx CreateContext, opts CreateOptions) (*shared.IssueMetadataState, error) { var milestoneTitles []string if opts.Milestone != "" { diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 27220d052..1b4e7dbf7 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1217,6 +1217,8 @@ func Test_createRun(t *testing.T) { pm.InputFunc = func(p, d string) (string, error) { if p == "Title (required)" { return d, nil + } else if p == "Base branch" { + return d, nil } else { return "", prompter.NoSuchPromptErr(p) } @@ -1323,6 +1325,8 @@ func Test_createRun(t *testing.T) { pm.InputFunc = func(p, d string) (string, error) { if p == "Title (required)" { return d, nil + } else if p == "Base branch" { + return d, nil } else if p == "Body" { return d, nil } else { @@ -1528,6 +1532,88 @@ func Test_createRun(t *testing.T) { expectedOut: "https://github.com/OWNER/REPO/pull/12\n", expectedErrOut: "\nCreating pull request for monalisa:task1 into feature/feat2 in OWNER/REPO\n\n", }, + { + name: "non-default base branch prompt", + tty: true, + setup: func(opts *CreateOptions, t *testing.T) func() { + opts.BodyProvided = true + 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/feat3", 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 + deadb01f refs/remotes/upstream/feature/feat3 + deadbeef refs/remotes/origin/task1`)) // determineTrackingBranch + cs.Register( + "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry upstream/feature/feat2...task1", + 0, + "3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000my title\u0000my body\u0000\n") // initDefaultTitleBody from original base branch + }, + promptStubs: func(pm *prompter.PrompterMock) { + pm.InputFunc = func(p, d string) (string, error) { + switch p { + case "Title (required)": + return "my title", nil + case "Base branch": + return "feature/feat3", nil + default: + return "", prompter.NoSuchPromptErr(p) + } + } + pm.SelectFunc = func(p, _ string, opts []string) (int, error) { + if p == "What's next?" { + return 0, nil + } else { + return -1, prompter.NoSuchPromptErr(p) + } + } + }, + expectedOut: "https://github.com/OWNER/REPO/pull/12\n", + // Output message is created based on initial configuration; prompt will later override. + 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) { @@ -1541,6 +1627,14 @@ func Test_createRun(t *testing.T) { } pm := &prompter.PrompterMock{} + pm.InputFunc = func(p, d string) (string, error) { + switch p { + case "Base branch": + return d, nil + default: + return "", prompter.NoSuchPromptErr(p) + } + } if tt.promptStubs != nil { tt.promptStubs(pm) From 54a7f4de7016cf9dcbc4caac4ff13efc6954212f Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 9 Dec 2024 13:01:00 +0100 Subject: [PATCH 5/5] Revert "Confirm auto-detected base branch" This reverts commit e021a072850dac409ce011b01238cd6b3931f7ac. --- pkg/cmd/pr/create/create.go | 24 +------- pkg/cmd/pr/create/create_test.go | 94 -------------------------------- 2 files changed, 3 insertions(+), 115 deletions(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index ac747c36b..f3bd12870 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -41,9 +41,8 @@ type CreateOptions struct { Finder shared.PRFinder TitledEditSurvey func(string, string) (string, string, error) - TitleProvided bool - BodyProvided bool - BaseBranchProvided bool + TitleProvided bool + BodyProvided bool RootDirOverride string RepoOverride string @@ -148,7 +147,6 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co opts.TitleProvided = cmd.Flags().Changed("title") opts.RepoOverride, _ = cmd.Flags().GetString("repo") - opts.BaseBranchProvided = cmd.Flags().Changed("base") // Workaround: Due to the way this command is implemented, we need to manually check GH_REPO. // Commands should use the standard BaseRepoOverride functionality to handle this behavior instead. if opts.RepoOverride == "" { @@ -342,7 +340,7 @@ func createRun(opts *CreateOptions) (err error) { ghrepo.FullName(ctx.BaseRepo)) } - if !opts.EditorMode && (opts.FillVerbose || opts.Autofill || opts.FillFirst || (opts.TitleProvided && opts.BodyProvided && ctx.BaseBranch != "")) { + if !opts.EditorMode && (opts.FillVerbose || opts.Autofill || opts.FillFirst || (opts.TitleProvided && opts.BodyProvided)) { err = handlePush(*opts, *ctx) if err != nil { return @@ -424,14 +422,6 @@ func createRun(opts *CreateOptions) (err error) { } } - // Confirm the automatically-selected base branch. - if !opts.BaseBranchProvided { - err = confirmTrackingBranch(opts, ctx) - if err != nil { - return - } - } - openURL, err = generateCompareURL(*ctx, *state) if err != nil { return @@ -567,14 +557,6 @@ func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, h return nil } -func confirmTrackingBranch(opts *CreateOptions, ctx *CreateContext) (err error) { - ctx.BaseBranch, err = opts.Prompter.Input("Base branch", ctx.BaseBranch) - if err != nil { - return - } - return nil -} - func NewIssueState(ctx CreateContext, opts CreateOptions) (*shared.IssueMetadataState, error) { var milestoneTitles []string if opts.Milestone != "" { diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 1b4e7dbf7..27220d052 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1217,8 +1217,6 @@ func Test_createRun(t *testing.T) { pm.InputFunc = func(p, d string) (string, error) { if p == "Title (required)" { return d, nil - } else if p == "Base branch" { - return d, nil } else { return "", prompter.NoSuchPromptErr(p) } @@ -1325,8 +1323,6 @@ func Test_createRun(t *testing.T) { pm.InputFunc = func(p, d string) (string, error) { if p == "Title (required)" { return d, nil - } else if p == "Base branch" { - return d, nil } else if p == "Body" { return d, nil } else { @@ -1532,88 +1528,6 @@ func Test_createRun(t *testing.T) { expectedOut: "https://github.com/OWNER/REPO/pull/12\n", expectedErrOut: "\nCreating pull request for monalisa:task1 into feature/feat2 in OWNER/REPO\n\n", }, - { - name: "non-default base branch prompt", - tty: true, - setup: func(opts *CreateOptions, t *testing.T) func() { - opts.BodyProvided = true - 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/feat3", 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 - deadb01f refs/remotes/upstream/feature/feat3 - deadbeef refs/remotes/origin/task1`)) // determineTrackingBranch - cs.Register( - "git -c log.ShowSignature=false log --pretty=format:%H%x00%s%x00%b%x00 --cherry upstream/feature/feat2...task1", - 0, - "3a9b48085046d156c5acce8f3b3a0532cd706a4a\u0000my title\u0000my body\u0000\n") // initDefaultTitleBody from original base branch - }, - promptStubs: func(pm *prompter.PrompterMock) { - pm.InputFunc = func(p, d string) (string, error) { - switch p { - case "Title (required)": - return "my title", nil - case "Base branch": - return "feature/feat3", nil - default: - return "", prompter.NoSuchPromptErr(p) - } - } - pm.SelectFunc = func(p, _ string, opts []string) (int, error) { - if p == "What's next?" { - return 0, nil - } else { - return -1, prompter.NoSuchPromptErr(p) - } - } - }, - expectedOut: "https://github.com/OWNER/REPO/pull/12\n", - // Output message is created based on initial configuration; prompt will later override. - 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) { @@ -1627,14 +1541,6 @@ func Test_createRun(t *testing.T) { } pm := &prompter.PrompterMock{} - pm.InputFunc = func(p, d string) (string, error) { - switch p { - case "Base branch": - return d, nil - default: - return "", prompter.NoSuchPromptErr(p) - } - } if tt.promptStubs != nil { tt.promptStubs(pm)