From 121173c3308b264d0565251976f07917de8f75fa Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 23 Mar 2020 15:21:58 -0500 Subject: [PATCH 01/12] better error reporting + catch for empty repo --- command/pr.go | 2 +- context/context.go | 2 +- git/git.go | 9 +++++++++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/command/pr.go b/command/pr.go index bd7743f46..924059882 100644 --- a/command/pr.go +++ b/command/pr.go @@ -84,7 +84,7 @@ func prStatus(cmd *cobra.Command, args []string) error { repoOverride, _ := cmd.Flags().GetString("repo") currentPRNumber, currentPRHeadRef, err := prSelectorForCurrentBranch(ctx, baseRepo) if err != nil && repoOverride == "" && err.Error() != "git: not on any branch" { - return err + return fmt.Errorf("could not query for pull request for current branch: %w", err) } prPayload, err := api.PullRequests(apiClient, baseRepo, currentPRNumber, currentPRHeadRef, currentUser) diff --git a/context/context.go b/context/context.go index 6c3bd5aee..23ce74d85 100644 --- a/context/context.go +++ b/context/context.go @@ -184,7 +184,7 @@ func (c *fsContext) Branch() (string, error) { currentBranch, err := git.CurrentBranch() if err != nil { - return "", err + return "", fmt.Errorf("could not determine current branch: %w", err) } c.branch = currentBranch diff --git a/git/git.go b/git/git.go index 37253462a..cdf1e869f 100644 --- a/git/git.go +++ b/git/git.go @@ -21,6 +21,15 @@ func VerifyRef(ref string) bool { // CurrentBranch reads the checked-out branch for the git repository func CurrentBranch() (string, error) { + err := utils.PrepareCmd(GitCommand("log")).Run() + if err != nil { + // this is a hack. + errRe := regexp.MustCompile("your current branch '([^']+)' does not have any commits yet") + matches := errRe.FindAllStringSubmatch(err.Error(), -1) + if len(matches) > 0 && matches[0][1] != "" { + return matches[0][1], nil + } + } // we avoid using `git branch --show-current` for compatibility with git < 2.22 branchCmd := exec.Command("git", "rev-parse", "--abbrev-ref", "HEAD") output, err := utils.PrepareCmd(branchCmd).Output() From 633ccddb7ad41fb51d381164ad36b45abb820ad2 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 23 Mar 2020 15:58:18 -0500 Subject: [PATCH 02/12] remove unused test helper --- test/helpers.go | 62 ------------------------------------------------- 1 file changed, 62 deletions(-) diff --git a/test/helpers.go b/test/helpers.go index 6bcfcadca..add7b90c3 100644 --- a/test/helpers.go +++ b/test/helpers.go @@ -1,13 +1,5 @@ package test -import ( - "fmt" - "io/ioutil" - "os" - "os/exec" - "path/filepath" -) - // OutputStub implements a simple utils.Runnable type OutputStub struct { Out []byte @@ -20,57 +12,3 @@ func (s OutputStub) Output() ([]byte, error) { func (s OutputStub) Run() error { return nil } - -type TempGitRepo struct { - Remote string - TearDown func() -} - -func UseTempGitRepo() *TempGitRepo { - pwd, _ := os.Getwd() - oldEnv := make(map[string]string) - overrideEnv := func(name, value string) { - oldEnv[name] = os.Getenv(name) - os.Setenv(name, value) - } - - remotePath := filepath.Join(pwd, "..", "test", "fixtures", "test.git") - home, err := ioutil.TempDir("", "test-repo") - if err != nil { - panic(err) - } - - overrideEnv("HOME", home) - overrideEnv("XDG_CONFIG_HOME", "") - overrideEnv("XDG_CONFIG_DIRS", "") - - targetPath := filepath.Join(home, "test.git") - cmd := exec.Command("git", "clone", remotePath, targetPath) - if output, err := cmd.Output(); err != nil { - panic(fmt.Errorf("error running %s\n%s\n%s", cmd, err, output)) - } - - if err = os.Chdir(targetPath); err != nil { - panic(err) - } - - // Our libs expect the origin to be a github url - cmd = exec.Command("git", "remote", "set-url", "origin", "https://github.com/github/FAKE-GITHUB-REPO-NAME") - if output, err := cmd.Output(); err != nil { - panic(fmt.Errorf("error running %s\n%s\n%s", cmd, err, output)) - } - - tearDown := func() { - if err := os.Chdir(pwd); err != nil { - panic(err) - } - for name, value := range oldEnv { - os.Setenv(name, value) - } - if err = os.RemoveAll(home); err != nil { - panic(err) - } - } - - return &TempGitRepo{Remote: remotePath, TearDown: tearDown} -} From 5187ad4431f953b5690701536c832dd85f9b85e3 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 23 Mar 2020 16:32:29 -0500 Subject: [PATCH 03/12] move preparecmd and Runnable to its own package --- command/issue_test.go | 14 +++++----- command/pr_checkout.go | 4 +-- command/pr_checkout_test.go | 20 +++++++------- command/pr_test.go | 22 ++++++++-------- command/repo.go | 11 ++++---- command/repo_test.go | 29 ++++++++++----------- command/testing.go | 8 +++--- git/git.go | 24 ++++++++--------- git/git_test.go | 6 ++--- git/remote.go | 6 ++--- utils/prepare_cmd.go => internal/run/run.go | 2 +- utils/utils.go | 3 ++- 12 files changed, 75 insertions(+), 74 deletions(-) rename utils/prepare_cmd.go => internal/run/run.go (99%) diff --git a/command/issue_test.go b/command/issue_test.go index 0d839d5a2..17a698a22 100644 --- a/command/issue_test.go +++ b/command/issue_test.go @@ -10,8 +10,8 @@ import ( "strings" "testing" + "github.com/cli/cli/internal/run" "github.com/cli/cli/test" - "github.com/cli/cli/utils" ) func TestIssueStatus(t *testing.T) { @@ -229,7 +229,7 @@ func TestIssueView(t *testing.T) { `)) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) @@ -263,7 +263,7 @@ func TestIssueView_numberArgWithHash(t *testing.T) { `)) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) @@ -386,7 +386,7 @@ func TestIssueView_notFound(t *testing.T) { `)) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) @@ -433,7 +433,7 @@ func TestIssueView_urlArg(t *testing.T) { `)) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) @@ -518,7 +518,7 @@ func TestIssueCreate_web(t *testing.T) { http.StubRepoResponse("OWNER", "REPO") var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) @@ -544,7 +544,7 @@ func TestIssueCreate_webTitleBody(t *testing.T) { http.StubRepoResponse("OWNER", "REPO") var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) diff --git a/command/pr_checkout.go b/command/pr_checkout.go index 08a37033d..bf7cedbb7 100644 --- a/command/pr_checkout.go +++ b/command/pr_checkout.go @@ -7,7 +7,7 @@ import ( "os/exec" "github.com/cli/cli/git" - "github.com/cli/cli/utils" + "github.com/cli/cli/internal/run" "github.com/spf13/cobra" ) @@ -91,7 +91,7 @@ func prCheckout(cmd *cobra.Command, args []string) error { cmd := exec.Command(args[0], args[1:]...) cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr - if err := utils.PrepareCmd(cmd).Run(); err != nil { + if err := run.PrepareCmd(cmd).Run(); err != nil { return err } } diff --git a/command/pr_checkout_test.go b/command/pr_checkout_test.go index ce0266c5e..cc7eddd2b 100644 --- a/command/pr_checkout_test.go +++ b/command/pr_checkout_test.go @@ -7,8 +7,8 @@ import ( "testing" "github.com/cli/cli/context" + "github.com/cli/cli/internal/run" "github.com/cli/cli/test" - "github.com/cli/cli/utils" ) func TestPRCheckout_sameRepo(t *testing.T) { @@ -41,7 +41,7 @@ func TestPRCheckout_sameRepo(t *testing.T) { `)) ranCommands := [][]string{} - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { switch strings.Join(cmd.Args, " ") { case "git show-ref --verify --quiet refs/heads/feature": return &errorStub{"exit status: 1"} @@ -93,7 +93,7 @@ func TestPRCheckout_urlArg(t *testing.T) { `)) ranCommands := [][]string{} - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { switch strings.Join(cmd.Args, " ") { case "git show-ref --verify --quiet refs/heads/feature": return &errorStub{"exit status: 1"} @@ -142,7 +142,7 @@ func TestPRCheckout_branchArg(t *testing.T) { `)) ranCommands := [][]string{} - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { switch strings.Join(cmd.Args, " ") { case "git show-ref --verify --quiet refs/heads/feature": return &errorStub{"exit status: 1"} @@ -191,7 +191,7 @@ func TestPRCheckout_existingBranch(t *testing.T) { `)) ranCommands := [][]string{} - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { switch strings.Join(cmd.Args, " ") { case "git show-ref --verify --quiet refs/heads/feature": return &test.OutputStub{} @@ -243,7 +243,7 @@ func TestPRCheckout_differentRepo_remoteExists(t *testing.T) { `)) ranCommands := [][]string{} - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { switch strings.Join(cmd.Args, " ") { case "git show-ref --verify --quiet refs/heads/feature": return &errorStub{"exit status: 1"} @@ -295,7 +295,7 @@ func TestPRCheckout_differentRepo(t *testing.T) { `)) ranCommands := [][]string{} - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { switch strings.Join(cmd.Args, " ") { case "git config branch.feature.merge": return &errorStub{"exit status 1"} @@ -347,7 +347,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { `)) ranCommands := [][]string{} - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { switch strings.Join(cmd.Args, " ") { case "git config branch.feature.merge": return &test.OutputStub{[]byte("refs/heads/feature\n")} @@ -397,7 +397,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { `)) ranCommands := [][]string{} - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { switch strings.Join(cmd.Args, " ") { case "git config branch.feature.merge": return &test.OutputStub{[]byte("refs/heads/feature\n")} @@ -447,7 +447,7 @@ func TestPRCheckout_maintainerCanModify(t *testing.T) { `)) ranCommands := [][]string{} - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { switch strings.Join(cmd.Args, " ") { case "git config branch.feature.merge": return &errorStub{"exit status 1"} diff --git a/command/pr_test.go b/command/pr_test.go index 4b9d0c6de..c47e28963 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -11,8 +11,8 @@ import ( "strings" "testing" + "github.com/cli/cli/internal/run" "github.com/cli/cli/test" - "github.com/cli/cli/utils" "github.com/google/shlex" "github.com/spf13/cobra" "github.com/spf13/pflag" @@ -108,7 +108,7 @@ func TestPRStatus_fork(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - defer utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + defer run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { switch strings.Join(cmd.Args, " ") { case `git config --get-regexp ^branch\.blueberries\.(remote|merge)$`: return &test.OutputStub{[]byte(`branch.blueberries.remote origin @@ -439,7 +439,7 @@ func TestPRView_previewCurrentBranch(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { return &test.OutputStub{} }) defer restoreCmd() @@ -474,7 +474,7 @@ func TestPRView_previewCurrentBranchWithEmptyBody(t *testing.T) { defer jsonFile.Close() http.StubResponse(200, jsonFile) - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { return &test.OutputStub{} }) defer restoreCmd() @@ -509,7 +509,7 @@ func TestPRView_currentBranch(t *testing.T) { http.StubResponse(200, jsonFile) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { switch strings.Join(cmd.Args, " ") { case `git config --get-regexp ^branch\.blueberries\.(remote|merge)$`: return &test.OutputStub{} @@ -547,7 +547,7 @@ func TestPRView_noResultsForBranch(t *testing.T) { http.StubResponse(200, jsonFile) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { switch strings.Join(cmd.Args, " ") { case `git config --get-regexp ^branch\.blueberries\.(remote|merge)$`: return &test.OutputStub{} @@ -580,7 +580,7 @@ func TestPRView_numberArg(t *testing.T) { `)) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) @@ -612,7 +612,7 @@ func TestPRView_numberArgWithHash(t *testing.T) { `)) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) @@ -644,7 +644,7 @@ func TestPRView_urlArg(t *testing.T) { `)) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) @@ -678,7 +678,7 @@ func TestPRView_branchArg(t *testing.T) { `)) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) @@ -713,7 +713,7 @@ func TestPRView_branchWithOwnerArg(t *testing.T) { `)) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) diff --git a/command/repo.go b/command/repo.go index 1dc9b13a5..5ea27a2ce 100644 --- a/command/repo.go +++ b/command/repo.go @@ -12,6 +12,7 @@ import ( "github.com/cli/cli/api" "github.com/cli/cli/git" "github.com/cli/cli/internal/ghrepo" + "github.com/cli/cli/internal/run" "github.com/cli/cli/utils" "github.com/spf13/cobra" ) @@ -98,7 +99,7 @@ func repoClone(cmd *cobra.Command, args []string) error { cloneCmd.Stdin = os.Stdin cloneCmd.Stdout = os.Stdout cloneCmd.Stderr = os.Stderr - return utils.PrepareCmd(cloneCmd).Run() + return run.PrepareCmd(cloneCmd).Run() } func repoCreate(cmd *cobra.Command, args []string) error { @@ -198,7 +199,7 @@ func repoCreate(cmd *cobra.Command, args []string) error { remoteAdd := git.GitCommand("remote", "add", "origin", remoteURL) remoteAdd.Stdout = os.Stdout remoteAdd.Stderr = os.Stderr - err = utils.PrepareCmd(remoteAdd).Run() + err = run.PrepareCmd(remoteAdd).Run() if err != nil { return err } @@ -218,14 +219,14 @@ func repoCreate(cmd *cobra.Command, args []string) error { gitInit := git.GitCommand("init", path) gitInit.Stdout = os.Stdout gitInit.Stderr = os.Stderr - err = utils.PrepareCmd(gitInit).Run() + err = run.PrepareCmd(gitInit).Run() if err != nil { return err } gitRemoteAdd := git.GitCommand("-C", path, "remote", "add", "origin", remoteURL) gitRemoteAdd.Stdout = os.Stdout gitRemoteAdd.Stderr = os.Stderr - err = utils.PrepareCmd(gitRemoteAdd).Run() + err = run.PrepareCmd(gitRemoteAdd).Run() if err != nil { return err } @@ -364,7 +365,7 @@ func repoFork(cmd *cobra.Command, args []string) error { cloneCmd.Stdin = os.Stdin cloneCmd.Stdout = os.Stdout cloneCmd.Stderr = os.Stderr - err = utils.PrepareCmd(cloneCmd).Run() + err = run.PrepareCmd(cloneCmd).Run() if err != nil { return fmt.Errorf("failed to clone fork: %w", err) } diff --git a/command/repo_test.go b/command/repo_test.go index 6001be817..d3b035d18 100644 --- a/command/repo_test.go +++ b/command/repo_test.go @@ -11,8 +11,8 @@ import ( "time" "github.com/cli/cli/context" + "github.com/cli/cli/internal/run" "github.com/cli/cli/test" - "github.com/cli/cli/utils" ) func TestRepoFork_already_forked(t *testing.T) { @@ -116,7 +116,7 @@ func TestRepoFork_in_parent_yes(t *testing.T) { defer http.StubWithFixture(200, "forkResult.json")() var seenCmds []*exec.Cmd - defer utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + defer run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmds = append(seenCmds, cmd) return &test.OutputStub{} })() @@ -155,7 +155,7 @@ func TestRepoFork_outside_yes(t *testing.T) { defer http.StubWithFixture(200, "forkResult.json")() var seenCmd *exec.Cmd - defer utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + defer run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} })() @@ -187,7 +187,7 @@ func TestRepoFork_outside_survey_yes(t *testing.T) { defer http.StubWithFixture(200, "forkResult.json")() var seenCmd *exec.Cmd - defer utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + defer run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} })() @@ -226,7 +226,7 @@ func TestRepoFork_outside_survey_no(t *testing.T) { defer http.StubWithFixture(200, "forkResult.json")() cmdRun := false - defer utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + defer run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { cmdRun = true return &test.OutputStub{} })() @@ -262,7 +262,7 @@ func TestRepoFork_in_parent_survey_yes(t *testing.T) { defer http.StubWithFixture(200, "forkResult.json")() var seenCmds []*exec.Cmd - defer utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + defer run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmds = append(seenCmds, cmd) return &test.OutputStub{} })() @@ -310,7 +310,7 @@ func TestRepoFork_in_parent_survey_no(t *testing.T) { defer http.StubWithFixture(200, "forkResult.json")() cmdRun := false - defer utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + defer run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { cmdRun = true return &test.OutputStub{} })() @@ -368,7 +368,7 @@ func TestRepoClone(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) @@ -408,7 +408,7 @@ func TestRepoCreate(t *testing.T) { `)) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) @@ -473,7 +473,7 @@ func TestRepoCreate_org(t *testing.T) { `)) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) @@ -538,7 +538,7 @@ func TestRepoCreate_orgWithTeam(t *testing.T) { `)) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) @@ -580,7 +580,6 @@ func TestRepoCreate_orgWithTeam(t *testing.T) { } } - func TestRepoView(t *testing.T) { initBlankContext("OWNER/REPO", "master") http := initFakeHTTP() @@ -590,7 +589,7 @@ func TestRepoView(t *testing.T) { `)) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) @@ -623,7 +622,7 @@ func TestRepoView_ownerRepo(t *testing.T) { `)) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) @@ -655,7 +654,7 @@ func TestRepoView_fullURL(t *testing.T) { { } `)) var seenCmd *exec.Cmd - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { seenCmd = cmd return &test.OutputStub{} }) diff --git a/command/testing.go b/command/testing.go index 0221ce8cf..6d1a2594e 100644 --- a/command/testing.go +++ b/command/testing.go @@ -11,8 +11,8 @@ import ( "github.com/cli/cli/api" "github.com/cli/cli/context" + "github.com/cli/cli/internal/run" "github.com/cli/cli/test" - "github.com/cli/cli/utils" ) // TODO this is split between here and test/helpers.go. I did that because otherwise our test @@ -27,7 +27,7 @@ type CmdStubber struct { func initCmdStubber() (*CmdStubber, func()) { cs := CmdStubber{} - teardown := utils.SetPrepareCmd(createStubbedPrepareCmd(&cs)) + teardown := run.SetPrepareCmd(createStubbedPrepareCmd(&cs)) return &cs, teardown } @@ -36,8 +36,8 @@ func (cs *CmdStubber) Stub(desiredOutput string) { cs.Stubs = append(cs.Stubs, &test.OutputStub{[]byte(desiredOutput)}) } -func createStubbedPrepareCmd(cs *CmdStubber) func(*exec.Cmd) utils.Runnable { - return func(cmd *exec.Cmd) utils.Runnable { +func createStubbedPrepareCmd(cs *CmdStubber) func(*exec.Cmd) run.Runnable { + return func(cmd *exec.Cmd) run.Runnable { cs.Calls = append(cs.Calls, cmd) call := cs.Count cs.Count += 1 diff --git a/git/git.go b/git/git.go index cdf1e869f..b604c398a 100644 --- a/git/git.go +++ b/git/git.go @@ -10,18 +10,18 @@ import ( "regexp" "strings" - "github.com/cli/cli/utils" + "github.com/cli/cli/internal/run" ) func VerifyRef(ref string) bool { showRef := exec.Command("git", "show-ref", "--verify", "--quiet", ref) - err := utils.PrepareCmd(showRef).Run() + err := run.PrepareCmd(showRef).Run() return err == nil } // CurrentBranch reads the checked-out branch for the git repository func CurrentBranch() (string, error) { - err := utils.PrepareCmd(GitCommand("log")).Run() + err := run.PrepareCmd(GitCommand("log")).Run() if err != nil { // this is a hack. errRe := regexp.MustCompile("your current branch '([^']+)' does not have any commits yet") @@ -32,7 +32,7 @@ func CurrentBranch() (string, error) { } // we avoid using `git branch --show-current` for compatibility with git < 2.22 branchCmd := exec.Command("git", "rev-parse", "--abbrev-ref", "HEAD") - output, err := utils.PrepareCmd(branchCmd).Output() + output, err := run.PrepareCmd(branchCmd).Output() branchName := firstLine(output) if err == nil && branchName == "HEAD" { return "", errors.New("git: not on any branch") @@ -42,13 +42,13 @@ func CurrentBranch() (string, error) { func listRemotes() ([]string, error) { remoteCmd := exec.Command("git", "remote", "-v") - output, err := utils.PrepareCmd(remoteCmd).Output() + output, err := run.PrepareCmd(remoteCmd).Output() return outputLines(output), err } func Config(name string) (string, error) { configCmd := exec.Command("git", "config", name) - output, err := utils.PrepareCmd(configCmd).Output() + output, err := run.PrepareCmd(configCmd).Output() if err != nil { return "", fmt.Errorf("unknown config key: %s", name) } @@ -63,7 +63,7 @@ var GitCommand = func(args ...string) *exec.Cmd { func UncommittedChangeCount() (int, error) { statusCmd := GitCommand("status", "--porcelain") - output, err := utils.PrepareCmd(statusCmd).Output() + output, err := run.PrepareCmd(statusCmd).Output() if err != nil { return 0, err } @@ -90,7 +90,7 @@ func Commits(baseRef, headRef string) ([]*Commit, error) { "-c", "log.ShowSignature=false", "log", "--pretty=format:%H,%s", "--cherry", fmt.Sprintf("%s...%s", baseRef, headRef)) - output, err := utils.PrepareCmd(logCmd).Output() + output, err := run.PrepareCmd(logCmd).Output() if err != nil { return []*Commit{}, err } @@ -118,7 +118,7 @@ func Commits(baseRef, headRef string) ([]*Commit, error) { func CommitBody(sha string) (string, error) { showCmd := GitCommand("-c", "log.ShowSignature=false", "show", "-s", "--pretty=format:%b", sha) - output, err := utils.PrepareCmd(showCmd).Output() + output, err := run.PrepareCmd(showCmd).Output() if err != nil { return "", err } @@ -130,7 +130,7 @@ func Push(remote string, ref string) error { pushCmd := GitCommand("push", "--set-upstream", remote, ref) pushCmd.Stdout = os.Stdout pushCmd.Stderr = os.Stderr - return utils.PrepareCmd(pushCmd).Run() + return run.PrepareCmd(pushCmd).Run() } type BranchConfig struct { @@ -143,7 +143,7 @@ type BranchConfig struct { func ReadBranchConfig(branch string) (cfg BranchConfig) { prefix := regexp.QuoteMeta(fmt.Sprintf("branch.%s.", branch)) configCmd := GitCommand("config", "--get-regexp", fmt.Sprintf("^%s(remote|merge)$", prefix)) - output, err := utils.PrepareCmd(configCmd).Output() + output, err := run.PrepareCmd(configCmd).Output() if err != nil { return } @@ -174,7 +174,7 @@ func ReadBranchConfig(branch string) (cfg BranchConfig) { // ToplevelDir returns the top-level directory path of the current repository func ToplevelDir() (string, error) { showCmd := exec.Command("git", "rev-parse", "--show-toplevel") - output, err := utils.PrepareCmd(showCmd).Output() + output, err := run.PrepareCmd(showCmd).Output() return firstLine(output), err } diff --git a/git/git_test.go b/git/git_test.go index 9e3f9a333..d52902dbe 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -4,8 +4,8 @@ import ( "os/exec" "testing" + "github.com/cli/cli/internal/run" "github.com/cli/cli/test" - "github.com/cli/cli/utils" ) func Test_UncommittedChangeCount(t *testing.T) { @@ -20,13 +20,13 @@ func Test_UncommittedChangeCount(t *testing.T) { c{Label: "untracked file", Expected: 2, Output: " M poem.txt\n?? new.txt"}, } - teardown := utils.SetPrepareCmd(func(*exec.Cmd) utils.Runnable { + teardown := run.SetPrepareCmd(func(*exec.Cmd) run.Runnable { return &test.OutputStub{} }) defer teardown() for _, v := range cases { - _ = utils.SetPrepareCmd(func(*exec.Cmd) utils.Runnable { + _ = run.SetPrepareCmd(func(*exec.Cmd) run.Runnable { return &test.OutputStub{[]byte(v.Output)} }) ucc, _ := UncommittedChangeCount() diff --git a/git/remote.go b/git/remote.go index f98606a2e..bfecfd3b0 100644 --- a/git/remote.go +++ b/git/remote.go @@ -6,7 +6,7 @@ import ( "regexp" "strings" - "github.com/cli/cli/utils" + "github.com/cli/cli/internal/run" ) var remoteRE = regexp.MustCompile(`(.+)\s+(.+)\s+\((push|fetch)\)`) @@ -76,7 +76,7 @@ func parseRemotes(gitRemotes []string) (remotes RemoteSet) { // after the fetch. func AddRemote(name, initURL, finalURL string) (*Remote, error) { addCmd := exec.Command("git", "remote", "add", "-f", name, initURL) - err := utils.PrepareCmd(addCmd).Run() + err := run.PrepareCmd(addCmd).Run() if err != nil { return nil, err } @@ -85,7 +85,7 @@ func AddRemote(name, initURL, finalURL string) (*Remote, error) { finalURL = initURL } else { setCmd := exec.Command("git", "remote", "set-url", name, finalURL) - err := utils.PrepareCmd(setCmd).Run() + err := run.PrepareCmd(setCmd).Run() if err != nil { return nil, err } diff --git a/utils/prepare_cmd.go b/internal/run/run.go similarity index 99% rename from utils/prepare_cmd.go rename to internal/run/run.go index 6b354cb80..67de76fa2 100644 --- a/utils/prepare_cmd.go +++ b/internal/run/run.go @@ -1,4 +1,4 @@ -package utils +package run import ( "bytes" diff --git a/utils/utils.go b/utils/utils.go index 6217871b7..1f7fd7106 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -7,6 +7,7 @@ import ( "github.com/briandowns/spinner" "github.com/charmbracelet/glamour" + "github.com/cli/cli/internal/run" "github.com/cli/cli/pkg/browser" ) @@ -16,7 +17,7 @@ func OpenInBrowser(url string) error { if err != nil { return err } - return PrepareCmd(browseCmd).Run() + return run.PrepareCmd(browseCmd).Run() } func RenderMarkdown(text string) (string, error) { From 88cf6ce16e2f237c3e98f43c729398dab8647edb Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 23 Mar 2020 16:36:24 -0500 Subject: [PATCH 04/12] consolidate CmdStubber into test package --- command/pr_create_test.go | 23 ++++++++++++----------- command/testing.go | 36 ------------------------------------ test/helpers.go | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 47 deletions(-) diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 688dfd9c8..3b8769ad2 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/cli/cli/context" + "github.com/cli/cli/test" ) func TestPRCreate(t *testing.T) { @@ -24,7 +25,7 @@ func TestPRCreate(t *testing.T) { } } } } `)) - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() cs.Stub("") // git status @@ -68,7 +69,7 @@ func TestPRCreate_alreadyExists(t *testing.T) { ] } } } } `)) - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() cs.Stub("") // git status @@ -88,7 +89,7 @@ func TestPRCreate_web(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() cs.Stub("") // git status @@ -123,7 +124,7 @@ func TestPRCreate_ReportsUncommittedChanges(t *testing.T) { } } } } `)) - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() cs.Stub(" M git/git.go") // git status @@ -193,7 +194,7 @@ func TestPRCreate_cross_repo_same_branch(t *testing.T) { } } } } `)) - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() cs.Stub("") // git status @@ -242,7 +243,7 @@ func TestPRCreate_survey_defaults_multicommit(t *testing.T) { } } } } `)) - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() cs.Stub("") // git status @@ -312,7 +313,7 @@ func TestPRCreate_survey_defaults_monocommit(t *testing.T) { } } } } `)) - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() cs.Stub("") // git status @@ -383,7 +384,7 @@ func TestPRCreate_survey_autofill(t *testing.T) { } } } } `)) - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() cs.Stub("") // git status @@ -426,7 +427,7 @@ func TestPRCreate_defaults_error_autofill(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() cs.Stub("") // git status @@ -442,7 +443,7 @@ func TestPRCreate_defaults_error_web(t *testing.T) { http := initFakeHTTP() http.StubRepoResponse("OWNER", "REPO") - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() cs.Stub("") // git status @@ -463,7 +464,7 @@ func TestPRCreate_defaults_error_interactive(t *testing.T) { } } } } `)) - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() cs.Stub("") // git status diff --git a/command/testing.go b/command/testing.go index 6d1a2594e..8ab97fc81 100644 --- a/command/testing.go +++ b/command/testing.go @@ -3,7 +3,6 @@ package command import ( "errors" "fmt" - "os/exec" "reflect" "github.com/AlecAivazis/survey/v2" @@ -11,43 +10,8 @@ import ( "github.com/cli/cli/api" "github.com/cli/cli/context" - "github.com/cli/cli/internal/run" - "github.com/cli/cli/test" ) -// TODO this is split between here and test/helpers.go. I did that because otherwise our test -// package would have to import utils (for utils.Runnable) which felt wrong. while utils_test -// currently doesn't import test helpers, i don't see why that ought to be precluded. -// I'm wondering if this is a case for having a global package just for storing Interfaces. -type CmdStubber struct { - Stubs []*test.OutputStub - Count int - Calls []*exec.Cmd -} - -func initCmdStubber() (*CmdStubber, func()) { - cs := CmdStubber{} - teardown := run.SetPrepareCmd(createStubbedPrepareCmd(&cs)) - return &cs, teardown -} - -func (cs *CmdStubber) Stub(desiredOutput string) { - // TODO maybe have some kind of command mapping but going simple for now - cs.Stubs = append(cs.Stubs, &test.OutputStub{[]byte(desiredOutput)}) -} - -func createStubbedPrepareCmd(cs *CmdStubber) func(*exec.Cmd) run.Runnable { - return func(cmd *exec.Cmd) run.Runnable { - cs.Calls = append(cs.Calls, cmd) - call := cs.Count - cs.Count += 1 - if call >= len(cs.Stubs) { - panic(fmt.Sprintf("more execs than stubs. most recent call: %v", cmd)) - } - return cs.Stubs[call] - } -} - type askStubber struct { Asks [][]*survey.Question Count int diff --git a/test/helpers.go b/test/helpers.go index add7b90c3..d9c3c8f68 100644 --- a/test/helpers.go +++ b/test/helpers.go @@ -1,5 +1,12 @@ package test +import ( + "fmt" + "os/exec" + + "github.com/cli/cli/internal/run" +) + // OutputStub implements a simple utils.Runnable type OutputStub struct { Out []byte @@ -12,3 +19,32 @@ func (s OutputStub) Output() ([]byte, error) { func (s OutputStub) Run() error { return nil } + +type CmdStubber struct { + Stubs []*OutputStub + Count int + Calls []*exec.Cmd +} + +func InitCmdStubber() (*CmdStubber, func()) { + cs := CmdStubber{} + teardown := run.SetPrepareCmd(createStubbedPrepareCmd(&cs)) + return &cs, teardown +} + +func (cs *CmdStubber) Stub(desiredOutput string) { + // TODO maybe have some kind of command mapping but going simple for now + cs.Stubs = append(cs.Stubs, &OutputStub{[]byte(desiredOutput)}) +} + +func createStubbedPrepareCmd(cs *CmdStubber) func(*exec.Cmd) run.Runnable { + return func(cmd *exec.Cmd) run.Runnable { + cs.Calls = append(cs.Calls, cmd) + call := cs.Count + cs.Count += 1 + if call >= len(cs.Stubs) { + panic(fmt.Sprintf("more execs than stubs. most recent call: %v", cmd)) + } + return cs.Stubs[call] + } +} From dd1e2a2dfc0a275f749dbc78b5e1632de11c74db Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 24 Mar 2020 14:18:49 -0500 Subject: [PATCH 05/12] add StubError to CmdStubber --- command/pr_checkout_test.go | 4 ++-- command/pr_test.go | 2 +- git/git_test.go | 2 +- test/helpers.go | 17 +++++++++++++++-- 4 files changed, 19 insertions(+), 6 deletions(-) diff --git a/command/pr_checkout_test.go b/command/pr_checkout_test.go index cc7eddd2b..3ab8afeee 100644 --- a/command/pr_checkout_test.go +++ b/command/pr_checkout_test.go @@ -350,7 +350,7 @@ func TestPRCheckout_differentRepo_existingBranch(t *testing.T) { restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { switch strings.Join(cmd.Args, " ") { case "git config branch.feature.merge": - return &test.OutputStub{[]byte("refs/heads/feature\n")} + return &test.OutputStub{[]byte("refs/heads/feature\n"), nil} default: ranCommands = append(ranCommands, cmd.Args) return &test.OutputStub{} @@ -400,7 +400,7 @@ func TestPRCheckout_differentRepo_currentBranch(t *testing.T) { restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { switch strings.Join(cmd.Args, " ") { case "git config branch.feature.merge": - return &test.OutputStub{[]byte("refs/heads/feature\n")} + return &test.OutputStub{[]byte("refs/heads/feature\n"), nil} default: ranCommands = append(ranCommands, cmd.Args) return &test.OutputStub{} diff --git a/command/pr_test.go b/command/pr_test.go index c47e28963..8647b32d7 100644 --- a/command/pr_test.go +++ b/command/pr_test.go @@ -112,7 +112,7 @@ func TestPRStatus_fork(t *testing.T) { switch strings.Join(cmd.Args, " ") { case `git config --get-regexp ^branch\.blueberries\.(remote|merge)$`: return &test.OutputStub{[]byte(`branch.blueberries.remote origin -branch.blueberries.merge refs/heads/blueberries`)} +branch.blueberries.merge refs/heads/blueberries`), nil} default: panic("not implemented") } diff --git a/git/git_test.go b/git/git_test.go index d52902dbe..d8698751f 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -27,7 +27,7 @@ func Test_UncommittedChangeCount(t *testing.T) { for _, v := range cases { _ = run.SetPrepareCmd(func(*exec.Cmd) run.Runnable { - return &test.OutputStub{[]byte(v.Output)} + return &test.OutputStub{[]byte(v.Output), nil} }) ucc, _ := UncommittedChangeCount() diff --git a/test/helpers.go b/test/helpers.go index d9c3c8f68..9ffb18c41 100644 --- a/test/helpers.go +++ b/test/helpers.go @@ -1,6 +1,7 @@ package test import ( + "errors" "fmt" "os/exec" @@ -9,14 +10,21 @@ import ( // OutputStub implements a simple utils.Runnable type OutputStub struct { - Out []byte + Out []byte + Error error } func (s OutputStub) Output() ([]byte, error) { + if s.Error != nil { + return s.Out, s.Error + } return s.Out, nil } func (s OutputStub) Run() error { + if s.Error != nil { + return s.Error + } return nil } @@ -34,7 +42,12 @@ func InitCmdStubber() (*CmdStubber, func()) { func (cs *CmdStubber) Stub(desiredOutput string) { // TODO maybe have some kind of command mapping but going simple for now - cs.Stubs = append(cs.Stubs, &OutputStub{[]byte(desiredOutput)}) + cs.Stubs = append(cs.Stubs, &OutputStub{[]byte(desiredOutput), nil}) +} + +func (cs *CmdStubber) StubError(msg string) { + // TODO consider handling CmdErr instead of a raw error + cs.Stubs = append(cs.Stubs, &OutputStub{[]byte{}, errors.New(msg)}) } func createStubbedPrepareCmd(cs *CmdStubber) func(*exec.Cmd) run.Runnable { From 4632ab1a1ec9b22c7ff96c639ca96c673762d09f Mon Sep 17 00:00:00 2001 From: vilmibm Date: Tue, 24 Mar 2020 14:19:02 -0500 Subject: [PATCH 06/12] test for CurrentBranch on an empty repo --- git/git.go | 2 +- git/git_test.go | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/git/git.go b/git/git.go index b604c398a..8f2025f54 100644 --- a/git/git.go +++ b/git/git.go @@ -28,7 +28,7 @@ func CurrentBranch() (string, error) { matches := errRe.FindAllStringSubmatch(err.Error(), -1) if len(matches) > 0 && matches[0][1] != "" { return matches[0][1], nil - } + } // else we continue on. it's weird that git log failed but we'll let other errors surface. } // we avoid using `git branch --show-current` for compatibility with git < 2.22 branchCmd := exec.Command("git", "rev-parse", "--abbrev-ref", "HEAD") diff --git a/git/git_test.go b/git/git_test.go index d8698751f..d5791e451 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -1,6 +1,7 @@ package git import ( + "fmt" "os/exec" "testing" @@ -36,3 +37,21 @@ func Test_UncommittedChangeCount(t *testing.T) { } } } + +func Test_CurrentBranch_no_commits(t *testing.T) { + cs, teardown := test.InitCmdStubber() + defer teardown() + + expected := "cool_branch-name" + + cs.StubError(fmt.Sprintf("fatal: your current branch '%s' does not have any commits yet", expected)) + + result, err := CurrentBranch() + if err != nil { + t.Errorf("got unexpected error: %w", err) + } + if result != expected { + t.Errorf("unexpected branch name: %s instead of %s", result, expected) + } + +} From 897d5c3dcae96587a4337df739831320a246622c Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 25 Mar 2020 11:57:53 -0500 Subject: [PATCH 07/12] fall back to symbolic-ref in CurrentBranch --- git/git.go | 32 ++++++++++++++++++-------------- git/git_test.go | 49 +++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 63 insertions(+), 18 deletions(-) diff --git a/git/git.go b/git/git.go index 8f2025f54..09c731885 100644 --- a/git/git.go +++ b/git/git.go @@ -21,23 +21,27 @@ func VerifyRef(ref string) bool { // CurrentBranch reads the checked-out branch for the git repository func CurrentBranch() (string, error) { - err := run.PrepareCmd(GitCommand("log")).Run() - if err != nil { - // this is a hack. - errRe := regexp.MustCompile("your current branch '([^']+)' does not have any commits yet") - matches := errRe.FindAllStringSubmatch(err.Error(), -1) - if len(matches) > 0 && matches[0][1] != "" { - return matches[0][1], nil - } // else we continue on. it's weird that git log failed but we'll let other errors surface. - } + noBranchError := errors.New("git: not on any branch") + // we avoid using `git branch --show-current` for compatibility with git < 2.22 - branchCmd := exec.Command("git", "rev-parse", "--abbrev-ref", "HEAD") - output, err := run.PrepareCmd(branchCmd).Output() + output, err := run.PrepareCmd(GitCommand("rev-parse", "--abbrev-ref", "HEAD")).Output() branchName := firstLine(output) - if err == nil && branchName == "HEAD" { - return "", errors.New("git: not on any branch") + if err == nil { + if branchName == "HEAD" { + return "", noBranchError + } else { + return branchName, nil + } } - return branchName, err + + // Fall back to symbolic-ref in case we're in a repository with no commits + output, err = run.PrepareCmd(GitCommand("symbolic-ref", "--short", "HEAD")).Output() + if err != nil { + return "", noBranchError + } + branchName = firstLine(output) + + return branchName, nil } func listRemotes() ([]string, error) { diff --git a/git/git_test.go b/git/git_test.go index d5791e451..50ce4c71c 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -1,7 +1,6 @@ package git import ( - "fmt" "os/exec" "testing" @@ -42,16 +41,58 @@ func Test_CurrentBranch_no_commits(t *testing.T) { cs, teardown := test.InitCmdStubber() defer teardown() - expected := "cool_branch-name" + expected := "branch-name" - cs.StubError(fmt.Sprintf("fatal: your current branch '%s' does not have any commits yet", expected)) + cs.StubError("") + cs.Stub(expected) result, err := CurrentBranch() if err != nil { t.Errorf("got unexpected error: %w", err) } + if len(cs.Calls) != 2 { + t.Errorf("expected 2 git calls, saw %d", len(cs.Calls)) + } if result != expected { t.Errorf("unexpected branch name: %s instead of %s", result, expected) } - +} + +func Test_CurrentBranch(t *testing.T) { + cs, teardown := test.InitCmdStubber() + defer teardown() + + expected := "branch-name" + + cs.Stub(expected) + + result, err := CurrentBranch() + if err != nil { + t.Errorf("got unexpected error: %w", err) + } + if len(cs.Calls) != 1 { + t.Errorf("expected 1 git call, saw %d", len(cs.Calls)) + } + if result != expected { + t.Errorf("unexpected branch name: %s instead of %s", result, expected) + } +} + +func Test_CurrentBranch_detached_head(t *testing.T) { + cs, teardown := test.InitCmdStubber() + defer teardown() + + cs.Stub("HEAD") + + _, err := CurrentBranch() + if err == nil { + t.Errorf("expected an error") + } + expectedError := "git: not on any branch" + if err.Error() != expectedError { + t.Errorf("got unexpected error: %s instead of %s", err.Error(), expectedError) + } + if len(cs.Calls) != 1 { + t.Errorf("expected 1 git call, saw %d", len(cs.Calls)) + } } From dd877b6a34923fc721ed44d3bb3400af8144b5e0 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 25 Mar 2020 12:01:44 -0500 Subject: [PATCH 08/12] merge fallout --- command/pr_checkout_test.go | 2 +- command/pr_create_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/command/pr_checkout_test.go b/command/pr_checkout_test.go index 4f48babe4..9aca71901 100644 --- a/command/pr_checkout_test.go +++ b/command/pr_checkout_test.go @@ -145,7 +145,7 @@ func TestPRCheckout_urlArg_differentBase(t *testing.T) { `)) ranCommands := [][]string{} - restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable { + restoreCmd := run.SetPrepareCmd(func(cmd *exec.Cmd) run.Runnable { switch strings.Join(cmd.Args, " ") { case "git show-ref --verify --quiet refs/heads/feature": return &errorStub{"exit status: 1"} diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 4ec940f77..76a7c8803 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -98,7 +98,7 @@ func TestPRCreate_alreadyExistsDifferentBase(t *testing.T) { `)) http.StubResponse(200, bytes.NewBufferString("{}")) - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() cs.Stub("") // git status From 0af232444cbde4cacdf00a05e81f57770fb0bf67 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 26 Mar 2020 15:00:58 -0500 Subject: [PATCH 09/12] consolidate into one symbolic-ref call --- git/git.go | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/git/git.go b/git/git.go index 09c731885..bbdc6e301 100644 --- a/git/git.go +++ b/git/git.go @@ -21,27 +21,23 @@ func VerifyRef(ref string) bool { // CurrentBranch reads the checked-out branch for the git repository func CurrentBranch() (string, error) { - noBranchError := errors.New("git: not on any branch") - // we avoid using `git branch --show-current` for compatibility with git < 2.22 - output, err := run.PrepareCmd(GitCommand("rev-parse", "--abbrev-ref", "HEAD")).Output() - branchName := firstLine(output) + refCmd := GitCommand("symbolic-ref", "--quiet", "--short", "HEAD") + + output, err := run.PrepareCmd(refCmd).Output() if err == nil { - if branchName == "HEAD" { - return "", noBranchError - } else { - return branchName, nil - } + // Found the branch name + return firstLine(output), nil } - // Fall back to symbolic-ref in case we're in a repository with no commits - output, err = run.PrepareCmd(GitCommand("symbolic-ref", "--short", "HEAD")).Output() - if err != nil { - return "", noBranchError + ce := err.(*run.CmdError) + if ce.Stderr.Len() == 0 { + // Detached head + return "", errors.New("git: not on any branch") } - branchName = firstLine(output) - return branchName, nil + // Unknown error + return "", err } func listRemotes() ([]string, error) { From ca99096ca8e97dcbb4abeb54b297cf91304e2898 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Thu, 26 Mar 2020 15:32:31 -0500 Subject: [PATCH 10/12] use CmdError in StubError and fix git_test --- git/git.go | 1 - git/git_test.go | 43 +++++++++++++++++++++---------------------- test/helpers.go | 11 +++++++---- 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/git/git.go b/git/git.go index bbdc6e301..16570a0fa 100644 --- a/git/git.go +++ b/git/git.go @@ -21,7 +21,6 @@ func VerifyRef(ref string) bool { // CurrentBranch reads the checked-out branch for the git repository func CurrentBranch() (string, error) { - refCmd := GitCommand("symbolic-ref", "--quiet", "--short", "HEAD") output, err := run.PrepareCmd(refCmd).Output() diff --git a/git/git_test.go b/git/git_test.go index 50ce4c71c..b73104c95 100644 --- a/git/git_test.go +++ b/git/git_test.go @@ -37,27 +37,6 @@ func Test_UncommittedChangeCount(t *testing.T) { } } -func Test_CurrentBranch_no_commits(t *testing.T) { - cs, teardown := test.InitCmdStubber() - defer teardown() - - expected := "branch-name" - - cs.StubError("") - cs.Stub(expected) - - result, err := CurrentBranch() - if err != nil { - t.Errorf("got unexpected error: %w", err) - } - if len(cs.Calls) != 2 { - t.Errorf("expected 2 git calls, saw %d", len(cs.Calls)) - } - if result != expected { - t.Errorf("unexpected branch name: %s instead of %s", result, expected) - } -} - func Test_CurrentBranch(t *testing.T) { cs, teardown := test.InitCmdStubber() defer teardown() @@ -82,7 +61,7 @@ func Test_CurrentBranch_detached_head(t *testing.T) { cs, teardown := test.InitCmdStubber() defer teardown() - cs.Stub("HEAD") + cs.StubError("") _, err := CurrentBranch() if err == nil { @@ -96,3 +75,23 @@ func Test_CurrentBranch_detached_head(t *testing.T) { t.Errorf("expected 1 git call, saw %d", len(cs.Calls)) } } + +func Test_CurrentBranch_unexpected_error(t *testing.T) { + cs, teardown := test.InitCmdStubber() + defer teardown() + + cs.StubError("lol") + + expectedError := "lol\nstub: lol" + + _, err := CurrentBranch() + if err == nil { + t.Errorf("expected an error") + } + if err.Error() != expectedError { + t.Errorf("got unexpected error: %s instead of %s", err.Error(), expectedError) + } + if len(cs.Calls) != 1 { + t.Errorf("expected 1 git call, saw %d", len(cs.Calls)) + } +} diff --git a/test/helpers.go b/test/helpers.go index cffdc5272..390d779c4 100644 --- a/test/helpers.go +++ b/test/helpers.go @@ -1,6 +1,7 @@ package test import ( + "bytes" "errors" "fmt" "os/exec" @@ -13,7 +14,7 @@ import ( // OutputStub implements a simple utils.Runnable type OutputStub struct { Out []byte - Error error + Error *run.CmdError } func (s OutputStub) Output() ([]byte, error) { @@ -47,9 +48,11 @@ func (cs *CmdStubber) Stub(desiredOutput string) { cs.Stubs = append(cs.Stubs, &OutputStub{[]byte(desiredOutput), nil}) } -func (cs *CmdStubber) StubError(msg string) { - // TODO consider handling CmdErr instead of a raw error - cs.Stubs = append(cs.Stubs, &OutputStub{[]byte{}, errors.New(msg)}) +func (cs *CmdStubber) StubError(errText string) { + stderrBuff := bytes.NewBufferString(errText) + args := []string{"stub"} // TODO make more real? + err := errors.New(errText) + cs.Stubs = append(cs.Stubs, &OutputStub{[]byte{}, &run.CmdError{stderrBuff, args, err}}) } func createStubbedPrepareCmd(cs *CmdStubber) func(*exec.Cmd) run.Runnable { From eb403a3b1e992400beef25ddb8d2181cb7d26b05 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Fri, 27 Mar 2020 11:35:39 -0500 Subject: [PATCH 11/12] more nuanced error typing --- git/git.go | 10 ++++++---- test/helpers.go | 3 ++- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/git/git.go b/git/git.go index 16570a0fa..a4fae4408 100644 --- a/git/git.go +++ b/git/git.go @@ -29,10 +29,12 @@ func CurrentBranch() (string, error) { return firstLine(output), nil } - ce := err.(*run.CmdError) - if ce.Stderr.Len() == 0 { - // Detached head - return "", errors.New("git: not on any branch") + var cmdErr *run.CmdError + if errors.As(err, &cmdErr) { + if cmdErr.Stderr.Len() == 0 { + // Detached head + return "", errors.New("git: not on any branch") + } } // Unknown error diff --git a/test/helpers.go b/test/helpers.go index 390d779c4..556b27157 100644 --- a/test/helpers.go +++ b/test/helpers.go @@ -14,7 +14,7 @@ import ( // OutputStub implements a simple utils.Runnable type OutputStub struct { Out []byte - Error *run.CmdError + Error error } func (s OutputStub) Output() ([]byte, error) { @@ -49,6 +49,7 @@ func (cs *CmdStubber) Stub(desiredOutput string) { } func (cs *CmdStubber) StubError(errText string) { + // TODO support error types beyond CmdError stderrBuff := bytes.NewBufferString(errText) args := []string{"stub"} // TODO make more real? err := errors.New(errText) From c966f0dca5a472fba79485cefc71780295c59309 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 30 Mar 2020 12:04:45 -0500 Subject: [PATCH 12/12] test fallout --- command/pr_create_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/command/pr_create_test.go b/command/pr_create_test.go index 2791a083d..72a439677 100644 --- a/command/pr_create_test.go +++ b/command/pr_create_test.go @@ -589,7 +589,7 @@ func TestPRCreate_defaults_error_interactive(t *testing.T) { } func Test_determineTrackingBranch_empty(t *testing.T) { - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() remotes := context.Remotes{} @@ -604,7 +604,7 @@ func Test_determineTrackingBranch_empty(t *testing.T) { } func Test_determineTrackingBranch_noMatch(t *testing.T) { - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() remotes := context.Remotes{ @@ -631,7 +631,7 @@ deadb00f refs/remotes/origin/feature`) // git show-ref --verify (ShowRefs) } func Test_determineTrackingBranch_hasMatch(t *testing.T) { - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() remotes := context.Remotes{ @@ -664,7 +664,7 @@ deadbeef refs/remotes/upstream/feature`) // git show-ref --verify (ShowRefs) } func Test_determineTrackingBranch_respectTrackingConfig(t *testing.T) { - cs, cmdTeardown := initCmdStubber() + cs, cmdTeardown := test.InitCmdStubber() defer cmdTeardown() remotes := context.Remotes{