[PATCHv2 0/3] grep color enhancements

[PATCHv2 0/3] grep color enhancements

[Mark Lodato]

This is a re-spin of the second half of Gmane 141186,  "color
enhancements, particularly for grep", which is currently in pu as
'ml/color-grep'.  The following changes were made, all to the second
patch:

* Fix the parsing of color.grep.  The old code caused a segfault if this
  option was set, and it also disallowed setting this option without
  a value.

* Color the filename in --name-only output.  I still don't like this,
  but I understand that makes the coloring more consistent.  As Junio
  said, if there's no strong argument one way or the other, it's
  probably best to follow GNU's lead.

* Style changes: Remove the cast to int for "rm_eo - rm_so", put else
  on same line as curly brace, and remove an unnecessary variable
  assignment.

* Asciidoc fixes: Use ";;" instead of ":::" for nested lists, and
  un-indent the last sentence for color.grep.<slot>.

Mark Lodato (3):
  Add GIT_COLOR_BOLD_* and GIT_COLOR_BG_*
  grep: Colorize filename, line number, and separator
  grep: Colorize selected, context, and function lines

 Documentation/config.txt |   26 ++++++++++++++++--
 builtin-grep.c           |   38 +++++++++++++++++++++-----
 color.h                  |   11 +++++++
 graph.c                  |   12 ++++----
 grep.c                   |   66 ++++++++++++++++++++++++++++++----------------
 grep.h                   |    6 ++++
 6 files changed, 119 insertions(+), 40 deletions(-)

[PATCHv2 1/3] Add GIT_COLOR_BOLD_* and GIT_COLOR_BG_*

[Mark Lodato]

Add GIT_COLOR_BOLD_* macros to set both bold and the color in one
sequence.  This saves two characters of output ("ESC [ m", minus ";")
and makes the code more readable.

Add the remaining GIT_COLOR_BG_* macros to make the list complete.
The white and black colors are not included since they look bad on most
terminals.

Signed-off-by: Mark Lodato <lodatom@gmail.com>
---
 builtin-grep.c |    2 +-
 color.h        |   11 +++++++++++
 graph.c        |   12 ++++++------
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/builtin-grep.c b/builtin-grep.c
index 2697957..2a876c4 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -847,7 +847,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	opt.regflags = REG_NEWLINE;
 	opt.max_depth = -1;
 
-	strcpy(opt.color_match, GIT_COLOR_RED GIT_COLOR_BOLD);
+	strcpy(opt.color_match, GIT_COLOR_BOLD_RED);
 	opt.color = -1;
 	git_config(grep_config, &opt);
 	if (opt.color == -1)
diff --git a/color.h b/color.h
index 3cb4b7f..bfeea1f 100644
--- a/color.h
+++ b/color.h
@@ -18,7 +18,18 @@
 #define GIT_COLOR_BLUE		"\033[34m"
 #define GIT_COLOR_MAGENTA	"\033[35m"
 #define GIT_COLOR_CYAN		"\033[36m"
+#define GIT_COLOR_BOLD_RED	"\033[1;31m"
+#define GIT_COLOR_BOLD_GREEN	"\033[1;32m"
+#define GIT_COLOR_BOLD_YELLOW	"\033[1;33m"
+#define GIT_COLOR_BOLD_BLUE	"\033[1;34m"
+#define GIT_COLOR_BOLD_MAGENTA	"\033[1;35m"
+#define GIT_COLOR_BOLD_CYAN	"\033[1;36m"
 #define GIT_COLOR_BG_RED	"\033[41m"
+#define GIT_COLOR_BG_GREEN	"\033[42m"
+#define GIT_COLOR_BG_YELLOW	"\033[43m"
+#define GIT_COLOR_BG_BLUE	"\033[44m"
+#define GIT_COLOR_BG_MAGENTA	"\033[45m"
+#define GIT_COLOR_BG_CYAN	"\033[46m"
 
 /*
  * This variable stores the value of color.ui
diff --git a/graph.c b/graph.c
index 6746d42..e6bbcaa 100644
--- a/graph.c
+++ b/graph.c
@@ -80,12 +80,12 @@ static char column_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_BLUE,
 	GIT_COLOR_MAGENTA,
 	GIT_COLOR_CYAN,
-	GIT_COLOR_BOLD GIT_COLOR_RED,
-	GIT_COLOR_BOLD GIT_COLOR_GREEN,
-	GIT_COLOR_BOLD GIT_COLOR_YELLOW,
-	GIT_COLOR_BOLD GIT_COLOR_BLUE,
-	GIT_COLOR_BOLD GIT_COLOR_MAGENTA,
-	GIT_COLOR_BOLD GIT_COLOR_CYAN,
+	GIT_COLOR_BOLD_RED,
+	GIT_COLOR_BOLD_GREEN,
+	GIT_COLOR_BOLD_YELLOW,
+	GIT_COLOR_BOLD_BLUE,
+	GIT_COLOR_BOLD_MAGENTA,
+	GIT_COLOR_BOLD_CYAN,
 };
 
 #define COLUMN_COLORS_MAX (ARRAY_SIZE(column_colors))
-- 
1.7.0

[PATCHv2 2/3] grep: Colorize filename, line number, and separator

[Mark Lodato]

Colorize the filename, line number, and separator in git grep output, as
GNU grep does.  The colors are customizable through color.grep.<slot>.
The default is to only color the separator (in cyan), since this gives
the biggest legibility increase without overwhelming the user with
colors.  GNU grep also defaults cyan for the separator, but defaults to
magenta for the filename and to green for the line number, as well.

There is one difference from GNU grep: When a binary file matches
without -a, GNU grep does not color the <file> in "Binary file <file>
matches", but we do.

Like GNU grep, if --null is given, the null separators are not colored.

For config.txt, use a a sub-list to describe the slots, rather than
a single paragraph with parentheses, since this is much more readable.

Remove the cast to int for `rm_eo - rm_so` since it is not necessary.

Signed-off-by: Mark Lodato <lodatom@gmail.com>
---
 Documentation/config.txt |   20 ++++++++++++++--
 builtin-grep.c           |   27 ++++++++++++++++------
 grep.c                   |   55 ++++++++++++++++++++++++++++-----------------
 grep.h                   |    3 ++
 4 files changed, 74 insertions(+), 31 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2dc3a05..711519e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -678,9 +678,23 @@ 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.match::
-	Use customized color for matches.  The value of this variable
-	may be specified as in color.branch.<slot>.
+color.grep.<slot>::
+	Use customized color for grep colorization.  `<slot>` specifies which
+	part of the line to use the specified color, and is one of
++
+--
+`filename`;;
+	filename prefix (when not using `-h`)
+`linenumber`;;
+	line number prefix (when using `-n`)
+`match`;;
+	matching text
+`separator`;;
+	separators between fields on a line (`:`, `-`, and `=`)
+	and between hunks (`--`)
+--
++
+The values of these variables may be specified as in color.branch.<slot>.
 
 color.interactive::
 	When set to `always`, always use colors for interactive prompts
diff --git a/builtin-grep.c b/builtin-grep.c
index 2a876c4..0c8115d 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -288,6 +288,7 @@ static int wait_all(void)
 static int grep_config(const char *var, const char *value, void *cb)
 {
 	struct grep_opt *opt = cb;
+	char *color = NULL;
 
 	switch (userdiff_config(var, value)) {
 	case 0: break;
@@ -295,17 +296,26 @@ static int grep_config(const char *var, const char *value, void *cb)
 	default: return 0;
 	}
 
-	if (!strcmp(var, "color.grep")) {
+	if (!strcmp(var, "color.grep"))
 		opt->color = git_config_colorbool(var, value, -1);
-		return 0;
-	}
-	if (!strcmp(var, "color.grep.match")) {
+	else if (!strcmp(var, "color.grep.filename"))
+		color = opt->color_filename;
+	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.separator"))
+		color = opt->color_sep;
+	else
+		return git_color_default_config(var, value, cb);
+	if (color) {
 		if (!value)
 			return config_error_nonbool(var);
-		color_parse(value, var, opt->color_match);
-		return 0;
+		color_parse(value, var, color);
+		if (!strcmp(color, GIT_COLOR_RESET))
+			color[0] = '\0';
 	}
-	return git_color_default_config(var, value, cb);
+	return 0;
 }
 
 /*
@@ -847,7 +857,10 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	opt.regflags = REG_NEWLINE;
 	opt.max_depth = -1;
 
+	strcpy(opt.color_filename, "");
+	strcpy(opt.color_lineno, "");
 	strcpy(opt.color_match, GIT_COLOR_BOLD_RED);
+	strcpy(opt.color_sep, GIT_COLOR_CYAN);
 	opt.color = -1;
 	git_config(grep_config, &opt);
 	if (opt.color == -1)
diff --git a/grep.c b/grep.c
index a0864f1..3a03060 100644
--- a/grep.c
+++ b/grep.c
@@ -270,9 +270,28 @@ static int word_char(char ch)
 	return isalnum(ch) || ch == '_';
 }
 
+static void output_color(struct grep_opt *opt, const void *data, size_t size,
+			 const char *color)
+{
+	if (opt->color && color && color[0]) {
+		opt->output(opt, color, strlen(color));
+		opt->output(opt, data, size);
+		opt->output(opt, GIT_COLOR_RESET, strlen(GIT_COLOR_RESET));
+	} else
+		opt->output(opt, data, size);
+}
+
+static void output_sep(struct grep_opt *opt, char sign)
+{
+	if (opt->null_following_name)
+		opt->output(opt, "\0", 1);
+	else
+		output_color(opt, &sign, 1, opt->color_sep);
+}
+
 static void show_name(struct grep_opt *opt, const char *name)
 {
-	opt->output(opt, name, strlen(name));
+	output_color(opt, name, strlen(name), opt->color_filename);
 	opt->output(opt, opt->null_following_name ? "\0" : "\n", 1);
 }
 
@@ -510,31 +529,27 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		      const char *name, unsigned lno, char sign)
 {
 	int rest = eol - bol;
-	char sign_str[1];
 
-	sign_str[0] = sign;
 	if (opt->pre_context || opt->post_context) {
 		if (opt->last_shown == 0) {
 			if (opt->show_hunk_mark)
-				opt->output(opt, "--\n", 3);
+				output_color(opt, "--\n", 3, opt->color_sep);
 			else
 				opt->show_hunk_mark = 1;
 		} else if (lno > opt->last_shown + 1)
-			opt->output(opt, "--\n", 3);
+			output_color(opt, "--\n", 3, opt->color_sep);
 	}
 	opt->last_shown = lno;
 
-	if (opt->null_following_name)
-		sign_str[0] = '\0';
 	if (opt->pathname) {
-		opt->output(opt, name, strlen(name));
-		opt->output(opt, sign_str, 1);
+		output_color(opt, name, strlen(name), opt->color_filename);
+		output_sep(opt, sign);
 	}
 	if (opt->linenum) {
 		char buf[32];
 		snprintf(buf, sizeof(buf), "%d", lno);
-		opt->output(opt, buf, strlen(buf));
-		opt->output(opt, sign_str, 1);
+		output_color(opt, buf, strlen(buf), opt->color_lineno);
+		output_sep(opt, sign);
 	}
 	if (opt->color) {
 		regmatch_t match;
@@ -548,12 +563,9 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 				break;
 
 			opt->output(opt, bol, match.rm_so);
-			opt->output(opt, opt->color_match,
-				    strlen(opt->color_match));
-			opt->output(opt, bol + match.rm_so,
-				    (int)(match.rm_eo - match.rm_so));
-			opt->output(opt, GIT_COLOR_RESET,
-				    strlen(GIT_COLOR_RESET));
+			output_color(opt, bol + match.rm_so,
+				     match.rm_eo - match.rm_so,
+				     opt->color_match);
 			bol += match.rm_eo;
 			rest -= match.rm_eo;
 			eflags = REG_NOTBOL;
@@ -823,7 +835,8 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 				return 1;
 			if (binary_match_only) {
 				opt->output(opt, "Binary file ", 12);
-				opt->output(opt, name, strlen(name));
+				output_color(opt, name, strlen(name),
+					     opt->color_filename);
 				opt->output(opt, " matches\n", 9);
 				return 1;
 			}
@@ -882,9 +895,9 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
 	 */
 	if (opt->count && count) {
 		char buf[32];
-		opt->output(opt, name, strlen(name));
-		snprintf(buf, sizeof(buf), "%c%u\n",
-			 opt->null_following_name ? '\0' : ':', count);
+		output_color(opt, name, strlen(name), opt->color_filename);
+		output_sep(opt, ':');
+		snprintf(buf, sizeof(buf), "%u\n", count);
 		opt->output(opt, buf, strlen(buf));
 	}
 	return !!last_hit;
diff --git a/grep.h b/grep.h
index 9703087..36919ee 100644
--- a/grep.h
+++ b/grep.h
@@ -84,7 +84,10 @@ struct grep_opt {
 	int color;
 	int max_depth;
 	int funcname;
+	char color_filename[COLOR_MAXLEN];
+	char color_lineno[COLOR_MAXLEN];
 	char color_match[COLOR_MAXLEN];
+	char color_sep[COLOR_MAXLEN];
 	int regflags;
 	unsigned pre_context;
 	unsigned post_context;
-- 
1.7.0

[PATCHv2 3/3] grep: Colorize selected, context, and function lines

[Mark Lodato]

Colorize non-matching text of selected lines, context lines, and
function name lines.  The default for all three is no color, but they
can be configured using color.grep.<slot>.  The first two are similar
to the corresponding options in GNU grep, except that GNU grep applies
the color to the entire line, not just non-matching text.

Signed-off-by: Mark Lodato <lodatom@gmail.com>
---
 Documentation/config.txt |    6 ++++++
 builtin-grep.c           |    9 +++++++++
 grep.c                   |   11 +++++++++--
 grep.h                   |    3 +++
 4 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 711519e..38bc729 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -683,12 +683,18 @@ color.grep.<slot>::
 	part of the line to use the specified color, and is one of
 +
 --
+`context`;;
+	non-matching text in context lines (when using `-A`, `-B`, or `-C`)
 `filename`;;
 	filename prefix (when not using `-h`)
+`function`;;
+	function name lines (when using `-p`)
 `linenumber`;;
 	line number prefix (when using `-n`)
 `match`;;
 	matching text
+`selected`;;
+	non-matching text in selected lines
 `separator`;;
 	separators between fields on a line (`:`, `-`, and `=`)
 	and between hunks (`--`)
diff --git a/builtin-grep.c b/builtin-grep.c
index 0c8115d..e423eac 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -298,12 +298,18 @@ static int grep_config(const char *var, const char *value, void *cb)
 
 	if (!strcmp(var, "color.grep"))
 		opt->color = git_config_colorbool(var, value, -1);
+	else if (!strcmp(var, "color.grep.context"))
+		color = opt->color_context;
 	else if (!strcmp(var, "color.grep.filename"))
 		color = opt->color_filename;
+	else if (!strcmp(var, "color.grep.function"))
+		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.selected"))
+		color = opt->color_selected;
 	else if (!strcmp(var, "color.grep.separator"))
 		color = opt->color_sep;
 	else
@@ -857,9 +863,12 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	opt.regflags = REG_NEWLINE;
 	opt.max_depth = -1;
 
+	strcpy(opt.color_context, "");
 	strcpy(opt.color_filename, "");
+	strcpy(opt.color_function, "");
 	strcpy(opt.color_lineno, "");
 	strcpy(opt.color_match, GIT_COLOR_BOLD_RED);
+	strcpy(opt.color_selected, "");
 	strcpy(opt.color_sep, GIT_COLOR_CYAN);
 	opt.color = -1;
 	git_config(grep_config, &opt);
diff --git a/grep.c b/grep.c
index 3a03060..1e0b1e6 100644
--- a/grep.c
+++ b/grep.c
@@ -529,6 +529,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;
 
 	if (opt->pre_context || opt->post_context) {
 		if (opt->last_shown == 0) {
@@ -557,12 +558,18 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		int ch = *eol;
 		int eflags = 0;
 
+		if (sign == ':')
+			line_color = opt->color_selected;
+		else if (sign == '-')
+			line_color = opt->color_context;
+		else if (sign == '=')
+			line_color = opt->color_function;
 		*eol = '\0';
 		while (next_match(opt, bol, eol, ctx, &match, eflags)) {
 			if (match.rm_so == match.rm_eo)
 				break;
 
-			opt->output(opt, bol, match.rm_so);
+			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);
@@ -572,7 +579,7 @@ static void show_line(struct grep_opt *opt, char *bol, char *eol,
 		}
 		*eol = ch;
 	}
-	opt->output(opt, bol, rest);
+	output_color(opt, bol, rest, line_color);
 	opt->output(opt, "\n", 1);
 }
 
diff --git a/grep.h b/grep.h
index 36919ee..2c4bdac 100644
--- a/grep.h
+++ b/grep.h
@@ -84,9 +84,12 @@ struct grep_opt {
 	int color;
 	int max_depth;
 	int funcname;
+	char color_context[COLOR_MAXLEN];
 	char color_filename[COLOR_MAXLEN];
+	char color_function[COLOR_MAXLEN];
 	char color_lineno[COLOR_MAXLEN];
 	char color_match[COLOR_MAXLEN];
+	char color_selected[COLOR_MAXLEN];
 	char color_sep[COLOR_MAXLEN];
 	int regflags;
 	unsigned pre_context;
-- 
1.7.0

Re: [PATCHv2 2/3] grep: Colorize filename, line number, and separator

[Junio C Hamano]

Mark Lodato <lodatom@gmail.com> writes:

> +	if (color) {
>  		if (!value)
>  			return config_error_nonbool(var);
> -		color_parse(value, var, opt->color_match);
> -		return 0;
> +		color_parse(value, var, color);
> +		if (!strcmp(color, GIT_COLOR_RESET))
> +			color[0] = '\0';

I don't know this "optimization" is warranted.  I can understand that you
are trying to help the user to save a handful of useless bytes per line of
output in the normal case, but doesn't "color.cmd.foo = normal" set the
value to an empty string already if that is what the user wants?

The user may know about his contents better than this code does; perhaps
the payload sometimes has funny ANSI sequence in it and the user may be
trying to explicitly reset at the beginning of the line for the "plain"
part of the output by saying "reset".  Wouldn't this make it impossible to
do so?

Re: [PATCHv2 2/3] grep: Colorize filename, line number, and separator

[Mark Lodato]

On Sun, Mar 7, 2010 at 2:02 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Mark Lodato <lodatom@gmail.com> writes:
>
>> +     if (color) {
>>               if (!value)
>>                       return config_error_nonbool(var);
>> -             color_parse(value, var, opt->color_match);
>> -             return 0;
>> +             color_parse(value, var, color);
>> +             if (!strcmp(color, GIT_COLOR_RESET))
>> +                     color[0] = '\0';
>
> I don't know this "optimization" is warranted.  I can understand that you
> are trying to help the user to save a handful of useless bytes per line of
> output in the normal case, but doesn't "color.cmd.foo = normal" set the
> value to an empty string already if that is what the user wants?

Ah, I didn't know about the "normal" color.  I tried using a blank
value, which defaulted to "reset", so I thought there was no way to
say "no ANSI sequence".  The above was my workaround to get no
sequence.  Since the "normal" color exists, this workaround should be
removed.

The squash-able patch below removes the workaround, and also fixes
another bug: color was not reset at the end of the line for hunk
separators ("--").

As an aside, perhaps a blank color value should default to no ANSI
sequence, rather than the reset sequence?


diff --git i/builtin-grep.c w/builtin-grep.c
index e423eac..c519fcf 100644
--- i/builtin-grep.c
+++ w/builtin-grep.c
@@ -318,8 +318,6 @@
 		if (!value)
 			return config_error_nonbool(var);
 		color_parse(value, var, color);
-		if (!strcmp(color, GIT_COLOR_RESET))
-			color[0] = '\0';
 	}
 	return 0;
 }
diff --git i/grep.c w/grep.c
index 1e0b1e6..b641305 100644
--- i/grep.c
+++ w/grep.c
@@ -533,12 +533,15 @@

 	if (opt->pre_context || opt->post_context) {
 		if (opt->last_shown == 0) {
-			if (opt->show_hunk_mark)
-				output_color(opt, "--\n", 3, opt->color_sep);
-			else
+			if (opt->show_hunk_mark) {
+				output_color(opt, "--", 2, opt->color_sep);
+				opt->output(opt, "\n", 1);
+			} else
 				opt->show_hunk_mark = 1;
-		} else if (lno > opt->last_shown + 1)
-			output_color(opt, "--\n", 3, opt->color_sep);
+		} else if (lno > opt->last_shown + 1) {
+			output_color(opt, "--", 2, opt->color_sep);
+			opt->output(opt, "\n", 1);
+		}
 	}
 	opt->last_shown = lno;