threads / discuss / 47505

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

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

## tl;dr

12 messages between Dec 28, 2017 and Jan 8, 2018.

replies: 11people: 2as markdown or json

Stefan Beller· Dec 28, 2017, 21:03 UTC · lore
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
Stefan Beller· Dec 28, 2017, 21:03 UTC · re: Stefan Beller · lore

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

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 | 48 +++++++++++++++++++++++++++++++++++-------------
 2 files changed, 35 insertions(+), 15 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..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.1.620.gb9897f4670-goog
Eric Sunshine· Dec 28, 2017, 22:00 UTC · re: Stefan Beller · lore

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

On Thu, Dec 28, 2017 at 4:03 PM, Stefan Beller <sbeller@google.com> wrote:
Show 16 quoted lines
> Add documentation explaining the functions in color.h.
> While at it, mark them extern.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/color.h b/color.h
> @@ -72,26 +72,48 @@ extern int color_stdout_is_tty;
>  /*
> - * 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);

I don't see an explanation of what "color bytes" are. From where does one obtain such bytes? How is this function used? The function comment does not particularly answer these questions.

Show 5 quoted lines
> +/*
> + * 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);

I suppose that "constant for the given setting" means one of GIT_COLOR_NEVER , GIT_COLOR_AUTO, GIT_COLOR_ALWAYS? Perhaps say so explicitly?

Would it also make sense to say that boolean "true" ("yes", etc.) results in GIT_COLOR_ALWAYS and "false" ("no", etc.)" results in GIT_COLOR_NEVER?

Finally, for grammatical consistency with other comments:
    s/Parses/Parse
    s/Returns/Return/
> +/* Is the output supposed to be colored? Resolve and cache the 'auto' setting */
> +extern int want_color(int var);

What is the 'var' argument? How is it interpreted? (...goes and checks implementation...) I guess this documentation should explain that the caller would pass in the result of git_config_colorbool().

Also, the meaning of "Resolve and cache 'auto' setting" stumped me for a while since it's not clear why it's here (or why it's missing the full stop), but I eventually realized that it's describing an implementation detail, which probably doesn't belong in API documentation.

Show 6 quoted lines
> +/*
> + * 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);

What does "human readable" mean in this context? Is it talking about color names or RGB(A) tuples or what?

Also, how does the caller know how large to make 'dst'? At minimum, you should say something about COLOR_MAXLEN.

Finally, for the 'len' case, what happens if 'dst' is too small? This should be documented.

And, the return value of these functions should be discussed.
> +/*
> + * Print the format string `fmt`, encapsulated by setting and resetting the
> + * color. Omits the color encapsulation if `color` is NULL.

The "encapsulated by setting and resetting the color" bit is hard to grok. Perhaps instead say something along the lines of:

    Output the formatted string in the specified color (and
    then reset to normal color so subsequent output is
    uncolored).
> + * The `color_fprintf_ln` prints a new line after resetting the color.
> + * The `color_print_strbuf` prints the given pre-formatted strbuf instead.

Should the strbuf variation warn that it only outputs content up to the first embedded NUL? (Or should that bug/misfeature be fixed?)

Show 11 quoted lines
> + */
>  __attribute__((format (printf, 3, 4)))
> +extern int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
>  __attribute__((format (printf, 3, 4)))
> +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.
Maybe: s/application/caller/
> + */
> +extern int color_is_nil(const char *color);
Stefan Beller· Jan 4, 2018, 19:49 UTC · re: Eric Sunshine · lore

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

On Thu, Dec 28, 2017 at 2:00 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 21 quoted lines
> On Thu, Dec 28, 2017 at 4:03 PM, Stefan Beller <sbeller@google.com> wrote:
>> Add documentation explaining the functions in color.h.
>> While at it, mark them extern.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> diff --git a/color.h b/color.h
>> @@ -72,26 +72,48 @@ extern int color_stdout_is_tty;
>>  /*
>> - * 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);
>
> I don't see an explanation of what "color bytes" are. From where does
> one obtain such bytes? How is this function used? The function comment
> does not particularly answer these questions.
Right. This description is bad.
It's implementation is just
    xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
Apparently this function is only ever used by grep.c which uses
it to fill in struct grep_opt {
    ...
    char color_context[COLOR_MAXLEN];
    char color_filename[COLOR_MAXLEN];
    char color_function[COLOR_MAXLEN];
    char color_lineno[COLOR_MAXLEN];
    char color_match_context[COLOR_MAXLEN];
    char color_match_selected[COLOR_MAXLEN];
    char color_selected[COLOR_MAXLEN];
    char color_sep[COLOR_MAXLEN];
    ...
}

I guess I'll drop the documentation for color_set, and put a NEEDSWORK comment there as I'd think we don't need to copy around the colors using snprintf, but either can keep pointers or use xmemdup.

Show 10 quoted lines
>
>> +/*
>> + * 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);
>
> I suppose that "constant for the given setting" means one of
> GIT_COLOR_NEVER , GIT_COLOR_AUTO, GIT_COLOR_ALWAYS? Perhaps say so
> explicitly?

Maybe I should fix the code as well. Currently it only returns one of 0 (=GIT_COLOR_NEVER), 1 (=GIT_COLOR_ALWAYS) and GIT_COLOR_AUTO.

> Would it also make sense to say that boolean "true" ("yes", etc.)
> results in GIT_COLOR_ALWAYS and "false" ("no", etc.)" results in
> GIT_COLOR_NEVER?
done.
>
> Finally, for grammatical consistency with other comments:
>     s/Parses/Parse
>     s/Returns/Return/
done
Show 13 quoted lines
>
>> +/* Is the output supposed to be colored? Resolve and cache the 'auto' setting */
>> +extern int want_color(int var);
>
> What is the 'var' argument? How is it interpreted? (...goes and checks
> implementation...) I guess this documentation should explain that the
> caller would pass in the result of git_config_colorbool().
>
> Also, the meaning of "Resolve and cache 'auto' setting" stumped me for
> a while since it's not clear why it's here (or why it's missing the
> full stop), but I eventually realized that it's describing an
> implementation detail, which probably doesn't belong in API
> documentation.
done
Show 29 quoted lines
>
>> +/*
>> + * 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);
>
> What does "human readable" mean in this context? Is it talking about
> color names or RGB(A) tuples or what?
>
> Also, how does the caller know how large to make 'dst'? At minimum,
> you should say something about COLOR_MAXLEN.
>
> Finally, for the 'len' case, what happens if 'dst' is too small? This
> should be documented.
>
> And, the return value of these functions should be discussed.
>
>> +/*
>> + * Print the format string `fmt`, encapsulated by setting and resetting the
>> + * color. Omits the color encapsulation if `color` is NULL.
>
> The "encapsulated by setting and resetting the color" bit is hard to
> grok. Perhaps instead say something along the lines of:
>
>     Output the formatted string in the specified color (and
>     then reset to normal color so subsequent output is
>     uncolored).
sounds good.
Show 6 quoted lines
>
>> + * The `color_fprintf_ln` prints a new line after resetting the color.
>> + * The `color_print_strbuf` prints the given pre-formatted strbuf instead.
>
> Should the strbuf variation warn that it only outputs content up to
> the first embedded NUL? (Or should that bug/misfeature be fixed?)
added.
Show 14 quoted lines
>
>> + */
>>  __attribute__((format (printf, 3, 4)))
>> +extern int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
>>  __attribute__((format (printf, 3, 4)))
>> +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.
>
> Maybe: s/application/caller/
done
>
>> + */
>> +extern int color_is_nil(const char *color);
Stefan Beller· Dec 28, 2017, 21:03 UTC · re: Stefan Beller · lore

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

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 b18c0f97fe..2c211b6e7d 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..09afb4d3ea 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.1.620.gb9897f4670-goog
Eric Sunshine· Dec 28, 2017, 22:29 UTC · re: Stefan Beller · lore

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

On Thu, Dec 28, 2017 at 4:03 PM, Stefan Beller <sbeller@google.com> wrote:
Show 31 quoted lines
> 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>
> ---
> diff --git a/builtin/blame.c b/builtin/blame.c
> @@ -367,6 +370,28 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
> +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);
> +}

I'm not convinced that this colors_unset() / colors_set() / setup_line_color() abstraction is buying much. With this abstraction, I found the code more difficult to reason about than if the colors were just set/unset manually in the code which needs the colors. I *could* perhaps imagine setup_line_color() existing as a separate function since it is slightly more complex than the other two, but as it has only a single caller through all patches, even that may not be sufficient to warrant its existence.

Show 5 quoted lines
> @@ -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;

I can't help but read these as "column" and "[r]column"; the former, especially, is just too ingrained to interpret it any other way. Perhaps call these "color" and "reset" instead?

Show 5 quoted lines
> @@ -607,6 +636,12 @@ static int git_blame_config(const char *var, const char *value, void *cb)
> +       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);

Does this need to say "ignore"? If you drop that word, you still have a valid warning message.

Show 7 quoted lines
> +               return 0;
> +       }
> @@ -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),

Not sure what this help message means. Do you mean "color redundant ... _differently_ ..."? Or "_dim_ redundant..."?

Show 11 quoted lines
> diff --git a/t/t8012-blame-colors.sh 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' '
What are "continuous lines"? Did you mean "contiguous"?
Show 7 quoted 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
> +'
Stefan Beller· Jan 4, 2018, 22:10 UTC · re: Eric Sunshine · lore

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

On Thu, Dec 28, 2017 at 2:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 41 quoted lines
> On Thu, Dec 28, 2017 at 4:03 PM, Stefan Beller <sbeller@google.com> wrote:
>> 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>
>> ---
>> diff --git a/builtin/blame.c b/builtin/blame.c
>> @@ -367,6 +370,28 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
>> +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);
>> +}
>
> I'm not convinced that this colors_unset() / colors_set() /
> setup_line_color() abstraction is buying much. With this abstraction,
> I found the code more difficult to reason about than if the colors
> were just set/unset manually in the code which needs the colors. I
> *could* perhaps imagine setup_line_color() existing as a separate
> function since it is slightly more complex than the other two, but as
> it has only a single caller through all patches, even that may not be
> sufficient to warrant its existence.

Have you viewed this patch in context of the following patch? Originally I was convinced an abstraction was not needed, but as the next patch shows, a helper per field seems handy.

Show 10 quoted lines
>
>> @@ -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;
>
> I can't help but read these as "column" and "[r]column"; the former,
> especially, is just too ingrained to interpret it any other way.
> Perhaps call these "color" and "reset" instead?
will fix.
Show 9 quoted lines
>
>> @@ -607,6 +636,12 @@ static int git_blame_config(const char *var, const char *value, void *cb)
>> +       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);
>
> Does this need to say "ignore"? If you drop that word, you still have
> a valid warning message.
dropped 'ignore'.
Show 11 quoted lines
>
>> +               return 0;
>> +       }
>> @@ -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),
>
> Not sure what this help message means. Do you mean "color redundant
> ... _differently_ ..."? Or "_dim_ redundant..."?

Originally (in a patch set a couple months back) I had 'dim', but Junio seems to have an interesting color setup, such that he would not call it dimming IIRC, hence I think I wanted to say color _differently_. Fixed.

Show 13 quoted lines
>> diff --git a/t/t8012-blame-colors.sh 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' '
>
> What are "continuous lines"? Did you mean "contiguous"?
Thanks for hinting at the subtle difference!

Thanks for the review! (I will drop the abstraction and see how it goes)

Stefan
Eric Sunshine· Jan 6, 2018, 08:26 UTC · re: Stefan Beller · lore

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

On Thu, Jan 4, 2018 at 5:10 PM, Stefan Beller <sbeller@google.com> wrote:
Show 26 quoted lines
> On Thu, Dec 28, 2017 at 2:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Thu, Dec 28, 2017 at 4:03 PM, Stefan Beller <sbeller@google.com> wrote:
>>> +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;
>>> +}
>>
>> I'm not convinced that this colors_unset() / colors_set() /
>> setup_line_color() abstraction is buying much. With this abstraction,
>> I found the code more difficult to reason about than if the colors
>> were just set/unset manually in the code which needs the colors. I
>> *could* perhaps imagine setup_line_color() existing as a separate
>> function since it is slightly more complex than the other two, but as
>> it has only a single caller through all patches, even that may not be
>> sufficient to warrant its existence.
>
> Have you viewed this patch in context of the following patch?
> Originally I was convinced an abstraction was not needed, but
> as the next patch shows, a helper per field seems handy.

I did take the other patch into consideration when making the observation, and I still found the code more difficult to reason about than if these minor bits of code had merely been inlined into the callers. I neglected to mention previously that part of the problem may be that these function names do not do a good job of conveying what is being done, thus I repeatedly had to consult the function implementations while reading callers in order to understand what was going on.

Stefan Beller· Jan 8, 2018, 19:38 UTC · re: Eric Sunshine · lore

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

On Sat, Jan 6, 2018 at 12:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 36 quoted lines
> On Thu, Jan 4, 2018 at 5:10 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Thu, Dec 28, 2017 at 2:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Thu, Dec 28, 2017 at 4:03 PM, Stefan Beller <sbeller@google.com> wrote:
>>>> +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;
>>>> +}
>>>
>>> I'm not convinced that this colors_unset() / colors_set() /
>>> setup_line_color() abstraction is buying much. With this abstraction,
>>> I found the code more difficult to reason about than if the colors
>>> were just set/unset manually in the code which needs the colors. I
>>> *could* perhaps imagine setup_line_color() existing as a separate
>>> function since it is slightly more complex than the other two, but as
>>> it has only a single caller through all patches, even that may not be
>>> sufficient to warrant its existence.
>>
>> Have you viewed this patch in context of the following patch?
>> Originally I was convinced an abstraction was not needed, but
>> as the next patch shows, a helper per field seems handy.
>
> I did take the other patch into consideration when making the
> observation, and I still found the code more difficult to reason about
> than if these minor bits of code had merely been inlined into the
> callers. I neglected to mention previously that part of the problem
> may be that these function names do not do a good job of conveying
> what is being done, thus I repeatedly had to consult the function
> implementations while reading callers in order to understand what was
> going on.

I asked the question before rewriting and resending, and now I agree that we do not want to have these small helpers.

Thanks, Stefan

Stefan Beller· Dec 28, 2017, 21:03 UTC · re: Stefan Beller · lore

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

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

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 2c211b6e7d..52f27b2ced 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 39e8c0b222..b23e98e2ac 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.1.620.gb9897f4670-goog
Eric Sunshine· Dec 28, 2017, 22:45 UTC · re: Stefan Beller · lore

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

On Thu, Dec 28, 2017 at 4:03 PM, Stefan Beller <sbeller@google.com> wrote:
> Choose a different color for dates and imitate a 'temperature cool down'
> for the dates.
s/for the dates/depending upon age/
Show 6 quoted lines
> [...]
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/blame.c b/builtin/blame.c
> @@ -777,6 +847,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
> +               OPT_BIT(0, "heated-lines", &output_option, N_("color lines by date"), OUTPUT_HEATED_LINES),
s/date/age/
Stefan Beller· Dec 28, 2017, 21:03 UTC · re: Stefan Beller · lore

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

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

← back to recent threads