diff --git a/pkg/cmd/pr/merge/http.go b/pkg/cmd/pr/merge/http.go index 217290d9d..30276903b 100644 --- a/pkg/cmd/pr/merge/http.go +++ b/pkg/cmd/pr/merge/http.go @@ -19,6 +19,16 @@ const ( PullRequestMergeMethodSquash ) +const ( + MergeStateStatusBehind = "BEHIND" + MergeStateStatusBlocked = "BLOCKED" + MergeStateStatusClean = "CLEAN" + MergeStateStatusDirty = "DIRTY" + MergeStateStatusHasHooks = "HAS_HOOKS" + MergeStateStatusMerged = "MERGED" + MergeStateStatusUnstable = "UNSTABLE" +) + type mergePayload struct { repo ghrepo.Interface pullRequestID string diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 92f94d521..30e2c609e 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -168,224 +168,326 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm return cmd } -func mergeRun(opts *MergeOptions) error { - cs := opts.IO.ColorScheme() +// mergeContext contains state and dependencies to merge a pull request. +type mergeContext struct { + pr *api.PullRequest + baseRepo ghrepo.Interface + apiClient *api.Client + httpClient *http.Client + opts *MergeOptions + cs *iostreams.ColorScheme + isTerminal bool + merged bool + localBranchExists bool + autoMerge bool + crossRepoPR bool + deleteBranch bool + currentBranch string +} +// Attempt to disable auto merge on the pull request. +func (m *mergeContext) disableAutoMerge() error { + err := disableAutoMerge(m.httpClient, m.baseRepo, m.pr.ID) + if err != nil { + return err + } else if m.isTerminal { + m.log(fmt.Sprintf("%s Auto-merge disabled for pull request #%d\n", m.cs.SuccessIconWithColor(m.cs.Green), m.pr.Number)) + } + return nil +} + +// Warn if the pull request and the remote branch have diverged. +func (m *mergeContext) warnIfDiverged() { + if m.opts.SelectorArg == "" && len(m.pr.Commits.Nodes) > 0 { + if localBranchLastCommit, err := git.LastCommit(); err == nil { + if localBranchLastCommit.Sha != m.pr.Commits.Nodes[len(m.pr.Commits.Nodes)-1].Commit.OID { + m.log(fmt.Sprintf("%s Pull request #%d (%s) has diverged from local branch\n", m.cs.Yellow("!"), m.pr.Number, m.pr.Title)) + } + } + } +} + +// Check if the current state of the pull request allows for merging +func (m *mergeContext) canMerge() error { + reason := blockedReason(m.pr.MergeStateStatus, m.opts.UseAdmin) + + if reason == "" || m.autoMerge || m.merged { + return nil + } + + m.log(fmt.Sprintf("%s Pull request #%d is not mergeable: %s.\n", m.cs.FailureIcon(), m.pr.Number, reason)) + m.log("To have the pull request merged after all the requirements have been met, add the `--auto` flag.\n") + if remote := remoteForMergeConflictResolution(m.baseRepo, m.pr, m.opts); remote != nil { + mergeOrRebase := "merge" + if m.opts.MergeMethod == PullRequestMergeMethodRebase { + mergeOrRebase = "rebase" + } + fetchBranch := fmt.Sprintf("%s %s", remote.Name, m.pr.BaseRefName) + mergeBranch := fmt.Sprintf("%s %s/%s", mergeOrRebase, remote.Name, m.pr.BaseRefName) + cmd := fmt.Sprintf("gh pr checkout %d && git fetch %s && git %s", m.pr.Number, fetchBranch, mergeBranch) + m.log(fmt.Sprintf("Run the following to resolve the merge conflicts locally:\n %s\n", m.cs.Bold(cmd))) + } + if !m.opts.UseAdmin && allowsAdminOverride(m.pr.MergeStateStatus) { + // TODO: show this flag only to repo admins + m.log("To use administrator privileges to immediately merge the pull request, add the `--admin` flag.\n") + } + return cmdutil.SilentError +} + +// Merge the pull request. May prompt the user for input parameters for the merge. +func (m *mergeContext) merge() error { + if m.merged { + return nil + } + + payload := mergePayload{ + repo: m.baseRepo, + pullRequestID: m.pr.ID, + method: m.opts.MergeMethod, + auto: m.autoMerge, + commitSubject: m.opts.Subject, + commitBody: m.opts.Body, + setCommitBody: m.opts.BodySet, + } + + // get user input if not already given + if m.opts.InteractiveMode { + r, err := api.GitHubRepo(m.apiClient, m.baseRepo) + if err != nil { + return err + } + + payload.method, err = mergeMethodSurvey(r) + if err != nil { + return err + } + + m.deleteBranch, err = deleteBranchSurvey(m.opts, m.crossRepoPR, m.localBranchExists) + if err != nil { + return err + } + + allowEditMsg := payload.method != PullRequestMergeMethodRebase + for { + action, err := confirmSurvey(allowEditMsg) + if err != nil { + return fmt.Errorf("unable to confirm: %w", err) + } + + submit, err := confirmSubmission(m.httpClient, m.opts, action, &payload) + if err != nil { + return err + } + if submit { + break + } + } + } + + err := mergePullRequest(m.httpClient, payload) + if err != nil { + return err + } + + // log successful merge + if m.isTerminal { + if payload.auto { + method := "" + switch payload.method { + case PullRequestMergeMethodRebase: + method = " via rebase" + case PullRequestMergeMethodSquash: + method = " via squash" + } + m.log(fmt.Sprintf("%s Pull request #%d will be automatically merged%s when all requirements are met\n", m.cs.SuccessIconWithColor(m.cs.Green), m.pr.Number, method)) + } else { + action := "Merged" + switch payload.method { + case PullRequestMergeMethodRebase: + action = "Rebased and merged" + case PullRequestMergeMethodSquash: + action = "Squashed and merged" + } + m.log(fmt.Sprintf("%s %s pull request #%d (%s)\n", m.cs.SuccessIconWithColor(m.cs.Magenta), action, m.pr.Number, m.pr.Title)) + } + } + + return nil +} + +// Delete local branch if requested and if allowed. +func (m *mergeContext) deleteLocalBranch() error { + if m.crossRepoPR || m.autoMerge { + return nil + } + + if m.merged { + // prompt for delete + if m.opts.InteractiveMode && !m.opts.IsDeleteBranchIndicated { + err := prompt.SurveyAskOne(&survey.Confirm{ + Message: fmt.Sprintf("Pull request #%d was already merged. Delete the branch locally?", m.pr.Number), + Default: false, + }, &m.deleteBranch) + if err != nil { + return fmt.Errorf("could not prompt: %w", err) + } + } else { + m.log(fmt.Sprintf("%s Pull request #%d was already merged\n", m.cs.WarningIcon(), m.pr.Number)) + } + } + + // the user does not want to delete the branch + if !m.deleteBranch { + return nil + } + + if m.opts.CanDeleteLocalBranch && m.localBranchExists { + currentBranch, err := m.opts.Branch() + if err != nil { + return err + } + + // branch the command was run on is the same as the pull request branch + if currentBranch == m.pr.HeadRefName { + // if the target branch of the PR is not known, set the current branch to the + // default branch of the repository + m.currentBranch = m.pr.BaseRefName + if m.currentBranch == "" { + m.currentBranch, err = api.RepoDefaultBranch(m.apiClient, m.baseRepo) + if err != nil { + return err + } + } + + remotes, err := m.opts.Remotes() + if err != nil { + return err + } + + baseRemote, err := remotes.FindByRepo(m.baseRepo.RepoOwner(), m.baseRepo.RepoName()) + if err != nil { + return err + } + + if git.HasLocalBranch(m.currentBranch) { + if err := git.CheckoutBranch(m.currentBranch); err != nil { + return err + } + } else { + if err := git.CheckoutNewBranch(baseRemote.Name, m.currentBranch); err != nil { + return err + } + } + + if err := git.Pull(baseRemote.Name, m.currentBranch); err != nil { + m.log(fmt.Sprintf("%s warning: not possible to fast-forward to: %q\n", m.cs.WarningIcon(), m.currentBranch)) + } + } + + if err := git.DeleteLocalBranch(m.pr.HeadRefName); err != nil { + err = fmt.Errorf("failed to delete local branch %s: %w", m.cs.Cyan(m.pr.HeadRefName), err) + return err + } + } + + return nil +} + +// Delete the remote branch if requested and if allowed. +func (m *mergeContext) deleteRemoteBranch() error { + // the user was already asked if they want to delete the branch if they didn't provide the flag + if !m.deleteBranch || m.crossRepoPR || m.autoMerge { + return nil + } + + if !m.merged { + err := api.BranchDeleteRemote(m.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 m.isTerminal { + branch := "" + if m.currentBranch != "" { + branch = fmt.Sprintf(" and switched to branch %s", m.cs.Cyan(m.currentBranch)) + } + m.log(fmt.Sprintf("%s Deleted branch %s%s\n", m.cs.SuccessIconWithColor(m.cs.Red), m.cs.Cyan(m.pr.HeadRefName), branch)) + } + + return nil +} + +// Log operations and results to the error stream. +func (m *mergeContext) log(s string) { + fmt.Fprint(m.opts.IO.ErrOut, s) +} + +// Creates a new MergeConext from MergeOptions. +func NewMergeContext(opts *MergeOptions) (*mergeContext, error) { findOptions := shared.FindOptions{ Selector: opts.SelectorArg, Fields: []string{"id", "number", "state", "title", "lastCommit", "mergeStateStatus", "headRepositoryOwner", "headRefName"}, } pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { - return err + return nil, err } isTerminal := opts.IO.IsStdoutTTY() httpClient, err := opts.HttpClient() if err != nil { - return err + return nil, err } apiClient := api.NewClientFromHTTP(httpClient) + return &mergeContext{ + opts: opts, + pr: pr, + cs: opts.IO.ColorScheme(), + baseRepo: baseRepo, + isTerminal: isTerminal, + apiClient: apiClient, + httpClient: httpClient, + merged: pr.State == MergeStateStatusMerged, + deleteBranch: opts.DeleteBranch, + crossRepoPR: pr.HeadRepositoryOwner.Login != baseRepo.RepoOwner(), + autoMerge: opts.AutoMergeEnable && !isImmediatelyMergeable(pr.MergeStateStatus), + localBranchExists: opts.CanDeleteLocalBranch && git.HasLocalBranch(pr.HeadRefName), + }, nil +} + +// Run the merge command. +func mergeRun(opts *MergeOptions) error { + ctx, err := NewMergeContext(opts) + if err != nil { + return err + } + + // no further action is possible when disabling auto merge if opts.AutoMergeDisable { - err := disableAutoMerge(httpClient, baseRepo, pr.ID) - if err != nil { - return err - } - if isTerminal { - fmt.Fprintf(opts.IO.ErrOut, "%s Auto-merge disabled for pull request #%d\n", cs.SuccessIconWithColor(cs.Green), pr.Number) - } - return nil + return ctx.disableAutoMerge() } - if opts.SelectorArg == "" && len(pr.Commits.Nodes) > 0 { - if localBranchLastCommit, err := git.LastCommit(); err == nil { - if localBranchLastCommit.Sha != pr.Commits.Nodes[len(pr.Commits.Nodes)-1].Commit.OID { - fmt.Fprintf(opts.IO.ErrOut, - "%s Pull request #%d (%s) has diverged from local branch\n", cs.Yellow("!"), pr.Number, pr.Title) - } - } + ctx.warnIfDiverged() + + if err := ctx.canMerge(); err != nil { + return err } - isPRAlreadyMerged := pr.State == "MERGED" - if reason := blockedReason(pr.MergeStateStatus, opts.UseAdmin); !opts.AutoMergeEnable && !isPRAlreadyMerged && reason != "" { - fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d is not mergeable: %s.\n", cs.FailureIcon(), pr.Number, reason) - fmt.Fprintf(opts.IO.ErrOut, "To have the pull request merged after all the requirements have been met, add the `--auto` flag.\n") - if remote := remoteForMergeConflictResolution(baseRepo, pr, opts); remote != nil { - mergeOrRebase := "merge" - if opts.MergeMethod == PullRequestMergeMethodRebase { - mergeOrRebase = "rebase" - } - fetchBranch := fmt.Sprintf("%s %s", remote.Name, pr.BaseRefName) - mergeBranch := fmt.Sprintf("%s %s/%s", mergeOrRebase, remote.Name, pr.BaseRefName) - cmd := fmt.Sprintf("gh pr checkout %d && git fetch %s && git %s", pr.Number, fetchBranch, mergeBranch) - fmt.Fprintf(opts.IO.ErrOut, "Run the following to resolve the merge conflicts locally:\n %s\n", cs.Bold(cmd)) - } - if !opts.UseAdmin && allowsAdminOverride(pr.MergeStateStatus) { - // TODO: show this flag only to repo admins - fmt.Fprintf(opts.IO.ErrOut, "To use administrator privileges to immediately merge the pull request, add the `--admin` flag.\n") - } - return cmdutil.SilentError + if err := ctx.merge(); err != nil { + return err } - deleteBranch := opts.DeleteBranch - crossRepoPR := pr.HeadRepositoryOwner.Login != baseRepo.RepoOwner() - autoMerge := opts.AutoMergeEnable && !isImmediatelyMergeable(pr.MergeStateStatus) - localBranchExists := false - if opts.CanDeleteLocalBranch { - localBranchExists = git.HasLocalBranch(pr.HeadRefName) + if err := ctx.deleteLocalBranch(); err != nil { + return err } - if !isPRAlreadyMerged { - payload := mergePayload{ - repo: baseRepo, - pullRequestID: pr.ID, - method: opts.MergeMethod, - auto: autoMerge, - commitSubject: opts.Subject, - commitBody: opts.Body, - setCommitBody: opts.BodySet, - } - - if opts.InteractiveMode { - r, err := api.GitHubRepo(apiClient, baseRepo) - if err != nil { - return err - } - payload.method, err = mergeMethodSurvey(r) - if err != nil { - return err - } - deleteBranch, err = deleteBranchSurvey(opts, crossRepoPR, localBranchExists) - if err != nil { - return err - } - - allowEditMsg := payload.method != PullRequestMergeMethodRebase - - for { - action, err := confirmSurvey(allowEditMsg) - if err != nil { - return fmt.Errorf("unable to confirm: %w", err) - } - - submit, err := confirmSubmission(httpClient, opts, action, &payload) - if err != nil { - return err - } - if submit { - break - } - } - } - - err = mergePullRequest(httpClient, payload) - if err != nil { - return err - } - - if isTerminal { - if payload.auto { - method := "" - switch payload.method { - case PullRequestMergeMethodRebase: - method = " via rebase" - case PullRequestMergeMethodSquash: - method = " via squash" - } - fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d will be automatically merged%s when all requirements are met\n", cs.SuccessIconWithColor(cs.Green), pr.Number, method) - } else { - action := "Merged" - switch payload.method { - case PullRequestMergeMethodRebase: - action = "Rebased and merged" - case PullRequestMergeMethodSquash: - action = "Squashed and merged" - } - fmt.Fprintf(opts.IO.ErrOut, "%s %s pull request #%d (%s)\n", cs.SuccessIconWithColor(cs.Magenta), action, pr.Number, pr.Title) - } - } - } else if !opts.IsDeleteBranchIndicated && opts.InteractiveMode && !crossRepoPR && !opts.AutoMergeEnable { - 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) - } - } else { - fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d was already merged\n", cs.WarningIcon(), pr.Number) - } - - if !deleteBranch || crossRepoPR || autoMerge { - return nil - } - - branchSwitchString := "" - - if opts.CanDeleteLocalBranch && localBranchExists { - currentBranch, err := opts.Branch() - if err != nil { - return err - } - - var branchToSwitchTo string - if currentBranch == pr.HeadRefName { - branchToSwitchTo = pr.BaseRefName - if branchToSwitchTo == "" { - branchToSwitchTo, err = api.RepoDefaultBranch(apiClient, baseRepo) - if err != nil { - return err - } - } - - remotes, err := opts.Remotes() - if err != nil { - return err - } - - baseRemote, err := remotes.FindByRepo(baseRepo.RepoOwner(), baseRepo.RepoName()) - if err != nil { - return err - } - - if git.HasLocalBranch(branchToSwitchTo) { - if err := git.CheckoutBranch(branchToSwitchTo); err != nil { - return err - } - } else { - if err := git.CheckoutNewBranch(baseRemote.Name, branchToSwitchTo); err != nil { - return err - } - } - - if err := git.Pull(baseRemote.Name, branchToSwitchTo); err != nil { - fmt.Fprintf(opts.IO.ErrOut, "%s warning: not possible to fast-forward to: %q\n", cs.WarningIcon(), branchToSwitchTo) - } - } - - if err := git.DeleteLocalBranch(pr.HeadRefName); 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 { - 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.SuccessIconWithColor(cs.Red), cs.Cyan(pr.HeadRefName), branchSwitchString) + if err := ctx.deleteRemoteBranch(); err != nil { + return err } return nil @@ -547,17 +649,17 @@ func (e *userEditor) Edit(filename, startingText string) (string, error) { // blockedReason translates various MergeStateStatus GraphQL values into human-readable reason func blockedReason(status string, useAdmin bool) string { switch status { - case "BLOCKED": + case MergeStateStatusBlocked: if useAdmin { return "" } return "the base branch policy prohibits the merge" - case "BEHIND": + case MergeStateStatusBehind: if useAdmin { return "" } return "the head branch is not up to date with the base branch" - case "DIRTY": + case MergeStateStatusDirty: return "the merge commit cannot be cleanly created" default: return "" @@ -566,7 +668,7 @@ func blockedReason(status string, useAdmin bool) string { func allowsAdminOverride(status string) bool { switch status { - case "BLOCKED", "BEHIND": + case MergeStateStatusBlocked, MergeStateStatusBehind: return true default: return false @@ -589,12 +691,12 @@ func remoteForMergeConflictResolution(baseRepo ghrepo.Interface, pr *api.PullReq } func mergeConflictStatus(status string) bool { - return status == "DIRTY" + return status == MergeStateStatusDirty } func isImmediatelyMergeable(status string) bool { switch status { - case "CLEAN", "HAS_HOOKS", "UNSTABLE": + case MergeStateStatusClean, MergeStateStatusHasHooks, MergeStateStatusUnstable: return true default: return false diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 742cfd184..1c352c7b7 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -319,8 +319,9 @@ func TestPrMerge_blocked(t *testing.T) { baseRepo("OWNER", "REPO", "master"), ) - _, cmdTeardown := run.Stub() + cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git rev-parse --verify refs/heads/`, 0, "") output, err := runCommand(http, "master", true, "pr merge 1 --merge") assert.EqualError(t, err, "SilentError") @@ -351,8 +352,9 @@ func TestPrMerge_dirty(t *testing.T) { baseRepo("OWNER", "REPO", "master"), ) - _, cmdTeardown := run.Stub() + cs, cmdTeardown := run.Stub() defer cmdTeardown(t) + cs.Register(`git rev-parse --verify refs/heads/`, 0, "") output, err := runCommand(http, "master", true, "pr merge 1 --merge") assert.EqualError(t, err, "SilentError") @@ -926,7 +928,7 @@ func TestPrMerge_alreadyMerged_nonInteractive(t *testing.T) { ID: "THE-ID", Number: 4, State: "MERGED", - HeadRepositoryOwner: api.Owner{Login: "monalisa"}, + HeadRepositoryOwner: api.Owner{Login: "OWNER"}, MergeStateStatus: "CLEAN", }, baseRepo("OWNER", "REPO", "master"), @@ -946,6 +948,35 @@ func TestPrMerge_alreadyMerged_nonInteractive(t *testing.T) { assert.Equal(t, "! Pull request #4 was already merged\n", output.Stderr()) } +func TestPrMerge_alreadyMerged_nonInteractive_crossRepo(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + shared.RunCommandFinder( + "4", + &api.PullRequest{ + ID: "THE-ID", + Number: 4, + State: "MERGED", + HeadRepositoryOwner: api.Owner{Login: "monalisa"}, + MergeStateStatus: "CLEAN", + }, + baseRepo("OWNER", "REPO", "master"), + ) + + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + cs.Register(`git rev-parse --verify refs/heads/`, 0, "") + + 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) { http := initFakeHTTP() defer http.Verify(t)