Re: [PATCH 1/2] color: make it easier for non-config to parse color specs

["René Scharfe" <rene.scharfe@lsrfire.ath.cx>]

Jeff King schrieb:
> On Sun, Jan 18, 2009 at 06:10:36PM +0100, René Scharfe wrote:
> 
>>>  - right now, it is implemented in terms of color_parse().
>>>    But it would be more efficient to reverse this and
>>>    implement color_parse in terms of color_parse_mem.
>> Thusly?
> 
> Yes, except for the bugs you introduced. :)

Eeek! :)

>> +void color_parse_mem(const char *value, int len, const char *var, char *dst)
>> +{
>>  	const char *ptr = value;
>>  	int attr = -1;
>>  	int fg = -2;
> 
> What's missing in the context here (because it wasn't changed) is:
> 
>> 	if (!strcasecmp(value, "reset")) {
>> 		strcpy(dst, "\033[m");
>> 		return;
>> 	}
> 
> which doesn't work, since our string is actually something like
> "reset)\0" or even "reset)some totally unrelated string". So we would
> need a "memcasecmp" here.

	if (!strncasecmp(value, "reset", len)) {

> And then in the error case, we call:
> 
>> die("bad color value '%s' for variable '%s'", value, var);
> 
> which is also bogus.

   die("bad color value '%.*s' for variable '%s', len, value, var);

> I don't know if this is really even worth it. The timing difference is
> pretty minimal:
> 
>   $ time ./git log --pretty=tformat:'%Credfoo%Creset' >/dev/null
>   real    0m0.673s
>   user    0m0.652s
>   sys     0m0.016s
>   $ time ./git log --pretty=tformat:'%C(red)foo%C(reset)' >/dev/null
>   real    0m0.692s
>   user    0m0.660s
>   sys     0m0.032s
> 
> That's about 1 microsecond per commit.

Hmm, not too much overhead, agreed, but it would still be nice to avoid it.

René