Re: [PATCHv2 2/3] grep: Colorize filename, line number, and separator
- From
Mark Lodato <lodatom@gmail.com>
- Date
- Mar 7, 2010, 20:47 UTC
- Message-ID
- <ca433831003071247j489177ceke9c3394fdc6d2372@mail.gmail.com>
- In-Reply-To
- <7v1vfwrm51.fsf@alter.siamese.dyndns.org>
On Sun, Mar 7, 2010 at 2:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> 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;