Fix PR checkout panic when base repo is not in remotes

acceptance/testdata/pr/pr-checkout-by-number.txtar

@@ -0,0 +1,33 @@
+# Set up env vars
+env REPO=${SCRIPT_NAME}-${RANDOM_STRING}
+
+# Use gh as a credential helper
+exec gh auth setup-git
+
+# Create a repository with a file so it has a default branch
+exec gh repo create ${ORG}/${REPO} --add-readme --private
+
+# Defer repo cleanup
+defer gh repo delete --yes ${ORG}/${REPO}
+
+# Clone the repo
+exec gh repo clone ${ORG}/${REPO}
+
+# Prepare a branch to PR
+cd ${REPO}
+exec git checkout -b feature-branch
+exec git commit --allow-empty -m 'Empty Commit'
+exec git push -u origin feature-branch
+
+# Create the PR
+exec gh pr create --title 'Feature Title' --body 'Feature Body'
+stdout2env PR_URL
+
+# Remove the local branch
+exec git checkout main
+exec git branch -D feature-branch
+stdout 'Deleted branch feature-branch'
+
+# Checkout the PR
+exec gh pr checkout 1
+stderr 'Switched to a new branch ''feature-branch'''

acceptance/testdata/pr/pr-checkout-with-url-from-fork.txtar

@@ -0,0 +1,37 @@
+# Set up env vars
+env REPO=${SCRIPT_NAME}-${RANDOM_STRING}
+
+# Use gh as a credential helper
+exec gh auth setup-git
+
+# Create a repository with a file so it has a default branch
+exec gh repo create ${ORG}/${REPO} --add-readme --private
+
+# Defer upstream cleanup
+defer gh repo delete --yes ${ORG}/${REPO}
+
+# Create a fork
+exec gh repo fork ${ORG}/${REPO} --org ${ORG} --fork-name ${REPO}-fork
+
+# Defer fork cleanup
+defer gh repo delete --yes ${ORG}/${REPO}-fork
+
+# Clone both repos
+exec gh repo clone ${ORG}/${REPO}
+exec gh repo clone ${ORG}/${REPO}-fork
+
+# Prepare a branch to PR in the fork itself
+cd ${REPO}-fork
+exec git checkout -b feature-branch
+exec git commit --allow-empty -m 'Empty Commit'
+exec git push -u origin feature-branch
+
+# Create the PR inside the fork
+exec gh repo set-default ${ORG}/${REPO}-fork
+exec gh pr create --title 'Feature Title' --body 'Feature Body'
+stdout2env PR_URL
+
+# Checkout the PR by full URL in the upstream repo
+cd ${WORK}/${REPO}
+exec gh pr checkout ${PR_URL}
+stderr 'Switched to branch ''feature-branch'''

git/client.go

@@ -115,23 +115,23 @@ type CredentialPattern struct {
 var AllMatchingCredentialsPattern = CredentialPattern{allMatching: true, pattern: ""}
 var disallowedCredentialPattern = CredentialPattern{allMatching: false, pattern: ""}
 
-// WM-TODO: Are there any funny remotes that might not resolve to a URL?
-func CredentialPatternFromRemote(ctx context.Context, c *Client, remote string) (CredentialPattern, error) {
-	gitURL, err := c.GetRemoteURL(ctx, remote)
-	if err != nil {
-		return CredentialPattern{}, err
-	}
-	return CredentialPatternFromGitURL(gitURL)
-}
-
+// CredentialPatternFromGitURL takes a git remote URL e.g. "https://github.com/cli/cli.git" or
+// "git@github.com:cli/cli.git" and returns the credential pattern that should be used for it.
 func CredentialPatternFromGitURL(gitURL string) (CredentialPattern, error) {
 	normalizedURL, err := ParseURL(gitURL)
 	if err != nil {
 		return CredentialPattern{}, fmt.Errorf("failed to parse remote URL: %w", err)
 	}
+	return CredentialPatternFromHost(normalizedURL.Host), nil
+}
+
+// CredentialPatternFromHost expects host to be in the form "github.com" and returns
+// the credential pattern that should be used for it.
+// It does not perform any canonicalisation e.g. "api.github.com" will not work as expected.
+func CredentialPatternFromHost(host string) CredentialPattern {
 	return CredentialPattern{
-		pattern: strings.TrimSuffix(ghinstance.HostPrefix(normalizedURL.Host), "/"),
-	}, nil
+		pattern: strings.TrimSuffix(ghinstance.HostPrefix(host), "/"),
+	}
 }
 
 // AuthenticatedCommand is a wrapper around Command that included configuration to use gh
@@ -202,19 +202,6 @@ func (c *Client) UpdateRemoteURL(ctx context.Context, name, url string) error {
 	return nil
 }
 
-func (c *Client) GetRemoteURL(ctx context.Context, name string) (string, error) {
-	args := []string{"remote", "get-url", name}
-	cmd, err := c.Command(ctx, args...)
-	if err != nil {
-		return "", err
-	}
-	out, err := cmd.Output()
-	if err != nil {
-		return "", err
-	}
-	return strings.TrimSpace(string(out)), nil
-}
-
 func (c *Client) SetRemoteResolution(ctx context.Context, name, resolution string) error {
 	args := []string{"config", "--add", fmt.Sprintf("remote.%s.gh-resolved", name), resolution}
 	cmd, err := c.Command(ctx, args...)

git/client_test.go

@@ -1564,7 +1564,7 @@ func TestCredentialPatternFromGitURL(t *testing.T) {
 	}{
 		{
 			name:   "Given a well formed gitURL, it returns the corresponding CredentialPattern",
-			gitURL: "https://github.com/OWNER/REPO",
+			gitURL: "https://github.com/OWNER/REPO.git",
 			wantCredentialPattern: CredentialPattern{
 				pattern:     "https://github.com",
 				allMatching: false,
@@ -1591,47 +1591,25 @@ func TestCredentialPatternFromGitURL(t *testing.T) {
 	}
 }
 
-func TestCredentialPatternFromRemote(t *testing.T) {
+func TestCredentialPatternFromHost(t *testing.T) {
 	tests := []struct {
 		name                  string
-		remote                string
+		host                  string
 		wantCredentialPattern CredentialPattern
-		wantErr               bool
 	}{
 		{
-			name:   "Given a well formed remote, it returns the corresponding CredentialPattern",
-			remote: "https://github.com/OWNER/REPO",
+			name: "Given a well formed host, it returns the corresponding CredentialPattern",
+			host: "github.com",
 			wantCredentialPattern: CredentialPattern{
 				pattern:     "https://github.com",
 				allMatching: false,
 			},
 		},
-		{
-			name:    "Given an error from GetRemoteURL, it returns that error",
-			remote:  "foo remote",
-			wantErr: true,
-		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			var cmdCtx func(ctx context.Context, name string, args ...string) *exec.Cmd
-			if tt.wantErr {
-				_, cmdCtx = createCommandContext(t, 1, tt.remote, "GetRemoteURL error")
-			} else {
-				_, cmdCtx = createCommandContext(t, 0, tt.remote, "")
-			}
-
-			client := Client{
-				GitPath:        "path/to/git",
-				commandContext: cmdCtx,
-			}
-			credentialPattern, err := CredentialPatternFromRemote(context.Background(), &client, tt.remote)
-			if tt.wantErr {
-				assert.ErrorContains(t, err, "GetRemoteURL error")
-			} else {
-				assert.NoError(t, err)
-				assert.Equal(t, tt.wantCredentialPattern, credentialPattern)
-			}
+			credentialPattern := CredentialPatternFromHost(tt.host)
+			require.Equal(t, tt.wantCredentialPattern, credentialPattern)
 		})
 	}
 }

pkg/cmd/pr/checkout/checkout.go

@@ -130,14 +130,9 @@ func checkoutRun(opts *CheckoutOptions) error {
 		cmdQueue = append(cmdQueue, []string{"submodule", "update", "--init", "--recursive"})
 	}
 
-	// Note that although we will probably be fetching from the headRemote, in practice, PR checkout can only
-	// ever point to one host, and we know baseRemote must be populated, where headRemote might be nil (e.g. when
-	// it was deleted).
-	credentialPattern, err := git.CredentialPatternFromRemote(context.Background(), opts.GitClient, baseRemote.Name)
-	if err != nil {
-		return err
-	}
-	err = executeCmds(opts.GitClient, credentialPattern, cmdQueue)
+	// Note that although we will probably be fetching from the head, in practice, PR checkout can only
+	// ever point to one host, and we know baseRepo must be populated.
+	err = executeCmds(opts.GitClient, git.CredentialPatternFromHost(baseRepo.RepoHost()), cmdQueue)
 	if err != nil {
 		return err
 	}

pkg/cmd/pr/checkout/checkout_test.go

@@ -93,7 +93,6 @@ func Test_checkoutRun(t *testing.T) {
 			},
 			runStubs: func(cs *run.CommandStubber) {
 				cs.Register(`git show-ref --verify -- refs/heads/feature`, 1, "")
-				cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git")
 				cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "")
 				cs.Register(`git checkout -b feature --track origin/feature`, 0, "")
 			},
@@ -120,7 +119,6 @@ func Test_checkoutRun(t *testing.T) {
 				"origin": "OWNER/REPO",
 			},
 			runStubs: func(cs *run.CommandStubber) {
-				cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git")
 				cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "")
 				cs.Register(`git config branch\.feature\.merge`, 1, "")
 				cs.Register(`git checkout feature`, 0, "")
@@ -151,7 +149,6 @@ func Test_checkoutRun(t *testing.T) {
 			},
 			runStubs: func(cs *run.CommandStubber) {
 				cs.Register(`git show-ref --verify -- refs/heads/foobar`, 1, "")
-				cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git")
 				cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "")
 				cs.Register(`git checkout -b foobar --track origin/feature`, 0, "")
 			},
@@ -179,7 +176,6 @@ func Test_checkoutRun(t *testing.T) {
 			},
 			runStubs: func(cs *run.CommandStubber) {
 				cs.Register(`git config branch\.foobar\.merge`, 1, "")
-				cs.Register(`git remote get-url origin`, 0, "https://github.com/hubot/REPO.git")
 				cs.Register(`git fetch origin refs/pull/123/head:foobar`, 0, "")
 				cs.Register(`git checkout foobar`, 0, "")
 				cs.Register(`git config branch\.foobar\.remote https://github.com/hubot/REPO.git`, 0, "")
@@ -305,7 +301,6 @@ func TestPRCheckout_sameRepo(t *testing.T) {
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
 
-	cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git")
 	cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "")
 	cs.Register(`git show-ref --verify -- refs/heads/feature`, 1, "")
 	cs.Register(`git checkout -b feature --track origin/feature`, 0, "")
@@ -325,8 +320,6 @@ func TestPRCheckout_existingBranch(t *testing.T) {
 
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
-
-	cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git")
 	cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "")
 	cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "")
 	cs.Register(`git checkout feature`, 0, "")
@@ -359,8 +352,6 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) {
 
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
-
-	cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git")
 	cs.Register(`git fetch robot-fork \+refs/heads/feature:refs/remotes/robot-fork/feature`, 0, "")
 	cs.Register(`git show-ref --verify -- refs/heads/feature`, 1, "")
 	cs.Register(`git checkout -b feature --track robot-fork/feature`, 0, "")
@@ -381,8 +372,6 @@ func TestPRCheckout_differentRepo(t *testing.T) {
 
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
-
-	cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git")
 	cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "")
 	cs.Register(`git config branch\.feature\.merge`, 1, "")
 	cs.Register(`git checkout feature`, 0, "")
@@ -405,8 +394,6 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) {
 
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
-
-	cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git")
 	cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "")
 	cs.Register(`git config branch\.feature\.merge`, 0, "refs/heads/feature\n")
 	cs.Register(`git checkout feature`, 0, "")
@@ -426,8 +413,6 @@ func TestPRCheckout_detachedHead(t *testing.T) {
 
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
-
-	cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git")
 	cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "")
 	cs.Register(`git config branch\.feature\.merge`, 0, "refs/heads/feature\n")
 	cs.Register(`git checkout feature`, 0, "")
@@ -447,8 +432,6 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) {
 
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
-
-	cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git")
 	cs.Register(`git fetch origin refs/pull/123/head`, 0, "")
 	cs.Register(`git config branch\.feature\.merge`, 0, "refs/heads/feature\n")
 	cs.Register(`git merge --ff-only FETCH_HEAD`, 0, "")
@@ -468,7 +451,6 @@ func TestPRCheckout_differentRepo_invalidBranchName(t *testing.T) {
 
 	_, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
-
 	output, err := runCommand(http, nil, "master", `123`)
 	assert.EqualError(t, err, `invalid branch name: "-foo"`)
 	assert.Equal(t, "", output.Stderr())
@@ -485,8 +467,6 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) {
 
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
-
-	cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git")
 	cs.Register(`git fetch origin refs/pull/123/head:feature`, 0, "")
 	cs.Register(`git config branch\.feature\.merge`, 1, "")
 	cs.Register(`git checkout feature`, 0, "")
@@ -508,8 +488,6 @@ func TestPRCheckout_recurseSubmodules(t *testing.T) {
 
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
-
-	cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git")
 	cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "")
 	cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "")
 	cs.Register(`git checkout feature`, 0, "")
@@ -531,8 +509,6 @@ func TestPRCheckout_force(t *testing.T) {
 
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
-
-	cs.Register(`git remote get-url origin`, 0, "https://github.com/OWNER/REPO.git")
 	cs.Register(`git fetch origin \+refs/heads/feature:refs/remotes/origin/feature`, 0, "")
 	cs.Register(`git show-ref --verify -- refs/heads/feature`, 0, "")
 	cs.Register(`git checkout feature`, 0, "")
@@ -554,9 +530,7 @@ func TestPRCheckout_detach(t *testing.T) {
 
 	cs, cmdTeardown := run.Stub()
 	defer cmdTeardown(t)
-
 	cs.Register(`git checkout --detach FETCH_HEAD`, 0, "")
-	cs.Register(`git remote get-url origin`, 0, "https://github.com/hubot/REPO.git")
 	cs.Register(`git fetch origin refs/pull/123/head`, 0, "")
 
 	output, err := runCommand(http, nil, "", `123 --detach`)