threads / patch / 22841

patch, 5 partsAllow explicit ANSI codes for colors

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

## tl;dr

25 messages between Feb 27, 2010 and Mar 3, 2010. Diffs are folded; open one to read it.

replies: 24people: 6as markdown or json

Mark Lodato· Feb 27, 2010, 04:57 UTC · lore

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

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(-)
Mark Lodato· Feb 27, 2010, 04:57 UTC · re: Mark Lodato · lore

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(-)
Show changes to 3 files +38 −0

Documentation/config.txt, color.c, t/t4026-color.sh

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
Jeff King· Feb 27, 2010, 08:51 UTC · re: Mark Lodato · lore

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

On Fri, Feb 26, 2010 at 11:57:46PM -0500, Mark Lodato wrote:
Show 6 quoted lines
> 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
Mark Lodato· Feb 27, 2010, 18:24 UTC · re: Jeff King · lore

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

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.

Show 5 quoted lines
> 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

Junio C Hamano· Feb 27, 2010, 21:21 UTC · re: Mark Lodato · lore

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

Mark Lodato <lodatom@gmail.com> writes:
Show 10 quoted lines
> 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.

Show 5 quoted lines
>> 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(-)
Show changes to color.c +27 −8
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++)
Junio C Hamano· Feb 28, 2010, 02:56 UTC · re: Junio C Hamano · lore

[PATCH] color: allow multiple attributes

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(-)
Show changes to 2 files +50 −12

color.c, t/t4026-color.sh

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
Jeff King· Feb 28, 2010, 12:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] color: allow multiple attributes

On Sat, Feb 27, 2010 at 06:56:38PM -0800, Junio C Hamano wrote:
Show 7 quoted lines
>   >>> 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:
Show 13 quoted lines
> +		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
Junio C Hamano· Feb 28, 2010, 18:16 UTC · re: Jeff King · lore

Re: [PATCH] color: allow multiple attributes

Jeff King <peff@peff.net> writes:
Show 14 quoted lines
>> +		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 ;-)

Show 7 quoted lines
>> +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.

Jeff King· Feb 28, 2010, 18:33 UTC · re: Junio C Hamano · lore

Re: [PATCH] color: allow multiple attributes

On Sun, Feb 28, 2010 at 10:16:19AM -0800, Junio C Hamano wrote:
Show 10 quoted lines
> > 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
Mark Lodato· Feb 27, 2010, 04:57 UTC · re: Mark Lodato · lore

[PATCH 2/5] Add GIT_COLOR_BOLD_* and GIT_COLOR_BG_*

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(-)
Show changes to 3 files +18 −7

builtin-grep.c, color.h, graph.c

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
Mark Lodato· Feb 27, 2010, 04:57 UTC · re: Mark Lodato · lore

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

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(-)
Show changes to Documentation/config.txt +1 −3
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
Mark Lodato· Feb 27, 2010, 04:57 UTC · re: Mark Lodato · lore

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

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(-)
Show changes to 4 files +76 −33

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

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
René Scharfe· Feb 27, 2010, 11:43 UTC · re: Mark Lodato · lore

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

Am 27.02.2010 05:57, schrieb Mark Lodato:
Show 11 quoted lines
> 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. :)

Show 40 quoted lines
> 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.

Show 17 quoted lines
> 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
Show 9 quoted lines
> +		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);
> +}
Mark Lodato· Feb 28, 2010, 20:14 UTC · re: René Scharfe · lore

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

On Sat, Feb 27, 2010 at 6:43 AM, René Scharfe <rene.scharfe@lsrfire.ath.cx> wrote:

Show 8 quoted lines
> 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.

Show 6 quoted lines
>> 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.
Show 6 quoted lines
>> +     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.
Show 8 quoted lines
>> +     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?

Michael Witten· Feb 28, 2010, 22:26 UTC · re: Mark Lodato · lore

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

On Sun, Feb 28, 2010 at 14:14, Mark Lodato <lodatom@gmail.com> wrote:
Show 14 quoted lines
> 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

Mark Lodato· Mar 2, 2010, 01:49 UTC · re: Michael Witten · lore

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

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
Show 8 quoted lines
> 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.

Michael Witten· Mar 2, 2010, 06:43 UTC · re: Mark Lodato · lore

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

> Something like [1]?
Ah! Good!
Show 5 quoted lines
>> 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.

Show 8 quoted lines
>> 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.

Show 15 quoted lines
>> 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

Mark Lodato· Mar 3, 2010, 04:26 UTC · re: Michael Witten · lore

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

On Tue, Mar 2, 2010 at 1:43 AM, Michael Witten <mfwitten@gmail.com> wrote:
Show 11 quoted lines
>> 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.

Show 5 quoted lines
> 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.

Miles Bader· Mar 3, 2010, 04:49 UTC · re: Mark Lodato · lore

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

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.
René Scharfe· Feb 27, 2010, 11:53 UTC · re: Mark Lodato · lore

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

Forgot one thing in my earlier reply:
Am 27.02.2010 05:57, schrieb Mark Lodato:
Show 13 quoted lines
> @@ -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?

Junio C Hamano· Feb 27, 2010, 17:08 UTC · re: René Scharfe · lore

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

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
Show 6 quoted lines
>> -			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.

Mark Lodato· Feb 28, 2010, 20:15 UTC · re: Junio C Hamano · lore

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

On Sat, Feb 27, 2010 at 12:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
> 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?
Junio C Hamano· Feb 28, 2010, 19:29 UTC · re: Mark Lodato · lore

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

Mark Lodato <lodatom@gmail.com> writes:
Show 8 quoted lines
> +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.

Mark Lodato· Feb 28, 2010, 20:39 UTC · re: Junio C Hamano · lore

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

On Sun, Feb 28, 2010 at 2:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
> 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.

Mark Lodato· Feb 27, 2010, 04:57 UTC · re: Mark Lodato · lore

[PATCH 5/5] grep: Colorize selected, context, and function lines

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(-)
Show changes to 4 files +27 −2

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

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

← back to recent threads