From eb204c0dee5415ce2904e068a56aa55f64772200 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Fri, 17 Jul 2020 16:10:49 -0500 Subject: [PATCH] scriptability improvements for repo commands --- command/repo.go | 87 +++++++++++++++----- command/repo_test.go | 184 +++++++++++++++++++++++++++++++++++-------- 2 files changed, 218 insertions(+), 53 deletions(-) diff --git a/command/repo.go b/command/repo.go index e68a86bdd..d705a4a5f 100644 --- a/command/repo.go +++ b/command/repo.go @@ -434,21 +434,36 @@ func repoFork(cmd *cobra.Command, args []string) error { } } + if !connectedToTerminal(cmd) { + if (inParent && remotePref == "prompt") || (!inParent && clonePref == "prompt") { + return errors.New("--remote or --clone must be explicitly set when not attached to tty") + } + } + greenCheck := utils.Green("✓") - out := colorableOut(cmd) - s := utils.Spinner(out) - loading := utils.Gray("Forking ") + utils.Bold(utils.Gray(ghrepo.FullName(repoToFork))) + utils.Gray("...") - s.Suffix = " " + loading - s.FinalMSG = utils.Gray(fmt.Sprintf("- %s\n", loading)) - utils.StartSpinner(s) + stderr := colorableErr(cmd) + s := utils.Spinner(stderr) + stopSpinner := func() {} + + if connectedToTerminal(cmd) { + loading := utils.Gray("Forking ") + utils.Bold(utils.Gray(ghrepo.FullName(repoToFork))) + utils.Gray("...") + s.Suffix = " " + loading + s.FinalMSG = utils.Gray(fmt.Sprintf("- %s\n", loading)) + utils.StartSpinner(s) + stopSpinner = func() { + utils.StopSpinner(s) + + } + } forkedRepo, err := api.ForkRepo(apiClient, repoToFork) if err != nil { - utils.StopSpinner(s) + stopSpinner() return fmt.Errorf("failed to fork: %w", err) } - s.Stop() + stopSpinner() + // This is weird. There is not an efficient way to determine via the GitHub API whether or not a // given user has forked a given repo. We noticed, also, that the create fork API endpoint just // returns the fork repo data even if it already exists -- with no change in status code or @@ -456,12 +471,19 @@ func repoFork(cmd *cobra.Command, args []string) error { // we assume the fork already existed and report an error. createdAgo := Since(forkedRepo.CreatedAt) if createdAgo > time.Minute { - fmt.Fprintf(out, "%s %s %s\n", - utils.Yellow("!"), - utils.Bold(ghrepo.FullName(forkedRepo)), - "already exists") + if connectedToTerminal(cmd) { + fmt.Fprintf(stderr, "%s %s %s\n", + utils.Yellow("!"), + utils.Bold(ghrepo.FullName(forkedRepo)), + "already exists") + } else { + fmt.Fprintf(stderr, "%s already exists", ghrepo.FullName(forkedRepo)) + return nil + } } else { - fmt.Fprintf(out, "%s Created fork %s\n", greenCheck, utils.Bold(ghrepo.FullName(forkedRepo))) + if connectedToTerminal(cmd) { + fmt.Fprintf(stderr, "%s Created fork %s\n", greenCheck, utils.Bold(ghrepo.FullName(forkedRepo))) + } } if (inParent && remotePref == "false") || (!inParent && clonePref == "false") { @@ -474,7 +496,9 @@ func repoFork(cmd *cobra.Command, args []string) error { return err } if remote, err := remotes.FindByRepo(forkedRepo.RepoOwner(), forkedRepo.RepoName()); err == nil { - fmt.Fprintf(out, "%s Using existing remote %s\n", greenCheck, utils.Bold(remote.Name)) + if connectedToTerminal(cmd) { + fmt.Fprintf(stderr, "%s Using existing remote %s\n", greenCheck, utils.Bold(remote.Name)) + } return nil } @@ -499,7 +523,9 @@ func repoFork(cmd *cobra.Command, args []string) error { if err != nil { return err } - fmt.Fprintf(out, "%s Renamed %s remote to %s\n", greenCheck, utils.Bold(remoteName), utils.Bold(renameTarget)) + if connectedToTerminal(cmd) { + fmt.Fprintf(stderr, "%s Renamed %s remote to %s\n", greenCheck, utils.Bold(remoteName), utils.Bold(renameTarget)) + } } forkedRepoCloneURL := formatRemoteURL(cmd, forkedRepo) @@ -509,7 +535,9 @@ func repoFork(cmd *cobra.Command, args []string) error { return fmt.Errorf("failed to add remote: %w", err) } - fmt.Fprintf(out, "%s Added remote %s\n", greenCheck, utils.Bold(remoteName)) + if connectedToTerminal(cmd) { + fmt.Fprintf(stderr, "%s Added remote %s\n", greenCheck, utils.Bold(remoteName)) + } } } else { cloneDesired := clonePref == "true" @@ -531,7 +559,9 @@ func repoFork(cmd *cobra.Command, args []string) error { return err } - fmt.Fprintf(out, "%s Cloned fork\n", greenCheck) + if connectedToTerminal(cmd) { + fmt.Fprintf(stderr, "%s Cloned fork\n", greenCheck) + } } } @@ -591,14 +621,31 @@ func repoView(cmd *cobra.Command, args []string) error { return err } - fullName := ghrepo.FullName(toView) - openURL := generateRepoURL(toView, "") if web { - fmt.Fprintf(cmd.ErrOrStderr(), "Opening %s in your browser.\n", displayURL(openURL)) + 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}} diff --git a/command/repo_test.go b/command/repo_test.go index 8d103645c..8c8e2cf31 100644 --- a/command/repo_test.go +++ b/command/repo_test.go @@ -17,6 +17,7 @@ import ( "github.com/cli/cli/pkg/httpmock" "github.com/cli/cli/test" "github.com/cli/cli/utils" + "github.com/stretchr/testify/assert" ) func stubSpinner() { @@ -27,6 +28,72 @@ func stubSpinner() { } } +func TestRepoFork_nontty_insufficient_flags(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + defer stubSince(2 * time.Second)() + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + defer stubTerminal(false)() + + _, err := RunCommand("repo fork") + if err == nil { + t.Fatal("expected error") + } + + assert.Equal(t, "--remote or --clone must be explicitly set when not attached to tty", err.Error()) +} + +func TestRepoFork_in_parent_nontty(t *testing.T) { + initBlankContext("", "OWNER/REPO", "master") + defer stubSince(2 * time.Second)() + http := initFakeHTTP() + http.StubRepoResponse("OWNER", "REPO") + defer http.StubWithFixture(200, "forkResult.json")() + defer stubTerminal(false)() + + cs, restore := test.InitCmdStubber() + defer restore() + + cs.Stub("") // git remote rename + cs.Stub("") // git remote add + + output, err := RunCommand("repo fork --remote") + if err != nil { + t.Fatalf("error running command `repo fork`: %v", err) + } + + eq(t, strings.Join(cs.Calls[0].Args, " "), "git remote rename origin upstream") + eq(t, strings.Join(cs.Calls[1].Args, " "), "git remote add -f origin https://github.com/someone/REPO.git") + + assert.Equal(t, "", output.String()) + assert.Equal(t, "", output.Stderr()) +} + +func TestRepoFork_outside_parent_nontty(t *testing.T) { + defer stubSince(2 * time.Second)() + http := initFakeHTTP() + defer http.StubWithFixture(200, "forkResult.json")() + defer stubTerminal(false)() + + cs, restore := test.InitCmdStubber() + defer restore() + + cs.Stub("") // git clone + cs.Stub("") // git remote add + + output, err := RunCommand("repo fork --clone OWNER/REPO") + if err != nil { + t.Errorf("error running command `repo fork`: %v", err) + } + + eq(t, output.String(), "") + + eq(t, strings.Join(cs.Calls[0].Args, " "), "git clone https://github.com/someone/REPO.git") + eq(t, strings.Join(cs.Calls[1].Args, " "), "git -C REPO remote add -f upstream https://github.com/OWNER/REPO.git") + + assert.Equal(t, output.Stderr(), "") +} + func TestRepoFork_already_forked(t *testing.T) { stubSpinner() initContext = func() context.Context { @@ -41,14 +108,15 @@ func TestRepoFork_already_forked(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") defer http.StubWithFixture(200, "forkResult.json")() + defer stubTerminal(true)() output, err := RunCommand("repo fork --remote=false") if err != nil { t.Errorf("got unexpected error: %v", err) } - r := regexp.MustCompile(`someone/REPO already exists`) - if !r.MatchString(output.String()) { - t.Errorf("output did not match regexp /%s/\n> output\n%s\n", r, output) + r := regexp.MustCompile(`someone/REPO.*already exists`) + if !r.MatchString(output.Stderr()) { + t.Errorf("output did not match regexp /%s/\n> output\n%s\n", r, output.Stderr()) return } } @@ -68,13 +136,15 @@ func TestRepoFork_reuseRemote(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") defer http.StubWithFixture(200, "forkResult.json")() + defer stubTerminal(true)() output, err := RunCommand("repo fork") if err != nil { t.Errorf("got unexpected error: %v", err) } - if !strings.Contains(output.String(), "Using existing remote origin") { - t.Errorf("output did not match: %q", output) + r := regexp.MustCompile(`Using existing remote.*origin`) + if !r.MatchString(output.Stderr()) { + t.Errorf("output did not match: %q", output.Stderr()) return } } @@ -96,16 +166,17 @@ func TestRepoFork_in_parent(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") defer http.StubWithFixture(200, "forkResult.json")() + defer stubTerminal(true)() output, err := RunCommand("repo fork --remote=false") if err != nil { t.Errorf("error running command `repo fork`: %v", err) } - eq(t, output.Stderr(), "") + eq(t, output.String(), "") - r := regexp.MustCompile(`Created fork someone/REPO`) - if !r.MatchString(output.String()) { + r := regexp.MustCompile(`Created fork.*someone/REPO`) + if !r.MatchString(output.Stderr()) { t.Errorf("output did not match regexp /%s/\n> output\n%s\n", r, output) return } @@ -131,16 +202,17 @@ func TestRepoFork_outside(t *testing.T) { defer stubSince(2 * time.Second)() http := initFakeHTTP() defer http.StubWithFixture(200, "forkResult.json")() + defer stubTerminal(true)() output, err := RunCommand(tt.args) if err != nil { t.Errorf("error running command `repo fork`: %v", err) } - eq(t, output.Stderr(), "") + eq(t, output.String(), "") - r := regexp.MustCompile(`Created fork someone/REPO`) - if !r.MatchString(output.String()) { + r := regexp.MustCompile(`Created fork.*someone/REPO`) + if !r.MatchString(output.Stderr()) { t.Errorf("output did not match regexp /%s/\n> output\n%s\n", r, output) return } @@ -155,6 +227,7 @@ func TestRepoFork_in_parent_yes(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") defer http.StubWithFixture(200, "forkResult.json")() + defer stubTerminal(true)() var seenCmds []*exec.Cmd defer run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { @@ -176,11 +249,11 @@ func TestRepoFork_in_parent_yes(t *testing.T) { eq(t, strings.Join(cmd.Args, " "), expectedCmds[x]) } - eq(t, output.Stderr(), "") + eq(t, output.String(), "") - test.ExpectLines(t, output.String(), - "Created fork someone/REPO", - "Added remote origin") + test.ExpectLines(t, output.Stderr(), + "Created fork.*someone/REPO", + "Added remote.*origin") } func TestRepoFork_outside_yes(t *testing.T) { @@ -188,6 +261,7 @@ func TestRepoFork_outside_yes(t *testing.T) { defer stubSince(2 * time.Second)() http := initFakeHTTP() defer http.StubWithFixture(200, "forkResult.json")() + defer stubTerminal(true)() cs, restore := test.InitCmdStubber() defer restore() @@ -200,13 +274,13 @@ func TestRepoFork_outside_yes(t *testing.T) { t.Errorf("error running command `repo fork`: %v", err) } - eq(t, output.Stderr(), "") + eq(t, output.String(), "") eq(t, strings.Join(cs.Calls[0].Args, " "), "git clone https://github.com/someone/REPO.git") eq(t, strings.Join(cs.Calls[1].Args, " "), "git -C REPO remote add -f upstream https://github.com/OWNER/REPO.git") - test.ExpectLines(t, output.String(), - "Created fork someone/REPO", + test.ExpectLines(t, output.Stderr(), + "Created fork.*someone/REPO", "Cloned fork") } @@ -215,6 +289,7 @@ func TestRepoFork_outside_survey_yes(t *testing.T) { defer stubSince(2 * time.Second)() http := initFakeHTTP() defer http.StubWithFixture(200, "forkResult.json")() + defer stubTerminal(true)() cs, restore := test.InitCmdStubber() defer restore() @@ -234,13 +309,13 @@ func TestRepoFork_outside_survey_yes(t *testing.T) { t.Errorf("error running command `repo fork`: %v", err) } - eq(t, output.Stderr(), "") + eq(t, output.String(), "") eq(t, strings.Join(cs.Calls[0].Args, " "), "git clone https://github.com/someone/REPO.git") eq(t, strings.Join(cs.Calls[1].Args, " "), "git -C REPO remote add -f upstream https://github.com/OWNER/REPO.git") - test.ExpectLines(t, output.String(), - "Created fork someone/REPO", + test.ExpectLines(t, output.Stderr(), + "Created fork.*someone/REPO", "Cloned fork") } @@ -249,6 +324,7 @@ func TestRepoFork_outside_survey_no(t *testing.T) { defer stubSince(2 * time.Second)() http := initFakeHTTP() defer http.StubWithFixture(200, "forkResult.json")() + defer stubTerminal(true)() cmdRun := false defer run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { @@ -268,12 +344,12 @@ func TestRepoFork_outside_survey_no(t *testing.T) { t.Errorf("error running command `repo fork`: %v", err) } - eq(t, output.Stderr(), "") + eq(t, output.String(), "") eq(t, cmdRun, false) - r := regexp.MustCompile(`Created fork someone/REPO`) - if !r.MatchString(output.String()) { + r := regexp.MustCompile(`Created fork.*someone/REPO`) + if !r.MatchString(output.Stderr()) { t.Errorf("output did not match regexp /%s/\n> output\n%s\n", r, output) return } @@ -286,6 +362,7 @@ func TestRepoFork_in_parent_survey_yes(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") defer http.StubWithFixture(200, "forkResult.json")() + defer stubTerminal(true)() var seenCmds []*exec.Cmd defer run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { @@ -314,12 +391,12 @@ func TestRepoFork_in_parent_survey_yes(t *testing.T) { eq(t, strings.Join(cmd.Args, " "), expectedCmds[x]) } - eq(t, output.Stderr(), "") + eq(t, output.String(), "") - test.ExpectLines(t, output.String(), - "Created fork someone/REPO", - "Renamed origin remote to upstream", - "Added remote origin") + test.ExpectLines(t, output.Stderr(), + "Created fork.*someone/REPO", + "Renamed.*origin.*remote to.*upstream", + "Added remote.*origin") } func TestRepoFork_in_parent_survey_no(t *testing.T) { @@ -329,6 +406,7 @@ func TestRepoFork_in_parent_survey_no(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") defer http.StubWithFixture(200, "forkResult.json")() + defer stubTerminal(true)() cmdRun := false defer run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { @@ -348,12 +426,12 @@ func TestRepoFork_in_parent_survey_no(t *testing.T) { t.Errorf("error running command `repo fork`: %v", err) } - eq(t, output.Stderr(), "") + eq(t, output.String(), "") eq(t, cmdRun, false) - r := regexp.MustCompile(`Created fork someone/REPO`) - if !r.MatchString(output.String()) { + r := regexp.MustCompile(`Created fork.*someone/REPO`) + if !r.MatchString(output.Stderr()) { t.Errorf("output did not match regexp /%s/\n> output\n%s\n", r, output) return } @@ -766,6 +844,8 @@ func TestRepoView_web(t *testing.T) { }) defer restoreCmd() + defer stubTerminal(true)() + output, err := RunCommand("repo view -w") if err != nil { t.Errorf("error running command `repo view`: %v", err) @@ -799,6 +879,7 @@ func TestRepoView_web_ownerRepo(t *testing.T) { return &test.OutputStub{} }) defer restoreCmd() + defer stubTerminal(true)() output, err := RunCommand("repo view -w cli/cli") if err != nil { @@ -833,6 +914,7 @@ func TestRepoView_web_fullURL(t *testing.T) { }) 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) @@ -865,6 +947,8 @@ func TestRepoView(t *testing.T) { { "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) @@ -875,7 +959,6 @@ func TestRepoView(t *testing.T) { "social distancing", "truly cool readme", "View this repository on GitHub: https://github.com/OWNER/REPO") - } func TestRepoView_nonmarkdown_readme(t *testing.T) { @@ -895,6 +978,8 @@ func TestRepoView_nonmarkdown_readme(t *testing.T) { { "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) @@ -916,6 +1001,8 @@ func TestRepoView_blanks(t *testing.T) { 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) @@ -927,3 +1014,34 @@ func TestRepoView_blanks(t *testing.T) { "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", + ) +}