Skip to content

Commit fdee02e

Browse files
mislavmislav
authored
Merge pull request cli#47 from github/exec-cmd-error-info
Ensure git operations preserve their stderr in error output
2 parents cdd4951 + 524fe0a commit fdee02e

File tree

7 files changed

+147
-58
lines changed

7 files changed

+147
-58
lines changed

‎command/issue_test.go‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,12 @@ package command
22

33
import (
44
"os"
5+
"os/exec"
56
"regexp"
67
"testing"
78

89
"github.com/github/gh-cli/test"
10+
"github.com/github/gh-cli/utils"
911
)
1012

1113
func TestIssueStatus(t *testing.T) {
@@ -43,8 +45,12 @@ func TestIssueView(t *testing.T) {
4345
defer jsonFile.Close()
4446
http.StubResponse(200, jsonFile)
4547

46-
teardown, callCount := mockOpenInBrowser()
47-
defer teardown()
48+
var seenCmd *exec.Cmd
49+
restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable {
50+
seenCmd = cmd
51+
return &outputStub{}
52+
})
53+
defer restoreCmd()
4854

4955
output, err := test.RunCommand(RootCmd, "issue view 8")
5056
if err != nil {
@@ -55,7 +61,11 @@ func TestIssueView(t *testing.T) {
5561
t.Errorf("command output expected got an empty string")
5662
}
5763

58-
if *callCount != 1 {
59-
t.Errorf("OpenInBrowser should be called 1 time but was called %d time(s)", *callCount)
64+
if seenCmd == nil {
65+
t.Fatal("expected a command to run")
66+
}
67+
url := seenCmd.Args[len(seenCmd.Args)-1]
68+
if url != "https://github.com/OWNER/REPO/issues/8" {
69+
t.Errorf("got: %q", url)
6070
}
6171
}

‎command/pr_test.go‎

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,12 @@ package command
22

33
import (
44
"os"
5+
"os/exec"
56
"regexp"
67
"testing"
78

89
"github.com/github/gh-cli/test"
10+
"github.com/github/gh-cli/utils"
911
)
1012

1113
func TestPRList(t *testing.T) {
@@ -43,8 +45,12 @@ func TestPRView(t *testing.T) {
4345
defer jsonFile.Close()
4446
http.StubResponse(200, jsonFile)
4547

46-
teardown, callCount := mockOpenInBrowser()
47-
defer teardown()
48+
var seenCmd *exec.Cmd
49+
restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable {
50+
seenCmd = cmd
51+
return &outputStub{}
52+
})
53+
defer restoreCmd()
4854

4955
output, err := test.RunCommand(RootCmd, "pr view")
5056
if err != nil {
@@ -55,8 +61,12 @@ func TestPRView(t *testing.T) {
5561
t.Errorf("command output expected got an empty string")
5662
}
5763

58-
if *callCount != 1 {
59-
t.Errorf("OpenInBrowser should be called 1 time but was called %d time(s)", *callCount)
64+
if seenCmd == nil {
65+
t.Fatal("expected a command to run")
66+
}
67+
url := seenCmd.Args[len(seenCmd.Args)-1]
68+
if url != "https://github.com/OWNER/REPO/pull/10" {
69+
t.Errorf("got: %q", url)
6070
}
6171
}
6272

@@ -68,16 +78,20 @@ func TestPRView_NoActiveBranch(t *testing.T) {
6878
defer jsonFile.Close()
6979
http.StubResponse(200, jsonFile)
7080

71-
teardown, callCount := mockOpenInBrowser()
72-
defer teardown()
81+
var seenCmd *exec.Cmd
82+
restoreCmd := utils.SetPrepareCmd(func(cmd *exec.Cmd) utils.Runnable {
83+
seenCmd = cmd
84+
return &outputStub{}
85+
})
86+
defer restoreCmd()
7387

7488
output, err := test.RunCommand(RootCmd, "pr view")
7589
if err == nil || err.Error() != "the 'master' branch has no open pull requests" {
7690
t.Errorf("error running command `pr view`: %v", err)
7791
}
7892

79-
if *callCount > 0 {
80-
t.Errorf("OpenInBrowser should NOT be called but was called %d time(s)", *callCount)
93+
if seenCmd != nil {
94+
t.Fatalf("unexpected command: %v", seenCmd.Args)
8195
}
8296

8397
// Now run again but provide a PR number
@@ -90,7 +104,11 @@ func TestPRView_NoActiveBranch(t *testing.T) {
90104
t.Errorf("command output expected got an empty string")
91105
}
92106

93-
if *callCount != 1 {
94-
t.Errorf("OpenInBrowser should be called once but was called %d time(s)", *callCount)
107+
if seenCmd == nil {
108+
t.Fatal("expected a command to run")
109+
}
110+
url := seenCmd.Args[len(seenCmd.Args)-1]
111+
if url != "https://github.com/OWNER/REPO/pull/23" {
112+
t.Errorf("got: %q", url)
95113
}
96114
}

‎command/testing.go‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ package command
33
import (
44
"github.com/github/gh-cli/api"
55
"github.com/github/gh-cli/context"
6-
"github.com/github/gh-cli/utils"
76
)
87

98
func initBlankContext(repo, branch string) {
@@ -23,17 +22,15 @@ func initFakeHTTP() *api.FakeHTTP {
2322
return http
2423
}
2524

26-
func mockOpenInBrowser() (func(), *int) {
27-
callCount := 0
28-
originalOpenInBrowser := utils.OpenInBrowser
29-
teardown := func() {
30-
utils.OpenInBrowser = originalOpenInBrowser
31-
}
25+
// outputStub implements a simple utils.Runnable
26+
type outputStub struct {
27+
output []byte
28+
}
3229

33-
utils.OpenInBrowser = func(_ string) error {
34-
callCount++
35-
return nil
36-
}
30+
func (s outputStub) Output() ([]byte, error) {
31+
return s.output, nil
32+
}
3733

38-
return teardown, &callCount
34+
func (s outputStub) Run() error {
35+
return nil
3936
}

‎git/git.go‎

Lines changed: 13 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,13 @@ import (
88
"os/exec"
99
"path/filepath"
1010
"strings"
11+
12+
"github.com/github/gh-cli/utils"
1113
)
1214

1315
func Dir() (string, error) {
1416
dirCmd := exec.Command("git", "rev-parse", "-q", "--git-dir")
15-
dirCmd.Stderr = nil
16-
output, err := dirCmd.Output()
17+
output, err := utils.PrepareCmd(dirCmd).Output()
1718
if err != nil {
1819
return "", fmt.Errorf("Not a git repository (or any of the parent directories): .git")
1920
}
@@ -33,7 +34,7 @@ func Dir() (string, error) {
3334
func WorkdirName() (string, error) {
3435
toplevelCmd := exec.Command("git", "rev-parse", "--show-toplevel")
3536
toplevelCmd.Stderr = nil
36-
output, err := toplevelCmd.Output()
37+
output, err := utils.PrepareCmd(toplevelCmd).Output()
3738
dir := firstLine(output)
3839
if dir == "" {
3940
return "", fmt.Errorf("unable to determine git working directory")
@@ -44,8 +45,7 @@ func WorkdirName() (string, error) {
4445
func HasFile(segments ...string) bool {
4546
// The blessed way to resolve paths within git dir since Git 2.5.0
4647
pathCmd := exec.Command("git", "rev-parse", "-q", "--git-path", filepath.Join(segments...))
47-
pathCmd.Stderr = nil
48-
if output, err := pathCmd.Output(); err == nil {
48+
if output, err := utils.PrepareCmd(pathCmd).Output(); err == nil {
4949
if lines := outputLines(output); len(lines) == 1 {
5050
if _, err := os.Stat(lines[0]); err == nil {
5151
return true
@@ -97,8 +97,7 @@ func BranchAtRef(paths ...string) (name string, err error) {
9797

9898
func Editor() (string, error) {
9999
varCmd := exec.Command("git", "var", "GIT_EDITOR")
100-
varCmd.Stderr = nil
101-
output, err := varCmd.Output()
100+
output, err := utils.PrepareCmd(varCmd).Output()
102101
if err != nil {
103102
return "", fmt.Errorf("Can't load git var: GIT_EDITOR")
104103
}
@@ -112,8 +111,7 @@ func Head() (string, error) {
112111

113112
func SymbolicFullName(name string) (string, error) {
114113
parseCmd := exec.Command("git", "rev-parse", "--symbolic-full-name", name)
115-
parseCmd.Stderr = nil
116-
output, err := parseCmd.Output()
114+
output, err := utils.PrepareCmd(parseCmd).Output()
117115
if err != nil {
118116
return "", fmt.Errorf("Unknown revision or path not in the working tree: %s", name)
119117
}
@@ -145,9 +143,7 @@ func CommentChar(text string) (string, error) {
145143

146144
func Show(sha string) (string, error) {
147145
cmd := exec.Command("git", "-c", "log.showSignature=false", "show", "-s", "--format=%s%n%+b", sha)
148-
cmd.Stderr = nil
149-
150-
output, err := cmd.Output()
146+
output, err := utils.PrepareCmd(cmd).Output()
151147
return strings.TrimSpace(string(output)), err
152148
}
153149

@@ -157,7 +153,7 @@ func Log(sha1, sha2 string) (string, error) {
157153
"-c", "log.showSignature=false", "log", "--no-color",
158154
"--format=%h (%aN, %ar)%n%w(78,3,3)%s%n%+b",
159155
"--cherry", shaRange)
160-
outputs, err := cmd.Output()
156+
outputs, err := utils.PrepareCmd(cmd).Output()
161157
if err != nil {
162158
return "", fmt.Errorf("Can't load git log %s..%s", sha1, sha2)
163159
}
@@ -167,14 +163,13 @@ func Log(sha1, sha2 string) (string, error) {
167163

168164
func listRemotes() ([]string, error) {
169165
remoteCmd := exec.Command("git", "remote", "-v")
170-
remoteCmd.Stderr = nil
171-
output, err := remoteCmd.Output()
166+
output, err := utils.PrepareCmd(remoteCmd).Output()
172167
return outputLines(output), err
173168
}
174169

175170
func Config(name string) (string, error) {
176171
configCmd := exec.Command("git", "config", name)
177-
output, err := configCmd.Output()
172+
output, err := utils.PrepareCmd(configCmd).Output()
178173
if err != nil {
179174
return "", fmt.Errorf("unknown config key: %s", name)
180175
}
@@ -190,21 +185,16 @@ func ConfigAll(name string) ([]string, error) {
190185
}
191186

192187
configCmd := exec.Command("git", "config", mode, name)
193-
output, err := configCmd.Output()
188+
output, err := utils.PrepareCmd(configCmd).Output()
194189
if err != nil {
195190
return nil, fmt.Errorf("Unknown config %s", name)
196191
}
197192
return outputLines(output), nil
198193
}
199194

200-
func Run(args ...string) error {
201-
cmd := exec.Command("git", args...)
202-
return cmd.Run()
203-
}
204-
205195
func LocalBranches() ([]string, error) {
206196
branchesCmd := exec.Command("git", "branch", "--list")
207-
output, err := branchesCmd.Output()
197+
output, err := utils.PrepareCmd(branchesCmd).Output()
208198
if err != nil {
209199
return nil, err
210200
}
@@ -218,9 +208,6 @@ func LocalBranches() ([]string, error) {
218208

219209
func outputLines(output []byte) []string {
220210
lines := strings.TrimSuffix(string(output), "\n")
221-
if lines == "" {
222-
return []string{}
223-
}
224211
return strings.Split(lines, "\n")
225212

226213
}

‎test/fixtures/prView.json‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
"node": {
77
"number": 10,
88
"title": "Blueberries are a good fruit",
9-
"url": "https://github.com/github/gh-cli/pull/10",
9+
"url": "https://github.com/OWNER/REPO/pull/10",
1010
"headRefName": "[blueberries]"
1111
}
1212
}
@@ -19,7 +19,7 @@
1919
"node": {
2020
"number": 8,
2121
"title": "Strawberries are not actually berries",
22-
"url": "https://github.com/github/gh-cli/pull/8",
22+
"url": "https://github.com/OWNER/REPO/pull/8",
2323
"headRefName": "[strawberries]"
2424
}
2525
}
@@ -32,15 +32,15 @@
3232
"node": {
3333
"number": 9,
3434
"title": "Apples are tasty",
35-
"url": "https://github.com/github/gh-cli/pull/9",
35+
"url": "https://github.com/OWNER/REPO/pull/9",
3636
"headRefName": "[apples]"
3737
}
3838
},
3939
{
4040
"node": {
4141
"number": 11,
4242
"title": "Figs are my favorite",
43-
"url": "https://github.com/github/gh-cli/pull/1",
43+
"url": "https://github.com/OWNER/REPO/pull/1",
4444
"headRefName": "[figs]"
4545
}
4646
}

‎utils/prepare_cmd.go‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
package utils
2+
3+
import (
4+
"bytes"
5+
"fmt"
6+
"os"
7+
"os/exec"
8+
"strings"
9+
)
10+
11+
// Runnable is typically an exec.Cmd or its stub in tests
12+
type Runnable interface {
13+
Output() ([]byte, error)
14+
Run() error
15+
}
16+
17+
// PrepareCmd extends exec.Cmd with extra error reporting features and provides a
18+
// hook to stub command execution in tests
19+
var PrepareCmd = func(cmd *exec.Cmd) Runnable {
20+
return &cmdWithStderr{cmd}
21+
}
22+
23+
// SetPrepareCmd overrides PrepareCmd and returns a func to revert it back
24+
func SetPrepareCmd(fn func(*exec.Cmd) Runnable) func() {
25+
origPrepare := PrepareCmd
26+
PrepareCmd = fn
27+
return func() {
28+
PrepareCmd = origPrepare
29+
}
30+
}
31+
32+
// cmdWithStderr augments exec.Cmd by adding stderr to the error message
33+
type cmdWithStderr struct {
34+
*exec.Cmd
35+
}
36+
37+
func (c cmdWithStderr) Output() ([]byte, error) {
38+
if os.Getenv("DEBUG") != "" {
39+
fmt.Fprintf(os.Stderr, "%v\n", c.Cmd.Args)
40+
}
41+
errStream := &bytes.Buffer{}
42+
c.Cmd.Stderr = errStream
43+
out, err := c.Cmd.Output()
44+
if err != nil {
45+
err = &CmdError{errStream, c.Cmd.Args, err}
46+
}
47+
return out, err
48+
}
49+
50+
func (c cmdWithStderr) Run() error {
51+
if os.Getenv("DEBUG") != "" {
52+
fmt.Fprintf(os.Stderr, "%v\n", c.Cmd.Args)
53+
}
54+
errStream := &bytes.Buffer{}
55+
c.Cmd.Stderr = errStream
56+
err := c.Cmd.Run()
57+
if err != nil {
58+
err = &CmdError{errStream, c.Cmd.Args, err}
59+
}
60+
return err
61+
}
62+
63+
// CmdError provides more visibility into why an exec.Cmd had failed
64+
type CmdError struct {
65+
Stderr *bytes.Buffer
66+
Args []string
67+
Err error
68+
}
69+
70+
func (e CmdError) Error() string {
71+
msg := e.Stderr.String()
72+
if msg != "" && !strings.HasSuffix(msg, "\n") {
73+
msg += "\n"
74+
}
75+
return fmt.Sprintf("%s%s: %s", msg, e.Args[0], e.Err)
76+
}

0 commit comments

Comments
 (0)