Repository: cli/cli

Issue:
If PR is merged, make "gh pr merge" propose cleanup steps
### Currently

If a PR has already been merged through the web interface, running the command `gh pr merge` on my local branch currently returns the following:
> `! Pull request #151 (PULL REQUEST NAME) can't be merged right now; try again in a few seconds`

This seems to happen whether the origin branch has been deleted or not.

### Proposal

I would like `gh pr merge` to be able to go through the cleanup steps of deleting branches and returning to the default branch even if the target PR has already been merged.

### Additional context

I see an issue related to the error message in question has been submitted already at https://github.com/cli/cli/issues/2544 but I don't think it pertains to this exact scenario.

Hints from issue comments:
@mislav: Thank you for reporting! I've reclassified this as a bug because the error message is definitely misleading.@sindrekjr: After trying out the new release I've noticed that the error message I've described was removed in this commit: https://github.com/cli/cli/commit/f853a4b0e2d389206a9f641325ed3ca9e94fb91d

The new functionality gives the new error message `! Pull request #1 (PR_NAME) was already merged`, so as a bug this issue seems to be fixed now.

However, I'd still like to suggest the enhancement that I proposed in my initial post. :smile: 

Solution patch:
diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go
index abd761807..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)
@@ -123,107 +126,110 @@ func mergeRun(opts *MergeOptions) error {
 	if pr.Mergeable == "CONFLICTING" {
 		err := fmt.Errorf("%s Pull request #%d (%s) has conflicts and isn't mergeable ", cs.Red("!"), pr.Number, pr.Title)
 		return err
-	} else if pr.State == "MERGED" {
-		err := fmt.Errorf("%s Pull request #%d (%s) was already merged", cs.Red("!"), pr.Number, pr.Title)
-		return err
 	}
 
-	mergeMethod := opts.MergeMethod
 	deleteBranch := opts.DeleteBranch
 	crossRepoPR := pr.HeadRepositoryOwner.Login != baseRepo.RepoOwner()
+	isTerminal := opts.IO.IsStdoutTTY()
 
-	if opts.InteractiveMode {
-		mergeMethod, deleteBranch, err = prInteractiveMerge(opts.DeleteLocalBranch, crossRepoPR)
-		if err != nil {
-			if errors.Is(err, cancelError) {
-				fmt.Fprintln(opts.IO.ErrOut, "Cancelled.")
-				return cmdutil.SilentError
+	isPRAlreadyMerged := pr.State == "MERGED"
+	if !isPRAlreadyMerged {
+		mergeMethod := opts.MergeMethod
+
+		if opts.InteractiveMode {
+			mergeMethod, deleteBranch, err = prInteractiveMerge(opts, crossRepoPR)
+			if err != nil {
+				if errors.Is(err, cancelError) {
+					fmt.Fprintln(opts.IO.ErrOut, "Cancelled.")
+					return cmdutil.SilentError
+				}
+				return err
 			}
+		}
+
+		err = api.PullRequestMerge(apiClient, baseRepo, pr, mergeMethod)
+		if err != nil {
 			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 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.IsDeleteBranchIndicated && opts.InteractiveMode {
+		err := prompt.SurveyAskOne(&survey.Confirm{
+			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 err != nil {
-		return fmt.Errorf("API call failed: %w", err)
+	if !deleteBranch {
+		return nil
 	}
 
-	isTerminal := opts.IO.IsStdoutTTY()
-
-	if isTerminal {
-		fmt.Fprintf(opts.IO.ErrOut, "%s %s pull request #%d (%s)\n", cs.Magenta("✔"), action, pr.Number, pr.Title)
-	}
+	branchSwitchString := ""
 
-	if deleteBranch {
-		branchSwitchString := ""
+	if opts.CanDeleteLocalBranch && !crossRepoPR {
+		currentBranch, err := opts.Branch()
+		if err != nil {
+			return err
+		}
 
-		if opts.DeleteLocalBranch && !crossRepoPR {
-			currentBranch, err := opts.Branch()
+		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))
+			err = git.CheckoutBranch(branchToSwitchTo)
+			if err != nil {
+				return err
 			}
 		}
 
-		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", cs.Cyan(pr.HeadRefName), 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 isTerminal {
-			fmt.Fprintf(opts.IO.ErrOut, "%s Deleted branch %s%s\n", cs.Red("✔"), cs.Cyan(pr.HeadRefName), branchSwitchString)
+		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
 }
 
 var cancelError = errors.New("cancelError")
 
-func prInteractiveMerge(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{
@@ -235,9 +241,9 @@ func prInteractiveMerge(deleteLocalBranch bool, crossRepoPR bool) (api.PullReque
 
 	qs := []*survey.Question{mergeMethodQuestion}
 
-	if !crossRepoPR {
+	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?"
@@ -265,7 +271,9 @@ func prInteractiveMerge(deleteLocalBranch bool, crossRepoPR bool) (api.PullReque
 		MergeMethod  int
 		DeleteBranch bool
 		IsConfirmed  bool
-	}{}
+	}{
+		DeleteBranch: opts.DeleteBranch,
+	}
 
 	err := prompt.SurveyAsk(qs, &answers)
 	if err != nil {
@@ -285,6 +293,5 @@ func prInteractiveMerge(deleteLocalBranch bool, crossRepoPR bool) (api.PullReque
 		mergeMethod = api.PullRequestMergeMethodSquash
 	}
 
-	deleteBranch := answers.DeleteBranch
-	return mergeMethod, deleteBranch, nil
+	return mergeMethod, answers.DeleteBranch, nil
 }



Test patch:
diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go
index 6cd0e19dc..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)
 		})
@@ -485,27 +500,58 @@ func TestPrMerge_alreadyMerged(t *testing.T) {
 		httpmock.GraphQL(`query PullRequestByNumber\b`),
 		httpmock.StringResponse(`
 		{ "data": { "repository": {
-			"pullRequest": { "number": 4, "title": "The title of the PR", "state": "MERGED"}
+			"pullRequest": {
+				"number": 4,
+				"title": "The title of the PR",
+				"state": "MERGED",
+				"baseRefName": "master",
+				"headRefName": "blueberries",
+				"headRepositoryOwner": {
+					"login": "OWNER"
+				},
+				"isCrossRepository": false
+			}
 		} } }`))
 
-	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()
+	as.StubOne(true)
 
-	output, err := runCommand(http, "master", true, "pr merge 4")
-	if err == nil {
-		t.Fatalf("expected an error running command `pr merge`: %v", err)
+	output, err := runCommand(http, "blueberries", true, "pr merge 4")
+	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`)
+	test.ExpectLines(t, output.Stderr(), "✔ Deleted branch blueberries and switched to branch master")
+}
 
-	if !r.MatchString(err.Error()) {
-		t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr())
+func TestPrMerge_alreadyMerged_nonInteractive(t *testing.T) {
+	http := initFakeHTTP()
+	defer http.Verify(t)
+	http.Register(
+		httpmock.GraphQL(`query PullRequestByNumber\b`),
+		httpmock.StringResponse(`
+		{ "data": { "repository": {
+			"pullRequest": { "number": 4, "title": "The title of the PR", "state": "MERGED"}
+		} } }`))
+
+	_, cmdTeardown := run.Stub()
+	defer cmdTeardown(t)
+
+	output, err := runCommand(http, "blueberries", true, "pr merge 4 --merge")
+	if err != nil {
+		t.Fatalf("Got unexpected error running `pr merge` %s", err)
 	}
+
+	assert.Equal(t, "", output.String())
+	assert.Equal(t, "", output.Stderr())
 }
 
 func TestPRMerge_interactive(t *testing.T) {
@@ -566,6 +612,60 @@ func TestPRMerge_interactive(t *testing.T) {
 	test.ExpectLines(t, output.Stderr(), "Merged pull request #3")
 }
 
+func TestPRMerge_interactiveWithDeleteBranch(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(`mutation PullRequestMerge\b`),
+		httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) {
+			assert.Equal(t, "THE-ID", input["pullRequestId"].(string))
+			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 := 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:  "isConfirmed",
+			Value: true,
+		},
+	})
+
+	output, err := runCommand(http, "blueberries", true, "-d")
+	if err != nil {
+		t.Fatalf("Got unexpected error running `pr merge` %s", err)
+	}
+
+	test.ExpectLines(t, output.Stderr(), "Merged pull request #3", "Deleted branch blueberries and switched to branch master")
+}
+
 func TestPRMerge_interactiveCancelled(t *testing.T) {
 	http := initFakeHTTP()
 	defer http.Verify(t)