From 66546e2245108f117f262b49b172eba1d63328e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 18 Jan 2021 18:10:20 +0100 Subject: [PATCH] When `pr merge --delete-branch` flag is supplied, avoid prompting for it --- pkg/cmd/pr/merge/merge.go | 151 +++++++++++++++------------------ pkg/cmd/pr/merge/merge_test.go | 58 +++++++------ 2 files changed, 102 insertions(+), 107 deletions(-) diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 5c447b365..4cac24242 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -27,11 +27,13 @@ type MergeOptions struct { Remotes func() (context.Remotes, error) Branch func() (string, error) - SelectorArg string - DeleteBranch bool - DeleteLocalBranch bool - MergeMethod api.PullRequestMergeMethod - InteractiveMode bool + SelectorArg string + DeleteBranch bool + MergeMethod api.PullRequestMergeMethod + + IsDeleteBranchIndicated bool + CanDeleteLocalBranch bool + InteractiveMode bool } func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Command { @@ -90,7 +92,8 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm return &cmdutil.FlagError{Err: errors.New("only one of --merge, --rebase, or --squash can be enabled")} } - opts.DeleteLocalBranch = !cmd.Flags().Changed("repo") + opts.IsDeleteBranchIndicated = cmd.Flags().Changed("delete-branch") + opts.CanDeleteLocalBranch = !cmd.Flags().Changed("repo") if runF != nil { return runF(opts) @@ -125,22 +128,16 @@ func mergeRun(opts *MergeOptions) error { return err } - isPRAlreadyMerged := pr.State == "MERGED" - - if isPRAlreadyMerged && !opts.InteractiveMode && !opts.DeleteBranch { - err := fmt.Errorf("%s Pull request #%d (%s) was already merged", cs.Red("!"), pr.Number, pr.Title) - return err - } - deleteBranch := opts.DeleteBranch crossRepoPR := pr.HeadRepositoryOwner.Login != baseRepo.RepoOwner() isTerminal := opts.IO.IsStdoutTTY() + isPRAlreadyMerged := pr.State == "MERGED" if !isPRAlreadyMerged { mergeMethod := opts.MergeMethod if opts.InteractiveMode { - mergeMethod, deleteBranch, err = prInteractiveMerge(opts.DeleteBranch, opts.DeleteLocalBranch, crossRepoPR) + mergeMethod, deleteBranch, err = prInteractiveMerge(opts, crossRepoPR) if err != nil { if errors.Is(err, cancelError) { fmt.Fprintln(opts.IO.ErrOut, "Cancelled.") @@ -150,87 +147,81 @@ func mergeRun(opts *MergeOptions) error { } } - 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) + err = api.PullRequestMerge(apiClient, baseRepo, pr, mergeMethod) + if err != nil { return err } - if err != nil { - return fmt.Errorf("API call failed: %w", err) - } - if isTerminal { + action := "Merged" + switch mergeMethod { + case api.PullRequestMergeMethodRebase: + action = "Rebased and merged" + case api.PullRequestMergeMethodSquash: + action = "Squashed and merged" + } fmt.Fprintf(opts.IO.ErrOut, "%s %s pull request #%d (%s)\n", cs.Magenta("✔"), action, pr.Number, pr.Title) } - } else if !opts.DeleteBranch { + } else if !opts.IsDeleteBranchIndicated && opts.InteractiveMode { err := prompt.SurveyAskOne(&survey.Confirm{ - Message: fmt.Sprintf("Pull request #%d (%s) was already merged. Delete the branch locally and switch to default branch?", pr.Number, pr.Title), + Message: fmt.Sprintf("Pull request #%d was already merged. Delete the branch locally?", pr.Number), Default: false, }, &deleteBranch) - if err != nil { return fmt.Errorf("could not prompt: %w", err) } } - if deleteBranch { - branchSwitchString := "" + if !deleteBranch { + return nil + } - if opts.DeleteLocalBranch && !crossRepoPR { - currentBranch, err := opts.Branch() + branchSwitchString := "" + + if opts.CanDeleteLocalBranch && !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", cs.Cyan(pr.HeadRefName), err) - return err - } - } - - if branchToSwitchTo != "" { - branchSwitchString = fmt.Sprintf(" and switched to branch %s", cs.Cyan(branchToSwitchTo)) - } - } - - if !isPRAlreadyMerged && !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", cs.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", cs.Red("✔"), cs.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", cs.Cyan(pr.HeadRefName), err) + return err + } } + + if branchToSwitchTo != "" { + branchSwitchString = fmt.Sprintf(" and switched to branch %s", cs.Cyan(branchToSwitchTo)) + } + } + + if !isPRAlreadyMerged && !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", cs.Cyan(pr.HeadRefName), err) + return err + } + } + + if isTerminal { + fmt.Fprintf(opts.IO.ErrOut, "%s Deleted branch %s%s\n", cs.Red("✔"), cs.Cyan(pr.HeadRefName), branchSwitchString) } return nil @@ -238,7 +229,7 @@ func mergeRun(opts *MergeOptions) error { var cancelError = errors.New("cancelError") -func prInteractiveMerge(deleteBranch bool, deleteLocalBranch bool, crossRepoPR bool) (api.PullRequestMergeMethod, bool, error) { +func prInteractiveMerge(opts *MergeOptions, crossRepoPR bool) (api.PullRequestMergeMethod, bool, error) { mergeMethodQuestion := &survey.Question{ Name: "mergeMethod", Prompt: &survey.Select{ @@ -250,9 +241,9 @@ func prInteractiveMerge(deleteBranch bool, deleteLocalBranch bool, crossRepoPR b qs := []*survey.Question{mergeMethodQuestion} - if !crossRepoPR && !deleteBranch { + if !crossRepoPR && !opts.IsDeleteBranchIndicated { var message string - if deleteLocalBranch { + if opts.CanDeleteLocalBranch { message = "Delete the branch locally and on GitHub?" } else { message = "Delete the branch on GitHub?" @@ -280,7 +271,9 @@ func prInteractiveMerge(deleteBranch bool, deleteLocalBranch bool, crossRepoPR b MergeMethod int DeleteBranch bool IsConfirmed bool - }{} + }{ + DeleteBranch: opts.DeleteBranch, + } err := prompt.SurveyAsk(qs, &answers) if err != nil { @@ -300,9 +293,5 @@ func prInteractiveMerge(deleteBranch bool, deleteLocalBranch bool, crossRepoPR b mergeMethod = api.PullRequestMergeMethodSquash } - if !deleteBranch { - deleteBranch = answers.DeleteBranch - } - - return mergeMethod, deleteBranch, nil + return mergeMethod, answers.DeleteBranch, nil } diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 4f9e19ed3..963fe13fe 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -14,6 +14,7 @@ import ( "github.com/cli/cli/git" "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/internal/run" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/pkg/iostreams" @@ -37,11 +38,25 @@ func Test_NewCmdMerge(t *testing.T) { args: "123", isTTY: true, want: MergeOptions{ - SelectorArg: "123", - DeleteBranch: false, - DeleteLocalBranch: true, - MergeMethod: api.PullRequestMergeMethodMerge, - InteractiveMode: true, + SelectorArg: "123", + DeleteBranch: false, + IsDeleteBranchIndicated: false, + CanDeleteLocalBranch: true, + MergeMethod: api.PullRequestMergeMethodMerge, + InteractiveMode: true, + }, + }, + { + name: "delete-branch specified", + args: "--delete-branch=false", + isTTY: true, + want: MergeOptions{ + SelectorArg: "", + DeleteBranch: false, + IsDeleteBranchIndicated: true, + CanDeleteLocalBranch: true, + MergeMethod: api.PullRequestMergeMethodMerge, + InteractiveMode: true, }, }, { @@ -105,7 +120,7 @@ func Test_NewCmdMerge(t *testing.T) { assert.Equal(t, tt.want.SelectorArg, opts.SelectorArg) assert.Equal(t, tt.want.DeleteBranch, opts.DeleteBranch) - assert.Equal(t, tt.want.DeleteLocalBranch, opts.DeleteLocalBranch) + assert.Equal(t, tt.want.CanDeleteLocalBranch, opts.CanDeleteLocalBranch) assert.Equal(t, tt.want.MergeMethod, opts.MergeMethod) assert.Equal(t, tt.want.InteractiveMode, opts.InteractiveMode) }) @@ -498,13 +513,12 @@ func TestPrMerge_alreadyMerged(t *testing.T) { } } } }`)) - cs, cmdTeardown := test.InitCmdStubber() - defer cmdTeardown() + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) - 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 branch -d + 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() @@ -528,24 +542,16 @@ func TestPrMerge_alreadyMerged_nonInteractive(t *testing.T) { "pullRequest": { "number": 4, "title": "The title of the PR", "state": "MERGED"} } } }`)) - 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 branch -d + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) output, err := runCommand(http, "blueberries", true, "pr merge 4 --merge") - if err == nil { - t.Fatalf("expected an error running command `pr merge`: %v", err) + if err != nil { + t.Fatalf("Got unexpected error running `pr merge` %s", err) } - r := regexp.MustCompile(`Pull request #4 \(The title of the PR\) was already merged`) - - if !r.MatchString(err.Error()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) - } + assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) } func TestPRMerge_interactive(t *testing.T) {