color_parse_mem: allow empty color spec

Prior to c2f41bf52 (color.c: fix color_parse_mem() with
value_len == 0, 2017-01-19), the empty string was
interpreted as a color "reset". This was an accidental
outcome, and that commit turned it into an error.

However, scripts may pass the empty string as a default
value to "git config --get-color" to disable color when the
value is not defined. The git-add--interactive script does
this. As a result, the script is unusable since c2f41bf52
unless you have color.diff.plain defined (if it is defined,
then we don't parse the empty default at all).

Our test scripts didn't notice the recent breakage because
they run without a terminal, and thus without color. They
never hit this code path at all. And nobody noticed the
original buggy "reset" behavior, because it was effectively
a noop.

Let's fix the code to have an empty color name produce an
empty sequence of color codes. The tests need a few fixups:

  - we'll add a new test in t4026 to cover this case. But
    note that we need to tweak the color() helper. While
    we're there, let's factor out the literal ANSI ESC
    character. Otherwise it makes the diff quite hard to
    read.

  - we'll add a basic sanity-check in t4026 that "git add
    -p" works at all when color is enabled. That would have
    caught this bug, as well as any others that are specific
    to the color code paths.

  - 73c727d69 (log --graph: customize the graph lines with
    config log.graphColors, 2017-01-19) added a test to
    t4202 that checks some "invalid" graph color config.
    Since ",, blue" before yielded only "blue" as valid, and
    now yields "empty, empty, blue", we don't match the
    expected output.

    One way to fix this would be to change the expectation
    to the empty color strings. But that makes the test much
    less interesting, since we show only two graph lines,
    both of which would be colorless.

    Since the empty-string case is now covered by t4026,
    let's remove them entirely here. They're just in the way
    of the primary thing the test is supposed to be
    checking.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/color.c b/color.c
index 7bb4a96f8c1dfb0b5769829a133cd8a86d8beee6..2925a819b219453a5f5be974c034babccdfa8417 100644 (file)
--- a/color.c
+++ b/color.c
@@ -212,8 +212,10 @@ int color_parse_mem(const char *value, int value_len, char *dst)
                len--;
        }
 
-       if (!len)
-               return -1;
+       if (!len) {
+               dst[0] = '\0';
+               return 0;
+       }
 
        if (!strncasecmp(ptr, "reset", len)) {
                xsnprintf(dst, end - dst, GIT_COLOR_RESET);

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index deae948c76bd12f9b3ef6b54568717b21d641d8a..5ffe78e920c4aa292a6c16e2aa8078ede4ab065c 100755 (executable)
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -380,4 +380,18 @@ test_expect_success 'patch mode ignores unmerged entries' '
        test_cmp expected diff
 '
 
+test_expect_success 'diffs can be colorized' '
+       git reset --hard &&
+
+       # force color even though the test script has no terminal
+       test_config color.ui always &&
+
+       echo content >test &&
+       printf y | git add -p >output 2>&1 &&
+
+       # We do not want to depend on the exact coloring scheme
+       # git uses for diffs, so just check that we saw some kind of color.
+       grep "$(printf "\\033")" output
+'
+
 test_done

diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index ec78c5e3ac5661977b30582df9bec95265e1c613..671e951ee5498c7691c5e996c9dfd75dac3e8861 100755 (executable)
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -6,10 +6,11 @@
 test_description='Test diff/status color escape codes'
 . ./test-lib.sh
 
+ESC=$(printf '\033')
 color()
 {
        actual=$(git config --get-color no.such.slot "$1") &&
-       test "$actual" = "\e$2"
+       test "$actual" = "${2:+$ESC}$2"
 }
 
 invalid_color()
@@ -21,6 +22,10 @@ test_expect_success 'reset' '
        color "reset" "[m"
 '
 
+test_expect_success 'empty color is empty' '
+       color "" ""
+'
+
 test_expect_success 'attribute before color name' '
        color "bold red" "[1;31m"
 '

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 0aeabed96dacab05c1f47ad2dae49de089547b65..1edbb1e7f1d51778193dbeb071fdc94d85e283a5 100755 (executable)
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -329,7 +329,7 @@ cat > expect.colors <<\EOF
 EOF
 
 test_expect_success 'log --graph with merge with log.graphColors' '
-       test_config log.graphColors ",, blue,invalid-color, cyan, red  , " &&
+       test_config log.graphColors " blue,invalid-color, cyan, red  , " &&
        git log --color=always --graph --date-order --pretty=tformat:%s |
                test_decode_color | sed "s/ *\$//" >actual &&
        test_cmp expect.colors actual