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:
>
> > * 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@xxxxxxxxxxxxxxxxxxxxx>
>
> 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).
..
> --- 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