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

[Jeff King]

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

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

[Jeff King]

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.

-Peff

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

[Nguyễn Thái Ngọc Duy]

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.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 color.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/color.c b/color.c
index 81c2676..a9eadd1 100644
--- a/color.c
+++ b/color.c
@@ -207,6 +207,9 @@ int color_parse_mem(const char *value, int value_len, char 
*dst)
struct color fg = { COLOR_UNSPECIFIED };
struct color bg = { COLOR_UNSPECIFIED };
 
+   if (!len)
+   return -1;
+
if (!strncasecmp(value, "reset", len)) {
xsnprintf(dst, end - dst, GIT_COLOR_RESET);
return 0;
-- 
2.8.2.524.g6ff3d78