On Thu, Jan 19, 2017 at 11:38:41AM -0500, Jeff King wrote:
> 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