threads / patch / 18190

patch, 5 partsgrep: color search patterns

Subject: [PATCH 0/5] grep: color search patterns

## tl;dr

10 messages between Mar 7, 2009 and Mar 16, 2009. Diffs are folded; open one to read it.

replies: 9people: 2as markdown or json

René Scharfe· Mar 7, 2009, 12:24 UTC · lore

Match coloring is a major missing feature of git grep. It makes reading the output much easier and brings grep onto the same visual level as the other colorized git commands.

Two earlier attempts to implement this feature didn't go in. They didn't support support pattern expressions (--and, --or etc.) or external greps, unlike this series.

[PATCH 1/5] grep: micro-optimize hit collection for AND nodes [PATCH 2/5] grep: remove grep_opt argument from match_expr_eval()

   These are cleanup patches and unrelated to coloring.  They touch the
   same code, though, so it's a good idea to take this opportunity to
   beautify the code a bit before adding new features.
[PATCH 3/5] grep: add pmatch and eflags arguments to match_one_pattern()
   The two arguments added by this patch are used by the next one.
[PATCH 4/5] grep: color patterns in output
   Add color support to internal git grep.
[PATCH 5/5] grep: add support for coloring with external greps
   Extends color support to external greps.
René Scharfe· Mar 7, 2009, 12:27 UTC · re: René Scharfe · lore

[PATCH 1/5] grep: micro-optimize hit collection for AND nodes

In addition to returning if an expression matches a line, match_expr_eval() updates the expression's hit flag if the parameter collect_hits is set. It never sets collect_hits for children of AND nodes, though, so their hit flag will never be updated. Because of that we can return early if the first child didn't match, no matter if collect_hits is set or not.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 grep.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)
Show changes to grep.c +3 −7
diff --git a/grep.c b/grep.c
index 062b2b6..db341b6 100644
--- a/grep.c
+++ b/grep.c
@@ -394,13 +394,9 @@ static int match_expr_eval(struct grep_opt *o,
 		h = !match_expr_eval(o, x->u.unary, bol, eol, ctx, 0);
 		break;
 	case GREP_NODE_AND:
-		if (!collect_hits)
-			return (match_expr_eval(o, x->u.binary.left,
-						bol, eol, ctx, 0) &&
-				match_expr_eval(o, x->u.binary.right,
-						bol, eol, ctx, 0));
-		h = match_expr_eval(o, x->u.binary.left, bol, eol, ctx, 0);
-		h &= match_expr_eval(o, x->u.binary.right, bol, eol, ctx, 0);
+		if (!match_expr_eval(o, x->u.binary.left, bol, eol, ctx, 0))
+			return 0;
+		h = match_expr_eval(o, x->u.binary.right, bol, eol, ctx, 0);
 		break;
 	case GREP_NODE_OR:
 		if (!collect_hits)

-- 
1.6.2
René Scharfe· Mar 7, 2009, 12:28 UTC · re: René Scharfe · lore

[PATCH 2/5] grep: remove grep_opt argument from match_expr_eval()

The only use of the struct grep_opt argument of match_expr_eval() is to pass the option word_regexp to match_one_pattern(). By adding a pattern flag for it we can reduce the number of function arguments of these two functions, as a cleanup and preparation for adding more in the next patch.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 grep.c |   34 +++++++++++++++++-----------------
 grep.h |    1 +
 2 files changed, 18 insertions(+), 17 deletions(-)
Show changes to 2 files +18 −17

grep.c, grep.h

diff --git a/grep.c b/grep.c
index db341b6..f455182 100644
--- a/grep.c
+++ b/grep.c
@@ -39,6 +39,8 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
 {
 	int err;
 
+	p->word_regexp = opt->word_regexp;
+
 	if (opt->fixed || is_fixed(p->pattern))
 		p->fixed = 1;
 	if (opt->regflags & REG_ICASE)
@@ -306,7 +308,8 @@ static struct {
 	{ "committer ", 10 },
 };
 
-static int match_one_pattern(struct grep_opt *opt, struct grep_pat *p, char *bol, char *eol, enum grep_context ctx)
+static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
+			     enum grep_context ctx)
 {
 	int hit = 0;
 	int saved_ch = 0;
@@ -338,7 +341,7 @@ static int match_one_pattern(struct grep_opt *opt, struct grep_pat *p, char *bol
 		hit = !fixmatch(p->pattern, bol, pmatch);
 	}
 
-	if (hit && opt->word_regexp) {
+	if (hit && p->word_regexp) {
 		if ((pmatch[0].rm_so < 0) ||
 		    (eol - bol) <= pmatch[0].rm_so ||
 		    (pmatch[0].rm_eo < 0) ||
@@ -378,35 +381,32 @@ static int match_one_pattern(struct grep_opt *opt, struct grep_pat *p, char *bol
 	return hit;
 }
 
-static int match_expr_eval(struct grep_opt *o,
-			   struct grep_expr *x,
-			   char *bol, char *eol,
-			   enum grep_context ctx,
-			   int collect_hits)
+static int match_expr_eval(struct grep_expr *x, char *bol, char *eol,
+			   enum grep_context ctx, int collect_hits)
 {
 	int h = 0;
 
 	switch (x->node) {
 	case GREP_NODE_ATOM:
-		h = match_one_pattern(o, x->u.atom, bol, eol, ctx);
+		h = match_one_pattern(x->u.atom, bol, eol, ctx);
 		break;
 	case GREP_NODE_NOT:
-		h = !match_expr_eval(o, x->u.unary, bol, eol, ctx, 0);
+		h = !match_expr_eval(x->u.unary, bol, eol, ctx, 0);
 		break;
 	case GREP_NODE_AND:
-		if (!match_expr_eval(o, x->u.binary.left, bol, eol, ctx, 0))
+		if (!match_expr_eval(x->u.binary.left, bol, eol, ctx, 0))
 			return 0;
-		h = match_expr_eval(o, x->u.binary.right, bol, eol, ctx, 0);
+		h = match_expr_eval(x->u.binary.right, bol, eol, ctx, 0);
 		break;
 	case GREP_NODE_OR:
 		if (!collect_hits)
-			return (match_expr_eval(o, x->u.binary.left,
+			return (match_expr_eval(x->u.binary.left,
 						bol, eol, ctx, 0) ||
-				match_expr_eval(o, x->u.binary.right,
+				match_expr_eval(x->u.binary.right,
 						bol, eol, ctx, 0));
-		h = match_expr_eval(o, x->u.binary.left, bol, eol, ctx, 0);
+		h = match_expr_eval(x->u.binary.left, bol, eol, ctx, 0);
 		x->u.binary.left->hit |= h;
-		h |= match_expr_eval(o, x->u.binary.right, bol, eol, ctx, 1);
+		h |= match_expr_eval(x->u.binary.right, bol, eol, ctx, 1);
 		break;
 	default:
 		die("Unexpected node type (internal error) %d", x->node);
@@ -420,7 +420,7 @@ static int match_expr(struct grep_opt *opt, char *bol, char *eol,
 		      enum grep_context ctx, int collect_hits)
 {
 	struct grep_expr *x = opt->pattern_expression;
-	return match_expr_eval(opt, x, bol, eol, ctx, collect_hits);
+	return match_expr_eval(x, bol, eol, ctx, collect_hits);
 }
 
 static int match_line(struct grep_opt *opt, char *bol, char *eol,
@@ -432,7 +432,7 @@ static int match_line(struct grep_opt *opt, char *bol, char *eol,
 
 	/* we do not call with collect_hits without being extended */
 	for (p = opt->pattern_list; p; p = p->next) {
-		if (match_one_pattern(opt, p, bol, eol, ctx))
+		if (match_one_pattern(p, bol, eol, ctx))
 			return 1;
 	}
 	return 0;
diff --git a/grep.h b/grep.h
index 5102ce3..d2a8674 100644
--- a/grep.h
+++ b/grep.h
@@ -31,6 +31,7 @@ struct grep_pat {
 	enum grep_header_field field;
 	regex_t regexp;
 	unsigned fixed:1;
+	unsigned word_regexp:1;
 };
 
 enum grep_expr_node {

-- 
1.6.2
René Scharfe· Mar 7, 2009, 12:30 UTC · re: René Scharfe · lore

[PATCH 3/5] grep: add pmatch and eflags arguments to match_one_pattern()

Push pmatch and eflags to the callers of match_one_pattern(), which allows them to specify regex execution flags and to get the location of a match.

Since we only use the first element of the matches array and aren't interested in submatches, no provision is made for callers to provide a larger array.

eflags are ignored for fixed patterns, but that's OK, since they only have a meaning in connection with regular expressions containing ^ or $.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 grep.c |   21 ++++++++++-----------
 1 files changed, 10 insertions(+), 11 deletions(-)
Show changes to grep.c +10 −11
diff --git a/grep.c b/grep.c
index f455182..bdcff7b 100644
--- a/grep.c
+++ b/grep.c
@@ -309,11 +309,11 @@ static struct {
 };
 
 static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
-			     enum grep_context ctx)
+			     enum grep_context ctx,
+			     regmatch_t *pmatch, int eflags)
 {
 	int hit = 0;
 	int saved_ch = 0;
-	regmatch_t pmatch[10];
 
 	if ((p->token != GREP_PATTERN) &&
 	    ((p->token == GREP_PATTERN_HEAD) != (ctx == GREP_CONTEXT_HEAD)))
@@ -332,14 +332,10 @@ static int match_one_pattern(struct grep_pat *p, char *bol, char *eol,
 	}
 
  again:
-	if (!p->fixed) {
-		regex_t *exp = &p->regexp;
-		hit = !regexec(exp, bol, ARRAY_SIZE(pmatch),
-			       pmatch, 0);
-	}
-	else {
+	if (p->fixed)
 		hit = !fixmatch(p->pattern, bol, pmatch);
-	}
+	else
+		hit = !regexec(&p->regexp, bol, 1, pmatch, eflags);
 
 	if (hit && p->word_regexp) {
 		if ((pmatch[0].rm_so < 0) ||
@@ -385,10 +381,11 @@ static int match_expr_eval(struct grep_expr *x, char *bol, char *eol,
 			   enum grep_context ctx, int collect_hits)
 {
 	int h = 0;
+	regmatch_t match;
 
 	switch (x->node) {
 	case GREP_NODE_ATOM:
-		h = match_one_pattern(x->u.atom, bol, eol, ctx);
+		h = match_one_pattern(x->u.atom, bol, eol, ctx, &match, 0);
 		break;
 	case GREP_NODE_NOT:
 		h = !match_expr_eval(x->u.unary, bol, eol, ctx, 0);
@@ -427,12 +424,14 @@ static int match_line(struct grep_opt *opt, char *bol, char *eol,
 		      enum grep_context ctx, int collect_hits)
 {
 	struct grep_pat *p;
+	regmatch_t match;
+
 	if (opt->extended)
 		return match_expr(opt, bol, eol, ctx, collect_hits);
 
 	/* we do not call with collect_hits without being extended */
 	for (p = opt->pattern_list; p; p = p->next) {
-		if (match_one_pattern(p, bol, eol, ctx))
+		if (match_one_pattern(p, bol, eol, ctx, &match, 0))
 			return 1;
 	}
 	return 0;

-- 
1.6.2
René Scharfe· Mar 7, 2009, 12:32 UTC · re: René Scharfe · lore

[PATCH 4/5] grep: color patterns in output

Coloring matches makes them easier to spot in the output.

Add two options and two parameters: color.grep (to turn coloring on or off), color.grep.match (to set the color of matches), --color and --no-color (to turn coloring on or off, respectively).

The output of external greps is not changed.

This patch is based on earlier ones by Nguyễn Thái Ngọc Duy and Thiago Alves.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 Documentation/config.txt   |    9 ++++
 Documentation/git-grep.txt |    8 ++++
 builtin-grep.c             |   32 +++++++++++++++
 grep.c                     |   90 ++++++++++++++++++++++++++++++++++++++------
 grep.h                     |    3 +
 5 files changed, 130 insertions(+), 12 deletions(-)
Show changes to 5 files +130 −12

Documentation/config.txt, Documentation/git-grep.txt, builtin-grep.c, grep.c, grep.h

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f5152c5..b75dada 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -548,6 +548,15 @@ color.diff.<slot>::
 	whitespace errors). The values of these variables may be specified as
 	in color.branch.<slot>.
 
+color.grep::
+	When set to `always`, always highlight matches.  When `false` (or
+	`never`), never.  When set to `true` or `auto`, use color only
+	when the output is written to the terminal.  Defaults to `false`.
+
+color.grep.match::
+	Use customized color for matches.  The value of this variable
+	may be specified as in color.branch.<slot>.
+
 color.interactive::
 	When set to `always`, always use colors for interactive prompts
 	and displays (such as those used by "git-add --interactive").
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 553da6c..fccb82d 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -17,6 +17,7 @@ SYNOPSIS
 	   [-l | --files-with-matches] [-L | --files-without-match]
 	   [-z | --null]
 	   [-c | --count] [--all-match]
+	   [--color | --no-color]
 	   [-A <post-context>] [-B <pre-context>] [-C <context>]
 	   [-f <file>] [-e] <pattern>
 	   [--and|--or|--not|(|)|-e <pattern>...] [<tree>...]
@@ -105,6 +106,13 @@ OPTIONS
 	Instead of showing every matched line, show the number of
 	lines that match.
 
+--color::
+	Show colored matches.
+
+--no-color::
+	Turn off match highlighting, even when the configuration file
+	gives the default to color output.
+
 -[ABC] <context>::
 	Show `context` trailing (`A` -- after), or leading (`B`
 	-- before), or both (`C` -- context) lines, and place a
diff --git a/builtin-grep.c b/builtin-grep.c
index 3f12ba3..e2c0f01 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -22,6 +22,24 @@
 
 static int builtin_grep;
 
+static int grep_config(const char *var, const char *value, void *cb)
+{
+	struct grep_opt *opt = cb;
+
+	if (!strcmp(var, "grep.color") || !strcmp(var, "color.grep")) {
+		opt->color = git_config_colorbool(var, value, -1);
+		return 0;
+	}
+	if (!strcmp(var, "grep.color.match") ||
+	    !strcmp(var, "color.grep.match")) {
+		if (!value)
+			return config_error_nonbool(var);
+		color_parse(value, var, opt->color_match);
+		return 0;
+	}
+	return git_color_default_config(var, value, cb);
+}
+
 /*
  * git grep pathspecs are somewhat different from diff-tree pathspecs;
  * pathname wildcards are allowed.
@@ -536,6 +554,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	opt.pattern_tail = &opt.pattern_list;
 	opt.regflags = REG_NEWLINE;
 
+	strcpy(opt.color_match, GIT_COLOR_RED GIT_COLOR_BOLD);
+	opt.color = -1;
+	git_config(grep_config, &opt);
+	if (opt.color == -1)
+		opt.color = git_use_color_default;
+
 	/*
 	 * If there is no -- then the paths must exist in the working
 	 * tree.  If there is no explicit pattern specified with -e or
@@ -732,6 +756,14 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 			opt.relative = 0;
 			continue;
 		}
+		if (!strcmp("--color", arg)) {
+			opt.color = 1;
+			continue;
+		}
+		if (!strcmp("--no-color", arg)) {
+			opt.color = 0;
+			continue;
+		}
 		if (!strcmp("--", arg)) {
 			/* later processing wants to have this at argv[1] */
 			argv--;
diff --git a/grep.c b/grep.c
index bdcff7b..cace1c8 100644
--- a/grep.c
+++ b/grep.c
@@ -253,18 +253,6 @@ static int word_char(char ch)
 	return isalnum(ch) || ch == '_';
 }
 
-static void show_line(struct grep_opt *opt, const char *bol, const char *eol,
-		      const char *name, unsigned lno, char sign)
-{
-	if (opt->null_following_name)
-		sign = '\0';
-	if (opt->pathname)
-		printf("%s%c", name, sign);
-	if (opt->linenum)
-		printf("%d%c", lno, sign);
-	printf("%.*s\n", (int)(eol-bol), bol);
-}
-
 static void show_name(struct grep_opt *opt, const char *name)
 {
 	printf("%s%c", name, opt->null_following_name ? '\0' : '\n');
@@ -437,6 +425,84 @@ static int match_line(struct grep_opt *opt, char *bol, char *eol,
 	return 0;
 }
 
+static int match_next_pattern(struct grep_pat *p, char *bol, char *eol,
+			      enum grep_context ctx,
+			      regmatch_t *pmatch, int eflags)
+{
+	regmatch_t match;
+
+	if (!match_one_pattern(p, bol, eol, ctx, &match, eflags))
+		return 0;
+	if (match.rm_so < 0 || match.rm_eo < 0)
+		return 0;
+	if (pmatch->rm_so >= 0 && pmatch->rm_eo >= 0) {
+		if (match.rm_so > pmatch->rm_so)
+			return 1;
+		if (match.rm_so == pmatch->rm_so && match.rm_eo < pmatch->rm_eo)
+			return 1;
+	}
+	pmatch->rm_so = match.rm_so;
+	pmatch->rm_eo = match.rm_eo;
+	return 1;
+}
+
+static int next_match(struct grep_opt *opt, char *bol, char *eol,
+		      enum grep_context ctx, regmatch_t *pmatch, int eflags)
+{
+	struct grep_pat *p;
+	int hit = 0;
+
+	pmatch->rm_so = pmatch->rm_eo = -1;
+	if (bol < eol) {
+		for (p = opt->pattern_list; p; p = p->next) {
+			switch (p->token) {
+			case GREP_PATTERN: /* atom */
+			case GREP_PATTERN_HEAD:
+			case GREP_PATTERN_BODY:
+				hit |= match_next_pattern(p, bol, eol, ctx,
+							  pmatch, eflags);
+				break;
+			default:
+				break;
+			}
+		}
+	}
+	return hit;
+}
+
+static void show_line(struct grep_opt *opt, char *bol, char *eol,
+		      const char *name, unsigned lno, char sign)
+{
+	int rest = eol - bol;
+
+	if (opt->null_following_name)
+		sign = '\0';
+	if (opt->pathname)
+		printf("%s%c", name, sign);
+	if (opt->linenum)
+		printf("%d%c", lno, sign);
+	if (opt->color) {
+		regmatch_t match;
+		enum grep_context ctx = GREP_CONTEXT_BODY;
+		int ch = *eol;
+		int eflags = 0;
+
+		*eol = '\0';
+		while (next_match(opt, bol, eol, ctx, &match, eflags)) {
+			printf("%.*s%s%.*s%s",
+			       match.rm_so, bol,
+			       opt->color_match,
+			       match.rm_eo - match.rm_so, bol + match.rm_so,
+			       GIT_COLOR_RESET);
+			bol += match.rm_eo;
+			rest -= match.rm_eo;
+			eflags = REG_NOTBOL;
+		}
+		*eol = ch;
+	}
+	printf("%.*s\n", rest, bol);
+}
+
 static int grep_buffer_1(struct grep_opt *opt, const char *name,
 			 char *buf, unsigned long size, int collect_hits)
 {
diff --git a/grep.h b/grep.h
index d2a8674..73b33ab 100644
--- a/grep.h
+++ b/grep.h
@@ -1,5 +1,6 @@
 #ifndef GREP_H
 #define GREP_H
+#include "color.h"
 
 enum grep_pat_token {
 	GREP_PATTERN,
@@ -77,6 +78,8 @@ struct grep_opt {
 	unsigned relative:1;
 	unsigned pathname:1;
 	unsigned null_following_name:1;
+	int color;
+	char color_match[COLOR_MAXLEN];
 	int regflags;
 	unsigned pre_context;
 	unsigned post_context;

-- 
1.6.2
René Scharfe· Mar 7, 2009, 12:34 UTC · re: René Scharfe · lore

[PATCH 5/5] grep: add support for coloring with external greps

Add the config variable color.grep.external, which can be used to switch on coloring of external greps. To enable auto coloring with GNU grep, one needs to set color.grep.external to --color=always to defeat the pager started by git grep. The value of the config variable will be passed to the external grep only if it would colorize internal grep's output, so automatic terminal detected works. The default is to not pass any option, because the external grep command could be a program without color support.

Also set the environment variables GREP_COLOR and GREP_COLORS to pass the configured color for matches to the external grep. This works with GNU grep; other variables could be added as needed.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 Documentation/config.txt |   12 +++++++++++-
 builtin-grep.c           |   36 ++++++++++++++++++++++++++++++++++++
 grep.h                   |    1 +
 3 files changed, 48 insertions(+), 1 deletions(-)
Show changes to 3 files +48 −1

Documentation/config.txt, builtin-grep.c, grep.h

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b75dada..4d42bff 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -553,9 +553,19 @@ color.grep::
 	`never`), never.  When set to `true` or `auto`, use color only
 	when the output is written to the terminal.  Defaults to `false`.
 
+color.grep.external::
+	The string value of this variable is passed to an external 'grep'
+	command as a command line option if match highlighting is turned
+	on.  If set to an empty string, no option is passed at all,
+	turning off coloring for external 'grep' calls; this is the default.
+	For GNU grep, set it to `--color=always` to highlight matches even
+	when a pager is used.
+
 color.grep.match::
 	Use customized color for matches.  The value of this variable
-	may be specified as in color.branch.<slot>.
+	may be specified as in color.branch.<slot>.  It is passed using
+	the environment variables 'GREP_COLOR' and 'GREP_COLORS' when
+	calling an external 'grep'.
 
 color.interactive::
 	When set to `always`, always use colors for interactive prompts
diff --git a/builtin-grep.c b/builtin-grep.c
index e2c0f01..9e7e766 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -30,6 +30,10 @@ static int grep_config(const char *var, const char *value, void *cb)
 		opt->color = git_config_colorbool(var, value, -1);
 		return 0;
 	}
+	if (!strcmp(var, "grep.color.external") ||
+	    !strcmp(var, "color.grep.external")) {
+		return git_config_string(&(opt->color_external), var, value);
+	}
 	if (!strcmp(var, "grep.color.match") ||
 	    !strcmp(var, "color.grep.match")) {
 		if (!value)
@@ -287,6 +291,21 @@ static int flush_grep(struct grep_opt *opt,
 	return status;
 }
 
+static void grep_add_color(struct strbuf *sb, const char *escape_seq)
+{
+	size_t orig_len = sb->len;
+
+	while (*escape_seq) {
+		if (*escape_seq == 'm')
+			strbuf_addch(sb, ';');
+		else if (*escape_seq != '\033' && *escape_seq  != '[')
+			strbuf_addch(sb, *escape_seq);
+		escape_seq++;
+	}
+	if (sb->len > orig_len && sb->buf[sb->len - 1] == ';')
+		strbuf_setlen(sb, sb->len - 1);
+}
+
 static int external_grep(struct grep_opt *opt, const char **paths, int cached)
 {
 	int i, nr, argc, hit, len, status;
@@ -357,6 +376,23 @@ static int external_grep(struct grep_opt *opt, const char **paths, int cached)
 		push_arg("-e");
 		push_arg(p->pattern);
 	}
+	if (opt->color) {
+		struct strbuf sb = STRBUF_INIT;
+
+		grep_add_color(&sb, opt->color_match);
+		setenv("GREP_COLOR", sb.buf, 1);
+
+		strbuf_reset(&sb);
+		strbuf_addstr(&sb, "mt=");
+		grep_add_color(&sb, opt->color_match);
+		strbuf_addstr(&sb, ":sl=:cx=:fn=:ln=:bn=:se=");
+		setenv("GREP_COLORS", sb.buf, 1);
+
+		strbuf_release(&sb);
+
+		if (opt->color_external && strlen(opt->color_external) > 0)
+			push_arg(opt->color_external);
+	}
 
 	hit = 0;
 	argc = nr;
diff --git a/grep.h b/grep.h
index 73b33ab..a67005d 100644
--- a/grep.h
+++ b/grep.h
@@ -80,6 +80,7 @@ struct grep_opt {
 	unsigned null_following_name:1;
 	int color;
 	char color_match[COLOR_MAXLEN];
+	const char *color_external;
 	int regflags;
 	unsigned pre_context;
 	unsigned post_context;

-- 
1.6.2
Nguyen Thai Ngoc Duy· Mar 10, 2009, 06:01 UTC · re: René Scharfe · lore

Re: [PATCH 0/5] grep: color search patterns

On 3/7/09, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
> Match coloring is a major missing feature of git grep.  It makes reading
>  the output much easier and brings grep onto the same visual level as the
>  other colorized git commands.

"git --color test" did not colorize the result for me. "git --color --no-ext-grep test" did. Maybe you should ignore external grep unless it is explicitly requested, like in the last patch. I have very limited net access these days. Let's see if I can work out something for tomorrow, unless you beat me to it.

-- 
Duy
René Scharfe· Mar 10, 2009, 16:38 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH 0/5] grep: color search patterns

Nguyen Thai Ngoc Duy schrieb:
Show 10 quoted lines
> On 3/7/09, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:
>> Match coloring is a major missing feature of git grep.  It makes reading
>>  the output much easier and brings grep onto the same visual level as the
>>  other colorized git commands.
> 
> "git --color test" did not colorize the result for me. "git --color
> --no-ext-grep test" did. Maybe you should ignore external grep unless
> it is explicitly requested, like in the last patch. I have very
> limited net access these days. Let's see if I can work out something
> for tomorrow, unless you beat me to it.

I assume with "last patch" you mean your last patch, right? It automatically switched to internal grep if coloring was turned on.

Patch 5 of my series adds support for coloring external greps, but you have to explicitly specify the config option grep.color.external. It can be set by distributors in the system config file -- they should know which grep option is needed.

But I can see that a half-colored git grep can be confusing.
René
pclouds@gmail.com· Mar 16, 2009, 02:20 UTC · re: René Scharfe · lore

[PATCH] grep: prefer builtin over external one when coloring results

As far as I know, not all grep programs support coloring, so we should rely on builtin grep. If you want external grep, set color.grep.external to empty string.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 How about this?
 builtin-grep.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Show changes to builtin-grep.c +2 −0
diff --git a/builtin-grep.c b/builtin-grep.c
index 9e7e766..89489dd 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -825,6 +825,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 		}
 	}
 
+	if (opt.color && !opt.color_external)
+		builtin_grep = 1;
 	if (!opt.pattern_list)
 		die("no pattern given.");
 	if ((opt.regflags != REG_NEWLINE) && opt.fixed)

-- 
1.6.1.446.gc7851
René Scharfe· Mar 16, 2009, 17:31 UTC · re: pclouds@gmail.com · lore

Re: [PATCH] grep: prefer builtin over external one when coloring results

pclouds@gmail.com schrieb:
Show 24 quoted lines
> As far as I know, not all grep programs support coloring, so we should
> rely on builtin grep. If you want external grep, set
> color.grep.external to empty string.
> 
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  How about this?
> 
>  builtin-grep.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/builtin-grep.c b/builtin-grep.c
> index 9e7e766..89489dd 100644
> --- a/builtin-grep.c
> +++ b/builtin-grep.c
> @@ -825,6 +825,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		}
>  	}
>  
> +	if (opt.color && !opt.color_external)
> +		builtin_grep = 1;
>  	if (!opt.pattern_list)
>  		die("no pattern given.");
>  	if ((opt.regflags != REG_NEWLINE) && opt.fixed)
Makes sense, thanks!
René

← back to recent threads