Re: [PATCH] color_parse_mem: allow empty color spec

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 2, 2017, 09:16 UTC
Message-ID: <20170202091615.GA22337@ash>
In-Reply-To: <20170201002129.xb62hmxwrzwgp6vg@sigill.intra.peff.net>

On Wed, Feb 01, 2017 at 01:21:29AM +0100, Jeff King wrote:

Show 16 quoted lines

> 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,

Sorry I was accidentally busy during Luna new year holiday.

Show 18 quoted lines

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

..

Show 15 quoted lines

> --- 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,
 				&given_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