Merge pull request #1365 from cli/pr-checkout-fixes

pr checkout fixes for branch names
This commit is contained in:
Mislav Marohnić 2020-07-16 14:31:04 +02:00 committed by GitHub
commit b0fede93c9
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
7 changed files with 148 additions and 156 deletions

View file

@ -432,9 +432,6 @@ func PullRequestByNumber(client *Client, repo ghrepo.Interface, number int) (*Pu
}
headRepository {
name
defaultBranchRef {
name
}
}
isCrossRepository
isDraft

View file

@ -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 {

View file

@ -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
}

View file

@ -5,9 +5,11 @@ import (
"fmt"
"os"
"os/exec"
"strings"
"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"
@ -45,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
@ -65,8 +70,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)
}

View file

@ -1,7 +1,6 @@
package command
import (
"bytes"
"encoding/json"
"io/ioutil"
"os/exec"
@ -10,7 +9,9 @@ import (
"github.com/cli/cli/context"
"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) {
@ -23,9 +24,10 @@ func TestPRCheckout_sameRepo(t *testing.T) {
return ctx
}
http := initFakeHTTP()
defer http.Verify(t)
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",
@ -33,10 +35,7 @@ func TestPRCheckout_sameRepo(t *testing.T) {
"login": "hubot"
},
"headRepository": {
"name": "REPO",
"defaultBranchRef": {
"name": "master"
}
"name": "REPO"
},
"isCrossRepository": false,
"maintainerCanModify": false
@ -76,7 +75,8 @@ func TestPRCheckout_urlArg(t *testing.T) {
return ctx
}
http := initFakeHTTP()
http.StubResponse(200, bytes.NewBufferString(`
defer http.Verify(t)
http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(`
{ "data": { "repository": { "pullRequest": {
"number": 123,
"headRefName": "feature",
@ -84,10 +84,7 @@ func TestPRCheckout_urlArg(t *testing.T) {
"login": "hubot"
},
"headRepository": {
"name": "REPO",
"defaultBranchRef": {
"name": "master"
}
"name": "REPO"
},
"isCrossRepository": false,
"maintainerCanModify": false
@ -124,7 +121,8 @@ func TestPRCheckout_urlArg_differentBase(t *testing.T) {
return ctx
}
http := initFakeHTTP()
http.StubResponse(200, bytes.NewBufferString(`
defer http.Verify(t)
http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(`
{ "data": { "repository": { "pullRequest": {
"number": 123,
"headRefName": "feature",
@ -132,15 +130,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 {
@ -185,9 +185,10 @@ func TestPRCheckout_branchArg(t *testing.T) {
return ctx
}
http := initFakeHTTP()
defer http.Verify(t)
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",
@ -195,10 +196,7 @@ func TestPRCheckout_branchArg(t *testing.T) {
"login": "hubot"
},
"headRepository": {
"name": "REPO",
"defaultBranchRef": {
"name": "master"
}
"name": "REPO"
},
"isCrossRepository": true,
"maintainerCanModify": false }
@ -235,9 +233,10 @@ func TestPRCheckout_existingBranch(t *testing.T) {
return ctx
}
http := initFakeHTTP()
defer http.Verify(t)
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",
@ -245,10 +244,7 @@ func TestPRCheckout_existingBranch(t *testing.T) {
"login": "hubot"
},
"headRepository": {
"name": "REPO",
"defaultBranchRef": {
"name": "master"
}
"name": "REPO"
},
"isCrossRepository": false,
"maintainerCanModify": false
@ -288,9 +284,10 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) {
return ctx
}
http := initFakeHTTP()
defer http.Verify(t)
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",
@ -298,10 +295,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) {
"login": "hubot"
},
"headRepository": {
"name": "REPO",
"defaultBranchRef": {
"name": "master"
}
"name": "REPO"
},
"isCrossRepository": true,
"maintainerCanModify": false
@ -341,9 +335,10 @@ func TestPRCheckout_differentRepo(t *testing.T) {
return ctx
}
http := initFakeHTTP()
defer http.Verify(t)
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",
@ -351,10 +346,7 @@ func TestPRCheckout_differentRepo(t *testing.T) {
"login": "hubot"
},
"headRepository": {
"name": "REPO",
"defaultBranchRef": {
"name": "master"
}
"name": "REPO"
},
"isCrossRepository": true,
"maintainerCanModify": false
@ -394,9 +386,10 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) {
return ctx
}
http := initFakeHTTP()
defer http.Verify(t)
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",
@ -404,10 +397,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) {
"login": "hubot"
},
"headRepository": {
"name": "REPO",
"defaultBranchRef": {
"name": "master"
}
"name": "REPO"
},
"isCrossRepository": true,
"maintainerCanModify": false
@ -445,9 +435,10 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) {
return ctx
}
http := initFakeHTTP()
defer http.Verify(t)
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",
@ -455,10 +446,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) {
"login": "hubot"
},
"headRepository": {
"name": "REPO",
"defaultBranchRef": {
"name": "master"
}
"name": "REPO"
},
"isCrossRepository": true,
"maintainerCanModify": false
@ -486,6 +474,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")
@ -496,9 +525,10 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) {
return ctx
}
http := initFakeHTTP()
defer http.Verify(t)
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",
@ -506,10 +536,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) {
"login": "hubot"
},
"headRepository": {
"name": "REPO",
"defaultBranchRef": {
"name": "master"
}
"name": "REPO"
},
"isCrossRepository": true,
"maintainerCanModify": true

View file

@ -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(`{}`))

View file

@ -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)