From a27c8a9c210e87b8123113a916984aff891aa4a9 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 22 Jul 2020 11:13:28 -0500 Subject: [PATCH 01/12] isolate repo view cmd --- api/queries_pr.go | 4 + api/queries_repo.go | 32 --- command/repo.go | 155 ----------- command/repo_test.go | 218 --------------- command/root.go | 4 + pkg/cmd/gist/create/create.go | 1 + pkg/cmd/repo/view/http.go | 41 +++ pkg/cmd/repo/view/view.go | 211 +++++++++++++++ pkg/cmd/repo/view/view_test.go | 471 +++++++++++++++++++++++++++++++++ pkg/iostreams/iostreams.go | 21 +- 10 files changed, 751 insertions(+), 407 deletions(-) create mode 100644 pkg/cmd/repo/view/http.go create mode 100644 pkg/cmd/repo/view/view.go create mode 100644 pkg/cmd/repo/view/view_test.go diff --git a/api/queries_pr.go b/api/queries_pr.go index 4437f8f7e..46e036465 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -134,6 +134,10 @@ type NotFoundError struct { error } +func NewNotFoundError(err error) *NotFoundError { + return &NotFoundError{err} +} + func (err *NotFoundError) Unwrap() error { return err.error } diff --git a/api/queries_repo.go b/api/queries_repo.go index 35079095c..e2ae274ed 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -3,7 +3,6 @@ package api import ( "bytes" "context" - "encoding/base64" "encoding/json" "errors" "fmt" @@ -405,37 +404,6 @@ func RepoCreate(client *Client, input RepoCreateInput) (*Repository, error) { return initRepoHostname(&response.CreateRepository.Repository, "github.com"), nil } -type RepoReadme struct { - Filename string - Content string -} - -func RepositoryReadme(client *Client, repo ghrepo.Interface) (*RepoReadme, error) { - var response struct { - Name string - Content string - } - - err := client.REST("GET", fmt.Sprintf("repos/%s/readme", ghrepo.FullName(repo)), nil, &response) - if err != nil { - var httpError HTTPError - if errors.As(err, &httpError) && httpError.StatusCode == 404 { - return nil, &NotFoundError{err} - } - return nil, err - } - - decoded, err := base64.StdEncoding.DecodeString(response.Content) - if err != nil { - return nil, fmt.Errorf("failed to decode readme: %w", err) - } - - return &RepoReadme{ - Filename: response.Name, - Content: string(decoded), - }, nil -} - type RepoMetadataResult struct { AssignableUsers []RepoAssignee Labels []RepoLabel diff --git a/command/repo.go b/command/repo.go index d705a4a5f..e0114a080 100644 --- a/command/repo.go +++ b/command/repo.go @@ -7,7 +7,6 @@ import ( "os" "path" "strings" - "text/template" "time" "github.com/AlecAivazis/survey/v2" @@ -21,7 +20,6 @@ import ( ) func init() { - RootCmd.AddCommand(repoCmd) repoCmd.AddCommand(repoCloneCmd) repoCmd.AddCommand(repoCreateCmd) @@ -38,9 +36,6 @@ func init() { repoForkCmd.Flags().Lookup("clone").NoOptDefVal = "true" repoForkCmd.Flags().Lookup("remote").NoOptDefVal = "true" - repoCmd.AddCommand(repoViewCmd) - repoViewCmd.Flags().BoolP("web", "w", false, "Open a repository in the browser") - repoCmd.AddCommand(repoCreditsCmd) repoCreditsCmd.Flags().BoolP("static", "s", false, "Print a static version of the credits") } @@ -104,17 +99,6 @@ With no argument, creates a fork of the current repository. Otherwise, forks the RunE: repoFork, } -var repoViewCmd = &cobra.Command{ - Use: "view []", - Short: "View a repository", - Long: `Display the description and the README of a GitHub repository. - -With no argument, the repository for the current directory is displayed. - -With '--web', open the repository in a web browser instead.`, - RunE: repoView, -} - var repoCreditsCmd = &cobra.Command{ Use: "credits []", Short: "View credits for a repository", @@ -576,145 +560,6 @@ var Confirm = func(prompt string, result *bool) error { return survey.AskOne(p, result) } -func repoView(cmd *cobra.Command, args []string) error { - ctx := contextForCommand(cmd) - apiClient, err := apiClientForContext(ctx) - if err != nil { - return err - } - - var toView ghrepo.Interface - if len(args) == 0 { - var err error - toView, err = determineBaseRepo(apiClient, cmd, ctx) - if err != nil { - return err - } - } else { - repoArg := args[0] - if isURL(repoArg) { - parsedURL, err := url.Parse(repoArg) - if err != nil { - return fmt.Errorf("did not understand argument: %w", err) - } - - toView, err = ghrepo.FromURL(parsedURL) - if err != nil { - return fmt.Errorf("did not understand argument: %w", err) - } - } else { - var err error - toView, err = ghrepo.FromFullName(repoArg) - if err != nil { - return fmt.Errorf("argument error: %w", err) - } - } - } - - repo, err := api.GitHubRepo(apiClient, toView) - if err != nil { - return err - } - - web, err := cmd.Flags().GetBool("web") - if err != nil { - return err - } - - openURL := generateRepoURL(toView, "") - if web { - if connectedToTerminal(cmd) { - fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", displayURL(openURL)) - } - return utils.OpenInBrowser(openURL) - } - - fullName := ghrepo.FullName(toView) - - if !connectedToTerminal(cmd) { - readme, err := api.RepositoryReadme(apiClient, toView) - if err != nil { - return err - } - - out := cmd.OutOrStdout() - fmt.Fprintf(out, "name:\t%s\n", fullName) - fmt.Fprintf(out, "description:\t%s\n", repo.Description) - fmt.Fprintln(out, "--") - fmt.Fprintf(out, readme.Content) - - return nil - } - - repoTmpl := ` -{{.FullName}} -{{.Description}} - -{{.Readme}} - -{{.View}} -` - - tmpl, err := template.New("repo").Parse(repoTmpl) - if err != nil { - return err - } - - readme, err := api.RepositoryReadme(apiClient, toView) - var notFound *api.NotFoundError - if err != nil && !errors.As(err, ¬Found) { - return err - } - - var readmeContent string - if readme == nil { - readmeContent = utils.Gray("This repository does not have a README") - } else if isMarkdownFile(readme.Filename) { - var err error - readmeContent, err = utils.RenderMarkdown(readme.Content) - if err != nil { - return fmt.Errorf("error rendering markdown: %w", err) - } - } else { - readmeContent = readme.Content - } - - description := repo.Description - if description == "" { - description = utils.Gray("No description provided") - } - - repoData := struct { - FullName string - Description string - Readme string - View string - }{ - FullName: utils.Bold(fullName), - Description: description, - Readme: readmeContent, - View: utils.Gray(fmt.Sprintf("View this repository on GitHub: %s", openURL)), - } - - out := colorableOut(cmd) - - err = tmpl.Execute(out, repoData) - if err != nil { - return err - } - - return nil -} - func repoCredits(cmd *cobra.Command, args []string) error { return credits(cmd, args) } - -func isMarkdownFile(filename string) bool { - // kind of gross, but i'm assuming that 90% of the time the suffix will just be .md. it didn't - // seem worth executing a regex for this given that assumption. - return strings.HasSuffix(filename, ".md") || - strings.HasSuffix(filename, ".markdown") || - strings.HasSuffix(filename, ".mdown") || - strings.HasSuffix(filename, ".mkdown") -} diff --git a/command/repo_test.go b/command/repo_test.go index 8c8e2cf31..cec4f114f 100644 --- a/command/repo_test.go +++ b/command/repo_test.go @@ -827,221 +827,3 @@ func TestRepoCreate_orgWithTeam(t *testing.T) { t.Errorf("expected %q, got %q", "TEAMID", teamID) } } - -func TestRepoView_web(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(` - { }`)) - - var seenCmd *exec.Cmd - restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { - seenCmd = cmd - return &test.OutputStub{} - }) - defer restoreCmd() - - defer stubTerminal(true)() - - output, err := RunCommand("repo view -w") - if err != nil { - t.Errorf("error running command `repo view`: %v", err) - } - - eq(t, output.String(), "") - eq(t, output.Stderr(), "Opening github.com/OWNER/REPO in your browser.\n") - - if seenCmd == nil { - t.Fatal("expected a command to run") - } - url := seenCmd.Args[len(seenCmd.Args)-1] - eq(t, url, "https://github.com/OWNER/REPO") -} - -func TestRepoView_web_ownerRepo(t *testing.T) { - ctx := context.NewBlank() - ctx.SetBranch("master") - initContext = func() context.Context { - return ctx - } - http := initFakeHTTP() - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(` - { }`)) - - var seenCmd *exec.Cmd - restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { - seenCmd = cmd - return &test.OutputStub{} - }) - defer restoreCmd() - defer stubTerminal(true)() - - output, err := RunCommand("repo view -w cli/cli") - if err != nil { - t.Errorf("error running command `repo view`: %v", err) - } - - eq(t, output.String(), "") - eq(t, output.Stderr(), "Opening github.com/cli/cli in your browser.\n") - - if seenCmd == nil { - t.Fatal("expected a command to run") - } - url := seenCmd.Args[len(seenCmd.Args)-1] - eq(t, url, "https://github.com/cli/cli") -} - -func TestRepoView_web_fullURL(t *testing.T) { - ctx := context.NewBlank() - ctx.SetBranch("master") - initContext = func() context.Context { - return ctx - } - http := initFakeHTTP() - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(` - { }`)) - var seenCmd *exec.Cmd - restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { - seenCmd = cmd - return &test.OutputStub{} - }) - defer restoreCmd() - - defer stubTerminal(true)() - output, err := RunCommand("repo view -w https://github.com/cli/cli") - if err != nil { - t.Errorf("error running command `repo view`: %v", err) - } - - eq(t, output.String(), "") - eq(t, output.Stderr(), "Opening github.com/cli/cli in your browser.\n") - - if seenCmd == nil { - t.Fatal("expected a command to run") - } - url := seenCmd.Args[len(seenCmd.Args)-1] - eq(t, url, "https://github.com/cli/cli") -} - -func TestRepoView(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(` - { "data": { - "repository": { - "description": "social distancing" - } } }`)) - http.Register( - httpmock.REST("GET", "repos/OWNER/REPO/readme"), - httpmock.StringResponse(` - { "name": "readme.md", - "content": "IyB0cnVseSBjb29sIHJlYWRtZSBjaGVjayBpdCBvdXQ="}`)) - - defer stubTerminal(true)() - - output, err := RunCommand("repo view") - if err != nil { - t.Errorf("error running command `repo view`: %v", err) - } - - test.ExpectLines(t, output.String(), - "OWNER/REPO", - "social distancing", - "truly cool readme", - "View this repository on GitHub: https://github.com/OWNER/REPO") -} - -func TestRepoView_nonmarkdown_readme(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(` - { "data": { - "repository": { - "description": "social distancing" - } } }`)) - http.Register( - httpmock.REST("GET", "repos/OWNER/REPO/readme"), - httpmock.StringResponse(` - { "name": "readme.org", - "content": "IyB0cnVseSBjb29sIHJlYWRtZSBjaGVjayBpdCBvdXQ="}`)) - - defer stubTerminal(true)() - - output, err := RunCommand("repo view") - if err != nil { - t.Errorf("error running command `repo view`: %v", err) - } - - test.ExpectLines(t, output.String(), - "OWNER/REPO", - "social distancing", - "# truly cool readme", - "View this repository on GitHub: https://github.com/OWNER/REPO") -} - -func TestRepoView_blanks(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.Register(httpmock.GraphQL(`query RepositoryInfo\b`), httpmock.StringResponse("{}")) - http.Register( - httpmock.REST("GET", "repos/OWNER/REPO/readme"), - httpmock.StatusStringResponse(404, `{}`)) - - defer stubTerminal(true)() - - output, err := RunCommand("repo view") - if err != nil { - t.Errorf("error running command `repo view`: %v", err) - } - - test.ExpectLines(t, output.String(), - "OWNER/REPO", - "No description provided", - "This repository does not have a README", - "View this repository on GitHub: https://github.com/OWNER/REPO") -} - -func TestRepoView_nontty(t *testing.T) { - initBlankContext("", "OWNER/REPO", "master") - http := initFakeHTTP() - http.StubRepoResponse("OWNER", "REPO") - http.Register( - httpmock.GraphQL(`query RepositoryInfo\b`), - httpmock.StringResponse(` - { "data": { - "repository": { - "description": "social distancing" - } } }`)) - http.Register( - httpmock.REST("GET", "repos/OWNER/REPO/readme"), - httpmock.StringResponse(` - { "name": "readme.md", - "content": "IyB0cnVseSBjb29sIHJlYWRtZSBjaGVjayBpdCBvdXQ="}`)) - - defer stubTerminal(false)() - - output, err := RunCommand("repo view") - if err != nil { - t.Errorf("error running command `repo view`: %v", err) - } - - test.ExpectLines(t, output.String(), - "OWNER/REPO", - "social distancing", - "# truly cool readme check it out", - ) -} diff --git a/command/root.go b/command/root.go index 01f7d524a..b988fddfb 100644 --- a/command/root.go +++ b/command/root.go @@ -21,6 +21,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" + repoViewCmd "github.com/cli/cli/pkg/cmd/repo/view" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" "github.com/cli/cli/utils" @@ -104,6 +105,9 @@ func init() { } RootCmd.AddCommand(gistCmd) gistCmd.AddCommand(gistCreateCmd.NewCmdCreate(cmdFactory, nil)) + + RootCmd.AddCommand(repoCmd) + repoCmd.AddCommand(repoViewCmd.NewCmdView(cmdFactory, nil)) } // RootCmd is the entry point of command-line execution diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index d4fcd276c..2bfedf684 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -70,6 +70,7 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co return nil }, RunE: func(c *cobra.Command, args []string) error { + // TODO redundant? opts.HttpClient = f.HttpClient opts.Filenames = args diff --git a/pkg/cmd/repo/view/http.go b/pkg/cmd/repo/view/http.go new file mode 100644 index 000000000..4b82ef448 --- /dev/null +++ b/pkg/cmd/repo/view/http.go @@ -0,0 +1,41 @@ +package view + +import ( + "encoding/base64" + "errors" + "fmt" + + "github.com/cli/cli/api" + "github.com/cli/cli/internal/ghrepo" +) + +type RepoReadme struct { + Filename string + Content string +} + +func RepositoryReadme(client *api.Client, repo ghrepo.Interface) (*RepoReadme, error) { + var response struct { + Name string + Content string + } + + err := client.REST("GET", fmt.Sprintf("repos/%s/readme", ghrepo.FullName(repo)), nil, &response) + if err != nil { + var httpError api.HTTPError + if errors.As(err, &httpError) && httpError.StatusCode == 404 { + return nil, api.NewNotFoundError(err) + } + return nil, err + } + + decoded, err := base64.StdEncoding.DecodeString(response.Content) + if err != nil { + return nil, fmt.Errorf("failed to decode readme: %w", err) + } + + return &RepoReadme{ + Filename: response.Name, + Content: string(decoded), + }, nil +} diff --git a/pkg/cmd/repo/view/view.go b/pkg/cmd/repo/view/view.go new file mode 100644 index 000000000..7b4d723c7 --- /dev/null +++ b/pkg/cmd/repo/view/view.go @@ -0,0 +1,211 @@ +package view + +import ( + "errors" + "fmt" + "html/template" + "net/http" + "net/url" + "strings" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/api" + "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/utils" + "github.com/spf13/cobra" +) + +type ViewOptions struct { + HttpClient func() (*http.Client, error) + IO *iostreams.IOStreams + BaseRepo func() (ghrepo.Interface, error) + + RepoArg string + Web bool +} + +func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Command { + opts := ViewOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + BaseRepo: f.BaseRepo, + } + + cmd := &cobra.Command{ + Use: "view []", + Short: "View a repository", + Long: `Display the description and the README of a GitHub repository. + +With no argument, the repository for the current directory is displayed. + +With '--web', open the repository in a web browser instead.`, + Args: cobra.MaximumNArgs(1), + RunE: func(c *cobra.Command, args []string) error { + if len(args) > 0 { + opts.RepoArg = args[0] + } + if runF != nil { + return runF(&opts) + } + return viewRun(&opts) + }, + } + + cmd.Flags().BoolVarP(&opts.Web, "web", "w", false, "Open a repository in the browser") + + return cmd +} + +func viewRun(opts *ViewOptions) error { + var toView ghrepo.Interface + if opts.RepoArg == "" { + var err error + toView, err = opts.BaseRepo() + if err != nil { + return err + } + } else { + if isURL(opts.RepoArg) { + parsedURL, err := url.Parse(opts.RepoArg) + if err != nil { + return fmt.Errorf("did not understand argument: %w", err) + } + + toView, err = ghrepo.FromURL(parsedURL) + if err != nil { + return fmt.Errorf("did not understand argument: %w", err) + } + } else { + var err error + toView, err = ghrepo.FromFullName(opts.RepoArg) + if err != nil { + return fmt.Errorf("argument error: %w", err) + } + } + } + + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + + apiClient := api.NewClientFromHTTP(httpClient) + + repo, err := api.GitHubRepo(apiClient, toView) + if err != nil { + return err + } + + openURL := generateRepoURL(toView, "") + if opts.Web { + if opts.IO.IsStdoutTTY() { + fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", displayURL(openURL)) + } + return utils.OpenInBrowser(openURL) + } + + fullName := ghrepo.FullName(toView) + + readme, err := RepositoryReadme(apiClient, toView) + var notFound *api.NotFoundError + if err != nil && !errors.As(err, ¬Found) { + return err + } + + stdout := opts.IO.Out + + if !opts.IO.IsStdoutTTY() { + fmt.Fprintf(stdout, "name:\t%s\n", fullName) + fmt.Fprintf(stdout, "description:\t%s\n", repo.Description) + if readme != nil { + fmt.Fprintln(stdout, "--") + fmt.Fprintf(stdout, readme.Content) + fmt.Fprintln(stdout) + } + + return nil + } + + repoTmpl := heredoc.Doc(` + {{.FullName}} + {{.Description}} + + {{.Readme}} + + {{.View}} + `) + + tmpl, err := template.New("repo").Parse(repoTmpl) + if err != nil { + return err + } + + var readmeContent string + if readme == nil { + readmeContent = utils.Gray("This repository does not have a README") + } else if isMarkdownFile(readme.Filename) { + var err error + readmeContent, err = utils.RenderMarkdown(readme.Content) + if err != nil { + return fmt.Errorf("error rendering markdown: %w", err) + } + } else { + readmeContent = readme.Content + } + + description := repo.Description + if description == "" { + description = utils.Gray("No description provided") + } + + repoData := struct { + FullName string + Description string + Readme string + View string + }{ + FullName: utils.Bold(fullName), + Description: description, + Readme: readmeContent, + View: utils.Gray(fmt.Sprintf("View this repository on GitHub: %s", openURL)), + } + + err = tmpl.Execute(stdout, repoData) + if err != nil { + return err + } + + return nil +} + +func isMarkdownFile(filename string) bool { + // kind of gross, but i'm assuming that 90% of the time the suffix will just be .md. it didn't + // seem worth executing a regex for this given that assumption. + return strings.HasSuffix(filename, ".md") || + strings.HasSuffix(filename, ".markdown") || + strings.HasSuffix(filename, ".mdown") || + strings.HasSuffix(filename, ".mkdown") +} + +// TODO COPYPASTA FROM command; CONSIDER FOR cmdutil? +func isURL(arg string) bool { + return strings.HasPrefix(arg, "http:/") || strings.HasPrefix(arg, "https:/") +} + +func generateRepoURL(repo ghrepo.Interface, p string, args ...interface{}) string { + baseURL := fmt.Sprintf("https://%s/%s/%s", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) + if p != "" { + return baseURL + "/" + fmt.Sprintf(p, args...) + } + return baseURL +} + +func displayURL(urlStr string) string { + u, err := url.Parse(urlStr) + if err != nil { + return urlStr + } + return u.Hostname() + u.Path +} diff --git a/pkg/cmd/repo/view/view_test.go b/pkg/cmd/repo/view/view_test.go new file mode 100644 index 000000000..5a6d08702 --- /dev/null +++ b/pkg/cmd/repo/view/view_test.go @@ -0,0 +1,471 @@ +package view + +import ( + "bytes" + "fmt" + "net/http" + "testing" + + "github.com/MakeNowJust/heredoc" + "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/shlex" + "github.com/stretchr/testify/assert" +) + +func TestNewCmdView(t *testing.T) { + tests := []struct { + name string + cli string + wants ViewOptions + wantsErr bool + }{ + { + name: "no args", + cli: "", + wants: ViewOptions{ + RepoArg: "", + Web: false, + }, + }, + { + name: "sets repo arg", + cli: "some/repo", + wants: ViewOptions{ + RepoArg: "some/repo", + Web: false, + }, + }, + { + name: "sets web", + cli: "-w", + wants: ViewOptions{ + RepoArg: "", + Web: true, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + io, _, _, _ := iostreams.Test() + + f := &cmdutil.Factory{ + IOStreams: io, + } + + // THOUGHT: this seems ripe for cmdutil. It's almost identical to the set up for the same test + // in gist create. + argv, err := shlex.Split(tt.cli) + assert.NoError(t, err) + + var gotOpts *ViewOptions + cmd := NewCmdView(f, func(opts *ViewOptions) error { + gotOpts = opts + return nil + }) + cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + _, err = cmd.ExecuteC() + if tt.wantsErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + + assert.Equal(t, tt.wants.Web, gotOpts.Web) + assert.Equal(t, tt.wants.RepoArg, gotOpts.RepoArg) + }) + } +} + +func Test_RepoView_Web(t *testing.T) { + tests := []struct { + name string + stdoutTTY bool + wantStderr string + }{ + { + name: "tty", + stdoutTTY: true, + wantStderr: "Opening github.com/OWNER/REPO in your browser.\n", + }, + { + name: "nontty", + wantStderr: "", + }, + } + + for _, tt := range tests { + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(`{}`)) + + opts := &ViewOptions{ + Web: true, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + } + + io, _, stdout, stderr := iostreams.Test() + + opts.IO = io + + t.Run(tt.name, func(t *testing.T) { + io.SetStdoutTTY(tt.stdoutTTY) + + cs, teardown := test.InitCmdStubber() + defer teardown() + + cs.Stub("") // browser open + + if err := viewRun(opts); err != nil { + t.Errorf("viewRun() error = %v", err) + } + assert.Equal(t, "", stdout.String()) + assert.Equal(t, 1, len(cs.Calls)) + call := cs.Calls[0] + assert.Equal(t, "https://github.com/OWNER/REPO", call.Args[len(call.Args)-1]) + assert.Equal(t, tt.wantStderr, stderr.String()) + reg.Verify(t) + }) + } +} + +func Test_ViewRun(t *testing.T) { + tests := []struct { + name string + opts *ViewOptions + repoName string + stdoutTTY bool + wantOut string + wantStderr string + wantErr bool + }{ + { + name: "nontty", + wantOut: heredoc.Doc(` + name: OWNER/REPO + description: social distancing + -- + # truly cool readme check it out + `), + }, + { + name: "url arg", + repoName: "jill/valentine", + opts: &ViewOptions{ + RepoArg: "https://github.com/jill/valentine", + }, + stdoutTTY: true, + wantOut: heredoc.Doc(` + jill/valentine + social distancing + + + # truly cool readme check it out + + + + View this repository on GitHub: https://github.com/jill/valentine + `), + }, + { + name: "name arg", + repoName: "jill/valentine", + opts: &ViewOptions{ + RepoArg: "jill/valentine", + }, + stdoutTTY: true, + wantOut: heredoc.Doc(` + jill/valentine + social distancing + + + # truly cool readme check it out + + + + View this repository on GitHub: https://github.com/jill/valentine + `), + }, + { + name: "no args", + stdoutTTY: true, + wantOut: heredoc.Doc(` + OWNER/REPO + social distancing + + + # truly cool readme check it out + + + + View this repository on GitHub: https://github.com/OWNER/REPO + `), + }, + } + for _, tt := range tests { + if tt.opts == nil { + tt.opts = &ViewOptions{} + } + + if tt.repoName == "" { + tt.repoName = "OWNER/REPO" + } + + tt.opts.BaseRepo = func() (ghrepo.Interface, error) { + repo, _ := ghrepo.FromFullName(tt.repoName) + return repo, nil + } + + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { + "repository": { + "description": "social distancing" + } } }`)) + reg.Register( + httpmock.REST("GET", fmt.Sprintf("repos/%s/readme", tt.repoName)), + httpmock.StringResponse(` + { "name": "readme.md", + "content": "IyB0cnVseSBjb29sIHJlYWRtZSBjaGVjayBpdCBvdXQ="}`)) + + tt.opts.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + + io, _, stdout, stderr := iostreams.Test() + tt.opts.IO = io + + t.Run(tt.name, func(t *testing.T) { + io.SetStdoutTTY(tt.stdoutTTY) + + if err := viewRun(tt.opts); (err != nil) != tt.wantErr { + t.Errorf("viewRun() error = %v, wantErr %v", err, tt.wantErr) + } + assert.Equal(t, tt.wantStderr, stderr.String()) + assert.Equal(t, tt.wantOut, stdout.String()) + reg.Verify(t) + }) + } +} + +func Test_ViewRun_NonMarkdownReadme(t *testing.T) { + tests := []struct { + name string + stdoutTTY bool + wantOut string + }{ + { + name: "tty", + wantOut: heredoc.Doc(` + OWNER/REPO + social distancing + + # truly cool readme check it out + + View this repository on GitHub: https://github.com/OWNER/REPO + `), + stdoutTTY: true, + }, + { + name: "nontty", + wantOut: heredoc.Doc(` + name: OWNER/REPO + description: social distancing + -- + # truly cool readme check it out + `), + }, + } + + for _, tt := range tests { + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { + "repository": { + "description": "social distancing" + } } }`)) + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/readme"), + httpmock.StringResponse(` + { "name": "readme.org", + "content": "IyB0cnVseSBjb29sIHJlYWRtZSBjaGVjayBpdCBvdXQ="}`)) + + opts := &ViewOptions{ + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + } + + io, _, stdout, stderr := iostreams.Test() + + opts.IO = io + + t.Run(tt.name, func(t *testing.T) { + io.SetStdoutTTY(tt.stdoutTTY) + + if err := viewRun(opts); err != nil { + t.Errorf("viewRun() error = %v", err) + } + assert.Equal(t, tt.wantOut, stdout.String()) + assert.Equal(t, "", stderr.String()) + reg.Verify(t) + }) + } +} + +func Test_ViewRun_NoReadme(t *testing.T) { + tests := []struct { + name string + stdoutTTY bool + wantOut string + }{ + { + name: "tty", + wantOut: heredoc.Doc(` + OWNER/REPO + social distancing + + This repository does not have a README + + View this repository on GitHub: https://github.com/OWNER/REPO + `), + stdoutTTY: true, + }, + { + name: "nontty", + wantOut: heredoc.Doc(` + name: OWNER/REPO + description: social distancing + `), + }, + } + + for _, tt := range tests { + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { + "repository": { + "description": "social distancing" + } } }`)) + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/readme"), + httpmock.StatusStringResponse(404, `{}`)) + + opts := &ViewOptions{ + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + } + + io, _, stdout, stderr := iostreams.Test() + + opts.IO = io + + t.Run(tt.name, func(t *testing.T) { + io.SetStdoutTTY(tt.stdoutTTY) + + if err := viewRun(opts); err != nil { + t.Errorf("viewRun() error = %v", err) + } + assert.Equal(t, tt.wantOut, stdout.String()) + assert.Equal(t, "", stderr.String()) + reg.Verify(t) + }) + } +} + +func Test_ViewRun_NoDescription(t *testing.T) { + tests := []struct { + name string + stdoutTTY bool + wantOut string + }{ + { + name: "tty", + wantOut: heredoc.Doc(` + OWNER/REPO + No description provided + + # truly cool readme check it out + + View this repository on GitHub: https://github.com/OWNER/REPO + `), + stdoutTTY: true, + }, + { + name: "nontty", + wantOut: heredoc.Doc(` + name: OWNER/REPO + description: + -- + # truly cool readme check it out + `), + }, + } + + for _, tt := range tests { + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(` + { "data": { + "repository": { + "description": "" + } } }`)) + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/readme"), + httpmock.StringResponse(` + { "name": "readme.org", + "content": "IyB0cnVseSBjb29sIHJlYWRtZSBjaGVjayBpdCBvdXQ="}`)) + + opts := &ViewOptions{ + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.New("OWNER", "REPO"), nil + }, + } + + io, _, stdout, stderr := iostreams.Test() + + opts.IO = io + + t.Run(tt.name, func(t *testing.T) { + io.SetStdoutTTY(tt.stdoutTTY) + + if err := viewRun(opts); err != nil { + t.Errorf("viewRun() error = %v", err) + } + assert.Equal(t, tt.wantOut, stdout.String()) + assert.Equal(t, "", stderr.String()) + reg.Verify(t) + }) + } +} diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index 91660a135..f2e31c439 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -17,8 +17,10 @@ type IOStreams struct { colorEnabled bool - stdinTTYOverride bool - stdinIsTTY bool + stdinTTYOverride bool + stdinIsTTY bool + stdoutTTYOverride bool + stdoutIsTTY bool } func (s *IOStreams) ColorEnabled() bool { @@ -40,6 +42,21 @@ func (s *IOStreams) IsStdinTTY() bool { return false } +func (s *IOStreams) SetStdoutTTY(isTTY bool) { + s.stdoutTTYOverride = true + s.stdoutIsTTY = isTTY +} + +func (s *IOStreams) IsStdoutTTY() bool { + if s.stdoutTTYOverride { + return s.stdoutIsTTY + } + if stdout, ok := s.Out.(*os.File); ok { + return isTerminal(stdout) + } + return false +} + func System() *IOStreams { var out io.Writer = os.Stdout var colorEnabled bool From 7b8d226e0f69e4ccf2f0262edba760b7b16625cf Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 22 Jul 2020 11:15:54 -0500 Subject: [PATCH 02/12] remove redundant statement --- pkg/cmd/gist/create/create.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/cmd/gist/create/create.go b/pkg/cmd/gist/create/create.go index 2bfedf684..76cdd1463 100644 --- a/pkg/cmd/gist/create/create.go +++ b/pkg/cmd/gist/create/create.go @@ -70,9 +70,6 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co return nil }, RunE: func(c *cobra.Command, args []string) error { - // TODO redundant? - opts.HttpClient = f.HttpClient - opts.Filenames = args if runF != nil { From 2a99ed9afb45f7198aac211b5c363d56e1c0e334 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 23 Jul 2020 10:37:01 -0500 Subject: [PATCH 03/12] add ResolvedBaseRepo --- command/root.go | 20 ++++++++++++++++++++ pkg/cmd/repo/view/http.go | 6 ++++-- pkg/cmd/repo/view/view.go | 26 +++++++++++++------------- pkg/cmd/repo/view/view_test.go | 10 +++++----- pkg/cmdutil/factory.go | 7 ++++--- 5 files changed, 46 insertions(+), 23 deletions(-) diff --git a/command/root.go b/command/root.go index b988fddfb..98399ccf7 100644 --- a/command/root.go +++ b/command/root.go @@ -90,6 +90,26 @@ func init() { } return httpClient(token), nil }, + ResolvedBaseRepo: func(client *http.Client) (ghrepo.Interface, error) { + // TODO this may be abandoned when we stop trying to be magical about picking base repos for + // users. We can merge this with BaseRepo at that point. + apiClient := api.NewClientFromHTTP(client) + ctx := context.New() + remotes, err := ctx.Remotes() + if err != nil { + return nil, err + } + repoContext, err := context.ResolveRemotesToRepos(remotes, apiClient, "") + if err != nil { + return nil, err + } + baseRepo, err := repoContext.BaseRepo() + if err != nil { + return nil, err + } + + return baseRepo, nil + }, BaseRepo: func() (ghrepo.Interface, error) { // TODO: decouple from `context` ctx := context.New() diff --git a/pkg/cmd/repo/view/http.go b/pkg/cmd/repo/view/http.go index 4b82ef448..98baa86f3 100644 --- a/pkg/cmd/repo/view/http.go +++ b/pkg/cmd/repo/view/http.go @@ -4,6 +4,7 @@ import ( "encoding/base64" "errors" "fmt" + "net/http" "github.com/cli/cli/api" "github.com/cli/cli/internal/ghrepo" @@ -14,13 +15,14 @@ type RepoReadme struct { Content string } -func RepositoryReadme(client *api.Client, repo ghrepo.Interface) (*RepoReadme, error) { +func RepositoryReadme(client *http.Client, repo ghrepo.Interface) (*RepoReadme, error) { + apiClient := api.NewClientFromHTTP(client) var response struct { Name string Content string } - err := client.REST("GET", fmt.Sprintf("repos/%s/readme", ghrepo.FullName(repo)), nil, &response) + err := apiClient.REST("GET", fmt.Sprintf("repos/%s/readme", ghrepo.FullName(repo)), nil, &response) if err != nil { var httpError api.HTTPError if errors.As(err, &httpError) && httpError.StatusCode == 404 { diff --git a/pkg/cmd/repo/view/view.go b/pkg/cmd/repo/view/view.go index 7b4d723c7..c5b49e361 100644 --- a/pkg/cmd/repo/view/view.go +++ b/pkg/cmd/repo/view/view.go @@ -18,9 +18,9 @@ import ( ) type ViewOptions struct { - HttpClient func() (*http.Client, error) - IO *iostreams.IOStreams - BaseRepo func() (ghrepo.Interface, error) + HttpClient func() (*http.Client, error) + IO *iostreams.IOStreams + ResolvedBaseRepo func(*http.Client) (ghrepo.Interface, error) RepoArg string Web bool @@ -28,9 +28,9 @@ type ViewOptions struct { func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Command { opts := ViewOptions{ - IO: f.IOStreams, - HttpClient: f.HttpClient, - BaseRepo: f.BaseRepo, + IO: f.IOStreams, + HttpClient: f.HttpClient, + ResolvedBaseRepo: f.ResolvedBaseRepo, } cmd := &cobra.Command{ @@ -59,10 +59,15 @@ With '--web', open the repository in a web browser instead.`, } func viewRun(opts *ViewOptions) error { + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + var toView ghrepo.Interface if opts.RepoArg == "" { var err error - toView, err = opts.BaseRepo() + toView, err = opts.ResolvedBaseRepo(httpClient) if err != nil { return err } @@ -86,11 +91,6 @@ func viewRun(opts *ViewOptions) error { } } - httpClient, err := opts.HttpClient() - if err != nil { - return err - } - apiClient := api.NewClientFromHTTP(httpClient) repo, err := api.GitHubRepo(apiClient, toView) @@ -108,7 +108,7 @@ func viewRun(opts *ViewOptions) error { fullName := ghrepo.FullName(toView) - readme, err := RepositoryReadme(apiClient, toView) + readme, err := RepositoryReadme(httpClient, toView) var notFound *api.NotFoundError if err != nil && !errors.As(err, ¬Found) { return err diff --git a/pkg/cmd/repo/view/view_test.go b/pkg/cmd/repo/view/view_test.go index 5a6d08702..810e6b573 100644 --- a/pkg/cmd/repo/view/view_test.go +++ b/pkg/cmd/repo/view/view_test.go @@ -113,7 +113,7 @@ func Test_RepoView_Web(t *testing.T) { HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - BaseRepo: func() (ghrepo.Interface, error) { + ResolvedBaseRepo: func(_ *http.Client) (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil }, } @@ -225,7 +225,7 @@ func Test_ViewRun(t *testing.T) { tt.repoName = "OWNER/REPO" } - tt.opts.BaseRepo = func() (ghrepo.Interface, error) { + tt.opts.ResolvedBaseRepo = func(_ *http.Client) (ghrepo.Interface, error) { repo, _ := ghrepo.FromFullName(tt.repoName) return repo, nil } @@ -312,7 +312,7 @@ func Test_ViewRun_NonMarkdownReadme(t *testing.T) { HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - BaseRepo: func() (ghrepo.Interface, error) { + ResolvedBaseRepo: func(_ *http.Client) (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil }, } @@ -378,7 +378,7 @@ func Test_ViewRun_NoReadme(t *testing.T) { HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - BaseRepo: func() (ghrepo.Interface, error) { + ResolvedBaseRepo: func(_ *http.Client) (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil }, } @@ -448,7 +448,7 @@ func Test_ViewRun_NoDescription(t *testing.T) { HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - BaseRepo: func() (ghrepo.Interface, error) { + ResolvedBaseRepo: func(_ *http.Client) (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil }, } diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go index ad7162415..0cdc2915a 100644 --- a/pkg/cmdutil/factory.go +++ b/pkg/cmdutil/factory.go @@ -8,7 +8,8 @@ import ( ) type Factory struct { - IOStreams *iostreams.IOStreams - HttpClient func() (*http.Client, error) - BaseRepo func() (ghrepo.Interface, error) + IOStreams *iostreams.IOStreams + HttpClient func() (*http.Client, error) + ResolvedBaseRepo func(*http.Client) (ghrepo.Interface, error) + BaseRepo func() (ghrepo.Interface, error) } From 12e88da4003d9a17feea29a46b577d2dae60f09a Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 23 Jul 2020 11:10:25 -0500 Subject: [PATCH 04/12] w h i t e s p a c e --- pkg/cmd/repo/view/view.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/repo/view/view.go b/pkg/cmd/repo/view/view.go index c5b49e361..0617f8d9a 100644 --- a/pkg/cmd/repo/view/view.go +++ b/pkg/cmd/repo/view/view.go @@ -135,7 +135,7 @@ func viewRun(opts *ViewOptions) error { {{.Readme}} {{.View}} - `) + `) tmpl, err := template.New("repo").Parse(repoTmpl) if err != nil { From 895af993a5515873a837a47507448a3ccb6b188e Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 23 Jul 2020 11:12:33 -0500 Subject: [PATCH 05/12] static not found error --- api/queries_pr.go | 4 ---- pkg/cmd/repo/view/http.go | 4 +++- pkg/cmd/repo/view/view.go | 4 +--- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 46e036465..4437f8f7e 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -134,10 +134,6 @@ type NotFoundError struct { error } -func NewNotFoundError(err error) *NotFoundError { - return &NotFoundError{err} -} - func (err *NotFoundError) Unwrap() error { return err.error } diff --git a/pkg/cmd/repo/view/http.go b/pkg/cmd/repo/view/http.go index 98baa86f3..343efe7af 100644 --- a/pkg/cmd/repo/view/http.go +++ b/pkg/cmd/repo/view/http.go @@ -10,6 +10,8 @@ import ( "github.com/cli/cli/internal/ghrepo" ) +var NotFoundError = errors.New("not found") + type RepoReadme struct { Filename string Content string @@ -26,7 +28,7 @@ func RepositoryReadme(client *http.Client, repo ghrepo.Interface) (*RepoReadme, if err != nil { var httpError api.HTTPError if errors.As(err, &httpError) && httpError.StatusCode == 404 { - return nil, api.NewNotFoundError(err) + return nil, NotFoundError } return nil, err } diff --git a/pkg/cmd/repo/view/view.go b/pkg/cmd/repo/view/view.go index 0617f8d9a..f3a5cb998 100644 --- a/pkg/cmd/repo/view/view.go +++ b/pkg/cmd/repo/view/view.go @@ -1,7 +1,6 @@ package view import ( - "errors" "fmt" "html/template" "net/http" @@ -109,8 +108,7 @@ func viewRun(opts *ViewOptions) error { fullName := ghrepo.FullName(toView) readme, err := RepositoryReadme(httpClient, toView) - var notFound *api.NotFoundError - if err != nil && !errors.As(err, ¬Found) { + if err != nil && err != NotFoundError { return err } From dc023661280988cb8d2bd8cfb14255d0537d032c Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 23 Jul 2020 11:16:47 -0500 Subject: [PATCH 06/12] move generateRepoURL to ghrepo --- command/issue.go | 14 +++----------- command/pr.go | 2 +- command/pr_create.go | 2 +- internal/ghrepo/repo.go | 8 ++++++++ pkg/cmd/repo/view/view.go | 10 +--------- 5 files changed, 14 insertions(+), 22 deletions(-) diff --git a/command/issue.go b/command/issue.go index 5de86da91..0bc870dcc 100644 --- a/command/issue.go +++ b/command/issue.go @@ -227,7 +227,7 @@ func issueList(cmd *cobra.Command, args []string) error { } if web { - issueListURL := generateRepoURL(baseRepo, "issues") + issueListURL := ghrepo.GenerateRepoURL(baseRepo, "issues") openURL, err := listURLWithQuery(issueListURL, filterOptions{ entity: "issue", state: state, @@ -504,7 +504,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { } if isWeb, err := cmd.Flags().GetBool("web"); err == nil && isWeb { - openURL := generateRepoURL(baseRepo, "issues/new") + openURL := ghrepo.GenerateRepoURL(baseRepo, "issues/new") if title != "" || body != "" { milestone := "" if len(milestoneTitles) > 0 { @@ -582,7 +582,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { } if action == PreviewAction { - openURL := generateRepoURL(baseRepo, "issues/new") + openURL := ghrepo.GenerateRepoURL(baseRepo, "issues/new") milestone := "" if len(milestoneTitles) > 0 { milestone = milestoneTitles[0] @@ -618,14 +618,6 @@ func issueCreate(cmd *cobra.Command, args []string) error { return nil } -func generateRepoURL(repo ghrepo.Interface, p string, args ...interface{}) string { - baseURL := fmt.Sprintf("https://%s/%s/%s", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) - if p != "" { - return baseURL + "/" + fmt.Sprintf(p, args...) - } - return baseURL -} - func addMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, params map[string]interface{}, tb *issueMetadataState) error { if !tb.HasMetadata() { return nil diff --git a/command/pr.go b/command/pr.go index 9978d18be..6d276d833 100644 --- a/command/pr.go +++ b/command/pr.go @@ -235,7 +235,7 @@ func prList(cmd *cobra.Command, args []string) error { } if web { - prListURL := generateRepoURL(baseRepo, "pulls") + prListURL := ghrepo.GenerateRepoURL(baseRepo, "pulls") openURL, err := listURLWithQuery(prListURL, filterOptions{ entity: "pr", state: state, diff --git a/command/pr_create.go b/command/pr_create.go index b66a7ff5d..97dc018d1 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -447,7 +447,7 @@ func withPrAndIssueQueryParams(baseURL, title, body string, assignees, labels, p } func generateCompareURL(r ghrepo.Interface, base, head, title, body string, assignees, labels, projects []string, milestone string) (string, error) { - u := generateRepoURL(r, "compare/%s...%s?expand=1", base, head) + u := ghrepo.GenerateRepoURL(r, "compare/%s...%s?expand=1", base, head) url, err := withPrAndIssueQueryParams(u, title, body, assignees, labels, projects, milestone) if err != nil { return "", err diff --git a/internal/ghrepo/repo.go b/internal/ghrepo/repo.go index fbe748204..9b0a7ac30 100644 --- a/internal/ghrepo/repo.go +++ b/internal/ghrepo/repo.go @@ -77,6 +77,14 @@ func IsSame(a, b Interface) bool { normalizeHostname(a.RepoHost()) == normalizeHostname(b.RepoHost()) } +func GenerateRepoURL(repo Interface, p string, args ...interface{}) string { + baseURL := fmt.Sprintf("https://%s/%s/%s", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) + if p != "" { + return baseURL + "/" + fmt.Sprintf(p, args...) + } + return baseURL +} + type ghRepo struct { owner string name string diff --git a/pkg/cmd/repo/view/view.go b/pkg/cmd/repo/view/view.go index f3a5cb998..aaa553371 100644 --- a/pkg/cmd/repo/view/view.go +++ b/pkg/cmd/repo/view/view.go @@ -97,7 +97,7 @@ func viewRun(opts *ViewOptions) error { return err } - openURL := generateRepoURL(toView, "") + openURL := ghrepo.GenerateRepoURL(toView, "") if opts.Web { if opts.IO.IsStdoutTTY() { fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", displayURL(openURL)) @@ -192,14 +192,6 @@ func isURL(arg string) bool { return strings.HasPrefix(arg, "http:/") || strings.HasPrefix(arg, "https:/") } -func generateRepoURL(repo ghrepo.Interface, p string, args ...interface{}) string { - baseURL := fmt.Sprintf("https://%s/%s/%s", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) - if p != "" { - return baseURL + "/" + fmt.Sprintf(p, args...) - } - return baseURL -} - func displayURL(urlStr string) string { u, err := url.Parse(urlStr) if err != nil { From 70c948a7519e7e3f74dedf44baf5bcdace227ec8 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 23 Jul 2020 11:23:45 -0500 Subject: [PATCH 07/12] move isURL --- command/repo.go | 6 +----- pkg/cmd/repo/view/view.go | 6 +----- utils/utils.go | 4 ++++ 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/command/repo.go b/command/repo.go index e0114a080..364366b3e 100644 --- a/command/repo.go +++ b/command/repo.go @@ -353,10 +353,6 @@ func repoCreate(cmd *cobra.Command, args []string) error { return nil } -func isURL(arg string) bool { - return strings.HasPrefix(arg, "http:/") || strings.HasPrefix(arg, "https:/") -} - var Since = func(t time.Time) time.Duration { return time.Since(t) } @@ -390,7 +386,7 @@ func repoFork(cmd *cobra.Command, args []string) error { } else { repoArg := args[0] - if isURL(repoArg) { + if utils.IsURL(repoArg) { parsedURL, err := url.Parse(repoArg) if err != nil { return fmt.Errorf("did not understand argument: %w", err) diff --git a/pkg/cmd/repo/view/view.go b/pkg/cmd/repo/view/view.go index aaa553371..b4c0436d9 100644 --- a/pkg/cmd/repo/view/view.go +++ b/pkg/cmd/repo/view/view.go @@ -71,7 +71,7 @@ func viewRun(opts *ViewOptions) error { return err } } else { - if isURL(opts.RepoArg) { + if utils.IsURL(opts.RepoArg) { parsedURL, err := url.Parse(opts.RepoArg) if err != nil { return fmt.Errorf("did not understand argument: %w", err) @@ -188,10 +188,6 @@ func isMarkdownFile(filename string) bool { } // TODO COPYPASTA FROM command; CONSIDER FOR cmdutil? -func isURL(arg string) bool { - return strings.HasPrefix(arg, "http:/") || strings.HasPrefix(arg, "https:/") -} - func displayURL(urlStr string) string { u, err := url.Parse(urlStr) if err != nil { diff --git a/utils/utils.go b/utils/utils.go index 2f100883f..ca011b9f3 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -102,3 +102,7 @@ var StopSpinner = func(s *spinner.Spinner) { func Spinner(w io.Writer) *spinner.Spinner { return spinner.New(spinner.CharSets[11], 400*time.Millisecond, spinner.WithWriter(w)) } + +func IsURL(s string) bool { + return strings.HasPrefix(s, "http:/") || strings.HasPrefix(s, "https:/") +} From 086fb48d31be4a58872b3a4307441613bd10eb53 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 23 Jul 2020 11:25:31 -0500 Subject: [PATCH 08/12] move displayURL --- command/issue.go | 14 +++----------- command/pr.go | 2 +- command/pr_create.go | 2 +- pkg/cmd/repo/view/view.go | 11 +---------- utils/utils.go | 9 +++++++++ 5 files changed, 15 insertions(+), 23 deletions(-) diff --git a/command/issue.go b/command/issue.go index 0bc870dcc..43049faa2 100644 --- a/command/issue.go +++ b/command/issue.go @@ -240,7 +240,7 @@ func issueList(cmd *cobra.Command, args []string) error { if err != nil { return err } - fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", displayURL(openURL)) + fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", utils.DisplayURL(openURL)) return utils.OpenInBrowser(openURL) } @@ -518,7 +518,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { openURL += "/choose" } if connectedToTerminal(cmd) { - cmd.Printf("Opening %s in your browser.\n", displayURL(openURL)) + cmd.Printf("Opening %s in your browser.\n", utils.DisplayURL(openURL)) } return utils.OpenInBrowser(openURL) } @@ -592,7 +592,7 @@ func issueCreate(cmd *cobra.Command, args []string) error { return err } // TODO could exceed max url length for explorer - fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", displayURL(openURL)) + fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", utils.DisplayURL(openURL)) return utils.OpenInBrowser(openURL) } else if action == SubmitAction { params := map[string]interface{}{ @@ -836,11 +836,3 @@ func issueReopen(cmd *cobra.Command, args []string) error { return nil } - -func displayURL(urlStr string) string { - u, err := url.Parse(urlStr) - if err != nil { - return urlStr - } - return u.Hostname() + u.Path -} diff --git a/command/pr.go b/command/pr.go index 6d276d833..2ef865066 100644 --- a/command/pr.go +++ b/command/pr.go @@ -246,7 +246,7 @@ func prList(cmd *cobra.Command, args []string) error { if err != nil { return err } - fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", displayURL(openURL)) + fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", utils.DisplayURL(openURL)) return utils.OpenInBrowser(openURL) } diff --git a/command/pr_create.go b/command/pr_create.go index 97dc018d1..b9efca0fa 100644 --- a/command/pr_create.go +++ b/command/pr_create.go @@ -367,7 +367,7 @@ func prCreate(cmd *cobra.Command, _ []string) error { } if connectedToTerminal(cmd) { // TODO could exceed max url length for explorer - fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", displayURL(openURL)) + fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", utils.DisplayURL(openURL)) } return utils.OpenInBrowser(openURL) } else { diff --git a/pkg/cmd/repo/view/view.go b/pkg/cmd/repo/view/view.go index b4c0436d9..eed90a9ad 100644 --- a/pkg/cmd/repo/view/view.go +++ b/pkg/cmd/repo/view/view.go @@ -100,7 +100,7 @@ func viewRun(opts *ViewOptions) error { openURL := ghrepo.GenerateRepoURL(toView, "") if opts.Web { if opts.IO.IsStdoutTTY() { - fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", displayURL(openURL)) + fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", utils.DisplayURL(openURL)) } return utils.OpenInBrowser(openURL) } @@ -186,12 +186,3 @@ func isMarkdownFile(filename string) bool { strings.HasSuffix(filename, ".mdown") || strings.HasSuffix(filename, ".mkdown") } - -// TODO COPYPASTA FROM command; CONSIDER FOR cmdutil? -func displayURL(urlStr string) string { - u, err := url.Parse(urlStr) - if err != nil { - return urlStr - } - return u.Hostname() + u.Path -} diff --git a/utils/utils.go b/utils/utils.go index ca011b9f3..c03d6ccf3 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -3,6 +3,7 @@ package utils import ( "fmt" "io" + "net/url" "strings" "time" @@ -106,3 +107,11 @@ func Spinner(w io.Writer) *spinner.Spinner { func IsURL(s string) bool { return strings.HasPrefix(s, "http:/") || strings.HasPrefix(s, "https:/") } + +func DisplayURL(urlStr string) string { + u, err := url.Parse(urlStr) + if err != nil { + return urlStr + } + return u.Hostname() + u.Path +} From d92c80b560bcd1aa22696c8d8bddf5bb02c44a3d Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 23 Jul 2020 11:58:22 -0500 Subject: [PATCH 09/12] just swap BaseRepo implementation --- command/root.go | 20 ------------------ pkg/cmd/repo/view/view.go | 38 +++++++++++++++++++++++++++------- pkg/cmd/repo/view/view_test.go | 10 ++++----- pkg/cmdutil/factory.go | 7 +++---- 4 files changed, 39 insertions(+), 36 deletions(-) diff --git a/command/root.go b/command/root.go index 98399ccf7..b988fddfb 100644 --- a/command/root.go +++ b/command/root.go @@ -90,26 +90,6 @@ func init() { } return httpClient(token), nil }, - ResolvedBaseRepo: func(client *http.Client) (ghrepo.Interface, error) { - // TODO this may be abandoned when we stop trying to be magical about picking base repos for - // users. We can merge this with BaseRepo at that point. - apiClient := api.NewClientFromHTTP(client) - ctx := context.New() - remotes, err := ctx.Remotes() - if err != nil { - return nil, err - } - repoContext, err := context.ResolveRemotesToRepos(remotes, apiClient, "") - if err != nil { - return nil, err - } - baseRepo, err := repoContext.BaseRepo() - if err != nil { - return nil, err - } - - return baseRepo, nil - }, BaseRepo: func() (ghrepo.Interface, error) { // TODO: decouple from `context` ctx := context.New() diff --git a/pkg/cmd/repo/view/view.go b/pkg/cmd/repo/view/view.go index eed90a9ad..6e8e5b8cf 100644 --- a/pkg/cmd/repo/view/view.go +++ b/pkg/cmd/repo/view/view.go @@ -9,6 +9,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/api" + "github.com/cli/cli/context" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" @@ -17,9 +18,9 @@ import ( ) type ViewOptions struct { - HttpClient func() (*http.Client, error) - IO *iostreams.IOStreams - ResolvedBaseRepo func(*http.Client) (ghrepo.Interface, error) + HttpClient func() (*http.Client, error) + IO *iostreams.IOStreams + BaseRepo func() (ghrepo.Interface, error) RepoArg string Web bool @@ -27,9 +28,32 @@ type ViewOptions struct { func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Command { opts := ViewOptions{ - IO: f.IOStreams, - HttpClient: f.HttpClient, - ResolvedBaseRepo: f.ResolvedBaseRepo, + IO: f.IOStreams, + HttpClient: f.HttpClient, + BaseRepo: func() (ghrepo.Interface, error) { + httpClient, err := f.HttpClient() + if err != nil { + return nil, err + } + + apiClient := api.NewClientFromHTTP(httpClient) + + ctx := context.New() + remotes, err := ctx.Remotes() + if err != nil { + return nil, err + } + repoContext, err := context.ResolveRemotesToRepos(remotes, apiClient, "") + if err != nil { + return nil, err + } + baseRepo, err := repoContext.BaseRepo() + if err != nil { + return nil, err + } + + return baseRepo, nil + }, } cmd := &cobra.Command{ @@ -66,7 +90,7 @@ func viewRun(opts *ViewOptions) error { var toView ghrepo.Interface if opts.RepoArg == "" { var err error - toView, err = opts.ResolvedBaseRepo(httpClient) + toView, err = opts.BaseRepo() if err != nil { return err } diff --git a/pkg/cmd/repo/view/view_test.go b/pkg/cmd/repo/view/view_test.go index 810e6b573..5a6d08702 100644 --- a/pkg/cmd/repo/view/view_test.go +++ b/pkg/cmd/repo/view/view_test.go @@ -113,7 +113,7 @@ func Test_RepoView_Web(t *testing.T) { HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - ResolvedBaseRepo: func(_ *http.Client) (ghrepo.Interface, error) { + BaseRepo: func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil }, } @@ -225,7 +225,7 @@ func Test_ViewRun(t *testing.T) { tt.repoName = "OWNER/REPO" } - tt.opts.ResolvedBaseRepo = func(_ *http.Client) (ghrepo.Interface, error) { + tt.opts.BaseRepo = func() (ghrepo.Interface, error) { repo, _ := ghrepo.FromFullName(tt.repoName) return repo, nil } @@ -312,7 +312,7 @@ func Test_ViewRun_NonMarkdownReadme(t *testing.T) { HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - ResolvedBaseRepo: func(_ *http.Client) (ghrepo.Interface, error) { + BaseRepo: func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil }, } @@ -378,7 +378,7 @@ func Test_ViewRun_NoReadme(t *testing.T) { HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - ResolvedBaseRepo: func(_ *http.Client) (ghrepo.Interface, error) { + BaseRepo: func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil }, } @@ -448,7 +448,7 @@ func Test_ViewRun_NoDescription(t *testing.T) { HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil }, - ResolvedBaseRepo: func(_ *http.Client) (ghrepo.Interface, error) { + BaseRepo: func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil }, } diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go index 0cdc2915a..ad7162415 100644 --- a/pkg/cmdutil/factory.go +++ b/pkg/cmdutil/factory.go @@ -8,8 +8,7 @@ import ( ) type Factory struct { - IOStreams *iostreams.IOStreams - HttpClient func() (*http.Client, error) - ResolvedBaseRepo func(*http.Client) (ghrepo.Interface, error) - BaseRepo func() (ghrepo.Interface, error) + IOStreams *iostreams.IOStreams + HttpClient func() (*http.Client, error) + BaseRepo func() (ghrepo.Interface, error) } From b9ce1a1dafe1f9e2368138c0cd097ac7be0d6afb Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 23 Jul 2020 12:13:27 -0500 Subject: [PATCH 10/12] pass resolving baserepo into newcmdview --- command/root.go | 31 ++++++++++++++++++++++++++++++- pkg/cmd/repo/view/view.go | 26 +------------------------- 2 files changed, 31 insertions(+), 26 deletions(-) diff --git a/command/root.go b/command/root.go index b988fddfb..ab3b60140 100644 --- a/command/root.go +++ b/command/root.go @@ -106,8 +106,37 @@ func init() { RootCmd.AddCommand(gistCmd) gistCmd.AddCommand(gistCreateCmd.NewCmdCreate(cmdFactory, nil)) + resolvedBaseRepo := func() (ghrepo.Interface, error) { + httpClient, err := cmdFactory.HttpClient() + if err != nil { + return nil, err + } + + apiClient := api.NewClientFromHTTP(httpClient) + + ctx := context.New() + remotes, err := ctx.Remotes() + if err != nil { + return nil, err + } + repoContext, err := context.ResolveRemotesToRepos(remotes, apiClient, "") + if err != nil { + return nil, err + } + baseRepo, err := repoContext.BaseRepo() + if err != nil { + return nil, err + } + + return baseRepo, nil + } + + repoResolvingCmdFactory := *cmdFactory + + repoResolvingCmdFactory.BaseRepo = resolvedBaseRepo + RootCmd.AddCommand(repoCmd) - repoCmd.AddCommand(repoViewCmd.NewCmdView(cmdFactory, nil)) + repoCmd.AddCommand(repoViewCmd.NewCmdView(&repoResolvingCmdFactory, nil)) } // RootCmd is the entry point of command-line execution diff --git a/pkg/cmd/repo/view/view.go b/pkg/cmd/repo/view/view.go index 6e8e5b8cf..f4741e9cd 100644 --- a/pkg/cmd/repo/view/view.go +++ b/pkg/cmd/repo/view/view.go @@ -9,7 +9,6 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/api" - "github.com/cli/cli/context" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" @@ -30,30 +29,7 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman opts := ViewOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, - BaseRepo: func() (ghrepo.Interface, error) { - httpClient, err := f.HttpClient() - if err != nil { - return nil, err - } - - apiClient := api.NewClientFromHTTP(httpClient) - - ctx := context.New() - remotes, err := ctx.Remotes() - if err != nil { - return nil, err - } - repoContext, err := context.ResolveRemotesToRepos(remotes, apiClient, "") - if err != nil { - return nil, err - } - baseRepo, err := repoContext.BaseRepo() - if err != nil { - return nil, err - } - - return baseRepo, nil - }, + BaseRepo: f.BaseRepo, } cmd := &cobra.Command{ From 1831d95433c59d1d7b83774924d2a377a632fd90 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 23 Jul 2020 11:49:25 -0500 Subject: [PATCH 11/12] isolated clone command This commit hacks the existing repo clone tests into something usable by the new isolated command. It went ok and was less effort than trying to introduce the same kind of test format as repo view and gist create. --- command/repo.go | 120 +++---------------- command/repo_test.go | 185 ----------------------------- command/root.go | 12 ++ git/git.go | 43 +++++++ git/git_test.go | 60 ++++++++++ internal/ghrepo/repo.go | 9 ++ pkg/cmd/repo/clone/clone.go | 126 ++++++++++++++++++++ pkg/cmd/repo/clone/clone_test.go | 195 +++++++++++++++++++++++++++++++ pkg/cmdutil/factory.go | 2 + 9 files changed, 461 insertions(+), 291 deletions(-) create mode 100644 pkg/cmd/repo/clone/clone.go create mode 100644 pkg/cmd/repo/clone/clone_test.go diff --git a/command/repo.go b/command/repo.go index 364366b3e..92ea292a5 100644 --- a/command/repo.go +++ b/command/repo.go @@ -20,8 +20,6 @@ import ( ) func init() { - repoCmd.AddCommand(repoCloneCmd) - repoCmd.AddCommand(repoCreateCmd) repoCreateCmd.Flags().StringP("description", "d", "", "Description of repository") repoCreateCmd.Flags().StringP("homepage", "h", "", "Repository home page URL") @@ -57,19 +55,6 @@ A repository can be supplied as an argument in any of the following formats: - by URL, e.g. "https://github.com/OWNER/REPO"`}, } -var repoCloneCmd = &cobra.Command{ - Use: "clone []", - Args: cobra.MinimumNArgs(1), - Short: "Clone a repository locally", - Long: `Clone a GitHub repository locally. - -If the "OWNER/" portion of the "OWNER/REPO" repository argument is omitted, it -defaults to the name of the authenticating user. - -To pass 'git clone' flags, separate them with '--'.`, - RunE: repoClone, -} - var repoCreateCmd = &cobra.Command{ Use: "create []", Short: "Create a new repository", @@ -120,95 +105,6 @@ var repoCreditsCmd = &cobra.Command{ Hidden: true, } -func parseCloneArgs(extraArgs []string) (args []string, target string) { - args = extraArgs - - if len(args) > 0 { - if !strings.HasPrefix(args[0], "-") { - target, args = args[0], args[1:] - } - } - return -} - -func runClone(cloneURL string, args []string) (target string, err error) { - cloneArgs, target := parseCloneArgs(args) - - cloneArgs = append(cloneArgs, cloneURL) - - // If the args contain an explicit target, pass it to clone - // otherwise, parse the URL to determine where git cloned it to so we can return it - if target != "" { - cloneArgs = append(cloneArgs, target) - } else { - target = path.Base(strings.TrimSuffix(cloneURL, ".git")) - } - - cloneArgs = append([]string{"clone"}, cloneArgs...) - - cloneCmd := git.GitCommand(cloneArgs...) - cloneCmd.Stdin = os.Stdin - cloneCmd.Stdout = os.Stdout - cloneCmd.Stderr = os.Stderr - - err = run.PrepareCmd(cloneCmd).Run() - return -} - -func repoClone(cmd *cobra.Command, args []string) error { - ctx := contextForCommand(cmd) - apiClient, err := apiClientForContext(ctx) - if err != nil { - return err - } - - cloneURL := args[0] - if !strings.Contains(cloneURL, ":") { - if !strings.Contains(cloneURL, "/") { - currentUser, err := api.CurrentLoginName(apiClient) - if err != nil { - return err - } - cloneURL = currentUser + "/" + cloneURL - } - repo, err := ghrepo.FromFullName(cloneURL) - if err != nil { - return err - } - cloneURL = formatRemoteURL(cmd, repo) - } - - var repo ghrepo.Interface - var parentRepo ghrepo.Interface - - // TODO: consider caching and reusing `git.ParseSSHConfig().Translator()` - // here to handle hostname aliases in SSH remotes - if u, err := git.ParseURL(cloneURL); err == nil { - repo, _ = ghrepo.FromURL(u) - } - - if repo != nil { - parentRepo, err = api.RepoParent(apiClient, repo) - if err != nil { - return err - } - } - - cloneDir, err := runClone(cloneURL, args[1:]) - if err != nil { - return err - } - - if parentRepo != nil { - err := addUpstreamRemote(cmd, parentRepo, cloneDir) - if err != nil { - return err - } - } - - return nil -} - func addUpstreamRemote(cmd *cobra.Command, parentRepo ghrepo.Interface, cloneDir string) error { upstreamURL := formatRemoteURL(cmd, parentRepo) @@ -529,12 +425,24 @@ func repoFork(cmd *cobra.Command, args []string) error { } if cloneDesired { forkedRepoCloneURL := formatRemoteURL(cmd, forkedRepo) - cloneDir, err := runClone(forkedRepoCloneURL, []string{}) + cloneDir, err := git.RunClone(forkedRepoCloneURL, []string{}) if err != nil { return fmt.Errorf("failed to clone fork: %w", err) } - err = addUpstreamRemote(cmd, repoToFork, cloneDir) + // TODO This is overly wordy and I'd like to streamline this. + cfg, err := ctx.Config() + if err != nil { + return err + } + protocol, err := cfg.Get("", "git_protocol") + if err != nil { + return err + } + + upstreamURL := ghrepo.FormatRemoteURL(repoToFork, protocol) + + err = git.AddUpstreamRemote(upstreamURL, cloneDir) if err != nil { return err } diff --git a/command/repo_test.go b/command/repo_test.go index cec4f114f..50375dbcd 100644 --- a/command/repo_test.go +++ b/command/repo_test.go @@ -4,7 +4,6 @@ import ( "encoding/json" "io/ioutil" "os/exec" - "reflect" "regexp" "strings" "testing" @@ -437,190 +436,6 @@ func TestRepoFork_in_parent_survey_no(t *testing.T) { } } -func TestParseExtraArgs(t *testing.T) { - type Wanted struct { - args []string - dir string - } - tests := []struct { - name string - args []string - want Wanted - }{ - { - name: "args and target", - args: []string{"target_directory", "-o", "upstream", "--depth", "1"}, - want: Wanted{ - args: []string{"-o", "upstream", "--depth", "1"}, - dir: "target_directory", - }, - }, - { - name: "only args", - args: []string{"-o", "upstream", "--depth", "1"}, - want: Wanted{ - args: []string{"-o", "upstream", "--depth", "1"}, - dir: "", - }, - }, - { - name: "only target", - args: []string{"target_directory"}, - want: Wanted{ - args: []string{}, - dir: "target_directory", - }, - }, - { - name: "no args", - args: []string{}, - want: Wanted{ - args: []string{}, - dir: "", - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - args, dir := parseCloneArgs(tt.args) - got := Wanted{ - args: args, - dir: dir, - } - - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("got %#v want %#v", got, tt.want) - } - }) - } - -} - -func TestRepoClone(t *testing.T) { - tests := []struct { - name string - args string - want string - }{ - { - name: "shorthand", - args: "repo clone OWNER/REPO", - want: "git clone https://github.com/OWNER/REPO.git", - }, - { - name: "shorthand with directory", - args: "repo clone OWNER/REPO target_directory", - want: "git clone https://github.com/OWNER/REPO.git target_directory", - }, - { - name: "clone arguments", - args: "repo clone OWNER/REPO -- -o upstream --depth 1", - want: "git clone -o upstream --depth 1 https://github.com/OWNER/REPO.git", - }, - { - name: "clone arguments with directory", - args: "repo clone OWNER/REPO target_directory -- -o upstream --depth 1", - want: "git clone -o upstream --depth 1 https://github.com/OWNER/REPO.git target_directory", - }, - { - name: "HTTPS URL", - args: "repo clone https://github.com/OWNER/REPO", - want: "git clone https://github.com/OWNER/REPO", - }, - { - name: "SSH URL", - args: "repo clone git@github.com:OWNER/REPO.git", - want: "git clone git@github.com:OWNER/REPO.git", - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - http := initFakeHTTP() - http.Register( - httpmock.GraphQL(`query RepositoryFindParent\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "parent": null - } } } - `)) - - cs, restore := test.InitCmdStubber() - defer restore() - - cs.Stub("") // git clone - - output, err := RunCommand(tt.args) - if err != nil { - t.Fatalf("error running command `repo clone`: %v", err) - } - - eq(t, output.String(), "") - eq(t, output.Stderr(), "") - eq(t, cs.Count, 1) - eq(t, strings.Join(cs.Calls[0].Args, " "), tt.want) - }) - } -} - -func TestRepoClone_hasParent(t *testing.T) { - http := initFakeHTTP() - http.Register( - httpmock.GraphQL(`query RepositoryFindParent\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "parent": { - "owner": {"login": "hubot"}, - "name": "ORIG" - } - } } } - `)) - - cs, restore := test.InitCmdStubber() - defer restore() - - cs.Stub("") // git clone - cs.Stub("") // git remote add - - _, err := RunCommand("repo clone OWNER/REPO") - if err != nil { - t.Fatalf("error running command `repo clone`: %v", err) - } - - eq(t, cs.Count, 2) - eq(t, strings.Join(cs.Calls[1].Args, " "), "git -C REPO remote add -f upstream https://github.com/hubot/ORIG.git") -} - -func TestRepo_withoutUsername(t *testing.T) { - http := initFakeHTTP() - http.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(` - { "data": { "viewer": { - "login": "OWNER" - }}}`)) - http.Register( - httpmock.GraphQL(`query RepositoryFindParent\b`), - httpmock.StringResponse(` - { "data": { "repository": { - "parent": null - } } }`)) - - cs, restore := test.InitCmdStubber() - defer restore() - - cs.Stub("") // git clone - - output, err := RunCommand("repo clone REPO") - if err != nil { - t.Fatalf("error running command `repo clone`: %v", err) - } - - eq(t, output.String(), "") - eq(t, output.Stderr(), "") - eq(t, cs.Count, 1) - eq(t, strings.Join(cs.Calls[0].Args, " "), "git clone https://github.com/OWNER/REPO.git") -} - func TestRepoCreate(t *testing.T) { ctx := context.NewBlank() ctx.SetBranch("master") diff --git a/command/root.go b/command/root.go index ab3b60140..fdd1c8d11 100644 --- a/command/root.go +++ b/command/root.go @@ -21,6 +21,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" + repoCloneCmd "github.com/cli/cli/pkg/cmd/repo/clone" repoViewCmd "github.com/cli/cli/pkg/cmd/repo/view" "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/iostreams" @@ -95,6 +96,15 @@ func init() { ctx := context.New() return ctx.BaseRepo() }, + Config: func() (config.Config, error) { + cfg, err := config.ParseDefaultConfig() + if errors.Is(err, os.ErrNotExist) { + cfg = config.NewBlankConfig() + } else if err != nil { + return nil, err + } + return cfg, nil + }, } RootCmd.AddCommand(apiCmd.NewCmdApi(cmdFactory, nil)) @@ -137,6 +147,7 @@ func init() { RootCmd.AddCommand(repoCmd) repoCmd.AddCommand(repoViewCmd.NewCmdView(&repoResolvingCmdFactory, nil)) + repoCmd.AddCommand(repoCloneCmd.NewCmdClone(cmdFactory, nil)) } // RootCmd is the entry point of command-line execution @@ -355,6 +366,7 @@ func determineBaseRepo(apiClient *api.Client, cmd *cobra.Command, ctx context.Co return baseRepo, nil } +// TODO there is a parallel implementation for isolated commands func formatRemoteURL(cmd *cobra.Command, repo ghrepo.Interface) string { ctx := contextForCommand(cmd) diff --git a/git/git.go b/git/git.go index ff40adde3..645a64fd1 100644 --- a/git/git.go +++ b/git/git.go @@ -7,6 +7,7 @@ import ( "net/url" "os" "os/exec" + "path" "regexp" "strings" @@ -224,6 +225,48 @@ func CheckoutBranch(branch string) error { return err } +func parseCloneArgs(extraArgs []string) (args []string, target string) { + args = extraArgs + + if len(args) > 0 { + if !strings.HasPrefix(args[0], "-") { + target, args = args[0], args[1:] + } + } + return +} + +func RunClone(cloneURL string, args []string) (target string, err error) { + cloneArgs, target := parseCloneArgs(args) + + cloneArgs = append(cloneArgs, cloneURL) + + // If the args contain an explicit target, pass it to clone + // otherwise, parse the URL to determine where git cloned it to so we can return it + if target != "" { + cloneArgs = append(cloneArgs, target) + } else { + target = path.Base(strings.TrimSuffix(cloneURL, ".git")) + } + + cloneArgs = append([]string{"clone"}, cloneArgs...) + + cloneCmd := GitCommand(cloneArgs...) + cloneCmd.Stdin = os.Stdin + cloneCmd.Stdout = os.Stdout + cloneCmd.Stderr = os.Stderr + + err = run.PrepareCmd(cloneCmd).Run() + return +} + +func AddUpstreamRemote(upstreamURL, cloneDir string) error { + cloneCmd := GitCommand("-C", cloneDir, "remote", "add", "-f", "upstream", upstreamURL) + cloneCmd.Stdout = os.Stdout + cloneCmd.Stderr = os.Stderr + return run.PrepareCmd(cloneCmd).Run() +} + func isFilesystemPath(p string) bool { return p == "." || strings.HasPrefix(p, "./") || strings.HasPrefix(p, "/") } diff --git a/git/git_test.go b/git/git_test.go index c03e7153d..537054012 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -2,6 +2,7 @@ package git import ( "os/exec" + "reflect" "testing" "github.com/cli/cli/internal/run" @@ -94,3 +95,62 @@ func Test_CurrentBranch_unexpected_error(t *testing.T) { t.Errorf("expected 1 git call, saw %d", len(cs.Calls)) } } + +func TestParseExtraCloneArgs(t *testing.T) { + type Wanted struct { + args []string + dir string + } + tests := []struct { + name string + args []string + want Wanted + }{ + { + name: "args and target", + args: []string{"target_directory", "-o", "upstream", "--depth", "1"}, + want: Wanted{ + args: []string{"-o", "upstream", "--depth", "1"}, + dir: "target_directory", + }, + }, + { + name: "only args", + args: []string{"-o", "upstream", "--depth", "1"}, + want: Wanted{ + args: []string{"-o", "upstream", "--depth", "1"}, + dir: "", + }, + }, + { + name: "only target", + args: []string{"target_directory"}, + want: Wanted{ + args: []string{}, + dir: "target_directory", + }, + }, + { + name: "no args", + args: []string{}, + want: Wanted{ + args: []string{}, + dir: "", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + args, dir := parseCloneArgs(tt.args) + got := Wanted{ + args: args, + dir: dir, + } + + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("got %#v want %#v", got, tt.want) + } + }) + } + +} diff --git a/internal/ghrepo/repo.go b/internal/ghrepo/repo.go index 9b0a7ac30..b80b09219 100644 --- a/internal/ghrepo/repo.go +++ b/internal/ghrepo/repo.go @@ -85,6 +85,15 @@ func GenerateRepoURL(repo Interface, p string, args ...interface{}) string { return baseURL } +// TODO there is a parallel implementation for non-isolated commands +func FormatRemoteURL(repo Interface, protocol string) string { + if protocol == "ssh" { + return fmt.Sprintf("git@%s:%s/%s.git", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) + } + + return fmt.Sprintf("https://%s/%s/%s.git", repo.RepoHost(), repo.RepoOwner(), repo.RepoName()) +} + type ghRepo struct { owner string name string diff --git a/pkg/cmd/repo/clone/clone.go b/pkg/cmd/repo/clone/clone.go new file mode 100644 index 000000000..30209b0cc --- /dev/null +++ b/pkg/cmd/repo/clone/clone.go @@ -0,0 +1,126 @@ +package clone + +import ( + "net/http" + "strings" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/api" + "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/iostreams" + "github.com/spf13/cobra" +) + +type CloneOptions struct { + HttpClient func() (*http.Client, error) + Config func() (config.Config, error) + IO *iostreams.IOStreams + + GitArgs []string + Directory string + Repository string +} + +func NewCmdClone(f *cmdutil.Factory, runF func(*CloneOptions) error) *cobra.Command { + opts := &CloneOptions{ + IO: f.IOStreams, + HttpClient: f.HttpClient, + Config: f.Config, + } + + cmd := &cobra.Command{ + Use: "clone []", + Args: cobra.MinimumNArgs(1), + Short: "Clone a repository locally", + Long: heredoc.Doc( + `Clone a GitHub repository locally. + + If the "OWNER/" portion of the "OWNER/REPO" repository argument is omitted, it + defaults to the name of the authenticating user. + + To pass 'git clone' flags, separate them with '--'. + `), + RunE: func(cmd *cobra.Command, args []string) error { + opts.Repository = args[0] + opts.GitArgs = args[1:] + + if runF != nil { + return runF(opts) + } + + return cloneRun(opts) + }, + } + + return cmd +} + +func cloneRun(opts *CloneOptions) error { + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + + // TODO This is overly wordy and I'd like to streamline this. + cfg, err := opts.Config() + if err != nil { + return err + } + protocol, err := cfg.Get("", "git_protocol") + if err != nil { + return err + } + + apiClient := api.NewClientFromHTTP(httpClient) + cloneURL := opts.Repository + if !strings.Contains(cloneURL, ":") { + if !strings.Contains(cloneURL, "/") { + currentUser, err := api.CurrentLoginName(apiClient) + if err != nil { + return err + } + cloneURL = currentUser + "/" + cloneURL + } + repo, err := ghrepo.FromFullName(cloneURL) + if err != nil { + return err + } + + cloneURL = ghrepo.FormatRemoteURL(repo, protocol) + } + + var repo ghrepo.Interface + var parentRepo ghrepo.Interface + + // TODO: consider caching and reusing `git.ParseSSHConfig().Translator()` + // here to handle hostname aliases in SSH remotes + if u, err := git.ParseURL(cloneURL); err == nil { + repo, _ = ghrepo.FromURL(u) + } + + if repo != nil { + parentRepo, err = api.RepoParent(apiClient, repo) + if err != nil { + return err + } + } + + cloneDir, err := git.RunClone(cloneURL, opts.GitArgs) + if err != nil { + return err + } + + if parentRepo != nil { + upstreamURL := ghrepo.FormatRemoteURL(parentRepo, protocol) + + err := git.AddUpstreamRemote(upstreamURL, cloneDir) + if err != nil { + return err + } + } + + return nil +} diff --git a/pkg/cmd/repo/clone/clone_test.go b/pkg/cmd/repo/clone/clone_test.go new file mode 100644 index 000000000..0ceb17050 --- /dev/null +++ b/pkg/cmd/repo/clone/clone_test.go @@ -0,0 +1,195 @@ +package clone + +import ( + "bytes" + "net/http" + "strings" + "testing" + + "github.com/cli/cli/internal/config" + "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" +) + +// TODO copypasta from command package +type cmdOut struct { + outBuf, errBuf *bytes.Buffer +} + +func (c cmdOut) String() string { + return c.outBuf.String() +} + +func (c cmdOut) Stderr() string { + return c.errBuf.String() +} + +func runCloneCommand(httpClient *http.Client, cli string) (*cmdOut, error) { + io, stdin, stdout, stderr := iostreams.Test() + fac := &cmdutil.Factory{ + IOStreams: io, + HttpClient: func() (*http.Client, error) { + return httpClient, nil + }, + Config: func() (config.Config, error) { + return config.NewBlankConfig(), nil + }, + } + + cmd := NewCmdClone(fac, nil) + + argv, err := shlex.Split(cli) + cmd.SetArgs(argv) + + cmd.SetIn(stdin) + cmd.SetOut(stdout) + cmd.SetErr(stderr) + + if err != nil { + panic(err) + } + + _, err = cmd.ExecuteC() + + if err != nil { + return nil, err + } + + return &cmdOut{stdout, stderr}, nil +} + +func Test_RepoClone(t *testing.T) { + tests := []struct { + name string + args string + want string + }{ + { + name: "shorthand", + args: "OWNER/REPO", + want: "git clone https://github.com/OWNER/REPO.git", + }, + { + name: "shorthand with directory", + args: "OWNER/REPO target_directory", + want: "git clone https://github.com/OWNER/REPO.git target_directory", + }, + { + name: "clone arguments", + args: "OWNER/REPO -- -o upstream --depth 1", + want: "git clone -o upstream --depth 1 https://github.com/OWNER/REPO.git", + }, + { + name: "clone arguments with directory", + args: "OWNER/REPO target_directory -- -o upstream --depth 1", + want: "git clone -o upstream --depth 1 https://github.com/OWNER/REPO.git target_directory", + }, + { + name: "HTTPS URL", + args: "https://github.com/OWNER/REPO", + want: "git clone https://github.com/OWNER/REPO", + }, + { + name: "SSH URL", + args: "git@github.com:OWNER/REPO.git", + want: "git clone git@github.com:OWNER/REPO.git", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`query RepositoryFindParent\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "parent": null + } } } + `)) + + httpClient := &http.Client{Transport: reg} + + cs, restore := test.InitCmdStubber() + defer restore() + + cs.Stub("") // git clone + + output, err := runCloneCommand(httpClient, tt.args) + if err != nil { + t.Fatalf("error running command `repo clone`: %v", err) + } + + assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) + assert.Equal(t, 1, cs.Count) + assert.Equal(t, tt.want, strings.Join(cs.Calls[0].Args, " ")) + reg.Verify(t) + }) + } +} + +func Test_RepoClone_hasParent(t *testing.T) { + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`query RepositoryFindParent\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "parent": { + "owner": {"login": "hubot"}, + "name": "ORIG" + } + } } } + `)) + + httpClient := &http.Client{Transport: reg} + + cs, restore := test.InitCmdStubber() + defer restore() + + cs.Stub("") // git clone + cs.Stub("") // git remote add + + _, err := runCloneCommand(httpClient, "OWNER/REPO") + if err != nil { + t.Fatalf("error running command `repo clone`: %v", err) + } + + assert.Equal(t, 2, cs.Count) + assert.Equal(t, "git -C REPO remote add -f upstream https://github.com/hubot/ORIG.git", strings.Join(cs.Calls[1].Args, " ")) +} + +func Test_RepoClone_withoutUsername(t *testing.T) { + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(` + { "data": { "viewer": { + "login": "OWNER" + }}}`)) + reg.Register( + httpmock.GraphQL(`query RepositoryFindParent\b`), + httpmock.StringResponse(` + { "data": { "repository": { + "parent": null + } } }`)) + + httpClient := &http.Client{Transport: reg} + + cs, restore := test.InitCmdStubber() + defer restore() + + cs.Stub("") // git clone + + output, err := runCloneCommand(httpClient, "REPO") + if err != nil { + t.Fatalf("error running command `repo clone`: %v", err) + } + + assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) + assert.Equal(t, 1, cs.Count) + assert.Equal(t, "git clone https://github.com/OWNER/REPO.git", strings.Join(cs.Calls[0].Args, " ")) +} diff --git a/pkg/cmdutil/factory.go b/pkg/cmdutil/factory.go index ad7162415..0c66a2b10 100644 --- a/pkg/cmdutil/factory.go +++ b/pkg/cmdutil/factory.go @@ -3,6 +3,7 @@ package cmdutil import ( "net/http" + "github.com/cli/cli/internal/config" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/iostreams" ) @@ -11,4 +12,5 @@ type Factory struct { IOStreams *iostreams.IOStreams HttpClient func() (*http.Client, error) BaseRepo func() (ghrepo.Interface, error) + Config func() (config.Config, error) } From cc862d03e745995171e874481033eb664f3ac533 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 23 Jul 2020 16:43:16 -0500 Subject: [PATCH 12/12] remove dead code --- command/repo.go | 9 --------- 1 file changed, 9 deletions(-) diff --git a/command/repo.go b/command/repo.go index 92ea292a5..cca74d8fb 100644 --- a/command/repo.go +++ b/command/repo.go @@ -105,15 +105,6 @@ var repoCreditsCmd = &cobra.Command{ Hidden: true, } -func addUpstreamRemote(cmd *cobra.Command, parentRepo ghrepo.Interface, cloneDir string) error { - upstreamURL := formatRemoteURL(cmd, parentRepo) - - cloneCmd := git.GitCommand("-C", cloneDir, "remote", "add", "-f", "upstream", upstreamURL) - cloneCmd.Stdout = os.Stdout - cloneCmd.Stderr = os.Stderr - return run.PrepareCmd(cloneCmd).Run() -} - func repoCreate(cmd *cobra.Command, args []string) error { projectDir, projectDirErr := git.ToplevelDir()