Merge pull request #687 from cli/pr-checkout-repo-override

Respect base repository when passing URL argument to `pr checkout/view`

command/pr.go

@@ -255,9 +255,21 @@ func prView(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	baseRepo, err := determineBaseRepo(cmd, ctx)
+	var baseRepo ghrepo.Interface
-	if err != nil {
+	var prArg string
-		return err
+	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)
 }
 

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)

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": {

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": {