Skip to content

Commit 6906dea

Browse files
author
Nate Smith
authored
Merge pull request cli#4833 from cli/diff-color
pr diff color output fixes
2 parents c6821b6 + c33eb3b commit 6906dea

File tree

2 files changed

+245
-161
lines changed

2 files changed

+245
-161
lines changed

‎pkg/cmd/pr/diff/diff.go‎

Lines changed: 83 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ type DiffOptions struct {
2626
Finder shared.PRFinder
2727

2828
SelectorArg string
29-
UseColor string
29+
UseColor bool
3030
Patch bool
3131
}
3232

@@ -36,6 +36,8 @@ func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Comman
3636
HttpClient: f.HttpClient,
3737
}
3838

39+
var colorFlag string
40+
3941
cmd := &cobra.Command{
4042
Use: "diff [<number> | <url> | <branch>]",
4143
Short: "View changes in a pull request",
@@ -50,19 +52,22 @@ func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Comman
5052
opts.Finder = shared.NewFinder(f)
5153

5254
if repoOverride, _ := cmd.Flags().GetString("repo"); repoOverride != "" && len(args) == 0 {
53-
return cmdutil.FlagErrorf("argument required when using the --repo flag")
55+
return cmdutil.FlagErrorf("argument required when using the `--repo` flag")
5456
}
5557

5658
if len(args) > 0 {
5759
opts.SelectorArg = args[0]
5860
}
5961

60-
if !validColorFlag(opts.UseColor) {
61-
return cmdutil.FlagErrorf("did not understand color: %q. Expected one of always, never, or auto", opts.UseColor)
62-
}
63-
64-
if opts.UseColor == "auto" && !opts.IO.IsStdoutTTY() {
65-
opts.UseColor = "never"
62+
switch colorFlag {
63+
case "always":
64+
opts.UseColor = true
65+
case "auto":
66+
opts.UseColor = opts.IO.ColorEnabled()
67+
case "never":
68+
opts.UseColor = false
69+
default:
70+
return cmdutil.FlagErrorf("the value for `--color` must be one of \"auto\", \"always\", or \"never\"")
6671
}
6772

6873
if runF != nil {
@@ -72,7 +77,7 @@ func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Comman
7277
},
7378
}
7479

75-
cmd.Flags().StringVar(&opts.UseColor, "color", "auto", "Use color in diff output: {always|never|auto}")
80+
cmd.Flags().StringVar(&colorFlag, "color", "auto", "Use color in diff output: {always|never|auto}")
7681
cmd.Flags().BoolVar(&opts.Patch, "patch", false, "Display diff in patch format")
7782

7883
return cmd
@@ -105,34 +110,15 @@ func diffRun(opts *DiffOptions) error {
105110
}
106111
defer opts.IO.StopPager()
107112

108-
if opts.UseColor == "never" {
113+
if !opts.UseColor {
109114
_, err = io.Copy(opts.IO.Out, diff)
110115
if errors.Is(err, syscall.EPIPE) {
111116
return nil
112117
}
113118
return err
114119
}
115120

116-
diffLines := bufio.NewScanner(diff)
117-
for diffLines.Scan() {
118-
diffLine := diffLines.Text()
119-
switch {
120-
case isHeaderLine(diffLine):
121-
fmt.Fprintf(opts.IO.Out, "\x1b[1;38m%s\x1b[m\n", diffLine)
122-
case isAdditionLine(diffLine):
123-
fmt.Fprintf(opts.IO.Out, "\x1b[32m%s\x1b[m\n", diffLine)
124-
case isRemovalLine(diffLine):
125-
fmt.Fprintf(opts.IO.Out, "\x1b[31m%s\x1b[m\n", diffLine)
126-
default:
127-
fmt.Fprintln(opts.IO.Out, diffLine)
128-
}
129-
}
130-
131-
if err := diffLines.Err(); err != nil {
132-
return fmt.Errorf("error reading pull request diff: %w", err)
133-
}
134-
135-
return nil
121+
return colorDiffLines(opts.IO.Out, diff)
136122
}
137123

138124
func fetchDiff(httpClient *http.Client, baseRepo ghrepo.Interface, prNumber int, asPatch bool) (io.ReadCloser, error) {
@@ -165,9 +151,71 @@ func fetchDiff(httpClient *http.Client, baseRepo ghrepo.Interface, prNumber int,
165151
return resp.Body, nil
166152
}
167153

154+
const lineBufferSize = 4096
155+
156+
var (
157+
colorHeader = []byte("\x1b[1;38m")
158+
colorAddition = []byte("\x1b[32m")
159+
colorRemoval = []byte("\x1b[31m")
160+
colorReset = []byte("\x1b[m")
161+
)
162+
163+
func colorDiffLines(w io.Writer, r io.Reader) error {
164+
diffLines := bufio.NewReaderSize(r, lineBufferSize)
165+
wasPrefix := false
166+
needsReset := false
167+
168+
for {
169+
diffLine, isPrefix, err := diffLines.ReadLine()
170+
if err != nil {
171+
if errors.Is(err, io.EOF) {
172+
break
173+
}
174+
return fmt.Errorf("error reading pull request diff: %w", err)
175+
}
176+
177+
var color []byte
178+
if !wasPrefix {
179+
if isHeaderLine(diffLine) {
180+
color = colorHeader
181+
} else if isAdditionLine(diffLine) {
182+
color = colorAddition
183+
} else if isRemovalLine(diffLine) {
184+
color = colorRemoval
185+
}
186+
}
187+
188+
if color != nil {
189+
if _, err := w.Write(color); err != nil {
190+
return err
191+
}
192+
needsReset = true
193+
}
194+
195+
if _, err := w.Write(diffLine); err != nil {
196+
return err
197+
}
198+
199+
if !isPrefix {
200+
if needsReset {
201+
if _, err := w.Write(colorReset); err != nil {
202+
return err
203+
}
204+
needsReset = false
205+
}
206+
if _, err := w.Write([]byte{'\n'}); err != nil {
207+
return err
208+
}
209+
}
210+
wasPrefix = isPrefix
211+
}
212+
return nil
213+
}
214+
168215
var diffHeaderPrefixes = []string{"+++", "---", "diff", "index"}
169216

170-
func isHeaderLine(dl string) bool {
217+
func isHeaderLine(l []byte) bool {
218+
dl := string(l)
171219
for _, p := range diffHeaderPrefixes {
172220
if strings.HasPrefix(dl, p) {
173221
return true
@@ -176,14 +224,10 @@ func isHeaderLine(dl string) bool {
176224
return false
177225
}
178226

179-
func isAdditionLine(dl string) bool {
180-
return strings.HasPrefix(dl, "+")
181-
}
182-
183-
func isRemovalLine(dl string) bool {
184-
return strings.HasPrefix(dl, "-")
227+
func isAdditionLine(l []byte) bool {
228+
return len(l) > 0 && l[0] == '+'
185229
}
186230

187-
func validColorFlag(c string) bool {
188-
return c == "auto" || c == "always" || c == "never"
231+
func isRemovalLine(l []byte) bool {
232+
return len(l) > 0 && l[0] == '-'
189233
}

0 commit comments

Comments
 (0)