From 694e565384379f23db8f906f2caeae84bc2e26a9 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 2 Dec 2024 16:22:52 +0100 Subject: [PATCH] Fix PR checkout panic when base repo is not in remotes --- .../testdata/pr/pr-checkout-by-number.txtar | 33 +++++++++++++++++ .../pr/pr-checkout-with-url-from-fork.txtar | 37 +++++++++++++++++++ git/client.go | 35 ++++++------------ git/client_test.go | 36 ++++-------------- pkg/cmd/pr/checkout/checkout.go | 11 ++---- pkg/cmd/pr/checkout/checkout_test.go | 26 ------------- 6 files changed, 91 insertions(+), 87 deletions(-) create mode 100644 acceptance/testdata/pr/pr-checkout-by-number.txtar create mode 100644 acceptance/testdata/pr/pr-checkout-with-url-from-fork.txtar diff --git a/acceptance/testdata/pr/pr-checkout-by-number.txtar b/acceptance/testdata/pr/pr-checkout-by-number.txtar new file mode 100644 index 000000000..374926f1d --- /dev/null +++ b/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''' diff --git a/acceptance/testdata/pr/pr-checkout-with-url-from-fork.txtar b/acceptance/testdata/pr/pr-checkout-with-url-from-fork.txtar new file mode 100644 index 000000000..9a0494f4b --- /dev/null +++ b/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''' diff --git a/git/client.go b/git/client.go index b2cfbce45..1dea7a6d6 100644 --- a/git/client.go +++ b/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...) diff --git a/git/client_test.go b/git/client_test.go index 41b651d0a..0fb7953bc 100644 --- a/git/client_test.go +++ b/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) }) } } diff --git a/pkg/cmd/pr/checkout/checkout.go b/pkg/cmd/pr/checkout/checkout.go index c4549d89b..a26fa9183 100644 --- a/pkg/cmd/pr/checkout/checkout.go +++ b/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 } diff --git a/pkg/cmd/pr/checkout/checkout_test.go b/pkg/cmd/pr/checkout/checkout_test.go index 32f7b5b80..f38671236 100644 --- a/pkg/cmd/pr/checkout/checkout_test.go +++ b/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`)