From 487dd06bf347ceebf622093b134fc99a6cbf28b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 29 Jul 2020 22:50:34 +0200 Subject: [PATCH] Isolate `pr checkout` command --- api/queries_repo.go | 9 +- command/pr.go | 1 - command/root.go | 2 + command/root_test.go | 21 -- .../cmd/pr/checkout/checkout.go | 94 +++++-- .../cmd/pr/checkout/checkout_test.go | 236 +++++++++--------- 6 files changed, 195 insertions(+), 168 deletions(-) rename command/pr_checkout.go => pkg/cmd/pr/checkout/checkout.go (65%) rename command/pr_checkout_test.go => pkg/cmd/pr/checkout/checkout_test.go (79%) diff --git a/api/queries_repo.go b/api/queries_repo.go index 06d4bfdab..ac4628cd3 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 3f8e53481..75c4b9e9d 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/root.go b/command/root.go index 6eac27025..e1d90e453 100644 --- a/command/root.go +++ b/command/root.go @@ -22,6 +22,7 @@ import ( "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" @@ -172,6 +173,7 @@ func init() { prCmd.AddCommand(prReviewCmd.NewCmdReview(&repoResolvingCmdFactory, nil)) prCmd.AddCommand(prDiffCmd.NewCmdDiff(&repoResolvingCmdFactory, nil)) + prCmd.AddCommand(prCheckoutCmd.NewCmdCheckout(&repoResolvingCmdFactory, nil)) RootCmd.AddCommand(creditsCmd.NewCmdCredits(cmdFactory, nil)) } 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/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..055015c49 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.Repository{ + Name: "REPO", + Owner: api.RepositoryOwner{Login: "OWNER"}, + DefaultBranchRef: api.BranchRef{Name: "master"}, + }, 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,7 +128,7 @@ func TestPRCheckout_sameRepo(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(), "") @@ -66,15 +140,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 +169,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 +178,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 +212,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 +234,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 +264,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 +273,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 +303,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 +314,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 +355,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 +367,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 +397,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 +409,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 +439,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 +449,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 +479,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 +489,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 +513,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 +521,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 +551,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(), "")