diff --git a/api/queries_pr.go b/api/queries_pr.go index 642e83c34..b2fade857 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -4,7 +4,7 @@ import ( "context" "errors" "fmt" - "io/ioutil" + "io" "net/http" "strings" @@ -209,36 +209,28 @@ func (pr *PullRequest) ChecksStatus() (summary PullRequestChecksStatus) { return } -func (c Client) PullRequestDiff(baseRepo ghrepo.Interface, prNumber int) (string, error) { +func (c Client) PullRequestDiff(baseRepo ghrepo.Interface, prNumber int) (io.ReadCloser, error) { url := fmt.Sprintf("https://api.github.com/repos/%s/pulls/%d", ghrepo.FullName(baseRepo), prNumber) req, err := http.NewRequest("GET", url, nil) if err != nil { - return "", err + return nil, err } req.Header.Set("Accept", "application/vnd.github.v3.diff; charset=utf-8") resp, err := c.http.Do(req) if err != nil { - return "", err - } - defer resp.Body.Close() - - b, err := ioutil.ReadAll(resp.Body) - if err != nil { - return "", err - } - - if resp.StatusCode == 200 { - return string(b), nil + return nil, err } if resp.StatusCode == 404 { - return "", &NotFoundError{errors.New("pull request not found")} + return nil, &NotFoundError{errors.New("pull request not found")} + } else if resp.StatusCode != 200 { + return nil, handleHTTPError(resp) } - return "", errors.New("pull request diff lookup failed") + return resp.Body, nil } func PullRequests(client *Client, repo ghrepo.Interface, currentPRNumber int, currentPRHeadRef, currentUsername string) (*PullRequestsPayload, error) { diff --git a/api/queries_repo.go b/api/queries_repo.go index 024520882..e19bc1cf6 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -27,9 +27,7 @@ type Repository struct { IsPrivate bool HasIssuesEnabled bool ViewerPermission string - DefaultBranchRef struct { - Name string - } + DefaultBranchRef BranchRef Parent *Repository @@ -42,6 +40,11 @@ type RepositoryOwner struct { Login string } +// BranchRef is the branch name in a GitHub repository +type BranchRef struct { + Name string +} + // RepoOwner is the login name of the owner func (r Repository) RepoOwner() string { return r.Owner.Login diff --git a/command/pr.go b/command/pr.go index 441f49935..2090b4dc1 100644 --- a/command/pr.go +++ b/command/pr.go @@ -27,7 +27,6 @@ func init() { prCmd.PersistentFlags().StringP("repo", "R", "", "Select another repository using the `OWNER/REPO` format") RootCmd.AddCommand(prCmd) - prCmd.AddCommand(prCheckoutCmd) prCmd.AddCommand(prCreateCmd) prCmd.AddCommand(prStatusCmd) prCmd.AddCommand(prCloseCmd) diff --git a/command/pr_diff.go b/command/pr_diff.go deleted file mode 100644 index ae50a2618..000000000 --- a/command/pr_diff.go +++ /dev/null @@ -1,104 +0,0 @@ -package command - -import ( - "fmt" - "os" - "strings" - - "github.com/cli/cli/utils" - "github.com/spf13/cobra" -) - -var prDiffCmd = &cobra.Command{ - Use: "diff { | }", - Short: "View a pull request's changes.", - RunE: prDiff, -} - -func init() { - prDiffCmd.Flags().StringP("color", "c", "auto", "Whether or not to output color: {always|never|auto}") - - prCmd.AddCommand(prDiffCmd) -} - -func prDiff(cmd *cobra.Command, args []string) error { - color, err := cmd.Flags().GetString("color") - if err != nil { - return err - } - if !validColorFlag(color) { - return fmt.Errorf("did not understand color: %q. Expected one of always, never, or auto", color) - } - - ctx := contextForCommand(cmd) - apiClient, err := apiClientForContext(ctx) - if err != nil { - return err - } - - pr, baseRepo, err := prFromArgs(ctx, apiClient, cmd, args) - if err != nil { - return fmt.Errorf("could not find pull request: %w", err) - } - - diff, err := apiClient.PullRequestDiff(baseRepo, pr.Number) - if err != nil { - return fmt.Errorf("could not find pull request diff: %w", err) - } - - out := cmd.OutOrStdout() - if color == "auto" { - color = "never" - isTTY := false - if outFile, isFile := out.(*os.File); isFile { - isTTY = utils.IsTerminal(outFile) - if isTTY { - color = "always" - } - } - } - - if color == "never" { - fmt.Fprint(out, diff) - return nil - } - - out = colorableOut(cmd) - for _, diffLine := range strings.Split(diff, "\n") { - output := diffLine - switch { - case isHeaderLine(diffLine): - output = utils.Bold(diffLine) - case isAdditionLine(diffLine): - output = utils.Green(diffLine) - case isRemovalLine(diffLine): - output = utils.Red(diffLine) - } - - fmt.Fprintln(out, output) - } - - return nil -} - -func isHeaderLine(dl string) bool { - prefixes := []string{"+++", "---", "diff", "index"} - for _, p := range prefixes { - if strings.HasPrefix(dl, p) { - return true - } - } - return false -} - -func isAdditionLine(dl string) bool { - return strings.HasPrefix(dl, "+") -} - -func isRemovalLine(dl string) bool { - return strings.HasPrefix(dl, "-") -} - -func validColorFlag(c string) bool { - return c == "auto" || c == "always" || c == "never" -} diff --git a/command/pr_diff_test.go b/command/pr_diff_test.go deleted file mode 100644 index 725b21f03..000000000 --- a/command/pr_diff_test.go +++ /dev/null @@ -1,104 +0,0 @@ -package command - -import ( - "bytes" - "testing" -) - -func TestPRDiff_validation(t *testing.T) { - _, err := RunCommand("pr diff --color=doublerainbow") - if err == nil { - t.Fatal("expected error") - } - eq(t, err.Error(), `did not understand color: "doublerainbow". Expected one of always, never, or auto`) -} - -func TestPRDiff_no_current_pr(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { "pullRequests": { "nodes": [ - { "url": "https://github.com/OWNER/REPO/pull/123", - "number": 123, - "id": "foobar123", - "headRefName": "feature", - "baseRefName": "master" } - ] } } } }`)) - http.StubResponse(200, bytes.NewBufferString(testDiff)) - _, err := RunCommand("pr diff") - if err == nil { - t.Fatal("expected error", err) - } - eq(t, err.Error(), `could not find pull request: no open pull requests found for branch "master"`) -} - -func TestPRDiff_argument_not_found(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 123 } - } } } -`)) - http.StubResponse(404, bytes.NewBufferString("")) - _, err := RunCommand("pr diff 123") - if err == nil { - t.Fatal("expected error", err) - } - eq(t, err.Error(), `could not find pull request diff: pull request not found`) -} - -func TestPRDiff(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", - "number": 123, - "id": "foobar123", - "headRefName": "feature", - "baseRefName": "master" } - ] } } } }`)) - http.StubResponse(200, bytes.NewBufferString(testDiff)) - output, err := RunCommand("pr diff") - if err != nil { - t.Fatalf("unexpected error: %s", err) - } - eq(t, output.String(), testDiff) -} - -const testDiff = `diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml -index 73974448..b7fc0154 100644 ---- a/.github/workflows/releases.yml -+++ b/.github/workflows/releases.yml -@@ -44,6 +44,11 @@ jobs: - token: ${{secrets.SITE_GITHUB_TOKEN}} - - name: Publish documentation site - if: "!contains(github.ref, '-')" # skip prereleases -+ env: -+ GIT_COMMITTER_NAME: cli automation -+ GIT_AUTHOR_NAME: cli automation -+ GIT_COMMITTER_EMAIL: noreply@github.com -+ GIT_AUTHOR_EMAIL: noreply@github.com - run: make site-publish - - name: Move project cards - if: "!contains(github.ref, '-')" # skip prereleases -diff --git a/Makefile b/Makefile -index f2b4805c..3d7bd0f9 100644 ---- a/Makefile -+++ b/Makefile -@@ -22,8 +22,8 @@ test: - go test ./... - .PHONY: test - --site: -- git clone https://github.com/github/cli.github.com.git "$@" -+site: bin/gh -+ bin/gh repo clone github/cli.github.com "$@" - - site-docs: site - git -C site pull -` diff --git a/command/pr_lookup.go b/command/pr_lookup.go index cf59f0717..727259f7b 100644 --- a/command/pr_lookup.go +++ b/command/pr_lookup.go @@ -2,120 +2,33 @@ package command import ( "fmt" - "net/url" - "regexp" - "strconv" - "strings" "github.com/cli/cli/api" "github.com/cli/cli/context" - "github.com/cli/cli/git" "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmd/pr/shared" "github.com/spf13/cobra" ) func prFromArgs(ctx context.Context, apiClient *api.Client, cmd *cobra.Command, args []string) (*api.PullRequest, ghrepo.Interface, error) { - if len(args) == 1 { - // First check to see if the prString is a url, return repo from url if found. This - // is run first because we don't need to run determineBaseRepo for this path - prString := args[0] - pr, r, err := prFromURL(ctx, apiClient, prString) - if pr != nil || err != nil { - return pr, r, err - } + var arg string + if len(args) > 0 { + arg = args[0] } - repo, err := determineBaseRepo(apiClient, cmd, ctx) - if err != nil { - return nil, nil, fmt.Errorf("could not determine base repo: %w", err) - } - - // If there are no args see if we can guess the PR from the current branch - if len(args) == 0 { - pr, err := prForCurrentBranch(ctx, apiClient, repo) - return pr, repo, err - } else { - prString := args[0] - // Next see if the prString is a number and use that to look up the url - pr, err := prFromNumberString(ctx, apiClient, repo, prString) - if pr != nil || err != nil { - return pr, repo, err - } - - // Last see if it is a branch name - pr, err = api.PullRequestForBranch(apiClient, repo, "", prString) - return pr, repo, err - } -} - -func prFromNumberString(ctx context.Context, apiClient *api.Client, repo ghrepo.Interface, s string) (*api.PullRequest, error) { - if prNumber, err := strconv.Atoi(strings.TrimPrefix(s, "#")); err == nil { - return api.PullRequestByNumber(apiClient, repo, prNumber) - } - - return nil, nil -} - -var pullURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/pull/(\d+)`) - -func prFromURL(ctx context.Context, apiClient *api.Client, s string) (*api.PullRequest, ghrepo.Interface, error) { - u, err := url.Parse(s) - if err != nil { - return nil, nil, nil - } - - if u.Scheme != "https" && u.Scheme != "http" { - return nil, nil, nil - } - - m := pullURLRE.FindStringSubmatch(u.Path) - if m == nil { - return nil, nil, nil - } - - repo := ghrepo.NewWithHost(m[1], m[2], u.Hostname()) - prNumberString := m[3] - pr, err := prFromNumberString(ctx, apiClient, repo, prNumberString) - return pr, repo, err -} - -func prForCurrentBranch(ctx context.Context, apiClient *api.Client, repo ghrepo.Interface) (*api.PullRequest, error) { - prHeadRef, err := ctx.Branch() - if err != nil { - return nil, err - } - - branchConfig := git.ReadBranchConfig(prHeadRef) - - // the branch is configured to merge a special PR head ref - prHeadRE := regexp.MustCompile(`^refs/pull/(\d+)/head$`) - if m := prHeadRE.FindStringSubmatch(branchConfig.MergeRef); m != nil { - return prFromNumberString(ctx, apiClient, repo, m[1]) - } - - var branchOwner string - if branchConfig.RemoteURL != nil { - // the branch merges from a remote specified by URL - if r, err := ghrepo.FromURL(branchConfig.RemoteURL); err == nil { - branchOwner = r.RepoOwner() - } - } else if branchConfig.RemoteName != "" { - // the branch merges from a remote specified by name - rem, _ := ctx.Remotes() - if r, err := rem.FindByName(branchConfig.RemoteName); err == nil { - branchOwner = r.RepoOwner() - } - } - - if branchOwner != "" { - if strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { - prHeadRef = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") - } - // prepend `OWNER:` if this branch is pushed to a fork - if !strings.EqualFold(branchOwner, repo.RepoOwner()) { - prHeadRef = fmt.Sprintf("%s:%s", branchOwner, prHeadRef) - } - } - - return api.PullRequestForBranch(apiClient, repo, "", prHeadRef) + return shared.PRFromArgs( + apiClient, + func() (ghrepo.Interface, error) { + repo, err := determineBaseRepo(apiClient, cmd, ctx) + if err != nil { + return nil, fmt.Errorf("could not determine base repo: %w", err) + } + return repo, nil + }, + func() (string, error) { + return ctx.Branch() + }, + func() (context.Remotes, error) { + return ctx.Remotes() + }, arg) } diff --git a/command/root.go b/command/root.go index 6020d1063..452db221b 100644 --- a/command/root.go +++ b/command/root.go @@ -16,12 +16,16 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/api" "github.com/cli/cli/context" + "github.com/cli/cli/git" "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghinstance" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" apiCmd "github.com/cli/cli/pkg/cmd/api" gistCreateCmd "github.com/cli/cli/pkg/cmd/gist/create" + prCheckoutCmd "github.com/cli/cli/pkg/cmd/pr/checkout" + prDiffCmd "github.com/cli/cli/pkg/cmd/pr/diff" + prReviewCmd "github.com/cli/cli/pkg/cmd/pr/review" repoCmd "github.com/cli/cli/pkg/cmd/repo" repoCloneCmd "github.com/cli/cli/pkg/cmd/repo/clone" repoCreateCmd "github.com/cli/cli/pkg/cmd/repo/create" @@ -112,6 +116,13 @@ func init() { } return cfg, nil }, + Branch: func() (string, error) { + currentBranch, err := git.CurrentBranch() + if err != nil { + return "", fmt.Errorf("could not determine current branch: %w", err) + } + return currentBranch, nil + }, } RootCmd.AddCommand(apiCmd.NewCmdApi(cmdFactory, nil)) @@ -159,6 +170,10 @@ func init() { repoCmd.Cmd.AddCommand(repoCreateCmd.NewCmdCreate(cmdFactory, nil)) repoCmd.Cmd.AddCommand(creditsCmd.NewCmdRepoCredits(&repoResolvingCmdFactory, nil)) + prCmd.AddCommand(prReviewCmd.NewCmdReview(&repoResolvingCmdFactory, nil)) + prCmd.AddCommand(prDiffCmd.NewCmdDiff(&repoResolvingCmdFactory, nil)) + prCmd.AddCommand(prCheckoutCmd.NewCmdCheckout(&repoResolvingCmdFactory, nil)) + RootCmd.AddCommand(creditsCmd.NewCmdCredits(cmdFactory, nil)) } @@ -393,6 +408,7 @@ func formatRemoteURL(cmd *cobra.Command, repo ghrepo.Interface) string { return fmt.Sprintf("https://%s/%s/%s.git", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) } +// TODO there is a parallel implementation for isolated commands func determineEditor(cmd *cobra.Command) (string, error) { editorCommand := os.Getenv("GH_EDITOR") if editorCommand == "" { diff --git a/command/root_test.go b/command/root_test.go index 7d93169a4..c0b1fd7d3 100644 --- a/command/root_test.go +++ b/command/root_test.go @@ -2,8 +2,6 @@ package command import ( "testing" - - "github.com/cli/cli/internal/ghrepo" ) func TestChangelogURL(t *testing.T) { @@ -42,22 +40,3 @@ func TestChangelogURL(t *testing.T) { t.Errorf("expected %s to create url %s but got %s", tag, url, result) } } - -func TestRemoteURLFormatting_no_config(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - result := formatRemoteURL(prCheckoutCmd, ghrepo.New("OWNER", "REPO")) - eq(t, result, "https://github.com/OWNER/REPO.git") -} - -func TestRemoteURLFormatting_ssh_config(t *testing.T) { - cfg := `--- -hosts: - github.com: - user: OWNER - oauth_token: MUSTBEHIGHCUZIMATOKEN -git_protocol: ssh -` - initBlankContext(cfg, "OWNER/REPO", "master") - result := formatRemoteURL(prCheckoutCmd, ghrepo.New("OWNER", "REPO")) - eq(t, result, "git@github.com:OWNER/REPO.git") -} diff --git a/command/testing.go b/command/testing.go index e12cc38e3..367d510bb 100644 --- a/command/testing.go +++ b/command/testing.go @@ -2,7 +2,6 @@ package command import ( "bytes" - "errors" "fmt" "github.com/cli/cli/api" @@ -102,18 +101,6 @@ func RunCommand(args string) (*cmdOut, error) { return &cmdOut{&outBuf, &errBuf}, err } -type errorStub struct { - message string -} - -func (s errorStub) Output() ([]byte, error) { - return nil, errors.New(s.message) -} - -func (s errorStub) Run() error { - return errors.New(s.message) -} - func stubTerminal(connected bool) func() { isTerminal := utils.IsTerminal utils.IsTerminal = func(_ interface{}) bool { diff --git a/command/pr_checkout.go b/pkg/cmd/pr/checkout/checkout.go similarity index 65% rename from command/pr_checkout.go rename to pkg/cmd/pr/checkout/checkout.go index 256a3dfe8..20eef580c 100644 --- a/command/pr_checkout.go +++ b/pkg/cmd/pr/checkout/checkout.go @@ -1,41 +1,101 @@ -package command +package checkout import ( "errors" "fmt" + "net/http" "os" "os/exec" "strings" - "github.com/spf13/cobra" - "github.com/cli/cli/api" + "github.com/cli/cli/context" "github.com/cli/cli/git" + "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" + "github.com/cli/cli/pkg/cmd/pr/shared" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" + "github.com/spf13/cobra" ) -func prCheckout(cmd *cobra.Command, args []string) error { - ctx := contextForCommand(cmd) - currentBranch, _ := ctx.Branch() - remotes, err := ctx.Remotes() +type CheckoutOptions struct { + HttpClient func() (*http.Client, error) + Config func() (config.Config, error) + IO *iostreams.IOStreams + BaseRepo func() (ghrepo.Interface, error) + Remotes func() (context.Remotes, error) + Branch func() (string, error) + + SelectorArg string +} + +func NewCmdCheckout(f *cmdutil.Factory, runF func(*CheckoutOptions) error) *cobra.Command { + opts := &CheckoutOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + Config: f.Config, + BaseRepo: f.BaseRepo, + Remotes: f.Remotes, + Branch: f.Branch, + } + + cmd := &cobra.Command{ + Use: "checkout { | | }", + Short: "Check out a pull request in git", + Args: func(cmd *cobra.Command, args []string) error { + if len(args) == 0 { + return &cmdutil.FlagError{Err: errors.New("argument required")} + } + return nil + }, + RunE: func(cmd *cobra.Command, args []string) error { + if len(args) > 0 { + opts.SelectorArg = args[0] + } + + if runF != nil { + return runF(opts) + } + return checkoutRun(opts) + }, + } + + return cmd +} + +func checkoutRun(opts *CheckoutOptions) error { + currentBranch, err := opts.Branch() if err != nil { return err } - apiClient, err := apiClientForContext(ctx) + remotes, err := opts.Remotes() if err != nil { return err } - pr, baseRepo, err := prFromArgs(ctx, apiClient, cmd, args) + httpClient, err := opts.HttpClient() if err != nil { return err } + apiClient := api.NewClientFromHTTP(httpClient) + + pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg) + if err != nil { + return err + } + + cfg, err := opts.Config() + if err != nil { + return err + } + protocol, _ := cfg.Get(baseRepo.RepoHost(), "git_protocol") baseRemote, _ := remotes.FindByRepo(baseRepo.RepoOwner(), baseRepo.RepoName()) // baseRemoteSpec is a repository URL or a remote name to be used in git fetch - baseURLOrName := formatRemoteURL(cmd, baseRepo) + baseURLOrName := ghrepo.FormatRemoteURL(baseRepo, protocol) if baseRemote != nil { baseURLOrName = baseRemote.Name } @@ -95,7 +155,7 @@ func prCheckout(cmd *cobra.Command, args []string) error { mergeRef := ref if pr.MaintainerCanModify { headRepo := ghrepo.NewWithHost(pr.HeadRepositoryOwner.Login, pr.HeadRepository.Name, baseRepo.RepoHost()) - remote = formatRemoteURL(cmd, headRepo) + remote = ghrepo.FormatRemoteURL(headRepo, protocol) mergeRef = fmt.Sprintf("refs/heads/%s", pr.HeadRefName) } if mc, err := git.Config(fmt.Sprintf("branch.%s.merge", newBranchName)); err != nil || mc == "" { @@ -115,15 +175,3 @@ func prCheckout(cmd *cobra.Command, args []string) error { return nil } - -var prCheckoutCmd = &cobra.Command{ - Use: "checkout { | | }", - Short: "Check out a pull request in Git", - Args: func(cmd *cobra.Command, args []string) error { - if len(args) < 1 { - return errors.New("requires a pull request number as an argument") - } - return nil - }, - RunE: prCheckout, -} diff --git a/command/pr_checkout_test.go b/pkg/cmd/pr/checkout/checkout_test.go similarity index 79% rename from command/pr_checkout_test.go rename to pkg/cmd/pr/checkout/checkout_test.go index 525225802..847702c77 100644 --- a/command/pr_checkout_test.go +++ b/pkg/cmd/pr/checkout/checkout_test.go @@ -1,31 +1,105 @@ -package command +package checkout import ( + "bytes" "encoding/json" + "errors" "io/ioutil" + "net/http" "os/exec" + "reflect" "strings" "testing" + "github.com/cli/cli/api" "github.com/cli/cli/context" + "github.com/cli/cli/git" + "github.com/cli/cli/internal/config" + "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" + "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/httpmock" + "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/test" + "github.com/google/shlex" "github.com/stretchr/testify/assert" ) -func TestPRCheckout_sameRepo(t *testing.T) { - ctx := context.NewBlank() - ctx.SetBranch("master") - ctx.SetRemotes(map[string]string{ - "origin": "OWNER/REPO", - }) - initContext = func() context.Context { - return ctx +func eq(t *testing.T, got interface{}, expected interface{}) { + t.Helper() + if !reflect.DeepEqual(got, expected) { + t.Errorf("expected: %v, got: %v", expected, got) } - http := initFakeHTTP() +} + +type errorStub struct { + message string +} + +func (s errorStub) Output() ([]byte, error) { + return nil, errors.New(s.message) +} + +func (s errorStub) Run() error { + return errors.New(s.message) +} + +func runCommand(rt http.RoundTripper, remotes context.Remotes, branch string, cli string) (*test.CmdOut, error) { + io, _, stdout, stderr := iostreams.Test() + + factory := &cmdutil.Factory{ + IOStreams: io, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: rt}, nil + }, + Config: func() (config.Config, error) { + return config.NewBlankConfig(), nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return api.InitRepoHostname(&api.Repository{ + Name: "REPO", + Owner: api.RepositoryOwner{Login: "OWNER"}, + DefaultBranchRef: api.BranchRef{Name: "master"}, + }, "github.com"), nil + }, + Remotes: func() (context.Remotes, error) { + if remotes == nil { + return context.Remotes{ + { + Remote: &git.Remote{Name: "origin"}, + Repo: ghrepo.New("OWNER", "REPO"), + }, + }, nil + } + return remotes, nil + }, + Branch: func() (string, error) { + return branch, nil + }, + } + + cmd := NewCmdCheckout(factory, nil) + + argv, err := shlex.Split(cli) + if err != nil { + return nil, err + } + cmd.SetArgs(argv) + + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(ioutil.Discard) + cmd.SetErr(ioutil.Discard) + + _, err = cmd.ExecuteC() + return &test.CmdOut{ + OutBuf: stdout, + ErrBuf: stderr, + }, err +} + +func TestPRCheckout_sameRepo(t *testing.T) { + http := &httpmock.Registry{} defer http.Verify(t) - http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { @@ -54,11 +128,15 @@ func TestPRCheckout_sameRepo(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(`pr checkout 123`) - eq(t, err, nil) - eq(t, output.String(), "") + output, err := runCommand(http, nil, "master", `123`) + if !assert.NoError(t, err) { + return + } - eq(t, len(ranCommands), 4) + assert.Equal(t, "", output.String()) + if !assert.Equal(t, 4, len(ranCommands)) { + return + } eq(t, strings.Join(ranCommands[0], " "), "git fetch origin +refs/heads/feature:refs/remotes/origin/feature") eq(t, strings.Join(ranCommands[1], " "), "git checkout -b feature --no-track origin/feature") eq(t, strings.Join(ranCommands[2], " "), "git config branch.feature.remote origin") @@ -66,15 +144,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { } func TestPRCheckout_urlArg(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 := &httpmock.Registry{} defer http.Verify(t) http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { @@ -103,7 +173,7 @@ func TestPRCheckout_urlArg(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(`pr checkout https://github.com/OWNER/REPO/pull/123/files`) + output, err := runCommand(http, nil, "master", `https://github.com/OWNER/REPO/pull/123/files`) eq(t, err, nil) eq(t, output.String(), "") @@ -112,15 +182,7 @@ func TestPRCheckout_urlArg(t *testing.T) { } 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 := &httpmock.Registry{} defer http.Verify(t) http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { @@ -154,7 +216,7 @@ func TestPRCheckout_urlArg_differentBase(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(`pr checkout https://github.com/OTHER/POE/pull/123/files`) + output, err := runCommand(http, nil, "master", `https://github.com/OTHER/POE/pull/123/files`) eq(t, err, nil) eq(t, output.String(), "") @@ -176,17 +238,8 @@ func TestPRCheckout_urlArg_differentBase(t *testing.T) { } func TestPRCheckout_branchArg(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 := &httpmock.Registry{} defer http.Verify(t) - http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestForBranch\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequests": { "nodes": [ @@ -215,7 +268,7 @@ func TestPRCheckout_branchArg(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(`pr checkout hubot:feature`) + output, err := runCommand(http, nil, "master", `hubot:feature`) eq(t, err, nil) eq(t, output.String(), "") @@ -224,17 +277,8 @@ func TestPRCheckout_branchArg(t *testing.T) { } func TestPRCheckout_existingBranch(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 := &httpmock.Registry{} defer http.Verify(t) - http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { @@ -263,7 +307,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(`pr checkout 123`) + output, err := runCommand(http, nil, "master", `123`) eq(t, err, nil) eq(t, output.String(), "") @@ -274,18 +318,19 @@ func TestPRCheckout_existingBranch(t *testing.T) { } func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { - ctx := context.NewBlank() - ctx.SetBranch("master") - ctx.SetRemotes(map[string]string{ - "origin": "OWNER/REPO", - "robot-fork": "hubot/REPO", - }) - initContext = func() context.Context { - return ctx + remotes := context.Remotes{ + { + Remote: &git.Remote{Name: "origin"}, + Repo: ghrepo.New("OWNER", "REPO"), + }, + { + Remote: &git.Remote{Name: "robot-fork"}, + Repo: ghrepo.New("hubot", "REPO"), + }, } - http := initFakeHTTP() + + http := &httpmock.Registry{} defer http.Verify(t) - http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { @@ -314,7 +359,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(`pr checkout 123`) + output, err := runCommand(http, remotes, "master", `123`) eq(t, err, nil) eq(t, output.String(), "") @@ -326,17 +371,8 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { } func TestPRCheckout_differentRepo(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 := &httpmock.Registry{} defer http.Verify(t) - http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { @@ -365,7 +401,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(`pr checkout 123`) + output, err := runCommand(http, nil, "master", `123`) eq(t, err, nil) eq(t, output.String(), "") @@ -377,17 +413,8 @@ func TestPRCheckout_differentRepo(t *testing.T) { } func TestPRCheckout_differentRepo_existingBranch(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 := &httpmock.Registry{} defer http.Verify(t) - http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { @@ -416,7 +443,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(`pr checkout 123`) + output, err := runCommand(http, nil, "master", `123`) eq(t, err, nil) eq(t, output.String(), "") @@ -426,17 +453,8 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { } func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { - ctx := context.NewBlank() - ctx.SetBranch("feature") - ctx.SetRemotes(map[string]string{ - "origin": "OWNER/REPO", - }) - initContext = func() context.Context { - return ctx - } - http := initFakeHTTP() + http := &httpmock.Registry{} defer http.Verify(t) - http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { @@ -465,7 +483,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(`pr checkout 123`) + output, err := runCommand(http, nil, "feature", `123`) eq(t, err, nil) eq(t, output.String(), "") @@ -475,17 +493,8 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { } func TestPRCheckout_differentRepo_invalidBranchName(t *testing.T) { - ctx := context.NewBlank() - ctx.SetBranch("feature") - ctx.SetRemotes(map[string]string{ - "origin": "OWNER/REPO", - }) - initContext = func() context.Context { - return ctx - } - http := initFakeHTTP() + http := &httpmock.Registry{} defer http.Verify(t) - http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { @@ -508,7 +517,7 @@ func TestPRCheckout_differentRepo_invalidBranchName(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(`pr checkout 123`) + output, err := runCommand(http, nil, "master", `123`) if assert.Errorf(t, err, "expected command to fail") { assert.Equal(t, `invalid branch name: "-foo"`, err.Error()) } @@ -516,17 +525,8 @@ func TestPRCheckout_differentRepo_invalidBranchName(t *testing.T) { } func TestPRCheckout_maintainerCanModify(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 := &httpmock.Registry{} defer http.Verify(t) - http.StubRepoResponse("OWNER", "REPO") http.Register(httpmock.GraphQL(`query PullRequestByNumber\b`), httpmock.StringResponse(` { "data": { "repository": { "pullRequest": { @@ -555,7 +555,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { }) defer restoreCmd() - output, err := RunCommand(`pr checkout 123`) + output, err := runCommand(http, nil, "master", `123`) eq(t, err, nil) eq(t, output.String(), "") diff --git a/pkg/cmd/pr/diff/diff.go b/pkg/cmd/pr/diff/diff.go new file mode 100644 index 000000000..c38e7c78b --- /dev/null +++ b/pkg/cmd/pr/diff/diff.go @@ -0,0 +1,134 @@ +package diff + +import ( + "bufio" + "fmt" + "io" + "net/http" + "strings" + + "github.com/cli/cli/api" + "github.com/cli/cli/context" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmd/pr/shared" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" + "github.com/spf13/cobra" +) + +type DiffOptions struct { + HttpClient func() (*http.Client, error) + IO *iostreams.IOStreams + BaseRepo func() (ghrepo.Interface, error) + Remotes func() (context.Remotes, error) + Branch func() (string, error) + + SelectorArg string + UseColor string +} + +func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Command { + opts := &DiffOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + BaseRepo: f.BaseRepo, + Remotes: f.Remotes, + Branch: f.Branch, + } + + cmd := &cobra.Command{ + Use: "diff [ | | ]", + Short: "View changes in a pull request", + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if len(args) > 0 { + opts.SelectorArg = args[0] + } + + if !validColorFlag(opts.UseColor) { + return &cmdutil.FlagError{Err: fmt.Errorf("did not understand color: %q. Expected one of always, never, or auto", opts.UseColor)} + } + + if opts.UseColor == "auto" && !opts.IO.IsStdoutTTY() { + opts.UseColor = "never" + } + + if runF != nil { + return runF(opts) + } + return diffRun(opts) + }, + } + + cmd.Flags().StringVar(&opts.UseColor, "color", "auto", "Use color in diff output: {always|never|auto}") + + return cmd +} + +func diffRun(opts *DiffOptions) error { + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + + pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg) + if err != nil { + return err + } + + diff, err := apiClient.PullRequestDiff(baseRepo, pr.Number) + if err != nil { + return fmt.Errorf("could not find pull request diff: %w", err) + } + defer diff.Close() + + if opts.UseColor == "never" { + _, err = io.Copy(opts.IO.Out, diff) + return err + } + + diffLines := bufio.NewScanner(diff) + for diffLines.Scan() { + diffLine := diffLines.Text() + switch { + case isHeaderLine(diffLine): + fmt.Fprintf(opts.IO.Out, "\x1b[1;38m%s\x1b[m\n", diffLine) + case isAdditionLine(diffLine): + fmt.Fprintf(opts.IO.Out, "\x1b[32m%s\x1b[m\n", diffLine) + case isRemovalLine(diffLine): + fmt.Fprintf(opts.IO.Out, "\x1b[31m%s\x1b[m\n", diffLine) + default: + fmt.Fprintln(opts.IO.Out, diffLine) + } + } + + if err := diffLines.Err(); err != nil { + return fmt.Errorf("error reading pull request diff: %w", err) + } + + return nil +} + +var diffHeaderPrefixes = []string{"+++", "---", "diff", "index"} + +func isHeaderLine(dl string) bool { + for _, p := range diffHeaderPrefixes { + if strings.HasPrefix(dl, p) { + return true + } + } + return false +} + +func isAdditionLine(dl string) bool { + return strings.HasPrefix(dl, "+") +} + +func isRemovalLine(dl string) bool { + return strings.HasPrefix(dl, "-") +} + +func validColorFlag(c string) bool { + return c == "auto" || c == "always" || c == "never" +} diff --git a/pkg/cmd/pr/diff/diff_test.go b/pkg/cmd/pr/diff/diff_test.go new file mode 100644 index 000000000..85c74cb7f --- /dev/null +++ b/pkg/cmd/pr/diff/diff_test.go @@ -0,0 +1,183 @@ +package diff + +import ( + "bytes" + "io/ioutil" + "net/http" + "testing" + + "github.com/cli/cli/context" + "github.com/cli/cli/git" + "github.com/cli/cli/internal/config" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/httpmock" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/test" + "github.com/google/go-cmp/cmp" + "github.com/google/shlex" + "github.com/stretchr/testify/assert" +) + +func runCommand(rt http.RoundTripper, remotes context.Remotes, isTTY bool, cli string) (*test.CmdOut, error) { + io, _, stdout, stderr := iostreams.Test() + io.SetStdoutTTY(isTTY) + io.SetStdinTTY(isTTY) + io.SetStderrTTY(isTTY) + + factory := &cmdutil.Factory{ + IOStreams: io, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: rt}, nil + }, + Config: func() (config.Config, error) { + return config.NewBlankConfig(), nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + Remotes: func() (context.Remotes, error) { + if remotes == nil { + return context.Remotes{ + { + Remote: &git.Remote{Name: "origin"}, + Repo: ghrepo.New("OWNER", "REPO"), + }, + }, nil + } + + return remotes, nil + }, + Branch: func() (string, error) { + return "feature", nil + }, + } + + cmd := NewCmdDiff(factory, nil) + + argv, err := shlex.Split(cli) + if err != nil { + return nil, err + } + cmd.SetArgs(argv) + + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(ioutil.Discard) + cmd.SetErr(ioutil.Discard) + + _, err = cmd.ExecuteC() + return &test.CmdOut{ + OutBuf: stdout, + ErrBuf: stderr, + }, err +} + +func TestPRDiff_validation(t *testing.T) { + _, err := runCommand(nil, nil, false, "--color=doublerainbow") + if err == nil { + t.Fatal("expected error") + } + assert.Equal(t, `did not understand color: "doublerainbow". Expected one of always, never, or auto`, err.Error()) +} + +func TestPRDiff_no_current_pr(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [] } } } } + `)) + _, err := runCommand(http, nil, false, "") + if err == nil { + t.Fatal("expected error") + } + assert.Equal(t, `no open pull requests found for branch "feature"`, err.Error()) +} + +func TestPRDiff_argument_not_found(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { + "pullRequest": { "number": 123 } + } } } +`)) + http.StubResponse(404, bytes.NewBufferString("")) + _, err := runCommand(http, nil, false, "123") + if err == nil { + t.Fatal("expected error", err) + } + assert.Equal(t, `could not find pull request diff: pull request not found`, err.Error()) +} + +func TestPRDiff_notty(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "number": 123, + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } }`)) + http.StubResponse(200, bytes.NewBufferString(testDiff)) + output, err := runCommand(http, nil, false, "") + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + if diff := cmp.Diff(testDiff, output.String()); diff != "" { + t.Errorf("command output did not match:\n%s", diff) + } +} + +func TestPRDiff_tty(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "number": 123, + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } }`)) + http.StubResponse(200, bytes.NewBufferString(testDiff)) + output, err := runCommand(http, nil, true, "") + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + assert.Contains(t, output.String(), "\x1b[32m+site: bin/gh\x1b[m") +} + +const testDiff = `diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml +index 73974448..b7fc0154 100644 +--- a/.github/workflows/releases.yml ++++ b/.github/workflows/releases.yml +@@ -44,6 +44,11 @@ jobs: + token: ${{secrets.SITE_GITHUB_TOKEN}} + - name: Publish documentation site + if: "!contains(github.ref, '-')" # skip prereleases ++ env: ++ GIT_COMMITTER_NAME: cli automation ++ GIT_AUTHOR_NAME: cli automation ++ GIT_COMMITTER_EMAIL: noreply@github.com ++ GIT_AUTHOR_EMAIL: noreply@github.com + run: make site-publish + - name: Move project cards + if: "!contains(github.ref, '-')" # skip prereleases +diff --git a/Makefile b/Makefile +index f2b4805c..3d7bd0f9 100644 +--- a/Makefile ++++ b/Makefile +@@ -22,8 +22,8 @@ test: + go test ./... + .PHONY: test + +-site: +- git clone https://github.com/github/cli.github.com.git "$@" ++site: bin/gh ++ bin/gh repo clone github/cli.github.com "$@" + + site-docs: site + git -C site pull +` diff --git a/command/pr_review.go b/pkg/cmd/pr/review/review.go similarity index 50% rename from command/pr_review.go rename to pkg/cmd/pr/review/review.go index 2dba6c766..61bfcea59 100644 --- a/command/pr_review.go +++ b/pkg/cmd/pr/review/review.go @@ -1,79 +1,171 @@ -package command +package review import ( "errors" "fmt" + "net/http" "github.com/AlecAivazis/survey/v2" "github.com/MakeNowJust/heredoc" - "github.com/spf13/cobra" - "github.com/cli/cli/api" + "github.com/cli/cli/context" + "github.com/cli/cli/internal/config" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmd/pr/shared" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/pkg/prompt" "github.com/cli/cli/pkg/surveyext" "github.com/cli/cli/utils" + "github.com/spf13/cobra" ) -func init() { - prCmd.AddCommand(prReviewCmd) +type ReviewOptions struct { + HttpClient func() (*http.Client, error) + Config func() (config.Config, error) + IO *iostreams.IOStreams + BaseRepo func() (ghrepo.Interface, error) + Remotes func() (context.Remotes, error) + Branch func() (string, error) - prReviewCmd.Flags().BoolP("approve", "a", false, "Approve pull request") - prReviewCmd.Flags().BoolP("request-changes", "r", false, "Request changes on a pull request") - prReviewCmd.Flags().BoolP("comment", "c", false, "Comment on a pull request") - prReviewCmd.Flags().StringP("body", "b", "", "Specify the body of a review") + SelectorArg string + Approve bool + RequestChanges bool + Comment bool + Body string } -var prReviewCmd = &cobra.Command{ - Use: "review [ | | ]", - Short: "Add a review to a pull request", - Long: `Add a review to a pull request. +func NewCmdReview(f *cmdutil.Factory, runF func(*ReviewOptions) error) *cobra.Command { + opts := &ReviewOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + Config: f.Config, + BaseRepo: f.BaseRepo, + Remotes: f.Remotes, + Branch: f.Branch, + } -Without an argument, the pull request that belongs to the current branch is reviewed.`, - Example: heredoc.Doc(` - # approve the pull request of the current branch - $ gh pr review --approve - - # leave a review comment for the current branch - $ gh pr review --comment -b "interesting" - - # add a review for a specific pull request - $ gh pr review 123 - - # request changes on a specific pull request - $ gh pr review 123 -r -b "needs more ASCII art" - `), - Args: cobra.MaximumNArgs(1), - RunE: prReview, + cmd := &cobra.Command{ + Use: "review [ | | ]", + Short: "Add a review to a pull request", + Long: heredoc.Doc(` + Add a review to a pull request. + + Without an argument, the pull request that belongs to the current branch is reviewed. + `), + Example: heredoc.Doc(` + # approve the pull request of the current branch + $ gh pr review --approve + + # leave a review comment for the current branch + $ gh pr review --comment -b "interesting" + + # add a review for a specific pull request + $ gh pr review 123 + + # request changes on a specific pull request + $ gh pr review 123 -r -b "needs more ASCII art" + `), + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if len(args) > 0 { + opts.SelectorArg = args[0] + } + + if runF != nil { + return runF(opts) + } + return reviewRun(opts) + }, + } + + cmd.Flags().BoolVarP(&opts.Approve, "approve", "a", false, "Approve pull request") + cmd.Flags().BoolVarP(&opts.RequestChanges, "request-changes", "r", false, "Request changes on a pull request") + cmd.Flags().BoolVarP(&opts.Comment, "comment", "c", false, "Comment on a pull request") + cmd.Flags().StringVarP(&opts.Body, "body", "b", "", "Specify the body of a review") + + return cmd } -func processReviewOpt(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { +func reviewRun(opts *ReviewOptions) error { + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + + reviewData, err := processReviewOpt(opts) + if err != nil { + return fmt.Errorf("did not understand desired review action: %w", err) + } + + pr, baseRepo, err := shared.PRFromArgs(apiClient, opts.BaseRepo, opts.Branch, opts.Remotes, opts.SelectorArg) + if err != nil { + return err + } + + if reviewData == nil { + editorCommand, err := cmdutil.DetermineEditor(opts.Config) + if err != nil { + return err + } + reviewData, err = reviewSurvey(opts.IO, editorCommand) + if err != nil { + return err + } + if reviewData == nil && err == nil { + fmt.Fprint(opts.IO.ErrOut, "Discarding.\n") + return nil + } + } + + err = api.AddReview(apiClient, baseRepo, pr, reviewData) + if err != nil { + return fmt.Errorf("failed to create review: %w", err) + } + + if !opts.IO.IsStdoutTTY() || !opts.IO.IsStderrTTY() { + return nil + } + + switch reviewData.State { + case api.ReviewComment: + fmt.Fprintf(opts.IO.ErrOut, "%s Reviewed pull request #%d\n", utils.Gray("-"), pr.Number) + case api.ReviewApprove: + fmt.Fprintf(opts.IO.ErrOut, "%s Approved pull request #%d\n", utils.Green("✓"), pr.Number) + case api.ReviewRequestChanges: + fmt.Fprintf(opts.IO.ErrOut, "%s Requested changes to pull request #%d\n", utils.Red("+"), pr.Number) + } + + return nil +} + +// TODO: move to Command.Args, raise FlagError +func processReviewOpt(opts *ReviewOptions) (*api.PullRequestReviewInput, error) { found := 0 flag := "" var state api.PullRequestReviewState - if cmd.Flags().Changed("approve") { + if opts.Approve { found++ flag = "approve" state = api.ReviewApprove } - if cmd.Flags().Changed("request-changes") { + if opts.RequestChanges { found++ flag = "request-changes" state = api.ReviewRequestChanges } - if cmd.Flags().Changed("comment") { + if opts.Comment { found++ flag = "comment" state = api.ReviewComment } - body, err := cmd.Flags().GetString("body") - if err != nil { - return nil, err - } + body := opts.Body if found == 0 && body == "" { - if connectedToTerminal(cmd) { + if opts.IO.IsStdoutTTY() && opts.IO.IsStderrTTY() { return nil, nil // signal interactive mode } return nil, errors.New("--approve, --request-changes, or --comment required when not attached to a tty") @@ -93,63 +185,7 @@ func processReviewOpt(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { }, nil } -func prReview(cmd *cobra.Command, args []string) error { - ctx := contextForCommand(cmd) - apiClient, err := apiClientForContext(ctx) - if err != nil { - return err - } - - pr, baseRepo, err := prFromArgs(ctx, apiClient, cmd, args) - if err != nil { - return err - } - - reviewData, err := processReviewOpt(cmd) - if err != nil { - return fmt.Errorf("did not understand desired review action: %w", err) - } - - stderr := colorableErr(cmd) - - if reviewData == nil { - reviewData, err = reviewSurvey(cmd) - if err != nil { - return err - } - if reviewData == nil && err == nil { - fmt.Fprint(stderr, "Discarding.\n") - return nil - } - } - - err = api.AddReview(apiClient, baseRepo, pr, reviewData) - if err != nil { - return fmt.Errorf("failed to create review: %w", err) - } - - if !connectedToTerminal(cmd) { - return nil - } - - switch reviewData.State { - case api.ReviewComment: - fmt.Fprintf(stderr, "%s Reviewed pull request #%d\n", utils.Gray("-"), pr.Number) - case api.ReviewApprove: - fmt.Fprintf(stderr, "%s Approved pull request #%d\n", utils.Green("✓"), pr.Number) - case api.ReviewRequestChanges: - fmt.Fprintf(stderr, "%s Requested changes to pull request #%d\n", utils.Red("+"), pr.Number) - } - - return nil -} - -func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { - editorCommand, err := determineEditor(cmd) - if err != nil { - return nil, err - } - +func reviewSurvey(io *iostreams.IOStreams, editorCommand string) (*api.PullRequestReviewInput, error) { typeAnswers := struct { ReviewType string }{} @@ -167,7 +203,7 @@ func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { }, } - err = prompt.SurveyAsk(typeQs, &typeAnswers) + err := prompt.SurveyAsk(typeQs, &typeAnswers) if err != nil { return nil, err } @@ -218,13 +254,12 @@ func reviewSurvey(cmd *cobra.Command) (*api.PullRequestReviewInput, error) { } if len(bodyAnswers.Body) > 0 { - out := colorableOut(cmd) renderedBody, err := utils.RenderMarkdown(bodyAnswers.Body) if err != nil { return nil, err } - fmt.Fprintf(out, "Got:\n%s", renderedBody) + fmt.Fprintf(io.Out, "Got:\n%s", renderedBody) } confirm := false diff --git a/command/pr_review_test.go b/pkg/cmd/pr/review/review_test.go similarity index 55% rename from command/pr_review_test.go rename to pkg/cmd/pr/review/review_test.go index b2bb36536..fa033d0a5 100644 --- a/command/pr_review_test.go +++ b/pkg/cmd/pr/review/review_test.go @@ -1,58 +1,112 @@ -package command +package review import ( "bytes" "encoding/json" "io/ioutil" + "net/http" "regexp" "testing" + "github.com/cli/cli/context" + "github.com/cli/cli/git" + "github.com/cli/cli/internal/config" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/httpmock" + "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/pkg/prompt" "github.com/cli/cli/test" + "github.com/google/shlex" "github.com/stretchr/testify/assert" ) +func runCommand(rt http.RoundTripper, remotes context.Remotes, isTTY bool, cli string) (*test.CmdOut, error) { + io, _, stdout, stderr := iostreams.Test() + io.SetStdoutTTY(isTTY) + io.SetStdinTTY(isTTY) + io.SetStderrTTY(isTTY) + + factory := &cmdutil.Factory{ + IOStreams: io, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: rt}, nil + }, + Config: func() (config.Config, error) { + return config.NewBlankConfig(), nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + Remotes: func() (context.Remotes, error) { + if remotes == nil { + return context.Remotes{ + { + Remote: &git.Remote{Name: "origin"}, + Repo: ghrepo.New("OWNER", "REPO"), + }, + }, nil + } + + return remotes, nil + }, + Branch: func() (string, error) { + return "feature", nil + }, + } + + cmd := NewCmdReview(factory, nil) + + argv, err := shlex.Split(cli) + if err != nil { + return nil, err + } + cmd.SetArgs(argv) + + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(ioutil.Discard) + cmd.SetErr(ioutil.Discard) + + _, err = cmd.ExecuteC() + return &test.CmdOut{ + OutBuf: stdout, + ErrBuf: stderr, + }, err +} + func TestPRReview_validation(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() for _, cmd := range []string{ - `pr review --approve --comment 123`, - `pr review --approve --comment -b"hey" 123`, + `--approve --comment 123`, + `--approve --comment -b"hey" 123`, } { - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 123 } - } } } - `)) - _, err := RunCommand(cmd) - if err == nil { - t.Fatal("expected error") - } - eq(t, err.Error(), "did not understand desired review action: need exactly one of --approve, --request-changes, or --comment") + t.Run(cmd, func(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + + _, err := runCommand(http, nil, false, cmd) + if err == nil { + t.Fatal("expected error") + } + assert.Equal(t, "did not understand desired review action: need exactly one of --approve, --request-changes, or --comment", err.Error()) + }) + } } func TestPRReview_bad_body(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 123 } - } } } - `)) - _, err := RunCommand(`pr review 123 -b "radical"`) + http := &httpmock.Registry{} + defer http.Verify(t) + + _, err := runCommand(http, nil, false, `123 -b "radical"`) if err == nil { t.Fatal("expected error") } - eq(t, err.Error(), "did not understand desired review action: --body unsupported without --approve, --request-changes, or --comment") + assert.Equal(t, "did not understand desired review action: --body unsupported without --approve, --request-changes, or --comment", err.Error()) } func TestPRReview_url_arg(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - defer stubTerminal(true)() - http := initFakeHTTP() + http := &httpmock.Registry{} + defer http.Verify(t) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequest": { "id": "foobar123", @@ -72,7 +126,7 @@ func TestPRReview_url_arg(t *testing.T) { } } } } `)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - output, err := RunCommand("pr review --approve https://github.com/OWNER/REPO/pull/123") + output, err := runCommand(http, nil, true, "--approve https://github.com/OWNER/REPO/pull/123") if err != nil { t.Fatalf("error running pr review: %s", err) } @@ -91,16 +145,14 @@ func TestPRReview_url_arg(t *testing.T) { }{} _ = json.Unmarshal(bodyBytes, &reqBody) - eq(t, reqBody.Variables.Input.PullRequestID, "foobar123") - eq(t, reqBody.Variables.Input.Event, "APPROVE") - eq(t, reqBody.Variables.Input.Body, "") + assert.Equal(t, "foobar123", reqBody.Variables.Input.PullRequestID) + assert.Equal(t, "APPROVE", reqBody.Variables.Input.Event) + assert.Equal(t, "", reqBody.Variables.Input.Body) } func TestPRReview_number_arg(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") + http := &httpmock.Registry{} + defer http.Verify(t) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequest": { "id": "foobar123", @@ -120,14 +172,14 @@ func TestPRReview_number_arg(t *testing.T) { } } } } `)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - output, err := RunCommand("pr review --approve 123") + output, err := runCommand(http, nil, true, "--approve 123") if err != nil { t.Fatalf("error running pr review: %s", err) } test.ExpectLines(t, output.Stderr(), "Approved pull request #123") - bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + bodyBytes, _ := ioutil.ReadAll(http.Requests[1].Body) reqBody := struct { Variables struct { Input struct { @@ -139,16 +191,14 @@ func TestPRReview_number_arg(t *testing.T) { }{} _ = json.Unmarshal(bodyBytes, &reqBody) - eq(t, reqBody.Variables.Input.PullRequestID, "foobar123") - eq(t, reqBody.Variables.Input.Event, "APPROVE") - eq(t, reqBody.Variables.Input.Body, "") + assert.Equal(t, "foobar123", reqBody.Variables.Input.PullRequestID) + assert.Equal(t, "APPROVE", reqBody.Variables.Input.Event) + assert.Equal(t, "", reqBody.Variables.Input.Body) } func TestPRReview_no_arg(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") + http := &httpmock.Registry{} + defer http.Verify(t) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", @@ -159,14 +209,14 @@ func TestPRReview_no_arg(t *testing.T) { ] } } } }`)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - output, err := RunCommand(`pr review --comment -b "cool story"`) + output, err := runCommand(http, nil, true, `--comment -b "cool story"`) if err != nil { t.Fatalf("error running pr review: %s", err) } test.ExpectLines(t, output.Stderr(), "Reviewed pull request #123") - bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + bodyBytes, _ := ioutil.ReadAll(http.Requests[1].Body) reqBody := struct { Variables struct { Input struct { @@ -178,37 +228,25 @@ func TestPRReview_no_arg(t *testing.T) { }{} _ = json.Unmarshal(bodyBytes, &reqBody) - eq(t, reqBody.Variables.Input.PullRequestID, "foobar123") - eq(t, reqBody.Variables.Input.Event, "COMMENT") - eq(t, reqBody.Variables.Input.Body, "cool story") + assert.Equal(t, "foobar123", reqBody.Variables.Input.PullRequestID) + assert.Equal(t, "COMMENT", reqBody.Variables.Input.Event) + assert.Equal(t, "cool story", reqBody.Variables.Input.Body) } func TestPRReview_blank_comment(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 123 } - } } } - `)) + http := &httpmock.Registry{} + defer http.Verify(t) - _, err := RunCommand(`pr review --comment 123`) - eq(t, err.Error(), "did not understand desired review action: body cannot be blank for comment review") + _, err := runCommand(http, nil, false, `--comment 123`) + assert.Equal(t, "did not understand desired review action: body cannot be blank for comment review", err.Error()) } func TestPRReview_blank_request_changes(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { - "pullRequest": { "number": 123 } - } } } - `)) + http := &httpmock.Registry{} + defer http.Verify(t) - _, err := RunCommand(`pr review -r 123`) - eq(t, err.Error(), "did not understand desired review action: body cannot be blank for request-changes review") + _, err := runCommand(http, nil, false, `-r 123`) + assert.Equal(t, "did not understand desired review action: body cannot be blank for request-changes review", err.Error()) } func TestPRReview(t *testing.T) { @@ -218,63 +256,53 @@ func TestPRReview(t *testing.T) { ExpectedBody string } cases := []c{ - {`pr review --request-changes -b"bad"`, "REQUEST_CHANGES", "bad"}, - {`pr review --approve`, "APPROVE", ""}, - {`pr review --approve -b"hot damn"`, "APPROVE", "hot damn"}, - {`pr review --comment --body "i donno"`, "COMMENT", "i donno"}, + {`--request-changes -b"bad"`, "REQUEST_CHANGES", "bad"}, + {`--approve`, "APPROVE", ""}, + {`--approve -b"hot damn"`, "APPROVE", "hot damn"}, + {`--comment --body "i donno"`, "COMMENT", "i donno"}, } for _, kase := range cases { - 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", - "id": "foobar123", - "headRefName": "feature", - "baseRefName": "master" } - ] } } } } - `)) - http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) + t.Run(kase.Cmd, func(t *testing.T) { + http := &httpmock.Registry{} + defer http.Verify(t) + http.StubResponse(200, bytes.NewBufferString(` + { "data": { "repository": { "pullRequests": { "nodes": [ + { "url": "https://github.com/OWNER/REPO/pull/123", + "id": "foobar123", + "headRefName": "feature", + "baseRefName": "master" } + ] } } } } + `)) + http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - _, err := RunCommand(kase.Cmd) - if err != nil { - t.Fatalf("got unexpected error running %s: %s", kase.Cmd, err) - } - - bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) - reqBody := struct { - Variables struct { - Input struct { - Event string - Body string - } + _, err := runCommand(http, nil, false, kase.Cmd) + if err != nil { + t.Fatalf("got unexpected error running %s: %s", kase.Cmd, err) } - }{} - _ = json.Unmarshal(bodyBytes, &reqBody) - eq(t, reqBody.Variables.Input.Event, kase.ExpectedEvent) - eq(t, reqBody.Variables.Input.Body, kase.ExpectedBody) + bodyBytes, _ := ioutil.ReadAll(http.Requests[1].Body) + reqBody := struct { + Variables struct { + Input struct { + Event string + Body string + } + } + }{} + _ = json.Unmarshal(bodyBytes, &reqBody) + + assert.Equal(t, kase.ExpectedEvent, reqBody.Variables.Input.Event) + assert.Equal(t, kase.ExpectedBody, reqBody.Variables.Input.Body) + }) } } func TestPRReview_nontty_insufficient_flags(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(false)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.StubResponse(200, bytes.NewBufferString(` - { "data": { "repository": { "pullRequests": { "nodes": [ - { "url": "https://github.com/OWNER/REPO/pull/123", - "number": 123, - "id": "foobar123", - "headRefName": "feature", - "baseRefName": "master" } - ] } } } } - `)) + http := &httpmock.Registry{} + defer http.Verify(t) - output, err := RunCommand("pr review") + output, err := runCommand(http, nil, false, "") if err == nil { t.Fatal("expected error") } @@ -285,10 +313,8 @@ func TestPRReview_nontty_insufficient_flags(t *testing.T) { } func TestPRReview_nontty(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(false)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") + http := &httpmock.Registry{} + defer http.Verify(t) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", @@ -300,7 +326,7 @@ func TestPRReview_nontty(t *testing.T) { `)) http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) - output, err := RunCommand("pr review -c -bcool") + output, err := runCommand(http, nil, false, "-c -bcool") if err != nil { t.Fatalf("unexpected error running command: %s", err) } @@ -308,7 +334,7 @@ func TestPRReview_nontty(t *testing.T) { assert.Equal(t, "", output.String()) assert.Equal(t, "", output.Stderr()) - bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + bodyBytes, _ := ioutil.ReadAll(http.Requests[1].Body) reqBody := struct { Variables struct { Input struct { @@ -324,10 +350,8 @@ func TestPRReview_nontty(t *testing.T) { } func TestPRReview_interactive(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") + http := &httpmock.Registry{} + defer http.Verify(t) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", @@ -360,7 +384,7 @@ func TestPRReview_interactive(t *testing.T) { }, }) - output, err := RunCommand(`pr review`) + output, err := runCommand(http, nil, true, "") if err != nil { t.Fatalf("got unexpected error running pr review: %s", err) } @@ -371,7 +395,7 @@ func TestPRReview_interactive(t *testing.T) { "Got:", "cool.*story") - bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + bodyBytes, _ := ioutil.ReadAll(http.Requests[1].Body) reqBody := struct { Variables struct { Input struct { @@ -382,15 +406,13 @@ func TestPRReview_interactive(t *testing.T) { }{} _ = json.Unmarshal(bodyBytes, &reqBody) - eq(t, reqBody.Variables.Input.Event, "APPROVE") - eq(t, reqBody.Variables.Input.Body, "cool story") + assert.Equal(t, "APPROVE", reqBody.Variables.Input.Event) + assert.Equal(t, "cool story", reqBody.Variables.Input.Body) } func TestPRReview_interactive_no_body(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") + http := &httpmock.Registry{} + defer http.Verify(t) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", @@ -399,7 +421,7 @@ func TestPRReview_interactive_no_body(t *testing.T) { "baseRefName": "master" } ] } } } } `)) - http.StubResponse(200, bytes.NewBufferString(`{"data": {} }`)) + as, teardown := prompt.InitAskStubber() defer teardown() @@ -422,18 +444,16 @@ func TestPRReview_interactive_no_body(t *testing.T) { }, }) - _, err := RunCommand(`pr review`) + _, err := runCommand(http, nil, true, "") if err == nil { t.Fatal("expected error") } - eq(t, err.Error(), "this type of review cannot be blank") + assert.Equal(t, "this type of review cannot be blank", err.Error()) } func TestPRReview_interactive_blank_approve(t *testing.T) { - initBlankContext("", "OWNER/REPO", "feature") - defer stubTerminal(true)() - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") + http := &httpmock.Registry{} + defer http.Verify(t) http.StubResponse(200, bytes.NewBufferString(` { "data": { "repository": { "pullRequests": { "nodes": [ { "url": "https://github.com/OWNER/REPO/pull/123", @@ -466,7 +486,7 @@ func TestPRReview_interactive_blank_approve(t *testing.T) { }, }) - output, err := RunCommand(`pr review`) + output, err := runCommand(http, nil, true, "") if err != nil { t.Fatalf("got unexpected error running pr review: %s", err) } @@ -478,7 +498,7 @@ func TestPRReview_interactive_blank_approve(t *testing.T) { test.ExpectLines(t, output.Stderr(), "Approved pull request #123") - bodyBytes, _ := ioutil.ReadAll(http.Requests[2].Body) + bodyBytes, _ := ioutil.ReadAll(http.Requests[1].Body) reqBody := struct { Variables struct { Input struct { @@ -489,7 +509,6 @@ func TestPRReview_interactive_blank_approve(t *testing.T) { }{} _ = json.Unmarshal(bodyBytes, &reqBody) - eq(t, reqBody.Variables.Input.Event, "APPROVE") - eq(t, reqBody.Variables.Input.Body, "") - + assert.Equal(t, "APPROVE", reqBody.Variables.Input.Event) + assert.Equal(t, "", reqBody.Variables.Input.Body) } diff --git a/pkg/cmd/pr/shared/lookup.go b/pkg/cmd/pr/shared/lookup.go new file mode 100644 index 000000000..9583b854f --- /dev/null +++ b/pkg/cmd/pr/shared/lookup.go @@ -0,0 +1,121 @@ +package shared + +import ( + "fmt" + "net/url" + "regexp" + "strconv" + "strings" + + "github.com/cli/cli/api" + "github.com/cli/cli/context" + "github.com/cli/cli/git" + "github.com/cli/cli/internal/ghrepo" +) + +// PRFromArgs looks up the pull request from either the number/branch/URL argument or one belonging to the current branch +// +// NOTE: this API isn't great, but is here as a compatibility layer between old-style and new-style commands +func PRFromArgs(apiClient *api.Client, baseRepoFn func() (ghrepo.Interface, error), branchFn func() (string, error), remotesFn func() (context.Remotes, error), arg string) (*api.PullRequest, ghrepo.Interface, error) { + if arg != "" { + // First check to see if the prString is a url, return repo from url if found. This + // is run first because we don't need to run determineBaseRepo for this path + pr, r, err := prFromURL(apiClient, arg) + if pr != nil || err != nil { + return pr, r, err + } + } + + repo, err := baseRepoFn() + if err != nil { + return nil, nil, fmt.Errorf("could not determine base repo: %w", err) + } + + // If there are no args see if we can guess the PR from the current branch + if arg == "" { + pr, err := prForCurrentBranch(apiClient, repo, branchFn, remotesFn) + return pr, repo, err + } else { + // Next see if the prString is a number and use that to look up the url + pr, err := prFromNumberString(apiClient, repo, arg) + if pr != nil || err != nil { + return pr, repo, err + } + + // Last see if it is a branch name + pr, err = api.PullRequestForBranch(apiClient, repo, "", arg) + return pr, repo, err + } +} + +func prFromNumberString(apiClient *api.Client, repo ghrepo.Interface, s string) (*api.PullRequest, error) { + if prNumber, err := strconv.Atoi(strings.TrimPrefix(s, "#")); err == nil { + return api.PullRequestByNumber(apiClient, repo, prNumber) + } + + return nil, nil +} + +var pullURLRE = regexp.MustCompile(`^/([^/]+)/([^/]+)/pull/(\d+)`) + +func prFromURL(apiClient *api.Client, s string) (*api.PullRequest, ghrepo.Interface, error) { + u, err := url.Parse(s) + if err != nil { + return nil, nil, nil + } + + if u.Scheme != "https" && u.Scheme != "http" { + return nil, nil, nil + } + + m := pullURLRE.FindStringSubmatch(u.Path) + if m == nil { + return nil, nil, nil + } + + repo := ghrepo.NewWithHost(m[1], m[2], u.Hostname()) + prNumberString := m[3] + pr, err := prFromNumberString(apiClient, repo, prNumberString) + return pr, repo, err +} + +func prForCurrentBranch(apiClient *api.Client, repo ghrepo.Interface, branchFn func() (string, error), remotesFn func() (context.Remotes, error)) (*api.PullRequest, error) { + prHeadRef, err := branchFn() + if err != nil { + return nil, err + } + + branchConfig := git.ReadBranchConfig(prHeadRef) + + // the branch is configured to merge a special PR head ref + prHeadRE := regexp.MustCompile(`^refs/pull/(\d+)/head$`) + if m := prHeadRE.FindStringSubmatch(branchConfig.MergeRef); m != nil { + return prFromNumberString(apiClient, repo, m[1]) + } + + var branchOwner string + if branchConfig.RemoteURL != nil { + // the branch merges from a remote specified by URL + if r, err := ghrepo.FromURL(branchConfig.RemoteURL); err == nil { + branchOwner = r.RepoOwner() + } + } else if branchConfig.RemoteName != "" { + // the branch merges from a remote specified by name + rem, _ := remotesFn() + if r, err := rem.FindByName(branchConfig.RemoteName); err == nil { + branchOwner = r.RepoOwner() + } + } + + if branchOwner != "" { + if strings.HasPrefix(branchConfig.MergeRef, "refs/heads/") { + prHeadRef = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") + } + // prepend `OWNER:` if this branch is pushed to a fork + if !strings.EqualFold(branchOwner, repo.RepoOwner()) { + prHeadRef = fmt.Sprintf("%s:%s", branchOwner, prHeadRef) + } + } + + return api.PullRequestForBranch(apiClient, repo, "", prHeadRef) +} diff --git a/pkg/cmd/repo/clone/clone.go b/pkg/cmd/repo/clone/clone.go index 6a859e7e6..ef82d5081 100644 --- a/pkg/cmd/repo/clone/clone.go +++ b/pkg/cmd/repo/clone/clone.go @@ -70,6 +70,7 @@ func cloneRun(opts *CloneOptions) error { if err != nil { return err } + // TODO: GHE support protocol, err := cfg.Get("", "git_protocol") if err != nil { return err diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index 313f9a91e..32313ca52 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -147,6 +147,7 @@ func createRun(opts *CreateOptions) error { if err != nil { return err } + // TODO: GHE support protocol, err := cfg.Get("", "git_protocol") if err != nil { return err diff --git a/pkg/cmd/repo/fork/fork.go b/pkg/cmd/repo/fork/fork.go index d443e52e8..4fad93fec 100644 --- a/pkg/cmd/repo/fork/fork.go +++ b/pkg/cmd/repo/fork/fork.go @@ -186,6 +186,7 @@ func forkRun(opts *ForkOptions) error { if err != nil { return err } + // TODO: GHE support protocol, err := cfg.Get("", "git_protocol") if err != nil { return err diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go index 9a6cd5c6f..62ed5d802 100644 --- a/pkg/cmdutil/factory.go +++ b/pkg/cmdutil/factory.go @@ -15,4 +15,5 @@ type Factory struct { BaseRepo func() (ghrepo.Interface, error) Remotes func() (context.Remotes, error) Config func() (config.Config, error) + Branch func() (string, error) } diff --git a/pkg/cmdutil/legacy.go b/pkg/cmdutil/legacy.go new file mode 100644 index 000000000..57cdf855b --- /dev/null +++ b/pkg/cmdutil/legacy.go @@ -0,0 +1,23 @@ +package cmdutil + +import ( + "fmt" + "os" + + "github.com/cli/cli/internal/config" +) + +// TODO: consider passing via Factory +// TODO: support per-hostname settings +func DetermineEditor(cf func() (config.Config, error)) (string, error) { + editorCommand := os.Getenv("GH_EDITOR") + if editorCommand == "" { + cfg, err := cf() + if err != nil { + return "", fmt.Errorf("could not read config: %w", err) + } + editorCommand, _ = cfg.Get("", "editor") + } + + return editorCommand, nil +}