STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Merge pull request #47 from github/exec-cmd-error-info

Browse source 
Ensure git operations preserve their stderr in error output
This commit is contained in:
Mislav Marohnić 2019-11-06 19:45:27 +01:00 • committed by GitHub
commit fdee02e3e8
7 changed files with 149 additions and 60 deletions
Download patch file Download diff file Expand all files Collapse all files

18
command/issue_test.go
View file

@ -2,10 +2,12 @@ package command
import (
"os"
"os/exec"
"regexp"
"testing"
"github.com/github/gh-cli/test"
"github.com/github/gh-cli/utils"
)
func TestIssueStatus(t *testing.T) {
@ -43,8 +45,12 @@ func TestIssueView(t *testing.T) {
defer jsonFile.Close()
http.StubResponse(200, jsonFile)
teardown, callCount := mockOpenInBrowser()
defer teardown()
var seenCmd *exec.Cmd
restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable {
seenCmd = cmd
return &outputStub{}
})
defer restoreCmd()
output, err := test.RunCommand(RootCmd, "issue view 8")
if err != nil {
@ -55,7 +61,11 @@ func TestIssueView(t *testing.T) {
t.Errorf("command output expected got an empty string")
}
if *callCount != 1 {
t.Errorf("OpenInBrowser should be called 1 time but was called %d time(s)", *callCount)
if seenCmd == nil {
t.Fatal("expected a command to run")
}
url := seenCmd.Args[len(seenCmd.Args)-1]
if url != "https://github.com/OWNER/REPO/issues/8" {
t.Errorf("got: %q", url)
}
}

38
command/pr_test.go
View file

@ -2,10 +2,12 @@ package command
import (
"os"
"os/exec"
"regexp"
"testing"
"github.com/github/gh-cli/test"
"github.com/github/gh-cli/utils"
)
func TestPRList(t *testing.T) {
@ -43,8 +45,12 @@ func TestPRView(t *testing.T) {
defer jsonFile.Close()
http.StubResponse(200, jsonFile)
teardown, callCount := mockOpenInBrowser()
defer teardown()
var seenCmd *exec.Cmd
restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable {
seenCmd = cmd
return &outputStub{}
})
defer restoreCmd()
output, err := test.RunCommand(RootCmd, "pr view")
if err != nil {
@ -55,8 +61,12 @@ func TestPRView(t *testing.T) {
t.Errorf("command output expected got an empty string")
}
if *callCount != 1 {
t.Errorf("OpenInBrowser should be called 1 time but was called %d time(s)", *callCount)
if seenCmd == nil {
t.Fatal("expected a command to run")
}
url := seenCmd.Args[len(seenCmd.Args)-1]
if url != "https://github.com/OWNER/REPO/pull/10" {
t.Errorf("got: %q", url)
}
}
@ -68,16 +78,20 @@ func TestPRView_NoActiveBranch(t *testing.T) {
defer jsonFile.Close()
http.StubResponse(200, jsonFile)
teardown, callCount := mockOpenInBrowser()
defer teardown()
var seenCmd *exec.Cmd
restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable {
seenCmd = cmd
return &outputStub{}
})
defer restoreCmd()
output, err := test.RunCommand(RootCmd, "pr view")
if err == nil || err.Error() != "the 'master' branch has no open pull requests" {
t.Errorf("error running command `pr view`: %v", err)
}
if *callCount > 0 {
t.Errorf("OpenInBrowser should NOT be called but was called %d time(s)", *callCount)
if seenCmd != nil {
t.Fatalf("unexpected command: %v", seenCmd.Args)
}
// Now run again but provide a PR number
@ -90,7 +104,11 @@ func TestPRView_NoActiveBranch(t *testing.T) {
t.Errorf("command output expected got an empty string")
}
if *callCount != 1 {
t.Errorf("OpenInBrowser should be called once but was called %d time(s)", *callCount)
if seenCmd == nil {
t.Fatal("expected a command to run")
}
url := seenCmd.Args[len(seenCmd.Args)-1]
if url != "https://github.com/OWNER/REPO/pull/23" {
t.Errorf("got: %q", url)
}
}

25
command/testing.go
View file

@ -3,7 +3,6 @@ package command
import (
"github.com/github/gh-cli/api"
"github.com/github/gh-cli/context"
"github.com/github/gh-cli/utils"
)
func initBlankContext(repo, branch string) {
@ -23,17 +22,15 @@ func initFakeHTTP() *api.FakeHTTP {
return http
}
func mockOpenInBrowser() (func(), *int) {
callCount := 0
originalOpenInBrowser := utils.OpenInBrowser
teardown := func() {
utils.OpenInBrowser = originalOpenInBrowser
}
utils.OpenInBrowser = func(_ string) error {
callCount++
return nil
}
return teardown, &callCount
// outputStub implements a simple utils.Runnable
type outputStub struct {
output []byte
}
func (s outputStub) Output() ([]byte, error) {
return s.output, nil
}
func (s outputStub) Run() error {
return nil
}

39
git/git.go
View file

@ -8,12 +8,13 @@ import (
"os/exec"
"path/filepath"
"strings"
"github.com/github/gh-cli/utils"
)
func Dir() (string, error) {
dirCmd := exec.Command("git", "rev-parse", "-q", "--git-dir")
dirCmd.Stderr = nil
output, err := dirCmd.Output()
output, err := utils.PrepareCmd(dirCmd).Output()
if err != nil {
return "", fmt.Errorf("Not a git repository (or any of the parent directories): .git")
}
@ -33,7 +34,7 @@ func Dir() (string, error) {
func WorkdirName() (string, error) {
toplevelCmd := exec.Command("git", "rev-parse", "--show-toplevel")
toplevelCmd.Stderr = nil
output, err := toplevelCmd.Output()
output, err := utils.PrepareCmd(toplevelCmd).Output()
dir := firstLine(output)
if dir == "" {
return "", fmt.Errorf("unable to determine git working directory")
@ -44,8 +45,7 @@ func WorkdirName() (string, error) {
func HasFile(segments ...string) bool {
// The blessed way to resolve paths within git dir since Git 2.5.0
pathCmd := exec.Command("git", "rev-parse", "-q", "--git-path", filepath.Join(segments...))
pathCmd.Stderr = nil
if output, err := pathCmd.Output(); err == nil {
if output, err := utils.PrepareCmd(pathCmd).Output(); err == nil {
if lines := outputLines(output); len(lines) == 1 {
if _, err := os.Stat(lines[0]); err == nil {
return true
@ -97,8 +97,7 @@ func BranchAtRef(paths ...string) (name string, err error) {
func Editor() (string, error) {
varCmd := exec.Command("git", "var", "GIT_EDITOR")
varCmd.Stderr = nil
output, err := varCmd.Output()
output, err := utils.PrepareCmd(varCmd).Output()
if err != nil {
return "", fmt.Errorf("Can't load git var: GIT_EDITOR")
}
@ -112,8 +111,7 @@ func Head() (string, error) {
func SymbolicFullName(name string) (string, error) {
parseCmd := exec.Command("git", "rev-parse", "--symbolic-full-name", name)
parseCmd.Stderr = nil
output, err := parseCmd.Output()
output, err := utils.PrepareCmd(parseCmd).Output()
if err != nil {
return "", fmt.Errorf("Unknown revision or path not in the working tree: %s", name)
}
@ -145,9 +143,7 @@ func CommentChar(text string) (string, error) {
func Show(sha string) (string, error) {
cmd := exec.Command("git", "-c", "log.showSignature=false", "show", "-s", "--format=%s%n%+b", sha)
cmd.Stderr = nil
output, err := cmd.Output()
output, err := utils.PrepareCmd(cmd).Output()
return strings.TrimSpace(string(output)), err
}
@ -157,7 +153,7 @@ func Log(sha1, sha2 string) (string, error) {
"-c", "log.showSignature=false", "log", "--no-color",
"--format=%h (%aN, %ar)%n%w(78,3,3)%s%n%+b",
"--cherry", shaRange)
outputs, err := cmd.Output()
outputs, err := utils.PrepareCmd(cmd).Output()
if err != nil {
return "", fmt.Errorf("Can't load git log %s..%s", sha1, sha2)
}
@ -167,14 +163,13 @@ func Log(sha1, sha2 string) (string, error) {
func listRemotes() ([]string, error) {
remoteCmd := exec.Command("git", "remote", "-v")
remoteCmd.Stderr = nil
output, err := remoteCmd.Output()
output, err := utils.PrepareCmd(remoteCmd).Output()
return outputLines(output), err
}
func Config(name string) (string, error) {
configCmd := exec.Command("git", "config", name)
output, err := configCmd.Output()
output, err := utils.PrepareCmd(configCmd).Output()
if err != nil {
return "", fmt.Errorf("unknown config key: %s", name)
}
@ -190,21 +185,16 @@ func ConfigAll(name string) ([]string, error) {
}
configCmd := exec.Command("git", "config", mode, name)
output, err := configCmd.Output()
output, err := utils.PrepareCmd(configCmd).Output()
if err != nil {
return nil, fmt.Errorf("Unknown config %s", name)
}
return outputLines(output), nil
}
func Run(args ...string) error {
cmd := exec.Command("git", args...)
return cmd.Run()
}
func LocalBranches() ([]string, error) {
branchesCmd := exec.Command("git", "branch", "--list")
output, err := branchesCmd.Output()
output, err := utils.PrepareCmd(branchesCmd).Output()
if err != nil {
return nil, err
}
@ -218,9 +208,6 @@ func LocalBranches() ([]string, error) {
func outputLines(output []byte) []string {
lines := strings.TrimSuffix(string(output), "\n")
if lines == "" {
return []string{}
}
return strings.Split(lines, "\n")
}

8
test/fixtures/prView.json vendored
View file

@ -6,7 +6,7 @@
"node": {
"number": 10,
"title": "Blueberries are a good fruit",
"url": "https://github.com/github/gh-cli/pull/10",
"url": "https://github.com/OWNER/REPO/pull/10",
"headRefName": "[blueberries]"
}
}
@ -19,7 +19,7 @@
"node": {
"number": 8,
"title": "Strawberries are not actually berries",
"url": "https://github.com/github/gh-cli/pull/8",
"url": "https://github.com/OWNER/REPO/pull/8",
"headRefName": "[strawberries]"
}
}
@ -32,7 +32,7 @@
"node": {
"number": 9,
"title": "Apples are tasty",
"url": "https://github.com/github/gh-cli/pull/9",
"url": "https://github.com/OWNER/REPO/pull/9",
"headRefName": "[apples]"
}
},
@ -40,7 +40,7 @@
"node": {
"number": 11,
"title": "Figs are my favorite",
"url": "https://github.com/github/gh-cli/pull/1",
"url": "https://github.com/OWNER/REPO/pull/1",
"headRefName": "[figs]"
}
}

76
utils/prepare_cmd.go Normal file
View file

@ -0,0 +1,76 @@
package utils
import (
"bytes"
"fmt"
"os"
"os/exec"
"strings"
)
// Runnable is typically an exec.Cmd or its stub in tests
type Runnable interface {
Output() ([]byte, error)
Run() error
}
// PrepareCmd extends exec.Cmd with extra error reporting features and provides a
// hook to stub command execution in tests
var PrepareCmd = func(cmd *exec.Cmd) Runnable {
return &cmdWithStderr{cmd}
}
// SetPrepareCmd overrides PrepareCmd and returns a func to revert it back
func SetPrepareCmd(fn func(*exec.Cmd) Runnable) func() {
origPrepare := PrepareCmd
PrepareCmd = fn
return func() {
PrepareCmd = origPrepare
}
}
// cmdWithStderr augments exec.Cmd by adding stderr to the error message
type cmdWithStderr struct {
*exec.Cmd
}
func (c cmdWithStderr) Output() ([]byte, error) {
if os.Getenv("DEBUG") != "" {
fmt.Fprintf(os.Stderr, "%v\n", c.Cmd.Args)
}
errStream := &bytes.Buffer{}
c.Cmd.Stderr = errStream
out, err := c.Cmd.Output()
if err != nil {
err = &CmdError{errStream, c.Cmd.Args, err}
}
return out, err
}
func (c cmdWithStderr) Run() error {
if os.Getenv("DEBUG") != "" {
fmt.Fprintf(os.Stderr, "%v\n", c.Cmd.Args)
}
errStream := &bytes.Buffer{}
c.Cmd.Stderr = errStream
err := c.Cmd.Run()
if err != nil {
err = &CmdError{errStream, c.Cmd.Args, err}
}
return err
}
// CmdError provides more visibility into why an exec.Cmd had failed
type CmdError struct {
Stderr *bytes.Buffer
Args []string
Err error
}
func (e CmdError) Error() string {
msg := e.Stderr.String()
if msg != "" && !strings.HasSuffix(msg, "\n") {
msg += "\n"
}
return fmt.Sprintf("%s%s: %s", msg, e.Args[0], e.Err)
}

5
utils/utils.go
View file

@ -27,7 +27,7 @@ func ConcatPaths(paths ...string) string {
return strings.Join(paths, "/")
}
var OpenInBrowser = func(url string) error {
func OpenInBrowser(url string) error {
browser := os.Getenv("BROWSER")
if browser == "" {
browser = searchBrowserLauncher(runtime.GOOS)
@ -45,7 +45,8 @@ var OpenInBrowser = func(url string) error {
}
endingArgs := append(browserArgs[1:], url)
return exec.Command(browserArgs[0], endingArgs...).Run()
browseCmd := exec.Command(browserArgs[0], endingArgs...)
return PrepareCmd(browseCmd).Run()
}
func searchBrowserLauncher(goos string) (browser string) {