Re: [PATCH 4/5] grep: Colorize filename, line number, and separator

[Mark Lodato <lodatom@gmail.com>]

On Sat, Feb 27, 2010 at 6:43 AM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> Am 27.02.2010 05:57, schrieb Mark Lodato:
>> 1. With --name-only, GNU grep colors the filenames, but we do not.  I do
>>    not see any point to making everything the same color.
>
> I guess they did it for consistency, so when you see "magenta" you think
> "filename", and because it can be turned off with a switch.  With your
> patch all filenames are coloured the same, too, by the way: using the
> default foreground colour. :)

Yes, I think I understand the reasoning, but to me it is very
annoying.  However, if there is a consensus that we should follow GNU
grep in this regard, I will do it.

>> diff --git a/builtin-grep.c b/builtin-grep.c
>> +     if (!value)
>> +             return config_error_nonbool(var);
>
> color.grep without a value used to turn on colourization, now it seems
> to error out.

Oops, that should be "if (color && !value)".  I will fix in next respin.

>> +     color_parse(value, var, color);
>> +     if (!strcmp(color, GIT_COLOR_RESET))
>> +             color[0] = '\0';
>
> This turns off colouring if the user specified "reset" as the colour,
> right?

Yes.

> Interesting optimization, but is it really needed? Perhaps it's
> just me, but I'd give the user the requested "<reset>text<reset>"
> sequence if she asked for it, even if it's longer than and looks the
> same as "text" alone.

The problem is that there's no way to say "no color".  A blank value
and "reset" both come to the same thing.  I would rather have as
little markup as possible in the output, and this tweak is very
simple.  While this is not strictly necessary, it does make the output
identical to the pre-patch output if you disable all the new colors
(just grep.color.separator, by default.)

>> +     }
>> +     else
>
>        } else

Oops, thanks.

>> +     if (opt->null_following_name) {
>> +             sign = '\0';
>> +             opt->output(opt, &sign, 1);
>> +     } else
>
>        if (opt->null_following_name)
>                opt->output(opt, "", 1);
>        else

Personally, I find your suggestion less readable.  My version is only
one line longer but makes the code completely obvious, whereas the
one-liner requires a second of thought.  Anyone else care to comment
on this?