[RFC PATCH 0/4] blame: (dim rep. metadata lines or fields, decay date coloring)

[RFC PATCH 0/4] blame: (dim rep. metadata lines or fields, decay date coloring)

[Stefan Beller]

This is picking up [1], but presenting it in another approach,
as I realized these are orthogonal features:
* dimming repeated lines/fields of information
* giving a quick visual information how old (as a proxy for 'well tested')
  a line of code is.
  
Both features are configurable.

Any feedback welcome.

Thanks,
Stefan

[1] https://public-inbox.org/git/20170613023151.9688-1-sbeller@google.com/

Stefan Beller (4):
  color.h: modernize header
  builtin/blame: dim uninteresting metadata
  builtin/blame: add option to color metadata fields separately
  builtin/blame: highlight recently changed lines

 Documentation/config.txt |  25 ++++++
 builtin/blame.c          | 216 ++++++++++++++++++++++++++++++++++++++++++-----
 color.c                  |   2 -
 color.h                  |  49 ++++++++---
 t/t8012-blame-colors.sh  |  56 ++++++++++++
 5 files changed, 313 insertions(+), 35 deletions(-)
 create mode 100755 t/t8012-blame-colors.sh

-- 
2.15.0.128.gcadd42da22

[PATCH 1/4] color.h: modernize header

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 color.c |  2 --
 color.h | 48 +++++++++++++++++++++++++++++++++++-------------
 2 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/color.c b/color.c
index 9a9261ac16..e43da3ce53 100644
--- a/color.c
+++ b/color.c
@@ -400,8 +400,6 @@ static int color_vfprintf(FILE *fp, const char *color, const char *fmt,
 	return r;
 }
 
-
-
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...)
 {
 	va_list args;
diff --git a/color.h b/color.h
index fd2b688dfb..6cd632c0d8 100644
--- a/color.h
+++ b/color.h
@@ -72,26 +72,48 @@ extern int color_stdout_is_tty;
  * Use the first one if you need only color config; the second is a convenience
  * if you are just going to change to git_default_config, too.
  */
-int git_color_config(const char *var, const char *value, void *cb);
-int git_color_default_config(const char *var, const char *value, void *cb);
+extern int git_color_config(const char *var, const char *value, void *cb);
+extern int git_color_default_config(const char *var, const char *value, void *cb);
 
 /*
- * Set the color buffer (which must be COLOR_MAXLEN bytes)
- * to the raw color bytes; this is useful for initializing
+ * Set the color buffer `dst` (which must be COLOR_MAXLEN bytes)
+ * to the raw color bytes `color_bytes`; this is useful for initializing
  * default color variables.
  */
-void color_set(char *dst, const char *color_bytes);
+extern void color_set(char *dst, const char *color_bytes);
 
-int git_config_colorbool(const char *var, const char *value);
-int want_color(int var);
-int color_parse(const char *value, char *dst);
-int color_parse_mem(const char *value, int len, char *dst);
+/*
+ * Parses a config option, which can be a boolean or one of
+ * "never", "auto", "always". Returns the constant for the given setting.
+ */
+extern int git_config_colorbool(const char *var, const char *value);
+
+/* Is the output supposed to be colored? Resolve and cache the 'auto' setting */
+extern int want_color(int var);
+
+/*
+ * Translate the human readable color string from `value` and into
+ * terminal color codes and store them in `dst`
+ */
+extern int color_parse(const char *value, char *dst);
+extern int color_parse_mem(const char *value, int len, char *dst);
+
+/*
+ * Print the format string `fmt`, encapsulated by setting and resetting the
+ * color. Omits the color encapsulation if `color` is NULL.
+ * The `color_fprintf_ln` prints a new line after resetting the color.
+ * The `color_print_strbuf` prints the given pre-formatted strbuf instead.
+ */
 __attribute__((format (printf, 3, 4)))
-int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
+extern int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
-int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
-void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
+extern int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
+extern void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
 
-int color_is_nil(const char *color);
+/*
+ * Check if the given color is GIT_COLOR_NIL that means "no color selected".
+ * The application needs to replace the color with the actual desired color.
+ */
+extern int color_is_nil(const char *color);
 
 #endif /* COLOR_H */
-- 
2.15.0.128.gcadd42da22

[PATCH 2/4] builtin/blame: dim uninteresting metadata

[Stefan Beller]

When using git-blame lots of lines contain redundant information, for
example in hunks that consist of multiple lines, the metadata (commit
name, author, date) are repeated. A reader may not be interested in those,
so offer an option to color the information that is repeated from the
previous line differently.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/config.txt |  5 ++++
 builtin/blame.c          | 68 ++++++++++++++++++++++++++++++++++++++----------
 color.h                  |  1 +
 t/t8012-blame-colors.sh  | 18 +++++++++++++
 4 files changed, 78 insertions(+), 14 deletions(-)
 create mode 100755 t/t8012-blame-colors.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5f0d62753d..85dfdc6a9b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1189,6 +1189,11 @@ color.status.<slot>::
 	status short-format), or
 	`unmerged` (files which have unmerged changes).
 
+color.blame.repeatedMeta::
+	Use the customized color for the part of git-blame output that
+	is repeated meta information per line (such as commit id,
+	author name, date and timezone). Defaults to dark gray.
+
 color.ui::
 	This variable determines the default value for variables such
 	as `color.diff` and `color.grep` that control the use of color
diff --git a/builtin/blame.c b/builtin/blame.c
index 67adaef4d8..d15e901357 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -7,6 +7,7 @@
 
 #include "cache.h"
 #include "config.h"
+#include "color.h"
 #include "builtin.h"
 #include "commit.h"
 #include "diff.h"
@@ -46,6 +47,7 @@ static int xdl_opts;
 static int abbrev = -1;
 static int no_whole_file_rename;
 static int show_progress;
+static char *repeated_meta_color;
 
 static struct date_mode blame_date_mode = { DATE_ISO8601 };
 static size_t blame_date_width;
@@ -316,10 +318,11 @@ static const char *format_time(timestamp_t time, const char *tz_str,
 #define OUTPUT_PORCELAIN	010
 #define OUTPUT_SHOW_NAME	020
 #define OUTPUT_SHOW_NUMBER	040
-#define OUTPUT_SHOW_SCORE      0100
-#define OUTPUT_NO_AUTHOR       0200
+#define OUTPUT_SHOW_SCORE	0100
+#define OUTPUT_NO_AUTHOR	0200
 #define OUTPUT_SHOW_EMAIL	0400
-#define OUTPUT_LINE_PORCELAIN 01000
+#define OUTPUT_LINE_PORCELAIN 	01000
+#define OUTPUT_COLOR_LINE	02000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -367,6 +370,28 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
 		putchar('\n');
 }
 
+static inline void colors_unset(const char **use_color, const char **reset_color)
+{
+	*use_color = "";
+	*reset_color = "";
+}
+
+static inline void colors_set(const char **use_color, const char **reset_color)
+{
+	*use_color = repeated_meta_color;
+	*reset_color = GIT_COLOR_RESET;
+}
+
+static void setup_line_color(int opt, int cnt,
+			     const char **use_color,
+			     const char **reset_color)
+{
+	colors_unset(use_color, reset_color);
+
+	if ((opt & OUTPUT_COLOR_LINE) && cnt > 0)
+		colors_set(use_color, reset_color);
+}
+
 static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int opt)
 {
 	int cnt;
@@ -383,6 +408,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 	for (cnt = 0; cnt < ent->num_lines; cnt++) {
 		char ch;
 		int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : abbrev;
+		const char *col, *rcol;
 
 		if (suspect->commit->object.flags & UNINTERESTING) {
 			if (blank_boundary)
@@ -393,7 +419,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 			}
 		}
 
-		printf("%.*s", length, hex);
+		setup_line_color(opt, cnt, &col, &rcol);
+		printf("%s%.*s%s", col, length, hex, rcol);
 		if (opt & OUTPUT_ANNOTATE_COMPAT) {
 			const char *name;
 			if (opt & OUTPUT_SHOW_EMAIL)
@@ -406,16 +433,17 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 			       ent->lno + 1 + cnt);
 		} else {
 			if (opt & OUTPUT_SHOW_SCORE)
-				printf(" %*d %02d",
+				printf(" %s%*d %02d%s", col,
 				       max_score_digits, ent->score,
-				       ent->suspect->refcnt);
+				       ent->suspect->refcnt, rcol);
 			if (opt & OUTPUT_SHOW_NAME)
-				printf(" %-*.*s", longest_file, longest_file,
-				       suspect->path);
+				printf(" %s%-*.*s%s", col, longest_file,
+						      longest_file,
+						      suspect->path,
+						      rcol);
 			if (opt & OUTPUT_SHOW_NUMBER)
-				printf(" %*d", max_orig_digits,
-				       ent->s_lno + 1 + cnt);
-
+				printf(" %s%*d%s", col, max_orig_digits,
+				       ent->s_lno + 1 + cnt, rcol);
 			if (!(opt & OUTPUT_NO_AUTHOR)) {
 				const char *name;
 				int pad;
@@ -424,11 +452,12 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 				else
 					name = ci.author.buf;
 				pad = longest_author - utf8_strwidth(name);
-				printf(" (%s%*s %10s",
+				printf(" %s(%s%*s %10s%s", col,
 				       name, pad, "",
 				       format_time(ci.author_time,
 						   ci.author_tz.buf,
-						   show_raw_time));
+						   show_raw_time),
+				       rcol);
 			}
 			printf(" %*d) ",
 			       max_digits, ent->lno + 1 + cnt);
@@ -607,6 +636,12 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 		parse_date_format(value, &blame_date_mode);
 		return 0;
 	}
+	if (!strcmp(var, "color.blame.repeatedmeta")) {
+		if (color_parse_mem(value, strlen(value), repeated_meta_color))
+			warning(_("ignore invalid color '%s' in color.blame.repeatedMeta"),
+				value);
+		return 0;
+	}
 
 	if (git_diff_heuristic_config(var, value, cb) < 0)
 		return -1;
@@ -681,6 +716,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_BIT('s', NULL, &output_option, N_("Suppress author name and timestamp (Default: off)"), OUTPUT_NO_AUTHOR),
 		OPT_BIT('e', "show-email", &output_option, N_("Show author email instead of name (Default: off)"), OUTPUT_SHOW_EMAIL),
 		OPT_BIT('w', NULL, &xdl_opts, N_("Ignore whitespace differences"), XDF_IGNORE_WHITESPACE),
+		OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line"), OUTPUT_COLOR_LINE),
 
 		/*
 		 * The following two options are parsed by parse_revision_opt()
@@ -940,8 +976,12 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 
 	blame_coalesce(&sb);
 
-	if (!(output_option & OUTPUT_PORCELAIN))
+	if (!(output_option & OUTPUT_PORCELAIN)) {
 		find_alignment(&sb, &output_option);
+		if ((output_option & OUTPUT_COLOR_LINE) &&
+		    !repeated_meta_color)
+			repeated_meta_color = GIT_COLOR_DARK;
+	}
 
 	output(&sb, output_option);
 	free((void *)sb.final_buf);
diff --git a/color.h b/color.h
index 6cd632c0d8..fce5588df9 100644
--- a/color.h
+++ b/color.h
@@ -30,6 +30,7 @@ struct strbuf;
 #define GIT_COLOR_BLUE		"\033[34m"
 #define GIT_COLOR_MAGENTA	"\033[35m"
 #define GIT_COLOR_CYAN		"\033[36m"
+#define GIT_COLOR_DARK		"\033[1;30m"
 #define GIT_COLOR_BOLD_RED	"\033[1;31m"
 #define GIT_COLOR_BOLD_GREEN	"\033[1;32m"
 #define GIT_COLOR_BOLD_YELLOW	"\033[1;33m"
diff --git a/t/t8012-blame-colors.sh b/t/t8012-blame-colors.sh
new file mode 100755
index 0000000000..b2de03b777
--- /dev/null
+++ b/t/t8012-blame-colors.sh
@@ -0,0 +1,18 @@
+#!/bin/sh
+
+test_description='colored git blame'
+. ./test-lib.sh
+
+PROG='git blame -c'
+. "$TEST_DIRECTORY"/annotate-tests.sh
+
+test_expect_success 'colored blame colors continuous lines' '
+	git blame --abbrev=12 --color-lines hello.c >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	grep "<BOLD;BLACK>(F" actual > F.expect &&
+	grep "<BOLD;BLACK>(H" actual > H.expect &&
+	test_line_count = 2 F.expect &&
+	test_line_count = 3 H.expect
+'
+
+test_done
-- 
2.15.0.128.gcadd42da22

[PATCH 3/4] builtin/blame: add option to color metadata fields separately

[Stefan Beller]

Unlike the previous commit, this dims colors for each
metadata field individually.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/blame.c         | 82 +++++++++++++++++++++++++++++++++++++++++++------
 t/t8012-blame-colors.sh | 38 +++++++++++++++++++++++
 2 files changed, 111 insertions(+), 9 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index d15e901357..9d460597a2 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -323,6 +323,7 @@ static const char *format_time(timestamp_t time, const char *tz_str,
 #define OUTPUT_SHOW_EMAIL	0400
 #define OUTPUT_LINE_PORCELAIN 	01000
 #define OUTPUT_COLOR_LINE	02000
+#define OUTPUT_COLOR_FIELDS	04000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -370,6 +371,33 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
 		putchar('\n');
 }
 
+static int had_same_field_previously(int opt, int field,
+			  struct blame_entry *ent,
+			  struct blame_entry *prev)
+{
+	struct commit_info ci, prev_ci;
+
+	switch (field) {
+	case OUTPUT_SHOW_SCORE:
+		return ent->score == prev->score;
+	case OUTPUT_SHOW_NAME:
+		return prev->suspect &&
+			!strcmp(ent->suspect->path, prev->suspect->path);
+	case OUTPUT_SHOW_NUMBER:
+		return ent->s_lno == prev->s_lno + prev->num_lines - 1;
+
+	case OUTPUT_NO_AUTHOR:
+		get_commit_info(ent->suspect->commit, &ci, 1);
+		get_commit_info(prev->suspect->commit, &prev_ci, 1);
+		return ((opt & OUTPUT_SHOW_EMAIL) &&
+			!strcmp(ci.author_mail.buf, prev_ci.author_mail.buf)) ||
+			!strcmp(ci.author.buf, prev_ci.author.buf);
+	default:
+		BUG("unknown field");
+	}
+	return 0;
+}
+
 static inline void colors_unset(const char **use_color, const char **reset_color)
 {
 	*use_color = "";
@@ -382,17 +410,36 @@ static inline void colors_set(const char **use_color, const char **reset_color)
 	*reset_color = GIT_COLOR_RESET;
 }
 
+static void setup_field_color(int opt, int cnt, int field,
+			      struct blame_entry *ent,
+			      struct blame_entry *prev,
+			      const char **use_color,
+			      const char **reset_color)
+{
+	if (!(opt & OUTPUT_COLOR_FIELDS))
+		return;
+
+	if ((cnt > 0 ||
+	     (prev && had_same_field_previously(opt, field, ent, prev))))
+		colors_set(use_color, reset_color);
+	else
+		colors_unset(use_color, reset_color);
+}
+
 static void setup_line_color(int opt, int cnt,
 			     const char **use_color,
 			     const char **reset_color)
 {
 	colors_unset(use_color, reset_color);
 
-	if ((opt & OUTPUT_COLOR_LINE) && cnt > 0)
+	if ((opt & (OUTPUT_COLOR_LINE | OUTPUT_COLOR_FIELDS)) && cnt > 0)
 		colors_set(use_color, reset_color);
 }
 
-static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int opt)
+static void emit_other(struct blame_scoreboard *sb,
+		       struct blame_entry *ent,
+		       struct blame_entry *prev,
+		       int opt)
 {
 	int cnt;
 	const char *cp;
@@ -432,18 +479,27 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 					   show_raw_time),
 			       ent->lno + 1 + cnt);
 		} else {
-			if (opt & OUTPUT_SHOW_SCORE)
+			if (opt & OUTPUT_SHOW_SCORE) {
+				setup_field_color(opt, cnt, OUTPUT_SHOW_SCORE,
+						  ent, prev, &col, &rcol);
 				printf(" %s%*d %02d%s", col,
 				       max_score_digits, ent->score,
 				       ent->suspect->refcnt, rcol);
-			if (opt & OUTPUT_SHOW_NAME)
+			}
+			if (opt & OUTPUT_SHOW_NAME) {
+				setup_field_color(opt, cnt, OUTPUT_SHOW_NAME,
+						   ent, prev, &col, &rcol);
 				printf(" %s%-*.*s%s", col, longest_file,
 						      longest_file,
 						      suspect->path,
 						      rcol);
-			if (opt & OUTPUT_SHOW_NUMBER)
+			}
+			if (opt & OUTPUT_SHOW_NUMBER) {
+				setup_field_color(opt, cnt, OUTPUT_SHOW_NUMBER,
+						  ent, prev, &col, &rcol);
 				printf(" %s%*d%s", col, max_orig_digits,
 				       ent->s_lno + 1 + cnt, rcol);
+			}
 			if (!(opt & OUTPUT_NO_AUTHOR)) {
 				const char *name;
 				int pad;
@@ -452,6 +508,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 				else
 					name = ci.author.buf;
 				pad = longest_author - utf8_strwidth(name);
+				setup_field_color(opt, cnt, OUTPUT_NO_AUTHOR,
+						  ent, prev, &col, &rcol);
 				printf(" %s(%s%*s %10s%s", col,
 				       name, pad, "",
 				       format_time(ci.author_time,
@@ -477,7 +535,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 
 static void output(struct blame_scoreboard *sb, int option)
 {
-	struct blame_entry *ent;
+	struct blame_entry *ent, *prev = NULL;
 
 	if (option & OUTPUT_PORCELAIN) {
 		for (ent = sb->ent; ent; ent = ent->next) {
@@ -499,7 +557,8 @@ static void output(struct blame_scoreboard *sb, int option)
 		if (option & OUTPUT_PORCELAIN)
 			emit_porcelain(sb, ent, option);
 		else {
-			emit_other(sb, ent, option);
+			emit_other(sb, ent, prev, option);
+			prev = ent;
 		}
 	}
 }
@@ -717,6 +776,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_BIT('e', "show-email", &output_option, N_("Show author email instead of name (Default: off)"), OUTPUT_SHOW_EMAIL),
 		OPT_BIT('w', NULL, &xdl_opts, N_("Ignore whitespace differences"), XDF_IGNORE_WHITESPACE),
 		OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line"), OUTPUT_COLOR_LINE),
+		OPT_BIT(0, "color-fields", &output_option, N_("color redundant metadata fields from previous line"), OUTPUT_COLOR_FIELDS),
 
 		/*
 		 * The following two options are parsed by parse_revision_opt()
@@ -775,6 +835,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
 	argc = parse_options_end(&ctx);
 
+	if ((output_option & OUTPUT_COLOR_LINE) &&
+	    (output_option & OUTPUT_COLOR_FIELDS))
+		die(_("cannot ask for colored lines and fields at the same time"));
+
 	if (incremental || (output_option & OUTPUT_PORCELAIN)) {
 		if (show_progress > 0)
 			die(_("--progress can't be used with --incremental or porcelain formats"));
@@ -978,8 +1042,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 
 	if (!(output_option & OUTPUT_PORCELAIN)) {
 		find_alignment(&sb, &output_option);
-		if ((output_option & OUTPUT_COLOR_LINE) &&
-		    !repeated_meta_color)
+		if (!repeated_meta_color &&
+		    (output_option & (OUTPUT_COLOR_LINE | OUTPUT_COLOR_FIELDS)))
 			repeated_meta_color = GIT_COLOR_DARK;
 	}
 
diff --git a/t/t8012-blame-colors.sh b/t/t8012-blame-colors.sh
index b2de03b777..be491bde9a 100755
--- a/t/t8012-blame-colors.sh
+++ b/t/t8012-blame-colors.sh
@@ -15,4 +15,42 @@ test_expect_success 'colored blame colors continuous lines' '
 	test_line_count = 3 H.expect
 '
 
+test_expect_success 'colored blame colors continuous fields' '
+
+	git mv hello.c world.c &&
+	git commit -a -m "moved file" &&
+	cat <<-EOF >> world.c &&
+	void world()
+	{
+		puts("world");
+	}
+	EOF
+	git add world.c &&
+	GIT_AUTHOR_NAME="F" GIT_AUTHOR_EMAIL="F@test.git" \
+		git commit -m "forgot to add changes to moved file" &&
+
+	git blame --abbrev=12 --color-fields world.c >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+
+	grep "<BOLD;BLACK>hello.c" actual > colored_hello.expect &&
+	grep "hello.c" actual > all_hello.expect &&
+	test_line_count = 9 colored_hello.expect &&
+	test_line_count = 10 all_hello.expect &&
+
+	grep "<BOLD;BLACK>world.c" actual > colored_world.expect &&
+	grep "world.c" actual > all_world.expect &&
+	test_line_count = 3 colored_world.expect &&
+	test_line_count = 4 all_world.expect &&
+
+	grep "(F" actual > all_F.expect &&
+	grep "<BOLD;BLACK>(F" actual > colored_F.expect &&
+	test_line_count = 8 all_F.expect &&
+	test_line_count = 5 colored_F.expect &&
+
+	grep "(H" actual > all_H.expect &&
+	grep "<BOLD;BLACK>(H" actual > colored_H.expect &&
+	test_line_count = 5 all_H.expect &&
+	test_line_count = 3 colored_H.expect
+'
+
 test_done
-- 
2.15.0.128.gcadd42da22

[PATCH 4/4] builtin/blame: highlight recently changed lines

[Stefan Beller]

Choose a different color for dates and imitate a 'temperature cool down'
for the dates.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/config.txt | 20 +++++++++++++
 builtin/blame.c          | 74 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 93 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 85dfdc6a9b..7d2efc5ad6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1194,6 +1194,26 @@ color.blame.repeatedMeta::
 	is repeated meta information per line (such as commit id,
 	author name, date and timezone). Defaults to dark gray.
 
+color.blame.highlightRecent::
+	This can be used to color the author and date of a blame line.
+	This overrides `color.blame.repeatedMeta` setting, which colors
+	repetitions.
++
+	This setting should be set to a comma-separated list of
+	color and date settings, starting and ending with a color, the dates
+	should be set from oldest to newest.
+	The metadata will be colored given the colors if the the line was
+	introduced before the given timestamp, overwriting older timestamped
+	colors.
++
+	Instead of an absolute timestamp relative timestamps work as
+	well, e.g. 2.weeks.ago is valid to address anything older than 2 weeks.
++
+	It defaults to "blue,12 month ago,white,1 month ago,red",
+	which colors everything older than one year blue, recent changes between
+	one month and one year old are kept white, and lines introduced within
+	the last month are colored red.
+
 color.ui::
 	This variable determines the default value for variables such
 	as `color.diff` and `color.grep` that control the use of color
diff --git a/builtin/blame.c b/builtin/blame.c
index 9d460597a2..e157ee3864 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -24,6 +24,7 @@
 #include "dir.h"
 #include "progress.h"
 #include "blame.h"
+#include "string-list.h"
 
 static char blame_usage[] = N_("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>");
 
@@ -324,6 +325,7 @@ static const char *format_time(timestamp_t time, const char *tz_str,
 #define OUTPUT_LINE_PORCELAIN 	01000
 #define OUTPUT_COLOR_LINE	02000
 #define OUTPUT_COLOR_FIELDS	04000
+#define OUTPUT_HEATED_LINES	010000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -436,6 +438,61 @@ static void setup_line_color(int opt, int cnt,
 		colors_set(use_color, reset_color);
 }
 
+static struct color_field {
+	timestamp_t hop;
+	char col[COLOR_MAXLEN];
+} *colorfield;
+static int colorfield_nr, colorfield_alloc;
+
+static void parse_color_fields(const char *s)
+{
+	struct string_list l = STRING_LIST_INIT_DUP;
+	struct string_list_item *item;
+	enum { EXPECT_DATE, EXPECT_COLOR } next = EXPECT_COLOR;
+
+	/* Ideally this would be stripped and split at the same time? */
+	string_list_split(&l, s, ',', -1);
+	ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
+
+	for_each_string_list_item(item, &l) {
+		switch (next) {
+		case EXPECT_DATE:
+			colorfield[colorfield_nr].hop = approxidate(item->string);
+			next = EXPECT_COLOR;
+			colorfield_nr++;
+			ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
+			break;
+		case EXPECT_COLOR:
+			if (color_parse(item->string, colorfield[colorfield_nr].col))
+				die(_("expecting a color: %s"), item->string);
+			next = EXPECT_DATE;
+			break;
+		}
+	}
+
+	if (next == EXPECT_COLOR)
+		die (_("must end with a color"));
+
+	colorfield[colorfield_nr].hop = TIME_MAX;
+}
+
+static void setup_default_colorfield(void)
+{
+	parse_color_fields("blue,12 month ago,white,1 month ago,red");
+}
+
+static void determine_line_heat(struct blame_entry *ent, const char **dest_color)
+{
+	int i = 0;
+	struct commit_info ci;
+	get_commit_info(ent->suspect->commit, &ci, 1);
+
+	while (i < colorfield_nr && ci.author_time > colorfield[i].hop)
+		i++;
+
+	*dest_color = colorfield[i].col;
+}
+
 static void emit_other(struct blame_scoreboard *sb,
 		       struct blame_entry *ent,
 		       struct blame_entry *prev,
@@ -443,6 +500,7 @@ static void emit_other(struct blame_scoreboard *sb,
 {
 	int cnt;
 	const char *cp;
+	const char *heatcolor = NULL;
 	struct blame_origin *suspect = ent->suspect;
 	struct commit_info ci;
 	char hex[GIT_MAX_HEXSZ + 1];
@@ -452,6 +510,10 @@ static void emit_other(struct blame_scoreboard *sb,
 	oid_to_hex_r(hex, &suspect->commit->object.oid);
 
 	cp = blame_nth_line(sb, ent->lno);
+
+	if (opt & OUTPUT_HEATED_LINES)
+		determine_line_heat(ent, &heatcolor);
+
 	for (cnt = 0; cnt < ent->num_lines; cnt++) {
 		char ch;
 		int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : abbrev;
@@ -503,6 +565,7 @@ static void emit_other(struct blame_scoreboard *sb,
 			if (!(opt & OUTPUT_NO_AUTHOR)) {
 				const char *name;
 				int pad;
+				int hcolor = opt & OUTPUT_HEATED_LINES;
 				if (opt & OUTPUT_SHOW_EMAIL)
 					name = ci.author_mail.buf;
 				else
@@ -510,7 +573,10 @@ static void emit_other(struct blame_scoreboard *sb,
 				pad = longest_author - utf8_strwidth(name);
 				setup_field_color(opt, cnt, OUTPUT_NO_AUTHOR,
 						  ent, prev, &col, &rcol);
-				printf(" %s(%s%*s %10s%s", col,
+				if (hcolor)
+					rcol = GIT_COLOR_RESET;
+				printf(" %s(%s%*s %10s%s",
+				       hcolor ? heatcolor : col,
 				       name, pad, "",
 				       format_time(ci.author_time,
 						   ci.author_tz.buf,
@@ -701,6 +767,10 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 				value);
 		return 0;
 	}
+	if (!strcmp(var, "color.blame.highlightrecent")) {
+		parse_color_fields(value);
+		return 0;
+	}
 
 	if (git_diff_heuristic_config(var, value, cb) < 0)
 		return -1;
@@ -777,6 +847,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_BIT('w', NULL, &xdl_opts, N_("Ignore whitespace differences"), XDF_IGNORE_WHITESPACE),
 		OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line"), OUTPUT_COLOR_LINE),
 		OPT_BIT(0, "color-fields", &output_option, N_("color redundant metadata fields from previous line"), OUTPUT_COLOR_FIELDS),
+		OPT_BIT(0, "heated-lines", &output_option, N_("color lines by date"), OUTPUT_HEATED_LINES),
 
 		/*
 		 * The following two options are parsed by parse_revision_opt()
@@ -801,6 +872,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	unsigned int range_i;
 	long anchor;
 
+	setup_default_colorfield();
 	git_config(git_blame_config, &output_option);
 	init_revisions(&revs, NULL);
 	revs.date_mode = blame_date_mode;
-- 
2.15.0.128.gcadd42da22

[PATCHv3 0/4] blame: (dim rep. metadata lines or fields, decay date coloring)

[Stefan Beller]

v3:

Thanks Eric for feedback, I implemented all of the suggestions.
Specifically I dropped the abstractions in patch2 but keep around a similar
abstraction in patch 3 as that still looks like it benefits (the condition
is just growing large).

Thanks,
Stefan

v2:
This is picking up [1], but presenting it in another approach,
as I realized these are orthogonal features:
* dimming repeated lines/fields of information
* giving a quick visual information how old (as a proxy for 'well tested')
  a line of code is.

Both features are configurable.

Changes from sending it out in November:
* better commit messages
* rebased on master

Any feedback welcome.

Thanks,
Stefan

[1] https://public-inbox.org/git/20171110011002.10179-1-sbeller@google.com/

Stefan Beller (4):
  color.h: document and modernize header
  builtin/blame: dim uninteresting metadata
  builtin/blame: add option to color metadata fields separately
  builtin/blame: highlight recently changed lines

 Documentation/config.txt |  23 ++++++
 builtin/blame.c          | 201 ++++++++++++++++++++++++++++++++++++++++++-----
 color.c                  |   2 -
 color.h                  |  59 ++++++++++----
 t/t8012-blame-colors.sh  |  56 +++++++++++++
 5 files changed, 305 insertions(+), 36 deletions(-)
 create mode 100755 t/t8012-blame-colors.sh

-- 
2.16.0.rc0.223.g4a4ac83678-goog

[PATCHv3 1/4] color.h: document and modernize header

[Stefan Beller]

Add documentation explaining the functions in color.h.
While at it, mark them extern.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 color.c |  2 --
 color.h | 58 ++++++++++++++++++++++++++++++++++++++++++++--------------
 2 files changed, 44 insertions(+), 16 deletions(-)

diff --git a/color.c b/color.c
index d48dd947c9..c4dc1cb989 100644
--- a/color.c
+++ b/color.c
@@ -399,8 +399,6 @@ static int color_vfprintf(FILE *fp, const char *color, const char *fmt,
 	return r;
 }
 
-
-
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...)
 {
 	va_list args;
diff --git a/color.h b/color.h
index fd2b688dfb..2e768a10c6 100644
--- a/color.h
+++ b/color.h
@@ -72,26 +72,56 @@ extern int color_stdout_is_tty;
  * Use the first one if you need only color config; the second is a convenience
  * if you are just going to change to git_default_config, too.
  */
-int git_color_config(const char *var, const char *value, void *cb);
-int git_color_default_config(const char *var, const char *value, void *cb);
+extern int git_color_config(const char *var, const char *value, void *cb);
+extern int git_color_default_config(const char *var, const char *value, void *cb);
 
 /*
- * Set the color buffer (which must be COLOR_MAXLEN bytes)
- * to the raw color bytes; this is useful for initializing
- * default color variables.
+ * NEEDSWWORK: document this function or refactor grep.c to stop using this
+ * function.
  */
-void color_set(char *dst, const char *color_bytes);
+extern void color_set(char *dst, const char *color_bytes);
 
-int git_config_colorbool(const char *var, const char *value);
-int want_color(int var);
-int color_parse(const char *value, char *dst);
-int color_parse_mem(const char *value, int len, char *dst);
+/*
+ * Parse a config option, which can be a boolean or one of
+ * "never", "auto", "always". Return a constant of
+ * GIT_COLOR_NEVER for "never" or negative boolean,
+ * GIT_COLOR_ALWAYS for "always" or a positive boolean,
+ * and GIT_COLOR_AUTO for "auto".
+ */
+extern int git_config_colorbool(const char *var, const char *value);
+
+/*
+ * Resolve the constants as returned by git_config_colorbool()
+ * (specifically "auto") to a boolean answer.
+ */
+extern int want_color(int var);
+
+/*
+ * Translate a Git color from 'value' into a string that the terminal can
+ * interpret and store it into 'dst'. The Git color values are of the form
+ * "foreground [background] [attr]" where fore- and background can be a color
+ * name ("red"), a RGB code (#0xFF0000) or a 256-color-mode from the terminal.
+ */
+extern int color_parse(const char *value, char *dst);
+extern int color_parse_mem(const char *value, int len, char *dst);
+
+/*
+ * Output the formatted string in the specified color (and then reset to normal
+ * color so subsequent output is uncolored). Omits the color encapsulation if
+ * `color` is NULL. The `color_fprintf_ln` prints a new line after resetting
+ * the color. The `color_print_strbuf` prints the given pre-formatted strbuf
+ * instead, up to its first NUL character.
+ */
 __attribute__((format (printf, 3, 4)))
-int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
+extern int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
-int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
-void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
+extern int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
+extern void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
 
-int color_is_nil(const char *color);
+/*
+ * Check if the given color is GIT_COLOR_NIL that means "no color selected".
+ * The caller needs to replace the color with the actual desired color.
+ */
+extern int color_is_nil(const char *color);
 
 #endif /* COLOR_H */
-- 
2.16.0.rc0.223.g4a4ac83678-goog

Re: [PATCHv3 1/4] color.h: document and modernize header

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>  /*
> - * Set the color buffer (which must be COLOR_MAXLEN bytes)
> - * to the raw color bytes; this is useful for initializing
> - * default color variables.
> + * NEEDSWWORK: document this function or refactor grep.c to stop using this
> + * function.
>   */
> -void color_set(char *dst, const char *color_bytes);
> +extern void color_set(char *dst, const char *color_bytes);

The original that is removed by the patch documents the function
well enough; as long as the NEEDSWORK comment is followed through
in a later step in the series, it's alright, though ;-)

> -int git_config_colorbool(const char *var, const char *value);
> -int want_color(int var);
> -int color_parse(const char *value, char *dst);
> -int color_parse_mem(const char *value, int len, char *dst);
> +/*
> + * Parse a config option, which can be a boolean or one of
> + * "never", "auto", "always". Return a constant of
> + * GIT_COLOR_NEVER for "never" or negative boolean,
> + * GIT_COLOR_ALWAYS for "always" or a positive boolean,
> + * and GIT_COLOR_AUTO for "auto".
> + */
> +extern int git_config_colorbool(const char *var, const char *value);

"never" and "always" not being part of usual boolean vocabulary
makes it a bit awkward to explain.

> +/*
> + * Output the formatted string in the specified color (and then reset to normal
> + * color so subsequent output is uncolored). Omits the color encapsulation if
> + * `color` is NULL. The `color_fprintf_ln` prints a new line after resetting
> + * the color. The `color_print_strbuf` prints the given pre-formatted strbuf
> + * instead, up to its first NUL character.
> + */

Obviously, it does not have to be part of this step nor series, but
the above observation makes us realize that color_print_strbuf()
would probably be an unreasonably narrow interface.  It is not too
much to ask the caller to dereference and pass only the .buf
component of the strbuf to an alternative helper that takes "const
char *" and by doing so would allow us to let other callers that do
not have a strbuf but just a plain string use it, too.

Looks good.

[PATCHv3 2/4] builtin/blame: dim uninteresting metadata

[Stefan Beller]

When using git-blame lots of lines contain redundant information, for
example in hunks that consist of multiple lines, the metadata (commit
name, author, date) are repeated. A reader may not be interested in those,
so offer an option to color the information that is repeated from the
previous line differently.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/config.txt |  5 +++++
 builtin/blame.c          | 50 ++++++++++++++++++++++++++++++++++--------------
 color.h                  |  1 +
 t/t8012-blame-colors.sh  | 18 +++++++++++++++++
 4 files changed, 60 insertions(+), 14 deletions(-)
 create mode 100755 t/t8012-blame-colors.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 64c1dbba94..45749a574d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1216,6 +1216,11 @@ color.status.<slot>::
 	status short-format), or
 	`unmerged` (files which have unmerged changes).
 
+color.blame.repeatedMeta::
+	Use the customized color for the part of git-blame output that
+	is repeated meta information per line (such as commit id,
+	author name, date and timezone). Defaults to dark gray.
+
 color.ui::
 	This variable determines the default value for variables such
 	as `color.diff` and `color.grep` that control the use of color
diff --git a/builtin/blame.c b/builtin/blame.c
index 005f55aaa2..7b9c6e8676 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -7,6 +7,7 @@
 
 #include "cache.h"
 #include "config.h"
+#include "color.h"
 #include "builtin.h"
 #include "commit.h"
 #include "diff.h"
@@ -46,6 +47,7 @@ static int xdl_opts;
 static int abbrev = -1;
 static int no_whole_file_rename;
 static int show_progress;
+static char *repeated_meta_color;
 
 static struct date_mode blame_date_mode = { DATE_ISO8601 };
 static size_t blame_date_width;
@@ -316,10 +318,11 @@ static const char *format_time(timestamp_t time, const char *tz_str,
 #define OUTPUT_PORCELAIN	010
 #define OUTPUT_SHOW_NAME	020
 #define OUTPUT_SHOW_NUMBER	040
-#define OUTPUT_SHOW_SCORE      0100
-#define OUTPUT_NO_AUTHOR       0200
+#define OUTPUT_SHOW_SCORE	0100
+#define OUTPUT_NO_AUTHOR	0200
 #define OUTPUT_SHOW_EMAIL	0400
-#define OUTPUT_LINE_PORCELAIN 01000
+#define OUTPUT_LINE_PORCELAIN 	01000
+#define OUTPUT_COLOR_LINE	02000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -383,6 +386,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 	for (cnt = 0; cnt < ent->num_lines; cnt++) {
 		char ch;
 		int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : abbrev;
+		const char *color = "", *reset = "";
 
 		if (suspect->commit->object.flags & UNINTERESTING) {
 			if (blank_boundary)
@@ -393,7 +397,12 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 			}
 		}
 
-		printf("%.*s", length, hex);
+		if ((opt & OUTPUT_COLOR_LINE) && cnt > 0) {
+			color = repeated_meta_color;
+			reset = GIT_COLOR_RESET;
+		}
+
+		printf("%s%.*s%s", color, length, hex, reset);
 		if (opt & OUTPUT_ANNOTATE_COMPAT) {
 			const char *name;
 			if (opt & OUTPUT_SHOW_EMAIL)
@@ -406,16 +415,17 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 			       ent->lno + 1 + cnt);
 		} else {
 			if (opt & OUTPUT_SHOW_SCORE)
-				printf(" %*d %02d",
+				printf(" %s%*d %02d%s", color,
 				       max_score_digits, ent->score,
-				       ent->suspect->refcnt);
+				       ent->suspect->refcnt, reset);
 			if (opt & OUTPUT_SHOW_NAME)
-				printf(" %-*.*s", longest_file, longest_file,
-				       suspect->path);
+				printf(" %s%-*.*s%s", color, longest_file,
+						      longest_file,
+						      suspect->path,
+						      reset);
 			if (opt & OUTPUT_SHOW_NUMBER)
-				printf(" %*d", max_orig_digits,
-				       ent->s_lno + 1 + cnt);
-
+				printf(" %s%*d%s", color, max_orig_digits,
+				       ent->s_lno + 1 + cnt, reset);
 			if (!(opt & OUTPUT_NO_AUTHOR)) {
 				const char *name;
 				int pad;
@@ -424,11 +434,12 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 				else
 					name = ci.author.buf;
 				pad = longest_author - utf8_strwidth(name);
-				printf(" (%s%*s %10s",
+				printf(" %s(%s%*s %10s%s", color,
 				       name, pad, "",
 				       format_time(ci.author_time,
 						   ci.author_tz.buf,
-						   show_raw_time));
+						   show_raw_time),
+				       reset);
 			}
 			printf(" %*d) ",
 			       max_digits, ent->lno + 1 + cnt);
@@ -607,6 +618,12 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 		parse_date_format(value, &blame_date_mode);
 		return 0;
 	}
+	if (!strcmp(var, "color.blame.repeatedmeta")) {
+		if (color_parse_mem(value, strlen(value), repeated_meta_color))
+			warning(_("invalid color '%s' in color.blame.repeatedMeta"),
+				value);
+		return 0;
+	}
 
 	if (git_diff_heuristic_config(var, value, cb) < 0)
 		return -1;
@@ -681,6 +698,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_BIT('s', NULL, &output_option, N_("Suppress author name and timestamp (Default: off)"), OUTPUT_NO_AUTHOR),
 		OPT_BIT('e', "show-email", &output_option, N_("Show author email instead of name (Default: off)"), OUTPUT_SHOW_EMAIL),
 		OPT_BIT('w', NULL, &xdl_opts, N_("Ignore whitespace differences"), XDF_IGNORE_WHITESPACE),
+		OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line differently"), OUTPUT_COLOR_LINE),
 
 		/*
 		 * The following two options are parsed by parse_revision_opt()
@@ -940,8 +958,12 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 
 	blame_coalesce(&sb);
 
-	if (!(output_option & OUTPUT_PORCELAIN))
+	if (!(output_option & OUTPUT_PORCELAIN)) {
 		find_alignment(&sb, &output_option);
+		if ((output_option & OUTPUT_COLOR_LINE) &&
+		    !repeated_meta_color)
+			repeated_meta_color = GIT_COLOR_DARK;
+	}
 
 	output(&sb, output_option);
 	free((void *)sb.final_buf);
diff --git a/color.h b/color.h
index 2e768a10c6..2df2f86698 100644
--- a/color.h
+++ b/color.h
@@ -30,6 +30,7 @@ struct strbuf;
 #define GIT_COLOR_BLUE		"\033[34m"
 #define GIT_COLOR_MAGENTA	"\033[35m"
 #define GIT_COLOR_CYAN		"\033[36m"
+#define GIT_COLOR_DARK		"\033[1;30m"
 #define GIT_COLOR_BOLD_RED	"\033[1;31m"
 #define GIT_COLOR_BOLD_GREEN	"\033[1;32m"
 #define GIT_COLOR_BOLD_YELLOW	"\033[1;33m"
diff --git a/t/t8012-blame-colors.sh b/t/t8012-blame-colors.sh
new file mode 100755
index 0000000000..a2b7090cef
--- /dev/null
+++ b/t/t8012-blame-colors.sh
@@ -0,0 +1,18 @@
+#!/bin/sh
+
+test_description='colored git blame'
+. ./test-lib.sh
+
+PROG='git blame -c'
+. "$TEST_DIRECTORY"/annotate-tests.sh
+
+test_expect_success 'colored blame colors contiguous lines' '
+	git blame --abbrev=12 --color-lines hello.c >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	grep "<BOLD;BLACK>(F" actual > F.expect &&
+	grep "<BOLD;BLACK>(H" actual > H.expect &&
+	test_line_count = 2 F.expect &&
+	test_line_count = 3 H.expect
+'
+
+test_done
-- 
2.16.0.rc0.223.g4a4ac83678-goog

Re: [PATCHv3 2/4] builtin/blame: dim uninteresting metadata

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +color.blame.repeatedMeta::
> +	Use the customized color for the part of git-blame output that
> +	is repeated meta information per line (such as commit id,
> +	author name, date and timezone). Defaults to dark gray.
> +
> ...

"Dark gray on default background" may alleviate worrries from those
who prefer black ink on white paper display by hinting that both
foreground and background colors can be configured.

Do we want to make this overridable from the command line,
i.e. --color-repeated-meta=gray?


> +#define OUTPUT_COLOR_LINE	02000

The name of the macro implies that this is (or at least can be) a
lot more generic UI request than merely "paint the same metadata on
adjacent lines in a different color".

> +		OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line differently"), OUTPUT_COLOR_LINE),

Should this eventually become "--color=<yes,no,auto>" that is more
usual and generic, possibly defaulting to "auto" in the future, I
wonder?

> diff --git a/color.h b/color.h
> index 2e768a10c6..2df2f86698 100644
> --- a/color.h
> +++ b/color.h
> @@ -30,6 +30,7 @@ struct strbuf;
>  #define GIT_COLOR_BLUE		"\033[34m"
>  #define GIT_COLOR_MAGENTA	"\033[35m"
>  #define GIT_COLOR_CYAN		"\033[36m"
> +#define GIT_COLOR_DARK		"\033[1;30m"
>  #define GIT_COLOR_BOLD_RED	"\033[1;31m"
>  #define GIT_COLOR_BOLD_GREEN	"\033[1;32m"
>  #define GIT_COLOR_BOLD_YELLOW	"\033[1;33m"

How about using CYAN just like "diff" output uses it to paint the
least interesting lines?  That way we will keep the "uninteresting
is cyan" consistency for the default settings without adding a new
color to the palette.

Re: [PATCHv3 2/4] builtin/blame: dim uninteresting metadata

[Stefan Beller]

On Mon, Jan 8, 2018 at 11:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> +color.blame.repeatedMeta::
>> +     Use the customized color for the part of git-blame output that
>> +     is repeated meta information per line (such as commit id,
>> +     author name, date and timezone). Defaults to dark gray.
>> +
>> ...
>
> "Dark gray on default background" may alleviate worrries from those
> who prefer black ink on white paper display by hinting that both
> foreground and background colors can be configured.
>
> Do we want to make this overridable from the command line,
> i.e. --color-repeated-meta=gray?
>
>> +#define OUTPUT_COLOR_LINE    02000
>
> The name of the macro implies that this is (or at least can be) a
> lot more generic UI request than merely "paint the same metadata on
> adjacent lines in a different color".
>
>> +             OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line differently"), OUTPUT_COLOR_LINE),
>
> Should this eventually become "--color=<yes,no,auto>" that is more
> usual and generic, possibly defaulting to "auto" in the future, I
> wonder?

This sounds like a good change to make, though what does "yes"
mean here? The following patches introduce other modes of
colorization, such that --color=<no, auto, lines, fields, decayed>
would make sense to me, with "lines" as the mode of this patch,
"fields" implemented in the next patch and "decayed" having the
meaning of the heating algorithm presented in the last patch.

However each of these (except no and auto) would want to have
extra parameters, which as mentioned above could go into its own
separate parameters, such that

  git blame --color=line --color-repeated-meta=cyan

would seem like a good UI. This might look like it could be a good
idea to have --color-repeated-meta imply --color=<line,auto> if no
--color is given. But as different coloring modes will have different
arguments to provide extra color information, I wonder what we
would do with

  git blame --color="mode1" --extra-color-arg-for-mode-2

So I don't think the idea of having separate options would be a
good idea as we'd have to think about the implications as mentioned
above. So maybe

  git blame "--color=lines,cyan yellow"

would work, if you want "cyan yellow" as the color configuration in
the "lines" mode? (bad choice of a background color btw)

>> diff --git a/color.h b/color.h
>> index 2e768a10c6..2df2f86698 100644
>> --- a/color.h
>> +++ b/color.h
>> @@ -30,6 +30,7 @@ struct strbuf;
>>  #define GIT_COLOR_BLUE               "\033[34m"
>>  #define GIT_COLOR_MAGENTA    "\033[35m"
>>  #define GIT_COLOR_CYAN               "\033[36m"
>> +#define GIT_COLOR_DARK               "\033[1;30m"
>>  #define GIT_COLOR_BOLD_RED   "\033[1;31m"
>>  #define GIT_COLOR_BOLD_GREEN "\033[1;32m"
>>  #define GIT_COLOR_BOLD_YELLOW        "\033[1;33m"
>
> How about using CYAN just like "diff" output uses it to paint the
> least interesting lines?  That way we will keep the "uninteresting
> is cyan" consistency for the default settings without adding a new
> color to the palette.

sounds good, I'll take that suggestion once we reach consensus on
the UI for the user.

Thanks,
Stefan

[PATCHv3 3/4] builtin/blame: add option to color metadata fields separately

[Stefan Beller]

Unlike the previous commit, this dims colors for each
metadata field individually.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/blame.c         | 83 ++++++++++++++++++++++++++++++++++++++++++++-----
 t/t8012-blame-colors.sh | 38 ++++++++++++++++++++++
 2 files changed, 113 insertions(+), 8 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 7b9c6e8676..60c8dadf4b 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -323,6 +323,7 @@ static const char *format_time(timestamp_t time, const char *tz_str,
 #define OUTPUT_SHOW_EMAIL	0400
 #define OUTPUT_LINE_PORCELAIN 	01000
 #define OUTPUT_COLOR_LINE	02000
+#define OUTPUT_COLOR_FIELDS	04000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -370,7 +371,56 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
 		putchar('\n');
 }
 
-static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int opt)
+static int had_same_field_previously(int opt, int field,
+			  struct blame_entry *ent,
+			  struct blame_entry *prev)
+{
+	struct commit_info ci, prev_ci;
+
+	switch (field) {
+	case OUTPUT_SHOW_SCORE:
+		return ent->score == prev->score;
+	case OUTPUT_SHOW_NAME:
+		return prev->suspect &&
+			!strcmp(ent->suspect->path, prev->suspect->path);
+	case OUTPUT_SHOW_NUMBER:
+		return ent->s_lno == prev->s_lno + prev->num_lines - 1;
+
+	case OUTPUT_NO_AUTHOR:
+		get_commit_info(ent->suspect->commit, &ci, 1);
+		get_commit_info(prev->suspect->commit, &prev_ci, 1);
+		return ((opt & OUTPUT_SHOW_EMAIL) &&
+			!strcmp(ci.author_mail.buf, prev_ci.author_mail.buf)) ||
+			!strcmp(ci.author.buf, prev_ci.author.buf);
+	default:
+		BUG("unknown field");
+	}
+	return 0;
+}
+
+static void setup_field_color(int opt, int cnt, int field,
+			      struct blame_entry *ent,
+			      struct blame_entry *prev,
+			      const char **use_color,
+			      const char **reset_color)
+{
+	if (!(opt & OUTPUT_COLOR_FIELDS))
+		return;
+
+	if ((cnt > 0 ||
+	     (prev && had_same_field_previously(opt, field, ent, prev)))) {
+		*use_color = repeated_meta_color;
+		*reset_color = GIT_COLOR_RESET;
+	} else {
+		*use_color = "";
+		*reset_color = "";
+	}
+}
+
+static void emit_other(struct blame_scoreboard *sb,
+		       struct blame_entry *ent,
+		       struct blame_entry *prev,
+		       int opt)
 {
 	int cnt;
 	const char *cp;
@@ -414,18 +464,27 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 					   show_raw_time),
 			       ent->lno + 1 + cnt);
 		} else {
-			if (opt & OUTPUT_SHOW_SCORE)
+			if (opt & OUTPUT_SHOW_SCORE) {
+				setup_field_color(opt, cnt, OUTPUT_SHOW_SCORE,
+						  ent, prev, &color, &reset);
 				printf(" %s%*d %02d%s", color,
 				       max_score_digits, ent->score,
 				       ent->suspect->refcnt, reset);
-			if (opt & OUTPUT_SHOW_NAME)
+			}
+			if (opt & OUTPUT_SHOW_NAME) {
+				setup_field_color(opt, cnt, OUTPUT_SHOW_NAME,
+						   ent, prev, &color, &reset);
 				printf(" %s%-*.*s%s", color, longest_file,
 						      longest_file,
 						      suspect->path,
 						      reset);
-			if (opt & OUTPUT_SHOW_NUMBER)
+			}
+			if (opt & OUTPUT_SHOW_NUMBER) {
+				setup_field_color(opt, cnt, OUTPUT_SHOW_NUMBER,
+						  ent, prev, &color, &reset);
 				printf(" %s%*d%s", color, max_orig_digits,
 				       ent->s_lno + 1 + cnt, reset);
+			}
 			if (!(opt & OUTPUT_NO_AUTHOR)) {
 				const char *name;
 				int pad;
@@ -434,6 +493,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 				else
 					name = ci.author.buf;
 				pad = longest_author - utf8_strwidth(name);
+				setup_field_color(opt, cnt, OUTPUT_NO_AUTHOR,
+						  ent, prev, &color, &reset);
 				printf(" %s(%s%*s %10s%s", color,
 				       name, pad, "",
 				       format_time(ci.author_time,
@@ -459,7 +520,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 
 static void output(struct blame_scoreboard *sb, int option)
 {
-	struct blame_entry *ent;
+	struct blame_entry *ent, *prev = NULL;
 
 	if (option & OUTPUT_PORCELAIN) {
 		for (ent = sb->ent; ent; ent = ent->next) {
@@ -481,7 +542,8 @@ static void output(struct blame_scoreboard *sb, int option)
 		if (option & OUTPUT_PORCELAIN)
 			emit_porcelain(sb, ent, option);
 		else {
-			emit_other(sb, ent, option);
+			emit_other(sb, ent, prev, option);
+			prev = ent;
 		}
 	}
 }
@@ -699,6 +761,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_BIT('e', "show-email", &output_option, N_("Show author email instead of name (Default: off)"), OUTPUT_SHOW_EMAIL),
 		OPT_BIT('w', NULL, &xdl_opts, N_("Ignore whitespace differences"), XDF_IGNORE_WHITESPACE),
 		OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line differently"), OUTPUT_COLOR_LINE),
+		OPT_BIT(0, "color-fields", &output_option, N_("color redundant metadata fields from previous line differently"), OUTPUT_COLOR_FIELDS),
 
 		/*
 		 * The following two options are parsed by parse_revision_opt()
@@ -757,6 +820,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	revs.diffopt.flags.follow_renames = 0;
 	argc = parse_options_end(&ctx);
 
+	if ((output_option & OUTPUT_COLOR_LINE) &&
+	    (output_option & OUTPUT_COLOR_FIELDS))
+		die(_("cannot ask for colored lines and fields at the same time"));
+
 	if (incremental || (output_option & OUTPUT_PORCELAIN)) {
 		if (show_progress > 0)
 			die(_("--progress can't be used with --incremental or porcelain formats"));
@@ -960,8 +1027,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 
 	if (!(output_option & OUTPUT_PORCELAIN)) {
 		find_alignment(&sb, &output_option);
-		if ((output_option & OUTPUT_COLOR_LINE) &&
-		    !repeated_meta_color)
+		if (!repeated_meta_color &&
+		    (output_option & (OUTPUT_COLOR_LINE | OUTPUT_COLOR_FIELDS)))
 			repeated_meta_color = GIT_COLOR_DARK;
 	}
 
diff --git a/t/t8012-blame-colors.sh b/t/t8012-blame-colors.sh
index a2b7090cef..18f9c9a16d 100755
--- a/t/t8012-blame-colors.sh
+++ b/t/t8012-blame-colors.sh
@@ -15,4 +15,42 @@ test_expect_success 'colored blame colors contiguous lines' '
 	test_line_count = 3 H.expect
 '
 
+test_expect_success 'colored blame colors continuous fields' '
+
+	git mv hello.c world.c &&
+	git commit -a -m "moved file" &&
+	cat <<-EOF >> world.c &&
+	void world()
+	{
+		puts("world");
+	}
+	EOF
+	git add world.c &&
+	GIT_AUTHOR_NAME="F" GIT_AUTHOR_EMAIL="F@test.git" \
+		git commit -m "forgot to add changes to moved file" &&
+
+	git blame --abbrev=12 --color-fields world.c >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+
+	grep "<BOLD;BLACK>hello.c" actual > colored_hello.expect &&
+	grep "hello.c" actual > all_hello.expect &&
+	test_line_count = 9 colored_hello.expect &&
+	test_line_count = 10 all_hello.expect &&
+
+	grep "<BOLD;BLACK>world.c" actual > colored_world.expect &&
+	grep "world.c" actual > all_world.expect &&
+	test_line_count = 3 colored_world.expect &&
+	test_line_count = 4 all_world.expect &&
+
+	grep "(F" actual > all_F.expect &&
+	grep "<BOLD;BLACK>(F" actual > colored_F.expect &&
+	test_line_count = 8 all_F.expect &&
+	test_line_count = 5 colored_F.expect &&
+
+	grep "(H" actual > all_H.expect &&
+	grep "<BOLD;BLACK>(H" actual > colored_H.expect &&
+	test_line_count = 5 all_H.expect &&
+	test_line_count = 3 colored_H.expect
+'
+
 test_done
-- 
2.16.0.rc0.223.g4a4ac83678-goog

[PATCHv3 4/4] builtin/blame: highlight recently changed lines

[Stefan Beller]

Choose a different color for dates and imitate a 'temperature cool down'
depending upon age.

Originally I had planned to have the temperature cooldown dependent on
the age of the project or file for example, as that might scale better,
but that can be added on top of this commit, e.g. instead of giving a
date, you could imagine giving a percentage that would be the linearly
interpolated between now and the beginning of the file.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/config.txt | 18 ++++++++++++
 builtin/blame.c          | 74 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 91 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 45749a574d..aa967ce3ed 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1221,6 +1221,24 @@ color.blame.repeatedMeta::
 	is repeated meta information per line (such as commit id,
 	author name, date and timezone). Defaults to dark gray.
 
+color.blame.highlightRecent::
+	This can be used to color the author and date of a blame line.
+	This overrides `color.blame.repeatedMeta` setting, which colors
+	repetitions.
++
+This setting should be set to a comma-separated list of color and date settings,
+starting and ending with a color, the dates should be set from oldest to newest.
+The metadata will be colored given the colors if the the line was introduced
+before the given timestamp, overwriting older timestamped colors.
++
+Instead of an absolute timestamp relative timestamps work as well, e.g.
+2.weeks.ago is valid to address anything older than 2 weeks.
++
+It defaults to "blue,12 month ago,white,1 month ago,red", which colors
+everything older than one year blue, recent changes between one month and
+one year old are kept white, and lines introduced within the last month are
+colored red.
+
 color.ui::
 	This variable determines the default value for variables such
 	as `color.diff` and `color.grep` that control the use of color
diff --git a/builtin/blame.c b/builtin/blame.c
index 60c8dadf4b..3d444e66b3 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -24,6 +24,7 @@
 #include "dir.h"
 #include "progress.h"
 #include "blame.h"
+#include "string-list.h"
 
 static char blame_usage[] = N_("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>");
 
@@ -324,6 +325,7 @@ static const char *format_time(timestamp_t time, const char *tz_str,
 #define OUTPUT_LINE_PORCELAIN 	01000
 #define OUTPUT_COLOR_LINE	02000
 #define OUTPUT_COLOR_FIELDS	04000
+#define OUTPUT_HEATED_LINES	010000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -417,6 +419,61 @@ static void setup_field_color(int opt, int cnt, int field,
 	}
 }
 
+static struct color_field {
+	timestamp_t hop;
+	char col[COLOR_MAXLEN];
+} *colorfield;
+static int colorfield_nr, colorfield_alloc;
+
+static void parse_color_fields(const char *s)
+{
+	struct string_list l = STRING_LIST_INIT_DUP;
+	struct string_list_item *item;
+	enum { EXPECT_DATE, EXPECT_COLOR } next = EXPECT_COLOR;
+
+	/* Ideally this would be stripped and split at the same time? */
+	string_list_split(&l, s, ',', -1);
+	ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
+
+	for_each_string_list_item(item, &l) {
+		switch (next) {
+		case EXPECT_DATE:
+			colorfield[colorfield_nr].hop = approxidate(item->string);
+			next = EXPECT_COLOR;
+			colorfield_nr++;
+			ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
+			break;
+		case EXPECT_COLOR:
+			if (color_parse(item->string, colorfield[colorfield_nr].col))
+				die(_("expecting a color: %s"), item->string);
+			next = EXPECT_DATE;
+			break;
+		}
+	}
+
+	if (next == EXPECT_COLOR)
+		die (_("must end with a color"));
+
+	colorfield[colorfield_nr].hop = TIME_MAX;
+}
+
+static void setup_default_colorfield(void)
+{
+	parse_color_fields("blue,12 month ago,white,1 month ago,red");
+}
+
+static void determine_line_heat(struct blame_entry *ent, const char **dest_color)
+{
+	int i = 0;
+	struct commit_info ci;
+	get_commit_info(ent->suspect->commit, &ci, 1);
+
+	while (i < colorfield_nr && ci.author_time > colorfield[i].hop)
+		i++;
+
+	*dest_color = colorfield[i].col;
+}
+
 static void emit_other(struct blame_scoreboard *sb,
 		       struct blame_entry *ent,
 		       struct blame_entry *prev,
@@ -424,6 +481,7 @@ static void emit_other(struct blame_scoreboard *sb,
 {
 	int cnt;
 	const char *cp;
+	const char *heatcolor = NULL;
 	struct blame_origin *suspect = ent->suspect;
 	struct commit_info ci;
 	char hex[GIT_MAX_HEXSZ + 1];
@@ -433,6 +491,10 @@ static void emit_other(struct blame_scoreboard *sb,
 	oid_to_hex_r(hex, &suspect->commit->object.oid);
 
 	cp = blame_nth_line(sb, ent->lno);
+
+	if (opt & OUTPUT_HEATED_LINES)
+		determine_line_heat(ent, &heatcolor);
+
 	for (cnt = 0; cnt < ent->num_lines; cnt++) {
 		char ch;
 		int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : abbrev;
@@ -488,6 +550,7 @@ static void emit_other(struct blame_scoreboard *sb,
 			if (!(opt & OUTPUT_NO_AUTHOR)) {
 				const char *name;
 				int pad;
+				int hcolor = opt & OUTPUT_HEATED_LINES;
 				if (opt & OUTPUT_SHOW_EMAIL)
 					name = ci.author_mail.buf;
 				else
@@ -495,7 +558,10 @@ static void emit_other(struct blame_scoreboard *sb,
 				pad = longest_author - utf8_strwidth(name);
 				setup_field_color(opt, cnt, OUTPUT_NO_AUTHOR,
 						  ent, prev, &color, &reset);
-				printf(" %s(%s%*s %10s%s", color,
+				if (hcolor)
+					reset = GIT_COLOR_RESET;
+				printf(" %s(%s%*s %10s%s",
+				       hcolor ? heatcolor : color,
 				       name, pad, "",
 				       format_time(ci.author_time,
 						   ci.author_tz.buf,
@@ -686,6 +752,10 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 				value);
 		return 0;
 	}
+	if (!strcmp(var, "color.blame.highlightrecent")) {
+		parse_color_fields(value);
+		return 0;
+	}
 
 	if (git_diff_heuristic_config(var, value, cb) < 0)
 		return -1;
@@ -762,6 +832,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_BIT('w', NULL, &xdl_opts, N_("Ignore whitespace differences"), XDF_IGNORE_WHITESPACE),
 		OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line differently"), OUTPUT_COLOR_LINE),
 		OPT_BIT(0, "color-fields", &output_option, N_("color redundant metadata fields from previous line differently"), OUTPUT_COLOR_FIELDS),
+		OPT_BIT(0, "heated-lines", &output_option, N_("color lines by age"), OUTPUT_HEATED_LINES),
 
 		/*
 		 * The following two options are parsed by parse_revision_opt()
@@ -786,6 +857,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	unsigned int range_i;
 	long anchor;
 
+	setup_default_colorfield();
 	git_config(git_blame_config, &output_option);
 	init_revisions(&revs, NULL);
 	revs.date_mode = blame_date_mode;
-- 
2.16.0.rc0.223.g4a4ac83678-goog

Re: [PATCHv3 4/4] builtin/blame: highlight recently changed lines

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> +static struct color_field {
> +	timestamp_t hop;
> +	char col[COLOR_MAXLEN];
> +} *colorfield;
> +static int colorfield_nr, colorfield_alloc;
> +
> +static void parse_color_fields(const char *s)
> +{
> +	struct string_list l = STRING_LIST_INIT_DUP;
> +	struct string_list_item *item;
> +	enum { EXPECT_DATE, EXPECT_COLOR } next = EXPECT_COLOR;
> +
> +	/* Ideally this would be stripped and split at the same time? */
> +	string_list_split(&l, s, ',', -1);
> +	ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
> +
> +	for_each_string_list_item(item, &l) {
> +		switch (next) {
> +		case EXPECT_DATE:
> +			colorfield[colorfield_nr].hop = approxidate(item->string);
> +			next = EXPECT_COLOR;
> +			colorfield_nr++;
> +			ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
> +			break;

This should make sure cf[i].hop is monotonically increasing to avoid
end-user mistakes, I would think (what's 'hop' by the way?).

> +		case EXPECT_COLOR:
> +			if (color_parse(item->string, colorfield[colorfield_nr].col))
> +				die(_("expecting a color: %s"), item->string);

When you have a typo in one of your configuration files, say "[color
"blame"] highlightrecent = 1,week,blue,...", you'd want to see a bit
more than just "expecting a color: week" to help you diagnose and
resolve the issue.  Giving the name of the variable and the file the
wrong definition was found in would be needed, givin that this is
called from the config callback git_blame_config() below.

> +			next = EXPECT_DATE;
> +			break;
> +		}
> +	}
> +
> +	if (next == EXPECT_COLOR)
> +		die (_("must end with a color"));

Same here.

>  		OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line differently"), OUTPUT_COLOR_LINE),
>  		OPT_BIT(0, "color-fields", &output_option, N_("color redundant metadata fields from previous line differently"), OUTPUT_COLOR_FIELDS),
> +		OPT_BIT(0, "heated-lines", &output_option, N_("color lines by age"), OUTPUT_HEATED_LINES),

These options may be useful while having fun experimenting, but my
gut feeling is that these are too fine-grained for end-users to
tweak per invocation basis (which is what command line options are
for).  

But perhaps I am biased (as anybody else), as personally I find
anything beyond step 2/4 uninteresting, and not adding too many of
these options is consistent with that viewpoint ;-)

In any case, thanks for a fun read.

Re: [PATCHv3 4/4] builtin/blame: highlight recently changed lines

[Stefan Beller]

On Mon, Jan 8, 2018 at 11:56 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> +static struct color_field {
>> +     timestamp_t hop;
>> +     char col[COLOR_MAXLEN];
>> +} *colorfield;
>> +static int colorfield_nr, colorfield_alloc;
>> +
>> +static void parse_color_fields(const char *s)
>> +{
>> +     struct string_list l = STRING_LIST_INIT_DUP;
>> +     struct string_list_item *item;
>> +     enum { EXPECT_DATE, EXPECT_COLOR } next = EXPECT_COLOR;
>> +
>> +     /* Ideally this would be stripped and split at the same time? */
>> +     string_list_split(&l, s, ',', -1);
>> +     ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
>> +
>> +     for_each_string_list_item(item, &l) {
>> +             switch (next) {
>> +             case EXPECT_DATE:
>> +                     colorfield[colorfield_nr].hop = approxidate(item->string);
>> +                     next = EXPECT_COLOR;
>> +                     colorfield_nr++;
>> +                     ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
>> +                     break;
>
> This should make sure cf[i].hop is monotonically increasing to avoid
> end-user mistakes, I would think (what's 'hop' by the way?).
>
>> +             case EXPECT_COLOR:
>> +                     if (color_parse(item->string, colorfield[colorfield_nr].col))
>> +                             die(_("expecting a color: %s"), item->string);
>
> When you have a typo in one of your configuration files, say "[color
> "blame"] highlightrecent = 1,week,blue,...", you'd want to see a bit
> more than just "expecting a color: week" to help you diagnose and
> resolve the issue.  Giving the name of the variable and the file the
> wrong definition was found in would be needed, givin that this is
> called from the config callback git_blame_config() below.
>
>> +                     next = EXPECT_DATE;
>> +                     break;
>> +             }
>> +     }
>> +
>> +     if (next == EXPECT_COLOR)
>> +             die (_("must end with a color"));
>
> Same here.
>
>>               OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line differently"), OUTPUT_COLOR_LINE),
>>               OPT_BIT(0, "color-fields", &output_option, N_("color redundant metadata fields from previous line differently"), OUTPUT_COLOR_FIELDS),
>> +             OPT_BIT(0, "heated-lines", &output_option, N_("color lines by age"), OUTPUT_HEATED_LINES),
>
> These options may be useful while having fun experimenting, but my
> gut feeling is that these are too fine-grained for end-users to
> tweak per invocation basis (which is what command line options are
> for).
>
> But perhaps I am biased (as anybody else), as personally I find
> anything beyond step 2/4 uninteresting, and not adding too many of
> these options is consistent with that viewpoint ;-)

See, I find 2 and 3 uninteresting and just did it 'because someone else
hinted at that is what they want'. Maybe I was a bad listener.

4 (maybe with 2 in combination) would be all I need as that allows me
to quickly judge the trustworthiness of code (old code is better,
just like most liquors? ;)

> In any case, thanks for a fun read.

Thanks, I'll reread the comments and see if I can remove some
options to make it useful for upstream consumption.

Thanks,
Stefan

Re: [PATCHv3 0/4] blame: (dim rep. metadata lines or fields, decay date coloring)

[Ævar Arnfjörð Bjarmason]

On Thu, Jan 04 2018, Stefan Beller jotted:

> Stefan Beller (4):
>   color.h: document and modernize header
>   builtin/blame: dim uninteresting metadata
>   builtin/blame: add option to color metadata fields separately
>   builtin/blame: highlight recently changed lines

I like this feature in principle, but I can't get it to work. Building
pu and:

    ./git -c color.ui=always --exec-path=$PWD blame Makefile

Shows no colors. Neither does:

    ./git -c color.ui=always --exec-path=$PWD -c color.blame.highlightRecent="red,12 month ago,blue" blame Makefile

And there's a bug, it segfaults on any custom value to the other config
option:

    ./git -c color.ui=always --exec-path=$PWD -c color.blame.repeatedMeta=red blame Makefile

    0x00000000004c312b in color_parse_mem (value=0x8f6520 "red", value_len=3, dst=0x1 <Address 0x1 out of bounds>) at color.c:272
    272                     OUT('\033');

The repeated_meta_color variable is NULL when passed to
color_parse_mem(). Didn't dig further.

Re: [PATCHv3 0/4] blame: (dim rep. metadata lines or fields, decay date coloring)

[Stefan Beller]

On Thu, Feb 1, 2018 at 11:29 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Thu, Jan 04 2018, Stefan Beller jotted:
>
>> Stefan Beller (4):
>>   color.h: document and modernize header
>>   builtin/blame: dim uninteresting metadata
>>   builtin/blame: add option to color metadata fields separately
>>   builtin/blame: highlight recently changed lines
>
> I like this feature in principle, but I can't get it to work. Building
> pu and:

Thanks for testing the feature!
Please use the flag `--color-lines`, `--color-fields` or `--heated-lines`
for experimentation.

In the future we may decide that one of them becomes the default
(which one?) and is triggered by the color.ui=always setting as well.

>     ./git -c color.ui=always --exec-path=$PWD blame Makefile
>
> Shows no colors. Neither does:
>
>     ./git -c color.ui=always --exec-path=$PWD -c color.blame.highlightRecent="red,12 month ago,blue" blame Makefile
>
> And there's a bug, it segfaults on any custom value to the other config
> option:
>
>     ./git -c color.ui=always --exec-path=$PWD -c color.blame.repeatedMeta=red blame Makefile
>
>     0x00000000004c312b in color_parse_mem (value=0x8f6520 "red", value_len=3, dst=0x1 <Address 0x1 out of bounds>) at color.c:272
>     272                     OUT('\033');
>
> The repeated_meta_color variable is NULL when passed to
> color_parse_mem(). Didn't dig further.

Thanks for noticing.
Fix below (white space mangled)

--- i/builtin/blame.c
+++ w/builtin/blame.c
@@ -48,7 +48,7 @@ static int xdl_opts;
 static int abbrev = -1;
 static int no_whole_file_rename;
 static int show_progress;
-static char *repeated_meta_color;
+static char repeated_meta_color[COLOR_MAXLEN];

 static struct date_mode blame_date_mode = { DATE_ISO8601 };
 static size_t blame_date_width;
@@ -1099,9 +1099,9 @@ int cmd_blame(int argc, const char **argv, const
char *prefix)

        if (!(output_option & OUTPUT_PORCELAIN)) {
                find_alignment(&sb, &output_option);
-               if (!repeated_meta_color &&
+               if (!*repeated_meta_color &&
                    (output_option & (OUTPUT_COLOR_LINE | OUTPUT_COLOR_FIELDS)))
-                       repeated_meta_color = GIT_COLOR_DARK;
+                       strcpy(repeated_meta_color, GIT_COLOR_DARK);
        }

[PATCHv2 0/4] blame: (dim rep. metadata lines or fields, decay date coloring)

[Stefan Beller]

This is picking up [1], but presenting it in another approach,
as I realized these are orthogonal features:
* dimming repeated lines/fields of information
* giving a quick visual information how old (as a proxy for 'well tested')
  a line of code is.
  
Both features are configurable.

Changes from sending it out in November:
* better commit messages
* rebased on master

Any feedback welcome.

Thanks,
Stefan

[1] https://public-inbox.org/git/20171110011002.10179-1-sbeller@google.com/

Stefan Beller (4):
  color.h: document and modernize header
  builtin/blame: dim uninteresting metadata
  builtin/blame: add option to color metadata fields separately
  builtin/blame: highlight recently changed lines

 Documentation/config.txt |  23 +++++
 builtin/blame.c          | 216 ++++++++++++++++++++++++++++++++++++++++++-----
 color.c                  |   2 -
 color.h                  |  49 ++++++++---
 t/t8012-blame-colors.sh  |  56 ++++++++++++
 5 files changed, 311 insertions(+), 35 deletions(-)
 create mode 100755 t/t8012-blame-colors.sh

-- 
2.15.1.620.gb9897f4670-goog

[PATCH 3/4] builtin/blame: add option to color metadata fields separately

[Stefan Beller]

Unlike the previous commit, this dims colors for each
metadata field individually.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/blame.c         | 82 +++++++++++++++++++++++++++++++++++++++++++------
 t/t8012-blame-colors.sh | 38 +++++++++++++++++++++++
 2 files changed, 111 insertions(+), 9 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 09afb4d3ea..39e8c0b222 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -323,6 +323,7 @@ static const char *format_time(timestamp_t time, const char *tz_str,
 #define OUTPUT_SHOW_EMAIL	0400
 #define OUTPUT_LINE_PORCELAIN 	01000
 #define OUTPUT_COLOR_LINE	02000
+#define OUTPUT_COLOR_FIELDS	04000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -370,6 +371,33 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
 		putchar('\n');
 }
 
+static int had_same_field_previously(int opt, int field,
+			  struct blame_entry *ent,
+			  struct blame_entry *prev)
+{
+	struct commit_info ci, prev_ci;
+
+	switch (field) {
+	case OUTPUT_SHOW_SCORE:
+		return ent->score == prev->score;
+	case OUTPUT_SHOW_NAME:
+		return prev->suspect &&
+			!strcmp(ent->suspect->path, prev->suspect->path);
+	case OUTPUT_SHOW_NUMBER:
+		return ent->s_lno == prev->s_lno + prev->num_lines - 1;
+
+	case OUTPUT_NO_AUTHOR:
+		get_commit_info(ent->suspect->commit, &ci, 1);
+		get_commit_info(prev->suspect->commit, &prev_ci, 1);
+		return ((opt & OUTPUT_SHOW_EMAIL) &&
+			!strcmp(ci.author_mail.buf, prev_ci.author_mail.buf)) ||
+			!strcmp(ci.author.buf, prev_ci.author.buf);
+	default:
+		BUG("unknown field");
+	}
+	return 0;
+}
+
 static inline void colors_unset(const char **use_color, const char **reset_color)
 {
 	*use_color = "";
@@ -382,17 +410,36 @@ static inline void colors_set(const char **use_color, const char **reset_color)
 	*reset_color = GIT_COLOR_RESET;
 }
 
+static void setup_field_color(int opt, int cnt, int field,
+			      struct blame_entry *ent,
+			      struct blame_entry *prev,
+			      const char **use_color,
+			      const char **reset_color)
+{
+	if (!(opt & OUTPUT_COLOR_FIELDS))
+		return;
+
+	if ((cnt > 0 ||
+	     (prev && had_same_field_previously(opt, field, ent, prev))))
+		colors_set(use_color, reset_color);
+	else
+		colors_unset(use_color, reset_color);
+}
+
 static void setup_line_color(int opt, int cnt,
 			     const char **use_color,
 			     const char **reset_color)
 {
 	colors_unset(use_color, reset_color);
 
-	if ((opt & OUTPUT_COLOR_LINE) && cnt > 0)
+	if ((opt & (OUTPUT_COLOR_LINE | OUTPUT_COLOR_FIELDS)) && cnt > 0)
 		colors_set(use_color, reset_color);
 }
 
-static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int opt)
+static void emit_other(struct blame_scoreboard *sb,
+		       struct blame_entry *ent,
+		       struct blame_entry *prev,
+		       int opt)
 {
 	int cnt;
 	const char *cp;
@@ -432,18 +479,27 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 					   show_raw_time),
 			       ent->lno + 1 + cnt);
 		} else {
-			if (opt & OUTPUT_SHOW_SCORE)
+			if (opt & OUTPUT_SHOW_SCORE) {
+				setup_field_color(opt, cnt, OUTPUT_SHOW_SCORE,
+						  ent, prev, &col, &rcol);
 				printf(" %s%*d %02d%s", col,
 				       max_score_digits, ent->score,
 				       ent->suspect->refcnt, rcol);
-			if (opt & OUTPUT_SHOW_NAME)
+			}
+			if (opt & OUTPUT_SHOW_NAME) {
+				setup_field_color(opt, cnt, OUTPUT_SHOW_NAME,
+						   ent, prev, &col, &rcol);
 				printf(" %s%-*.*s%s", col, longest_file,
 						      longest_file,
 						      suspect->path,
 						      rcol);
-			if (opt & OUTPUT_SHOW_NUMBER)
+			}
+			if (opt & OUTPUT_SHOW_NUMBER) {
+				setup_field_color(opt, cnt, OUTPUT_SHOW_NUMBER,
+						  ent, prev, &col, &rcol);
 				printf(" %s%*d%s", col, max_orig_digits,
 				       ent->s_lno + 1 + cnt, rcol);
+			}
 			if (!(opt & OUTPUT_NO_AUTHOR)) {
 				const char *name;
 				int pad;
@@ -452,6 +508,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 				else
 					name = ci.author.buf;
 				pad = longest_author - utf8_strwidth(name);
+				setup_field_color(opt, cnt, OUTPUT_NO_AUTHOR,
+						  ent, prev, &col, &rcol);
 				printf(" %s(%s%*s %10s%s", col,
 				       name, pad, "",
 				       format_time(ci.author_time,
@@ -477,7 +535,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 
 static void output(struct blame_scoreboard *sb, int option)
 {
-	struct blame_entry *ent;
+	struct blame_entry *ent, *prev = NULL;
 
 	if (option & OUTPUT_PORCELAIN) {
 		for (ent = sb->ent; ent; ent = ent->next) {
@@ -499,7 +557,8 @@ static void output(struct blame_scoreboard *sb, int option)
 		if (option & OUTPUT_PORCELAIN)
 			emit_porcelain(sb, ent, option);
 		else {
-			emit_other(sb, ent, option);
+			emit_other(sb, ent, prev, option);
+			prev = ent;
 		}
 	}
 }
@@ -717,6 +776,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 		OPT_BIT('e', "show-email", &output_option, N_("Show author email instead of name (Default: off)"), OUTPUT_SHOW_EMAIL),
 		OPT_BIT('w', NULL, &xdl_opts, N_("Ignore whitespace differences"), XDF_IGNORE_WHITESPACE),
 		OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line"), OUTPUT_COLOR_LINE),
+		OPT_BIT(0, "color-fields", &output_option, N_("color redundant metadata fields from previous line"), OUTPUT_COLOR_FIELDS),
 
 		/*
 		 * The following two options are parsed by parse_revision_opt()
@@ -775,6 +835,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	revs.diffopt.flags.follow_renames = 0;
 	argc = parse_options_end(&ctx);
 
+	if ((output_option & OUTPUT_COLOR_LINE) &&
+	    (output_option & OUTPUT_COLOR_FIELDS))
+		die(_("cannot ask for colored lines and fields at the same time"));
+
 	if (incremental || (output_option & OUTPUT_PORCELAIN)) {
 		if (show_progress > 0)
 			die(_("--progress can't be used with --incremental or porcelain formats"));
@@ -978,8 +1042,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 
 	if (!(output_option & OUTPUT_PORCELAIN)) {
 		find_alignment(&sb, &output_option);
-		if ((output_option & OUTPUT_COLOR_LINE) &&
-		    !repeated_meta_color)
+		if (!repeated_meta_color &&
+		    (output_option & (OUTPUT_COLOR_LINE | OUTPUT_COLOR_FIELDS)))
 			repeated_meta_color = GIT_COLOR_DARK;
 	}
 
diff --git a/t/t8012-blame-colors.sh b/t/t8012-blame-colors.sh
index b2de03b777..be491bde9a 100755
--- a/t/t8012-blame-colors.sh
+++ b/t/t8012-blame-colors.sh
@@ -15,4 +15,42 @@ test_expect_success 'colored blame colors continuous lines' '
 	test_line_count = 3 H.expect
 '
 
+test_expect_success 'colored blame colors continuous fields' '
+
+	git mv hello.c world.c &&
+	git commit -a -m "moved file" &&
+	cat <<-EOF >> world.c &&
+	void world()
+	{
+		puts("world");
+	}
+	EOF
+	git add world.c &&
+	GIT_AUTHOR_NAME="F" GIT_AUTHOR_EMAIL="F@test.git" \
+		git commit -m "forgot to add changes to moved file" &&
+
+	git blame --abbrev=12 --color-fields world.c >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+
+	grep "<BOLD;BLACK>hello.c" actual > colored_hello.expect &&
+	grep "hello.c" actual > all_hello.expect &&
+	test_line_count = 9 colored_hello.expect &&
+	test_line_count = 10 all_hello.expect &&
+
+	grep "<BOLD;BLACK>world.c" actual > colored_world.expect &&
+	grep "world.c" actual > all_world.expect &&
+	test_line_count = 3 colored_world.expect &&
+	test_line_count = 4 all_world.expect &&
+
+	grep "(F" actual > all_F.expect &&
+	grep "<BOLD;BLACK>(F" actual > colored_F.expect &&
+	test_line_count = 8 all_F.expect &&
+	test_line_count = 5 colored_F.expect &&
+
+	grep "(H" actual > all_H.expect &&
+	grep "<BOLD;BLACK>(H" actual > colored_H.expect &&
+	test_line_count = 5 all_H.expect &&
+	test_line_count = 3 colored_H.expect
+'
+
 test_done
-- 
2.15.1.620.gb9897f4670-goog