Re: [PATCH] color_parse_mem: allow empty color spec
From: Duy Nguyen <hidden>
Date: 2017-02-02 09:16:26
Subsystem:
the rest · Maintainer:
Linus Torvalds
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:quoted
* 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. [ref]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).
..
quoted hunk ↗ jump to hunk
--- 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