From d57cb56945ddd17c41cc420e8b3aafd8994dcb71 Mon Sep 17 00:00:00 2001 From: Cristian Dominguez Date: Tue, 2 Feb 2021 08:43:42 -0300 Subject: [PATCH 01/10] Allow editing commit msg when squash merging a PR --- api/queries_pr.go | 5 + pkg/cmd/pr/merge/merge.go | 121 +++++++++++++++++++++--- pkg/cmd/pr/merge/merge_test.go | 164 +++++++++++++++++++++++++++++++-- pkg/cmd/pr/shared/survey.go | 1 + 4 files changed, 269 insertions(+), 22 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 4c35a153a..1743031ee 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -88,6 +88,9 @@ type PullRequest struct { ReactionGroups ReactionGroups Reviews PullRequestReviews ReviewRequests ReviewRequests + + ViewerMergeBodyText string + ViewerMergeHeadlineText string } type ReviewRequests struct { @@ -584,6 +587,8 @@ func PullRequestByNumber(client *Client, repo ghrepo.Interface, number int) (*Pu milestone{ title } + viewerMergeBodyText + viewerMergeHeadlineText ` + commentsFragment() + ` ` + reactionGroupsFragment() + ` } diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 4c8c946c0..e5d3cb274 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -16,6 +16,7 @@ import ( "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/pkg/prompt" + "github.com/cli/cli/pkg/surveyext" "github.com/spf13/cobra" ) @@ -154,6 +155,8 @@ func mergeRun(opts *MergeOptions) error { if !isPRAlreadyMerged { mergeMethod := opts.MergeMethod + action := shared.SubmitAction + if opts.InteractiveMode { r, err := api.GitHubRepo(apiClient, baseRepo) if err != nil { @@ -167,19 +170,52 @@ func mergeRun(opts *MergeOptions) error { if err != nil { return err } - confirm, err := confirmSurvey() + + allowEditMsg := mergeMethod != api.PullRequestMergeMethodRebase + + action, err = confirmSubmission(allowEditMsg) if err != nil { - return err + return fmt.Errorf("unable to confirm: %w", err) } - if !confirm { + + if action == shared.EditCommitMessageAction { + var editorCommand string + editorCommand, err = cmdutil.DetermineEditor(opts.Config) + if err != nil { + return err + } + + if opts.Body == nil { + var body string + + if mergeMethod == api.PullRequestMergeMethodSquash { + body = fmt.Sprintf("%s\n\n%s", pr.ViewerMergeHeadlineText, pr.ViewerMergeBodyText) + } + + opts.Body = &body + } + + err := commitMsgSurvey(opts.Body, editorCommand) + if err != nil { + return err + } + + action, err = confirmSubmission(false) + if err != nil { + return fmt.Errorf("unable to confirm: %w", err) + } + } + + if action == shared.CancelAction { fmt.Fprintln(opts.IO.ErrOut, "Cancelled.") return cmdutil.SilentError } } - - err = api.PullRequestMerge(apiClient, baseRepo, pr, mergeMethod, opts.Body) - if err != nil { - return err + if action == shared.SubmitAction { + err = api.PullRequestMerge(apiClient, baseRepo, pr, mergeMethod, opts.Body) + if err != nil { + return err + } } if isTerminal { @@ -316,12 +352,69 @@ func deleteBranchSurvey(opts *MergeOptions, crossRepoPR bool) (bool, error) { return opts.DeleteBranch, nil } -func confirmSurvey() (bool, error) { - var confirm bool - submit := &survey.Confirm{ - Message: "Submit?", - Default: true, +func confirmSubmission(allowEditMsg bool) (shared.Action, error) { + const ( + submitLabel = "Submit" + editCommitMsgLabel = "Edit Commit Message" + cancelLabel = "Cancel" + ) + options := []string{submitLabel} + if allowEditMsg { + options = append(options, editCommitMsgLabel) + } + options = append(options, cancelLabel) + + confirmAnswers := struct { + Confirmation int + }{} + + confirmQs := []*survey.Question{ + { + Name: "confirmation", + Prompt: &survey.Select{ + Message: "What's next?", + Options: options, + }, + }, + } + + err := prompt.SurveyAsk(confirmQs, &confirmAnswers) + if err != nil { + return -1, fmt.Errorf("could not prompt: %w", err) + } + + switch options[confirmAnswers.Confirmation] { + case submitLabel: + return shared.SubmitAction, nil + case editCommitMsgLabel: + return shared.EditCommitMessageAction, nil + case cancelLabel: + return shared.CancelAction, nil + default: + return -1, fmt.Errorf("invalid index: %d", confirmAnswers.Confirmation) } - err := prompt.SurveyAskOne(submit, &confirm) - return confirm, err +} + +func commitMsgSurvey(body *string, editorCommand string) error { + qs := []*survey.Question{ + { + Name: "body", + Prompt: &surveyext.GhEditor{ + BlankAllowed: true, + EditorCommand: editorCommand, + Editor: &survey.Editor{ + Message: "Body", + AppendDefault: true, + Default: *body, + FileName: "*.md", + }, + }, + }, + } + err := prompt.SurveyAsk(qs, body) + if err != nil { + return err + } + + return nil } diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index ce772f1e6..ba31544f6 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -626,9 +626,14 @@ func TestPRMerge_interactive(t *testing.T) { as, surveyTeardown := prompt.InitAskStubber() defer surveyTeardown() - as.StubOne(0) // Merge method survey - as.StubOne(true) // Delete branch survey - as.StubOne(true) // Confirm submit survey + as.StubOne(0) // Merge method survey + as.StubOne(true) // Delete branch survey + as.Stub([]*prompt.QuestionStub{ // Confirm submit survey + { + Name: "confirmation", + Value: 0, + }, + }) output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -682,8 +687,14 @@ func TestPRMerge_interactiveWithDeleteBranch(t *testing.T) { as, surveyTeardown := prompt.InitAskStubber() defer surveyTeardown() - as.StubOne(0) // Merge method survey - as.StubOne(true) // Confirm submit survey + as.StubOne(0) // Merge method survey + as.StubOne(true) // Confirm submit survey + as.Stub([]*prompt.QuestionStub{ // Confirm submit survey + { + Name: "confirmation", + Value: 0, + }, + }) output, err := runCommand(http, "blueberries", true, "-d") if err != nil { @@ -694,6 +705,138 @@ func TestPRMerge_interactiveWithDeleteBranch(t *testing.T) { test.ExpectLines(t, output.Stderr(), "Merged pull request #3", "Deleted branch blueberries and switched to branch master") } +func TestPRMerge_interactiveSquashEditCommitMsg(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + http.Register( + httpmock.GraphQL(`query PullRequestForBranch\b`), + httpmock.StringResponse(` + { "data": { "repository": { "pullRequests": { "nodes": [{ + "headRefName": "blueberries", + "headRepositoryOwner": {"login": "OWNER"}, + "id": "THE-ID", + "number": 3 + }] } } } }`)) + http.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "mergeCommitAllowed": true, + "rebaseMergeAllowed": true, + "squashMergeAllowed": true + } } }`)) + http.Register( + httpmock.GraphQL(`mutation PullRequestMerge\b`), + httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { + assert.Equal(t, "THE-ID", input["pullRequestId"].(string)) + assert.Equal(t, "SQUASH", input["mergeMethod"].(string)) + assert.Equal(t, "cool story", input["commitBody"].(string)) + })) + + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "") + + as, surveyTeardown := prompt.InitAskStubber() + defer surveyTeardown() + + as.StubOne(2) // Merge method survey + as.StubOne(false) // Delete branch survey + as.Stub([]*prompt.QuestionStub{ // Confirm submit survey + { + Name: "confirmation", + Value: 1, + }, + }) + as.Stub([]*prompt.QuestionStub{ // Edit Commit Msg editor survey + { + Name: "body", + Value: "cool story", + }, + }) + as.Stub([]*prompt.QuestionStub{ // Confirm submit survey + { + Name: "confirmation", + Value: 0, + }, + }) + + output, err := runCommand(http, "blueberries", true, "") + if err != nil { + t.Fatalf("Got unexpected error running `pr merge` %s", err) + } + + assert.Equal(t, "✔ Squashed and merged pull request #3 ()\n", output.Stderr()) +} + +func TestPRMerge_interactiveSquashEditCommitMsgDefaultBody(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + http.Register( + httpmock.GraphQL(`query PullRequestForBranch\b`), + httpmock.StringResponse(` + { "data": { "repository": { "pullRequests": { "nodes": [{ + "headRefName": "blueberries", + "headRepositoryOwner": {"login": "OWNER"}, + "id": "THE-ID", + "number": 3, + "viewerMergeBodyText": "Co-authored by: Octocat ", + "viewerMergeHeadlineText": "Add new feat (#33)" + }] } } } }`)) + http.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "mergeCommitAllowed": true, + "rebaseMergeAllowed": true, + "squashMergeAllowed": true + } } }`)) + http.Register( + httpmock.GraphQL(`mutation PullRequestMerge\b`), + httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { + assert.Equal(t, "THE-ID", input["pullRequestId"].(string)) + assert.Equal(t, "SQUASH", input["mergeMethod"].(string)) + assert.Equal(t, "Add new feat (#33)\n\nCo-authored by: Octocat ", input["commitBody"]) + })) + + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "") + + as, surveyTeardown := prompt.InitAskStubber() + defer surveyTeardown() + + as.StubOne(2) // Merge method survey + as.StubOne(false) // Delete branch survey + as.Stub([]*prompt.QuestionStub{ // Confirm submit survey + { + Name: "confirmation", + Value: 1, + }, + }) + as.Stub([]*prompt.QuestionStub{ // Edit Commit Msg editor survey + { + Name: "body", + Default: true, + }, + }) + as.Stub([]*prompt.QuestionStub{ // Confirm submit survey + { + Name: "confirmation", + Value: 0, + }, + }) + + output, err := runCommand(http, "blueberries", true, "") + if err != nil { + t.Fatalf("Got unexpected error running `pr merge` %s", err) + } + + assert.Equal(t, "✔ Squashed and merged pull request #3 ()\n", output.Stderr()) +} + func TestPRMerge_interactiveCancelled(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) @@ -724,9 +867,14 @@ func TestPRMerge_interactiveCancelled(t *testing.T) { as, surveyTeardown := prompt.InitAskStubber() defer surveyTeardown() - as.StubOne(0) // Merge method survey - as.StubOne(true) // Delete branch survey - as.StubOne(false) // Confirm submit survey + as.StubOne(0) // Merge method survey + as.StubOne(true) // Delete branch survey + as.Stub([]*prompt.QuestionStub{ // Confirm submit survey + { + Name: "confirmation", + Value: 2, + }, + }) output, err := runCommand(http, "blueberries", true, "") if !errors.Is(err, cmdutil.SilentError) { diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index 8ef40d047..836f83889 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -21,6 +21,7 @@ const ( PreviewAction CancelAction MetadataAction + EditCommitMessageAction noMilestone = "(none)" ) From 4ea8d25b8566b4eaf1f69d4a7b7f5625654c781d Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Thu, 4 Feb 2021 13:22:26 -0800 Subject: [PATCH 02/10] Fix tests and polish --- api/queries_pr.go | 5 -- pkg/cmd/pr/merge/merge.go | 106 ++++++++-------------- pkg/cmd/pr/merge/merge_test.go | 158 ++++++--------------------------- 3 files changed, 65 insertions(+), 204 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 1743031ee..4c35a153a 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -88,9 +88,6 @@ type PullRequest struct { ReactionGroups ReactionGroups Reviews PullRequestReviews ReviewRequests ReviewRequests - - ViewerMergeBodyText string - ViewerMergeHeadlineText string } type ReviewRequests struct { @@ -587,8 +584,6 @@ func PullRequestByNumber(client *Client, repo ghrepo.Interface, number int) (*Pu milestone{ title } - viewerMergeBodyText - viewerMergeHeadlineText ` + commentsFragment() + ` ` + reactionGroupsFragment() + ` } diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index e5d3cb274..f9fcb3142 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -32,7 +32,8 @@ type MergeOptions struct { DeleteBranch bool MergeMethod api.PullRequestMergeMethod - Body *string + Body string + BodyProvided bool IsDeleteBranchIndicated bool CanDeleteLocalBranch bool @@ -99,8 +100,7 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm opts.CanDeleteLocalBranch = !cmd.Flags().Changed("repo") if cmd.Flags().Changed("body") { - bodyStr, _ := cmd.Flags().GetString("body") - opts.Body = &bodyStr + opts.BodyProvided = true } if runF != nil { @@ -111,7 +111,7 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm } cmd.Flags().BoolVarP(&opts.DeleteBranch, "delete-branch", "d", false, "Delete the local and remote branch after merge") - cmd.Flags().StringP("body", "b", "", "Body for merge commit") + cmd.Flags().StringVarP(&opts.Body, "body", "b", "", "Body for merge commit") cmd.Flags().BoolVarP(&flagMerge, "merge", "m", false, "Merge the commits with the base branch") cmd.Flags().BoolVarP(&flagRebase, "rebase", "r", false, "Rebase the commits onto the base branch") cmd.Flags().BoolVarP(&flagSquash, "squash", "s", false, "Squash the commits into one commit and merge it into the base branch") @@ -155,8 +155,6 @@ func mergeRun(opts *MergeOptions) error { if !isPRAlreadyMerged { mergeMethod := opts.MergeMethod - action := shared.SubmitAction - if opts.InteractiveMode { r, err := api.GitHubRepo(apiClient, baseRepo) if err != nil { @@ -171,9 +169,9 @@ func mergeRun(opts *MergeOptions) error { return err } - allowEditMsg := mergeMethod != api.PullRequestMergeMethodRebase + allowEditMsg := (mergeMethod != api.PullRequestMergeMethodRebase) && !opts.BodyProvided - action, err = confirmSubmission(allowEditMsg) + action, err := confirmSurvey(allowEditMsg) if err != nil { return fmt.Errorf("unable to confirm: %w", err) } @@ -185,37 +183,32 @@ func mergeRun(opts *MergeOptions) error { return err } - if opts.Body == nil { - var body string - - if mergeMethod == api.PullRequestMergeMethodSquash { - body = fmt.Sprintf("%s\n\n%s", pr.ViewerMergeHeadlineText, pr.ViewerMergeBodyText) - } - - opts.Body = &body - } - - err := commitMsgSurvey(opts.Body, editorCommand) + msg, err := commitMsgSurvey(editorCommand) if err != nil { return err } + opts.Body = msg + opts.BodyProvided = true - action, err = confirmSubmission(false) + action, err = confirmSurvey(false) if err != nil { return fmt.Errorf("unable to confirm: %w", err) } } - if action == shared.CancelAction { fmt.Fprintln(opts.IO.ErrOut, "Cancelled.") return cmdutil.SilentError } } - if action == shared.SubmitAction { - err = api.PullRequestMerge(apiClient, baseRepo, pr, mergeMethod, opts.Body) - if err != nil { - return err - } + + var body *string + if opts.BodyProvided { + body = &opts.Body + } + + err = api.PullRequestMerge(apiClient, baseRepo, pr, mergeMethod, body) + if err != nil { + return err } if isTerminal { @@ -352,69 +345,48 @@ func deleteBranchSurvey(opts *MergeOptions, crossRepoPR bool) (bool, error) { return opts.DeleteBranch, nil } -func confirmSubmission(allowEditMsg bool) (shared.Action, error) { +func confirmSurvey(allowEditMsg bool) (shared.Action, error) { const ( submitLabel = "Submit" - editCommitMsgLabel = "Edit Commit Message" + editCommitMsgLabel = "Edit commit message" cancelLabel = "Cancel" ) + options := []string{submitLabel} if allowEditMsg { options = append(options, editCommitMsgLabel) } options = append(options, cancelLabel) - confirmAnswers := struct { - Confirmation int - }{} - - confirmQs := []*survey.Question{ - { - Name: "confirmation", - Prompt: &survey.Select{ - Message: "What's next?", - Options: options, - }, - }, + var result string + submit := &survey.Select{ + Message: "What's next?", + Options: options, } - - err := prompt.SurveyAsk(confirmQs, &confirmAnswers) + err := prompt.SurveyAskOne(submit, &result) if err != nil { - return -1, fmt.Errorf("could not prompt: %w", err) + return shared.CancelAction, fmt.Errorf("could not prompt: %w", err) } - switch options[confirmAnswers.Confirmation] { + switch result { case submitLabel: return shared.SubmitAction, nil case editCommitMsgLabel: return shared.EditCommitMessageAction, nil - case cancelLabel: - return shared.CancelAction, nil default: - return -1, fmt.Errorf("invalid index: %d", confirmAnswers.Confirmation) + return shared.CancelAction, nil } } -func commitMsgSurvey(body *string, editorCommand string) error { - qs := []*survey.Question{ - { - Name: "body", - Prompt: &surveyext.GhEditor{ - BlankAllowed: true, - EditorCommand: editorCommand, - Editor: &survey.Editor{ - Message: "Body", - AppendDefault: true, - Default: *body, - FileName: "*.md", - }, - }, +func commitMsgSurvey(editorCommand string) (string, error) { + var result string + q := &surveyext.GhEditor{ + EditorCommand: editorCommand, + Editor: &survey.Editor{ + Message: "Body", + FileName: "*.md", }, } - err := prompt.SurveyAsk(qs, body) - if err != nil { - return err - } - - return nil + err := prompt.SurveyAskOne(q, &result) + return result, err } diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index ba31544f6..f3adc4e6d 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -27,12 +27,11 @@ import ( func Test_NewCmdMerge(t *testing.T) { tests := []struct { - name string - args string - isTTY bool - want MergeOptions - wantBody string - wantErr string + name string + args string + isTTY bool + want MergeOptions + wantErr string }{ { name: "number argument", @@ -71,8 +70,9 @@ func Test_NewCmdMerge(t *testing.T) { CanDeleteLocalBranch: true, MergeMethod: api.PullRequestMergeMethodMerge, InteractiveMode: true, + Body: "cool", + BodyProvided: true, }, - wantBody: "cool", }, { name: "no argument with --repo override", @@ -138,12 +138,8 @@ func Test_NewCmdMerge(t *testing.T) { assert.Equal(t, tt.want.CanDeleteLocalBranch, opts.CanDeleteLocalBranch) assert.Equal(t, tt.want.MergeMethod, opts.MergeMethod) assert.Equal(t, tt.want.InteractiveMode, opts.InteractiveMode) - - if tt.wantBody == "" { - assert.Nil(t, opts.Body) - } else { - assert.Equal(t, tt.wantBody, *opts.Body) - } + assert.Equal(t, tt.want.Body, opts.Body) + assert.Equal(t, tt.want.BodyProvided, opts.BodyProvided) }) } } @@ -610,30 +606,19 @@ func TestPRMerge_interactive(t *testing.T) { assert.Equal(t, "MERGE", input["mergeMethod"].(string)) assert.NotContains(t, input, "commitHeadline") })) - http.Register( - httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), - httpmock.StringResponse(`{}`)) cs, cmdTeardown := run.Stub() defer cmdTeardown(t) cs.Register("git -c log.ShowSignature=false log --pretty=format:%H,%s -1", 0, "") cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "") - cs.Register(`git checkout master`, 0, "") - cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") - cs.Register(`git branch -D blueberries`, 0, "") as, surveyTeardown := prompt.InitAskStubber() defer surveyTeardown() - as.StubOne(0) // Merge method survey - as.StubOne(true) // Delete branch survey - as.Stub([]*prompt.QuestionStub{ // Confirm submit survey - { - Name: "confirmation", - Value: 0, - }, - }) + as.StubOne(0) // Merge method survey + as.StubOne(false) // Delete branch survey + as.StubOne("Submit") // Confirm submit survey output, err := runCommand(http, "blueberries", true, "") if err != nil { @@ -687,14 +672,8 @@ func TestPRMerge_interactiveWithDeleteBranch(t *testing.T) { as, surveyTeardown := prompt.InitAskStubber() defer surveyTeardown() - as.StubOne(0) // Merge method survey - as.StubOne(true) // Confirm submit survey - as.Stub([]*prompt.QuestionStub{ // Confirm submit survey - { - Name: "confirmation", - Value: 0, - }, - }) + as.StubOne(0) // Merge method survey + as.StubOne("Submit") // Confirm submit survey output, err := runCommand(http, "blueberries", true, "-d") if err != nil { @@ -715,7 +694,8 @@ func TestPRMerge_interactiveSquashEditCommitMsg(t *testing.T) { "headRefName": "blueberries", "headRepositoryOwner": {"login": "OWNER"}, "id": "THE-ID", - "number": 3 + "number": 3, + "title": "title" }] } } } }`)) http.Register( httpmock.GraphQL(`query RepositoryInfo\b`), @@ -736,105 +716,24 @@ func TestPRMerge_interactiveSquashEditCommitMsg(t *testing.T) { cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register("git -c log.ShowSignature=false log --pretty=format:%H,%s -1", 0, "") cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "") as, surveyTeardown := prompt.InitAskStubber() defer surveyTeardown() - as.StubOne(2) // Merge method survey - as.StubOne(false) // Delete branch survey - as.Stub([]*prompt.QuestionStub{ // Confirm submit survey - { - Name: "confirmation", - Value: 1, - }, - }) - as.Stub([]*prompt.QuestionStub{ // Edit Commit Msg editor survey - { - Name: "body", - Value: "cool story", - }, - }) - as.Stub([]*prompt.QuestionStub{ // Confirm submit survey - { - Name: "confirmation", - Value: 0, - }, - }) + as.StubOne(2) // Merge method survey + as.StubOne(false) // Delete branch survey + as.StubOne("Edit commit message") // Confirm submit survey + as.StubOne("cool story") // Edit commit message survey + as.StubOne("Submit") // Confirm submit survey output, err := runCommand(http, "blueberries", true, "") if err != nil { t.Fatalf("Got unexpected error running `pr merge` %s", err) } - assert.Equal(t, "✔ Squashed and merged pull request #3 ()\n", output.Stderr()) -} - -func TestPRMerge_interactiveSquashEditCommitMsgDefaultBody(t *testing.T) { - http := initFakeHTTP() - defer http.Verify(t) - http.Register( - httpmock.GraphQL(`query PullRequestForBranch\b`), - httpmock.StringResponse(` - { "data": { "repository": { "pullRequests": { "nodes": [{ - "headRefName": "blueberries", - "headRepositoryOwner": {"login": "OWNER"}, - "id": "THE-ID", - "number": 3, - "viewerMergeBodyText": "Co-authored by: Octocat ", - "viewerMergeHeadlineText": "Add new feat (#33)" - }] } } } }`)) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "mergeCommitAllowed": true, - "rebaseMergeAllowed": true, - "squashMergeAllowed": true - } } }`)) - http.Register( - httpmock.GraphQL(`mutation PullRequestMerge\b`), - httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { - assert.Equal(t, "THE-ID", input["pullRequestId"].(string)) - assert.Equal(t, "SQUASH", input["mergeMethod"].(string)) - assert.Equal(t, "Add new feat (#33)\n\nCo-authored by: Octocat ", input["commitBody"]) - })) - - cs, cmdTeardown := run.Stub() - defer cmdTeardown(t) - - cs.Register(`git config --get-regexp.+branch\\\.blueberries\\\.`, 0, "") - - as, surveyTeardown := prompt.InitAskStubber() - defer surveyTeardown() - - as.StubOne(2) // Merge method survey - as.StubOne(false) // Delete branch survey - as.Stub([]*prompt.QuestionStub{ // Confirm submit survey - { - Name: "confirmation", - Value: 1, - }, - }) - as.Stub([]*prompt.QuestionStub{ // Edit Commit Msg editor survey - { - Name: "body", - Default: true, - }, - }) - as.Stub([]*prompt.QuestionStub{ // Confirm submit survey - { - Name: "confirmation", - Value: 0, - }, - }) - - output, err := runCommand(http, "blueberries", true, "") - if err != nil { - t.Fatalf("Got unexpected error running `pr merge` %s", err) - } - - assert.Equal(t, "✔ Squashed and merged pull request #3 ()\n", output.Stderr()) + assert.Equal(t, "✓ Squashed and merged pull request #3 (title)\n", output.Stderr()) } func TestPRMerge_interactiveCancelled(t *testing.T) { @@ -867,14 +766,9 @@ func TestPRMerge_interactiveCancelled(t *testing.T) { as, surveyTeardown := prompt.InitAskStubber() defer surveyTeardown() - as.StubOne(0) // Merge method survey - as.StubOne(true) // Delete branch survey - as.Stub([]*prompt.QuestionStub{ // Confirm submit survey - { - Name: "confirmation", - Value: 2, - }, - }) + as.StubOne(0) // Merge method survey + as.StubOne(true) // Delete branch survey + as.StubOne("Cancel") // Confirm submit survey output, err := runCommand(http, "blueberries", true, "") if !errors.Is(err, cmdutil.SilentError) { From f75bd7280fddce4e53cd6bd82f8e9b3e73a46e50 Mon Sep 17 00:00:00 2001 From: Cristian Dominguez Date: Mon, 8 Feb 2021 19:45:51 -0300 Subject: [PATCH 03/10] Pre-populate default merge commit message if no body was provided --- api/queries_pr.go | 3 +++ pkg/cmd/pr/merge/merge.go | 30 +++++++++++++++++------------- pkg/cmd/pr/merge/merge_test.go | 2 -- 3 files changed, 20 insertions(+), 15 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 4c35a153a..c84dd2880 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -88,6 +88,8 @@ type PullRequest struct { ReactionGroups ReactionGroups Reviews PullRequestReviews ReviewRequests ReviewRequests + + ViewerMergeBodyText string } type ReviewRequests struct { @@ -584,6 +586,7 @@ func PullRequestByNumber(client *Client, repo ghrepo.Interface, number int) (*Pu milestone{ title } + viewerMergeBodyText ` + commentsFragment() + ` ` + reactionGroupsFragment() + ` } diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index f9fcb3142..9003765d2 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -32,8 +32,7 @@ type MergeOptions struct { DeleteBranch bool MergeMethod api.PullRequestMergeMethod - Body string - BodyProvided bool + Body string IsDeleteBranchIndicated bool CanDeleteLocalBranch bool @@ -99,10 +98,6 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm opts.IsDeleteBranchIndicated = cmd.Flags().Changed("delete-branch") opts.CanDeleteLocalBranch = !cmd.Flags().Changed("repo") - if cmd.Flags().Changed("body") { - opts.BodyProvided = true - } - if runF != nil { return runF(opts) } @@ -169,7 +164,7 @@ func mergeRun(opts *MergeOptions) error { return err } - allowEditMsg := (mergeMethod != api.PullRequestMergeMethodRebase) && !opts.BodyProvided + allowEditMsg := mergeMethod != api.PullRequestMergeMethodRebase action, err := confirmSurvey(allowEditMsg) if err != nil { @@ -183,12 +178,19 @@ func mergeRun(opts *MergeOptions) error { return err } - msg, err := commitMsgSurvey(editorCommand) + if opts.Body == "" { + if mergeMethod == api.PullRequestMergeMethodMerge { + opts.Body = pr.Title + } else { + opts.Body = pr.ViewerMergeBodyText + } + } + + msg, err := commitMsgSurvey(opts.Body, editorCommand) if err != nil { return err } opts.Body = msg - opts.BodyProvided = true action, err = confirmSurvey(false) if err != nil { @@ -202,7 +204,7 @@ func mergeRun(opts *MergeOptions) error { } var body *string - if opts.BodyProvided { + if opts.Body != "" { body = &opts.Body } @@ -378,13 +380,15 @@ func confirmSurvey(allowEditMsg bool) (shared.Action, error) { } } -func commitMsgSurvey(editorCommand string) (string, error) { +func commitMsgSurvey(msg string, editorCommand string) (string, error) { var result string q := &surveyext.GhEditor{ EditorCommand: editorCommand, Editor: &survey.Editor{ - Message: "Body", - FileName: "*.md", + Message: "Body", + AppendDefault: true, + Default: msg, + FileName: "*.md", }, } err := prompt.SurveyAskOne(q, &result) diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index f3adc4e6d..94c75c2c2 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -71,7 +71,6 @@ func Test_NewCmdMerge(t *testing.T) { MergeMethod: api.PullRequestMergeMethodMerge, InteractiveMode: true, Body: "cool", - BodyProvided: true, }, }, { @@ -139,7 +138,6 @@ func Test_NewCmdMerge(t *testing.T) { assert.Equal(t, tt.want.MergeMethod, opts.MergeMethod) assert.Equal(t, tt.want.InteractiveMode, opts.InteractiveMode) assert.Equal(t, tt.want.Body, opts.Body) - assert.Equal(t, tt.want.BodyProvided, opts.BodyProvided) }) } } From 67bfedd56b3d68b11c838ebbb8123ab502992816 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 16 Feb 2021 15:55:24 +0100 Subject: [PATCH 04/10] Add `pr merge --auto` --- api/queries_pr.go | 49 ---------------- pkg/cmd/pr/merge/http.go | 100 ++++++++++++++++++++++++++++++++ pkg/cmd/pr/merge/merge.go | 102 ++++++++++++++++++++++----------- pkg/cmd/pr/merge/merge_test.go | 8 +-- 4 files changed, 173 insertions(+), 86 deletions(-) create mode 100644 pkg/cmd/pr/merge/http.go diff --git a/api/queries_pr.go b/api/queries_pr.go index c84dd2880..b18022ddb 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -140,14 +140,6 @@ type PullRequestReviewStatus struct { ReviewRequired bool } -type PullRequestMergeMethod int - -const ( - PullRequestMergeMethodMerge PullRequestMergeMethod = iota - PullRequestMergeMethodRebase - PullRequestMergeMethodSquash -) - func (pr *PullRequest) ReviewStatus() PullRequestReviewStatus { var status PullRequestReviewStatus switch pr.ReviewDecision { @@ -1074,47 +1066,6 @@ func PullRequestReopen(client *Client, repo ghrepo.Interface, pr *PullRequest) e return err } -func PullRequestMerge(client *Client, repo ghrepo.Interface, pr *PullRequest, m PullRequestMergeMethod, body *string) error { - mergeMethod := githubv4.PullRequestMergeMethodMerge - switch m { - case PullRequestMergeMethodRebase: - mergeMethod = githubv4.PullRequestMergeMethodRebase - case PullRequestMergeMethodSquash: - mergeMethod = githubv4.PullRequestMergeMethodSquash - } - - var mutation struct { - MergePullRequest struct { - PullRequest struct { - ID githubv4.ID - } - } `graphql:"mergePullRequest(input: $input)"` - } - - input := githubv4.MergePullRequestInput{ - PullRequestID: pr.ID, - MergeMethod: &mergeMethod, - } - - if m == PullRequestMergeMethodSquash { - commitHeadline := githubv4.String(fmt.Sprintf("%s (#%d)", pr.Title, pr.Number)) - input.CommitHeadline = &commitHeadline - } - if body != nil { - commitBody := githubv4.String(*body) - input.CommitBody = &commitBody - } - - variables := map[string]interface{}{ - "input": input, - } - - gql := graphQLClient(client.http, repo.RepoHost()) - err := gql.MutateNamed(context.Background(), "PullRequestMerge", &mutation, variables) - - return err -} - func PullRequestReady(client *Client, repo ghrepo.Interface, pr *PullRequest) error { var mutation struct { MarkPullRequestReadyForReview struct { diff --git a/pkg/cmd/pr/merge/http.go b/pkg/cmd/pr/merge/http.go new file mode 100644 index 000000000..7261be806 --- /dev/null +++ b/pkg/cmd/pr/merge/http.go @@ -0,0 +1,100 @@ +package merge + +import ( + "context" + "net/http" + + "github.com/cli/cli/internal/ghinstance" + "github.com/cli/cli/internal/ghrepo" + "github.com/shurcooL/githubv4" + "github.com/shurcooL/graphql" +) + +type PullRequestMergeMethod int + +const ( + PullRequestMergeMethodMerge PullRequestMergeMethod = iota + PullRequestMergeMethodRebase + PullRequestMergeMethodSquash +) + +type mergePayload struct { + repo ghrepo.Interface + pullRequestID string + method PullRequestMergeMethod + auto bool + commitSubject string + setCommitSubject bool + commitBody string + setCommitBody bool +} + +// TODO: drop after githubv4 gets updated +type EnablePullRequestAutoMergeInput struct { + githubv4.MergePullRequestInput +} + +func mergePullRequest(client *http.Client, payload mergePayload) error { + input := githubv4.MergePullRequestInput{ + PullRequestID: githubv4.ID(payload.pullRequestID), + } + + switch payload.method { + case PullRequestMergeMethodMerge: + m := githubv4.PullRequestMergeMethodMerge + input.MergeMethod = &m + case PullRequestMergeMethodRebase: + m := githubv4.PullRequestMergeMethodRebase + input.MergeMethod = &m + case PullRequestMergeMethodSquash: + m := githubv4.PullRequestMergeMethodSquash + input.MergeMethod = &m + } + + if payload.setCommitSubject { + commitHeadline := githubv4.String(payload.commitSubject) + input.CommitHeadline = &commitHeadline + } + if payload.setCommitBody { + commitBody := githubv4.String(payload.commitBody) + input.CommitBody = &commitBody + } + + variables := map[string]interface{}{ + "input": input, + } + + gql := graphql.NewClient(ghinstance.GraphQLEndpoint(payload.repo.RepoHost()), client) + + if payload.auto { + var mutation struct { + AnablePullRequestAutoMerge struct { + ClientMutationId string + } `graphql:"enablePullRequestAutoMerge(input: $input)"` + } + variables["input"] = EnablePullRequestAutoMergeInput{input} + return gql.MutateNamed(context.Background(), "PullRequestAutoMerge", &mutation, variables) + } + + var mutation struct { + MergePullRequest struct { + ClientMutationId string + } `graphql:"mergePullRequest(input: $input)"` + } + return gql.MutateNamed(context.Background(), "PullRequestMerge", &mutation, variables) +} + +func disableAutoMerge(client *http.Client, repo ghrepo.Interface, prID string) error { + var mutation struct { + DisablePullRequestAutoMerge struct { + ClientMutationId string + } `graphql:"disablePullRequestAutoMerge(input: {pullRequestId: $prID})"` + } + + variables := map[string]interface{}{ + "prID": githubv4.ID(prID), + } + + gql := graphql.NewClient(ghinstance.GraphQLEndpoint(repo.RepoHost()), client) + return gql.MutateNamed(context.Background(), "PullRequestAutoMergeDisable", &mutation, variables) +} diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 9003765d2..37953060e 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -30,7 +30,10 @@ type MergeOptions struct { SelectorArg string DeleteBranch bool - MergeMethod api.PullRequestMergeMethod + MergeMethod PullRequestMergeMethod + + AutoMerge bool + AutoMergeDisable bool Body string @@ -75,15 +78,15 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm methodFlags := 0 if flagMerge { - opts.MergeMethod = api.PullRequestMergeMethodMerge + opts.MergeMethod = PullRequestMergeMethodMerge methodFlags++ } if flagRebase { - opts.MergeMethod = api.PullRequestMergeMethodRebase + opts.MergeMethod = PullRequestMergeMethodRebase methodFlags++ } if flagSquash { - opts.MergeMethod = api.PullRequestMergeMethodSquash + opts.MergeMethod = PullRequestMergeMethodSquash methodFlags++ } if methodFlags == 0 { @@ -110,6 +113,8 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm cmd.Flags().BoolVarP(&flagMerge, "merge", "m", false, "Merge the commits with the base branch") cmd.Flags().BoolVarP(&flagRebase, "rebase", "r", false, "Rebase the commits onto the base branch") cmd.Flags().BoolVarP(&flagSquash, "squash", "s", false, "Squash the commits into one commit and merge it into the base branch") + cmd.Flags().BoolVar(&opts.AutoMerge, "auto", false, "Automatically merge only after necessary requirements are met") + cmd.Flags().BoolVar(&opts.AutoMergeDisable, "disable-auto", false, "Disable auto-merge for this pull request") return cmd } @@ -127,6 +132,19 @@ func mergeRun(opts *MergeOptions) error { return err } + isTerminal := opts.IO.IsStdoutTTY() + + if opts.AutoMergeDisable { + err := disableAutoMerge(httpClient, baseRepo, pr.ID) + if err != nil { + return err + } + if isTerminal { + fmt.Fprintf(opts.IO.ErrOut, "%s Auto-merge disabled for pull request #%d\n", cs.SuccessIconWithColor(cs.Green), pr.Number) + } + return nil + } + if opts.SelectorArg == "" { localBranchLastCommit, err := git.LastCommit() if err == nil { @@ -144,18 +162,26 @@ func mergeRun(opts *MergeOptions) error { deleteBranch := opts.DeleteBranch crossRepoPR := pr.HeadRepositoryOwner.Login != baseRepo.RepoOwner() - isTerminal := opts.IO.IsStdoutTTY() isPRAlreadyMerged := pr.State == "MERGED" if !isPRAlreadyMerged { - mergeMethod := opts.MergeMethod + payload := mergePayload{ + repo: baseRepo, + pullRequestID: pr.ID, + method: opts.MergeMethod, + auto: opts.AutoMerge, + commitBody: opts.Body, + } + if opts.Body != "" { + payload.setCommitBody = true + } if opts.InteractiveMode { r, err := api.GitHubRepo(apiClient, baseRepo) if err != nil { return err } - mergeMethod, err = mergeMethodSurvey(r) + payload.method, err = mergeMethodSurvey(r) if err != nil { return err } @@ -164,7 +190,7 @@ func mergeRun(opts *MergeOptions) error { return err } - allowEditMsg := mergeMethod != api.PullRequestMergeMethodRebase + allowEditMsg := payload.method != PullRequestMergeMethodRebase action, err := confirmSurvey(allowEditMsg) if err != nil { @@ -178,19 +204,19 @@ func mergeRun(opts *MergeOptions) error { return err } - if opts.Body == "" { - if mergeMethod == api.PullRequestMergeMethodMerge { - opts.Body = pr.Title + if payload.commitBody == "" { + if payload.method == PullRequestMergeMethodMerge { + payload.commitBody = pr.Title } else { - opts.Body = pr.ViewerMergeBodyText + payload.commitBody = pr.ViewerMergeBodyText } } - msg, err := commitMsgSurvey(opts.Body, editorCommand) + payload.commitBody, err = commitMsgSurvey(payload.commitBody, editorCommand) if err != nil { return err } - opts.Body = msg + payload.setCommitBody = true action, err = confirmSurvey(false) if err != nil { @@ -203,27 +229,38 @@ func mergeRun(opts *MergeOptions) error { } } - var body *string - if opts.Body != "" { - body = &opts.Body + if !payload.setCommitSubject && payload.method == PullRequestMergeMethodSquash { + payload.commitSubject = fmt.Sprintf("%s (#%d)", pr.Title, pr.Number) + payload.setCommitSubject = true } - err = api.PullRequestMerge(apiClient, baseRepo, pr, mergeMethod, body) + err = mergePullRequest(httpClient, payload) if err != nil { return err } if isTerminal { - action := "Merged" - switch mergeMethod { - case api.PullRequestMergeMethodRebase: - action = "Rebased and merged" - case api.PullRequestMergeMethodSquash: - action = "Squashed and merged" + if payload.auto { + method := "" + switch payload.method { + case PullRequestMergeMethodRebase: + method = " via rebase" + case PullRequestMergeMethodSquash: + method = " via squash" + } + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d will automatically get merged%s when all requiremenets are met\n", cs.SuccessIconWithColor(cs.Green), pr.Number, method) + } else { + action := "Merged" + switch payload.method { + case PullRequestMergeMethodRebase: + action = "Rebased and merged" + case PullRequestMergeMethodSquash: + action = "Squashed and merged" + } + fmt.Fprintf(opts.IO.ErrOut, "%s %s pull request #%d (%s)\n", cs.SuccessIconWithColor(cs.Magenta), action, pr.Number, pr.Title) } - fmt.Fprintf(opts.IO.ErrOut, "%s %s pull request #%d (%s)\n", cs.SuccessIconWithColor(cs.Magenta), action, pr.Number, pr.Title) } - } else if !opts.IsDeleteBranchIndicated && opts.InteractiveMode && !crossRepoPR { + } else if !opts.IsDeleteBranchIndicated && opts.InteractiveMode && !crossRepoPR && !opts.AutoMerge { err := prompt.SurveyAskOne(&survey.Confirm{ Message: fmt.Sprintf("Pull request #%d was already merged. Delete the branch locally?", pr.Number), Default: false, @@ -235,7 +272,7 @@ func mergeRun(opts *MergeOptions) error { fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d was already merged\n", cs.WarningIcon(), pr.Number) } - if !deleteBranch || crossRepoPR { + if !deleteBranch || crossRepoPR || opts.AutoMerge { return nil } @@ -290,23 +327,23 @@ func mergeRun(opts *MergeOptions) error { return nil } -func mergeMethodSurvey(baseRepo *api.Repository) (api.PullRequestMergeMethod, error) { +func mergeMethodSurvey(baseRepo *api.Repository) (PullRequestMergeMethod, error) { type mergeOption struct { title string - method api.PullRequestMergeMethod + method PullRequestMergeMethod } var mergeOpts []mergeOption if baseRepo.MergeCommitAllowed { - opt := mergeOption{title: "Create a merge commit", method: api.PullRequestMergeMethodMerge} + opt := mergeOption{title: "Create a merge commit", method: PullRequestMergeMethodMerge} mergeOpts = append(mergeOpts, opt) } if baseRepo.RebaseMergeAllowed { - opt := mergeOption{title: "Rebase and merge", method: api.PullRequestMergeMethodRebase} + opt := mergeOption{title: "Rebase and merge", method: PullRequestMergeMethodRebase} mergeOpts = append(mergeOpts, opt) } if baseRepo.SquashMergeAllowed { - opt := mergeOption{title: "Squash and merge", method: api.PullRequestMergeMethodSquash} + opt := mergeOption{title: "Squash and merge", method: PullRequestMergeMethodSquash} mergeOpts = append(mergeOpts, opt) } @@ -318,7 +355,6 @@ func mergeMethodSurvey(baseRepo *api.Repository) (api.PullRequestMergeMethod, er mergeQuestion := &survey.Select{ Message: "What merge method would you like to use?", Options: surveyOpts, - Default: "Create a merge commit", } var result int diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 94c75c2c2..6b3165e61 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -42,7 +42,7 @@ func Test_NewCmdMerge(t *testing.T) { DeleteBranch: false, IsDeleteBranchIndicated: false, CanDeleteLocalBranch: true, - MergeMethod: api.PullRequestMergeMethodMerge, + MergeMethod: PullRequestMergeMethodMerge, InteractiveMode: true, }, }, @@ -55,7 +55,7 @@ func Test_NewCmdMerge(t *testing.T) { DeleteBranch: false, IsDeleteBranchIndicated: true, CanDeleteLocalBranch: true, - MergeMethod: api.PullRequestMergeMethodMerge, + MergeMethod: PullRequestMergeMethodMerge, InteractiveMode: true, }, }, @@ -68,7 +68,7 @@ func Test_NewCmdMerge(t *testing.T) { DeleteBranch: false, IsDeleteBranchIndicated: false, CanDeleteLocalBranch: true, - MergeMethod: api.PullRequestMergeMethodMerge, + MergeMethod: PullRequestMergeMethodMerge, InteractiveMode: true, Body: "cool", }, @@ -787,5 +787,5 @@ func Test_mergeMethodSurvey(t *testing.T) { as.StubOne(0) // Select first option which is rebase merge method, err := mergeMethodSurvey(repo) assert.Nil(t, err) - assert.Equal(t, api.PullRequestMergeMethodRebase, method) + assert.Equal(t, PullRequestMergeMethodRebase, method) } From 57abe45b96c253f6bc921b86ef756dcbbce1298c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 16 Feb 2021 16:17:37 +0100 Subject: [PATCH 05/10] Let the server choose the commit subject for squashed merge For single-commit PRs, the commit subject will be the subject of the head commit and the PR number. For multi-commit PRs, the commit subject will be the PR title and PR number. Instead of trying to replicate this logic client-side, omit the `commitHeadline` param and let the server apply defaults appropriately. Reverts https://github.com/cli/cli/pull/1627 --- pkg/cmd/pr/merge/merge.go | 5 ----- pkg/cmd/pr/merge/merge_test.go | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 37953060e..7ab2ee076 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -229,11 +229,6 @@ func mergeRun(opts *MergeOptions) error { } } - if !payload.setCommitSubject && payload.method == PullRequestMergeMethodSquash { - payload.commitSubject = fmt.Sprintf("%s (#%d)", pr.Title, pr.Number) - payload.setCommitSubject = true - } - err = mergePullRequest(httpClient, payload) if err != nil { return err diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 6b3165e61..2f71eda32 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -500,7 +500,7 @@ func TestPrMerge_squash(t *testing.T) { httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { assert.Equal(t, "THE-ID", input["pullRequestId"].(string)) assert.Equal(t, "SQUASH", input["mergeMethod"].(string)) - assert.Equal(t, "The title of the PR (#3)", input["commitHeadline"].(string)) + assert.NotContains(t, input, "commitHeadline") })) _, cmdTeardown := run.Stub() From 3b650a8c566269097f76ebd199dfcb43d1ce1158 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 16 Feb 2021 16:28:23 +0100 Subject: [PATCH 06/10] Fix typo --- pkg/cmd/pr/merge/merge.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 7ab2ee076..63d83eb62 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -243,7 +243,7 @@ func mergeRun(opts *MergeOptions) error { case PullRequestMergeMethodSquash: method = " via squash" } - fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d will automatically get merged%s when all requiremenets are met\n", cs.SuccessIconWithColor(cs.Green), pr.Number, method) + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d will automatically get merged%s when all requirements are met\n", cs.SuccessIconWithColor(cs.Green), pr.Number, method) } else { action := "Merged" switch payload.method { From 2b36b09abf54661280f8e15873482e927418eabf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 17 Feb 2021 12:30:04 +0100 Subject: [PATCH 07/10] Update wording for auto-merge confirmation Co-authored-by: Amanda Pinsker --- pkg/cmd/pr/merge/merge.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 63d83eb62..61353c950 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -243,7 +243,7 @@ func mergeRun(opts *MergeOptions) error { case PullRequestMergeMethodSquash: method = " via squash" } - fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d will automatically get merged%s when all requirements are met\n", cs.SuccessIconWithColor(cs.Green), pr.Number, method) + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d will be automatically merged%s when all requirements are met\n", cs.SuccessIconWithColor(cs.Green), pr.Number, method) } else { action := "Merged" switch payload.method { From 12cf8ef65b68a296a8b00ecb2f7560d3dbdbf56b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 17 Feb 2021 14:06:27 +0100 Subject: [PATCH 08/10] Separately query `viewerMergeBodyText` for GHE compatibility GHE versions 2.22 and older will not have this GraphQL field. Avoid the resulting error and have the command proceeed with empty text as the default. --- api/queries_pr.go | 3 --- pkg/cmd/pr/merge/http.go | 38 ++++++++++++++++++++++++++++++++++ pkg/cmd/pr/merge/merge.go | 9 ++++---- pkg/cmd/pr/merge/merge_test.go | 6 ++++++ 4 files changed, 48 insertions(+), 8 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index b18022ddb..d20e7baf9 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -88,8 +88,6 @@ type PullRequest struct { ReactionGroups ReactionGroups Reviews PullRequestReviews ReviewRequests ReviewRequests - - ViewerMergeBodyText string } type ReviewRequests struct { @@ -578,7 +576,6 @@ func PullRequestByNumber(client *Client, repo ghrepo.Interface, number int) (*Pu milestone{ title } - viewerMergeBodyText ` + commentsFragment() + ` ` + reactionGroupsFragment() + ` } diff --git a/pkg/cmd/pr/merge/http.go b/pkg/cmd/pr/merge/http.go index 7261be806..f6c3d4c08 100644 --- a/pkg/cmd/pr/merge/http.go +++ b/pkg/cmd/pr/merge/http.go @@ -3,6 +3,7 @@ package merge import ( "context" "net/http" + "strings" "github.com/cli/cli/internal/ghinstance" "github.com/cli/cli/internal/ghrepo" @@ -98,3 +99,40 @@ func disableAutoMerge(client *http.Client, repo ghrepo.Interface, prID string) e gql := graphql.NewClient(ghinstance.GraphQLEndpoint(repo.RepoHost()), client) return gql.MutateNamed(context.Background(), "PullRequestAutoMergeDisable", &mutation, variables) } + +func getMergeText(client *http.Client, repo ghrepo.Interface, prID string, mergeMethod PullRequestMergeMethod) (string, error) { + var method githubv4.PullRequestMergeMethod + switch mergeMethod { + case PullRequestMergeMethodMerge: + method = githubv4.PullRequestMergeMethodMerge + case PullRequestMergeMethodRebase: + method = githubv4.PullRequestMergeMethodRebase + case PullRequestMergeMethodSquash: + method = githubv4.PullRequestMergeMethodSquash + } + + var query struct { + Node struct { + PullRequest struct { + ViewerMergeBodyText string `graphql:"viewerMergeBodyText(mergeType: $method)"` + } `graphql:"...on PullRequest"` + } `graphql:"node(id: $prID)"` + } + + variables := map[string]interface{}{ + "prID": githubv4.ID(prID), + "method": method, + } + + gql := graphql.NewClient(ghinstance.GraphQLEndpoint(repo.RepoHost()), client) + err := gql.QueryNamed(context.Background(), "PullRequestMergeText", &query, variables) + if err != nil { + // Tolerate this API missing in older GitHub Enterprise + if strings.Contains(err.Error(), "Field 'viewerMergeBodyText' doesn't exist") { + return "", nil + } + return "", err + } + + return query.Node.PullRequest.ViewerMergeBodyText, nil +} diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 61353c950..70509e787 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -204,11 +204,10 @@ func mergeRun(opts *MergeOptions) error { return err } - if payload.commitBody == "" { - if payload.method == PullRequestMergeMethodMerge { - payload.commitBody = pr.Title - } else { - payload.commitBody = pr.ViewerMergeBodyText + if !payload.setCommitBody { + payload.commitBody, err = getMergeText(httpClient, baseRepo, pr.ID, payload.method) + if err != nil { + return err } } diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 2f71eda32..ad289a143 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -703,6 +703,12 @@ func TestPRMerge_interactiveSquashEditCommitMsg(t *testing.T) { "rebaseMergeAllowed": true, "squashMergeAllowed": true } } }`)) + http.Register( + httpmock.GraphQL(`query PullRequestMergeText\b`), + httpmock.StringResponse(` + { "data": { "node": { + "viewerMergeBodyText": "" + } } }`)) http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { From ddddd95d7373f7e05c918907a56793b2938e1932 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 17 Feb 2021 14:38:33 +0100 Subject: [PATCH 09/10] Allow `pr merge --body ''` to prevent having the default body applied --- pkg/cmd/pr/merge/merge.go | 10 +++++----- pkg/cmd/pr/merge/merge_test.go | 6 ++++++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 70509e787..e204ce6cd 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -35,7 +35,8 @@ type MergeOptions struct { AutoMerge bool AutoMergeDisable bool - Body string + Body string + BodySet bool IsDeleteBranchIndicated bool CanDeleteLocalBranch bool @@ -100,6 +101,7 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm opts.IsDeleteBranchIndicated = cmd.Flags().Changed("delete-branch") opts.CanDeleteLocalBranch = !cmd.Flags().Changed("repo") + opts.BodySet = cmd.Flags().Changed("body") if runF != nil { return runF(opts) @@ -109,7 +111,7 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm } cmd.Flags().BoolVarP(&opts.DeleteBranch, "delete-branch", "d", false, "Delete the local and remote branch after merge") - cmd.Flags().StringVarP(&opts.Body, "body", "b", "", "Body for merge commit") + cmd.Flags().StringVarP(&opts.Body, "body", "b", "", "Body `text` for the merge commit") cmd.Flags().BoolVarP(&flagMerge, "merge", "m", false, "Merge the commits with the base branch") cmd.Flags().BoolVarP(&flagRebase, "rebase", "r", false, "Rebase the commits onto the base branch") cmd.Flags().BoolVarP(&flagSquash, "squash", "s", false, "Squash the commits into one commit and merge it into the base branch") @@ -171,9 +173,7 @@ func mergeRun(opts *MergeOptions) error { method: opts.MergeMethod, auto: opts.AutoMerge, commitBody: opts.Body, - } - if opts.Body != "" { - payload.setCommitBody = true + setCommitBody: opts.BodySet, } if opts.InteractiveMode { diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index ad289a143..38d66f909 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -44,6 +44,8 @@ func Test_NewCmdMerge(t *testing.T) { CanDeleteLocalBranch: true, MergeMethod: PullRequestMergeMethodMerge, InteractiveMode: true, + Body: "", + BodySet: false, }, }, { @@ -57,6 +59,8 @@ func Test_NewCmdMerge(t *testing.T) { CanDeleteLocalBranch: true, MergeMethod: PullRequestMergeMethodMerge, InteractiveMode: true, + Body: "", + BodySet: false, }, }, { @@ -71,6 +75,7 @@ func Test_NewCmdMerge(t *testing.T) { MergeMethod: PullRequestMergeMethodMerge, InteractiveMode: true, Body: "cool", + BodySet: true, }, }, { @@ -138,6 +143,7 @@ func Test_NewCmdMerge(t *testing.T) { assert.Equal(t, tt.want.MergeMethod, opts.MergeMethod) assert.Equal(t, tt.want.InteractiveMode, opts.InteractiveMode) assert.Equal(t, tt.want.Body, opts.Body) + assert.Equal(t, tt.want.BodySet, opts.BodySet) }) } } From 203397baf91b3a08523fd0643d27eb03b668780d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 17 Feb 2021 15:24:52 +0100 Subject: [PATCH 10/10] Add tests for `pr merge --auto/--disable-auto` --- pkg/cmd/pr/merge/http.go | 2 +- pkg/cmd/pr/merge/merge.go | 10 ++-- pkg/cmd/pr/merge/merge_test.go | 84 ++++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/pr/merge/http.go b/pkg/cmd/pr/merge/http.go index f6c3d4c08..ad1a9a78a 100644 --- a/pkg/cmd/pr/merge/http.go +++ b/pkg/cmd/pr/merge/http.go @@ -69,7 +69,7 @@ func mergePullRequest(client *http.Client, payload mergePayload) error { if payload.auto { var mutation struct { - AnablePullRequestAutoMerge struct { + EnablePullRequestAutoMerge struct { ClientMutationId string } `graphql:"enablePullRequestAutoMerge(input: $input)"` } diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index e204ce6cd..f2923573c 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -32,7 +32,7 @@ type MergeOptions struct { DeleteBranch bool MergeMethod PullRequestMergeMethod - AutoMerge bool + AutoMergeEnable bool AutoMergeDisable bool Body string @@ -115,7 +115,7 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm cmd.Flags().BoolVarP(&flagMerge, "merge", "m", false, "Merge the commits with the base branch") cmd.Flags().BoolVarP(&flagRebase, "rebase", "r", false, "Rebase the commits onto the base branch") cmd.Flags().BoolVarP(&flagSquash, "squash", "s", false, "Squash the commits into one commit and merge it into the base branch") - cmd.Flags().BoolVar(&opts.AutoMerge, "auto", false, "Automatically merge only after necessary requirements are met") + cmd.Flags().BoolVar(&opts.AutoMergeEnable, "auto", false, "Automatically merge only after necessary requirements are met") cmd.Flags().BoolVar(&opts.AutoMergeDisable, "disable-auto", false, "Disable auto-merge for this pull request") return cmd } @@ -171,7 +171,7 @@ func mergeRun(opts *MergeOptions) error { repo: baseRepo, pullRequestID: pr.ID, method: opts.MergeMethod, - auto: opts.AutoMerge, + auto: opts.AutoMergeEnable, commitBody: opts.Body, setCommitBody: opts.BodySet, } @@ -254,7 +254,7 @@ func mergeRun(opts *MergeOptions) error { fmt.Fprintf(opts.IO.ErrOut, "%s %s pull request #%d (%s)\n", cs.SuccessIconWithColor(cs.Magenta), action, pr.Number, pr.Title) } } - } else if !opts.IsDeleteBranchIndicated && opts.InteractiveMode && !crossRepoPR && !opts.AutoMerge { + } else if !opts.IsDeleteBranchIndicated && opts.InteractiveMode && !crossRepoPR && !opts.AutoMergeEnable { err := prompt.SurveyAskOne(&survey.Confirm{ Message: fmt.Sprintf("Pull request #%d was already merged. Delete the branch locally?", pr.Number), Default: false, @@ -266,7 +266,7 @@ func mergeRun(opts *MergeOptions) error { fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d was already merged\n", cs.WarningIcon(), pr.Number) } - if !deleteBranch || crossRepoPR || opts.AutoMerge { + if !deleteBranch || crossRepoPR || opts.AutoMergeEnable { return nil } diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 38d66f909..90e8d69c2 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -801,3 +801,87 @@ func Test_mergeMethodSurvey(t *testing.T) { assert.Nil(t, err) assert.Equal(t, PullRequestMergeMethodRebase, method) } + +func TestMergeRun_autoMerge(t *testing.T) { + io, _, stdout, stderr := iostreams.Test() + io.SetStdoutTTY(true) + io.SetStderrTTY(true) + + tr := initFakeHTTP() + defer tr.Verify(t) + + tr.Register( + httpmock.GraphQL(`query PullRequestByNumber\b`), + httpmock.StringResponse(` + { "data": { "repository": { "pullRequest": { + "id": "THE-ID", + "number": 123, + "title": "The title of the PR", + "state": "OPEN", + "headRefName": "blueberries", + "headRepositoryOwner": {"login": "OWNER"} + } } } }`)) + tr.Register( + httpmock.GraphQL(`mutation PullRequestAutoMerge\b`), + httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { + assert.Equal(t, "THE-ID", input["pullRequestId"].(string)) + assert.Equal(t, "SQUASH", input["mergeMethod"].(string)) + })) + + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + err := mergeRun(&MergeOptions{ + IO: io, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: tr}, nil + }, + SelectorArg: "https://github.com/OWNER/REPO/pull/123", + AutoMergeEnable: true, + MergeMethod: PullRequestMergeMethodSquash, + }) + assert.NoError(t, err) + + assert.Equal(t, "", stdout.String()) + assert.Equal(t, "✓ Pull request #123 will be automatically merged via squash when all requirements are met\n", stderr.String()) +} + +func TestMergeRun_disableAutoMerge(t *testing.T) { + io, _, stdout, stderr := iostreams.Test() + io.SetStdoutTTY(true) + io.SetStderrTTY(true) + + tr := initFakeHTTP() + defer tr.Verify(t) + + tr.Register( + httpmock.GraphQL(`query PullRequestByNumber\b`), + httpmock.StringResponse(` + { "data": { "repository": { "pullRequest": { + "id": "THE-ID", + "number": 123, + "title": "The title of the PR", + "state": "OPEN", + "headRefName": "blueberries", + "headRepositoryOwner": {"login": "OWNER"} + } } } }`)) + tr.Register( + httpmock.GraphQL(`mutation PullRequestAutoMergeDisable\b`), + httpmock.StringResponse(`{}`)) + + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + err := mergeRun(&MergeOptions{ + IO: io, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: tr}, nil + }, + SelectorArg: "https://github.com/OWNER/REPO/pull/123", + AutoMergeDisable: true, + }) + assert.NoError(t, err) + + assert.Equal(t, "", stdout.String()) + assert.Equal(t, "✓ Auto-merge disabled for pull request #123\n", stderr.String()) +}