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

From: Mark Lodato <lodatom@gmail.com>
Date: Feb 28, 2010, 20:14 UTC
Message-ID: <ca433831002281214q14e6e62bj54cf7227cd32873b@mail.gmail.com>
In-Reply-To: <4B890572.5040604@lsrfire.ath.cx>

On Sat, Feb 27, 2010 at 6:43 AM, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:

Show 8 quoted lines

> 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.

Show 6 quoted lines

>> 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.

Show 6 quoted lines

>> +     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.

Show 8 quoted lines

>> +     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?