diff --git a/api/queries_pr.go b/api/queries_pr.go index 6ee26176c..64925fae5 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -48,6 +48,7 @@ type PullRequest struct { BaseRefName string HeadRefName string Body string + Mergeable string Author struct { Login string @@ -264,6 +265,7 @@ func PullRequests(client *Client, repo ghrepo.Interface, currentPRNumber int, cu state url headRefName + mergeable headRepositoryOwner { login } @@ -418,6 +420,7 @@ func PullRequestByNumber(client *Client, repo ghrepo.Interface, number int) (*Pu state closed body + mergeable author { login } @@ -526,6 +529,7 @@ func PullRequestForBranch(client *Client, repo ghrepo.Interface, baseBranch, hea title state body + mergeable author { login } diff --git a/api/queries_repo.go b/api/queries_repo.go index c8ba31348..22d42cfbb 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -84,6 +84,9 @@ func GitHubRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { hasIssuesEnabled description viewerPermission + defaultBranchRef { + name + } } }` variables := map[string]interface{}{ diff --git a/command/pr.go b/command/pr.go index 3bf36b432..6aa39ff23 100644 --- a/command/pr.go +++ b/command/pr.go @@ -1,6 +1,7 @@ package command import ( + "errors" "fmt" "io" "regexp" @@ -8,6 +9,7 @@ import ( "strconv" "strings" + "github.com/AlecAivazis/survey/v2" "github.com/cli/cli/api" "github.com/cli/cli/context" "github.com/cli/cli/git" @@ -26,7 +28,8 @@ func init() { prCmd.AddCommand(prCloseCmd) prCmd.AddCommand(prReopenCmd) prCmd.AddCommand(prMergeCmd) - prMergeCmd.Flags().BoolP("merge", "m", true, "Merge the commits with the base branch") + prMergeCmd.Flags().BoolP("delete-branch", "d", true, "Delete the local branch after merge") + prMergeCmd.Flags().BoolP("merge", "m", false, "Merge the commits with the base branch") prMergeCmd.Flags().BoolP("rebase", "r", false, "Rebase the commits onto the base branch") prMergeCmd.Flags().BoolP("squash", "s", false, "Squash the commits into one commit and merge it into the base branch") prCmd.AddCommand(prReadyCmd) @@ -451,7 +454,15 @@ func prMerge(cmd *cobra.Command, args []string) error { var pr *api.PullRequest if len(args) > 0 { - pr, err = prFromArg(apiClient, baseRepo, args[0]) + var prNumber string + n, _ := prFromURL(args[0]) + if n != "" { + prNumber = n + } else { + prNumber = args[0] + } + + pr, err = prFromArg(apiClient, baseRepo, prNumber) if err != nil { return err } @@ -471,41 +482,146 @@ func prMerge(cmd *cobra.Command, args []string) error { } } - if pr.State == "MERGED" { + if pr.Mergeable == "CONFLICTING" { + err := fmt.Errorf("%s Pull request #%d has conflicts and isn't mergeable ", utils.Red("!"), pr.Number) + return err + } else if pr.Mergeable == "UNKNOWN" { + err := fmt.Errorf("%s Pull request #%d can't be merged right now; try again in a few seconds", utils.Red("!"), pr.Number) + return err + } else if pr.State == "MERGED" { err := fmt.Errorf("%s Pull request #%d was already merged", utils.Red("!"), pr.Number) return err } - rebase, err := cmd.Flags().GetBool("rebase") - if err != nil { - return err - } - squash, err := cmd.Flags().GetBool("squash") + var mergeMethod api.PullRequestMergeMethod + deleteBranch, err := cmd.Flags().GetBool("delete-branch") if err != nil { return err } - var output string - if rebase { - output = fmt.Sprintf("%s Rebased and merged pull request #%d\n", utils.Green("✔"), pr.Number) + // Ensure only one merge method is specified + enabledFlagCount := 0 + isInteractive := false + if b, _ := cmd.Flags().GetBool("merge"); b { + enabledFlagCount++ + mergeMethod = api.PullRequestMergeMethodMerge + } + if b, _ := cmd.Flags().GetBool("rebase"); b { + enabledFlagCount++ + mergeMethod = api.PullRequestMergeMethodRebase + } + if b, _ := cmd.Flags().GetBool("squash"); b { + enabledFlagCount++ + mergeMethod = api.PullRequestMergeMethodSquash + } + + if enabledFlagCount == 0 { + isInteractive = true + } else if enabledFlagCount > 1 { + return errors.New("expected exactly one of --merge, --rebase, or --squash to be true") + } + + if isInteractive { + mergeMethod, deleteBranch, err = prInteractiveMerge() + if err != nil { + return nil + } + } + + var action string + if mergeMethod == api.PullRequestMergeMethodRebase { + action = "Rebased and merged" err = api.PullRequestMerge(apiClient, baseRepo, pr, api.PullRequestMergeMethodRebase) - } else if squash { - output = fmt.Sprintf("%s Squashed and merged pull request #%d\n", utils.Green("✔"), pr.Number) + } else if mergeMethod == api.PullRequestMergeMethodSquash { + action = "Squashed and merged" err = api.PullRequestMerge(apiClient, baseRepo, pr, api.PullRequestMergeMethodSquash) - } else { - output = fmt.Sprintf("%s Merged pull request #%d\n", utils.Green("✔"), pr.Number) + } 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 err != nil { return fmt.Errorf("API call failed: %w", err) } - fmt.Fprint(colorableOut(cmd), output) + fmt.Fprintf(colorableOut(cmd), "%s %s pull request #%d\n", utils.Magenta("✔"), action, pr.Number) + + if deleteBranch && !cmd.Flags().Changed("repo") { + repo, err := api.GitHubRepo(apiClient, baseRepo) + if err != nil { + return err + } + + currentBranch, err := ctx.Branch() + if err != nil { + return err + } + + if currentBranch == pr.HeadRefName { + err = git.CheckoutBranch(repo.DefaultBranchRef.Name) + if err != nil { + return err + } + } + + err = git.DeleteLocalBranch(pr.HeadRefName) + if err != nil { + fmt.Fprintf(colorableErr(cmd), "%s Could not delete local branch %s: %s\n", utils.Red("!"), utils.Cyan(pr.HeadRefName), err) + return err + } + fmt.Fprintf(colorableOut(cmd), "%s Deleted local branch %s\n", utils.Red("✔"), utils.Cyan(pr.HeadRefName)) + } return nil } +func prInteractiveMerge() (api.PullRequestMergeMethod, bool, error) { + mergeMethodQuestion := &survey.Question{ + Name: "mergeMethod", + Prompt: &survey.Select{ + Message: "What merge method would you like to use?", + Options: []string{"Create a merge commit", "Rebase and merge", "Squash and merge"}, + Default: "Create a merge commit", + }, + } + + deleteBranchQuestion := &survey.Question{ + Name: "deleteBranch", + Prompt: &survey.Confirm{ + Message: "Delete the branch locally?", + Default: true, + }, + } + + qs := []*survey.Question{mergeMethodQuestion, deleteBranchQuestion} + + answers := struct { + MergeMethod int + DeleteBranch bool + }{} + + err := SurveyAsk(qs, &answers) + if err != nil { + return 0, false, fmt.Errorf("could not prompt: %w", err) + } + + var mergeMethod api.PullRequestMergeMethod + switch answers.MergeMethod { + case 0: + mergeMethod = api.PullRequestMergeMethodMerge + case 1: + mergeMethod = api.PullRequestMergeMethodRebase + case 2: + mergeMethod = api.PullRequestMergeMethodSquash + } + + deleteBranch := answers.DeleteBranch + return mergeMethod, deleteBranch, nil +} + func printPrPreview(out io.Writer, pr *api.PullRequest) error { // Header (Title and State) fmt.Fprintln(out, utils.Bold(pr.Title)) diff --git a/command/pr_test.go b/command/pr_test.go index 22f7102f9..1006b1c39 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -982,10 +982,10 @@ func initWithStubs(branch string, stubs ...stubResponse) { initBlankContext("", "OWNER/REPO", branch) http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - for _, s := range stubs { http.StubResponse(s.ResponseCode, s.ResponseBody) } + http.StubRepoResponse("OWNER", "REPO") } func TestPrMerge(t *testing.T) { @@ -996,7 +996,16 @@ func TestPrMerge(t *testing.T) { stubResponse{200, bytes.NewBufferString(`{"id": "THE-ID"}`)}, ) - output, err := RunCommand("pr merge 1") + cs, cmdTeardown := test.InitCmdStubber() + defer cmdTeardown() + + cs.Stub("branch.blueberries.remote origin\nbranch.blueberries.merge refs/heads/blueberries") // git config --get-regexp ^branch\.master\.(remote|merge) + 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("") + + output, err := RunCommand("pr merge 1 --merge") if err != nil { t.Fatalf("error running command `pr merge`: %v", err) } @@ -1008,11 +1017,88 @@ func TestPrMerge(t *testing.T) { } } +func TestPrMerge_withRepoFlag(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + http := initFakeHTTP() + http.StubResponse(200, bytes.NewBufferString(`{ "data": { "repository": { + "pullRequest": { "number": 1, "closed": false, "state": "OPEN"} + } } }`)) + http.StubResponse(200, bytes.NewBufferString(`{"id": "THE-ID"}`)) + + cs, cmdTeardown := test.InitCmdStubber() + defer cmdTeardown() + + eq(t, len(cs.Calls), 0) + + output, err := RunCommand("pr merge 1 --merge -R stinky/boi") + if err != nil { + t.Fatalf("error running command `pr merge`: %v", err) + } + + r := regexp.MustCompile(`Merged pull request #1`) + + if !r.MatchString(output.String()) { + t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.Stderr()) + } +} + +func TestPrMerge_deleteBranch(t *testing.T) { + initWithStubs("blueberries", + stubResponse{200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "headRefName": "blueberries", "id": "THE-ID", "number": 3} + ] } } } }`)}, + stubResponse{200, bytes.NewBufferString(`{ "data": {} }`)}) + + 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 + + output, err := RunCommand(`pr merge --merge --delete-branch`) + if err != nil { + t.Fatalf("Got unexpected error running `pr merge` %s", err) + } + + test.ExpectLines(t, output.String(), "Merged pull request #3", "Deleted local branch") +} + +func TestPrMerge_deleteNonCurrentBranch(t *testing.T) { + initBlankContext("", "OWNER/REPO", "another-branch") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "headRefName": "blueberries", "id": "THE-ID", "number": 3} + ] } } } }`)) + http.StubResponse(200, bytes.NewBufferString(`{ "data": {} }`)) + http.StubRepoResponse("OWNER", "REPO") + + cs, cmdTeardown := test.InitCmdStubber() + defer cmdTeardown() + // We don't expect the default branch to be checked out, just that blueberries is deleted + cs.Stub("") // git branch -d blueberries + + output, err := RunCommand(`pr merge --merge --delete-branch blueberries`) + if err != nil { + t.Fatalf("Got unexpected error running `pr merge` %s", err) + } + + test.ExpectLines(t, output.String(), "Merged pull request #3", "Deleted local branch") +} + func TestPrMerge_noPrNumberGiven(t *testing.T) { cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() cs.Stub("branch.blueberries.remote origin\nbranch.blueberries.merge refs/heads/blueberries") // git config --get-regexp ^branch\.master\.(remote|merge) + 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 jsonFile, _ := os.Open("../test/fixtures/prViewPreviewWithMetadataByBranch.json") defer jsonFile.Close() @@ -1022,7 +1108,7 @@ func TestPrMerge_noPrNumberGiven(t *testing.T) { stubResponse{200, bytes.NewBufferString(`{"id": "THE-ID"}`)}, ) - output, err := RunCommand("pr merge") + output, err := RunCommand("pr merge --merge") if err != nil { t.Fatalf("error running command `pr merge`: %v", err) } @@ -1042,6 +1128,14 @@ func TestPrMerge_rebase(t *testing.T) { stubResponse{200, bytes.NewBufferString(`{"id": "THE-ID"}`)}, ) + 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 + output, err := RunCommand("pr merge 2 --rebase") if err != nil { t.Fatalf("error running command `pr merge`: %v", err) @@ -1062,6 +1156,14 @@ func TestPrMerge_squash(t *testing.T) { stubResponse{200, bytes.NewBufferString(`{"id": "THE-ID"}`)}, ) + 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 + output, err := RunCommand("pr merge 3 --squash") if err != nil { t.Fatalf("error running command `pr merge`: %v", err) @@ -1082,6 +1184,14 @@ func TestPrMerge_alreadyMerged(t *testing.T) { stubResponse{200, bytes.NewBufferString(`{"id": "THE-ID"}`)}, ) + 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 + output, err := RunCommand("pr merge 4") if err == nil { t.Fatalf("expected an error running command `pr merge`: %v", err) @@ -1094,6 +1204,58 @@ func TestPrMerge_alreadyMerged(t *testing.T) { } } +func TestPRMerge_interactive(t *testing.T) { + initWithStubs("blueberries", + stubResponse{200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "headRefName": "blueberries", "id": "THE-ID", "number": 3} + ] } } } }`)}, + stubResponse{200, bytes.NewBufferString(`{ "data": {} }`)}) + + 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 + + as, surveyTeardown := initAskStubber() + defer surveyTeardown() + + as.Stub([]*QuestionStub{ + { + Name: "mergeMethod", + Value: 0, + }, + { + Name: "deleteBranch", + Value: true, + }, + }) + + output, err := RunCommand(`pr merge`) + if err != nil { + t.Fatalf("Got unexpected error running `pr merge` %s", err) + } + + test.ExpectLines(t, output.String(), "Merged pull request #3", "Deleted local branch") +} + +func TestPrMerge_multipleMergeMethods(t *testing.T) { + initWithStubs("master", + stubResponse{200, bytes.NewBufferString(`{ "data": { "repository": { + "pullRequest": { "number": 1, "closed": false, "state": "OPEN"} + } } }`)}, + stubResponse{200, bytes.NewBufferString(`{"id": "THE-ID"}`)}, + ) + + _, err := RunCommand("pr merge 1 --merge --squash") + if err == nil { + t.Fatal("expected error running `pr merge` with multiple merge methods") + } +} + func TestPRReady(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() diff --git a/git/git.go b/git/git.go index 360917eab..634b9e084 100644 --- a/git/git.go +++ b/git/git.go @@ -203,6 +203,18 @@ func ReadBranchConfig(branch string) (cfg BranchConfig) { return } +func DeleteLocalBranch(branch string) error { + branchCmd := GitCommand("branch", "-D", branch) + err := run.PrepareCmd(branchCmd).Run() + return err +} + +func CheckoutBranch(branch string) error { + configCmd := GitCommand("checkout", branch) + err := run.PrepareCmd(configCmd).Run() + return err +} + func isFilesystemPath(p string) bool { return p == "." || strings.HasPrefix(p, "./") || strings.HasPrefix(p, "/") } diff --git a/test/helpers.go b/test/helpers.go index c8b0efe53..c2d4e2f37 100644 --- a/test/helpers.go +++ b/test/helpers.go @@ -68,6 +68,7 @@ func createStubbedPrepareCmd(cs *CmdStubber) func(*exec.Cmd) run.Runnable { if call >= len(cs.Stubs) { panic(fmt.Sprintf("more execs than stubs. most recent call: %v", cmd)) } + // fmt.Printf("Called stub for `%v`\n", cmd) // Helpful for debugging return cs.Stubs[call] } }