* [PATCH 1/7] docs: describe ANSI 256-color mode
2014-11-20 15:14 ` [PATCH 0/7] color fixes and configurable diff-highlight Jeff King
@ 2014-11-20 15:15 ` Jeff King
2014-11-20 15:15 ` [PATCH 2/7] config: fix parsing of "git config --get-color some.key -1" Jeff King
` (5 subsequent siblings)
6 siblings, 0 replies; 22+ messages in thread
From: Jeff King @ 2014-11-20 15:15 UTC (permalink / raw)
To: Scott Baker; +Cc: git
Our color specifications have supported the 256-color ANSI
extension for years, but we never documented it.
Signed-off-by: Jeff King <peff@peff.net>
---
I have no clue which terminals do and don't support this. I would hope
the answer is "everything" by now, but I have seen some pretty awful
terminal emulators in my time.
Documentation/config.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9220725..f615a5c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -839,6 +839,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.
++
+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).
color.diff::
Whether to use ANSI escape sequences to add color to patches.
--
2.2.0.rc2.402.g4519813
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 2/7] config: fix parsing of "git config --get-color some.key -1"
2014-11-20 15:14 ` [PATCH 0/7] color fixes and configurable diff-highlight Jeff King
2014-11-20 15:15 ` [PATCH 1/7] docs: describe ANSI 256-color mode Jeff King
@ 2014-11-20 15:15 ` Jeff King
2014-11-20 15:16 ` [PATCH 3/7] t4026: test "normal" color Jeff King
` (4 subsequent siblings)
6 siblings, 0 replies; 22+ messages in thread
From: Jeff King @ 2014-11-20 15:15 UTC (permalink / raw)
To: Scott Baker; +Cc: git
Most of git-config's command line options use OPT_BIT to
choose an action, and then parse the non-option arguments
in a context-dependent way. However, --get-color and
--get-colorbool are unlike the rest of the options, in that
they are OPT_STRING, taking the option name as a parameter.
This generally works, because we then use the presence of
those strings to set an action bit anyway. But it does mean
that the option-parser will continue looking for options
even after the key (because it is not a non-option; it is an
argument to an option). And running:
git config --get-color some.key -1
(to use "-1" as the default color spec) will barf, claiming
that "-1" is not an option. Instead, we should treat
--get-color and --get-colorbool as action bits, just like
--add, --get, and all the other actions, and then check that
the non-option arguments we got are sane. This fixes the
weirdness above, and makes those two options like all the
others.
This "fixes" a test in t4026, which checked that feeding
"-2" as a color should fail (it does fail, but prior to this
patch, because parseopt barfed, not because we actually ever
tried to parse the color).
This also catches other errors, like:
git config --get-color some.key black blue
which previously silently ignored "blue" (and now will
complain that you gave too many arguments).
There are some possible regressions, though. We now disallow
these, which currently do what you would expect:
# specifying other options after the action
git config --get-color some.key --file whatever
# using long-arg syntax
git config --get-color=some.key
However, we have never advertised these in the
documentation, and in fact they did not work in some older
versions of git. The behavior was apparently switched as an
accidental side effect of d64ec16 (git config: reorganize to
use parseopt, 2009-02-21).
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/config.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
diff --git a/builtin/config.c b/builtin/config.c
index 8cc2604..fddafbb 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -69,8 +69,8 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, "remove-section", &actions, N_("remove a section: name"), ACTION_REMOVE_SECTION),
OPT_BIT('l', "list", &actions, N_("list all"), ACTION_LIST),
OPT_BIT('e', "edit", &actions, N_("open an editor"), ACTION_EDIT),
- OPT_STRING(0, "get-color", &get_color_slot, N_("slot"), N_("find the color configured: [default]")),
- OPT_STRING(0, "get-colorbool", &get_colorbool_slot, N_("slot"), N_("find the color setting: [stdout-is-tty]")),
+ OPT_BIT(0, "get-color", &actions, N_("find the color configured: slot [default]"), ACTION_GET_COLOR),
+ OPT_BIT(0, "get-colorbool", &actions, N_("find the color setting: slot [stdout-is-tty]"), ACTION_GET_COLORBOOL),
OPT_GROUP(N_("Type")),
OPT_BIT(0, "bool", &types, N_("value is \"true\" or \"false\""), TYPE_BOOL),
OPT_BIT(0, "int", &types, N_("value is decimal number"), TYPE_INT),
@@ -303,8 +303,9 @@ static int git_get_color_config(const char *var, const char *value, void *cb)
return 0;
}
-static void get_color(const char *def_color)
+static void get_color(const char *var, const char *def_color)
{
+ get_color_slot = var;
get_color_found = 0;
parsed_color[0] = '\0';
git_config_with_options(git_get_color_config, NULL,
@@ -333,8 +334,9 @@ static int git_get_colorbool_config(const char *var, const char *value,
return 0;
}
-static int get_colorbool(int print)
+static int get_colorbool(const char *var, int print)
{
+ get_colorbool_slot = var;
get_colorbool_found = -1;
get_diff_color_found = -1;
get_color_ui_found = -1;
@@ -532,12 +534,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
usage_with_options(builtin_config_usage, builtin_config_options);
}
- if (get_color_slot)
- actions |= ACTION_GET_COLOR;
- if (get_colorbool_slot)
- actions |= ACTION_GET_COLORBOOL;
-
- if ((get_color_slot || get_colorbool_slot) && types) {
+ if ((actions & (ACTION_GET_COLOR|ACTION_GET_COLORBOOL)) && types) {
error("--get-color and variable type are incoherent");
usage_with_options(builtin_config_usage, builtin_config_options);
}
@@ -683,12 +680,14 @@ int cmd_config(int argc, const char **argv, const char *prefix)
die("No such section!");
}
else if (actions == ACTION_GET_COLOR) {
- get_color(argv[0]);
+ check_argc(argc, 1, 2);
+ get_color(argv[0], argv[1]);
}
else if (actions == ACTION_GET_COLORBOOL) {
- if (argc == 1)
- color_stdout_is_tty = git_config_bool("command line", argv[0]);
- return get_colorbool(argc != 0);
+ check_argc(argc, 1, 2);
+ if (argc == 2)
+ color_stdout_is_tty = git_config_bool("command line", argv[1]);
+ return get_colorbool(argv[0], argc == 2);
}
return 0;
--
2.2.0.rc2.402.g4519813
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH 3/7] t4026: test "normal" color
2014-11-20 15:14 ` [PATCH 0/7] color fixes and configurable diff-highlight Jeff King
2014-11-20 15:15 ` [PATCH 1/7] docs: describe ANSI 256-color mode Jeff King
2014-11-20 15:15 ` [PATCH 2/7] config: fix parsing of "git config --get-color some.key -1" Jeff King
@ 2014-11-20 15:16 ` Jeff King
2014-11-20 18:53 ` Junio C Hamano
2014-11-20 15:17 ` [PATCH 4/7] parse_color: refactor color storage Jeff King
` (3 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2014-11-20 15:16 UTC (permalink / raw)
To: Scott Baker; +Cc: git
If the user specifiers "normal" for a foreground color, this
should be a noop (while this may sound useless, it is the
only way to specify an unchanged foreground color followed
by a specific background color).
We also check that color "-1" does the same thing. This is
not documented, but has worked forever, so let's make sure
we keep supporting it.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t4026-color.sh | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/t/t4026-color.sh b/t/t4026-color.sh
index 3726a0e..63e4238 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -53,6 +53,14 @@ test_expect_success '256 colors' '
color "254 bold 255" "[1;38;5;254;48;5;255m"
'
+test_expect_success '"normal" yields no color at all"' '
+ color "normal black" "[40m"
+'
+
+test_expect_success '-1 is a synonym for "normal"' '
+ color "-1 black" "[40m"
+'
+
test_expect_success 'color too small' '
invalid_color "-2"
'
--
2.2.0.rc2.402.g4519813
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 3/7] t4026: test "normal" color
2014-11-20 15:16 ` [PATCH 3/7] t4026: test "normal" color Jeff King
@ 2014-11-20 18:53 ` Junio C Hamano
2014-11-20 19:00 ` Jeff King
0 siblings, 1 reply; 22+ messages in thread
From: Junio C Hamano @ 2014-11-20 18:53 UTC (permalink / raw)
To: Jeff King; +Cc: Scott Baker, git
Jeff King <peff@peff.net> writes:
> If the user specifiers "normal" for a foreground color, this
> should be a noop (while this may sound useless, it is the
> only way to specify an unchanged foreground color followed
> by a specific background color).
>
> We also check that color "-1" does the same thing. This is
> not documented, but has worked forever, so let's make sure
> we keep supporting it.
YLNTED, really? I do not object to the conclusion, but I am
mildly surprised ;-)
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t4026-color.sh | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/t/t4026-color.sh b/t/t4026-color.sh
> index 3726a0e..63e4238 100755
> --- a/t/t4026-color.sh
> +++ b/t/t4026-color.sh
> @@ -53,6 +53,14 @@ test_expect_success '256 colors' '
> color "254 bold 255" "[1;38;5;254;48;5;255m"
> '
>
> +test_expect_success '"normal" yields no color at all"' '
> + color "normal black" "[40m"
> +'
> +
> +test_expect_success '-1 is a synonym for "normal"' '
> + color "-1 black" "[40m"
> +'
> +
> test_expect_success 'color too small' '
> invalid_color "-2"
> '
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/7] t4026: test "normal" color
2014-11-20 18:53 ` Junio C Hamano
@ 2014-11-20 19:00 ` Jeff King
0 siblings, 0 replies; 22+ messages in thread
From: Jeff King @ 2014-11-20 19:00 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Scott Baker, git
On Thu, Nov 20, 2014 at 10:53:56AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > If the user specifiers "normal" for a foreground color, this
Argh, s/specifiers/specifies/
> > We also check that color "-1" does the same thing. This is
> > not documented, but has worked forever, so let's make sure
> > we keep supporting it.
>
> YLNTED, really? I do not object to the conclusion, but I am
> mildly surprised ;-)
I was surprised by it, too, when writing the refactoring patch that
comes next in the series. :)
I was also surprised that we further check that "-2" is _not_ valid in
the tests. I do not mind declaring everything negative to be the same
(either invalid, or "normal"), but I decided that there was really no
benefit to breaking compatibility in this case. And I suppose if you are
using 256-color mode, then "-1 255" is perhaps a natural way to write
it.
-Peff
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 4/7] parse_color: refactor color storage
2014-11-20 15:14 ` [PATCH 0/7] color fixes and configurable diff-highlight Jeff King
` (2 preceding siblings ...)
2014-11-20 15:16 ` [PATCH 3/7] t4026: test "normal" color Jeff King
@ 2014-11-20 15:17 ` Jeff King
2014-11-20 19:37 ` Junio C Hamano
2014-12-09 20:14 ` Johannes Sixt
2014-11-20 15:25 ` [PATCH 5/7] parse_color: support 24-bit RGB values Jeff King
` (2 subsequent siblings)
6 siblings, 2 replies; 22+ messages in thread
From: Jeff King @ 2014-11-20 15:17 UTC (permalink / raw)
To: Scott Baker; +Cc: git
When we parse a color name like "red" into its ANSI color
value, we pack the storage into a single int that may take
on many values:
1. If it's "-2", no value has been specified.
2. If it's "-1", the value is "normal" (i.e., no color).
3. If it's 0 through 7, the value is a standard ANSI
color.
4. If it's larger (up to 255), it is a 256-color extended
value.
Given these magic numbers, it is often hard to see what is
going on in the code. Let's refactor this into a struct with
a flag that tells which scheme we are using, along with a
numeric value. This is more verbose, but should hopefully be
simpler to follow. It will also allow us to easily add
support for more schemes, like 24-bit RGB values.
The result is also slightly less efficient to store, but
that's OK; we only store this intermediate state during the
parse, after which we write out the actual ANSI bytes.
Signed-off-by: Jeff King <peff@peff.net>
---
color.c | 138 +++++++++++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 106 insertions(+), 32 deletions(-)
diff --git a/color.c b/color.c
index 7941e93..6edbcae 100644
--- a/color.c
+++ b/color.c
@@ -26,23 +26,77 @@ const char *column_colors_ansi[] = {
/* Ignore the RESET at the end when giving the size */
const int column_colors_ansi_max = ARRAY_SIZE(column_colors_ansi) - 1;
-static int parse_color(const char *name, int len)
+/* An individual foreground or background color. */
+struct color {
+ enum {
+ COLOR_UNSPECIFIED = 0,
+ COLOR_NORMAL,
+ COLOR_ANSI, /* basic 0-7 ANSI colors */
+ COLOR_256
+ } state;
+ /* The numeric value for ANSI and 256-color modes */
+ unsigned char value;
+};
+
+/*
+ * "word" is a buffer of length "len"; does it match the NUL-terminated
+ * "match" exactly?
+ */
+static int match_word(const char *word, int len, const char *match)
{
+ return !strncasecmp(word, match, len) && !match[len];
+}
+
+static int parse_color(struct color *out, const char *name, int len)
+{
+ /* Positions in array must match ANSI color codes */
static const char * const color_names[] = {
- "normal", "black", "red", "green", "yellow",
+ "black", "red", "green", "yellow",
"blue", "magenta", "cyan", "white"
};
char *end;
int i;
+ long val;
+
+ /* First try the special word "normal"... */
+ if (match_word(name, len, "normal")) {
+ out->state = COLOR_NORMAL;
+ return 0;
+ }
+
+ /* Then pick from our human-readable color names... */
for (i = 0; i < ARRAY_SIZE(color_names); i++) {
- const char *str = color_names[i];
- if (!strncasecmp(name, str, len) && !str[len])
- return i - 1;
+ if (match_word(name, len, color_names[i])) {
+ out->state = COLOR_ANSI;
+ out->value = i;
+ return 0;
+ }
}
- i = strtol(name, &end, 10);
- if (end - name == len && i >= -1 && i <= 255)
- return i;
- return -2;
+
+ /* And finally try a literal 256-color-mode number */
+ val = strtol(name, &end, 10);
+ if (end - name == len) {
+ /*
+ * Allow "-1" as an alias for "normal", but other negative
+ * numbers are bogus.
+ */
+ if (val < -1)
+ ; /* fall through to error */
+ else if (val < 0) {
+ out->state = COLOR_NORMAL;
+ return 0;
+ /* Rewrite low numbers as more-portable standard colors. */
+ } else if (val < 8) {
+ out->state = COLOR_ANSI;
+ out->value = val;
+ } else if (val < 256) {
+ out->state = COLOR_256;
+ out->value = val;
+ return 0;
+ }
+ }
+
+ return -1;
}
static int parse_attr(const char *name, int len)
@@ -65,13 +119,43 @@ int color_parse(const char *value, char *dst)
return color_parse_mem(value, strlen(value), dst);
}
+#define COLOR_FOREGROUND '3'
+#define COLOR_BACKGROUND '4'
+
+/*
+ * Write the ANSI color codes for "c" to "out"; the string should
+ * already have the ANSI escape code in it. "out" should have enough
+ * space in it to fit any color.
+ */
+static char *color_output(char *out, const struct color *c, char type)
+{
+ switch (c->state) {
+ case COLOR_UNSPECIFIED:
+ case COLOR_NORMAL:
+ break;
+ case COLOR_ANSI:
+ *out++ = type;
+ *out++ = '0' + c->value;
+ break;
+ case COLOR_256:
+ out += sprintf(out, "%c8;5;%d", type, c->value);
+ break;
+ }
+ return out;
+}
+
+static int color_empty(const struct color *c)
+{
+ return c->state <= COLOR_NORMAL;
+}
+
int color_parse_mem(const char *value, int value_len, char *dst)
{
const char *ptr = value;
int len = value_len;
unsigned int attr = 0;
- int fg = -2;
- int bg = -2;
+ struct color fg = { COLOR_UNSPECIFIED };
+ struct color bg = { COLOR_UNSPECIFIED };
if (!strncasecmp(value, "reset", len)) {
strcpy(dst, GIT_COLOR_RESET);
@@ -81,6 +165,7 @@ int color_parse_mem(const char *value, int value_len, char *dst)
/* [fg [bg]] [attr]... */
while (len > 0) {
const char *word = ptr;
+ struct color c;
int val, wordlen = 0;
while (len > 0 && !isspace(word[wordlen])) {
@@ -94,14 +179,13 @@ int color_parse_mem(const char *value, int value_len, char *dst)
len--;
}
- val = parse_color(word, wordlen);
- if (val >= -1) {
- if (fg == -2) {
- fg = val;
+ if (!parse_color(&c, word, wordlen)) {
+ if (fg.state == COLOR_UNSPECIFIED) {
+ fg = c;
continue;
}
- if (bg == -2) {
- bg = val;
+ if (bg.state == COLOR_UNSPECIFIED) {
+ bg = c;
continue;
}
goto bad;
@@ -113,7 +197,7 @@ int color_parse_mem(const char *value, int value_len, char *dst)
goto bad;
}
- if (attr || fg >= 0 || bg >= 0) {
+ if (attr || !color_empty(&fg) || !color_empty(&bg)) {
int sep = 0;
int i;
@@ -129,25 +213,15 @@ int color_parse_mem(const char *value, int value_len, char *dst)
*dst++ = ';';
*dst++ = '0' + i;
}
- if (fg >= 0) {
+ if (!color_empty(&fg)) {
if (sep++)
*dst++ = ';';
- if (fg < 8) {
- *dst++ = '3';
- *dst++ = '0' + fg;
- } else {
- dst += sprintf(dst, "38;5;%d", fg);
- }
+ dst = color_output(dst, &fg, COLOR_FOREGROUND);
}
- if (bg >= 0) {
+ if (!color_empty(&bg)) {
if (sep++)
*dst++ = ';';
- if (bg < 8) {
- *dst++ = '4';
- *dst++ = '0' + bg;
- } else {
- dst += sprintf(dst, "48;5;%d", bg);
- }
+ dst = color_output(dst, &bg, COLOR_BACKGROUND);
}
*dst++ = 'm';
}
--
2.2.0.rc2.402.g4519813
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 4/7] parse_color: refactor color storage
2014-11-20 15:17 ` [PATCH 4/7] parse_color: refactor color storage Jeff King
@ 2014-11-20 19:37 ` Junio C Hamano
2014-12-09 20:14 ` Johannes Sixt
1 sibling, 0 replies; 22+ messages in thread
From: Junio C Hamano @ 2014-11-20 19:37 UTC (permalink / raw)
To: Jeff King; +Cc: Scott Baker, git
Jeff King <peff@peff.net> writes:
> The result is also slightly less efficient to store, but
> that's OK; we only store this intermediate state during the
> parse, after which we write out the actual ANSI bytes.
You need up to 24 bits for the value and then 2 bits for what type
of color specification you have (what you called "state"), for which
32-bit should be sufficient. That would mean it is more efficient
than a single int on 64-bit archs, and you can arrange the struct
to be packed if we ever need to keep dozens of these things. Until
then, the updated simple struct is just fine, and either way it is
much easier to read ;-)
Thanks.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] parse_color: refactor color storage
2014-11-20 15:17 ` [PATCH 4/7] parse_color: refactor color storage Jeff King
2014-11-20 19:37 ` Junio C Hamano
@ 2014-12-09 20:14 ` Johannes Sixt
2014-12-09 20:21 ` Jeff King
2014-12-09 20:56 ` Eric Sunshine
1 sibling, 2 replies; 22+ messages in thread
From: Johannes Sixt @ 2014-12-09 20:14 UTC (permalink / raw)
To: Jeff King, Scott Baker; +Cc: git
Am 20.11.2014 um 16:17 schrieb Jeff King:
> +#define COLOR_FOREGROUND '3'
> +#define COLOR_BACKGROUND '4'
This (COLOR_BACKGROUND) causes an ugly redefinition warning on Windows,
because we inherit a definition from a Windows header. How would you
like it fixed? A different name or #undef in front of it?
-- Hannes
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] parse_color: refactor color storage
2014-12-09 20:14 ` Johannes Sixt
@ 2014-12-09 20:21 ` Jeff King
2014-12-09 20:52 ` Johannes Sixt
2014-12-09 20:56 ` Eric Sunshine
1 sibling, 1 reply; 22+ messages in thread
From: Jeff King @ 2014-12-09 20:21 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Scott Baker, git
On Tue, Dec 09, 2014 at 09:14:29PM +0100, Johannes Sixt wrote:
> Am 20.11.2014 um 16:17 schrieb Jeff King:
> > +#define COLOR_FOREGROUND '3'
> > +#define COLOR_BACKGROUND '4'
>
> This (COLOR_BACKGROUND) causes an ugly redefinition warning on Windows,
> because we inherit a definition from a Windows header. How would you
> like it fixed? A different name or #undef in front of it?
I think a different name would be fine. The constants are actually only
used once each. Their main function is to avoid a confusing parameter
'3' to the color_output function. But we could use the literal
constants with a parameter, like:
diff --git a/color.c b/color.c
index e2a0a99..809b359 100644
--- a/color.c
+++ b/color.c
@@ -144,9 +144,6 @@ int color_parse(const char *value, char *dst)
return color_parse_mem(value, strlen(value), dst);
}
-#define COLOR_FOREGROUND '3'
-#define COLOR_BACKGROUND '4'
-
/*
* Write the ANSI color codes for "c" to "out"; the string should
* already have the ANSI escape code in it. "out" should have enough
@@ -245,12 +242,14 @@ int color_parse_mem(const char *value, int value_len, char *dst)
if (!color_empty(&fg)) {
if (sep++)
*dst++ = ';';
- dst = color_output(dst, &fg, COLOR_FOREGROUND);
+ /* foreground colors are all in the 3x range */
+ dst = color_output(dst, &fg, '3');
}
if (!color_empty(&bg)) {
if (sep++)
*dst++ = ';';
- dst = color_output(dst, &bg, COLOR_BACKGROUND);
+ /* background colors are all in the 4x range */
+ dst = color_output(dst, &bg, '4');
}
*dst++ = 'm';
}
We could also pass in integer "30" and "40", and then format it with %d
inside color_output. I don't know if that would be more obvious or not.
-Peff
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 4/7] parse_color: refactor color storage
2014-12-09 20:21 ` Jeff King
@ 2014-12-09 20:52 ` Johannes Sixt
2014-12-09 21:01 ` Jeff King
0 siblings, 1 reply; 22+ messages in thread
From: Johannes Sixt @ 2014-12-09 20:52 UTC (permalink / raw)
To: Jeff King; +Cc: Scott Baker, git
Am 09.12.2014 um 21:21 schrieb Jeff King:
> On Tue, Dec 09, 2014 at 09:14:29PM +0100, Johannes Sixt wrote:
>
>> Am 20.11.2014 um 16:17 schrieb Jeff King:
>>> +#define COLOR_FOREGROUND '3'
>>> +#define COLOR_BACKGROUND '4'
>>
>> This (COLOR_BACKGROUND) causes an ugly redefinition warning on Windows,
>> because we inherit a definition from a Windows header. How would you
>> like it fixed? A different name or #undef in front of it?
>
> I think a different name would be fine. The constants are actually only
> used once each. Their main function is to avoid a confusing parameter
> '3' to the color_output function. But we could use the literal
> constants with a parameter, like:
>
> diff --git a/color.c b/color.c
> index e2a0a99..809b359 100644
> --- a/color.c
> +++ b/color.c
> @@ -144,9 +144,6 @@ int color_parse(const char *value, char *dst)
> return color_parse_mem(value, strlen(value), dst);
> }
>
> -#define COLOR_FOREGROUND '3'
> -#define COLOR_BACKGROUND '4'
> -
> /*
> * Write the ANSI color codes for "c" to "out"; the string should
> * already have the ANSI escape code in it. "out" should have enough
> @@ -245,12 +242,14 @@ int color_parse_mem(const char *value, int value_len, char *dst)
> if (!color_empty(&fg)) {
> if (sep++)
> *dst++ = ';';
> - dst = color_output(dst, &fg, COLOR_FOREGROUND);
> + /* foreground colors are all in the 3x range */
> + dst = color_output(dst, &fg, '3');
> }
> if (!color_empty(&bg)) {
> if (sep++)
> *dst++ = ';';
> - dst = color_output(dst, &bg, COLOR_BACKGROUND);
> + /* background colors are all in the 4x range */
> + dst = color_output(dst, &bg, '4');
> }
> *dst++ = 'm';
> }
>
> We could also pass in integer "30" and "40", and then format it with %d
> inside color_output. I don't know if that would be more obvious or not.
This patch would actually be my personal preference. The comment near
the literal makes it all clear.
-- Hannes
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 4/7] parse_color: refactor color storage
2014-12-09 20:52 ` Johannes Sixt
@ 2014-12-09 21:01 ` Jeff King
0 siblings, 0 replies; 22+ messages in thread
From: Jeff King @ 2014-12-09 21:01 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Scott Baker, git
On Tue, Dec 09, 2014 at 09:52:10PM +0100, Johannes Sixt wrote:
> This patch would actually be my personal preference. The comment near
> the literal makes it all clear.
Thanks. Here it is ready to be applied.
Junio, this goes on top of the jk/colors topic.
-- >8 --
Subject: [PATCH] parse_color: drop COLOR_BACKGROUND macro
Commit 695d95d (parse_color: refactor color storage,
2014-11-20) introduced two macros, COLOR_FOREGROUND and
COLOR_BACKGROUND. The latter conflicts with a system macro
defined on Windows, breaking compilation there.
The simplest solution is to just get rid of these macros
entirely. They are constants that are only used in one place
(since the whole point of 695d95d was to avoid repeating
ourselves). Their main function is to make the magic
character constants more readable, but we can do the same
thing with a comment.
Reported-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Jeff King <peff@peff.net>
---
color.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/color.c b/color.c
index e2a0a99..809b359 100644
--- a/color.c
+++ b/color.c
@@ -144,9 +144,6 @@ int color_parse(const char *value, char *dst)
return color_parse_mem(value, strlen(value), dst);
}
-#define COLOR_FOREGROUND '3'
-#define COLOR_BACKGROUND '4'
-
/*
* Write the ANSI color codes for "c" to "out"; the string should
* already have the ANSI escape code in it. "out" should have enough
@@ -245,12 +242,14 @@ int color_parse_mem(const char *value, int value_len, char *dst)
if (!color_empty(&fg)) {
if (sep++)
*dst++ = ';';
- dst = color_output(dst, &fg, COLOR_FOREGROUND);
+ /* foreground colors are all in the 3x range */
+ dst = color_output(dst, &fg, '3');
}
if (!color_empty(&bg)) {
if (sep++)
*dst++ = ';';
- dst = color_output(dst, &bg, COLOR_BACKGROUND);
+ /* background colors are all in the 4x range */
+ dst = color_output(dst, &bg, '4');
}
*dst++ = 'm';
}
--
2.2.0.454.g7eca6b7
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] parse_color: refactor color storage
2014-12-09 20:14 ` Johannes Sixt
2014-12-09 20:21 ` Jeff King
@ 2014-12-09 20:56 ` Eric Sunshine
1 sibling, 0 replies; 22+ messages in thread
From: Eric Sunshine @ 2014-12-09 20:56 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Jeff King, Scott Baker, Git List
On Tue, Dec 9, 2014 at 3:14 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 20.11.2014 um 16:17 schrieb Jeff King:
>> +#define COLOR_FOREGROUND '3'
>> +#define COLOR_BACKGROUND '4'
>
> This (COLOR_BACKGROUND) causes an ugly redefinition warning on Windows,
> because we inherit a definition from a Windows header. How would you
> like it fixed? A different name or #undef in front of it?
One way this sort of problem was handled in the past was the sanitize
the preprocessor namespace [1].
[1]: https://github.com/git/git/blob/master/compat/mingw.h#L80
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 5/7] parse_color: support 24-bit RGB values
2014-11-20 15:14 ` [PATCH 0/7] color fixes and configurable diff-highlight Jeff King
` (3 preceding siblings ...)
2014-11-20 15:17 ` [PATCH 4/7] parse_color: refactor color storage Jeff King
@ 2014-11-20 15:25 ` Jeff King
2014-11-20 19:44 ` Junio C Hamano
2014-11-20 15:25 ` [PATCH 6/7] parse_color: recognize "no$foo" to clear the $foo attribute Jeff King
2014-11-20 15:29 ` [PATCH 7/7] diff-highlight: allow configurable colors Jeff King
6 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2014-11-20 15:25 UTC (permalink / raw)
To: Scott Baker; +Cc: git
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 <peff@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(#0000$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' NUL 2
*
* 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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 5/7] parse_color: support 24-bit RGB values
2014-11-20 15:25 ` [PATCH 5/7] parse_color: support 24-bit RGB values Jeff King
@ 2014-11-20 19:44 ` Junio C Hamano
2014-11-20 20:10 ` Jeff King
0 siblings, 1 reply; 22+ messages in thread
From: Junio C Hamano @ 2014-11-20 19:44 UTC (permalink / raw)
To: Jeff King; +Cc: Scott Baker, git
Jeff King <peff@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 <peff@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(#0000$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.
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 5/7] parse_color: support 24-bit RGB values
2014-11-20 19:44 ` Junio C Hamano
@ 2014-11-20 20:10 ` Jeff King
2014-11-20 20:25 ` Junio C Hamano
0 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2014-11-20 20:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Scott Baker, git
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 5/7] parse_color: support 24-bit RGB values
2014-11-20 20:10 ` Jeff King
@ 2014-11-20 20:25 ` Junio C Hamano
0 siblings, 0 replies; 22+ messages in thread
From: Junio C Hamano @ 2014-11-20 20:25 UTC (permalink / raw)
To: Jeff King; +Cc: Scott Baker, git
Jeff King <peff@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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 6/7] parse_color: recognize "no$foo" to clear the $foo attribute
2014-11-20 15:14 ` [PATCH 0/7] color fixes and configurable diff-highlight Jeff King
` (4 preceding siblings ...)
2014-11-20 15:25 ` [PATCH 5/7] parse_color: support 24-bit RGB values Jeff King
@ 2014-11-20 15:25 ` Jeff King
2014-11-20 19:46 ` Junio C Hamano
2014-11-20 15:29 ` [PATCH 7/7] diff-highlight: allow configurable colors Jeff King
6 siblings, 1 reply; 22+ messages in thread
From: Jeff King @ 2014-11-20 15:25 UTC (permalink / raw)
To: Scott Baker; +Cc: git
You can turn on ANSI text attributes like "reverse" by
putting "reverse" in your color spec. However, you cannot
ask to turn reverse off.
For common cases, this does not matter. You would turn on
"reverse" at the start of a colored section, and then clear
all attributes with a "reset". However, you may wish to turn
on some attributes, then selectively disable others. For
example:
git log --format="%C(bold ul yellow)%h%C(noul) %s"
underlines just the hash, but without the need to re-specify
the rest of the attributes. This can also help third-party
programs, like contrib/diff-highlight, that want to turn
some attribute on/off without disrupting existing coloring.
Note that some attribute specifications are probably
nonsensical (e.g., "bold nobold"). We do not bother to flag
such constructs, and instead let the terminal sort it out.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/config.txt | 3 ++-
color.c | 8 +++++---
color.h | 4 ++--
t/t4026-color.sh | 11 +++++++++++
4 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index a237b82..7deae0b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -838,7 +838,8 @@ accepted are `normal`, `black`, `red`, `green`, `yellow`, `blue`,
`magenta`, `cyan` and `white`; the attributes are `bold`, `dim`, `ul`,
`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.
+doesn't matter. Attributes may be turned off specifically by prefixing
+them with `no` (e.g., `noreverse`, `noul`, etc).
+
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
diff --git a/color.c b/color.c
index 78cdbed..8dbd714 100644
--- a/color.c
+++ b/color.c
@@ -124,9 +124,11 @@ static int parse_color(struct color *out, const char *name, int len)
static int parse_attr(const char *name, int len)
{
- static const int attr_values[] = { 1, 2, 4, 5, 7 };
+ static const int attr_values[] = { 1, 2, 4, 5, 7,
+ 22, 22, 24, 25, 27 };
static const char * const attr_names[] = {
- "bold", "dim", "ul", "blink", "reverse"
+ "bold", "dim", "ul", "blink", "reverse",
+ "nobold", "nodim", "noul", "noblink", "noreverse"
};
int i;
for (i = 0; i < ARRAY_SIZE(attr_names); i++) {
@@ -238,7 +240,7 @@ int color_parse_mem(const char *value, int value_len, char *dst)
attr &= ~bit;
if (sep++)
*dst++ = ';';
- *dst++ = '0' + i;
+ dst += sprintf(dst, "%d", i);
}
if (!color_empty(&fg)) {
if (sep++)
diff --git a/color.h b/color.h
index 4ec34b4..7fe77fb 100644
--- a/color.h
+++ b/color.h
@@ -8,7 +8,7 @@ struct strbuf;
/*
* The maximum length of ANSI color sequence we would generate:
* - leading ESC '[' 2
- * - attr + ';' 2 * 8 (e.g. "1;")
+ * - attr + ';' 3 * 10 (e.g. "1;")
* - fg color + ';' 17 (e.g. "38;2;255;255;255;")
* - bg color + ';' 17 (e.g. "48;2;255;255;255;")
* - terminating 'm' NUL 2
@@ -16,7 +16,7 @@ struct strbuf;
* The above overcounts attr (we only use 5 not 8) and one semicolon
* but it is close enough.
*/
-#define COLOR_MAXLEN 56
+#define COLOR_MAXLEN 70
/*
* 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 65386db..267c43b 100755
--- a/t/t4026-color.sh
+++ b/t/t4026-color.sh
@@ -45,10 +45,21 @@ test_expect_success 'fg bg attr...' '
color "blue bold dim ul blink reverse" "[1;2;4;5;7;34m"
'
+# note that nobold and nodim are the same code (22)
+test_expect_success 'attr negation' '
+ color "nobold nodim noul noblink noreverse" "[22;24;25;27m"
+'
+
test_expect_success 'long color specification' '
color "254 255 bold dim ul blink reverse" "[1;2;4;5;7;38;5;254;48;5;255m"
'
+test_expect_success 'absurdly long color specification' '
+ color \
+ "#ffffff #ffffff bold nobold dim nodim ul noul blink noblink reverse noreverse" \
+ "[1;2;4;5;7;22;24;25;27;38;2;255;255;255;48;2;255;255;255m"
+'
+
test_expect_success '256 colors' '
color "254 bold 255" "[1;38;5;254;48;5;255m"
'
--
2.2.0.rc2.402.g4519813
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH 6/7] parse_color: recognize "no$foo" to clear the $foo attribute
2014-11-20 15:25 ` [PATCH 6/7] parse_color: recognize "no$foo" to clear the $foo attribute Jeff King
@ 2014-11-20 19:46 ` Junio C Hamano
0 siblings, 0 replies; 22+ messages in thread
From: Junio C Hamano @ 2014-11-20 19:46 UTC (permalink / raw)
To: Jeff King; +Cc: Scott Baker, git
Jeff King <peff@peff.net> writes:
> You can turn on ANSI text attributes like "reverse" by
> putting "reverse" in your color spec. However, you cannot
> ask to turn reverse off.
>
> For common cases, this does not matter. You would turn on
> "reverse" at the start of a colored section, and then clear
> all attributes with a "reset". However, you may wish to turn
> on some attributes, then selectively disable others. For
> example:
>
> git log --format="%C(bold ul yellow)%h%C(noul) %s"
>
> underlines just the hash, but without the need to re-specify
> the rest of the attributes. This can also help third-party
> programs, like contrib/diff-highlight, that want to turn
> some attribute on/off without disrupting existing coloring.
>
> Note that some attribute specifications are probably
> nonsensical (e.g., "bold nobold"). We do not bother to flag
> such constructs, and instead let the terminal sort it out.
>
> Signed-off-by: Jeff King <peff@peff.net>
Is "white noyellow" a new way to spell "blue" ;-)?
Thanks.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 7/7] diff-highlight: allow configurable colors
2014-11-20 15:14 ` [PATCH 0/7] color fixes and configurable diff-highlight Jeff King
` (5 preceding siblings ...)
2014-11-20 15:25 ` [PATCH 6/7] parse_color: recognize "no$foo" to clear the $foo attribute Jeff King
@ 2014-11-20 15:29 ` Jeff King
6 siblings, 0 replies; 22+ messages in thread
From: Jeff King @ 2014-11-20 15:29 UTC (permalink / raw)
To: Scott Baker; +Cc: git
Until now, the highlighting colors were hard-coded in the
script (as "reverse" and "noreverse"), and you had to edit
the script to change them. This patch teaches diff-highlight
to read from color.diff-highlight.* to set them.
In addition, it expands the possiblities considerably by
adding two features:
1. Old/new lines can be colored independently (so you can
use a color scheme that complements existing line
coloring).
2. Normal, unhighlighted parts of the lines can be colored,
too. Technically this can be done by separately
configuring color.diff.old/new and matching it to your
diff-highlight colors. But you may want a different
look for your highlighted diffs versus your regular
diffs.
Signed-off-by: Jeff King <peff@peff.net>
---
I originally designed the (2) feature above to do the GitHub-style
background outlining that you mentioned. But it occurs to me that you
probably _do_ want to set your color.diff.old and color.diff.new
variables, as diff-highlight will only ever touch highlighted lines (so
a hunk with a single added line will not be touched at all). So I dunno.
Maybe part (2) here should be dropped, as it adds a reasonable amount of
complexity (just read the docs below to see what I mean).
I do not plan on using the feature myself. After experimenting with
several color options, I do not really like any of them, and am
perfectly happy to stick with "reverse/noreverse". :)
contrib/diff-highlight/README | 41 +++++++++++++++++++++++
contrib/diff-highlight/diff-highlight | 62 +++++++++++++++++++++++++++--------
2 files changed, 90 insertions(+), 13 deletions(-)
diff --git a/contrib/diff-highlight/README b/contrib/diff-highlight/README
index 502e03b..836b97a 100644
--- a/contrib/diff-highlight/README
+++ b/contrib/diff-highlight/README
@@ -58,6 +58,47 @@ following in your git configuration:
diff = diff-highlight | less
---------------------------------------------
+
+Color Config
+------------
+
+You can configure the highlight colors and attributes using git's
+config. The colors for "old" and "new" lines can be specified
+independently. There are two "modes" of configuration:
+
+ 1. You can specify a "highlight" color and a matching "reset" color.
+ This will retain any existing colors in the diff, and apply the
+ "highlight" and "reset" colors before and after the highlighted
+ portion.
+
+ 2. You can specify a "normal" color and a "highlight" color. In this
+ case, existing colors are dropped from that line. The non-highlighted
+ bits of the line get the "normal" color, and the highlights get the
+ "highlight" color.
+
+If no "new" colors are specified, they default to the "old" colors. If
+no "old" colors are specified, the default is to reverse the foreground
+and background for highlighted portions.
+
+Examples:
+
+---------------------------------------------
+# Underline highlighted portions
+[color "diff-highlight"]
+oldHighlight = ul
+oldReset = noul
+---------------------------------------------
+
+---------------------------------------------
+# Varying background intensities
+[color "diff-highlight"]
+oldNormal = "black #f8cbcb"
+oldHighlight = "black #ffaaaa"
+newNormal = "black #cbeecb"
+newHighlight = "black #aaffaa"
+---------------------------------------------
+
+
Bugs
----
diff --git a/contrib/diff-highlight/diff-highlight b/contrib/diff-highlight/diff-highlight
index 69a652e..08c88bb 100755
--- a/contrib/diff-highlight/diff-highlight
+++ b/contrib/diff-highlight/diff-highlight
@@ -5,8 +5,18 @@ use strict;
# Highlight by reversing foreground and background. You could do
# other things like bold or underline if you prefer.
-my $HIGHLIGHT = "\x1b[7m";
-my $UNHIGHLIGHT = "\x1b[27m";
+my @OLD_HIGHLIGHT = (
+ color_config('color.diff-highlight.oldnormal'),
+ color_config('color.diff-highlight.oldhighlight', "\x1b[7m"),
+ color_config('color.diff-highlight.oldreset', "\x1b[27m")
+);
+my @NEW_HIGHLIGHT = (
+ color_config('color.diff-highlight.newnormal', $OLD_HIGHLIGHT[0]),
+ color_config('color.diff-highlight.newhighlight', $OLD_HIGHLIGHT[1]),
+ color_config('color.diff-highlight.newreset', $OLD_HIGHLIGHT[2])
+);
+
+my $RESET = "\x1b[m";
my $COLOR = qr/\x1b\[[0-9;]*m/;
my $BORING = qr/$COLOR|\s/;
@@ -57,6 +67,17 @@ show_hunk(\@removed, \@added);
exit 0;
+# Ideally we would feed the default as a human-readable color to
+# git-config as the fallback value. But diff-highlight does
+# not otherwise depend on git at all, and there are reports
+# of it being used in other settings. Let's handle our own
+# fallback, which means we will work even if git can't be run.
+sub color_config {
+ my ($key, $default) = @_;
+ my $s = `git config --get-color $key 2>/dev/null`;
+ return length($s) ? $s : $default;
+}
+
sub show_hunk {
my ($a, $b) = @_;
@@ -132,8 +153,8 @@ sub highlight_pair {
}
if (is_pair_interesting(\@a, $pa, $sa, \@b, $pb, $sb)) {
- return highlight_line(\@a, $pa, $sa),
- highlight_line(\@b, $pb, $sb);
+ return highlight_line(\@a, $pa, $sa, \@OLD_HIGHLIGHT),
+ highlight_line(\@b, $pb, $sb, \@NEW_HIGHLIGHT);
}
else {
return join('', @a),
@@ -148,15 +169,30 @@ sub split_line {
}
sub highlight_line {
- my ($line, $prefix, $suffix) = @_;
-
- return join('',
- @{$line}[0..($prefix-1)],
- $HIGHLIGHT,
- @{$line}[$prefix..$suffix],
- $UNHIGHLIGHT,
- @{$line}[($suffix+1)..$#$line]
- );
+ my ($line, $prefix, $suffix, $theme) = @_;
+
+ my $start = join('', @{$line}[0..($prefix-1)]);
+ my $mid = join('', @{$line}[$prefix..$suffix]);
+ my $end = join('', @{$line}[($suffix+1)..$#$line]);
+
+ # If we have a "normal" color specified, then take over the whole line.
+ # Otherwise, we try to just manipulate the highlighted bits.
+ if (defined $theme->[0]) {
+ s/$COLOR//g for ($start, $mid, $end);
+ chomp $end;
+ return join('',
+ $theme->[0], $start, $RESET,
+ $theme->[1], $mid, $RESET,
+ $theme->[0], $end, $RESET,
+ "\n"
+ );
+ } else {
+ return join('',
+ $start,
+ $theme->[1], $mid, $theme->[2],
+ $end
+ );
+ }
}
# Pairs are interesting to highlight only if we are going to end up
--
2.2.0.rc2.402.g4519813
^ permalink raw reply related [flat|nested] 22+ messages in thread