Repository: mvdan/sh
Pull request #388 (2019-06-12 15:09:58)

Support color output for the added/deleted lines in diff mode
This PR supports color output for the added/deleted lines in diff mode.

Current:

<img width="607" alt="Screen Shot 2019-06-13 at 12 07 31 AM" src="https://user-images.githubusercontent.com/6178510/59363004-70828900-8d6f-11e9-9bb9-4322c48714ee.png">

When PR is merged:

<img width="605" alt="Screen Shot 2019-06-13 at 12 07 09 AM" src="https://user-images.githubusercontent.com/6178510/59363030-7aa48780-8d6f-11e9-9adb-14c9762c06f9.png">


mvdan: Thanks for the PR. However, I disagree with always using colored output. Tools like `diff` will only do so when stdout is a terminal, for example. Printing color codes always isn't a good idea; for example, some people avoid them via `TERM=dumb`.

I'll take a look at an alternative solution.
mingrammer: Thank you for feedback. So how about using [fatih/color](https://github.com/fatih/color)? This package works well even if `TERM=dumb` set. (If set, it does not add the color codes around the text) See [this](https://github.com/fatih/color/blob/master/color.go#L20)

As you see, the author of this library has stopped maintaining, but it is already "done". (no need further maintenance)
mvdan: I'm still not convinced. That would be an extra dependency, which itself also pulls in another two dependencies, all just so we could have a bit of logic to color a few lines.

But I do see the use of colored diffs. I'm ok with merging something like this, as long as colors are only enabled when:

* `TERM` is not `dumb`
* `out` is an `*os.File`, and `IsTerminal` returns true

I'll also leave a code review shortly.
mingrammer: I fixed codes by reflecting the reviews.

Patch:
diff --git a/cmd/shfmt/main.go b/cmd/shfmt/main.go
index 46386a36..05c6aecf 100644
--- a/cmd/shfmt/main.go
+++ b/cmd/shfmt/main.go
@@ -14,6 +14,7 @@ import (
 	"path/filepath"
 	"regexp"
 
+	"golang.org/x/crypto/ssh/terminal"
 	"mvdan.cc/sh/v3/fileutil"
 	"mvdan.cc/sh/v3/syntax"
 )
@@ -48,9 +49,26 @@ var (
 	in  io.Reader = os.Stdin
 	out io.Writer = os.Stdout
 
+	color       bool
+	ansiFgRed   = "\u001b[31m"
+	ansiFgGreen = "\u001b[32m"
+	ansiReset   = "\u001b[0m"
+
 	version = "v3.0.0-alpha1"
 )
 
+func init() {
+	if term, ok := os.LookupEnv("TERM"); ok && term == "dumb" {
+		color = false
+		return
+	}
+	if f, ok := out.(*os.File); ok && terminal.IsTerminal(int(f.Fd())) {
+		color = true
+		return
+	}
+	color = false
+}
+
 func main() {
 	flag.Usage = func() {
 		fmt.Fprint(os.Stderr, `usage: shfmt [flags] [path ...]
@@ -322,16 +340,35 @@ func diffBytes(b1, b2 []byte, path string) ([]byte, error) {
 		// as diff will return non-zero if the files differ.
 		return nil, err
 	}
+
+	output := new(bytes.Buffer)
 	// We already print the filename, so remove the
 	// temporary filenames printed by diff.
 	lines := bytes.Split(data, []byte("\n"))
+	count := len(lines)
 	for i, line := range lines {
 		switch {
 		case bytes.HasPrefix(line, []byte("---")):
 		case bytes.HasPrefix(line, []byte("+++")):
+		case bytes.HasPrefix(line, []byte("-")):
+			if color {
+				fmt.Fprintf(output, "%s%s%s\n", ansiFgRed, string(line), ansiReset)
+			} else {
+				fmt.Fprintln(output, string(line))
+			}
+		case bytes.HasPrefix(line, []byte("+")):
+			if color {
+				fmt.Fprintf(output, "%s%s%s\n", ansiFgGreen, string(line), ansiReset)
+			} else {
+				fmt.Fprintln(output, string(line))
+			}
 		default:
-			return bytes.Join(lines[i:], []byte("\n")), nil
+			if i < count-1 {
+				fmt.Fprintln(output, string(line))
+			} else {
+				fmt.Fprint(output, string(line))
+			}
 		}
 	}
-	return data, nil
+	return output.Bytes(), nil
 }


Test patch:
diff --git a/cmd/shfmt/main_test.go b/cmd/shfmt/main_test.go
index 4d76ac04..b2a84355 100644
--- a/cmd/shfmt/main_test.go
+++ b/cmd/shfmt/main_test.go
@@ -71,6 +71,24 @@ func TestStdin(t *testing.T) {
 			t.Fatalf("got:\n%swant:\n%s", got, want)
 		}
 	})
+
+	t.Run("DiffColored", func(t *testing.T) {
+		if _, err := exec.LookPath("diff"); err != nil {
+			t.Skip("skipping as the diff tool is not available")
+		}
+		*diff = true
+		color = true
+		defer func() { *diff = false; color = false }()
+		in = strings.NewReader(" foo\nbar\n\n")
+		buf.Reset()
+		if err := formatStdin(); err != errChangedWithDiff {
+			t.Fatalf("got=%q want=%q", err, errChangedWithDiff)
+		}
+		want := "diff -u <standard input>.orig <standard input>\n@@ -1,3 +1,2 @@\n\x1b[31m- foo\x1b[0m\n\x1b[32m+foo\x1b[0m\n bar\n\x1b[31m-\x1b[0m\n"
+		if got := buf.String(); got != want {
+			t.Fatalf("got:\n%swant:\n%s", got, want)
+		}
+	})
 }
 
 type action uint