[PATCH 0/3] blame: allow blame.coloring to specify both --color-lines and --color-by-age

[PATCH 0/3] blame: allow blame.coloring to specify both --color-lines and --color-by-age

[Robert Estelle via GitGitGadget]

The flags --color-lines and --color-by-age are not mutually exclusive, and
both affect the output format of git blame. This adds a test verifying git
blame's output when run with both, where previously they were only tested
independently.

blame is then taught to accept multiple values from blame.coloring, so that
both modes can be set by configuration. Previously, the config only
permitted one (repeatedLines) or the other (highlightRecent), despite the
command line accepting both flags independently. This is a
backwards-compatible change that does not alter the interpretation of
existing configuration.

The list-parsing logic was derived from that of parse_color_moved_ws in
diff.c.

Robert Estelle (3):
  blame: add test for --color-lines + --color-by-age
  blame: document --color-{lines,by-age} config
  blame: support multiple values in blame.coloring

 Documentation/blame-options.txt |  4 ++
 Documentation/config/blame.txt  |  1 +
 builtin/blame.c                 | 47 ++++++++++++++++------
 t/t8012-blame-colors.sh         | 71 +++++++++++++++++++++++++++++++++
 4 files changed, 111 insertions(+), 12 deletions(-)


base-commit: 2fc9e9ca3c7505bc60069f11e7ef09b1aeeee473
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1449%2Frwe%2Fblame-color-config-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1449/rwe/blame-color-config-v1
Pull-Request: https://github.com/git/git/pull/1449
-- 
gitgitgadget

[PATCH 1/3] blame: add test for --color-lines + --color-by-age

[Robert Estelle via GitGitGadget]

From: Robert Estelle <robertestelle@gmail.com>

This adds a test verifying `git blame`'s output when run with both
`--color-lines` and `--color-by-age`. Those flags are not mutually
exclusive and both affect the output format. They were previously only
tested independently.

Signed-off-by: Robert Estelle <robertestelle@gmail.com>
---
 t/t8012-blame-colors.sh | 63 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/t/t8012-blame-colors.sh b/t/t8012-blame-colors.sh
index c3a5f6d01ff..820f86c3aed 100755
--- a/t/t8012-blame-colors.sh
+++ b/t/t8012-blame-colors.sh
@@ -49,4 +49,67 @@ test_expect_success 'blame color by age: new code is different' '
 	grep qfunc colored
 '
 
+# shellcheck disable=SC2317
+re_normalize_color_decoded_blame() {
+	# Construct the regex used by `normalize_color_decoded_blame`.
+	# This is simply for documentation: line comments aren't permitted in
+	# backslash-continuation lines, and POSIX sh does't support 'x+=' syntax.
+	printf '%s' '\(<[^>]*>\)'     # 1: capture the "<YELLOW>" etc
+	printf '%s' ' *'              # -- discard any spaces
+	printf '%s' '[0-9a-f]\{1,\}'  # -- discard the commit ID
+	printf '%s' ' *'              # -- discard any spaces
+	printf '%s' '('               # -- left paren
+	printf '%s' '\(.\)'           # 2: capture the single-char A/F/G/etc
+	printf '%s' '[^\)]*'          # -- discard author and timestamp stuff
+	printf '%s' '\([0-9]\)\{1,\}' # 3: capture the line number
+	printf '%s' ')'               # -- right paren
+	printf '%s' ' *'              # -- discard leading spaces
+	printf '%s' '\(.*$\)'         # 4: capture the remainder
+}
+
+# shellcheck disable=SC2317
+normalize_color_decoded_blame() {
+	# Reads from stdin and writes to stdout.
+	# Removes the commit ID and author/timestamp from blame output after
+	# "test_decode_color" has run.
+	#
+	# This is simply to make expected  outputs easier to describe
+	# and compare without having to refer to magic line counts.
+	re="$(re_normalize_color_decoded_blame)" || return $?
+	sed -e 's/^'"${re}"'/\1 (\2 \3) \4/'
+}
+
+test_expect_success 'blame color by age and lines' '
+	git \
+		-c color.blame.repeatedLines=blue \
+		-c color.blame.highlightRecent="yellow,1 month ago, cyan" \
+		blame \
+		--color-lines \
+		--color-by-age \
+		hello.c \
+		>actual.raw &&
+
+	test_decode_color <actual.raw >actual &&
+	normalize_color_decoded_blame <actual >actual.norm &&
+
+	normalize_color_decoded_blame >expected.norm <<-EOF &&
+		<YELLOW>11111111 (H ... 1)  <RESET>#include <stdio.h>
+		<YELLOW>22222222 (F ... 2)  <RESET>int main(int argc, const char *argv[])
+		<BLUE>  22222222 (F ... 3)  <RESET>{
+		<BLUE>  22222222 (F ... 4)  <RESET>	puts("hello");
+		<YELLOW>33333333 (G ... 5)  <RESET>	puts("goodbye");
+		<YELLOW>22222222 (F ... 6)  <RESET>}
+		<YELLOW>11111111 (H ... 7)  <RESET>void mail()
+		<BLUE>  11111111 (H ... 8)  <RESET>{
+		<BLUE>  11111111 (H ... 9)  <RESET>	puts("mail");
+		<BLUE>  11111111 (H ... 10) <RESET>}
+		<CYAN>  44444444 (A ... 11) <RESET>void qfunc();
+		EOF
+
+	test_cmp actual.norm expected.norm &&
+
+	grep "<YELLOW>" <actual.norm >sanity-check &&
+	test_line_count = 5 sanity-check
+'
+
 test_done
-- 
gitgitgadget

[PATCH 2/3] blame: document --color-{lines,by-age} config

[Robert Estelle via GitGitGadget]

From: Robert Estelle <robertestelle@gmail.com>

These flags correspond to specific values of the `blame.coloring` config
option. This adds a small note mentioning this correspondence.

Signed-off-by: Robert Estelle <robertestelle@gmail.com>
---
 Documentation/blame-options.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 9a663535f44..170016c941e 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -141,11 +141,15 @@ take effect.
 	the same commit as the preceding line. This makes it easier to distinguish
 	code blocks introduced by different commits. The color defaults to cyan and
 	can be adjusted using the `color.blame.repeatedLines` config option.
+	It corresponds to the value `repeatedLines` in the `blame.coloring`
+	configuration setting in linkgit:git-config[1] .
 
 --color-by-age::
 	Color line annotations depending on the age of the line in the default format.
 	The `color.blame.highlightRecent` config option controls what color is used for
 	each range of age.
+	It corresponds to the value `highlightRecent` in the `blame.coloring`
+	configuration setting in linkgit:git-config[1] .
 
 -h::
 	Show help message.
-- 
gitgitgadget

[PATCH 3/3] blame: support multiple values in blame.coloring

[Robert Estelle via GitGitGadget]

From: Robert Estelle <robertestelle@gmail.com>

`blame.coloring` is now parsed as a comma-separated list, so that both
colorizers ("by age" and "by repeated lines") can be enabled via
configuration. Previously, they could be specified together on the
command line via --color-by-age and --color-lines, but not by config.

Signed-off-by: Robert Estelle <robertestelle@gmail.com>
---
 Documentation/config/blame.txt |  1 +
 builtin/blame.c                | 47 +++++++++++++++++++++++++---------
 t/t8012-blame-colors.sh        |  8 ++++++
 3 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/Documentation/config/blame.txt b/Documentation/config/blame.txt
index 4d047c17908..8624c4dc5d4 100644
--- a/Documentation/config/blame.txt
+++ b/Documentation/config/blame.txt
@@ -6,6 +6,7 @@ blame.coloring::
 	This determines the coloring scheme to be applied to blame
 	output. It can be 'repeatedLines', 'highlightRecent',
 	or 'none' which is the default.
+	Multiple values may be separated by commas.
 
 blame.date::
 	Specifies the format used to output dates in linkgit:git-blame[1].
diff --git a/builtin/blame.c b/builtin/blame.c
index 71f925e456c..eea1418d209 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -685,6 +685,39 @@ static const char *add_prefix(const char *prefix, const char *path)
 	return prefix_path(prefix, prefix ? strlen(prefix) : 0, path);
 }
 
+static unsigned parse_blame_coloring_mode(const char *arg)
+{
+	int ret = 0;
+	struct string_list l = STRING_LIST_INIT_DUP;
+	struct string_list_item *i;
+
+	string_list_split(&l, arg, ',', -1);
+
+	for_each_string_list_item(i, &l) {
+		struct strbuf sb = STRBUF_INIT;
+		strbuf_addstr(&sb, i->string);
+		strbuf_trim(&sb);
+
+		if (!strcmp(sb.buf, "none")) {
+			ret = 0;
+		} else if (!strcmp(sb.buf, "repeatedLines")) {
+			ret |= OUTPUT_COLOR_LINE;
+		} else if (!strcmp(sb.buf, "highlightRecent")) {
+			ret |= OUTPUT_SHOW_AGE_WITH_COLOR;
+		} else {
+			warning(_("invalid value for '%s': '%s'"),
+				"blame.coloring", sb.buf);
+			ret = 0;
+		}
+
+		strbuf_release(&sb);
+	}
+
+	string_list_clear(&l, 0);
+
+	return ret;
+}
+
 static int git_blame_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "blame.showroot")) {
@@ -739,18 +772,8 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 	}
 
 	if (!strcmp(var, "blame.coloring")) {
-		if (!strcmp(value, "repeatedLines")) {
-			coloring_mode |= OUTPUT_COLOR_LINE;
-		} else if (!strcmp(value, "highlightRecent")) {
-			coloring_mode |= OUTPUT_SHOW_AGE_WITH_COLOR;
-		} else if (!strcmp(value, "none")) {
-			coloring_mode &= ~(OUTPUT_COLOR_LINE |
-					    OUTPUT_SHOW_AGE_WITH_COLOR);
-		} else {
-			warning(_("invalid value for '%s': '%s'"),
-				"blame.coloring", value);
-			return 0;
-		}
+		coloring_mode = parse_blame_coloring_mode(value);
+		return 0;
 	}
 
 	if (git_diff_heuristic_config(var, value, cb) < 0)
diff --git a/t/t8012-blame-colors.sh b/t/t8012-blame-colors.sh
index 820f86c3aed..051f734ea03 100755
--- a/t/t8012-blame-colors.sh
+++ b/t/t8012-blame-colors.sh
@@ -89,6 +89,14 @@ test_expect_success 'blame color by age and lines' '
 		hello.c \
 		>actual.raw &&
 
+	git \
+		-c color.blame.repeatedLines=blue \
+		-c color.blame.highlightRecent="yellow,1 month ago, cyan" \
+		-c blame.coloring=highlightRecent,repeatedLines \
+		blame hello.c \
+		>actual.raw.2 &&
+	test_cmp actual.raw actual.raw.2 &&
+
 	test_decode_color <actual.raw >actual &&
 	normalize_color_decoded_blame <actual >actual.norm &&
 
-- 
gitgitgadget