Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions pkg/cmd/pr/merge/merge.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@ import (
"errors"
"fmt"
"net/http"
"strings"
"time"

"github.com/MakeNowJust/heredoc"
"github.com/cenkalti/backoff/v4"
"github.com/cli/cli/v2/api"
ghContext "github.com/cli/cli/v2/context"
"github.com/cli/cli/v2/git"
Expand Down Expand Up @@ -57,6 +60,9 @@ type MergeOptions struct {
// ErrAlreadyInMergeQueue indicates that the pull request is already in a merge queue
var ErrAlreadyInMergeQueue = errors.New("already in merge queue")

// Allow injecting backoff interval in tests.
var mergeRetryInterval = 2 * time.Second

func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Command {
opts := &MergeOptions{
IO: f.IOStreams,
Expand Down Expand Up @@ -345,8 +351,14 @@ func (m *mergeContext) merge() error {
}
}

err := mergePullRequest(m.httpClient, payload)
if err != nil {
bo := backoff.NewConstantBackOff(mergeRetryInterval)
if err := backoff.Retry(func() error {
err := mergePullRequest(m.httpClient, payload)
if err == nil || shouldRetry(err) {
return err
}
return backoff.Permanent(err)
}, backoff.WithMaxRetries(bo, 3)); err != nil {
return err
}

Expand Down Expand Up @@ -760,6 +772,21 @@ func mergeConflictStatus(status string) bool {
return status == MergeStateStatusDirty
}

// The "Base branch was modified" error occurs when the base
// branch is updated by another merge and the mergeability
// computation hasn't completed yet.
func shouldRetry(err error) bool {
var graphErr api.GraphQLError
if errors.As(err, &graphErr) {
for _, e := range graphErr.Errors {
if strings.Contains(e.Message, "Base branch was modified.") {
return true
}
}
}
return false
}

func isImmediatelyMergeable(status string) bool {
switch status {
case MergeStateStatusClean, MergeStateStatusHasHooks, MergeStateStatusUnstable:
Expand Down
55 changes: 55 additions & 0 deletions pkg/cmd/pr/merge/merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2030,6 +2030,61 @@ func TestPrAddToMergeQueueAdminWithMergeStrategy(t *testing.T) {
assert.Equal(t, "✓ Merged pull request OWNER/REPO#1 (The title of the PR)\n", output.Stderr())
}

func TestPrMerge_retryOnBaseBranchModified(t *testing.T) {
orig := mergeRetryInterval
mergeRetryInterval = 0
t.Cleanup(func() { mergeRetryInterval = orig })

http := initFakeHTTP()
defer http.Verify(t)

shared.StubFinderForRunCommandStyleTests(t,
"1",
&api.PullRequest{
ID: "THE-ID",
Number: 1,
State: "OPEN",
Title: "The title of the PR",
MergeStateStatus: "CLEAN",
},
baseRepo("OWNER", "REPO", "main"),
)

http.Register(
httpmock.GraphQL(`mutation PullRequestMerge\b`),
httpmock.StringResponse(`
{ "data": {},
"errors": [
{
"message": "Base branch was modified. Review and try the merge again."
}
]
}`),
)
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))
}),
)

cs, cmdTeardown := run.Stub()
defer cmdTeardown(t)
cs.Register(`git rev-parse --verify refs/heads/`, 0, "")

output, err := runCommand(http, nil, "main", true, "pr merge 1 --merge")
if err != nil {
t.Fatalf("error running command `pr merge`: %v", err)
}

r := regexp.MustCompile(`Merged pull request OWNER/REPO#1 \(The title of the PR\)`)

if !r.MatchString(output.Stderr()) {
t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr())
}
}

type testEditor struct{}

func (e testEditor) Edit(filename, text string) (string, error) {
Expand Down
Loading