Re: [PATCHv2 2/3] grep: Colorize filename, line number, and separator

[Mark Lodato <lodatom@gmail.com>]

On Sun, Mar 7, 2010 at 2:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Mark Lodato <lodatom@gmail.com> writes:
>
>> +     if (color) {
>>               if (!value)
>>                       return config_error_nonbool(var);
>> -             color_parse(value, var, opt->color_match);
>> -             return 0;
>> +             color_parse(value, var, color);
>> +             if (!strcmp(color, GIT_COLOR_RESET))
>> +                     color[0] = '\0';
>
> I don't know this "optimization" is warranted.  I can understand that you
> are trying to help the user to save a handful of useless bytes per line of
> output in the normal case, but doesn't "color.cmd.foo = normal" set the
> value to an empty string already if that is what the user wants?

Ah, I didn't know about the "normal" color.  I tried using a blank
value, which defaulted to "reset", so I thought there was no way to
say "no ANSI sequence".  The above was my workaround to get no
sequence.  Since the "normal" color exists, this workaround should be
removed.

The squash-able patch below removes the workaround, and also fixes
another bug: color was not reset at the end of the line for hunk
separators ("--").

As an aside, perhaps a blank color value should default to no ANSI
sequence, rather than the reset sequence?


diff --git i/builtin-grep.c w/builtin-grep.c
index e423eac..c519fcf 100644
--- i/builtin-grep.c
+++ w/builtin-grep.c
@@ -318,8 +318,6 @@
 		if (!value)
 			return config_error_nonbool(var);
 		color_parse(value, var, color);
-		if (!strcmp(color, GIT_COLOR_RESET))
-			color[0] = '\0';
 	}
 	return 0;
 }
diff --git i/grep.c w/grep.c
index 1e0b1e6..b641305 100644
--- i/grep.c
+++ w/grep.c
@@ -533,12 +533,15 @@

 	if (opt->pre_context || opt->post_context) {
 		if (opt->last_shown == 0) {
-			if (opt->show_hunk_mark)
-				output_color(opt, "--\n", 3, opt->color_sep);
-			else
+			if (opt->show_hunk_mark) {
+				output_color(opt, "--", 2, opt->color_sep);
+				opt->output(opt, "\n", 1);
+			} else
 				opt->show_hunk_mark = 1;
-		} else if (lno > opt->last_shown + 1)
-			output_color(opt, "--\n", 3, opt->color_sep);
+		} else if (lno > opt->last_shown + 1) {
+			output_color(opt, "--", 2, opt->color_sep);
+			opt->output(opt, "\n", 1);
+		}
 	}
 	opt->last_shown = lno;