Re: [PATCH] document behavior of empty color name

From: Duy Nguyen <pclouds@gmail.com>
Date: Feb 3, 2017, 09:24 UTC
Message-ID: <20170203092430.GA10987@ash>
In-Reply-To: <20170202124238.53k74cedpp2rcmzo@sigill.intra.peff.net>

On Thu, Feb 02, 2017 at 01:42:44PM +0100, Jeff King wrote:

Show 28 quoted lines

> On Thu, Feb 02, 2017 at 04:16:15PM +0700, Duy Nguyen wrote:
> 
> > > I hadn't heard anything back,
> > 
> > Sorry I was accidentally busy during Luna new year holiday.
> 
> No problem. That sounds much more fun than working on Git. :)
> 
> > > -	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?
> 
> I don't think so. The default value is a git-config thing, but you would
> want to be able to do the same thing in a config file. For example, to
> disable coloring entirely for part of the diff, you could do:
> 
>   [color "diff"]
>   meta = ""

OK but it makes log.graphColors add empty colors though. In t4202.39, we have " blue,invalid-color, cyan, red , ". With this patch the color after red would be no color. Without it, we get a complaint and the next color would be cycled back to blue. The test does not catch this because the test graph does not have enough fork points to get to red and back to blue.

I suppose this is ok after your documentation patch ("that's by design!"). Just wanted to point it out in case I miss something. -- Duy