code review changes

core/commands/cmdenv/env.go

@@ -72,10 +72,11 @@ func GetConfigRoot(env cmds.Environment) (string, error) {
 	return ctx.ConfigRoot, nil
 }
 
-// EscNonPrint converts non-printable characters and backslash into Go
-// escape sequences, if the given string contains any.
+// EscNonPrint converts non-printable characters and backslash into Go escape
+// sequences.  This is done to display all characters in a string, including
+// those that would otherwise not be displayed or have an undesirable effect on
+// the display.
 func EscNonPrint(s string) string {
-	// First see if escaping is needed, to avoid creating garbage.
 	if !needEscape(s) {
 		return s
 	}

core/commands/cmdenv/env_test.go

@@ -0,0 +1,48 @@
+package cmdenv
+
+import (
+	"strconv"
+	"testing"
+)
+
+func TestEscNonPrint(t *testing.T) {
+	b := []byte("hello")
+	b[2] = 0x7f
+	s := string(b)
+	if !needEscape(s) {
+		t.Fatal("string needs escaping")
+	}
+	if !hasNonPrintable(s) {
+		t.Fatal("expected non-printable")
+	}
+	if hasNonPrintable(EscNonPrint(s)) {
+		t.Fatal("escaped string has non-printable")
+	}
+	if EscNonPrint(`hel\lo`) != `hel\\lo` {
+		t.Fatal("backslash not escaped")
+	}
+
+	s = `hello`
+	if needEscape(s) {
+		t.Fatal("string does not need escaping")
+	}
+	if EscNonPrint(s) != s {
+		t.Fatal("string should not have changed")
+	}
+	s = `"hello"`
+	if EscNonPrint(s) != s {
+		t.Fatal("string should not have changed")
+	}
+	if EscNonPrint(`"hel\"lo"`) != `"hel\\"lo"` {
+		t.Fatal("did not get expected escaped string")
+	}
+}
+
+func hasNonPrintable(s string) bool {
+	for _, r := range s {
+		if !strconv.IsPrint(r) {
+			return true
+		}
+	}
+	return false
+}