[PATCH 0/2] blame: color line by commit

[PATCH 0/2] blame: color line by commit

[Stefan Beller]

This is a resend of sb/blame-color, dropping the patch
"add option to color metadata fields separately" as I think it is not needed.

> Expecting a reroll.
> cf. https://public-inbox.org/git/20171110011002.10179-1-sbeller@google.com/#t
> error messages are funny, can segfault, ...

The error messages have been addressed. However a default mode that is enabled
by color.ui has not been added yet.

Thanks,
Stefan

Stefan Beller (2):
  builtin/blame: dim uninteresting metadata lines
  builtin/blame: highlight recently changed lines

 Documentation/config.txt |  23 ++++++++
 builtin/blame.c          | 118 +++++++++++++++++++++++++++++++++++++--
 color.h                  |   1 +
 t/t8012-blame-colors.sh  |  54 ++++++++++++++++++
 4 files changed, 192 insertions(+), 4 deletions(-)
 create mode 100755 t/t8012-blame-colors.sh

-- 
2.17.0.484.g0c8726318c-goog

[PATCH 1/2] builtin/blame: dim uninteresting metadata lines

[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. Both the command line option '--color-lines'
as well as a config option 'color.blame.colorLines' is provided.

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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..a223232263 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1218,6 +1218,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 db38c0b307..b49fee70a9 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[COLOR_MAXLEN];
 
 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)
 {
@@ -375,6 +378,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 	struct commit_info ci;
 	char hex[GIT_MAX_HEXSZ + 1];
 	int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
+	const char *color = "", *reset = "";
 
 	get_commit_info(suspect->commit, &ci, 1);
 	oid_to_hex_r(hex, &suspect->commit->object.oid);
@@ -384,6 +388,19 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 		char ch;
 		int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : abbrev;
 
+		if (!(opt & OUTPUT_ANNOTATE_COMPAT)) {
+			if (opt & OUTPUT_COLOR_LINE) {
+				if (cnt > 0) {
+					color = repeated_meta_color;
+					reset = GIT_COLOR_RESET;
+				} else  {
+					color ="";
+					reset = "";
+				}
+			}
+			fputs(color, stdout);
+		}
+
 		if (suspect->commit->object.flags & UNINTERESTING) {
 			if (blank_boundary)
 				memset(hex, ' ', length);
@@ -433,6 +450,9 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 			printf(" %*d) ",
 			       max_digits, ent->lno + 1 + cnt);
 		}
+		if (!(opt & OUTPUT_ANNOTATE_COMPAT) &&
+		    (opt & OUTPUT_COLOR_LINE))
+			fputs(reset, stdout);
 		do {
 			ch = *cp++;
 			putchar(ch);
@@ -585,6 +605,8 @@ static const char *add_prefix(const char *prefix, const char *path)
 
 static int git_blame_config(const char *var, const char *value, void *cb)
 {
+	int *output_option = cb;
+
 	if (!strcmp(var, "blame.showroot")) {
 		show_root = git_config_bool(var, value);
 		return 0;
@@ -607,6 +629,13 @@ 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.repeatedlines")) {
+		if (color_parse_mem(value, strlen(value), repeated_meta_color))
+			warning(_("invalid color '%s' in color.blame.repeatedLines"),
+				value);
+		*output_option |= OUTPUT_COLOR_LINE;
+		return 0;
+	}
 
 	if (git_diff_heuristic_config(var, value, cb) < 0)
 		return -1;
@@ -690,6 +719,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()
@@ -949,8 +979,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 (!*repeated_meta_color &&
+		    (output_option & OUTPUT_COLOR_LINE))
+			strcpy(repeated_meta_color, GIT_COLOR_DARK);
+	}
 
 	output(&sb, output_option);
 	free((void *)sb.final_buf);
diff --git a/color.h b/color.h
index cd0bcedd08..196be16058 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..bb0fe20e76
--- /dev/null
+++ b/t/t8012-blame-colors.sh
@@ -0,0 +1,29 @@
+#!/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>" <actual >darkened &&
+	grep "(F" darkened > F.expect &&
+	grep "(H" darkened > H.expect &&
+	test_line_count = 2 F.expect &&
+	test_line_count = 3 H.expect
+'
+
+test_expect_success 'colored blame colors contiguous lines via config' '
+	git -c color.blame.repeatedLines=yellow blame --abbrev=12 hello.c >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	grep "<YELLOW>" <actual >darkened &&
+	grep "(F" darkened > F.expect &&
+	grep "(H" darkened > H.expect &&
+	test_line_count = 2 F.expect &&
+	test_line_count = 3 H.expect
+'
+
+test_done
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 2/2] 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          | 78 +++++++++++++++++++++++++++++++++++++++-
 t/t8012-blame-colors.sh  | 25 +++++++++++++
 3 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a223232263..acc456e1fd 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1223,6 +1223,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 b49fee70a9..9b1021f10d 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>");
 
@@ -323,6 +324,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_HEATED_LINES	04000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -370,6 +372,63 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
 		putchar('\n');
 }
 
+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;
+
+	colorfield_nr = 0;
+
+	/* 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_colors_heated_lines(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, int opt)
 {
 	int cnt;
@@ -384,6 +443,12 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 	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, &color);
+		reset = GIT_COLOR_RESET;
+	}
+
 	for (cnt = 0; cnt < ent->num_lines; cnt++) {
 		char ch;
 		int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : abbrev;
@@ -451,7 +516,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 			       max_digits, ent->lno + 1 + cnt);
 		}
 		if (!(opt & OUTPUT_ANNOTATE_COMPAT) &&
-		    (opt & OUTPUT_COLOR_LINE))
+		    (opt & (OUTPUT_COLOR_LINE | OUTPUT_HEATED_LINES)))
 			fputs(reset, stdout);
 		do {
 			ch = *cp++;
@@ -636,6 +701,11 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 		*output_option |= OUTPUT_COLOR_LINE;
 		return 0;
 	}
+	if (!strcmp(var, "color.blame.highlightrecent")) {
+		parse_color_fields(value);
+		*output_option |= OUTPUT_HEATED_LINES;
+		return 0;
+	}
 
 	if (git_diff_heuristic_config(var, value, cb) < 0)
 		return -1;
@@ -720,6 +790,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, "heated-lines", &output_option, N_("color lines by age"), OUTPUT_HEATED_LINES),
 
 		/*
 		 * The following two options are parsed by parse_revision_opt()
@@ -744,6 +815,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	unsigned int range_i;
 	long anchor;
 
+	setup_default_colors_heated_lines();
 	git_config(git_blame_config, &output_option);
 	init_revisions(&revs, NULL);
 	revs.date_mode = blame_date_mode;
@@ -779,6 +851,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_HEATED_LINES))
+		die(_("cannot ask for colored lines and heated lines 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"));
diff --git a/t/t8012-blame-colors.sh b/t/t8012-blame-colors.sh
index bb0fe20e76..d6b2932bf8 100755
--- a/t/t8012-blame-colors.sh
+++ b/t/t8012-blame-colors.sh
@@ -26,4 +26,29 @@ test_expect_success 'colored blame colors contiguous lines via config' '
 	test_line_count = 3 H.expect
 '
 
+test_expect_success 'heated line coloring for old code' '
+	git blame --heated-lines hello.c >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	grep "<BLUE>" <actual >colored &&
+	test_line_count = 10 colored
+'
+
+test_expect_success 'heated line coloring' '
+	cat >>hello.c <<-EOF &&
+		void qfunc();
+	EOF
+	git add hello.c &&
+	GIT_AUTHOR_DATE="" git commit -m "new commit" &&
+
+	git blame --heated-lines hello.c >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+
+	grep "<BLUE>" <actual >colored &&
+	test_line_count = 10 colored &&
+
+	grep "<RED>" <actual >colored &&
+	test_line_count = 1 colored &&
+	grep qfunc colored
+'
+
 test_done
-- 
2.17.0.484.g0c8726318c-goog

Re: [PATCH 1/2] builtin/blame: dim uninteresting metadata lines

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> @@ -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

I wondered what these are about; they are about aligning with HT
assuming 8-place tab stop like the other lines.

>  #define OUTPUT_SHOW_EMAIL	0400
> -#define OUTPUT_LINE_PORCELAIN 01000
> +#define OUTPUT_LINE_PORCELAIN 	01000

But then this line has SP plus HT here; it should have been just a
single HT (i.e. replace a single SP with HT), to be consistent.

> +#define OUTPUT_COLOR_LINE	02000
>  
>  static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
>  {
> @@ -375,6 +378,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
>  	struct commit_info ci;
>  	char hex[GIT_MAX_HEXSZ + 1];
>  	int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
> +	const char *color = "", *reset = "";
>  
>  	get_commit_info(suspect->commit, &ci, 1);
>  	oid_to_hex_r(hex, &suspect->commit->object.oid);
> @@ -384,6 +388,19 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
>  		char ch;
>  		int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : abbrev;
>  
> +		if (!(opt & OUTPUT_ANNOTATE_COMPAT)) {
> +			if (opt & OUTPUT_COLOR_LINE) {
> +				if (cnt > 0) {
> +					color = repeated_meta_color;
> +					reset = GIT_COLOR_RESET;
> +				} else  {
> +					color ="";

You need a SP after '=' that assigns an empty string to 'color'.

> +					reset = "";
> +				}
> +			}
> +			fputs(color, stdout);
> +		}

This fputs() should be hidden to the case where color is *NOT* an
empty string, no?  In any case, it should be inside "if color-line
is in effect" block, I would think.

Users of "git annotate" would not pass the --color option, so I do
not think the outer if () block is worth the loss of readability due
to increased indent level.  

I would say that it is a job of the caller of git_config() to make
sure color.blame.lines would not take effect when the command is
annotate, if what you are worried about is the configuration in this
code.

> @@ -433,6 +450,9 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
>  			printf(" %*d) ",
>  			       max_digits, ent->lno + 1 + cnt);
>  		}
> +		if (!(opt & OUTPUT_ANNOTATE_COMPAT) &&
> +		    (opt & OUTPUT_COLOR_LINE))
> +			fputs(reset, stdout);

Likewise.

> @@ -949,8 +979,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 (!*repeated_meta_color &&
> +		    (output_option & OUTPUT_COLOR_LINE))
> +			strcpy(repeated_meta_color, GIT_COLOR_DARK);
> +	}

This code does not check OUTPUT_ANNOTATE_COMPAT because it assumes
that OUTPUT_COLOR_LINE won't be in output_option when we are working
in annotate compatibility mode.  The deeper codepaths we saw above
should do the same.  It should be enough to drop color-line when
anno-compat is set, or something like that, immediately after
reading the config and overriding them from the command line.

> diff --git a/color.h b/color.h
> index cd0bcedd08..196be16058 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"

I wonder if it is worth adding a new color only to give this a
different default.  

Traditionally, we use CYAN for lines that are less interesting than
others (e.g. hunk header), so reusing it might make the life easier
to the users, especially because I envision that we may want to
introduce another "logical" level to give another redirection
between the configuration keys like color.diff.frag and
color.blame.repeatedlines and the actual ANSI sequence like
"\033[36m".  I.e. we update the system so that these two
configuration keys take "uninteresting" (which is one of the
"logical" colors) by default, and then map "uninteresting" to
"\033[36m" at the physical level by default.  The users could then
change the mapping from "uninteresting" to "\033[1;30m" and
consistently modify both diff.frag and blame.repeated if they wanted
to.  Of course, if they want them to be different, they can come up
with a different "logical" color and split the two.  And from that
point of view, adding new colors to the default set we have above
will make life harder for the end users.

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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.
> ...
> @@ -323,6 +324,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_HEATED_LINES	04000

How about calling it OUTPUT_SHOW_AGE_WITH_COLOR or something like
that instead?  Anything with "AGE" in it, if that is what you are
trying to indicate, would be more appropriate than "HEATED", which
does not convey much meaning to readers unless you explain what
determines the temperature in your mind.

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Junio C Hamano]

It seems that this

$ git -c color.blame.repeatedlines=cyan blame --heated-lines builtin/blame.c

refuses to run.

Would it work if the configuration is in .git/config instead, or
would it forever disable --heated-lines once somebody choses to use
--color-lines feature by default by configuring it in?

Re: [PATCH 1/2] builtin/blame: dim uninteresting metadata lines

[Stefan Beller]

Hi Junio,

>> -#define OUTPUT_SHOW_SCORE      0100
>> -#define OUTPUT_NO_AUTHOR       0200
>> +#define OUTPUT_SHOW_SCORE    0100
>> +#define OUTPUT_NO_AUTHOR     0200
>
> I wondered what these are about; they are about aligning with HT
> assuming 8-place tab stop like the other lines.

correct.

>> -#define OUTPUT_LINE_PORCELAIN 01000
>> +#define OUTPUT_LINE_PORCELAIN        01000
>
> But then this line has SP plus HT here; it should have been just a
> single HT (i.e. replace a single SP with HT), to be consistent.

fixed

>> @@ -384,6 +388,19 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
>>               char ch;
>>               int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : abbrev;
>>
>> +             if (!(opt & OUTPUT_ANNOTATE_COMPAT)) {
>> +                     if (opt & OUTPUT_COLOR_LINE) {
>> +                             if (cnt > 0) {
>> +                                     color = repeated_meta_color;
>> +                                     reset = GIT_COLOR_RESET;
>> +                             } else  {
>> +                                     color ="";
>
> You need a SP after '=' that assigns an empty string to 'color'.
>
>> +                                     reset = "";
>> +                             }
>> +                     }
>> +                     fputs(color, stdout);
>> +             }
>
> This fputs() should be hidden to the case where color is *NOT* an
> empty string, no?  In any case, it should be inside "if color-line
> is in effect" block, I would think.
>
> Users of "git annotate" would not pass the --color option, so I do
> not think the outer if () block is worth the loss of readability due
> to increased indent level.
>
> I would say that it is a job of the caller of git_config() to make
> sure color.blame.lines would not take effect when the command is
> annotate, if what you are worried about is the configuration in this
> code.

ok, so we'll have to correct these mis aligned configs before hand and
here we just go by the bits set in the flags.

>> @@ -949,8 +979,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 (!*repeated_meta_color &&
>> +                 (output_option & OUTPUT_COLOR_LINE))
>> +                     strcpy(repeated_meta_color, GIT_COLOR_DARK);
>> +     }
>
> This code does not check OUTPUT_ANNOTATE_COMPAT because it assumes
> that OUTPUT_COLOR_LINE won't be in output_option when we are working
> in annotate compatibility mode.  The deeper codepaths we saw above
> should do the same.  It should be enough to drop color-line when
> anno-compat is set, or something like that, immediately after
> reading the config and overriding them from the command line.

Makes sense.

>
>> diff --git a/color.h b/color.h
>> index cd0bcedd08..196be16058 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"
>
> I wonder if it is worth adding a new color only to give this a
> different default.
>
> Traditionally, we use CYAN for lines that are less interesting than
> others (e.g. hunk header), so reusing it might make the life easier
> to the users, especially because I envision that we may want to
> introduce another "logical" level to give another redirection
> between the configuration keys like color.diff.frag and
> color.blame.repeatedlines and the actual ANSI sequence like
> "\033[36m".  I.e. we update the system so that these two
> configuration keys take "uninteresting" (which is one of the
> "logical" colors) by default, and then map "uninteresting" to
> "\033[36m" at the physical level by default.  The users could then
> change the mapping from "uninteresting" to "\033[1;30m" and
> consistently modify both diff.frag and blame.repeated if they wanted
> to.  Of course, if they want them to be different, they can come up
> with a different "logical" color and split the two.  And from that
> point of view, adding new colors to the default set we have above
> will make life harder for the end users.

That is a good longer term vision. I'll switch to cyan for now.
Thanks,
Stefan

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Stefan Beller]

On Mon, Apr 16, 2018 at 8:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> It seems that this
>
> $ git -c color.blame.repeatedlines=cyan blame --heated-lines builtin/blame.c
>
> refuses to run.
>
> Would it work if the configuration is in .git/config instead, or
> would it forever disable --heated-lines once somebody choses to use
> --color-lines feature by default by configuring it in?

That is the unfortunate part of this series, I have not figured out how to
treat these two options at the same time.

One could take the approach to check the config first and see if there
are conflicts and then overlay it with the command line options
(and resolve conflicts there, but CLI taking precedence over config).

Or we'd need to introduce another config
blame.coloring={none, repeatedlines, highlightrecent}
which breaks the tie.

Thanks,
Stefan

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Stefan Beller]

On Tue, Apr 17, 2018 at 12:31 PM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Apr 16, 2018 at 8:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> It seems that this
>>
>> $ git -c color.blame.repeatedlines=cyan blame --heated-lines builtin/blame.c
>>
>> refuses to run.
>>
>> Would it work if the configuration is in .git/config instead, or
>> would it forever disable --heated-lines once somebody choses to use
>> --color-lines feature by default by configuring it in?
>
> That is the unfortunate part of this series, I have not figured out how to
> treat these two options at the same time.
>
> One could take the approach to check the config first and see if there
> are conflicts and then overlay it with the command line options
> (and resolve conflicts there, but CLI taking precedence over config).
>
> Or we'd need to introduce another config
> blame.coloring={none, repeatedlines, highlightrecent}
> which breaks the tie.

Nevermind, we can overlay these color schemes just fine, which I'll do in
a resend.

[PATCH 1/2] builtin/blame: dim uninteresting metadata lines

[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. Both the command line option '--color-lines'
as well as a config option 'color.blame.colorLines' is provided.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Documentation/config.txt |  5 +++++
 builtin/blame.c          | 42 ++++++++++++++++++++++++++++++++++++----
 t/t8012-blame-colors.sh  | 29 +++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 4 deletions(-)
 create mode 100755 t/t8012-blame-colors.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..8128eee4f9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1218,6 +1218,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 cyan.
+
 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 db38c0b307..5190ff5df3 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[COLOR_MAXLEN];
 
 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)
 {
@@ -375,6 +378,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 	struct commit_info ci;
 	char hex[GIT_MAX_HEXSZ + 1];
 	int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
+	const char *color = NULL, *reset = NULL;
 
 	get_commit_info(suspect->commit, &ci, 1);
 	oid_to_hex_r(hex, &suspect->commit->object.oid);
@@ -384,6 +388,18 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 		char ch;
 		int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : abbrev;
 
+		if (opt & OUTPUT_COLOR_LINE) {
+			if (cnt > 0) {
+				color = repeated_meta_color;
+				reset = GIT_COLOR_RESET;
+			} else  {
+				color = "";
+				reset = "";
+			}
+		}
+		if (color)
+			fputs(color, stdout);
+
 		if (suspect->commit->object.flags & UNINTERESTING) {
 			if (blank_boundary)
 				memset(hex, ' ', length);
@@ -433,6 +449,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 			printf(" %*d) ",
 			       max_digits, ent->lno + 1 + cnt);
 		}
+		if (reset)
+			fputs(reset, stdout);
 		do {
 			ch = *cp++;
 			putchar(ch);
@@ -585,6 +603,8 @@ static const char *add_prefix(const char *prefix, const char *path)
 
 static int git_blame_config(const char *var, const char *value, void *cb)
 {
+	int *output_option = cb;
+
 	if (!strcmp(var, "blame.showroot")) {
 		show_root = git_config_bool(var, value);
 		return 0;
@@ -607,6 +627,13 @@ 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.repeatedlines")) {
+		if (color_parse_mem(value, strlen(value), repeated_meta_color))
+			warning(_("invalid color '%s' in color.blame.repeatedLines"),
+				value);
+		*output_option |= OUTPUT_COLOR_LINE;
+		return 0;
+	}
 
 	if (git_diff_heuristic_config(var, value, cb) < 0)
 		return -1;
@@ -690,6 +717,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()
@@ -949,8 +977,14 @@ 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 (!*repeated_meta_color &&
+		    (output_option & OUTPUT_COLOR_LINE))
+			strcpy(repeated_meta_color, GIT_COLOR_CYAN);
+	}
+	if (output_option & OUTPUT_ANNOTATE_COMPAT)
+		output_option &= ~OUTPUT_COLOR_LINE;
 
 	output(&sb, output_option);
 	free((void *)sb.final_buf);
diff --git a/t/t8012-blame-colors.sh b/t/t8012-blame-colors.sh
new file mode 100755
index 0000000000..9fc32ff813
--- /dev/null
+++ b/t/t8012-blame-colors.sh
@@ -0,0 +1,29 @@
+#!/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 "<CYAN>" <actual >darkened &&
+	grep "(F" darkened > F.expect &&
+	grep "(H" darkened > H.expect &&
+	test_line_count = 2 F.expect &&
+	test_line_count = 3 H.expect
+'
+
+test_expect_success 'colored blame colors contiguous lines via config' '
+	git -c color.blame.repeatedLines=yellow blame --abbrev=12 hello.c >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	grep "<YELLOW>" <actual >darkened &&
+	grep "(F" darkened > F.expect &&
+	grep "(H" darkened > H.expect &&
+	test_line_count = 2 F.expect &&
+	test_line_count = 3 H.expect
+'
+
+test_done
-- 
2.17.0.484.g0c8726318c-goog

[PATCH 2/2] 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>

# Conflicts:
#	builtin/blame.c
---
 Documentation/config.txt | 17 +++++++++
 builtin/blame.c          | 81 ++++++++++++++++++++++++++++++++++++++--
 t/t8012-blame-colors.sh  | 25 +++++++++++++
 3 files changed, 119 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8128eee4f9..eae88be662 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1223,6 +1223,23 @@ color.blame.repeatedMeta::
 	is repeated meta information per line (such as commit id,
 	author name, date and timezone). Defaults to cyan.
 
+color.blame.highlightRecent::
+	This can be used to color the metadata of a blame line depending
+	on age of the line.
++
+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 5190ff5df3..53999df511 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>");
 
@@ -323,6 +324,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_SHOW_AGE_WITH_COLOR	04000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -370,6 +372,63 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
 		putchar('\n');
 }
 
+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;
+
+	colorfield_nr = 0;
+
+	/* 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_color_by_age(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, int opt)
 {
 	int cnt;
@@ -378,12 +437,19 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 	struct commit_info ci;
 	char hex[GIT_MAX_HEXSZ + 1];
 	int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
-	const char *color = NULL, *reset = NULL;
+	const char *default_color = NULL, *color = NULL, *reset = NULL;
 
 	get_commit_info(suspect->commit, &ci, 1);
 	oid_to_hex_r(hex, &suspect->commit->object.oid);
 
 	cp = blame_nth_line(sb, ent->lno);
+
+	if (opt & OUTPUT_SHOW_AGE_WITH_COLOR) {
+		determine_line_heat(ent, &default_color);
+		color = default_color;
+		reset = GIT_COLOR_RESET;
+	}
+
 	for (cnt = 0; cnt < ent->num_lines; cnt++) {
 		char ch;
 		int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : abbrev;
@@ -393,8 +459,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 				color = repeated_meta_color;
 				reset = GIT_COLOR_RESET;
 			} else  {
-				color = "";
-				reset = "";
+				color = default_color ? default_color : "";
+				reset = default_color ? GIT_COLOR_RESET : "";
 			}
 		}
 		if (color)
@@ -634,6 +700,11 @@ static int git_blame_config(const char *var, const char *value, void *cb)
 		*output_option |= OUTPUT_COLOR_LINE;
 		return 0;
 	}
+	if (!strcmp(var, "color.blame.highlightrecent")) {
+		parse_color_fields(value);
+		*output_option |= OUTPUT_SHOW_AGE_WITH_COLOR;
+		return 0;
+	}
 
 	if (git_diff_heuristic_config(var, value, cb) < 0)
 		return -1;
@@ -718,6 +789,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-by-age", &output_option, N_("color lines by age"), OUTPUT_SHOW_AGE_WITH_COLOR),
 
 		/*
 		 * The following two options are parsed by parse_revision_opt()
@@ -742,6 +814,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 	unsigned int range_i;
 	long anchor;
 
+	setup_default_color_by_age();
 	git_config(git_blame_config, &output_option);
 	init_revisions(&revs, NULL);
 	revs.date_mode = blame_date_mode;
@@ -984,7 +1057,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 			strcpy(repeated_meta_color, GIT_COLOR_CYAN);
 	}
 	if (output_option & OUTPUT_ANNOTATE_COMPAT)
-		output_option &= ~OUTPUT_COLOR_LINE;
+		output_option &= ~(OUTPUT_COLOR_LINE | OUTPUT_SHOW_AGE_WITH_COLOR);
 
 	output(&sb, output_option);
 	free((void *)sb.final_buf);
diff --git a/t/t8012-blame-colors.sh b/t/t8012-blame-colors.sh
index 9fc32ff813..6f6be6c5dc 100755
--- a/t/t8012-blame-colors.sh
+++ b/t/t8012-blame-colors.sh
@@ -26,4 +26,29 @@ test_expect_success 'colored blame colors contiguous lines via config' '
 	test_line_count = 3 H.expect
 '
 
+test_expect_success 'color by age consistently colors old code' '
+	git blame --color-by-age hello.c >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+	grep "<BLUE>" <actual >colored &&
+	test_line_count = 10 colored
+'
+
+test_expect_success 'blame color by age: new code is different' '
+	cat >>hello.c <<-EOF &&
+		void qfunc();
+	EOF
+	git add hello.c &&
+	GIT_AUTHOR_DATE="" git commit -m "new commit" &&
+
+	git blame --color-by-age hello.c >actual.raw &&
+	test_decode_color <actual.raw >actual &&
+
+	grep "<BLUE>" <actual >colored &&
+	test_line_count = 10 colored &&
+
+	grep "<RED>" <actual >colored &&
+	test_line_count = 1 colored &&
+	grep qfunc colored
+'
+
 test_done
-- 
2.17.0.484.g0c8726318c-goog

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Eric Sunshine]

On Tue, Apr 17, 2018 at 5:30 PM, Stefan Beller <sbeller@google.com> wrote:
> 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>
>
> # Conflicts:
> #       builtin/blame.c

Meh.

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Mon, Apr 16, 2018 at 8:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> It seems that this
>>
>> $ git -c color.blame.repeatedlines=cyan blame --heated-lines builtin/blame.c
>>
>> refuses to run.
>>
>> Would it work if the configuration is in .git/config instead, or
>> would it forever disable --heated-lines once somebody choses to use
>> --color-lines feature by default by configuring it in?
>
> That is the unfortunate part of this series, I have not figured out how to
> treat these two options at the same time.


Perhaps I wasn't clear enough, but I did not want to use both at the
same time.  "git blame --color-lines --heated-lines" that errors out
saying these cannot be used at the same time is an acceptable
limitation.

My sole complaint was that just like command line is used to
override (weaker) configs, the wish to use "repeatedlines" painting
by default expressed in the configuration form should be overriden
when there is an explicit command line --heated-lines option that is
incompatible with it.

In this particular case, you might be able to come up with a scheme
where both can be made effective at the same time, but the principle
still stands, and that is the more important lesson I'd like to see
you learn.

Thanks.

Re: [PATCH 1/2] builtin/blame: dim uninteresting metadata lines

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> @@ -375,6 +378,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
>  	struct commit_info ci;
>  	char hex[GIT_MAX_HEXSZ + 1];
>  	int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
> +	const char *color = NULL, *reset = NULL;
>  
>  	get_commit_info(suspect->commit, &ci, 1);
>  	oid_to_hex_r(hex, &suspect->commit->object.oid);
> @@ -384,6 +388,18 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
>  		char ch;
>  		int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : abbrev;
>  
> +		if (opt & OUTPUT_COLOR_LINE) {
> +			if (cnt > 0) {
> +				color = repeated_meta_color;
> +				reset = GIT_COLOR_RESET;
> +			} else  {
> +				color = "";
> +				reset = "";

Shouldn't these be NULL as you do not want to fputs() these when not
in painting mode anyway?  Which would make it consistent with ...

> +			}
> +		}
> +		if (color)
> +			fputs(color, stdout);
> +

... this thing which otherwise needs to be "if (color && *color)",
but if you do NULL, can be left as-is ;-)

> @@ -433,6 +449,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
>  			printf(" %*d) ",
>  			       max_digits, ent->lno + 1 + cnt);
>  		}
> +		if (reset)
> +			fputs(reset, stdout);

Likewise.

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Apr 17, 2018 at 5:30 PM, Stefan Beller <sbeller@google.com> wrote:
>> 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>
>>
>> # Conflicts:
>> #       builtin/blame.c
>
> Meh.

Sloppy.

Will edit it out.

Thanks.

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[René Scharfe]

Am 17.04.2018 um 23:30 schrieb Stefan Beller:
> +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;
> +
> +	colorfield_nr = 0;
> +
> +	/* Ideally this would be stripped and split at the same time? */

Why?  Both approxidate() and color_parse() handle spaces.

> +	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;
> +}

This adds a minor memory leak; fix below.

-- >8 --
Subject: [PATCH] blame: release string_list after use in parse_color_fields()

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/blame.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/blame.c b/builtin/blame.c
index 4202584f97..3295718841 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -411,6 +411,7 @@ static void parse_color_fields(const char *s)
 		die (_("must end with a color"));
 
 	colorfield[colorfield_nr].hop = TIME_MAX;
+	string_list_clear(&l, 0);
 }
 
 static void setup_default_color_by_age(void)
-- 
2.17.1

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Stefan Beller]

On Sat, Jun 9, 2018 at 4:26 AM René Scharfe <l.s.r@web.de> wrote:
>
> Am 17.04.2018 um 23:30 schrieb Stefan Beller:
> > +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;
> > +
> > +     colorfield_nr = 0;
> > +
> > +     /* Ideally this would be stripped and split at the same time? */
>
> Why?  Both approxidate() and color_parse() handle spaces.

I think that comment is stale; I remember experimenting with
different internal structs to write a parser until I came up with this
implementation. It is a left over.

> This adds a minor memory leak; fix below.

Thanks!

> -- >8 --
> Subject: [PATCH] blame: release string_list after use in parse_color_fields()
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  builtin/blame.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 4202584f97..3295718841 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -411,6 +411,7 @@ static void parse_color_fields(const char *s)
>                 die (_("must end with a color"));
>
>         colorfield[colorfield_nr].hop = TIME_MAX;
> +       string_list_clear(&l, 0);
>  }
>
>  static void setup_default_color_by_age(void)
> --
> 2.17.1

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

>
> This adds a minor memory leak; fix below.
>
> -- >8 --
> Subject: [PATCH] blame: release string_list after use in parse_color_fields()
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---

Thanks.  Will apply.

>  builtin/blame.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 4202584f97..3295718841 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -411,6 +411,7 @@ static void parse_color_fields(const char *s)
>  		die (_("must end with a color"));
>  
>  	colorfield[colorfield_nr].hop = TIME_MAX;
> +	string_list_clear(&l, 0);
>  }
>  
>  static void setup_default_color_by_age(void)