Re: [PATCH 2/2] grep.c: teach 'git grep --only-matching'

[Taylor Blau <me@ttaylorr.com>]

On Wed, Jun 27, 2018 at 09:40:10AM -0700, Junio C Hamano wrote:
> Taylor Blau <me@ttaylorr.com> 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