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

On Sat, Feb 27, 2010 at 6:43 AM, René Scharfe
[off-list ref] wrote:

Am 27.02.2010 05:57, schrieb Mark Lodato:

quoted

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.

quoted

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.

quoted

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

quoted

+     }
+     else

       } else

Oops, thanks.

quoted

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