Re: [PATCH] color_parse_mem: allow empty color spec

[Jacob Keller <jacob.keller@gmail.com>]

On Tue, Jan 31, 2017 at 4:21 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Jan 31, 2017 at 02:45:40PM -0800, Junio C Hamano wrote:
>
>> * nd/log-graph-configurable-colors (2017-01-23) 3 commits
>>   (merged to 'next' on 2017-01-23 at c369982ad8)
>>  + log --graph: customize the graph lines with config log.graphColors
>>  + color.c: trim leading spaces in color_parse_mem()
>>  + color.c: fix color_parse_mem() with value_len == 0
>>
>>  Some people feel the default set of colors used by "git log --graph"
>>  rather limiting.  A mechanism to customize the set of colors has
>>  been introduced.
>>
>>  Reported to break "add -p".
>>  cf. <20170128040709.tqx4u45ktgpkbfm4@sigill.intra.peff.net>
>
> I hadn't heard anything back, so I wrapped up my fix with a commit
> message and tests (it needs to go on top anyway, since the breakage is
> in 'next').
>

I was just about to report this breakage... It definitely breaks usage
of git-add-interactive..

> -- >8 --
> Subject: [PATCH] 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>
> ---

The patch looks good to me. Very nice to have a detailed explanation
of why we didn't catch it before, and how to ensure we don't have this
happen again.

Thanks,
Jake

>  color.c                    |  6 ++++--
>  t/t3701-add-interactive.sh | 14 ++++++++++++++
>  t/t4026-color.sh           |  7 ++++++-
>  t/t4202-log.sh             |  2 +-
>  4 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/color.c b/color.c
> index 7bb4a96f8..2925a819b 100644
> --- 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 deae948c7..5ffe78e92 100755
> --- 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 ec78c5e3a..671e951ee 100755
> --- 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" = " $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 0aeabed96..1edbb1e7f 100755
> --- 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
> --
> 2.11.0.948.gca97da533
>