threads / patch / 13649

patchgit-grep: add --color to highlight matches

Subject: [PATCH] git-grep: add --color to highlight matches

## tl;dr

10 messages between May 24, 2008 and May 26, 2008. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

Nguyễn Thái Ngọc Duy· May 24, 2008, 04:31 UTC · lore
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/git-grep.txt |    4 +++
 builtin-grep.c             |    6 +++-
 grep.c                     |   62 +++++++++++++++++++++++++++++++++++++++++---
 grep.h                     |    1 +
 4 files changed, 68 insertions(+), 5 deletions(-)
Show changes to 4 files +68 −5

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

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index a97f055..20ef098 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -16,6 +16,7 @@ SYNOPSIS
 	   [-F | --fixed-strings] [-n]
 	   [-l | --files-with-matches] [-L | --files-without-match]
 	   [-c | --count] [--all-match]
+	   [--color]
 	   [-A <post-context>] [-B <pre-context>] [-C <context>]
 	   [-f <file>] [-e] <pattern>
 	   [--and|--or|--not|(|)|-e <pattern>...] [<tree>...]
@@ -85,6 +86,9 @@ OPTIONS
 	Instead of showing every matched line, show the number of
 	lines that match.
 
+--color::
+	Show colored matches.
+
 -[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 ef29910..118ec32 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -386,7 +386,7 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)
 	 * we grep through the checked-out files. It tends to
 	 * be a lot more optimized
 	 */
-	if (!cached) {
+	if (!cached && !opt->color) {
 		hit = external_grep(opt, paths, cached);
 		if (hit >= 0)
 			return hit;
@@ -708,6 +708,10 @@ 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("--", arg)) {
 			/* later processing wants to have this at argv[1] */
 			argv--;
diff --git a/grep.c b/grep.c
index f67d671..7cc05b0 100644
--- a/grep.c
+++ b/grep.c
@@ -247,7 +247,10 @@ static int fixmatch(const char *pattern, char *line, regmatch_t *match)
 	}
 }
 
-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_1(struct grep_opt *opt, struct grep_pat *p,
+			       char *bol, char *eol,
+			       enum grep_context ctx, int eflags,
+			       int *rm_so, int *rm_eo)
 {
 	int hit = 0;
 	int at_true_bol = 1;
@@ -261,7 +264,7 @@ static int match_one_pattern(struct grep_opt *opt, struct grep_pat *p, char *bol
 	if (!opt->fixed) {
 		regex_t *exp = &p->regexp;
 		hit = !regexec(exp, bol, ARRAY_SIZE(pmatch),
-			       pmatch, 0);
+			       pmatch, eflags);
 	}
 	else {
 		hit = !fixmatch(p->pattern, bol, pmatch);
@@ -298,9 +301,20 @@ static int match_one_pattern(struct grep_opt *opt, struct grep_pat *p, char *bol
 			goto again;
 		}
 	}
+	if (hit) {
+		if (rm_so)
+			*rm_so = pmatch[0].rm_so;
+		if (rm_eo)
+			*rm_eo = pmatch[0].rm_eo;
+	}
 	return hit;
 }
 
+static int match_one_pattern(struct grep_opt *opt, struct grep_pat *p, char *bol, char *eol, enum grep_context ctx)
+{
+	return match_one_pattern_1(opt, p, bol, eol, ctx, 0, NULL, NULL);
+}
+
 static int match_expr_eval(struct grep_opt *o,
 			   struct grep_expr *x,
 			   char *bol, char *eol,
@@ -365,6 +379,42 @@ static int match_line(struct grep_opt *opt, char *bol, char *eol,
 	return 0;
 }
 
+static void show_line_colored(struct grep_opt *opt, char *bol, char *eol,
+			      const char *name, unsigned lno, char sign)
+{
+	struct grep_pat *p;
+	int rm_so, rm_eo, eflags = 0;
+	int ch;
+
+	if (opt->pathname)
+		printf("%s%c", name, sign);
+	if (opt->linenum)
+		printf("%d%c", lno, sign);
+
+	ch = *eol;
+	*eol = 0;
+	while (bol < eol) {
+		for (p = opt->pattern_list; p; p = p->next) {
+			if (match_one_pattern_1(opt, p, bol, eol, GREP_CONTEXT_BODY, eflags, &rm_so, &rm_eo))
+				break;
+		}
+
+		/* No match, break the loop */
+		if (!p ||
+		    (rm_so < 0) || (eol - bol) <= rm_so ||
+		    (rm_eo < 0) || (eol - bol) < rm_eo)
+			break;
+
+		printf("%.*s\033[31m\033[1m%.*s\033[m",
+		       rm_so, bol,
+		       rm_eo-rm_so, bol+rm_so);
+		bol += rm_eo;
+		eflags = REG_NOTBOL;
+	}
+	printf("%s\n", bol);
+	*eol = ch;
+}
+
 static int grep_buffer_1(struct grep_opt *opt, const char *name,
 			 char *buf, unsigned long size, int collect_hits)
 {
@@ -466,8 +516,12 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 			}
 			if (last_shown && lno != last_shown + 1)
 				printf(hunk_mark);
-			if (!opt->count)
-				show_line(opt, bol, eol, name, lno, ':');
+			if (!opt->count) {
+				if (opt->color)
+					show_line_colored(opt, bol, eol, name, lno, ':');
+				else
+					show_line(opt, bol, eol, name, lno, ':');
+			}
 			last_shown = last_hit = lno;
 		}
 		else if (last_hit &&
diff --git a/grep.h b/grep.h
index d252dd2..979f7d0 100644
--- a/grep.h
+++ b/grep.h
@@ -68,6 +68,7 @@ struct grep_opt {
 	unsigned extended:1;
 	unsigned relative:1;
 	unsigned pathname:1;
+	unsigned color:1;
 	int regflags;
 	unsigned pre_context;
 	unsigned post_context;
-- 
1.5.5.GIT
Jakub Narebski· May 24, 2008, 08:28 UTC · re: Nguyễn Thái Ngọc Duy · lore

Re: [PATCH] git-grep: add --color to highlight matches

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> +--color::
> +	Show colored matches.
> +
Why not borrow from GNU grep manpage?
   --colour[=WHEN], --color[=WHEN]
      Surround  the  matching  string  with  the marker find in GREP_COLOR
      environment variable. WHEN may be 'never', 'always', or 'auto'

We probably would also want it configurable (via config variables), as `color.grep' I guess.

I think compatibility with grep (using GREP_COLOR) would be good idea, and easily implemented.

-- 
Jakub Narebski
Poland
ShadeHawk on #git
Nguyen Thai Ngoc Duy· May 24, 2008, 09:20 UTC · re: Jakub Narebski · lore

Re: [PATCH] git-grep: add --color to highlight matches

On Sat, May 24, 2008 at 3:28 PM, Jakub Narebski <jnareb@gmail.com> wrote:
Show 14 quoted lines
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> +--color::
>> +     Show colored matches.
>> +
>
> Why not borrow from GNU grep manpage?
>
>   --colour[=WHEN], --color[=WHEN]
>      Surround  the  matching  string  with  the marker find in GREP_COLOR
>      environment variable. WHEN may be 'never', 'always', or 'auto'
>
> We probably would also want it configurable (via config variables),
> as `color.grep' I guess.

Yes. But this patch was really to scratch my itch and I felt good after having it colorized so did not bother with configuration system. I will probably make it a config var when I get tired of typing --color too often ;)

> I think compatibility with grep (using GREP_COLOR) would be good idea,
> and easily implemented.
I had no idea that variable existed.
Show 5 quoted lines
> --
> Jakub Narebski
> Poland
> ShadeHawk on #git
>
-- 
Duy
Jakub Narebski· May 24, 2008, 10:59 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH] git-grep: add --color to highlight matches

On Sat, 24 May 2008, Nguyen Thai Ngoc Duy wrote:
Show 15 quoted lines
> On Sat, May 24, 2008 at 3:28 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>>
>>> +--color::
>>> +     Show colored matches.
>>> +
>>
>> Why not borrow from GNU grep manpage?
>>
>>   --colour[=WHEN], --color[=WHEN]
>>      Surround  the  matching  string  with  the marker find in GREP_COLOR
>>      environment variable. WHEN may be 'never', 'always', or 'auto'
>>
>> We probably would also want it configurable (via config variables),
>> as `color.grep' I guess.
And color.grep.<slot>, where slot can be 'match', and probably later also
'context' (I don't remember if git-grep implements -C<num> option...).
 
> Yes. But this patch was really to scratch my itch and I felt good
> after having it colorized so did not bother with configuration system.
> I will probably make it a config var when I get tired of typing
> --color too often ;)

Thanks for sharing, although I guess Junio would prefer more complete patch...

>> I think compatibility with grep (using GREP_COLOR) would be good idea,
>> and easily implemented.
> 
> I had no idea that variable existed.

Well, I have though that git always use external grep, so this would be for free. After reading the patch more carefully it seems like it isn't so.

Still, perhaps that would be a good idea, but I guess for later patch.
-- 
Jakub Narebski
Poland
Andreas Ericsson· May 26, 2008, 08:55 UTC · re: Jakub Narebski · lore

Re: [PATCH] git-grep: add --color to highlight matches

Jakub Narebski wrote:
Show 13 quoted lines
> On Sat, 24 May 2008, Nguyen Thai Ngoc Duy wrote:
>> On Sat, May 24, 2008 at 3:28 PM, Jakub Narebski <jnareb@gmail.com> wrote:
>>> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> 
>>> I think compatibility with grep (using GREP_COLOR) would be good idea,
>>> and easily implemented.
>> I had no idea that variable existed.
> 
> Well, I have though that git always use external grep, so this would
> be for free. After reading the patch more carefully it seems like it
> isn't so.
> 
> Still, perhaps that would be a good idea, but I guess for later patch.

I can't help but think that colorization for external-grep systems only would still be worthwhile, as the majority of git users are on such systems. It'd still be a separate implementation from adding it to the internal grep functionality anyways, so it's not as if work would be lost by going half the way here.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231
Johannes Schindelin· May 26, 2008, 10:16 UTC · re: Andreas Ericsson · lore

Re: [PATCH] git-grep: add --color to highlight matches

Hi,
On Mon, 26 May 2008, Andreas Ericsson wrote:
Show 5 quoted lines
> I can't help but think that colorization for external-grep systems only 
> would still be worthwhile, as the majority of git users are on such 
> systems. It'd still be a separate implementation from adding it to the 
> internal grep functionality anyways, so it's not as if work would be 
> lost by going half the way here.

Huh? I thought that the external grep is only a matter of fallback for performance reasons (if the files are already checked out), or if the user specifies the external grep explicitely.

At least for the platform we bend over most, our beloved anachronistic M$ Windows, I do not recall forcing external grep.

Besides, it would be a kludge at best to work _twice_ to find out the search terms, once in the external grep, and a second time in the coloring code. So I think it should not be done.

Ciao, Dscho

Nguyen Thai Ngoc Duy· May 26, 2008, 10:57 UTC · re: Johannes Schindelin · lore

Re: [PATCH] git-grep: add --color to highlight matches

On Mon, May 26, 2008 at 5:16 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> Besides, it would be a kludge at best to work _twice_ to find out the
> search terms, once in the external grep, and a second time in the coloring
> code.  So I think it should not be done.

I think if it's GNU grep, just passing it --color, it will grep and colorize search terms in one turn.

-- 
Duy
Johannes Schindelin· May 26, 2008, 11:00 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH] git-grep: add --color to highlight matches

Hi,
On Mon, 26 May 2008, Nguyen Thai Ngoc Duy wrote:
Show 8 quoted lines
> On Mon, May 26, 2008 at 5:16 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Besides, it would be a kludge at best to work _twice_ to find out the
> > search terms, once in the external grep, and a second time in the coloring
> > code.  So I think it should not be done.
> 
> I think if it's GNU grep, just passing it --color, it will grep and
> colorize search terms in one turn.
And what tells you that the called grep is GNU grep?

Ciao, Dscho

Nguyen Thai Ngoc Duy· May 26, 2008, 11:07 UTC · re: Johannes Schindelin · lore

Re: [PATCH] git-grep: add --color to highlight matches

On Mon, May 26, 2008 at 6:00 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 14 quoted lines
> Hi,
>
> On Mon, 26 May 2008, Nguyen Thai Ngoc Duy wrote:
>
>> On Mon, May 26, 2008 at 5:16 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> > Besides, it would be a kludge at best to work _twice_ to find out the
>> > search terms, once in the external grep, and a second time in the coloring
>> > code.  So I think it should not be done.
>>
>> I think if it's GNU grep, just passing it --color, it will grep and
>> colorize search terms in one turn.
>
> And what tells you that the called grep is GNU grep?

A newly added macro like HAS_GNU_GREP? Granted it won't work all the time. The user who set the macro should know what he is doing. This approach is IMHO fine as long as we don't allow color customization.

-- 
Duy
Johannes Schindelin· May 26, 2008, 11:29 UTC · re: Nguyen Thai Ngoc Duy · lore

Re: [PATCH] git-grep: add --color to highlight matches

Hi,
On Mon, 26 May 2008, Nguyen Thai Ngoc Duy wrote:
Show 19 quoted lines
> On Mon, May 26, 2008 at 6:00 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > On Mon, 26 May 2008, Nguyen Thai Ngoc Duy wrote:
> >
> >> On Mon, May 26, 2008 at 5:16 PM, Johannes Schindelin
> >> <Johannes.Schindelin@gmx.de> wrote:
> >> > Besides, it would be a kludge at best to work _twice_ to find out 
> >> > the search terms, once in the external grep, and a second time in 
> >> > the coloring code.  So I think it should not be done.
> >>
> >> I think if it's GNU grep, just passing it --color, it will grep and 
> >> colorize search terms in one turn.
> >
> > And what tells you that the called grep is GNU grep?
> 
> A newly added macro like HAS_GNU_GREP? Granted it won't work all the
> time. The user who set the macro should know what he is doing. This
> approach is IMHO fine as long as we don't allow color customization.

IMO this is just a kludge that is not even necessary, because the whole notion of relying on an external grep's colorization is crap.

The OP has a patch that adds colorization to Git's internal grep code. The user specifies "--color" with grep? Fine, then it's no external grep. Full stop.

Just like the original patch implements it. Nice, simple, and easy. What is so wrong with that?

Ciao, Dscho

← back to recent threads