[PATCH 5/7] parse_color: support 24-bit RGB values

2014-11-20 Thread Jeff King
Some terminals (like XTerm) allow full 24-bit RGB color
specifications using an extension to the regular ANSI color
scheme. Let's allow users to specify hex RGB colors,
enabling the all-important feature of hot pink ref
decorations:

  git log --format=%h%C(#ff69b4)%d%C(reset) %s

Signed-off-by: Jeff King p...@peff.net
---
Also no clue on which terminals support it. I did all of my testing on
a recent version of XTerm. It looks like it doesn't provide true 24-bit
support, though. It is happy to accept the 24-bit colors, but if you do:

  for b in $(seq 255); do
h=$(printf %02x $b)
git --no-pager log -1 --format=%C(#$h)$b%C(reset)
  done

the gradient seems to jump in discrete steps. That's fine, though.
It's a quality-of-implementation issue for the terminal, and I still
think that the RGB spec is way more readable than the 256-color mode
ones.

 Documentation/config.txt |  3 ++-
 color.c  | 29 -
 color.h  |  6 +++---
 t/t4026-color.sh |  4 
 4 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index f615a5c..a237b82 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -842,7 +842,8 @@ doesn't matter.
 +
 Colors (foreground and background) may also be given as numbers between
 0 and 255; these use ANSI 256-color mode (but note that not all
-terminals may support this).
+terminals may support this).  If your terminal supports it, you may also
+specify 24-bit RGB values as hex, like `#ff0ab3`.
 
 color.diff::
Whether to use ANSI escape sequences to add color to patches.
diff --git a/color.c b/color.c
index 6edbcae..78cdbed 100644
--- a/color.c
+++ b/color.c
@@ -32,10 +32,13 @@ struct color {
COLOR_UNSPECIFIED = 0,
COLOR_NORMAL,
COLOR_ANSI, /* basic 0-7 ANSI colors */
-   COLOR_256
+   COLOR_256,
+   COLOR_RGB
} state;
/* The numeric value for ANSI and 256-color modes */
unsigned char value;
+   /* 24-bit RGB color values */
+   unsigned char red, green, blue;
 };
 
 /*
@@ -47,6 +50,16 @@ static int match_word(const char *word, int len, const char 
*match)
return !strncasecmp(word, match, len)  !match[len];
 }
 
+static int get_hex_color(const char *in, unsigned char *out)
+{
+   unsigned int val;
+   val = (hexval(in[0])  4) | hexval(in[1]);
+   if (val  ~0xff)
+   return -1;
+   *out = val;
+   return 0;
+}
+
 static int parse_color(struct color *out, const char *name, int len)
 {
/* Positions in array must match ANSI color codes */
@@ -64,6 +77,16 @@ static int parse_color(struct color *out, const char *name, 
int len)
return 0;
}
 
+   /* Try a 24-bit RGB value */
+   if (len == 7  name[0] == '#') {
+   if (!get_hex_color(name + 1, out-red) 
+   !get_hex_color(name + 3, out-green) 
+   !get_hex_color(name + 5, out-blue)) {
+   out-state = COLOR_RGB;
+   return 0;
+   }
+   }
+
/* Then pick from our human-readable color names... */
for (i = 0; i  ARRAY_SIZE(color_names); i++) {
if (match_word(name, len, color_names[i])) {
@@ -140,6 +163,10 @@ static char *color_output(char *out, const struct color 
*c, char type)
case COLOR_256:
out += sprintf(out, %c8;5;%d, type, c-value);
break;
+   case COLOR_RGB:
+   out += sprintf(out, %c8;2;%d;%d;%d, type,
+  c-red, c-green, c-blue);
+   break;
}
return out;
 }
diff --git a/color.h b/color.h
index f5beab1..4ec34b4 100644
--- a/color.h
+++ b/color.h
@@ -9,14 +9,14 @@ struct strbuf;
  * The maximum length of ANSI color sequence we would generate:
  * - leading ESC '['2
  * - attr + ';' 2 * 8 (e.g. 1;)
- * - fg color + ';' 9 (e.g. 38;5;2xx;)
- * - fg color + ';' 9 (e.g. 48;5;2xx;)
+ * - fg color + ';' 17 (e.g. 38;2;255;255;255;)
+ * - bg color + ';' 17 (e.g. 48;2;255;255;255;)
  * - terminating 'm' NUL2
  *
  * The above overcounts attr (we only use 5 not 8) and one semicolon
  * but it is close enough.
  */
-#define COLOR_MAXLEN 40
+#define COLOR_MAXLEN 56
 
 /*
  * IMPORTANT: Due to the way these color codes are emulated on Windows,
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index 63e4238..65386db 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -53,6 +53,10 @@ test_expect_success '256 colors' '
color 254 bold 255 [1;38;5;254;48;5;255m
 '
 
+test_expect_success '24-bit colors' '
+   color #ff00ff black [38;2;255;0;255;40m
+'
+
 test_expect_success 'normal yields no color at all' '
color normal black [40m
 '
-- 
2.2.0.rc2.402.g4519813

--
To 

Re: [PATCH 5/7] parse_color: support 24-bit RGB values

2014-11-20 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 Some terminals (like XTerm) allow full 24-bit RGB color
 specifications using an extension to the regular ANSI color
 scheme. Let's allow users to specify hex RGB colors,
 enabling the all-important feature of hot pink ref
 decorations:

   git log --format=%h%C(#ff69b4)%d%C(reset) %s

 Signed-off-by: Jeff King p...@peff.net
 ---
 Also no clue on which terminals support it. I did all of my testing on
 a recent version of XTerm. It looks like it doesn't provide true 24-bit
 support, though. It is happy to accept the 24-bit colors, but if you do:

   for b in $(seq 255); do
 h=$(printf %02x $b)
 git --no-pager log -1 --format=%C(#$h)$b%C(reset)
   done

 the gradient seems to jump in discrete steps. That's fine, though.
 It's a quality-of-implementation issue for the terminal, and I still
 think that the RGB spec is way more readable than the 256-color mode
 ones.

  Documentation/config.txt |  3 ++-
  color.c  | 29 -
  color.h  |  6 +++---
  t/t4026-color.sh |  4 
  4 files changed, 37 insertions(+), 5 deletions(-)

 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index f615a5c..a237b82 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -842,7 +842,8 @@ doesn't matter.
  +
  Colors (foreground and background) may also be given as numbers between
  0 and 255; these use ANSI 256-color mode (but note that not all
 -terminals may support this).
 +terminals may support this).  If your terminal supports it, you may also
 +specify 24-bit RGB values as hex, like `#ff0ab3`.
  
  color.diff::
   Whether to use ANSI escape sequences to add color to patches.
 diff --git a/color.c b/color.c
 index 6edbcae..78cdbed 100644
 --- a/color.c
 +++ b/color.c
 @@ -32,10 +32,13 @@ struct color {
   COLOR_UNSPECIFIED = 0,
   COLOR_NORMAL,
   COLOR_ANSI, /* basic 0-7 ANSI colors */
 - COLOR_256
 + COLOR_256,
 + COLOR_RGB
   } state;
   /* The numeric value for ANSI and 256-color modes */
   unsigned char value;
 + /* 24-bit RGB color values */
 + unsigned char red, green, blue;

Do value and rgb have to be both valid at the same time, or is this
we are not wasting a byte by not using a union because it will be
in the padding of the outer struct anyway?

Not a satirical and/or rhetorical question.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 5/7] parse_color: support 24-bit RGB values

2014-11-20 Thread Jeff King
On Thu, Nov 20, 2014 at 11:44:26AM -0800, Junio C Hamano wrote:

  @@ -32,10 +32,13 @@ struct color {
  COLOR_UNSPECIFIED = 0,
  COLOR_NORMAL,
  COLOR_ANSI, /* basic 0-7 ANSI colors */
  -   COLOR_256
  +   COLOR_256,
  +   COLOR_RGB
  } state;
  /* The numeric value for ANSI and 256-color modes */
  unsigned char value;
  +   /* 24-bit RGB color values */
  +   unsigned char red, green, blue;
 
 Do value and rgb have to be both valid at the same time, or is this
 we are not wasting a byte by not using a union because it will be
 in the padding of the outer struct anyway?

The latter. I started with a union, and then realized that COLOR_ANSI
and COLOR_256 shared the value, so the union was not saving space and
just getting in the way (mostly because I had to think of useful names
for each of the members).

I'd be happy to do it as a union if you think that makes it clearer.

Also, the name state should perhaps be type. It originally started
as unspecified or an actual value, which is a state, but as I worked,
it grew into something more.

-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 5/7] parse_color: support 24-bit RGB values

2014-11-20 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 On Thu, Nov 20, 2014 at 11:44:26AM -0800, Junio C Hamano wrote:

  @@ -32,10 +32,13 @@ struct color {
 COLOR_UNSPECIFIED = 0,
 COLOR_NORMAL,
 COLOR_ANSI, /* basic 0-7 ANSI colors */
  -  COLOR_256
  +  COLOR_256,
  +  COLOR_RGB
 } state;
 /* The numeric value for ANSI and 256-color modes */
 unsigned char value;
  +  /* 24-bit RGB color values */
  +  unsigned char red, green, blue;
 
 Do value and rgb have to be both valid at the same time, or is this
 we are not wasting a byte by not using a union because it will be
 in the padding of the outer struct anyway?

 The latter. I started with a union, and then realized that COLOR_ANSI
 and COLOR_256 shared the value, so the union was not saving space and
 just getting in the way (mostly because I had to think of useful names
 for each of the members).

 I'd be happy to do it as a union if you think that makes it clearer.

 Also, the name state should perhaps be type. It originally
 started as unspecified or an actual value, which is a state, but
 as I worked, it grew into something more.

I think use of union might be more kosher, e.g.

struct color_spec {
enum { ... } type;
union {
struct { unsigned char r, g, b; } rgb;
unsigned char ansi;
} u;
} c;

but it is not like you have an array of these things for each slot,
and with the intervening .u.type you have to write every time
you refer to these fields, the result is probably much uglier and
harder to read.  So let's only do s/state/type/ and leave these
ought to be union but that will be uglier ones as they are.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html