Merge pull request #4833 from cli/diff-color

pr diff color output fixes
This commit is contained in:
Nate Smith 2021-12-01 10:48:31 -06:00 committed by GitHub
commit 6906dea671
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 245 additions and 161 deletions

View file

@ -26,7 +26,7 @@ type DiffOptions struct {
Finder shared.PRFinder
SelectorArg string
UseColor string
UseColor bool
Patch bool
}
@ -36,6 +36,8 @@ func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Comman
HttpClient: f.HttpClient,
}
var colorFlag string
cmd := &cobra.Command{
Use: "diff [<number> | <url> | <branch>]",
Short: "View changes in a pull request",
@ -50,19 +52,22 @@ func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Comman
opts.Finder = shared.NewFinder(f)
if repoOverride, _ := cmd.Flags().GetString("repo"); repoOverride != "" && len(args) == 0 {
return cmdutil.FlagErrorf("argument required when using the --repo flag")
return cmdutil.FlagErrorf("argument required when using the `--repo` flag")
}
if len(args) > 0 {
opts.SelectorArg = args[0]
}
if !validColorFlag(opts.UseColor) {
return cmdutil.FlagErrorf("did not understand color: %q. Expected one of always, never, or auto", opts.UseColor)
}
if opts.UseColor == "auto" && !opts.IO.IsStdoutTTY() {
opts.UseColor = "never"
switch colorFlag {
case "always":
opts.UseColor = true
case "auto":
opts.UseColor = opts.IO.ColorEnabled()
case "never":
opts.UseColor = false
default:
return cmdutil.FlagErrorf("the value for `--color` must be one of \"auto\", \"always\", or \"never\"")
}
if runF != nil {
@ -72,7 +77,7 @@ func NewCmdDiff(f *cmdutil.Factory, runF func(*DiffOptions) error) *cobra.Comman
},
}
cmd.Flags().StringVar(&opts.UseColor, "color", "auto", "Use color in diff output: {always|never|auto}")
cmd.Flags().StringVar(&colorFlag, "color", "auto", "Use color in diff output: {always|never|auto}")
cmd.Flags().BoolVar(&opts.Patch, "patch", false, "Display diff in patch format")
return cmd
@ -105,7 +110,7 @@ func diffRun(opts *DiffOptions) error {
}
defer opts.IO.StopPager()
if opts.UseColor == "never" {
if !opts.UseColor {
_, err = io.Copy(opts.IO.Out, diff)
if errors.Is(err, syscall.EPIPE) {
return nil
@ -113,26 +118,7 @@ func diffRun(opts *DiffOptions) error {
return err
}
diffLines := bufio.NewScanner(diff)
for diffLines.Scan() {
diffLine := diffLines.Text()
switch {
case isHeaderLine(diffLine):
fmt.Fprintf(opts.IO.Out, "\x1b[1;38m%s\x1b[m\n", diffLine)
case isAdditionLine(diffLine):
fmt.Fprintf(opts.IO.Out, "\x1b[32m%s\x1b[m\n", diffLine)
case isRemovalLine(diffLine):
fmt.Fprintf(opts.IO.Out, "\x1b[31m%s\x1b[m\n", diffLine)
default:
fmt.Fprintln(opts.IO.Out, diffLine)
}
}
if err := diffLines.Err(); err != nil {
return fmt.Errorf("error reading pull request diff: %w", err)
}
return nil
return colorDiffLines(opts.IO.Out, diff)
}
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,
return resp.Body, nil
}
const lineBufferSize = 4096
var (
colorHeader = []byte("\x1b[1;38m")
colorAddition = []byte("\x1b[32m")
colorRemoval = []byte("\x1b[31m")
colorReset = []byte("\x1b[m")
)
func colorDiffLines(w io.Writer, r io.Reader) error {
diffLines := bufio.NewReaderSize(r, lineBufferSize)
wasPrefix := false
needsReset := false
for {
diffLine, isPrefix, err := diffLines.ReadLine()
if err != nil {
if errors.Is(err, io.EOF) {
break
}
return fmt.Errorf("error reading pull request diff: %w", err)
}
var color []byte
if !wasPrefix {
if isHeaderLine(diffLine) {
color = colorHeader
} else if isAdditionLine(diffLine) {
color = colorAddition
} else if isRemovalLine(diffLine) {
color = colorRemoval
}
}
if color != nil {
if _, err := w.Write(color); err != nil {
return err
}
needsReset = true
}
if _, err := w.Write(diffLine); err != nil {
return err
}
if !isPrefix {
if needsReset {
if _, err := w.Write(colorReset); err != nil {
return err
}
needsReset = false
}
if _, err := w.Write([]byte{'\n'}); err != nil {
return err
}
}
wasPrefix = isPrefix
}
return nil
}
var diffHeaderPrefixes = []string{"+++", "---", "diff", "index"}
func isHeaderLine(dl string) bool {
func isHeaderLine(l []byte) bool {
dl := string(l)
for _, p := range diffHeaderPrefixes {
if strings.HasPrefix(dl, p) {
return true
@ -176,14 +224,10 @@ func isHeaderLine(dl string) bool {
return false
}
func isAdditionLine(dl string) bool {
return strings.HasPrefix(dl, "+")
func isAdditionLine(l []byte) bool {
return len(l) > 0 && l[0] == '+'
}
func isRemovalLine(dl string) bool {
return strings.HasPrefix(dl, "-")
}
func validColorFlag(c string) bool {
return c == "auto" || c == "always" || c == "never"
func isRemovalLine(l []byte) bool {
return len(l) > 0 && l[0] == '-'
}

View file

@ -2,19 +2,18 @@ package diff
import (
"bytes"
"fmt"
"io/ioutil"
"net/http"
"strings"
"testing"
"github.com/cli/cli/v2/api"
"github.com/cli/cli/v2/context"
"github.com/cli/cli/v2/internal/ghrepo"
"github.com/cli/cli/v2/pkg/cmd/pr/shared"
"github.com/cli/cli/v2/pkg/cmdutil"
"github.com/cli/cli/v2/pkg/httpmock"
"github.com/cli/cli/v2/pkg/iostreams"
"github.com/cli/cli/v2/test"
"github.com/google/go-cmp/cmp"
"github.com/google/shlex"
"github.com/stretchr/testify/assert"
@ -35,7 +34,7 @@ func Test_NewCmdDiff(t *testing.T) {
isTTY: true,
want: DiffOptions{
SelectorArg: "123",
UseColor: "auto",
UseColor: true,
},
},
{
@ -44,7 +43,7 @@ func Test_NewCmdDiff(t *testing.T) {
isTTY: true,
want: DiffOptions{
SelectorArg: "",
UseColor: "auto",
UseColor: true,
},
},
{
@ -53,20 +52,38 @@ func Test_NewCmdDiff(t *testing.T) {
isTTY: false,
want: DiffOptions{
SelectorArg: "",
UseColor: "never",
UseColor: false,
},
},
{
name: "force color",
args: "--color always",
isTTY: false,
want: DiffOptions{
SelectorArg: "",
UseColor: true,
},
},
{
name: "disable color",
args: "--color never",
isTTY: true,
want: DiffOptions{
SelectorArg: "",
UseColor: false,
},
},
{
name: "no argument with --repo override",
args: "-R owner/repo",
isTTY: true,
wantErr: "argument required when using the --repo flag",
wantErr: "argument required when using the `--repo` flag",
},
{
name: "invalid --color argument",
args: "--color doublerainbow",
isTTY: true,
wantErr: `did not understand color: "doublerainbow". Expected one of always, never, or auto`,
wantErr: "the value for `--color` must be one of \"auto\", \"always\", or \"never\"",
},
}
for _, tt := range tests {
@ -75,6 +92,7 @@ func Test_NewCmdDiff(t *testing.T) {
io.SetStdoutTTY(tt.isTTY)
io.SetStdinTTY(tt.isTTY)
io.SetStderrTTY(tt.isTTY)
io.SetColorEnabled(tt.isTTY)
f := &cmdutil.Factory{
IOStreams: io,
@ -109,145 +127,167 @@ func Test_NewCmdDiff(t *testing.T) {
}
}
func runCommand(rt http.RoundTripper, remotes context.Remotes, isTTY bool, cli string) (*test.CmdOut, error) {
io, _, stdout, stderr := iostreams.Test()
io.SetStdoutTTY(isTTY)
io.SetStdinTTY(isTTY)
io.SetStderrTTY(isTTY)
func Test_diffRun(t *testing.T) {
pr := &api.PullRequest{Number: 123}
factory := &cmdutil.Factory{
IOStreams: io,
HttpClient: func() (*http.Client, error) {
return &http.Client{Transport: rt}, nil
tests := []struct {
name string
opts DiffOptions
rawDiff string
wantAccept string
wantStdout string
}{
{
name: "no color",
opts: DiffOptions{
SelectorArg: "123",
UseColor: false,
Patch: false,
},
rawDiff: fmt.Sprintf(testDiff, "", "", "", ""),
wantAccept: "application/vnd.github.v3.diff",
wantStdout: fmt.Sprintf(testDiff, "", "", "", ""),
},
{
name: "with color",
opts: DiffOptions{
SelectorArg: "123",
UseColor: true,
Patch: false,
},
rawDiff: fmt.Sprintf(testDiff, "", "", "", ""),
wantAccept: "application/vnd.github.v3.diff",
wantStdout: fmt.Sprintf(testDiff, "\x1b[m", "\x1b[1;38m", "\x1b[32m", "\x1b[31m"),
},
{
name: "patch format",
opts: DiffOptions{
SelectorArg: "123",
UseColor: false,
Patch: true,
},
rawDiff: fmt.Sprintf(testDiff, "", "", "", ""),
wantAccept: "application/vnd.github.v3.patch",
wantStdout: fmt.Sprintf(testDiff, "", "", "", ""),
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
httpReg := &httpmock.Registry{}
defer httpReg.Verify(t)
cmd := NewCmdDiff(factory, nil)
var gotAccept string
httpReg.Register(
httpmock.REST("GET", "repos/OWNER/REPO/pulls/123"),
func(req *http.Request) (*http.Response, error) {
gotAccept = req.Header.Get("Accept")
return &http.Response{
StatusCode: 200,
Request: req,
Body: ioutil.NopCloser(strings.NewReader(tt.rawDiff)),
}, nil
})
argv, err := shlex.Split(cli)
if err != nil {
return nil, err
}
cmd.SetArgs(argv)
opts := tt.opts
opts.HttpClient = func() (*http.Client, error) {
return &http.Client{Transport: httpReg}, nil
}
cmd.SetIn(&bytes.Buffer{})
cmd.SetOut(ioutil.Discard)
cmd.SetErr(ioutil.Discard)
io, _, stdout, stderr := iostreams.Test()
opts.IO = io
_, err = cmd.ExecuteC()
return &test.CmdOut{
OutBuf: stdout,
ErrBuf: stderr,
}, err
}
finder := shared.NewMockFinder("123", pr, ghrepo.New("OWNER", "REPO"))
finder.ExpectFields([]string{"number"})
opts.Finder = finder
func TestPRDiff_notty_diff(t *testing.T) {
httpReg := &httpmock.Registry{}
defer httpReg.Verify(t)
shared.RunCommandFinder("", &api.PullRequest{Number: 123}, ghrepo.New("OWNER", "REPO"))
var gotAccept string
httpReg.Register(
httpmock.REST("GET", "repos/OWNER/REPO/pulls/123"),
func(req *http.Request) (*http.Response, error) {
gotAccept = req.Header.Get("Accept")
return &http.Response{
StatusCode: 200,
Request: req,
Body: ioutil.NopCloser(strings.NewReader(testDiff)),
}, nil
if err := diffRun(&opts); err != nil {
t.Fatalf("unexpected error: %s", err)
}
if diff := cmp.Diff(tt.wantStdout, stdout.String()); diff != "" {
t.Errorf("command output did not match:\n%s", diff)
}
if stderr.String() != "" {
t.Errorf("unexpected stderr output: %s", stderr.String())
}
if gotAccept != tt.wantAccept {
t.Errorf("unexpected Accept header: %s", gotAccept)
}
})
output, err := runCommand(httpReg, nil, false, "")
if err != nil {
t.Fatalf("unexpected error: %s", err)
}
if diff := cmp.Diff(testDiff, output.String()); diff != "" {
t.Errorf("command output did not match:\n%s", diff)
}
if gotAccept != "application/vnd.github.v3.diff" {
t.Errorf("unexpected Accept header: %s", gotAccept)
}
}
func TestPRDiff_notty_patch(t *testing.T) {
httpReg := &httpmock.Registry{}
defer httpReg.Verify(t)
shared.RunCommandFinder("", &api.PullRequest{Number: 123}, ghrepo.New("OWNER", "REPO"))
var gotAccept string
httpReg.Register(
httpmock.REST("GET", "repos/OWNER/REPO/pulls/123"),
func(req *http.Request) (*http.Response, error) {
gotAccept = req.Header.Get("Accept")
return &http.Response{
StatusCode: 200,
Request: req,
Body: ioutil.NopCloser(strings.NewReader(testDiff)),
}, nil
})
output, err := runCommand(httpReg, nil, false, "--patch")
if err != nil {
t.Fatalf("unexpected error: %s", err)
}
if diff := cmp.Diff(testDiff, output.String()); diff != "" {
t.Errorf("command output did not match:\n%s", diff)
}
if gotAccept != "application/vnd.github.v3.patch" {
t.Errorf("unexpected Accept header: %s", gotAccept)
}
}
func TestPRDiff_tty_diff(t *testing.T) {
http := &httpmock.Registry{}
defer http.Verify(t)
shared.RunCommandFinder("123", &api.PullRequest{Number: 123}, ghrepo.New("OWNER", "REPO"))
http.Register(
httpmock.REST("GET", "repos/OWNER/REPO/pulls/123"),
httpmock.StringResponse(testDiff),
)
output, err := runCommand(http, nil, true, "123")
if err != nil {
t.Fatalf("unexpected error: %s", err)
}
assert.Contains(t, output.String(), "\x1b[32m+site: bin/gh\x1b[m")
}
const testDiff = `diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml
index 73974448..b7fc0154 100644
--- a/.github/workflows/releases.yml
+++ b/.github/workflows/releases.yml
const testDiff = `%[2]sdiff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml%[1]s
%[2]sindex 73974448..b7fc0154 100644%[1]s
%[2]s--- a/.github/workflows/releases.yml%[1]s
%[2]s+++ b/.github/workflows/releases.yml%[1]s
@@ -44,6 +44,11 @@ jobs:
token: ${{secrets.SITE_GITHUB_TOKEN}}
- name: Publish documentation site
if: "!contains(github.ref, '-')" # skip prereleases
+ env:
+ GIT_COMMITTER_NAME: cli automation
+ GIT_AUTHOR_NAME: cli automation
+ GIT_COMMITTER_EMAIL: noreply@github.com
+ GIT_AUTHOR_EMAIL: noreply@github.com
%[3]s+ env:%[1]s
%[3]s+ GIT_COMMITTER_NAME: cli automation%[1]s
%[3]s+ GIT_AUTHOR_NAME: cli automation%[1]s
%[3]s+ GIT_COMMITTER_EMAIL: noreply@github.com%[1]s
%[3]s+ GIT_AUTHOR_EMAIL: noreply@github.com%[1]s
run: make site-publish
- name: Move project cards
if: "!contains(github.ref, '-')" # skip prereleases
diff --git a/Makefile b/Makefile
index f2b4805c..3d7bd0f9 100644
--- a/Makefile
+++ b/Makefile
%[2]sdiff --git a/Makefile b/Makefile%[1]s
%[2]sindex f2b4805c..3d7bd0f9 100644%[1]s
%[2]s--- a/Makefile%[1]s
%[2]s+++ b/Makefile%[1]s
@@ -22,8 +22,8 @@ test:
go test ./...
.PHONY: test
-site:
- git clone https://github.com/github/cli.github.com.git "$@"
+site: bin/gh
+ bin/gh repo clone github/cli.github.com "$@"
%[4]s-site:%[1]s
%[4]s- git clone https://github.com/github/cli.github.com.git "$@"%[1]s
%[3]s+site: bin/gh%[1]s
%[3]s+ bin/gh repo clone github/cli.github.com "$@"%[1]s
site-docs: site
git -C site pull
`
func Test_colorDiffLines(t *testing.T) {
inputs := []struct {
input, output string
}{
{
input: "",
output: "",
},
{
input: "\n",
output: "\n",
},
{
input: "foo\nbar\nbaz\n",
output: "foo\nbar\nbaz\n",
},
{
input: "foo\nbar\nbaz",
output: "foo\nbar\nbaz\n",
},
{
input: fmt.Sprintf("+foo\n-b%sr\n+++ baz\n", strings.Repeat("a", 2*lineBufferSize)),
output: fmt.Sprintf(
"%[4]s+foo%[2]s\n%[5]s-b%[1]sr%[2]s\n%[3]s+++ baz%[2]s\n",
strings.Repeat("a", 2*lineBufferSize),
"\x1b[m",
"\x1b[1;38m",
"\x1b[32m",
"\x1b[31m",
),
},
}
for _, tt := range inputs {
buf := bytes.Buffer{}
if err := colorDiffLines(&buf, strings.NewReader(tt.input)); err != nil {
t.Fatalf("unexpected error: %s", err)
}
if got := buf.String(); got != tt.output {
t.Errorf("expected: %q, got: %q", tt.output, got)
}
}
}