diff --git a/api/queries_pr.go b/api/queries_pr.go index 999d67126..46fa014cd 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -432,9 +432,6 @@ func PullRequestByNumber(client *Client, repo ghrepo.Interface, number int) (*Pu } headRepository { name - defaultBranchRef { - name - } } isCrossRepository isDraft diff --git a/api/queries_repo.go b/api/queries_repo.go index 049e305d8..3469ea80e 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -114,6 +114,18 @@ func GitHubRepo(client *Client, repo ghrepo.Interface) (*Repository, error) { return initRepoHostname(&result.Repository, repo.RepoHost()), nil } +func RepoDefaultBranch(client *Client, repo ghrepo.Interface) (string, error) { + if r, ok := repo.(*Repository); ok && r.DefaultBranchRef.Name != "" { + return r.DefaultBranchRef.Name, nil + } + + r, err := GitHubRepo(client, repo) + if err != nil { + return "", err + } + return r.DefaultBranchRef.Name, nil +} + // RepoParent finds out the parent repository of a fork func RepoParent(client *Client, repo ghrepo.Interface) (ghrepo.Interface, error) { var query struct { diff --git a/command/pr.go b/command/pr.go index 96cbee4ae..e9427d1c4 100644 --- a/command/pr.go +++ b/command/pr.go @@ -493,23 +493,21 @@ func prMerge(cmd *cobra.Command, args []string) error { fmt.Fprintf(colorableOut(cmd), "%s %s pull request #%d (%s)\n", utils.Magenta("āœ”"), action, pr.Number, pr.Title) if deleteBranch { - repo, err := api.GitHubRepo(apiClient, baseRepo) - if err != nil { - return err - } - - currentBranch, err := ctx.Branch() - if err != nil { - return err - } - branchSwitchString := "" if deleteLocalBranch && !crossRepoPR { + currentBranch, err := ctx.Branch() + if err != nil { + return err + } + var branchToSwitchTo string if currentBranch == pr.HeadRefName { - branchToSwitchTo = repo.DefaultBranchRef.Name - err = git.CheckoutBranch(repo.DefaultBranchRef.Name) + branchToSwitchTo, err = api.RepoDefaultBranch(apiClient, baseRepo) + if err != nil { + return err + } + err = git.CheckoutBranch(branchToSwitchTo) if err != nil { return err } diff --git a/command/pr_checkout.go b/command/pr_checkout.go index c81bf4f76..2c12cb6c3 100644 --- a/command/pr_checkout.go +++ b/command/pr_checkout.go @@ -8,6 +8,7 @@ import ( "github.com/spf13/cobra" + "github.com/cli/cli/api" "github.com/cli/cli/git" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" @@ -65,8 +66,13 @@ func prCheckout(cmd *cobra.Command, args []string) error { } else { // no git remote for PR head + defaultBranchName, err := api.RepoDefaultBranch(apiClient, baseRepo) + if err != nil { + return err + } + // avoid naming the new branch the same as the default branch - if newBranchName == pr.HeadRepository.DefaultBranchRef.Name { + if newBranchName == defaultBranchName { newBranchName = fmt.Sprintf("%s/%s", pr.HeadRepositoryOwner.Login, newBranchName) } diff --git a/command/pr_checkout_test.go b/command/pr_checkout_test.go index cf86d6f7c..d7f0e3e18 100644 --- a/command/pr_checkout_test.go +++ b/command/pr_checkout_test.go @@ -33,10 +33,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": false, "maintainerCanModify": false @@ -84,10 +81,7 @@ func TestPRCheckout_urlArg(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": false, "maintainerCanModify": false @@ -132,15 +126,17 @@ func TestPRCheckout_urlArg_differentBase(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "POE", - "defaultBranchRef": { - "name": "master" - } + "name": "POE" }, "isCrossRepository": false, "maintainerCanModify": false } } } } `)) + http.Register(httpmock.GraphQL(`query RepositoryInfo\b`), httpmock.StringResponse(` + { "data": { "repository": { + "defaultBranchRef": {"name": "master"} + } } } + `)) ranCommands := [][]string{} restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { @@ -195,10 +191,7 @@ func TestPRCheckout_branchArg(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": false } @@ -245,10 +238,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": false, "maintainerCanModify": false @@ -298,10 +288,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": false @@ -351,10 +338,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": false @@ -404,10 +388,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": false @@ -455,10 +436,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": false @@ -506,10 +484,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { "login": "hubot" }, "headRepository": { - "name": "REPO", - "defaultBranchRef": { - "name": "master" - } + "name": "REPO" }, "isCrossRepository": true, "maintainerCanModify": true diff --git a/command/pr_test.go b/command/pr_test.go index c3403252e..0963ce2cb 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -937,28 +937,25 @@ func TestPRReopen_alreadyMerged(t *testing.T) { func TestPrMerge(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 1, "title": "The title of the PR", "state": "OPEN", "id": "THE-ID"} - } } }`)) + { "data": { "repository": { "pullRequest": { + "id": "THE-ID", + "number": 1, + "title": "The title of the PR", + "state": "OPEN", + "headRefName": "blueberries", + "headRepositoryOwner": {"login": "OWNER"} + } } } }`)) 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)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) http.Register( httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), httpmock.StringResponse(`{}`)) @@ -987,6 +984,7 @@ func TestPrMerge(t *testing.T) { func TestPrMerge_withRepoFlag(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() + defer http.Verify(t) http.Register( httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.GraphQLQuery(` @@ -1002,18 +1000,6 @@ func TestPrMerge_withRepoFlag(t *testing.T) { assert.Equal(t, "THE-ID", input["pullRequestId"].(string)) assert.Equal(t, "MERGE", input["mergeMethod"].(string)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) - http.Register( - httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), - httpmock.StringResponse(`{}`)) cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() @@ -1035,6 +1021,7 @@ func TestPrMerge_withRepoFlag(t *testing.T) { func TestPrMerge_deleteBranch(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestForBranch\b`), @@ -1045,15 +1032,6 @@ func TestPrMerge_deleteBranch(t *testing.T) { assert.Equal(t, "PR_10", input["pullRequestId"].(string)) assert.Equal(t, "MERGE", input["mergeMethod"].(string)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) http.Register( httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), httpmock.StringResponse(`{}`)) @@ -1078,6 +1056,7 @@ func TestPrMerge_deleteBranch(t *testing.T) { func TestPrMerge_deleteNonCurrentBranch(t *testing.T) { initBlankContext("", "OWNER/REPO", "another-branch") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestForBranch\b`), @@ -1088,15 +1067,6 @@ func TestPrMerge_deleteNonCurrentBranch(t *testing.T) { assert.Equal(t, "PR_10", input["pullRequestId"].(string)) assert.Equal(t, "MERGE", input["mergeMethod"].(string)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) http.Register( httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), httpmock.StringResponse(`{}`)) @@ -1119,6 +1089,7 @@ func TestPrMerge_deleteNonCurrentBranch(t *testing.T) { func TestPrMerge_noPrNumberGiven(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestForBranch\b`), @@ -1129,15 +1100,6 @@ func TestPrMerge_noPrNumberGiven(t *testing.T) { assert.Equal(t, "PR_10", input["pullRequestId"].(string)) assert.Equal(t, "MERGE", input["mergeMethod"].(string)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) http.Register( httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), httpmock.StringResponse(`{}`)) @@ -1166,28 +1128,25 @@ func TestPrMerge_noPrNumberGiven(t *testing.T) { func TestPrMerge_rebase(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 2, "title": "The title of the PR", "state": "OPEN", "id": "THE-ID"} - } } }`)) + { "data": { "repository": { "pullRequest": { + "id": "THE-ID", + "number": 2, + "title": "The title of the PR", + "state": "OPEN", + "headRefName": "blueberries", + "headRepositoryOwner": {"login": "OWNER"} + } } } }`)) 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, "REBASE", input["mergeMethod"].(string)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) http.Register( httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), httpmock.StringResponse(`{}`)) @@ -1215,28 +1174,25 @@ func TestPrMerge_rebase(t *testing.T) { func TestPrMerge_squash(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` - { "data": { "repository": { - "pullRequest": { "number": 3, "title": "The title of the PR", "state": "OPEN", "id": "THE-ID"} - } } }`)) + { "data": { "repository": { "pullRequest": { + "id": "THE-ID", + "number": 3, + "title": "The title of the PR", + "state": "OPEN", + "headRefName": "blueberries", + "headRepositoryOwner": {"login": "OWNER"} + } } } }`)) 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, "SQUASH", input["mergeMethod"].(string)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) http.Register( httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), httpmock.StringResponse(`{}`)) @@ -1254,16 +1210,14 @@ func TestPrMerge_squash(t *testing.T) { t.Fatalf("error running command `pr merge`: %v", err) } - r := regexp.MustCompile(`Squashed and merged pull request #3`) - - if !r.MatchString(output.String()) { - t.Fatalf("output did not match regexp /%s/\n> output\n%q\n", r, output.String()) - } + expected := "āœ” Squashed and merged pull request #3 (The title of the PR)\nāœ” Deleted branch blueberries\n" + assert.Equal(t, expected, output.String()) } func TestPrMerge_alreadyMerged(t *testing.T) { initBlankContext("", "OWNER/REPO", "master") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestByNumber\b`), @@ -1295,6 +1249,7 @@ func TestPrMerge_alreadyMerged(t *testing.T) { func TestPRMerge_interactive(t *testing.T) { initBlankContext("", "OWNER/REPO", "blueberries") http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register( httpmock.GraphQL(`query PullRequestForBranch\b`), @@ -1311,15 +1266,6 @@ func TestPRMerge_interactive(t *testing.T) { assert.Equal(t, "THE-ID", input["pullRequestId"].(string)) assert.Equal(t, "MERGE", input["mergeMethod"].(string)) })) - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(`{ - "data": { - "repository": { - "defaultBranchRef": {"name": "master"} - } - } - }`)) http.Register( httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/blueberries"), httpmock.StringResponse(`{}`)) diff --git a/pkg/httpmock/registry.go b/pkg/httpmock/registry.go index 486d79a06..134c88d48 100644 --- a/pkg/httpmock/registry.go +++ b/pkg/httpmock/registry.go @@ -21,6 +21,7 @@ func (r *Registry) Register(m Matcher, resp Responder) { type Testing interface { Errorf(string, ...interface{}) + Helper() } func (r *Registry) Verify(t Testing) { @@ -31,6 +32,7 @@ func (r *Registry) Verify(t Testing) { } } if n > 0 { + t.Helper() // NOTE: stubs offer no useful reflection, so we can't print details // about dead stubs and what they were trying to match t.Errorf("%d unmatched HTTP stubs", n)