diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index e22b6c13d..bc22fdac1 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -33,7 +33,6 @@ type MergeOptions struct { DeleteLocalBranch bool MergeMethod api.PullRequestMergeMethod InteractiveMode bool - ConfirmSubmit bool } func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Command { @@ -108,7 +107,6 @@ 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().BoolVarP(&opts.ConfirmSubmit, "confirm", "y", false, "Confirm Merging the PR (default: false)") return cmd } @@ -142,101 +140,94 @@ func mergeRun(opts *MergeOptions) error { if opts.InteractiveMode { mergeMethod, deleteBranch, err = prInteractiveMerge(opts.DeleteLocalBranch, crossRepoPR) if err != nil { - return nil - } - } - - shouldSubmitFromFlag := opts.ConfirmSubmit - if !shouldSubmitFromFlag { - fmt.Println(opts.ConfirmSubmit) - err := prompt.Confirm("Submit? ", &shouldSubmitFromFlag) - if err != nil { + if errors.Is(err, cancelError) { + fmt.Fprintln(opts.IO.ErrOut, "Cancelled.") + return cmdutil.SilentError + } return err } } - if shouldSubmitFromFlag { - var action string - if mergeMethod == api.PullRequestMergeMethodRebase { - action = "Rebased and merged" - err = api.PullRequestMerge(apiClient, baseRepo, pr, api.PullRequestMergeMethodRebase) - } else if mergeMethod == api.PullRequestMergeMethodSquash { - action = "Squashed and merged" - err = api.PullRequestMerge(apiClient, baseRepo, pr, api.PullRequestMergeMethodSquash) - } else if mergeMethod == api.PullRequestMergeMethodMerge { - action = "Merged" - err = api.PullRequestMerge(apiClient, baseRepo, pr, api.PullRequestMergeMethodMerge) - } else { - err = fmt.Errorf("unknown merge method (%d) used", mergeMethod) - return err - } + var action string + if mergeMethod == api.PullRequestMergeMethodRebase { + action = "Rebased and merged" + err = api.PullRequestMerge(apiClient, baseRepo, pr, api.PullRequestMergeMethodRebase) + } else if mergeMethod == api.PullRequestMergeMethodSquash { + action = "Squashed and merged" + err = api.PullRequestMerge(apiClient, baseRepo, pr, api.PullRequestMergeMethodSquash) + } else if mergeMethod == api.PullRequestMergeMethodMerge { + action = "Merged" + err = api.PullRequestMerge(apiClient, baseRepo, pr, api.PullRequestMergeMethodMerge) + } else { + err = fmt.Errorf("unknown merge method (%d) used", mergeMethod) + return err + } - if err != nil { - return fmt.Errorf("API call failed: %w", err) - } + if err != nil { + return fmt.Errorf("API call failed: %w", err) + } - isTerminal := opts.IO.IsStdoutTTY() + isTerminal := opts.IO.IsStdoutTTY() - if isTerminal { - fmt.Fprintf(opts.IO.ErrOut, "%s %s pull request #%d (%s)\n", utils.Magenta("✔"), action, pr.Number, pr.Title) - } + if isTerminal { + fmt.Fprintf(opts.IO.ErrOut, "%s %s pull request #%d (%s)\n", utils.Magenta("✔"), action, pr.Number, pr.Title) + } - if deleteBranch { - branchSwitchString := "" + if deleteBranch { + branchSwitchString := "" - if opts.DeleteLocalBranch && !crossRepoPR { - currentBranch, err := opts.Branch() + if opts.DeleteLocalBranch && !crossRepoPR { + currentBranch, err := opts.Branch() + if err != nil { + return err + } + + var branchToSwitchTo string + if currentBranch == pr.HeadRefName { + branchToSwitchTo, err = api.RepoDefaultBranch(apiClient, baseRepo) if err != nil { return err } - - var branchToSwitchTo string - if currentBranch == pr.HeadRefName { - branchToSwitchTo, err = api.RepoDefaultBranch(apiClient, baseRepo) - if err != nil { - return err - } - err = git.CheckoutBranch(branchToSwitchTo) - if err != nil { - return err - } - } - - localBranchExists := git.HasLocalBranch(pr.HeadRefName) - if localBranchExists { - err = git.DeleteLocalBranch(pr.HeadRefName) - if err != nil { - err = fmt.Errorf("failed to delete local branch %s: %w", utils.Cyan(pr.HeadRefName), err) - return err - } - } - - if branchToSwitchTo != "" { - branchSwitchString = fmt.Sprintf(" and switched to branch %s", utils.Cyan(branchToSwitchTo)) - } - } - - if !crossRepoPR { - err = api.BranchDeleteRemote(apiClient, baseRepo, pr.HeadRefName) - var httpErr api.HTTPError - // The ref might have already been deleted by GitHub - if err != nil && (!errors.As(err, &httpErr) || httpErr.StatusCode != 422) { - err = fmt.Errorf("failed to delete remote branch %s: %w", utils.Cyan(pr.HeadRefName), err) + err = git.CheckoutBranch(branchToSwitchTo) + if err != nil { return err } } - if isTerminal { - fmt.Fprintf(opts.IO.ErrOut, "%s Deleted branch %s%s\n", utils.Red("✔"), utils.Cyan(pr.HeadRefName), branchSwitchString) + localBranchExists := git.HasLocalBranch(pr.HeadRefName) + if localBranchExists { + err = git.DeleteLocalBranch(pr.HeadRefName) + if err != nil { + err = fmt.Errorf("failed to delete local branch %s: %w", utils.Cyan(pr.HeadRefName), err) + return err + } + } + + if branchToSwitchTo != "" { + branchSwitchString = fmt.Sprintf(" and switched to branch %s", utils.Cyan(branchToSwitchTo)) } } - } else { - fmt.Println("Discarding") + + if !crossRepoPR { + err = api.BranchDeleteRemote(apiClient, baseRepo, pr.HeadRefName) + var httpErr api.HTTPError + // The ref might have already been deleted by GitHub + if err != nil && (!errors.As(err, &httpErr) || httpErr.StatusCode != 422) { + err = fmt.Errorf("failed to delete remote branch %s: %w", utils.Cyan(pr.HeadRefName), err) + return err + } + } + + if isTerminal { + fmt.Fprintf(opts.IO.ErrOut, "%s Deleted branch %s%s\n", utils.Red("✔"), utils.Cyan(pr.HeadRefName), branchSwitchString) + } } return nil } +var cancelError = errors.New("cancelError") + func prInteractiveMerge(deleteLocalBranch bool, crossRepoPR bool) (api.PullRequestMergeMethod, bool, error) { mergeMethodQuestion := &survey.Question{ Name: "mergeMethod", @@ -267,15 +258,27 @@ func prInteractiveMerge(deleteLocalBranch bool, crossRepoPR bool) (api.PullReque qs = append(qs, deleteBranchQuestion) } + qs = append(qs, &survey.Question{ + Name: "isConfirmed", + Prompt: &survey.Confirm{ + Message: "Submit?", + Default: false, + }, + }) + answers := struct { MergeMethod int DeleteBranch bool + IsConfirmed bool }{} err := prompt.SurveyAsk(qs, &answers) if err != nil { return 0, false, fmt.Errorf("could not prompt: %w", err) } + if !answers.IsConfirmed { + return 0, false, cancelError + } var mergeMethod api.PullRequestMergeMethod switch answers.MergeMethod { diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 5b272bff1..2815617d2 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -2,6 +2,7 @@ package merge import ( "bytes" + "errors" "io/ioutil" "net/http" "regexp" @@ -198,8 +199,6 @@ func TestPrMerge(t *testing.T) { cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() - prompt.StubConfirm(true) - cs.Stub("branch.blueberries.remote origin\nbranch.blueberries.merge refs/heads/blueberries") // git config --get-regexp ^branch\.master\.(remote|merge) cs.Stub("") // git config --get-regexp ^branch\.blueberries\.(remote|merge)$ cs.Stub("") // git symbolic-ref --quiet --short HEAD @@ -246,8 +245,6 @@ func TestPrMerge_nontty(t *testing.T) { cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() - prompt.StubConfirm(true) - cs.Stub("branch.blueberries.remote origin\nbranch.blueberries.merge refs/heads/blueberries") // git config --get-regexp ^branch\.master\.(remote|merge) cs.Stub("") // git config --get-regexp ^branch\.blueberries\.(remote|merge)$ cs.Stub("") // git symbolic-ref --quiet --short HEAD @@ -291,8 +288,6 @@ func TestPrMerge_withRepoFlag(t *testing.T) { cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() - prompt.StubConfirm(true) - output, err := runCommand(http, "master", true, "pr merge 1 --merge -R OWNER/REPO") if err != nil { t.Fatalf("error running command `pr merge`: %v", err) @@ -328,8 +323,6 @@ func TestPrMerge_deleteBranch(t *testing.T) { cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() - prompt.StubConfirm(true) - cs.Stub("") // git config --get-regexp ^branch\.blueberries\.(remote|merge)$ cs.Stub("") // git checkout master cs.Stub("") // git rev-parse --verify blueberries` @@ -365,8 +358,6 @@ func TestPrMerge_deleteNonCurrentBranch(t *testing.T) { cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() - prompt.StubConfirm(true) - // We don't expect the default branch to be checked out, just that blueberries is deleted cs.Stub("") // git rev-parse --verify blueberries cs.Stub("") // git branch -d blueberries @@ -401,8 +392,6 @@ func TestPrMerge_noPrNumberGiven(t *testing.T) { cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() - prompt.StubConfirm(true) - cs.Stub("branch.blueberries.remote origin\nbranch.blueberries.merge refs/heads/blueberries") // git config --get-regexp ^branch\.master\.(remote|merge) cs.Stub("") // git config --get-regexp ^branch\.blueberries\.(remote|merge)$ cs.Stub("") // git symbolic-ref --quiet --short HEAD @@ -449,8 +438,6 @@ func TestPrMerge_rebase(t *testing.T) { cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() - prompt.StubConfirm(true) - cs.Stub("") // git config --get-regexp ^branch\.blueberries\.(remote|merge)$ cs.Stub("") // git symbolic-ref --quiet --short HEAD cs.Stub("") // git checkout master @@ -496,8 +483,6 @@ func TestPrMerge_squash(t *testing.T) { cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() - prompt.StubConfirm(true) - cs.Stub("") // git config --get-regexp ^branch\.blueberries\.(remote|merge)$ cs.Stub("") // git symbolic-ref --quiet --short HEAD cs.Stub("") // git checkout master @@ -524,8 +509,6 @@ func TestPrMerge_alreadyMerged(t *testing.T) { cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() - prompt.StubConfirm(true) - cs.Stub("") // git config --get-regexp ^branch\.blueberries\.(remote|merge)$ cs.Stub("") // git symbolic-ref --quiet --short HEAD cs.Stub("") // git checkout master @@ -587,10 +570,12 @@ func TestPRMerge_interactive(t *testing.T) { Name: "deleteBranch", Value: true, }, + { + Name: "isConfirmed", + Value: true, + }, }) - prompt.StubConfirm(true) - output, err := runCommand(http, "blueberries", true, "") if err != nil { t.Fatalf("Got unexpected error running `pr merge` %s", err) @@ -598,3 +583,51 @@ func TestPRMerge_interactive(t *testing.T) { test.ExpectLines(t, output.Stderr(), "Merged pull request #3", `Deleted branch.*blueberries`) } + +func TestPRMerge_interactiveCancelled(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 + }] } } } }`)) + + cs, cmdTeardown := test.InitCmdStubber() + defer cmdTeardown() + + cs.Stub("") // git config --get-regexp ^branch\.blueberries\.(remote|merge)$ + cs.Stub("") // git symbolic-ref --quiet --short HEAD + cs.Stub("") // git checkout master + cs.Stub("") // git push origin --delete blueberries + cs.Stub("") // git branch -d + + as, surveyTeardown := prompt.InitAskStubber() + defer surveyTeardown() + + as.Stub([]*prompt.QuestionStub{ + { + Name: "mergeMethod", + Value: 0, + }, + { + Name: "deleteBranch", + Value: true, + }, + { + Name: "isConfirmed", + Value: false, + }, + }) + + output, err := runCommand(http, "blueberries", true, "") + if !errors.Is(err, cmdutil.SilentError) { + t.Fatalf("got error %v", err) + } + + assert.Equal(t, "Cancelled.\n", output.Stderr()) +}