From 6825944cadc3b4dd54cb91c2f5c115ad9ba86e3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 15 Jul 2020 14:54:30 +0200 Subject: [PATCH 1/4] Reference named queries in `pr checkout` HTTP stubs --- command/pr_checkout_test.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/command/pr_checkout_test.go b/command/pr_checkout_test.go index 16b7429be..cf86d6f7c 100644 --- a/command/pr_checkout_test.go +++ b/command/pr_checkout_test.go @@ -1,7 +1,6 @@ package command import ( - "bytes" "encoding/json" "io/ioutil" "os/exec" @@ -10,6 +9,7 @@ import ( "github.com/cli/cli/context" "github.com/cli/cli/internal/run" + "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/test" ) @@ -25,7 +25,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -76,7 +76,7 @@ func TestPRCheckout_urlArg(t *testing.T) { return ctx } http := initFakeHTTP() - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -124,7 +124,7 @@ func TestPRCheckout_urlArg_differentBase(t *testing.T) { return ctx } http := initFakeHTTP() - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -187,7 +187,7 @@ func TestPRCheckout_branchArg(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestForBranch\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequests": { "nodes": [ { "number": 123, "headRefName": "feature", @@ -237,7 +237,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -290,7 +290,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -343,7 +343,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -396,7 +396,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -447,7 +447,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", @@ -498,7 +498,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, "headRefName": "feature", From 305cd290ee7c2fe922e40db33120dd45321a31d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 15 Jul 2020 15:35:42 +0200 Subject: [PATCH 2/4] Fix `pr checkout :` when it matches the default branch First, consolidate the functionality between `pr merge` and `pr checkout` that resolves the default branch name of the base repo. With an added bonus, the new approach avoids an API request when one isn't necessary. Then, ensure that checking out 3rd-party PRs will result in local branch name such as `/` when the head branch of the repository matches the default branch of the base repository. We already have had code in place to take care of this, but it only took effect in the `pr checkout `-style invocation. --- api/queries_pr.go | 3 - api/queries_repo.go | 12 ++++ command/pr.go | 22 +++---- command/pr_checkout.go | 8 ++- command/pr_checkout_test.go | 55 +++++----------- command/pr_test.go | 124 ++++++++++-------------------------- pkg/httpmock/registry.go | 2 + 7 files changed, 81 insertions(+), 145 deletions(-) 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) From c232c8872c5e36d4f48ac89bfafd2fdd66fc31b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 15 Jul 2020 16:16:38 +0200 Subject: [PATCH 3/4] Abort `pr checkout` when PR has an invalid head branch name --- command/pr_checkout.go | 4 ++++ command/pr_checkout_test.go | 42 +++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) diff --git a/command/pr_checkout.go b/command/pr_checkout.go index 2c12cb6c3..256a3dfe8 100644 --- a/command/pr_checkout.go +++ b/command/pr_checkout.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "os/exec" + "strings" "github.com/spf13/cobra" @@ -46,6 +47,9 @@ func prCheckout(cmd *cobra.Command, args []string) error { var cmdQueue [][]string newBranchName := pr.HeadRefName + if strings.HasPrefix(newBranchName, "-") { + return fmt.Errorf("invalid branch name: %q", newBranchName) + } if headRemote != nil { // there is an existing git remote for PR head diff --git a/command/pr_checkout_test.go b/command/pr_checkout_test.go index d7f0e3e18..eeda8cee9 100644 --- a/command/pr_checkout_test.go +++ b/command/pr_checkout_test.go @@ -11,6 +11,7 @@ import ( "github.com/cli/cli/internal/run" "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/test" + "github.com/stretchr/testify/assert" ) func TestPRCheckout_sameRepo(t *testing.T) { @@ -464,6 +465,47 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { eq(t, strings.Join(ranCommands[1], " "), "git merge --ff-only FETCH_HEAD") } +func TestPRCheckout_differentRepo_invalidBranchName(t *testing.T) { + ctx := context.NewBlank() + ctx.SetBranch("feature") + ctx.SetRemotes(map[string]string{ + "origin": "OWNER/REPO", + }) + initContext = func() context.Context { + return ctx + } + http := initFakeHTTP() + defer http.Verify(t) + http.StubRepoResponse("OWNER", "REPO") + + http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` + { "data": { "repository": { "pullRequest": { + "number": 123, + "headRefName": "-foo", + "headRepositoryOwner": { + "login": "hubot" + }, + "headRepository": { + "name": "REPO" + }, + "isCrossRepository": true, + "maintainerCanModify": false + } } } } + `)) + + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { + t.Errorf("unexpected external invocation: %v", cmd.Args) + return &test.OutputStub{} + }) + defer restoreCmd() + + output, err := RunCommand(`pr checkout 123`) + if assert.Errorf(t, err, "expected command to fail") { + assert.Equal(t, `invalid branch name: "-foo"`, err.Error()) + } + assert.Equal(t, "", output.Stderr()) +} + func TestPRCheckout_maintainerCanModify(t *testing.T) { ctx := context.NewBlank() ctx.SetBranch("master") From dfb774cd38006b811ac8a166ff11eec3a3fb97b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 15 Jul 2020 16:18:36 +0200 Subject: [PATCH 4/4] Verify HTTP stubs in `pr checkout` tests --- command/pr_checkout_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/command/pr_checkout_test.go b/command/pr_checkout_test.go index eeda8cee9..525225802 100644 --- a/command/pr_checkout_test.go +++ b/command/pr_checkout_test.go @@ -24,6 +24,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` @@ -74,6 +75,7 @@ func TestPRCheckout_urlArg(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, @@ -119,6 +121,7 @@ func TestPRCheckout_urlArg_differentBase(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { "number": 123, @@ -182,6 +185,7 @@ func TestPRCheckout_branchArg(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestForBranch\b`), httpmock.StringResponse(` @@ -229,6 +233,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` @@ -279,6 +284,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` @@ -329,6 +335,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` @@ -379,6 +386,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` @@ -427,6 +435,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` @@ -516,6 +525,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { return ctx } http := initFakeHTTP() + defer http.Verify(t) http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(`