grep: fix match highlighting for combined patterns with context 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...
Hmm?

Junio C Hamano <[email protected]> writes:

It turns out that the result of such a change becomes more readable than the original, in that it makes it clear that reinspection of the lines are done only for matched ones and not context lines.

Quoted 7 lines

Junio C Hamano <[email protected]> writes:

It turns out that the result of such a change becomes more readable than the original, in that it makes it clear that reinspection of the lines are done only for matched ones and not context lines.

Agree, it looks much clearer now. Happy if you squashed your change (commit da736e6) in zk/grep-color-words branch.

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'.

The config option color.grep.match can be used to specify the highlighting color for matching strings. Add the options matchContext and matchSelected to allow different colors to be specified for matching strings in the context vs. in selected lines. This is similar to the ms and mc specifiers in GNU grep's environment variable GREP_COLORS.

Signed-off-by Rene Scharfe <[email protected]>

---
Only *very* lightly tested, and a test for t/is missing anyway. Just wanted to quickly show what I meant. You'd set color.grep.matchContext="" to turn off highlighting in context lines. What do you think?

 Documentation/config.txt |  6 +++++-
 grep.c                   | 29 ++++++++++++++++++++++-------
 grep.h                   |  3 ++-
 3 files changed, 29 insertions(+), 9 deletions(-)

Documentation/config.txt +5 -1

index 8b49813..78832ae 100644
@@ -885,7 +885,11 @@ color.grep.<slot>::
 `linenumber`;;
 	line number prefix (when using `-n`)
 `match`;;
-	matching text
+	matching text (same as setting `matchContext` and `matchSelected`)
+`matchContext`;;
+	matching text in context lines
+`matchSelected`;;
+	matching text in selected lines
 `selected`;;
 	non-matching text in selected lines
 `separator`;;

grep.c +22 -7

index 4dc31ea..6e085f8 100644
@@ -35,7 +35,8 @@ void init_grep_defaults(void)
 	strcpy(opt->color_filename, "");
 	strcpy(opt->color_function, "");
 	strcpy(opt->color_lineno, "");
-	strcpy(opt->color_match, GIT_COLOR_BOLD_RED);
+	strcpy(opt->color_match_context, GIT_COLOR_BOLD_RED);
+	strcpy(opt->color_match_selected, GIT_COLOR_BOLD_RED);
 	strcpy(opt->color_selected, "");
 	strcpy(opt->color_sep, GIT_COLOR_CYAN);
 	opt->color = -1;

@@ -101,12 +102,22 @@ int grep_config(const char *var, const char *value, void *cb)
 		color = opt->color_function;
 	else if (!strcmp(var, "color.grep.linenumber"))
 		color = opt->color_lineno;
-	else if (!strcmp(var, "color.grep.match"))
-		color = opt->color_match;
+	else if (!strcmp(var, "color.grep.matchcontext"))
+		color = opt->color_match_context;
+	else if (!strcmp(var, "color.grep.matchselected"))
+		color = opt->color_match_selected;
 	else if (!strcmp(var, "color.grep.selected"))
 		color = opt->color_selected;
 	else if (!strcmp(var, "color.grep.separator"))
 		color = opt->color_sep;
+	else if (!strcmp(var, "color.grep.match")) {
+		int rc = 0;
+		if (!value)
+			return config_error_nonbool(var);
+		rc |= color_parse(value, opt->color_match_context);
+		rc |= color_parse(value, opt->color_match_selected);
+		return rc;
+	}
 
 	if (color) {
 		if (!value)

@@ -144,7 +155,8 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 	strcpy(opt->color_filename, def->color_filename);
 	strcpy(opt->color_function, def->color_function);
 	strcpy(opt->color_lineno, def->color_lineno);
-	strcpy(opt->color_match, def->color_match);
+	strcpy(opt->color_match_context, def->color_match_context);
+	strcpy(opt->color_match_selected, def->color_match_selected);
 	strcpy(opt->color_selected, def->color_selected);
 	strcpy(opt->color_sep, def->color_sep);
 }

@@ -1084,7 +1096,7 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		      const char *name, unsigned lno, char sign)
 {
 	int rest = eol - bol;
-	char *line_color = NULL;
+	const char *match_color, *line_color = NULL;
 
 	if (opt->file_break && opt->last_shown == 0) {
 		if (opt->show_hunk_mark)

@@ -1123,6 +1135,10 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		int eflags = 0;
 
 		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;

@@ -1135,8 +1151,7 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 
 			output_color(opt, bol, match.rm_so, line_color);
 			output_color(opt, bol + match.rm_so,
-				     match.rm_eo - match.rm_so,
-				     opt->color_match);
+				     match.rm_eo - match.rm_so, match_color);
 			bol += match.rm_eo;
 			rest -= match.rm_eo;
 			eflags = REG_NOTBOL;

grep.h +2 -1

index eaaced1..95f197a 100644
@@ -124,7 +124,8 @@ struct grep_opt {
 	char color_filename[COLOR_MAXLEN];
 	char color_function[COLOR_MAXLEN];
 	char color_lineno[COLOR_MAXLEN];
-	char color_match[COLOR_MAXLEN];
+	char color_match_context[COLOR_MAXLEN];
+	char color_match_selected[COLOR_MAXLEN];
 	char color_selected[COLOR_MAXLEN];
 	char color_sep[COLOR_MAXLEN];
 	int regflags;

Quoted 3 lines

Only *very* lightly tested, and a test for t/is missing anyway. Just wanted to quickly show what I meant. You'd set color.grep.matchContext="" to turn off highlighting in context lines. What do you think?

I didn't realize that people wanted to see pieces on non-matching lines highlighted. It makes certain sense, e.g. it would allow you to spot near-misses, but that is only true for lines that neighbour real hits, so...

I like this approach better in that it makes those who want a different behaviour to do the work without breaking the expectation of those who are used to the established behaviour.

Zoltan?

I like René's approach, too. It's more flexible, supports the old behaviour and it scratches my itch as well.
Don't mind if you dropped my patch and used René's instead.

I like René's approach, too. It's more flexible, supports the old behaviour and it scratches my itch as well.

Quoted 11 lines · René Scharfe

The config option color.grep.match can be used to specify the highlighting color for matching strings. Add the options matchContext and matchSelected to allow different colors to be specified for matching strings in the context vs. in selected lines. This is similar to the ms and mc specifiers in GNU grep's environment variable GREP_COLORS.

Signed-off-by Rene Scharfe <[email protected]>

---
Only *very* lightly tested, and a test for t/is missing anyway. Just wanted to quickly show what I meant. You'd set color.grep.matchContext="" to turn off highlighting in context lines. What do you think?

I didn't realize that people wanted to see pieces on non-matching lines highlighted. It makes certain sense, e.g. it would allow you to spot near-misses, but that is only true for lines that neighbour real hits, so...

I like this approach better in that it makes those who want a different behaviour to do the work without breaking the expectation of those who are used to the established behaviour.