Re: [PATCH] color_parse_mem: allow empty color spec

[Duy Nguyen]

On Wed, Feb 01, 2017 at 01:21:29AM +0100, Jeff King 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.tqx4u45ktgpkb...@sigill.intra.peff.net>
> 
> I hadn't heard anything back,

Sorry I was accidentally busy during Luna new year holiday.

> 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').
> 
> -- >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).

..

> --- 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);

I wonder if it makes more sense to do this in builtin/config.c. The
"default value" business is strictly git-config command's. The parsing
function does not need to know. Maybe something like this?

diff --git a/builtin/config.c b/builtin/config.c
index 05843a0f96..ec1f4f0cf4 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -322,8 +322,10 @@ static void get_color(const char *var, const char 
*def_color)
git_config_with_options(git_get_color_config, NULL,
_config_source, respect_includes);
 
-   if (!get_color_found && def_color) {
-   if (color_parse(def_color, parsed_color) < 0)
+   if (!get_color_found) {
+   if (!def_color)
+   strcpy(parsed_color, GIT_COLOR_RESET);
+   else if (color_parse(def_color, parsed_color) < 0)
die(_("unable to parse default color value"));
}
 
This is also a good opportunity to document this behavior in
git-config.txt, I think.
--
Duy

Re: [PATCH] color_parse_mem: allow empty color spec

[Jacob Keller]

On Tue, Jan 31, 2017 at 4:21 PM, Jeff King  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.tqx4u45ktgpkb...@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 
> ---

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