[PATCH 0/5] color enhancements, particularly for grep

[PATCH 0/5] color enhancements, particularly for grep

[Mark Lodato]

The main purpose of this patch series is to add color to git grep a la
GNU grep.  The only change to the default is to colorize the separator
between filename, line number, and match (':', '-', or '=') and between
hunks ('--').  This improves readability immensely without being
distracting.  However, the filename, line number, function line (-p),
and non-matching text can also be colored, if desired.

The first three patches are each independent of any other patch, but
they seem like a good idea.

Mark Lodato (5):
  Allow explicit ANSI codes for colors
  Add GIT_COLOR_BOLD_* and GIT_COLOR_BG_*
  Remove reference to GREP_COLORS from documentation
  grep: Colorize filename, line number, and separator
  grep: Colorize selected, context, and function lines

 Documentation/config.txt |   32 +++++++++++++++++++---
 builtin-grep.c           |   42 +++++++++++++++++++++-------
 color.c                  |   16 +++++++++++
 color.h                  |   11 +++++++
 graph.c                  |   12 ++++----
 grep.c                   |   66 ++++++++++++++++++++++++++++++---------------
 grep.h                   |    6 ++++
 t/t4026-color.sh         |   18 ++++++++++++
 8 files changed, 159 insertions(+), 44 deletions(-)

[PATCH 1/5] Allow explicit ANSI codes for colors

[Mark Lodato]

Allow explicit ANSI codes to be used in configuration options expecting
a color.  The form is "[...m", where "..." are characters in the ASCII
range 0x30 to 0x3f.  This allows users to specify more complex colors
(generically, SGR attributes) than our color language allows.  For
example, to get blinking, bold, underlined, italic, red text,
use "[5;1;4;3;31m".

Signed-off-by: Mark Lodato <lodatom@gmail.com>
---
 Documentation/config.txt |    4 ++++
 color.c                  |   16 ++++++++++++++++
 t/t4026-color.sh         |   18 ++++++++++++++++++
 3 files changed, 38 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 664de6b..fed18cb 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -663,6 +663,10 @@ accepted are `normal`, `black`, `red`, `green`, `yellow`, `blue`,
 `blink` and `reverse`.  The first color given is the foreground; the
 second is the background.  The position of the attribute, if any,
 doesn't matter.
++
+Alternatively, a raw ANSI color code (SGR attribute) may be specified, in the
+form `[...m` (no escape character).  The `...` is a set of characters in the
+ASCII range 0x30 to 0x3f.
 
 color.diff::
 	When set to `always`, always use colors in patch.
diff --git a/color.c b/color.c
index 62977f4..1b42d9a 100644
--- a/color.c
+++ b/color.c
@@ -56,6 +56,22 @@ void color_parse_mem(const char *value, int value_len, const char *var,
 		return;
 	}
 
+	/* If "[...m", use this as the attribute string directly */
+	if (value_len >= 2 && value[0] == '[' && value[value_len-1] == 'm') {
+		int i;
+		for (i = 1; i < value_len-1; i++)
+			if (value[i] < 0x30 || value[i] >= 0x40)
+				goto parse;
+		if (value_len + 2 > COLOR_MAXLEN)
+			die("color value '%.*s' too long for variable '%s'",
+			    value_len, value, var);
+		*dst = '\033';
+		memcpy(dst+1, value, value_len);
+		*(dst+1+value_len) = 0;
+		return;
+	}
+
+parse:
 	/* [fg [bg]] [attr] */
 	while (len > 0) {
 		const char *word = ptr;
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index 5ade44c..754716f 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -46,6 +46,14 @@ test_expect_success '256 colors' '
 	color "254 bold 255" "[1;38;5;254;48;5;255m"
 '
 
+test_expect_success 'explicit attribute' '
+	color "[123;456;7890m" "[123;456;7890m"
+'
+
+test_expect_success 'explicit attribute maximum length' '
+	color "[00000000000000000000m" "[00000000000000000000m"
+'
+
 test_expect_success 'color too small' '
 	invalid_color "-2"
 '
@@ -66,6 +74,16 @@ test_expect_success 'extra character after attribute' '
 	invalid_color "dimX"
 '
 
+test_expect_success 'explicit attribute invalid characters' '
+	invalid_color "[/m" &&
+	invalid_color "[@m" &&
+	invalid_color "[mm"
+'
+
+test_expect_success 'explicit attribute too long' '
+	invalid_color "[000000000000000000000m"
+'
+
 test_expect_success 'unknown color slots are ignored (diff)' '
 	git config --unset diff.color.new
 	git config color.diff.nosuchslotwilleverbedefined white &&
-- 
1.7.0

Re: [PATCH 1/5] Allow explicit ANSI codes for colors

[Jeff King]

On Fri, Feb 26, 2010 at 11:57:46PM -0500, Mark Lodato wrote:

> Allow explicit ANSI codes to be used in configuration options expecting
> a color.  The form is "[...m", where "..." are characters in the ASCII
> range 0x30 to 0x3f.  This allows users to specify more complex colors
> (generically, SGR attributes) than our color language allows.  For
> example, to get blinking, bold, underlined, italic, red text,
> use "[5;1;4;3;31m".

I am not against this patch if it gets us some flexibility that is not
otherwise easy to attain, but wouldn't it be more user friendly for us
to support "red blink bold ul italic"? AFAICT, the only things standing
the way of that are:

  - we don't support the italic attribute yet (are there are a lot of
    others that we are missing?)

  - the parser in color_parse_mem already understands how to parse
    multiple attributes, but it just complains after the first one

-Peff

Re: [PATCH 1/5] Allow explicit ANSI codes for colors

[Mark Lodato]

On Sat, Feb 27, 2010 at 3:51 AM, Jeff King <peff@peff.net> wrote:
> I am not against this patch if it gets us some flexibility that is not
> otherwise easy to attain,

Besides disallowing multiple attributes (e.g. bold blink), the current
parser does not have a way to specify colors for 16-color mode colors
8-15, 256-color mode colors 0-7, or any 88-color mode colors.  There
are also other esoteric attributes [1] that some user might want to
use, such as italic or franktur.  I don't know if anyone will ever use
this feature, but it wasn't hard to implement.

> but wouldn't it be more user friendly for us
> to support "red blink bold ul italic"?

Yes, I think this should be done whether or not the patch in question
is accepted.

> AFAICT, the only things standing
> the way of that are:
>
>  - we don't support the italic attribute yet (are there are a lot of
>    others that we are missing?)

Wikipedia [1] lists a whole bunch of codes, including italic, but I
doubt anyone uses them.  My thought was if someone really wanted to
use one of these obscure codes, they could do it with the patch in
question.  I don't think it's worth allowing users to type "italic".

>  - the parser in color_parse_mem already understands how to parse
>    multiple attributes, but it just complains after the first one

It seems like this restriction should be lifted.  However, if this is
done, then COLOR_MAXLEN should be increased to 32 or so, and there
must be explicit checks to make sure the buffer does not overflow.
Technically, VT500 terminals accept up to 16 parameters up to 5 digits
each [2], which would be 98 bytes, but this is overkill.

[1] http://en.wikipedia.org/wiki/ANSI_escape_code
[2] http://vt100.net/emu/dec_ansi_parser#ACPAR

Re: [PATCH 1/5] Allow explicit ANSI codes for colors

[Junio C Hamano]

Mark Lodato <lodatom@gmail.com> writes:

> On Sat, Feb 27, 2010 at 3:51 AM, Jeff King <peff@peff.net> wrote:
>> I am not against this patch if it gets us some flexibility that is not
>> otherwise easy to attain,
>
> Besides disallowing multiple attributes (e.g. bold blink), the current
> parser does not have a way to specify colors for 16-color mode colors
> 8-15, 256-color mode colors 0-7, or any 88-color mode colors.  There
> are also other esoteric attributes [1] that some user might want to
> use, such as italic or franktur.  I don't know if anyone will ever use
> this feature, but it wasn't hard to implement.

The purist side of me has been hoping that we could later wean off this
ANSI centric view of the terminal attribute handling and move us to
something based on terminfo.  This patch makes it even harder by going
quite the opposite way.

But the pragmatic side of me has long held a feeling that nobody who would
use git uses real terminals these days anymore, and there is no terminal
emulator that does not grok ANSI sequences.  msysgit folks have even done
their own ANSI color emulation in their "Console" interface layer, so that
may be another reason that we are practically married to ANSI sequence and
there is not much gained by introducing terminfo as another layer of
abstraction to build GIT_COLOR_* on top of.

What I am saying is that the purist in me actively hates [PATCH 1/5], but
the pragmatist in me admits it would not hurt in practice.

>> but wouldn't it be more user friendly for us
>> to support "red blink bold ul italic"?
>
> Yes, I think this should be done whether or not the patch in question
> is accepted.

Hmm, I do not care much about italic, blink nor ul, but perhaps other
people do.  Combining attributes like "reverse bold" would probably make
sense.

 color.c |   35 +++++++++++++++++++++++++++--------
 1 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/color.c b/color.c
index 62977f4..f210d94 100644
--- a/color.c
+++ b/color.c
@@ -47,7 +47,8 @@ void color_parse_mem(const char *value, int value_len, const char *var,
 {
 	const char *ptr = value;
 	int len = value_len;
-	int attr = -1;
+	int attr[20];
+	int attr_idx = 0;
 	int fg = -2;
 	int bg = -2;
 
@@ -56,7 +57,7 @@ void color_parse_mem(const char *value, int value_len, const char *var,
 		return;
 	}
 
-	/* [fg [bg]] [attr] */
+	/* [fg [bg]] [attr]... */
 	while (len > 0) {
 		const char *word = ptr;
 		int val, wordlen = 0;
@@ -85,19 +86,37 @@ void color_parse_mem(const char *value, int value_len, const char *var,
 			goto bad;
 		}
 		val = parse_attr(word, wordlen);
-		if (val < 0 || attr != -1)
+		if (0 <= val && attr_idx < ARRAY_SIZE(attr))
+			attr[attr_idx++] = val;
+		else
 			goto bad;
-		attr = val;
 	}
 
-	if (attr >= 0 || fg >= 0 || bg >= 0) {
+	if (attr_idx > 0 || fg >= 0 || bg >= 0) {
 		int sep = 0;
+		int i;
+
+		if (COLOR_MAXLEN <=
+		    /* Number of bytes to denote colors and attributes */
+		    (attr_idx
+		     + (fg < 0 ? 0 :
+			((fg < 8) ? 2 : 8)) /* "3x" or "38;5;xxx" */
+		     + (bg < 0 ? 0 :
+			((bg < 8) ? 2 : 8)) /* "4x" or "48;5;xxx" */
+			    ) +
+		    /* Number of semicolons between the above */
+		    (attr_idx + (0 <= fg) + (0 <= bg) - 1) +
+		    /* ESC '[', terminating 'm' and NUL */
+		    4)
+			goto bad;
 
 		*dst++ = '\033';
 		*dst++ = '[';
-		if (attr >= 0) {
-			*dst++ = '0' + attr;
-			sep++;
+
+		for (i = 0; i < attr_idx; i++) {
+			if (sep++)
+				*dst++ = ';';
+			*dst++ = '0' + attr[i];
 		}
 		if (fg >= 0) {
 			if (sep++)

[PATCH] color: allow multiple attributes

[Junio C Hamano]

In configuration files (and "git config --color" command line), we
supported one and only one attribute after foreground and background
color.  Accept combinations of attributes, e.g.

    [diff.color]
            old = red reverse bold

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
  Junio C Hamano <gitster@pobox.com> writes:

  >>> but wouldn't it be more user friendly for us
  >>> to support "red blink bold ul italic"?
  >>
  >> Yes, I think this should be done whether or not the patch in question
  >> is accepted.

  This time with a bit of test updates as well for real inclusion.

  Also I realized that we can stuff them in an unsigned flag word as
  bitfields ("red bold" and "red bold bold bold" would give the same
  boldness anyway) to lift the artificial limit of number of attribute
  words.

 color.c          |   47 +++++++++++++++++++++++++++++++++++++++--------
 t/t4026-color.sh |   15 +++++++++++----
 2 files changed, 50 insertions(+), 12 deletions(-)

diff --git a/color.c b/color.c
index db4dccf..17eb3ec 100644
--- a/color.c
+++ b/color.c
@@ -44,12 +44,23 @@ void color_parse(const char *value, const char *var, char *dst)
 	color_parse_mem(value, strlen(value), var, dst);
 }
 
+static int count_bits(unsigned flag)
+{
+	int cnt = 0;
+	while (flag) {
+		if (flag & 01)
+			cnt++;
+		flag >>= 1;
+	}
+	return cnt;
+}
+
 void color_parse_mem(const char *value, int value_len, const char *var,
 		char *dst)
 {
 	const char *ptr = value;
 	int len = value_len;
-	int attr = -1;
+	unsigned int attr = 0;
 	int fg = -2;
 	int bg = -2;
 
@@ -58,7 +69,7 @@ void color_parse_mem(const char *value, int value_len, const char *var,
 		return;
 	}
 
-	/* [fg [bg]] [attr] */
+	/* [fg [bg]] [attr]... */
 	while (len > 0) {
 		const char *word = ptr;
 		int val, wordlen = 0;
@@ -87,19 +98,39 @@ void color_parse_mem(const char *value, int value_len, const char *var,
 			goto bad;
 		}
 		val = parse_attr(word, wordlen);
-		if (val < 0 || attr != -1)
+		if (0 <= val)
+			attr |= (1 << val);
+		else
 			goto bad;
-		attr = val;
 	}
 
-	if (attr >= 0 || fg >= 0 || bg >= 0) {
+	if (attr || fg >= 0 || bg >= 0) {
 		int sep = 0;
+		int i;
+		int num_attrs = count_bits(attr);
+
+		if (COLOR_MAXLEN <=
+		    /* Number of bytes to denote colors and attributes */
+		    num_attrs
+		    + (fg < 0 ? 0 : (fg < 8) ? 2 : 8) /* "3x" or "38;5;xxx" */
+		    + (bg < 0 ? 0 : (bg < 8) ? 2 : 8) /* "4x" or "48;5;xxx" */
+		    /* Number of semicolons between the above elements */
+		    + (num_attrs + (0 <= fg) + (0 <= bg) - 1)
+		    /* ESC '[', terminating 'm' and NUL */
+		    + 4)
+			goto bad;
 
 		*dst++ = '\033';
 		*dst++ = '[';
-		if (attr >= 0) {
-			*dst++ = '0' + attr;
-			sep++;
+
+		for (i = 0; attr; i++) {
+			unsigned bit = (1 << i);
+			if (!(attr & bit))
+				continue;
+			attr &= ~bit;
+			if (sep++)
+				*dst++ = ';';
+			*dst++ = '0' + i;
 		}
 		if (fg >= 0) {
 			if (sep++)
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index b61e516..c3af190 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -8,14 +8,13 @@ test_description='Test diff/status color escape codes'
 
 color()
 {
-	git config diff.color.new "$1" &&
-	test "`git config --get-color diff.color.new`" = "^[$2"
+	actual=$(git config --get-color no.such.slot "$1") &&
+	test "$actual" = "^[$2"
 }
 
 invalid_color()
 {
-	git config diff.color.new "$1" &&
-	test -z "`git config --get-color diff.color.new 2>/dev/null`"
+	test_must_fail git config --get-color no.such.slot "$1"
 }
 
 test_expect_success 'reset' '
@@ -42,6 +41,14 @@ test_expect_success 'fg bg attr' '
 	color "blue red ul" "[4;34;41m"
 '
 
+test_expect_success 'fg bg attr...' '
+	color "blue bold dim ul blink reverse" "[1;2;4;5;7;34m"
+'
+
+test_expect_success 'color specification too long' '
+	invalid_color "254 255 bold dim ul blink reverse" "[1;2;4;5;7;38;5;254;48;5;255m"
+'
+
 test_expect_success '256 colors' '
 	color "254 bold 255" "[1;38;5;254;48;5;255m"
 '
-- 
1.7.0.270.g320aa

Re: [PATCH] color: allow multiple attributes

[Jeff King]

On Sat, Feb 27, 2010 at 06:56:38PM -0800, Junio C Hamano wrote:

>   >>> but wouldn't it be more user friendly for us
>   >>> to support "red blink bold ul italic"?
>   >>
>   >> Yes, I think this should be done whether or not the patch in question
>   >> is accepted.
> 
>   This time with a bit of test updates as well for real inclusion.

Looks OK to me, but...

>   Also I realized that we can stuff them in an unsigned flag word as
>   bitfields ("red bold" and "red bold bold bold" would give the same
>   boldness anyway) to lift the artificial limit of number of attribute
>   words.

I also had this thought, but shouldn't that mean:

> +		int i;
> +		int num_attrs = count_bits(attr);
> +
> +		if (COLOR_MAXLEN <=
> +		    /* Number of bytes to denote colors and attributes */
> +		    num_attrs
> +		    + (fg < 0 ? 0 : (fg < 8) ? 2 : 8) /* "3x" or "38;5;xxx" */
> +		    + (bg < 0 ? 0 : (bg < 8) ? 2 : 8) /* "4x" or "48;5;xxx" */
> +		    /* Number of semicolons between the above elements */
> +		    + (num_attrs + (0 <= fg) + (0 <= bg) - 1)
> +		    /* ESC '[', terminating 'm' and NUL */
> +		    + 4)
> +			goto bad;

We don't need this, because the length of what can be specified is
bounded, and we simply need to set COLOR_MAXLEN high enough to handle
the longest case? Though I suppose it doesn't hurt to be paranoid.

> +test_expect_success 'fg bg attr...' '
> +	color "blue bold dim ul blink reverse" "[1;2;4;5;7;34m"
> +'

Hmm. Just a thought on the bit-setting approach, but does the order of
attributes ever matter? We are going to lose the ordering information
the user specifies, obviously.

-Peff

Re: [PATCH] color: allow multiple attributes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> +		if (COLOR_MAXLEN <=
>> +		    /* Number of bytes to denote colors and attributes */
>> +		    num_attrs
>> +		    + (fg < 0 ? 0 : (fg < 8) ? 2 : 8) /* "3x" or "38;5;xxx" */
>> +		    + (bg < 0 ? 0 : (bg < 8) ? 2 : 8) /* "4x" or "48;5;xxx" */
>> +		    /* Number of semicolons between the above elements */
>> +		    + (num_attrs + (0 <= fg) + (0 <= bg) - 1)
>> +		    /* ESC '[', terminating 'm' and NUL */
>> +		    + 4)
>> +			goto bad;
>
> We don't need this, because the length of what can be specified is
> bounded, and we simply need to set COLOR_MAXLEN high enough to handle
> the longest case?

Yes, I think we are now bounded and don't need this; I just thought it
would have an educational value to show how to comment a complex
expression in a readable way ;-)

>> +test_expect_success 'fg bg attr...' '
>> +	color "blue bold dim ul blink reverse" "[1;2;4;5;7;34m"
>> +'
>
> Hmm. Just a thought on the bit-setting approach, but does the order of
> attributes ever matter? We are going to lose the ordering information
> the user specifies, obviously.

True, I don't know if it matters.  I don't know if "blue bold bold" would
result in bolder blue than "blue bold" on some terminal emulators, either.

I'd suggest that we ignore the issue for now, and when somebody complains
with an actual non-working case, we would assess the damage that comes
from this reordering to decide what to do next.  Parhaps a "non-working
case" could be "'blink ul' blinks letter without blinking underline, but
'ul blink' makes both letter and underline blink".  At that point we can
say "Ok, you found a case the order changes the results.  But does that
difference matter in practice?" and move forward, either by fixing it, or
declaring it doesn't matter in practice.

We were already losing the order by emitting attr then fg then bg even
though attr can come before any colors (an undocumented side effect of a
sloppy parsing logic, but some of the existing tests insist on kepping it
working), by the way.

Re: [PATCH] color: allow multiple attributes

[Jeff King]

On Sun, Feb 28, 2010 at 10:16:19AM -0800, Junio C Hamano wrote:

> > Hmm. Just a thought on the bit-setting approach, but does the order of
> > attributes ever matter? We are going to lose the ordering information
> > the user specifies, obviously.
> 
> True, I don't know if it matters.  I don't know if "blue bold bold" would
> result in bolder blue than "blue bold" on some terminal emulators, either.
> 
> I'd suggest that we ignore the issue for now, and when somebody complains
> with an actual non-working case, we would assess the damage that comes
> from this reordering to decide what to do next.  Parhaps a "non-working

I'm fine with that. FWIW, I tested and blink-before-ul and
ul-before-blink look identical in an xterm. Certainly that's not the
only terminal emulator people will use, but I expect it to be
representative of the behavior of most emulators.  I can dig my VT100
out of the attic if we want a real answer. ;)

> We were already losing the order by emitting attr then fg then bg even
> though attr can come before any colors (an undocumented side effect of a
> sloppy parsing logic, but some of the existing tests insist on kepping it
> working), by the way.

True. I also tested attr-before-color, then color-before-attr for both
reverse and bold, and they look the same in an xterm. So I suspect it
doesn't matter, and I'm too lazy to do more research unless somebody
finds something that actually doesn't work.

-Peff

[PATCH 2/5] 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 552ef1f..dcc3d48 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -871,7 +871,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

[PATCH 3/5] Remove reference to GREP_COLORS from documentation

[Mark Lodato]

There is no longer support for external grep, as per bbc09c22b9, so
remove the reference to it in the documentation.

Signed-off-by: Mark Lodato <lodatom@gmail.com>
---
 Documentation/config.txt |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index fed18cb..791b065 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -689,9 +689,7 @@ color.grep::
 
 color.grep.match::
 	Use customized color for matches.  The value of this variable
-	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'.
+	may be specified as in color.branch.<slot>.
 
 color.interactive::
 	When set to `always`, always use colors for interactive prompts
-- 
1.7.0

[PATCH 4/5] 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 are a few differences from GNU grep:

1. With --name-only, GNU grep colors the filenames, but we do not.  I do
   not see any point to making everything the same color.

2. When a binary file matches without -a, GNU grep does not color the
   <file> in "Binary file <file> matches", but we do.  The point of
   colorization is to highlight important parts, and the filename is an
   important part.

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.

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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 791b065..154bc02 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -687,9 +687,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 dcc3d48..43b952b 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -289,6 +289,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;
@@ -296,17 +297,24 @@ 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")) {
-		if (!value)
-			return config_error_nonbool(var);
-		color_parse(value, var, opt->color_match);
-		return 0;
-	}
-	return git_color_default_config(var, value, cb);
+	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 (!value)
+		return config_error_nonbool(var);
+	color_parse(value, var, color);
+	if (!strcmp(color, GIT_COLOR_RESET))
+		color[0] = '\0';
+	return 0;
 }
 
 /*
@@ -871,7 +879,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..132798d 100644
--- a/grep.c
+++ b/grep.c
@@ -506,35 +506,52 @@ static int next_match(struct grep_opt *opt, char *bol, char *eol,
 	return hit;
 }
 
+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) {
+		sign = '\0';
+		opt->output(opt, &sign, 1);
+	} else
+		output_color(opt, &sign, 1, opt->color_sep);
+}
+
 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 +565,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,
+				     (int)(match.rm_eo - match.rm_so),
+				     opt->color_match);
 			bol += match.rm_eo;
 			rest -= match.rm_eo;
 			eflags = REG_NOTBOL;
@@ -823,7 +837,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 +897,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

Re: [PATCH 4/5] grep: Colorize filename, line number, and separator

[René Scharfe]

Am 27.02.2010 05:57, schrieb 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 are a few differences from GNU grep:
> 
> 1. With --name-only, GNU grep colors the filenames, but we do not.  I do
>    not see any point to making everything the same color.

I guess they did it for consistency, so when you see "magenta" you think
"filename", and because it can be turned off with a switch.  With your
patch all filenames are coloured the same, too, by the way: using the
default foreground colour. :)

> diff --git a/builtin-grep.c b/builtin-grep.c
> index dcc3d48..43b952b 100644
> --- a/builtin-grep.c
> +++ b/builtin-grep.c
> @@ -289,6 +289,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;
> @@ -296,17 +297,24 @@ 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")) {
> -		if (!value)
> -			return config_error_nonbool(var);
> -		color_parse(value, var, opt->color_match);
> -		return 0;
> -	}
> -	return git_color_default_config(var, value, cb);
> +	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 (!value)
> +		return config_error_nonbool(var);

color.grep without a value used to turn on colourization, now it seems
to error out.

> +	color_parse(value, var, color);
> +	if (!strcmp(color, GIT_COLOR_RESET))
> +		color[0] = '\0';

This turns off colouring if the user specified "reset" as the colour,
right?  Interesting optimization, but is it really needed?  Perhaps it's
just me, but I'd give the user the requested "<reset>text<reset>"
sequence if she asked for it, even if it's longer than and looks the
same as "text" alone.

> diff --git a/grep.c b/grep.c
> index a0864f1..132798d 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -506,35 +506,52 @@ static int next_match(struct grep_opt *opt, char *bol, char *eol,
>  	return hit;
>  }
>  
> +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

	} else

> +		opt->output(opt, data, size);
> +}
> +
> +static void output_sep(struct grep_opt *opt, char sign)
> +{
> +	if (opt->null_following_name) {
> +		sign = '\0';
> +		opt->output(opt, &sign, 1);
> +	} else

	if (opt->null_following_name)
		opt->output(opt, "", 1);
	else

> +		output_color(opt, &sign, 1, opt->color_sep);
> +}

Re: [PATCH 4/5] grep: Colorize filename, line number, and separator

[Mark Lodato]

On Sat, Feb 27, 2010 at 6:43 AM, René Scharfe
<rene.scharfe@lsrfire.ath.cx> wrote:
> Am 27.02.2010 05:57, schrieb Mark Lodato:
>> 1. With --name-only, GNU grep colors the filenames, but we do not.  I do
>>    not see any point to making everything the same color.
>
> I guess they did it for consistency, so when you see "magenta" you think
> "filename", and because it can be turned off with a switch.  With your
> patch all filenames are coloured the same, too, by the way: using the
> default foreground colour. :)

Yes, I think I understand the reasoning, but to me it is very
annoying.  However, if there is a consensus that we should follow GNU
grep in this regard, I will do it.

>> diff --git a/builtin-grep.c b/builtin-grep.c
>> +     if (!value)
>> +             return config_error_nonbool(var);
>
> color.grep without a value used to turn on colourization, now it seems
> to error out.

Oops, that should be "if (color && !value)".  I will fix in next respin.

>> +     color_parse(value, var, color);
>> +     if (!strcmp(color, GIT_COLOR_RESET))
>> +             color[0] = '\0';
>
> This turns off colouring if the user specified "reset" as the colour,
> right?

Yes.

> Interesting optimization, but is it really needed? Perhaps it's
> just me, but I'd give the user the requested "<reset>text<reset>"
> sequence if she asked for it, even if it's longer than and looks the
> same as "text" alone.

The problem is that there's no way to say "no color".  A blank value
and "reset" both come to the same thing.  I would rather have as
little markup as possible in the output, and this tweak is very
simple.  While this is not strictly necessary, it does make the output
identical to the pre-patch output if you disable all the new colors
(just grep.color.separator, by default.)

>> +     }
>> +     else
>
>        } else

Oops, thanks.

>> +     if (opt->null_following_name) {
>> +             sign = '\0';
>> +             opt->output(opt, &sign, 1);
>> +     } else
>
>        if (opt->null_following_name)
>                opt->output(opt, "", 1);
>        else

Personally, I find your suggestion less readable.  My version is only
one line longer but makes the code completely obvious, whereas the
one-liner requires a second of thought.  Anyone else care to comment
on this?

Re: [PATCH 4/5] grep: Colorize filename, line number, and separator

[Michael Witten]

On Sun, Feb 28, 2010 at 14:14, Mark Lodato <lodatom@gmail.com> wrote:
> On Sat, Feb 27, 2010 at 6:43 AM, René Scharfe
> <rene.scharfe@lsrfire.ath.cx> wrote:
>> Am 27.02.2010 05:57, schrieb Mark Lodato:
>>> 1. With --name-only, GNU grep colors the filenames, but we do not.  I do
>>>    not see any point to making everything the same color.
>>
>> I guess they did it for consistency, so when you see "magenta" you think
>> "filename", and because it can be turned off with a switch.  With your
>> patch all filenames are coloured the same, too, by the way: using the
>> default foreground colour. :)
>
> Yes, I think I understand the reasoning, but to me it is very
> annoying.  However, if there is a consensus that we should follow GNU
> grep in this regard, I will do it.

I'm in favor of colorizing the output even when just one piece of
information is presented. If I turn on colorization, then there should
be colorization; my brain would expect it, especially when I first
grep without --name-only and then turn on --name-only after getting
results that I like.

Of course, I bet you find colorizing the filenames a nuisance because
you don't care to pipe the relevant escape sequences to other
commands. On that note, it would be nice to have something like GNU's
--color=(auto|yes|no) with `auto' as the default for a plain --color.

As a compromise (and perhaps as an improvement), perhaps only the
basename of the filename should be colorized when --name-only is used;
that way, colorization is still being used to differentiate different
data, and the rest of the path is usually not that interesting anyway.
However, for consistency, I would still think it wise to colorize the
dirname portion with `color.grep.filename', but color the basename
portion with `color.grep.match' (as though the basename portion is the
text being matched).

Sincerely,
Michael Witten

Re: [PATCH 4/5] grep: Colorize filename, line number, and separator

[Mark Lodato]

On Sun, Feb 28, 2010 at 5:26 PM, Michael Witten <mfwitten@gmail.com> wrote:
> Of course, I bet you find colorizing the filenames a nuisance because
> you don't care to pipe the relevant escape sequences to other
> commands.

I'm not quite sure what you mean here, but my reason has nothing to do
with piping.  If the output is entirely in a single color, I would
prefer that color to be my terminal's default.  The color adds no
value.

> On that note, it would be nice to have something like GNU's
> --color=(auto|yes|no) with `auto' as the default for a plain --color.

Something like [1]?  By the way, the default should be 'always', not
'auto', to be consistent with GNU tools, and to be backwards
compatible with the old --color behavior.

[1] http://permalink.gmane.org/gmane.comp.version-control.git/139864

> As a compromise (and perhaps as an improvement), perhaps only the
> basename of the filename should be colorized when --name-only is used;
> that way, colorization is still being used to differentiate different
> data, and the rest of the path is usually not that interesting anyway.
> However, for consistency, I would still think it wise to colorize the
> dirname portion with `color.grep.filename', but color the basename
> portion with `color.grep.match' (as though the basename portion is the
> text being matched).

Personally, I am not a fan of this, but if it is implemented, it
should be an option, and should be turned off by default.  Instead of
highlighting the name, it may be better to simply highlight the
slashes so the reader can more easily parse the path.  But still, I
don't think this is worth the trouble.

Re: [PATCH 4/5] grep: Colorize filename, line number, and separator

[Michael Witten]

> Something like [1]?

Ah! Good!

>> with `auto' as the default for a plain --color.
>
> By the way, the default should be 'always', not
> 'auto', to be consistent with GNU tools, and to be backwards
> compatible with the old --color behavior.

Well, I've got:

    GNU grep 2.5.4

and the default for a plain `--color' seems to be `auto', whereby
colorization is turned on when stdout is attached to a tty capable
of color, but turned off otherwise:

    echo t > t

    /bin/grep t t --color              # t is colored red
    /bin/grep t t --color        | cat # t is not colored
    /bin/grep t t --color=always | cat # t is colored

Let's see how the GNU grep source sets up colorization:

    case COLOR_OPTION:
        if(optarg) {
          if(!strcasecmp(optarg, "always") || !strcasecmp(optarg, "yes") ||
             !strcasecmp(optarg, "force"))
            color_option = 1;
          else if(!strcasecmp(optarg, "never") || !strcasecmp(optarg, "no") ||
                  !strcasecmp(optarg, "none"))
            color_option = 0;
          else if(!strcasecmp(optarg, "auto") || !strcasecmp(optarg, "tty") ||
                  !strcasecmp(optarg, "if-tty"))
            color_option = 2;
          else
            show_help = 1;
        } else
          color_option = 2;     /* <--------------- default           */
        /* the rest of this case statement (see below) */

Firstly, note that GNU understands a wide set of option arguments:

    1: always , yes , force
    0: never  , no  , none
    2: auto   , tty , if-tty

Secondly, note that the default mode is that which is selected by
auto/tty/if-tty:

    color_option = 2;

However, there's a little code left that specially processes this
'auto' mode to transform it into either 'always' or 'never':

        if(color_option == 2) {
          if(isatty(STDOUT_FILENO) && getenv("TERM") &&
             strcmp(getenv("TERM"), "dumb"))
                  color_option = 1;
          else
            color_option = 0;
        }
        break;

Thus, if stdout is attached to a tty that understands color, then
colorization is turned on; otherwise, colorization is turned off.

In my opinion, Git grep should follow GNU grep's conventions, not
only to be consistent, but also because they are better.

>> Of course, I bet you find colorizing the filenames a nuisance because
>> you don't care to pipe the relevant escape sequences to other
>> commands.
>
> I'm not quite sure what you mean here, but my reason has nothing to do
> with piping.  If the output is entirely in a single color, I would
> prefer that color to be my terminal's default.  The color adds no
> value.

Unfortunately, Git grep interprets a plain `--color' the same way
that GNU grep interprets `--color=always', so that the color
escape sequences get piped to everything.

    $ cd $clean_repo_for_git_source
    $ grep ':-)' -R . --exclude-dir=.git --color | cut -c 3- > smilies-gnu
    $ git grep --color ':-)' > smilies-git

    $ ls -l smilies*     # Note the size difference
    -rw-r--r-- 1 mfwitten mfwitten 813 Mar  1 05:43 smilies-git
    -rw-r--r-- 1 mfwitten mfwitten 717 Mar  1 05:43 smilies-gnu
    
    $ cat -t smilies-gnu
    Documentation/glossary-content.txt:^IThe list you get with "ls" :-)
    Documentation/technical/pack-heuristics.txt:        have to build up a certain level of gumption first :-)
    Documentation/technical/pack-heuristics.txt:       even realize how much I wasn't realizing :-)
    Documentation/technical/pack-heuristics.txt:        the cases where you might have to wander, don't do that :-)
    Documentation/technical/pack-heuristics.txt:        I'm getting lost in all these orders, let me re-read :-)
    Documentation/technical/pack-heuristics.txt:        can just read what you said there :-)
    Documentation/technical/pack-heuristics.txt:    <njs`> :-)
    Documentation/technical/pack-heuristics.txt:        details on git packs :-)
    
    $ cat -t smilies-git
    Documentation/glossary-content.txt:^IThe list you get with "ls" ^[[31m^[[1m:-)^[[m
    Documentation/technical/pack-heuristics.txt:        have to build up a certain level of gumption first ^[[31m^[[1m:-)^[[m
    Documentation/technical/pack-heuristics.txt:       even realize how much I wasn't realizing ^[[31m^[[1m:-)^[[m
    Documentation/technical/pack-heuristics.txt:        the cases where you might have to wander, don't do that ^[[31m^[[1m:-)^[[m
    Documentation/technical/pack-heuristics.txt:        I'm getting lost in all these orders, let me re-read ^[[31m^[[1m:-)^[[m
    Documentation/technical/pack-heuristics.txt:        can just read what you said there ^[[31m^[[1m:-)^[[m
    Documentation/technical/pack-heuristics.txt:    <njs`> ^[[31m^[[1m:-)^[[m
    Documentation/technical/pack-heuristics.txt:        details on git packs ^[[31m^[[1m:-)^[[m

If you just run a plain `cat smilies-git', your terminal should
still render the colorization, as the escape sequences are
preserved. This is generally a nuisance because normally it
is desirable to lose that information when piping it to places
other than the screen.

> If the output is entirely in a single color, I would prefer that
> color to be my terminal's default. The color adds no value.

I would prefer whatever color to which I've become accustomed; the
color is also a quick indication of what you're viewing. I usually
keep altering the search pattern until I get the right set of
matches and THEN issue `--name-only' to get just the paths; when
I do that, I expect the output to change just by cutting out the
stuff to the right of the paths, but your suggestion would ALSO
cause the color to change. In my opinion, that's jarring.

Moreover, with a plain `--color' being interpreted as `--color=auto',
I would also have the benefit of piping the output anywhere else and
never having to bother with `--no-color' or `--color=never' or just
removing `--color'.

In short, I think the GNU strategy is the most intuitive and
streamlined approach.

>> As a compromise (and perhaps as an improvement), perhaps
>> only the basename of the filename should be colorized when
>> --name-only is used; that way, colorization is still being used
>> to differentiate different data, and the rest of the path is
>> usually not that interesting anyway. However, for consistency,
>> I would still think it wise to colorize the dirname portion
>> with `color.grep.filename', but color the basename portion with
>> `color.grep.match' (as though the basename portion is the text
>> being matched).
>
> Personally, I am not a fan of this, but if it is implemented, it
> should be an option, and should be turned off by default. Instead
> of highlighting the name, it may be better to simply highlight the
> slashes so the reader can more easily parse the path. But still, I
> don't think this is worth the trouble.

The basenames of paths are almost always the most important piece of
data. Still, my suggestion is to leave the whole path colorized as
usual when colorization is active.

Sincerely,
Michael Witten

Re: [PATCH 4/5] grep: Colorize filename, line number, and separator

[Mark Lodato]

On Tue, Mar 2, 2010 at 1:43 AM, Michael Witten <mfwitten@gmail.com> wrote:
>> By the way, the default should be 'always', not
>> 'auto', to be consistent with GNU tools, and to be backwards
>> compatible with the old --color behavior.
>
> Well, I've got:
>
>    GNU grep 2.5.4
>
> and the default for a plain `--color' seems to be `auto', whereby
> colorization is turned on when stdout is attached to a tty capable
> of color, but turned off otherwise:

Sorry, that was my mistake about GNU grep.  However, GNU ls (coreutils
7.4) defaults to 'always'.  So, GNU tools are not consistent in this
regard.  Furthermore, the current behavior of all git tools is to make
--color turn on color always, so I imagine you would have to make an
extremely compelling argument to break backwards compatibility.  I'll
add that this behavior makes the most sense, since most folks who use
color have done `git config color.ui auto'.  This is why no one have
created this patch until now.  The [=<when>] part is nice, but the git
config infrastructure usually obviates the need for --color=auto.

> Firstly, note that GNU understands a wide set of option arguments:
>
>    1: always , yes , force
>    0: never  , no  , none
>    2: auto   , tty , if-tty

I guess this is okay, but I don't see a need for it.  If we allow
these other synonyms, then we'll have to support them forever.  I say
just stick with always/never/auto for now, and we could add the others
later if there's a big demand.

> In my opinion, Git grep should follow GNU grep's conventions, not
> only to be consistent, but also because they are better.

It is more important to be consistent with the other git tools, so
that is why --color is a synonym for --color=always.

Re: [PATCH 4/5] grep: Colorize filename, line number, and separator

[Miles Bader]

Mark Lodato <lodatom@gmail.com> writes:
> Furthermore, the current behavior of all git tools is to make
> --color turn on color always, so I imagine you would have to make an
> extremely compelling argument to break backwards compatibility.

I don't think the usual backwards-compatibility arguments should really
apply to edge-cases in frippery like --color...

-Miles

-- 
Generous, adj. Originally this word meant noble by birth and was rightly
applied to a great multitude of persons. It now means noble by nature and is
taking a bit of a rest.

Re: [PATCH 4/5] grep: Colorize filename, line number, and separator

[René Scharfe]

Forgot one thing in my earlier reply:

Am 27.02.2010 05:57, schrieb Mark Lodato:
> @@ -548,12 +565,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,
> +				     (int)(match.rm_eo - match.rm_so),
> +				     opt->color_match);

The third parameter of output_color() (and of ->output(), so you didn't
introduce this, of course) is a size_t, so why cast to int?  Is a cast
needed at all?

Re: [PATCH 4/5] grep: Colorize filename, line number, and separator

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

>> -			opt->output(opt, bol + match.rm_so,
>> -				    (int)(match.rm_eo - match.rm_so));
>
> The third parameter of output_color() (and of ->output(), so you didn't
> introduce this, of course) is a size_t, so why cast to int?  Is a cast
> needed at all?

I don't think so.

Earlier in 747a322 (grep: cast printf %.*s "precision" argument explicitly
to int, 2009-03-08), I casted the difference between two regoff_t you were
feeding to printf's "%.*s" as a length, introduced by 7e8f59d (grep: color
patterns in output, 2009-03-07), and 5b594f4 (Threaded grep, 2010-01-25)
carried that cast over without thinking.

Re: [PATCH 4/5] grep: Colorize filename, line number, and separator

[Mark Lodato]

On Sat, Feb 27, 2010 at 12:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>
>>> -                    opt->output(opt, bol + match.rm_so,
>>> -                                (int)(match.rm_eo - match.rm_so));
>>
>> The third parameter of output_color() (and of ->output(), so you didn't
>> introduce this, of course) is a size_t, so why cast to int?  Is a cast
>> needed at all?
>
> I don't think so.
>
> Earlier in 747a322 (grep: cast printf %.*s "precision" argument explicitly
> to int, 2009-03-08), I casted the difference between two regoff_t you were
> feeding to printf's "%.*s" as a length, introduced by 7e8f59d (grep: color
> patterns in output, 2009-03-07), and 5b594f4 (Threaded grep, 2010-01-25)
> carried that cast over without thinking.

Ok.  I'll remove the cast.  Should I note this in the commit message?

Re: [PATCH 4/5] grep: Colorize filename, line number, and separator

[Junio C Hamano]

Mark Lodato <lodatom@gmail.com> writes:

> +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`:::

Why do I get a feeling that I already said something about three colons?

 ... goes and looks ...

Ah, it wasn't to you.  Please see:

  http://thread.gmane.org/gmane.comp.version-control.git/139014/focus=139343

BUT.

I tried the three-colons notation with AsciiDoc 8.2.7 and it seems to take
it as enumeration items that are nested a level deeper, so this might be
safe.

But the last sentence about color.branch.<slot> is indented as if it is a
part of description for "separator" slot, which you may want to fix
regardless.

Re: [PATCH 4/5] grep: Colorize filename, line number, and separator

[Mark Lodato]

On Sun, Feb 28, 2010 at 2:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Mark Lodato <lodatom@gmail.com> writes:
>
>> +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`:::
>
> Why do I get a feeling that I already said something about three colons?
>
>  ... goes and looks ...
>
> Ah, it wasn't to you.  Please see:
>
>  http://thread.gmane.org/gmane.comp.version-control.git/139014/focus=139343
>
> BUT.
>
> I tried the three-colons notation with AsciiDoc 8.2.7 and it seems to take
> it as enumeration items that are nested a level deeper, so this might be
> safe.

When I wrote the patch originally, I tried to find the difference
between triple colons and double semi-colons but failed.  Now that I
look at the changelog, double semi-colons was introduced in 5.0.9, but
there is no indication whatsoever of triple colons.  The wording
implies that double-semicolon is older, so it's probably safer to use.
 I'll switch to that.

> But the last sentence about color.branch.<slot> is indented as if it is a
> part of description for "separator" slot, which you may want to fix
> regardless.
>

Man!  It worked in AsciiDoc 8.4.4, but evidently not in 8.2.7.  What a
pain.  I just installed 8.2.7 so hopefully I won't run into these
problems in the future.  I'll fix this.

[PATCH 5/5] 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>
---
To me, the biggest benefit is the function line color.  I don't find the other
two useful, but they were trivial to implement.

 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 154bc02..999b1bd 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -692,12 +692,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 43b952b..2ae25c0 100644
--- a/builtin-grep.c
+++ b/builtin-grep.c
@@ -299,12 +299,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
@@ -879,9 +885,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 132798d..adc93b0 100644
--- a/grep.c
+++ b/grep.c
@@ -531,6 +531,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) {
@@ -559,12 +560,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,
 				     (int)(match.rm_eo - match.rm_so),
 				     opt->color_match);
@@ -574,7 +581,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