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

Mark Lodato [off-list ref] writes:

On Sat, Feb 27, 2010 at 3:51 AM, Jeff King [off-list ref] wrote:

quoted

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.

quoted

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++)

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 <redacted>
---
  Junio C Hamano [off-list ref] 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

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

Jeff King [off-list ref] writes:

quoted

+		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 ;-)

quoted

+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.

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

quoted

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