From c2f41bf521b5b2ffb9ea93b98e4a57bf73d70864 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Thu, 19 Jan 2017 18:41:21 +0700 Subject: [PATCH 1/5] color.c: fix color_parse_mem() with value_len == 0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In this code we want to match the word "reset". If len is zero, strncasecmp() will return zero and we incorrectly assume it's "reset" as a result. Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- color.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/color.c b/color.c index 81c2676723..a9eadd190a 100644 --- a/color.c +++ b/color.c @@ -207,6 +207,9 @@ int color_parse_mem(const char *value, int value_len, char *dst) struct color fg = { COLOR_UNSPECIFIED }; struct color bg = { COLOR_UNSPECIFIED }; + if (!len) + return -1; + if (!strncasecmp(value, "reset", len)) { xsnprintf(dst, end - dst, GIT_COLOR_RESET); return 0; From bc4075653e3f704f0440ec54e16f88fbc39a682d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Thu, 19 Jan 2017 18:41:22 +0700 Subject: [PATCH 2/5] color.c: trim leading spaces in color_parse_mem() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Normally color_parse_mem() is called from config parser which trims the leading spaces already. The new caller in the next patch won't. Let's be tidy and trim leading spaces too (we already trim trailing spaces after a word). Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- color.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/color.c b/color.c index a9eadd190a..7bb4a96f8c 100644 --- a/color.c +++ b/color.c @@ -207,10 +207,15 @@ int color_parse_mem(const char *value, int value_len, char *dst) struct color fg = { COLOR_UNSPECIFIED }; struct color bg = { COLOR_UNSPECIFIED }; + while (len > 0 && isspace(*ptr)) { + ptr++; + len--; + } + if (!len) return -1; - if (!strncasecmp(value, "reset", len)) { + if (!strncasecmp(ptr, "reset", len)) { xsnprintf(dst, end - dst, GIT_COLOR_RESET); return 0; } From 73c727d69f47572bf7f21fa31831f9a3fdad944c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nguy=E1=BB=85n=20Th=C3=A1i=20Ng=E1=BB=8Dc=20Duy?= Date: Thu, 19 Jan 2017 18:41:23 +0700 Subject: [PATCH 3/5] log --graph: customize the graph lines with config log.graphColors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you have a 256 colors terminal (or one with true color support), then the predefined 12 colors seem limited. On the other hand, you don't want to draw graph lines with every single color in this mode because the two colors could look extremely similar. This option allows you to hand pick the colors you want. Even with standard terminal, if your background color is neither black or white, then the graph line may match your background and become hidden. You can exclude your background color (or simply the colors you hate) with this. Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano --- Documentation/config.txt | 4 ++++ graph.c | 40 +++++++++++++++++++++++++++++++++++++--- t/t4202-log.sh | 22 ++++++++++++++++++++++ 3 files changed, 63 insertions(+), 3 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 0bcb6790d6..33a007b52e 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2003,6 +2003,10 @@ log.follow:: i.e. it cannot be used to follow multiple files and does not work well on non-linear history. +log.graphColors:: + A list of colors, separated by commas, that can be used to draw + history lines in `git log --graph`. + log.showRoot:: If true, the initial commit will be shown as a big creation event. This is equivalent to a diff against an empty tree. diff --git a/graph.c b/graph.c index dd1720148d..00aeee36d8 100644 --- a/graph.c +++ b/graph.c @@ -4,6 +4,7 @@ #include "graph.h" #include "diff.h" #include "revision.h" +#include "argv-array.h" /* Internal API */ @@ -62,6 +63,26 @@ enum graph_state { static const char **column_colors; static unsigned short column_colors_max; +static void parse_graph_colors_config(struct argv_array *colors, const char *string) +{ + const char *end, *start; + + start = string; + end = string + strlen(string); + while (start < end) { + const char *comma = strchrnul(start, ','); + char color[COLOR_MAXLEN]; + + if (!color_parse_mem(start, comma - start, color)) + argv_array_push(colors, color); + else + warning(_("ignore invalid color '%.*s' in log.graphColors"), + (int)(comma - start), start); + start = comma + 1; + } + argv_array_push(colors, GIT_COLOR_RESET); +} + void graph_set_column_colors(const char **colors, unsigned short colors_max) { column_colors = colors; @@ -207,9 +228,22 @@ struct git_graph *graph_init(struct rev_info *opt) { struct git_graph *graph = xmalloc(sizeof(struct git_graph)); - if (!column_colors) - graph_set_column_colors(column_colors_ansi, - column_colors_ansi_max); + if (!column_colors) { + char *string; + if (git_config_get_string("log.graphcolors", &string)) { + /* not configured -- use default */ + graph_set_column_colors(column_colors_ansi, + column_colors_ansi_max); + } else { + static struct argv_array custom_colors = ARGV_ARRAY_INIT; + argv_array_clear(&custom_colors); + parse_graph_colors_config(&custom_colors, string); + free(string); + /* graph_set_column_colors takes a max-index, not a count */ + graph_set_column_colors(custom_colors.argv, + custom_colors.argc - 1); + } + } graph->commit = NULL; graph->revs = opt; diff --git a/t/t4202-log.sh b/t/t4202-log.sh index e2db47c36e..0aeabed96d 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -313,6 +313,28 @@ test_expect_success 'log --graph with merge' ' test_cmp expect actual ' +cat > expect.colors <<\EOF +* Merge branch 'side' +|\ +| * side-2 +| * side-1 +* | Second +* | sixth +* | fifth +* | fourth +|/ +* third +* second +* initial +EOF + +test_expect_success 'log --graph with merge with log.graphColors' ' + test_config log.graphColors ",, blue,invalid-color, cyan, red , " && + git log --color=always --graph --date-order --pretty=tformat:%s | + test_decode_color | sed "s/ *\$//" >actual && + test_cmp expect.colors actual +' + test_expect_success 'log --raw --graph -m with merge' ' git log --raw --graph --oneline -m master | head -n 500 >actual && grep "initial" actual From 55cccf4bb3d7e99223b8ebb3d4480f73c456dd61 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 1 Feb 2017 01:21:29 +0100 Subject: [PATCH 4/5] color_parse_mem: allow empty color spec Prior to c2f41bf52 (color.c: fix color_parse_mem() with value_len == 0, 2017-01-19), the empty string was interpreted as a color "reset". This was an accidental outcome, and that commit turned it into an error. However, scripts may pass the empty string as a default value to "git config --get-color" to disable color when the value is not defined. The git-add--interactive script does this. As a result, the script is unusable since c2f41bf52 unless you have color.diff.plain defined (if it is defined, then we don't parse the empty default at all). Our test scripts didn't notice the recent breakage because they run without a terminal, and thus without color. They never hit this code path at all. And nobody noticed the original buggy "reset" behavior, because it was effectively a noop. Let's fix the code to have an empty color name produce an empty sequence of color codes. The tests need a few fixups: - we'll add a new test in t4026 to cover this case. But note that we need to tweak the color() helper. While we're there, let's factor out the literal ANSI ESC character. Otherwise it makes the diff quite hard to read. - we'll add a basic sanity-check in t4026 that "git add -p" works at all when color is enabled. That would have caught this bug, as well as any others that are specific to the color code paths. - 73c727d69 (log --graph: customize the graph lines with config log.graphColors, 2017-01-19) added a test to t4202 that checks some "invalid" graph color config. Since ",, blue" before yielded only "blue" as valid, and now yields "empty, empty, blue", we don't match the expected output. One way to fix this would be to change the expectation to the empty color strings. But that makes the test much less interesting, since we show only two graph lines, both of which would be colorless. Since the empty-string case is now covered by t4026, let's remove them entirely here. They're just in the way of the primary thing the test is supposed to be checking. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- color.c | 6 ++++-- t/t3701-add-interactive.sh | 14 ++++++++++++++ t/t4026-color.sh | 7 ++++++- t/t4202-log.sh | 2 +- 4 files changed, 25 insertions(+), 4 deletions(-) diff --git a/color.c b/color.c index 7bb4a96f8c..2925a819b2 100644 --- a/color.c +++ b/color.c @@ -212,8 +212,10 @@ int color_parse_mem(const char *value, int value_len, char *dst) len--; } - if (!len) - return -1; + if (!len) { + dst[0] = '\0'; + return 0; + } if (!strncasecmp(ptr, "reset", len)) { xsnprintf(dst, end - dst, GIT_COLOR_RESET); diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index deae948c76..5ffe78e920 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -380,4 +380,18 @@ test_expect_success 'patch mode ignores unmerged entries' ' test_cmp expected diff ' +test_expect_success 'diffs can be colorized' ' + git reset --hard && + + # force color even though the test script has no terminal + test_config color.ui always && + + echo content >test && + printf y | git add -p >output 2>&1 && + + # We do not want to depend on the exact coloring scheme + # git uses for diffs, so just check that we saw some kind of color. + grep "$(printf "\\033")" output +' + test_done diff --git a/t/t4026-color.sh b/t/t4026-color.sh index ec78c5e3ac..671e951ee5 100755 --- a/t/t4026-color.sh +++ b/t/t4026-color.sh @@ -6,10 +6,11 @@ test_description='Test diff/status color escape codes' . ./test-lib.sh +ESC=$(printf '\033') color() { actual=$(git config --get-color no.such.slot "$1") && - test "$actual" = "$2" + test "$actual" = "${2:+$ESC}$2" } invalid_color() @@ -21,6 +22,10 @@ test_expect_success 'reset' ' color "reset" "[m" ' +test_expect_success 'empty color is empty' ' + color "" "" +' + test_expect_success 'attribute before color name' ' color "bold red" "[1;31m" ' diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 0aeabed96d..1edbb1e7f1 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -329,7 +329,7 @@ cat > expect.colors <<\EOF EOF test_expect_success 'log --graph with merge with log.graphColors' ' - test_config log.graphColors ",, blue,invalid-color, cyan, red , " && + test_config log.graphColors " blue,invalid-color, cyan, red , " && git log --color=always --graph --date-order --pretty=tformat:%s | test_decode_color | sed "s/ *\$//" >actual && test_cmp expect.colors actual From 512aba261a8951a470147b493c720f205638ba14 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 2 Feb 2017 13:42:44 +0100 Subject: [PATCH 5/5] document behavior of empty color name Commit 55cccf4bb (color_parse_mem: allow empty color spec, 2017-02-01) clearly defined the behavior of an empty color config variable. Let's document that, and give a hint about why it might be useful. It's important not to say that it makes the item uncolored, because it doesn't. It just sets no attributes, which means that any previous attributes continue to take effect. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/config.txt | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index 33a007b52e..49b264566c 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -170,6 +170,9 @@ The position of any attributes with respect to the colors be turned off by prefixing them with `no` or `no-` (e.g., `noreverse`, `no-ul`, etc). + +An empty color string produces no color effect at all. This can be used +to avoid coloring specific elements without disabling color entirely. ++ For git's pre-defined color slots, the attributes are meant to be reset at the beginning of each item in the colored output. So setting `color.decorate.branch` to `black` will paint that branch name in a