From 9786498a25a9333da3e682bb0e78c92de91466eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 17 Mar 2020 20:26:52 +0100 Subject: [PATCH 1/6] Respect base repository when passing URL argument to `pr checkout/view` Previously, the repository owner+name component of the URL was ignored and only the pull request number was read. Now, the URL dictates which base repository will be used. --- command/pr.go | 32 ++++++++++++----- command/pr_checkout.go | 36 ++++++++++++++----- command/pr_checkout_test.go | 72 +++++++++++++++++++++++++++++++++++++ 3 files changed, 122 insertions(+), 18 deletions(-) diff --git a/command/pr.go b/command/pr.go index bd7743f46..7ddaca56c 100644 --- a/command/pr.go +++ b/command/pr.go @@ -255,9 +255,21 @@ func prView(cmd *cobra.Command, args []string) error { return err } - baseRepo, err := determineBaseRepo(cmd, ctx) - if err != nil { - return err + var baseRepo ghrepo.Interface + var prArg string + if len(args) > 0 { + prArg = args[0] + if prNum, repo := prFromURL(prArg); repo != nil { + prArg = prNum + baseRepo = repo + } + } + + if baseRepo == nil { + baseRepo, err = determineBaseRepo(cmd, ctx) + if err != nil { + return err + } } preview, err := cmd.Flags().GetBool("preview") @@ -268,7 +280,7 @@ func prView(cmd *cobra.Command, args []string) error { var openURL string var pr *api.PullRequest if len(args) > 0 { - pr, err = prFromArg(apiClient, baseRepo, args[0]) + pr, err = prFromArg(apiClient, baseRepo, prArg) if err != nil { return err } @@ -331,16 +343,18 @@ func printPrPreview(out io.Writer, pr *api.PullRequest) error { var prURLRE = regexp.MustCompile(`^https://github\.com/([^/]+)/([^/]+)/pull/(\d+)`) +func prFromURL(arg string) (string, ghrepo.Interface) { + if m := prURLRE.FindStringSubmatch(arg); m != nil { + return m[3], ghrepo.New(m[1], m[2]) + } + return "", nil +} + func prFromArg(apiClient *api.Client, baseRepo ghrepo.Interface, arg string) (*api.PullRequest, error) { if prNumber, err := strconv.Atoi(strings.TrimPrefix(arg, "#")); err == nil { return api.PullRequestByNumber(apiClient, baseRepo, prNumber) } - if m := prURLRE.FindStringSubmatch(arg); m != nil { - prNumber, _ := strconv.Atoi(m[3]) - return api.PullRequestByNumber(apiClient, baseRepo, prNumber) - } - return api.PullRequestForBranch(apiClient, baseRepo, arg) } diff --git a/command/pr_checkout.go b/command/pr_checkout.go index 08a37033d..336a5a8d1 100644 --- a/command/pr_checkout.go +++ b/command/pr_checkout.go @@ -7,6 +7,7 @@ import ( "os/exec" "github.com/cli/cli/git" + "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/utils" "github.com/spf13/cobra" ) @@ -18,21 +19,38 @@ func prCheckout(cmd *cobra.Command, args []string) error { if err != nil { return err } - // FIXME: duplicates logic from fsContext.BaseRepo - baseRemote, err := remotes.FindByName("upstream", "github", "origin", "*") - if err != nil { - return err - } + apiClient, err := apiClientForContext(ctx) if err != nil { return err } - pr, err := prFromArg(apiClient, baseRemote, args[0]) + var baseRepo ghrepo.Interface + prArg := args[0] + if prNum, repo := prFromURL(prArg); repo != nil { + prArg = prNum + baseRepo = repo + } + + if baseRepo == nil { + baseRepo, err = determineBaseRepo(cmd, ctx) + if err != nil { + return err + } + } + + pr, err := prFromArg(apiClient, baseRepo, prArg) if err != nil { return err } + baseRemote, _ := remotes.FindByRepo(baseRepo.RepoOwner(), baseRepo.RepoName()) + // baseRemoteSpec is a repository URL or a remote name to be used in git fetch + baseURLOrName := fmt.Sprintf("https://github.com/%s.git", ghrepo.FullName(baseRepo)) + if baseRemote != nil { + baseURLOrName = baseRemote.Name + } + headRemote := baseRemote if pr.IsCrossRepository { headRemote, _ = remotes.FindByRepo(pr.HeadRepositoryOwner.Login, pr.HeadRepository.Name) @@ -67,15 +85,15 @@ func prCheckout(cmd *cobra.Command, args []string) error { ref := fmt.Sprintf("refs/pull/%d/head", pr.Number) if newBranchName == currentBranch { // PR head matches currently checked out branch - cmdQueue = append(cmdQueue, []string{"git", "fetch", baseRemote.Name, ref}) + cmdQueue = append(cmdQueue, []string{"git", "fetch", baseURLOrName, ref}) cmdQueue = append(cmdQueue, []string{"git", "merge", "--ff-only", "FETCH_HEAD"}) } else { // create a new branch - cmdQueue = append(cmdQueue, []string{"git", "fetch", baseRemote.Name, fmt.Sprintf("%s:%s", ref, newBranchName)}) + cmdQueue = append(cmdQueue, []string{"git", "fetch", baseURLOrName, fmt.Sprintf("%s:%s", ref, newBranchName)}) cmdQueue = append(cmdQueue, []string{"git", "checkout", newBranchName}) } - remote := baseRemote.Name + remote := baseURLOrName mergeRef := ref if pr.MaintainerCanModify { remote = fmt.Sprintf("https://github.com/%s/%s.git", pr.HeadRepositoryOwner.Login, pr.HeadRepository.Name) diff --git a/command/pr_checkout_test.go b/command/pr_checkout_test.go index ce0266c5e..1a36756a7 100644 --- a/command/pr_checkout_test.go +++ b/command/pr_checkout_test.go @@ -2,6 +2,8 @@ package command import ( "bytes" + "encoding/json" + "io/ioutil" "os/exec" "strings" "testing" @@ -21,6 +23,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { return ctx } http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequest": { @@ -112,6 +115,68 @@ func TestPRCheckout_urlArg(t *testing.T) { eq(t, strings.Join(ranCommands[1], " "), "git checkout -b feature --no-track origin/feature") } +func TestPRCheckout_urlArg_differentBase(t *testing.T) { + ctx := context.NewBlank() + ctx.SetBranch("master") + ctx.SetRemotes(map[string]string{ + "origin": "OWNER/REPO", + }) + initContext = func() context.Context { + return ctx + } + http := initFakeHTTP() + + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequest": { + "number": 123, + "headRefName": "feature", + "headRepositoryOwner": { + "login": "hubot" + }, + "headRepository": { + "name": "POE", + "defaultBranchRef": { + "name": "master" + } + }, + "isCrossRepository": false, + "maintainerCanModify": false + } } } } + `)) + + ranCommands := [][]string{} + restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + switch strings.Join(cmd.Args, " ") { + case "git show-ref --verify --quiet refs/heads/feature": + return &errorStub{"exit status: 1"} + default: + ranCommands = append(ranCommands, cmd.Args) + return &test.OutputStub{} + } + }) + defer restoreCmd() + + output, err := RunCommand(prCheckoutCmd, `pr checkout https://github.com/OTHER/POE/pull/123/files`) + eq(t, err, nil) + eq(t, output.String(), "") + + bodyBytes, _ := ioutil.ReadAll(http.Requests[0].Body) + reqBody := struct { + Variables struct { + Owner string + Repo string + } + }{} + json.Unmarshal(bodyBytes, &reqBody) + + eq(t, reqBody.Variables.Owner, "OTHER") + eq(t, reqBody.Variables.Repo, "POE") + + eq(t, len(ranCommands), 5) + eq(t, strings.Join(ranCommands[1], " "), "git fetch https://github.com/OTHER/POE.git refs/pull/123/head:feature") + eq(t, strings.Join(ranCommands[3], " "), "git config branch.feature.remote https://github.com/OTHER/POE.git") +} + func TestPRCheckout_branchArg(t *testing.T) { ctx := context.NewBlank() ctx.SetBranch("master") @@ -122,6 +187,7 @@ func TestPRCheckout_branchArg(t *testing.T) { return ctx } http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ @@ -171,6 +237,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { return ctx } http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequest": { @@ -223,6 +290,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { return ctx } http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequest": { @@ -275,6 +343,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { return ctx } http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequest": { @@ -327,6 +396,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { return ctx } http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequest": { @@ -377,6 +447,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { return ctx } http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequest": { @@ -427,6 +498,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { return ctx } http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequest": { From 88c060c610c2e74267ff5b655b2c3a8abb5bd7f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 20 Mar 2020 18:15:09 +0100 Subject: [PATCH 2/6] Fix broken `pr view` test --- command/pr_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/command/pr_test.go b/command/pr_test.go index 4b9d0c6de..22ec702ff 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -635,7 +635,6 @@ func TestPRView_numberArgWithHash(t *testing.T) { func TestPRView_urlArg(t *testing.T) { initBlankContext("OWNER/REPO", "master") http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequest": { From 488b47bdedccc8506fdd7830d110b4b371f8993a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 20 Mar 2020 18:18:27 +0100 Subject: [PATCH 3/6] Enable tests to capture spinner output Explicitly assign the writer stream for the progress spinner so that tests may override it. The default when not in testing stays the same: the output stream is the colorable stdout. --- command/repo.go | 2 +- utils/utils.go | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/command/repo.go b/command/repo.go index 1dc9b13a5..be930c0c8 100644 --- a/command/repo.go +++ b/command/repo.go @@ -295,7 +295,7 @@ func repoFork(cmd *cobra.Command, args []string) error { greenCheck := utils.Green("✓") out := colorableOut(cmd) - s := utils.Spinner() + s := utils.Spinner(out) loading := utils.Gray("Forking ") + utils.Bold(utils.Gray(ghrepo.FullName(toFork))) + utils.Gray("...") s.Suffix = " " + loading s.FinalMSG = utils.Gray(fmt.Sprintf("- %s\n", loading)) diff --git a/utils/utils.go b/utils/utils.go index 6217871b7..77c643eba 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -2,6 +2,7 @@ package utils import ( "fmt" + "io" "strings" "time" @@ -68,6 +69,8 @@ func Humanize(s string) string { return strings.Map(h, s) } -func Spinner() *spinner.Spinner { - return spinner.New(spinner.CharSets[11], 400*time.Millisecond) +func Spinner(w io.Writer) *spinner.Spinner { + s := spinner.New(spinner.CharSets[11], 400*time.Millisecond) + s.Writer = w + return s } From d2fda98e1610fe7f0b01acdd4621497aa00d5f18 Mon Sep 17 00:00:00 2001 From: Christian Muehlhaeuser Date: Fri, 20 Mar 2020 18:49:48 +0100 Subject: [PATCH 4/6] Bump glamour dependency This fixes #647 by not retrieving the terminal's background color on init. --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 63a39c1bf..ee45df4ed 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.13 require ( github.com/AlecAivazis/survey/v2 v2.0.7 github.com/briandowns/spinner v1.9.0 - github.com/charmbracelet/glamour v0.1.1-0.20200304134224-7e5c90143acc + github.com/charmbracelet/glamour v0.1.1-0.20200320173916-301d3bcf3058 github.com/dlclark/regexp2 v1.2.0 // indirect github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 github.com/hashicorp/go-version v1.2.0 diff --git a/go.sum b/go.sum index 8d7665e96..b94236621 100644 --- a/go.sum +++ b/go.sum @@ -23,8 +23,8 @@ github.com/beorn7/perks v1.0.0/go.mod h1:KWe93zE9D1o94FZ5RNwFwVgaQK1VOXiVxmqh+Ce github.com/briandowns/spinner v1.9.0 h1:+OMAisemaHar1hjuJ3Z2hIvNhQl9Y7GLPWUwwz2Pxo8= github.com/briandowns/spinner v1.9.0/go.mod h1://Zf9tMcxfRUA36V23M6YGEAv+kECGfvpnLTnb8n4XQ= github.com/cespare/xxhash v1.1.0/go.mod h1:XrSqR1VqqWfGrhpAt58auRo0WTKS1nRRg3ghfAqPWnc= -github.com/charmbracelet/glamour v0.1.1-0.20200304134224-7e5c90143acc h1:WSZ06evYgfSZYqkcv0+jFVSVCJeYsIvU+J4ILZUrha4= -github.com/charmbracelet/glamour v0.1.1-0.20200304134224-7e5c90143acc/go.mod h1:sC1EP6T+3nFnl5vwf0TYEs1inMigQxZ7n912YKoxJow= +github.com/charmbracelet/glamour v0.1.1-0.20200320173916-301d3bcf3058 h1:Ks+RZ6s6UriHnL+yusm3OoaLwpV9WPvMV+FXQ6qMD7M= +github.com/charmbracelet/glamour v0.1.1-0.20200320173916-301d3bcf3058/go.mod h1:sC1EP6T+3nFnl5vwf0TYEs1inMigQxZ7n912YKoxJow= github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw= github.com/coreos/bbolt v1.3.2/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk= github.com/coreos/etcd v3.3.10+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE= From 2895252e3bdc3f39cbc447c85829eb28fce3e70f Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 23 Mar 2020 14:26:31 -0500 Subject: [PATCH 5/6] respect -B when checking for existing pull requests --- api/queries_pr.go | 17 +++++++++++------ command/pr.go | 4 ++-- command/pr_create.go | 4 ++-- command/pr_create_test.go | 31 +++++++++++++++++++++++++++++-- 4 files changed, 44 insertions(+), 12 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index be86dc679..b2e5f2f77 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -340,7 +340,7 @@ func PullRequestByNumber(client *Client, repo ghrepo.Interface, number int) (*Pu return &resp.Repository.PullRequest, nil } -func PullRequestForBranch(client *Client, repo ghrepo.Interface, branch string) (*PullRequest, error) { +func PullRequestForBranch(client *Client, repo ghrepo.Interface, baseBranch, headBranch string) (*PullRequest, error) { type response struct { Repository struct { PullRequests struct { @@ -376,9 +376,9 @@ func PullRequestForBranch(client *Client, repo ghrepo.Interface, branch string) } }` - branchWithoutOwner := branch - if idx := strings.Index(branch, ":"); idx >= 0 { - branchWithoutOwner = branch[idx+1:] + branchWithoutOwner := headBranch + if idx := strings.Index(headBranch, ":"); idx >= 0 { + branchWithoutOwner = headBranch[idx+1:] } variables := map[string]interface{}{ @@ -394,12 +394,17 @@ func PullRequestForBranch(client *Client, repo ghrepo.Interface, branch string) } for _, pr := range resp.Repository.PullRequests.Nodes { - if pr.HeadLabel() == branch { + if pr.HeadLabel() == headBranch { + if baseBranch != "" { + if pr.BaseRefName != baseBranch { + continue + } + } return &pr, nil } } - return nil, &NotFoundError{fmt.Errorf("no open pull requests found for branch %q", branch)} + return nil, &NotFoundError{fmt.Errorf("no open pull requests found for branch %q", headBranch)} } // CreatePullRequest creates a pull request in a GitHub repository diff --git a/command/pr.go b/command/pr.go index bd7743f46..55e00b0c5 100644 --- a/command/pr.go +++ b/command/pr.go @@ -288,7 +288,7 @@ func prView(cmd *cobra.Command, args []string) error { } } } else { - pr, err = api.PullRequestForBranch(apiClient, baseRepo, branchWithOwner) + pr, err = api.PullRequestForBranch(apiClient, baseRepo, "", branchWithOwner) if err != nil { return err } @@ -341,7 +341,7 @@ func prFromArg(apiClient *api.Client, baseRepo ghrepo.Interface, arg string) (*a return api.PullRequestByNumber(apiClient, baseRepo, prNumber) } - return api.PullRequestForBranch(apiClient, baseRepo, arg) + return api.PullRequestForBranch(apiClient, baseRepo, "", arg) } func prSelectorForCurrentBranch(ctx context.Context, baseRepo ghrepo.Interface) (prNumber int, prHeadRef string, err error) { diff --git a/command/pr_create.go b/command/pr_create.go index ca7800a19..2be922987 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -132,13 +132,13 @@ func prCreate(cmd *cobra.Command, _ []string) error { if headRepo != nil && !ghrepo.IsSame(baseRepo, headRepo) { headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), headBranch) } - existingPR, err := api.PullRequestForBranch(client, baseRepo, headBranchLabel) + existingPR, err := api.PullRequestForBranch(client, baseRepo, baseBranch, headBranchLabel) var notFound *api.NotFoundError if err != nil && !errors.As(err, ¬Found) { return fmt.Errorf("error checking for existing pull request: %w", err) } if err == nil { - return fmt.Errorf("a pull request for branch %q already exists:\n%s", headBranchLabel, existingPR.URL) + return fmt.Errorf("a pull request for branch %q into branch %q already exists:\n%s", headBranchLabel, baseBranch, existingPR.URL) } } diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 688dfd9c8..874b02a86 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -64,7 +64,8 @@ func TestPRCreate_alreadyExists(t *testing.T) { http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", - "headRefName": "feature" } + "headRefName": "feature", + "baseRefName": "master" } ] } } } } `)) @@ -78,11 +79,37 @@ func TestPRCreate_alreadyExists(t *testing.T) { if err == nil { t.Fatal("error expected, got nil") } - if err.Error() != "a pull request for branch \"feature\" already exists:\nhttps://github.com/OWNER/REPO/pull/123" { + if err.Error() != "a pull request for branch \"feature\" into branch \"master\" already exists:\nhttps://github.com/OWNER/REPO/pull/123" { t.Errorf("got error %q", err) } } +func TestPRCreate_alreadyExistsDifferentBase(t *testing.T) { + initBlankContext("OWNER/REPO", "feature") + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } } + `)) + http.StubResponse(200, bytes.NewBufferString("{}")) + + cs, cmdTeardown := initCmdStubber() + defer cmdTeardown() + + cs.Stub("") // git status + cs.Stub("1234567890,commit 0\n2345678901,commit 1") // git log + cs.Stub("") // git rev-parse + + _, err := RunCommand(prCreateCmd, `pr create -BanotherBase -t"cool" -b"nah"`) + if err != nil { + t.Errorf("got unexpected error %q", err) + } +} + func TestPRCreate_web(t *testing.T) { initBlankContext("OWNER/REPO", "feature") http := initFakeHTTP() From 653229c7b5b564bd94d43cabef4f2dba9fd8de25 Mon Sep 17 00:00:00 2001 From: Tariq Ibrahim Date: Tue, 24 Mar 2020 22:46:13 -0700 Subject: [PATCH 6/6] simplify sort string slice method call --- api/queries_repo.go | 2 +- pkg/githubtemplate/github_template.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 9d0c4f00d..8ff9b070a 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -155,7 +155,7 @@ func RepoNetwork(client *Client, repos []ghrepo.Interface) (RepoNetworkResult, e keys = append(keys, key) } // sort keys to ensure `repo_{N}` entries are processed in order - sort.Sort(sort.StringSlice(keys)) + sort.Strings(keys) // Iterate over keys of GraphQL response data and, based on its name, // dynamically allocate the target struct an individual message gets decoded to. diff --git a/pkg/githubtemplate/github_template.go b/pkg/githubtemplate/github_template.go index f60878725..3aac35554 100644 --- a/pkg/githubtemplate/github_template.go +++ b/pkg/githubtemplate/github_template.go @@ -59,7 +59,7 @@ mainLoop: } } - sort.Sort(sort.StringSlice(results)) + sort.Strings(results) return results }