Re: [PATCH v5 1/3] color.c: fix color_parse_mem() with value_len == 0

From: Jeff King <peff@peff.net>
Date: Jan 28, 2017, 04:07 UTC
Message-ID: <20170128040709.tqx4u45ktgpkbfm4@sigill.intra.peff.net>
In-Reply-To: <20170119163841.zhtpz6rxdieaywuy@sigill.intra.peff.net>

On Thu, Jan 19, 2017 at 11:38:41AM -0500, Jeff King wrote:

Show 9 quoted lines

> On Thu, Jan 19, 2017 at 06:41:21PM +0700, Nguyễn Thái Ngọc Duy wrote:
> 
> > In this code we want to match the word "reset". If len is zero,
> > strncasecmp() will return zero and we incorrectly assume it's "reset" as
> > a result.
> 
> This is probably a good idea. This _is_ user-visible, so it's possible
> somebody was using empty config as a synonym for "reset". But since it
> was never documented, I feel like relying on that is somewhat crazy.

Hrm. This seems to break the add--interactive script if you do not have color.diff.plain set:

  $ GIT_TRACE=1 git add -p
  ...
  22:58:12.568990 [pid=11401] git.c:387  trace: built-in: git 'config' '--get-color' 'color.diff.plain' ''
  fatal: unable to parse default color value
  config --get-color color.diff.plain : command returned error: 128

As you can see, the default value the empty string, which is now an error.

The default in the C code for that value is GIT_COLOR_NORMAL, which really is the empty string. So I think the old code was buggy to choose "reset", but the new one is worse because it fails entirely. :)

We probably want something like this instead:

diff --git a/color.c b/color.c
index 190b2da96..dee61557e 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);

The breakage is in 'next' (it looks like it went out a few days ago; I'm
surprised I didn't notice until now).

-Peff