Re: [PATCH 1/5] Allow explicit ANSI codes for colors
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 27, 2010, 21:21 UTC
- Message-ID
- <7vfx4mv0h9.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <ca433831002271024t5af1dba9m6ca719c114e54892@mail.gmail.com>
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(-)
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++)