Merge pull request #10354 from cli/jtmcg/fix-10352-not-in-repo-error

Hotfix: `gh pr view` fails with provided URL

acceptance/testdata/pr/pr-view-outside-repo.txtar

@@ -0,0 +1,28 @@
+# 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/$SCRIPT_NAME-$RANDOM_STRING --add-readme --private
+
+# Defer repo cleanup
+defer gh repo delete --yes $ORG/$SCRIPT_NAME-$RANDOM_STRING
+
+# Clone the repo
+exec gh repo clone $ORG/$SCRIPT_NAME-$RANDOM_STRING
+
+# Prepare a branch to PR
+cd $SCRIPT_NAME-$RANDOM_STRING
+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
+
+# Change directories
+cd ..
+
+# View the PR
+exec gh pr view $PR_URL
+stdout 'Feature Title'

git/command_test.go

@@ -0,0 +1,104 @@
+package git
+
+import (
+	"fmt"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestOutput(t *testing.T) {
+	tests := []struct {
+		name     string
+		exitCode int
+		stdout   string
+		stderr   string
+		wantErr  *GitError
+	}{
+		{
+			name:     "successful command",
+			stdout:   "hello world",
+			stderr:   "",
+			exitCode: 0,
+			wantErr:  nil,
+		},
+		{
+			name:     "not a repo failure",
+			stdout:   "",
+			stderr:   "fatal: not a git repository (or any of the parent directories): .git",
+			exitCode: 128,
+			wantErr: &GitError{
+				ExitCode: 128,
+				Stderr:   "fatal: not a git repository (or any of the parent directories): .git",
+				err:      &exec.ExitError{},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+
+			cmd := Command{
+				&exec.Cmd{
+					Path: createMockExecutable(t, tt.stdout, tt.stderr, tt.exitCode),
+				},
+			}
+
+			out, err := cmd.Output()
+			if tt.wantErr != nil {
+				require.Error(t, err)
+				var gitError *GitError
+				require.ErrorAs(t, err, &gitError)
+				assert.Equal(t, tt.wantErr.ExitCode, gitError.ExitCode)
+				assert.Equal(t, tt.wantErr.Stderr, gitError.Stderr)
+				assert.Equal(t, tt.wantErr.Error(), gitError.Error())
+			} else {
+				require.NoError(t, err)
+			}
+			assert.Equal(t, tt.stdout, string(out))
+		})
+	}
+}
+
+func createMockExecutable(t *testing.T, stdout string, stderr string, exitCode int) string {
+	tmpDir := t.TempDir()
+	sourcePath := filepath.Join(tmpDir, "main.go")
+	binaryPath := filepath.Join(tmpDir, "mockexec")
+	if runtime.GOOS == "windows" {
+		binaryPath += ".exe"
+	}
+
+	// Create Go source
+	source := buildCommandSourceCode(exitCode, stdout, stderr)
+
+	// Write source file
+	if err := os.WriteFile(sourcePath, []byte(source), 0600); err != nil {
+		t.Fatal(err)
+	}
+
+	// Compile
+	cmd := exec.Command("go", "build", "-o", binaryPath, sourcePath)
+	if out, err := cmd.CombinedOutput(); err != nil {
+		t.Fatalf("failed to compile: %v\n%s", err, out)
+	}
+	return binaryPath
+
+}
+
+func buildCommandSourceCode(exitCode int, stdout, stderr string) string {
+	return fmt.Sprintf(`package main
+	import (
+		   "fmt"
+		"os"
+	)
+	func main() {
+		fmt.Printf(%q)
+		fmt.Fprintf(os.Stderr, %q)
+		os.Exit(%d)
+	}`, stdout, stderr, exitCode)
+}

pkg/cmd/pr/shared/finder.go

@@ -120,6 +120,7 @@ func (s *PullRequestRefs) GetPRHeadLabel() string {
 }
 
 func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, error) {
+	// If we have a URL, we don't need git stuff
 	if len(opts.Fields) == 0 {
 		return nil, nil, errors.New("Find error: no fields specified")
 	}
@@ -137,37 +138,74 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err
 		f.baseRefRepo = repo
 	}
 
-	if f.prNumber == 0 && opts.Selector != "" {
-		// If opts.Selector is a valid number then assume it is the
-		// PR number unless opts.BaseBranch is specified. This is a
-		// special case for PR create command which will always want
-		// to assume that a numerical selector is a branch name rather
-		// than PR number.
-		prNumber, err := strconv.Atoi(strings.TrimPrefix(opts.Selector, "#"))
-		if opts.BaseBranch == "" && err == nil {
-			f.prNumber = prNumber
-		} else {
-			f.branchName = opts.Selector
-		}
-	} else {
+	var prRefs PullRequestRefs
+	if opts.Selector == "" {
+		// You must be in a git repo for this case to work
 		currentBranchName, err := f.branchFn()
 		if err != nil {
 			return nil, nil, err
 		}
 		f.branchName = currentBranchName
-	}
 
-	// Get the branch config for the current branchName
-	branchConfig, err := f.branchConfig(f.branchName)
-	if err != nil {
-		return nil, nil, err
-	}
+		// Get the branch config for the current branchName
+		branchConfig, err := f.branchConfig(f.branchName)
+		if err != nil {
+			return nil, nil, err
+		}
 
-	// Determine if the branch is configured to merge to a special PR ref
-	prHeadRE := regexp.MustCompile(`^refs/pull/(\d+)/head$`)
-	if m := prHeadRE.FindStringSubmatch(branchConfig.MergeRef); m != nil {
-		prNumber, _ := strconv.Atoi(m[1])
-		f.prNumber = prNumber
+		// Determine if the branch is configured to merge to a special PR ref
+		prHeadRE := regexp.MustCompile(`^refs/pull/(\d+)/head$`)
+		if m := prHeadRE.FindStringSubmatch(branchConfig.MergeRef); m != nil {
+			prNumber, _ := strconv.Atoi(m[1])
+			f.prNumber = prNumber
+		}
+
+		// Determine the PullRequestRefs from config
+		if f.prNumber == 0 {
+			rems, err := f.remotesFn()
+			if err != nil {
+				return nil, nil, err
+			}
+
+			// Suppressing these errors as we have other means of computing the PullRequestRefs when these fail.
+			parsedPushRevision, _ := f.parsePushRevision(f.branchName)
+
+			pushDefault, err := f.pushDefault()
+			if err != nil {
+				return nil, nil, err
+			}
+
+			remotePushDefault, err := f.remotePushDefault()
+			if err != nil {
+				return nil, nil, err
+			}
+
+			prRefs, err = ParsePRRefs(f.branchName, branchConfig, parsedPushRevision, pushDefault, remotePushDefault, f.baseRefRepo, rems)
+			if err != nil {
+				return nil, nil, err
+			}
+		}
+
+	} else if f.prNumber == 0 {
+		// You gave me a selector but I couldn't find a PR number (it wasn't a URL)
+
+		// Try to get a PR number from the selector
+		prNumber, err := strconv.Atoi(strings.TrimPrefix(opts.Selector, "#"))
+		// If opts.Selector is a valid number then assume it is the
+		// PR number unless opts.BaseBranch is specified. This is a
+		// special case for PR create command which will always want
+		// to assume that a numerical selector is a branch name rather
+		// than PR number.
+		if opts.BaseBranch == "" && err == nil {
+			f.prNumber = prNumber
+		} else {
+			f.branchName = opts.Selector
+			// We don't expect an error here because parsedPushRevision is empty
+			prRefs, err = ParsePRRefs(f.branchName, git.BranchConfig{}, "", "", "", f.baseRefRepo, remotes.Remotes{})
+			if err != nil {
+				return nil, nil, err
+			}
+		}
 	}
 
 	// Set up HTTP client
@@ -217,29 +255,6 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err
 			return pr, f.baseRefRepo, err
 		}
 	} else {
-		rems, err := f.remotesFn()
-		if err != nil {
-			return nil, nil, err
-		}
-
-		pushDefault, err := f.pushDefault()
-		if err != nil {
-			return nil, nil, err
-		}
-
-		// Suppressing these errors as we have other means of computing the PullRequestRefs when these fail.
-		parsedPushRevision, _ := f.parsePushRevision(f.branchName)
-
-		remotePushDefault, err := f.remotePushDefault()
-		if err != nil {
-			return nil, nil, err
-		}
-
-		prRefs, err := ParsePRRefs(f.branchName, branchConfig, parsedPushRevision, pushDefault, remotePushDefault, f.baseRefRepo, rems)
-		if err != nil {
-			return nil, nil, err
-		}
-
 		pr, err = findForBranch(httpClient, f.baseRefRepo, opts.BaseBranch, prRefs.GetPRHeadLabel(), opts.States, fields.ToSlice())
 		if err != nil {
 			return pr, f.baseRefRepo, err

pkg/cmd/pr/shared/finder_test.go

@@ -211,6 +211,38 @@ func TestFind(t *testing.T) {
 			wantPR:   13,
 			wantRepo: "https://example.org/OWNER/REPO",
 		},
+		{
+			name: "PR URL argument and not in a local git repo",
+			args: args{
+				selector:   "https://example.org/OWNER/REPO/pull/13/files",
+				fields:     []string{"id", "number"},
+				baseRepoFn: nil,
+				branchFn: func() (string, error) {
+					return "", &git.GitError{
+						Stderr:   "fatal: branchFn error",
+						ExitCode: 128,
+					}
+				},
+				branchConfig: stubBranchConfig(git.BranchConfig{}, &git.GitError{
+					Stderr:   "fatal: branchConfig error",
+					ExitCode: 128,
+				}),
+				pushDefault: stubPushDefault("simple", nil),
+				remotePushDefault: stubRemotePushDefault("", &git.GitError{
+					Stderr:   "fatal: remotePushDefault error",
+					ExitCode: 128,
+				}),
+			},
+			httpStub: func(r *httpmock.Registry) {
+				r.Register(
+					httpmock.GraphQL(`query PullRequestByNumber\b`),
+					httpmock.StringResponse(`{"data":{"repository":{
+						"pullRequest":{"number":13}
+					}}}`))
+			},
+			wantPR:   13,
+			wantRepo: "https://example.org/OWNER/REPO",
+		},
 		{
 			name: "when provided branch argument with an open and closed PR for that branch name, it returns the open PR",
 			args: args{

pkg/cmd/pr/status/status.go

@@ -87,6 +87,7 @@ func statusRun(opts *StatusOptions) error {
 	var currentHeadRefBranchName string
 
 	if !opts.HasRepoOverride {
+		// We must be in a repo without the override
 		currentBranchName, err = opts.Branch()
 		if err != nil && !errors.Is(err, git.ErrNotOnAnyBranch) {
 			return fmt.Errorf("could not query for pull request for current branch: %w", err)