On Wed, Jun 27, 2018 at 09:40:10AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@xxxxxxxxxxxx> writes:
>
> > - if (sign == ':')
> > - match_color = opt->color_match_selected;
> > - else
> > - match_color = opt->color_match_context;
> > - if (sign == ':')
> > - line_color = opt->color_selected;
> > - else if (sign == '-')
> > - line_color = opt->color_context;
> > - else if (sign == '=')
> > - line_color = opt->color_function;
> > + if (opt->color) {
> > + if (sign == ':')
> > + match_color = opt->color_match_selected;
> > + else
> > + match_color = opt->color_match_context;
> > + if (sign == ':')
> > + line_color = opt->color_selected;
> > + else if (sign == '-')
> > + line_color = opt->color_context;
> > + else if (sign == '=')
> > + line_color = opt->color_function;
> > + }
>
> The above change (specifically, the fact that this now is enclosed
> in "if (opt->color) { ... }") unfortunately leaves match_color
> undefined at this point in the control flow. The next loop then
> calls output_color() with an undefined match_color and tricks stupid
> compiler to issue a warning and makes -Werror build to fail.
Right, this is because we are using the `while (next_match(...))` loop
for non-colorized output, which is new. This seems like a definite
problem, and certainly causes the -Werror build to fail for me, too.
> > *eol = '\0';
> > while (next_match(opt, bol, eol, ctx, &match, eflags)) {
> > if (match.rm_so == match.rm_eo)
> > break;
> >
> > - output_color(opt, bol, match.rm_so, line_color);
> > + if (opt->only_matching)
> > + show_line_header(opt, name, lno, cno, sign);
> > + else
> > + output_color(opt, bol, match.rm_so, line_color);
> > output_color(opt, bol + match.rm_so,
> > match.rm_eo - match.rm_so, match_color);
>
> output_color() does check want_color(opt->color) before using its
> last parameter, and want_color() gives false for opt->color that is
> 0 (i.e. leaves match_color to be undefined), so in this case, the
> compiler is worried too much, but still, we should work it around if
> it is easy to do so.
>
> Just initializing match_color where it is defined at the beginning of
> show_line() should be sufficient, I think.
I think that we could also use the following, and leave the `if
(opt->color)` conditional where it is:
diff --git a/grep.c b/grep.c
index 48cca6723e..b985fb3ee0 100644
--- a/grep.c
+++ b/grep.c
@@ -1448,7 +1448,7 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
const char *name, unsigned lno, ssize_t cno, char sign)
{
int rest = eol - bol;
- const char *match_color, *line_color = NULL;
+ const char *match_color = NULL, *line_color = NULL;
if (opt->file_break && opt->last_shown == 0) {
if (opt->show_hunk_mark)
>From my reading, output_color() appears happy to treat a NULL color as
meaningless, and instead redirect the call to `opt->output` without the
preceding colorization and the reset afterwords.
We could also move that `if (opt->color)` block up closer to where
line_color and match_color are initialized, which might appear cleaner.
I think the best time to do that would be in a preparatory patch in this
series.
What do you think?
Thanks,
Taylor
