Re: [PATCH] grep: fix match highlighting for combined patterns with context lines

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 21, 2014, 19:23 UTC
Message-ID: <xmqqh9yxz0pc.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1413870963-66431-1-git-send-email-zoltan.klinger@gmail.com>

Zoltan Klinger <zoltan.klinger@gmail.com> writes:

Show 36 quoted lines

> When git grep is run with combined patterns such as '-e p1 --and -e p2'
> and surrounding context lines are requested, the output contains
> incorrectly highlighted matches.
>
> Consider the following output (highlighted matches are surrounded by '*'
> characters):
>     $ cat testfile
>     foo a
>     foo b
>     foo bar
>     baz bar foo
>     bar x
>     bar y
>     $ git grep -n -C2 -e foo --and -e bar testfile
>     testfile-1-*foo* a
>     testfile-2-*foo* b
>     testfile:3:*foo* *bar*
>     testfile:4:baz *bar* *foo*
>     testfile-5-*bar* x
>     testfile-6-*bar* y
>
> Lines 1, 2, 5 and 6 do not match the combined patterns, they only
> contain incorrectly highlighted 'false positives'.
>
> Modify the show_line() function in grep.c to highlight matches only on
> lines that match the combined pattern. Do not highlight matches on lines
> that provide only context or contain only the function name of the match.
>
> The output of the same command after the change:
>     $ git grep -n -C2 -e foo --and -e bar testfile
>     testfile-1-foo a
>     testfile-2-foo b
>     testfile:3:*foo* *bar*
>     testfile:4:baz *bar* *foo*
>     testfile-5-bar x
>     testfile-6-bar y

If your goal is to stop colouring words on context and other kinds of lines, do you still need the "while (next_match(...))" loop for them? Can't you make the resulting code clearer by restructuring the inside of the whole "if (opt->color)" block further, something along the lines of...

	if (sign != ':') {
		regmatch_t match; ...
		enum grep_context ctx = GREP_CONTEXT_BODY;
                ...
        	while (next_match(...)) {
                	... the "word-by-word" loop ...
		}
	} else {
        	switch (sign) {
		case '-':
                       	line_color = opt->color_context;
                        break;
		case ':':
                       	line_color = opt->color_function;
                        break;
		}
                output_color(opt, bol, ..., line_color);
	}

Hmm?