diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 422b5e93e07..2049b85ae95 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -460,10 +460,22 @@ func (m *mergeContext) deleteRemoteBranch() error { if !m.merged { apiClient := api.NewClientFromHTTP(m.httpClient) err := api.BranchDeleteRemote(apiClient, m.baseRepo, m.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) { - return fmt.Errorf("failed to delete remote branch %s: %w", m.cs.Cyan(m.pr.HeadRefName), err) + if err != nil { + // Normally, the API returns 422, with the message "Reference does not exist" + // when the branch has already been deleted. It also returns 404 with the same + // message, but that rarely happens. In both cases, we should not return an + // error because the goal is already achieved. + + var isAlreadyDeletedError bool + if httpErr := (api.HTTPError{}); errors.As(err, &httpErr) { + // TODO: since the API returns 422 for a couple of other reasons, for more accuracy + // we might want to check the error message against "Reference does not exist". + isAlreadyDeletedError = httpErr.StatusCode == http.StatusUnprocessableEntity || httpErr.StatusCode == http.StatusNotFound + } + + if !isAlreadyDeletedError { + return fmt.Errorf("failed to delete remote branch %s: %w", m.cs.Cyan(m.pr.HeadRefName), err) + } } } diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 4ca8c5d06df..03ddafa61ae 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -24,6 +24,7 @@ import ( "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/test" + ghapi "github.com/cli/go-gh/v2/pkg/api" "github.com/google/shlex" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -659,6 +660,103 @@ func TestPrMerge_deleteBranch(t *testing.T) { `), output.Stderr()) } +func TestPrMerge_deleteBranch_apiError(t *testing.T) { + tests := []struct { + name string + apiError ghapi.HTTPError + wantErr string + wantStderr string + }{ + { + name: "branch already deleted (422: Reference does not exist)", + apiError: ghapi.HTTPError{ + Message: "Reference does not exist", + StatusCode: http.StatusUnprocessableEntity, // 422 + }, + wantStderr: heredoc.Doc(` + ✓ Merged pull request OWNER/REPO#10 (Blueberries are a good fruit) + ✓ Deleted local branch blueberries and switched to branch main + ✓ Deleted remote branch blueberries + `), + }, + { + name: "branch already deleted (404: Reference does not exist) (#11187)", + apiError: ghapi.HTTPError{ + Message: "Reference does not exist", + StatusCode: http.StatusNotFound, // 404 + }, + wantStderr: heredoc.Doc(` + ✓ Merged pull request OWNER/REPO#10 (Blueberries are a good fruit) + ✓ Deleted local branch blueberries and switched to branch main + ✓ Deleted remote branch blueberries + `), + }, + { + name: "unknown API error", + apiError: ghapi.HTTPError{ + Message: "blah blah", + StatusCode: http.StatusInternalServerError, // 500 + }, + wantStderr: heredoc.Doc(` + ✓ Merged pull request OWNER/REPO#10 (Blueberries are a good fruit) + ✓ Deleted local branch blueberries and switched to branch main + `), + wantErr: "failed to delete remote branch blueberries: HTTP 500: blah blah (https://api.github.com/repos/OWNER/REPO/git/refs/heads/blueberries)", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + shared.StubFinderForRunCommandStyleTests(t, + "", + &api.PullRequest{ + ID: "PR_10", + Number: 10, + State: "OPEN", + Title: "Blueberries are a good fruit", + HeadRefName: "blueberries", + BaseRefName: "main", + MergeStateStatus: "CLEAN", + }, + baseRepo("OWNER", "REPO", "main"), + ) + + http.Register( + httpmock.GraphQL(`mutation PullRequestMerge\b`), + httpmock.GraphQLMutation(`{}`, func(input map[string]interface{}) { + assert.Equal(t, "PR_10", 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.JSONErrorResponse(tt.apiError.StatusCode, tt.apiError)) + + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + cs.Register(`git rev-parse --verify refs/heads/main`, 0, "") + cs.Register(`git checkout main`, 0, "") + cs.Register(`git rev-parse --verify refs/heads/blueberries`, 0, "") + cs.Register(`git branch -D blueberries`, 0, "") + cs.Register(`git pull --ff-only`, 0, "") + + output, err := runCommand(http, nil, "blueberries", true, `pr merge --merge --delete-branch`) + assert.Equal(t, "", output.String()) + assert.Equal(t, tt.wantStderr, output.Stderr()) + + if tt.wantErr != "" { + assert.EqualError(t, err, tt.wantErr) + return + } + assert.NoError(t, err) + }) + } +} + func TestPrMerge_deleteBranch_mergeQueue(t *testing.T) { http := initFakeHTTP() defer http.Verify(t)