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