threads / rfc / 46165

RFC patchbuiltin/blame: darken redundant line information

Subject: [RFC/PATCH] builtin/blame: darken redundant line information

## tl;dr

19 messages between Jun 13, 2017 and Jul 27, 2017. Diffs are folded; open one to read it.

replies: 18people: 3as markdown or json

Stefan Beller· Jun 13, 2017, 02:31 UTC · lore

When using git-blame lots of lines contain redundant information, for example in hunks that consist of multiple lines, the metadata (commit name, author, timezone) are repeated. A reader may not be interested in those, so darken them. The darkening is not just based on hunk, but actually takes the previous lines content for that field to compare to.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 Example output (blame of blame): http://i.imgur.com/0Y12p2f.png
 builtin/blame.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++----------
 color.h         |   1 +
 2 files changed, 112 insertions(+), 24 deletions(-)
Show changes to 2 files +112 −24

builtin/blame.c, color.h

diff --git a/builtin/blame.c b/builtin/blame.c
index d7a2df3b47..7f921df0e7 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -6,6 +6,7 @@
  */
 
 #include "cache.h"
+#include "color.h"
 #include "builtin.h"
 #include "commit.h"
 #include "diff.h"
@@ -282,7 +283,8 @@ static void found_guilty_entry(struct blame_entry *ent, void *data)
 }
 
 static const char *format_time(timestamp_t time, const char *tz_str,
-			       int show_raw_time)
+			       int show_raw_time,
+			       timestamp_t difftime, const char *difftz_str)
 {
 	static struct strbuf time_buf = STRBUF_INIT;
 
@@ -291,12 +293,41 @@ static const char *format_time(timestamp_t time, const char *tz_str,
 		strbuf_addf(&time_buf, "%"PRItime" %s", time, tz_str);
 	}
 	else {
-		const char *time_str;
+		const char *time_str, *prev_str;
 		size_t time_width;
 		int tz;
+
+		if (difftime)
+			prev_str = xstrdup(show_date(difftime,
+						     atoi(difftz_str),
+						     &blame_date_mode));
 		tz = atoi(tz_str);
 		time_str = show_date(time, tz, &blame_date_mode);
-		strbuf_addstr(&time_buf, time_str);
+
+
+		if (difftime) {
+			int len = strlen(prev_str) > strlen(time_str) ?
+				  strlen(time_str) : strlen(prev_str);
+			int i, j;
+			int last_nondigit = 0;
+			for (i = 0; i < len; i++) {
+				if (isdigit(time_str[i]) &&
+				    time_str[i] != prev_str[i])
+					break;
+				if (!isdigit(time_str[i])) {
+					strbuf_addstr(&time_buf, GIT_COLOR_DARK);
+					for (j = last_nondigit; j < i; j++)
+						strbuf_addch(&time_buf, time_str[j]);
+					strbuf_addstr(&time_buf, GIT_COLOR_RESET);
+					last_nondigit = i;
+				}
+			}
+			for (i = last_nondigit; i < strlen(time_str); i++)
+				strbuf_addch(&time_buf, time_str[i]);
+		} else {
+			strbuf_addstr(&time_buf, time_str);
+		}
+
 		/*
 		 * Add space paddings to time_buf to display a fixed width
 		 * string, and use time_width for display width calibration.
@@ -319,6 +350,7 @@ static const char *format_time(timestamp_t time, const char *tz_str,
 #define OUTPUT_NO_AUTHOR       0200
 #define OUTPUT_SHOW_EMAIL	0400
 #define OUTPUT_LINE_PORCELAIN 01000
+#define OUTPUT_SHOW_REDUNDANT 02000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -366,19 +398,43 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
 		putchar('\n');
 }
 
-static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int opt)
+static void emit_other(struct blame_scoreboard *sb,
+		       struct blame_entry *ent,
+		       struct blame_entry *prev,
+		       int opt)
 {
 	int cnt;
 	const char *cp;
 	struct blame_origin *suspect = ent->suspect;
-	struct commit_info ci;
+	struct commit_info ci, prev_ci;
 	char hex[GIT_MAX_HEXSZ + 1];
 	int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
+	int prev_same_field = 0;
+	const char *use_color, *reset_color = GIT_COLOR_RESET;
 
 	get_commit_info(suspect->commit, &ci, 1);
 	oid_to_hex_r(hex, &suspect->commit->object.oid);
 
 	cp = blame_nth_line(sb, ent->lno);
+
+	commit_info_init(&prev_ci);
+	if ((opt & OUTPUT_SHOW_REDUNDANT) && prev) {
+		get_commit_info(prev->suspect->commit, &prev_ci, 1);
+		if ((opt & OUTPUT_SHOW_SCORE) && ent->score == prev->score)
+			prev_same_field |= OUTPUT_SHOW_SCORE;
+		if ((opt & OUTPUT_SHOW_NAME) && prev->suspect && !strcmp(suspect->path, prev->suspect->path))
+			prev_same_field |= OUTPUT_SHOW_NAME;
+		if ((opt & OUTPUT_SHOW_NUMBER) &&
+		    ent->s_lno == prev->s_lno + prev->num_lines - 1)
+			prev_same_field |= OUTPUT_SHOW_NUMBER;
+		if (!(opt & OUTPUT_NO_AUTHOR)) {
+			if (((opt & OUTPUT_SHOW_EMAIL) &&
+			     !strcmp(ci.author_mail.buf, prev_ci.author_mail.buf)) ||
+			    !strcmp(ci.author.buf, prev_ci.author.buf))
+				prev_same_field |= OUTPUT_NO_AUTHOR;
+		}
+	}
+
 	for (cnt = 0; cnt < ent->num_lines; cnt++) {
 		char ch;
 		int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : abbrev;
@@ -391,8 +447,12 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 				putchar('^');
 			}
 		}
+		use_color = GIT_COLOR_NORMAL;
+		if ((opt & OUTPUT_SHOW_REDUNDANT) && cnt > 0)
+			use_color = GIT_COLOR_DARK;
+
+		printf("%s%.*s%s", use_color, length, hex, reset_color);
 
-		printf("%.*s", length, hex);
 		if (opt & OUTPUT_ANNOTATE_COMPAT) {
 			const char *name;
 			if (opt & OUTPUT_SHOW_EMAIL)
@@ -401,20 +461,36 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 				name = ci.author.buf;
 			printf("\t(%10s\t%10s\t%d)", name,
 			       format_time(ci.author_time, ci.author_tz.buf,
-					   show_raw_time),
+					   show_raw_time, 0, NULL),
 			       ent->lno + 1 + cnt);
 		} else {
-			if (opt & OUTPUT_SHOW_SCORE)
-				printf(" %*d %02d",
+			if (opt & OUTPUT_SHOW_SCORE) {
+				use_color = GIT_COLOR_NORMAL;
+				if ((opt & OUTPUT_SHOW_REDUNDANT) &&
+				    (cnt > 0 || prev_same_field & OUTPUT_SHOW_SCORE))
+					use_color = GIT_COLOR_DARK;
+				printf(" %s%*d %02d%s", use_color,
 				       max_score_digits, ent->score,
-				       ent->suspect->refcnt);
-			if (opt & OUTPUT_SHOW_NAME)
-				printf(" %-*.*s", longest_file, longest_file,
-				       suspect->path);
-			if (opt & OUTPUT_SHOW_NUMBER)
-				printf(" %*d", max_orig_digits,
-				       ent->s_lno + 1 + cnt);
-
+				       ent->suspect->refcnt, reset_color);
+			}
+			if (opt & OUTPUT_SHOW_NAME) {
+				use_color = GIT_COLOR_NORMAL;
+				if ((opt & OUTPUT_SHOW_REDUNDANT) &&
+				    (cnt > 0 || prev_same_field & OUTPUT_SHOW_NAME))
+					use_color = GIT_COLOR_DARK;
+				printf(" %s%-*.*s%s", use_color, longest_file,
+						      longest_file,
+						      suspect->path,
+						      reset_color);
+			}
+			if (opt & OUTPUT_SHOW_NUMBER) {
+				use_color = GIT_COLOR_NORMAL;
+				if ((opt & OUTPUT_SHOW_REDUNDANT) &&
+				    (cnt > 0 || prev_same_field & OUTPUT_SHOW_NUMBER))
+					use_color = GIT_COLOR_DARK;
+				printf(" %s%*d%s", use_color, max_orig_digits,
+				       ent->s_lno + 1 + cnt, reset_color);
+			}
 			if (!(opt & OUTPUT_NO_AUTHOR)) {
 				const char *name;
 				int pad;
@@ -422,12 +498,21 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 					name = ci.author_mail.buf;
 				else
 					name = ci.author.buf;
+
+				use_color = GIT_COLOR_NORMAL;
+				if ((opt & OUTPUT_SHOW_REDUNDANT) &&
+				    (cnt > 0 || prev_same_field & OUTPUT_NO_AUTHOR))
+					use_color = GIT_COLOR_DARK;
+
 				pad = longest_author - utf8_strwidth(name);
-				printf(" (%s%*s %10s",
-				       name, pad, "",
-				       format_time(ci.author_time,
-						   ci.author_tz.buf,
-						   show_raw_time));
+				printf(" %s(%s%*s%s", use_color,
+						      name, pad, "",
+						      reset_color);
+				printf(" %10s", format_time(ci.author_time,
+							    ci.author_tz.buf,
+							    show_raw_time,
+							    prev_ci.author_time,
+							    prev_ci.author_tz.buf));
 			}
 			printf(" %*d) ",
 			       max_digits, ent->lno + 1 + cnt);
@@ -447,7 +532,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) {
@@ -469,7 +554,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;
 		}
 	}
 }
@@ -680,6 +766,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('r', "redundant", &output_option, N_("darken redundancy from previous line (Default: off)"), OUTPUT_SHOW_REDUNDANT),
 
 		/*
 		 * The following two options are parsed by parse_revision_opt()
diff --git a/color.h b/color.h
index 90627650fc..bad2d7e29c 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"
-- 
2.13.0.17.gf3d7728391
Junio C Hamano· Jun 13, 2017, 15:25 UTC · re: Stefan Beller · lore

Re: [RFC/PATCH] builtin/blame: darken redundant line information

Stefan Beller <sbeller@google.com> writes:
Show 8 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, timezone) are repeated. A reader may not be interested in those,
> so darken them. The darkening is not just based on hunk, but actually
> takes the previous lines content for that field to compare to.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Not about "blame", but I was trying the --color-moved stuff on Brandon's "create config.h" patch and found its behaviour somewhat different from what I recall we discussed. I thought that the adjacentbounds mode was invented to dim (i.e. not attract undue attention) to most of the moved lines, but highlight only the boundary of moved blocks, so I expected most of the new and old lines in that patch would be shown in the "context" color, except for the boundary between two blocks of removed lines that have gone to different location (and similarly two blocks of new lines that have come from different location) would be painted in oldmoved and newmoved colors and their alternatives. Instead I see all old and new lines that are moved painted in these colors, without any dimming.

Is my expectation off?
Stefan Beller· Jun 13, 2017, 16:21 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] builtin/blame: darken redundant line information

On Tue, Jun 13, 2017 at 8:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> When using git-blame lots of lines contain redundant information, for
>> example in hunks that consist of multiple lines, the metadata (commit name,
>> author, timezone) are repeated. A reader may not be interested in those,
>> so darken them. The darkening is not just based on hunk, but actually
>> takes the previous lines content for that field to compare to.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>
> Not about "blame", but I was trying the --color-moved stuff on
> Brandon's "create config.h" patch and found its behaviour somewhat
> different from what I recall we discussed.

We discussed several things and we did not come to an agreement, what is best, so I implemented different things there.

> I thought that the
> adjacentbounds mode was invented to dim (i.e. not attract undue
> attention) to most of the moved lines, but highlight only the
> boundary of moved blocks,

I agree up to this part. And when you use the standard color scheme, the new/old moved is dimmed according to my perception of colors.

If you use an individual setup for colors, you need to adjust the four color.diff.{old, new}Moved[Alternative] slots.

> so I expected most of the new and old
> lines in that patch would be shown in the "context" color,

So instead of a special color in this mode you expected "context" for color.diff.{old, new}Moved and "highlighted" for the alternative slots.

Show 7 quoted lines
> except
> for the boundary between two blocks of removed lines that have gone
> to different location (and similarly two blocks of new lines that
> have come from different location) would be painted in oldmoved and
> newmoved colors and their alternatives.  Instead I see all old and
> new lines that are moved painted in these colors, without any
> dimming.

http://i.imgur.com/36DMRBl.png is what I see (regular colors, "git show --color-moved f2f1da5348ff2f297b43b") and that is what I would expect.

>
> Is my expectation off?

Maybe? It sounds as if you expected 'context' color to be used in the adjacentbounds.

* Do you expect 'context' to be used in other modes as well?
* Do you expect this as code/algorithm change or would rather
  a change of default colors (default {old/new}Moved = 'context')
  be sufficient to meet that expectation?
* As you have an individual color setup, maybe you can fix this
  for you by setting the appropriate slots to your perception of
  dimmed?

Thanks, Stefan

Junio C Hamano· Jun 13, 2017, 17:00 UTC · re: Stefan Beller · lore

Re: [RFC/PATCH] builtin/blame: darken redundant line information

Stefan Beller <sbeller@google.com> writes:
> * As you have an individual color setup, maybe you can fix this
>   for you by setting the appropriate slots to your perception of
>   dimmed?

I do not think it is possible with only {new,old}{,alternative} 4 colors.

Consider this diff:
         context
        -B
        -B
        -B
        -A
        -A
        -A
         context
        +A
        +A
        +A
        +B
        +B
        +B
         context

Two blocks (A and B) that are adjacent are moved but swapped to form a pair of new adjacent blocks.

We would like the boundary between the last "-B" and the first "-A" to be highlighted differently; all other "-A" and "-B" lines do not disappear but go elsewhere, so they want to be dimmed.

The newly added 6 lines are actually moved from elsewhere, and we would like the boundary between the last "+A" and the first "+B" to be highlighted differently, and others are dimmed.

So I'd think you would need at least two kinds of highlight colors plus a dimmed color.

         context
        -B		dim
        -B		dim
        -B		highlight
        -A		highlight
        -A		dim
        -A		dim
         context
        +A		dim
        +A		dim
        +A		highlight
        +B		highlight
        +B		dim
        +B		dim
         context

If old_moved and old_moved_alternative are meant for highlighting "-B" and "-A" above differently, while new_moved and new_moved_alternative are for highlighting "+A" and "+B" differently, you'd need a way to specify "dim" for old and new moved lines, which seems to be impossible with only 4 new colors.

Stefan Beller· Jun 13, 2017, 17:13 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] builtin/blame: darken redundant line information

On Tue, Jun 13, 2017 at 10:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 40 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> * As you have an individual color setup, maybe you can fix this
>>   for you by setting the appropriate slots to your perception of
>>   dimmed?
>
> I do not think it is possible with only {new,old}{,alternative} 4
> colors.
>
> Consider this diff:
>
>          context
>         -B
>         -B
>         -B
>         -A
>         -A
>         -A
>          context
>         +A
>         +A
>         +A
>         +B
>         +B
>         +B
>          context
>
> Two blocks (A and B) that are adjacent are moved but swapped to form
> a pair of new adjacent blocks.
>
> We would like the boundary between the last "-B" and the first "-A"
> to be highlighted differently; all other "-A" and "-B" lines do not
> disappear but go elsewhere, so they want to be dimmed.
>
> The newly added 6 lines are actually moved from elsewhere, and we
> would like the boundary between the last "+A" and the first "+B" to
> be highlighted differently, and others are dimmed.
>
> So I'd think you would need at least two kinds of highlight colors
> plus a dimmed color.
Here is what currently happens:
Show 17 quoted lines
>
>          context
>         -B              dim  oldMoved
>         -B              dim  oldMoved
>         -B              highlight oldMovedAlternative
>         -A              highlight oldMovedAlternative
>         -A              dim  oldMoved
>         -A              dim  oldMoved
>          context
>         +A              dim  newMoved
>         +A              dim  newMoved
>         +A              highlight  newMovedAlternative
>         +B              highlight  newMovedAlternative
>         +B              dim  newMoved
>         +B              dim  newMoved
>          context
>

So the there is only one "highlight" color in each block. There is no separate hightligh-for-ending-block and highlight-for-new-block respectively.

Show 5 quoted lines
> If old_moved and old_moved_alternative are meant for highlighting
> "-B" and "-A" above differently, while new_moved and
> new_moved_alternative are for highlighting "+A" and "+B"
> differently, you'd need a way to specify "dim" for old and new moved
> lines, which seems to be impossible with only 4 new colors.

The standard non alternative was dim in my mind for the adjacentbounds mode.

If you prefer to have the alternate mode, you rather want no dim, no highlight, but just 2 alternating colors of equal attention-drawing.

Junio C Hamano· Jun 13, 2017, 17:19 UTC · re: Stefan Beller · lore

Re: [RFC/PATCH] builtin/blame: darken redundant line information

Stefan Beller <sbeller@google.com> writes:
Show 23 quoted lines
> Here is what currently happens:
>
>>
>>          context
>>         -B              dim  oldMoved
>>         -B              dim  oldMoved
>>         -B              highlight oldMovedAlternative
>>         -A              highlight oldMovedAlternative
>>         -A              dim  oldMoved
>>         -A              dim  oldMoved
>>          context
>>         +A              dim  newMoved
>>         +A              dim  newMoved
>>         +A              highlight  newMovedAlternative
>>         +B              highlight  newMovedAlternative
>>         +B              dim  newMoved
>>         +B              dim  newMoved
>>          context
>>
>
> So the there is only one "highlight" color in each block.
> There is no separate hightligh-for-ending-block and
> highlight-for-new-block respectively.

I think the adjacentbounds mode is simply broken if that is the design.

In the above simplified case, you can get away with only a single "highlight" color, but you cannot tell where the boundaries are when three or more lines are shuffled, no?

Stefan Beller· Jun 13, 2017, 17:30 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] builtin/blame: darken redundant line information

On Tue, Jun 13, 2017 at 10:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 28 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> Here is what currently happens:
>>
>>>
>>>          context
>>>         -B              dim  oldMoved
>>>         -B              dim  oldMoved
>>>         -B              highlight oldMovedAlternative
>>>         -A              highlight oldMovedAlternative
>>>         -A              dim  oldMoved
>>>         -A              dim  oldMoved
>>>          context
>>>         +A              dim  newMoved
>>>         +A              dim  newMoved
>>>         +A              highlight  newMovedAlternative
>>>         +B              highlight  newMovedAlternative
>>>         +B              dim  newMoved
>>>         +B              dim  newMoved
>>>          context
>>>
>>
>> So the there is only one "highlight" color in each block.
>> There is no separate hightligh-for-ending-block and
>> highlight-for-new-block respectively.
>
> I think the adjacentbounds mode is simply broken if that is the
> design.

ok. Going by this reasoning, would you claim that allbounds would also be broken design:

Show 16 quoted lines
> git show --color-moved=allbounds:
>          context
>         -B              oldMovedAlternative
>         -B              oldMoved
>         -B              oldMovedAlternative
>         -A              oldMovedAlternative
>         -A              oldMoved
>         -A              oldMovedAlternative
>          context
>         +A              newMovedAlternative
>         +A              newMoved
>         +A              newMovedAlternative
>         +B              newMovedAlternative
>         +B              newMoved
>         +B              newMovedAlternative
>          context
>
> In the above simplified case, you can get away with only a single
> "highlight" color, but you cannot tell where the boundaries are when
> three or more lines are shuffled, no?

But you do not want to (yet)? The goal is not to tell you where the bounds are, but the goal is to point out that extra care is required for review of these particular 3 lines.

So IMHO this feature helps for drawing reviewer attention, but not for explaining blocks.

In an extreme alternative design, we would have just annotated each hunk in the context lines for example telling that there are n out of m new lines. But that information by itself is not useful for review

Instead this alternative moved line detection could have an impact on diff stats.

Junio C Hamano· Jun 13, 2017, 17:33 UTC · re: Stefan Beller · lore

Re: [RFC/PATCH] builtin/blame: darken redundant line information

Stefan Beller <sbeller@google.com> writes:
> But you do not want to (yet)? The goal is not to tell you where the bounds
> are, but the goal is to point out that extra care is required for review of
> these particular 3 lines.

And when you _can_ help users in that "extra care" by pointing out where the boundary is, what is the justification for hiding that information?

Stefan Beller· Jun 13, 2017, 17:44 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] builtin/blame: darken redundant line information

On Tue, Jun 13, 2017 at 10:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> But you do not want to (yet)? The goal is not to tell you where the bounds
>> are, but the goal is to point out that extra care is required for review of
>> these particular 3 lines.
>
> And when you _can_ help users in that "extra care" by pointing out
> where the boundary is, what is the justification for hiding that
> information?
It is very complicated and confusing. Consider this:
Show 18 quoted lines
>          context
>         -C
>          context
>         -B
>         -B
>         -B
>         -A
>         -A
>         -A
>          context
>         +A
>         +A
>         +A
>         +C
>         +B
>         +B
>         +B
>          context

So from your emails I understood you want to markup block starts and ends, but in this case C is *both* start and end of a block, and has also different blocks around.

So ideally we could tell the user this
Show 30 quoted lines
>          context
>         _context C goes to after +A
>         -C
>         _context C goes to before +B
>          context
>         _context -B goes to after +C
>         -B
>         -B
>         -B
>         _context -B goes to before contextB
>         _context -A goes to after contextA
>         -A
>         -A
>         -A
>         _context -A goes to after contextA
>          context
>         _context +A comes from after -B
>         +A
>         +A
>         +A
>         _context +A comes from before contextA
>         _context +C comes from after contextC
>         +C
>         _context +C comes from before contextC
>         _context +B comes from after contextB
>         +B
>         +B
>         +B
>         _context +B comes from after -A
>          context

(show the context of where the move is coming from/going to, maybe just one or two lines)

And how many colors would be confusing for the user?

I would think we want to start with a simple model and if a niche is not good (read: people think it can be improved easily compared to the usefulness they get out of it) enough we fix it up later.

I thought that adding 4 new colors is already maybe too much, as Git users were happy with a handful for 10 years, so I am very opposed to add more than 4 colors unless there is a very good reason. And I'd think that this is not adding a lot of useful information for a reviewer?

Thanks, Stefan

Junio C Hamano· Jun 13, 2017, 17:48 UTC · re: Stefan Beller · lore

Re: [RFC/PATCH] builtin/blame: darken redundant line information

Stefan Beller <sbeller@google.com> writes:
Show 35 quoted lines
> On Tue, Jun 13, 2017 at 10:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> But you do not want to (yet)? The goal is not to tell you where the bounds
>>> are, but the goal is to point out that extra care is required for review of
>>> these particular 3 lines.
>>
>> And when you _can_ help users in that "extra care" by pointing out
>> where the boundary is, what is the justification for hiding that
>> information?
>
> It is very complicated and confusing. Consider this:
>
>>          context
>>         -C
>>          context
>>         -B
>>         -B
>>         -B
>>         -A
>>         -A
>>         -A
>>          context
>>         +A
>>         +A
>>         +A
>>         +C
>>         +B
>>         +B
>>         +B
>>          context
>
> So from your emails I understood you want to markup
> block starts and ends, but in this case C is *both* start
> and end of a block, and has also different blocks around.

I never said "start and end" (you did). I just wanted the boundary of A and B and C clear, so I'd be perfectly happy with:

         context
        +A      dim
        +A      dim
        +A      highlight #1
        +C      highlight #2
        +B      highlight #1
        +B      dim
        +B      dim
         context
You can do that still with only two highlight colors, no?
Stefan Beller· Jun 13, 2017, 18:00 UTC · re: Junio C Hamano · lore

Re: [RFC/PATCH] builtin/blame: darken redundant line information

On Tue, Jun 13, 2017 at 10:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
>
> I never said "start and end" (you did).  I just wanted the boundary
> of A and B and C clear, so I'd be perfectly happy with:
>
>          context
>         +A      dim
>         +A      dim
>         +A      highlight #1
>         +C      highlight #2
>         +B      highlight #1
>         +B      dim
>         +B      dim
>          context
>
> You can do that still with only two highlight colors, no?
So to put it into an algorithm:
1) detect blocks
2) if blocks are adjacent, their bounds are eligible for highlighting
3) the highlighting is implemented using the "alternate" strategy
  in that any line highlighted belonging to a different block flips
  the highlighting, such that:
          context
         +A      dim
         +A      dim
         +A      highlight #1
         +B      highlight #2
         +B      dim
         +B      dim
          context
So if we go this way, we would need indeed 6 colors:
  Dimmed, Highlighted, HighlightedAlternative
color-moved modes:
nobounds::
  uses dimmed only
allbounds::
adjacentbounds::
  See algorithm above, using dimmed for inside the block and
  both highlights for bounds, making sure adjacent block bounds
  alternate the highlighting color.
alternate::
  Uses only highlighting colors, complete block is colored with
  one of the highlights

I think that is reasonable to implement. But I do still wonder if we really want to add so many new colors. I'll give it a try after my next submodule series.

Thanks, Stefan

Junio C Hamano· Jun 13, 2017, 18:06 UTC · re: Stefan Beller · lore

Re: [RFC/PATCH] builtin/blame: darken redundant line information

Stefan Beller <sbeller@google.com> writes:
Show 13 quoted lines
> On Tue, Jun 13, 2017 at 10:33 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> But you do not want to (yet)? The goal is not to tell you where the bounds
>>> are, but the goal is to point out that extra care is required for review of
>>> these particular 3 lines.
>>
>> And when you _can_ help users in that "extra care" by pointing out
>> where the boundary is, what is the justification for hiding that
>> information?
> ...
>
> And how many colors would be confusing for the user?

Well, having two "dimmed" for moved lines are not needed if we use the same as context, so we can reduce the colors to just two highlights times two (i.e. old and new).

Having said that,
> I would think we want to start with a simple model ...

would be OK. Let me configure the old-moved and new-moved to the same as context and see if I can live with just a single highlight color.

Thanks.
Jonathan Tan· Jun 13, 2017, 23:42 UTC · re: Stefan Beller · lore

Re: [RFC/PATCH] builtin/blame: darken redundant line information

On Mon, 12 Jun 2017 19:31:51 -0700 Stefan Beller <sbeller@google.com> wrote:

Show 10 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, timezone) are repeated. A reader may not be interested in those,
> so darken them. The darkening is not just based on hunk, but actually
> takes the previous lines content for that field to compare to.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
>  Example output (blame of blame): http://i.imgur.com/0Y12p2f.png

Looking at this image, how does blame decide what to dim? As it is, I see many identical timestamps (and also from the same commit) not being dimmed. (For example, see the very last line with "2013-01-05 ..." which is identical to the previous line, and I would expect that to be dimmed.)

Also, my preference is to have all-or-nothing dimming (dim the whole line up to and including the time zone if nothing has changed, and dim nothing otherwise) but I know that this is a subjective issue.

Stefan Beller· Jun 14, 2017, 00:33 UTC · re: Jonathan Tan · lore

Re: [RFC/PATCH] builtin/blame: darken redundant line information

On Tue, Jun 13, 2017 at 4:42 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
Show 18 quoted lines
> On Mon, 12 Jun 2017 19:31:51 -0700
> 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, timezone) are repeated. A reader may not be interested in those,
>> so darken them. The darkening is not just based on hunk, but actually
>> takes the previous lines content for that field to compare to.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>>  Example output (blame of blame): http://i.imgur.com/0Y12p2f.png
>
> Looking at this image, how does blame decide what to dim? As it is, I see many
> identical timestamps (and also from the same commit) not being dimmed.
> (For example, see the very last line with "2013-01-05 ..." which is
> identical to the previous line, and I would expect that to be dimmed.)
The date dimming is broken (it is implemented separately as a hack :/)
> Also, my preference is to have all-or-nothing dimming (dim the whole
> line up to and including the time zone if nothing has changed, and dim
> nothing otherwise) but I know that this is a subjective issue.
ok, noted.

That is what I had as a very first design (except for dimming, blanking out the lines with white spaces) and then went on "dimming even more".

I think this is also very similar to the discussion of boundary colors of the moved blocks in the neighboring thread, this is finding "blocks" (actually the finding part is easy as we are given the blocks) and then applying some "dim middle, highlight boundaries" algorithm, which in this particular case is a "highlight first line, dim the rest" for each field separately.

Originally I dreamed more a Zebra-style non-boundary coloring, the colors being temperature (https://en.wikipedia.org/wiki/Color_temperature) and the recency dictating the temperature, such that you can easily see which code is "hot" (i.e. modified recently)

Going by that I we could have different definitions of hotness:
* recency
* number of patches on a given line though that is hard to estimate
  when a line was changed vs newly-introduced
* number of different authors in a given function (= block of text with
  the same context hint in a virtual hunk header)
Stefan Beller· Jul 26, 2017, 23:04 UTC · re: Stefan Beller · lore

[PATCHv2] builtin/blame: highlight interesting things

When using git-blame lots of lines contain redundant information, for example in hunks that consist of multiple lines, the metadata (commit name, author) are repeated. A reader may not be interested in those, so darken (commit, author) information that is the same as in the previous line.

Choose a different approach for dates and imitate a 'temperature cool down'
for the dates. Compute the time range of all involved blamed commits
and then color
 * lines of old commits dark (aged 0-50% in that time range)
 * lines of medium age normal (50-80%)
 * lines of new age red (80-95%)
 * lines just introduced bright yellow (95-100%)
Signed-off-by: Stefan Beller <sbeller@google.com>
---
  I played around with it a bit more, using a different color scheme
  for dates, http://i.imgur.com/redhaLi.png
 builtin/blame.c | 140 ++++++++++++++++++++++++++++++++++++++++++++++----------
 color.h         |   1 +
 2 files changed, 118 insertions(+), 23 deletions(-)
Show changes to 2 files +118 −23

builtin/blame.c, color.h

diff --git a/builtin/blame.c b/builtin/blame.c
index bda1a78726..552ea8e6f7 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"
@@ -283,11 +284,14 @@ static void found_guilty_entry(struct blame_entry *ent, void *data)
 }
 
 static const char *format_time(timestamp_t time, const char *tz_str,
-			       int show_raw_time)
+			       int show_raw_time, const char *color,
+			       const char *reset)
 {
 	static struct strbuf time_buf = STRBUF_INIT;
 
 	strbuf_reset(&time_buf);
+	if (color)
+		strbuf_addstr(&time_buf, color);
 	if (show_raw_time) {
 		strbuf_addf(&time_buf, "%"PRItime" %s", time, tz_str);
 	}
@@ -307,6 +311,8 @@ static const char *format_time(timestamp_t time, const char *tz_str,
 		     time_width++)
 			strbuf_addch(&time_buf, ' ');
 	}
+	if (reset)
+		strbuf_addstr(&time_buf, reset);
 	return time_buf.buf;
 }
 
@@ -319,7 +325,8 @@ static const char *format_time(timestamp_t time, const char *tz_str,
 #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_SHOW_HIGHLIGHT	02000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -367,19 +374,62 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
 		putchar('\n');
 }
 
-static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int opt)
+static void emit_other(struct blame_scoreboard *sb,
+		       struct blame_entry *ent,
+		       struct blame_entry *prev,
+		       int opt,
+		       timestamp_t min_t,
+		       timestamp_t max_t)
 {
 	int cnt;
 	const char *cp;
 	struct blame_origin *suspect = ent->suspect;
-	struct commit_info ci;
+	struct commit_info ci, prev_ci;
 	char hex[GIT_MAX_HEXSZ + 1];
 	int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
+	int prev_same_field = 0;
+	const char *use_color, *reset_color = GIT_COLOR_RESET;
+	const char *date_color = NULL;
 
 	get_commit_info(suspect->commit, &ci, 1);
 	oid_to_hex_r(hex, &suspect->commit->object.oid);
 
 	cp = blame_nth_line(sb, ent->lno);
+
+	commit_info_init(&prev_ci);
+	if ((opt & OUTPUT_SHOW_HIGHLIGHT) && prev) {
+		get_commit_info(prev->suspect->commit, &prev_ci, 1);
+		if ((opt & OUTPUT_SHOW_SCORE) && ent->score == prev->score)
+			prev_same_field |= OUTPUT_SHOW_SCORE;
+		if ((opt & OUTPUT_SHOW_NAME) && prev->suspect && !strcmp(suspect->path, prev->suspect->path))
+			prev_same_field |= OUTPUT_SHOW_NAME;
+		if ((opt & OUTPUT_SHOW_NUMBER) &&
+		    ent->s_lno == prev->s_lno + prev->num_lines - 1)
+			prev_same_field |= OUTPUT_SHOW_NUMBER;
+		if (!(opt & OUTPUT_NO_AUTHOR)) {
+			if (((opt & OUTPUT_SHOW_EMAIL) &&
+			     !strcmp(ci.author_mail.buf, prev_ci.author_mail.buf)) ||
+			    !strcmp(ci.author.buf, prev_ci.author.buf))
+				prev_same_field |= OUTPUT_NO_AUTHOR;
+		}
+	}
+	if (opt & OUTPUT_SHOW_HIGHLIGHT) {
+		if (max_t == min_t) {
+			date_color = GIT_COLOR_NORMAL;
+		} else {
+			float score = 1.0 * (ci.author_time - min_t);
+			score /= (1.0 * (max_t - min_t));
+			if (score > 0.95)
+				date_color = GIT_COLOR_BOLD_YELLOW;
+			else if (score > 0.8)
+				date_color = GIT_COLOR_RED;
+			else if (score > 0.5)
+				date_color = GIT_COLOR_NORMAL;
+			else
+				date_color = GIT_COLOR_DARK;
+		}
+	}
+
 	for (cnt = 0; cnt < ent->num_lines; cnt++) {
 		char ch;
 		int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : abbrev;
@@ -392,8 +442,12 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 				putchar('^');
 			}
 		}
+		use_color = GIT_COLOR_NORMAL;
+		if ((opt & OUTPUT_SHOW_HIGHLIGHT) && cnt > 0)
+			use_color = GIT_COLOR_DARK;
+
+		printf("%s%.*s%s", use_color, length, hex, reset_color);
 
-		printf("%.*s", length, hex);
 		if (opt & OUTPUT_ANNOTATE_COMPAT) {
 			const char *name;
 			if (opt & OUTPUT_SHOW_EMAIL)
@@ -402,20 +456,36 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 				name = ci.author.buf;
 			printf("\t(%10s\t%10s\t%d)", name,
 			       format_time(ci.author_time, ci.author_tz.buf,
-					   show_raw_time),
+					   show_raw_time, NULL, NULL),
 			       ent->lno + 1 + cnt);
 		} else {
-			if (opt & OUTPUT_SHOW_SCORE)
-				printf(" %*d %02d",
+			if (opt & OUTPUT_SHOW_SCORE) {
+				use_color = GIT_COLOR_NORMAL;
+				if ((opt & OUTPUT_SHOW_HIGHLIGHT) &&
+				    (cnt > 0 || prev_same_field & OUTPUT_SHOW_SCORE))
+					use_color = GIT_COLOR_DARK;
+				printf(" %s%*d %02d%s", use_color,
 				       max_score_digits, ent->score,
-				       ent->suspect->refcnt);
-			if (opt & OUTPUT_SHOW_NAME)
-				printf(" %-*.*s", longest_file, longest_file,
-				       suspect->path);
-			if (opt & OUTPUT_SHOW_NUMBER)
-				printf(" %*d", max_orig_digits,
-				       ent->s_lno + 1 + cnt);
-
+				       ent->suspect->refcnt, reset_color);
+			}
+			if (opt & OUTPUT_SHOW_NAME) {
+				use_color = GIT_COLOR_NORMAL;
+				if ((opt & OUTPUT_SHOW_HIGHLIGHT) &&
+				    (cnt > 0 || prev_same_field & OUTPUT_SHOW_NAME))
+					use_color = GIT_COLOR_DARK;
+				printf(" %s%-*.*s%s", use_color, longest_file,
+						      longest_file,
+						      suspect->path,
+						      reset_color);
+			}
+			if (opt & OUTPUT_SHOW_NUMBER) {
+				use_color = GIT_COLOR_NORMAL;
+				if ((opt & OUTPUT_SHOW_HIGHLIGHT) &&
+				    (cnt > 0 || prev_same_field & OUTPUT_SHOW_NUMBER))
+					use_color = GIT_COLOR_DARK;
+				printf(" %s%*d%s", use_color, max_orig_digits,
+				       ent->s_lno + 1 + cnt, reset_color);
+			}
 			if (!(opt & OUTPUT_NO_AUTHOR)) {
 				const char *name;
 				int pad;
@@ -423,12 +493,21 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 					name = ci.author_mail.buf;
 				else
 					name = ci.author.buf;
+
+				use_color = GIT_COLOR_NORMAL;
+				if ((opt & OUTPUT_SHOW_HIGHLIGHT) &&
+				    (cnt > 0 || prev_same_field & OUTPUT_NO_AUTHOR))
+					use_color = GIT_COLOR_DARK;
+
 				pad = longest_author - utf8_strwidth(name);
-				printf(" (%s%*s %10s",
-				       name, pad, "",
-				       format_time(ci.author_time,
-						   ci.author_tz.buf,
-						   show_raw_time));
+				printf(" %s(%s%*s%s", use_color,
+						      name, pad, "",
+						      reset_color);
+				printf(" %10s", format_time(ci.author_time,
+							    ci.author_tz.buf,
+							    show_raw_time,
+							    date_color,
+							    reset_color));
 			}
 			printf(" %*d) ",
 			       max_digits, ent->lno + 1 + cnt);
@@ -448,7 +527,8 @@ 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;
+	timestamp_t min_t = TIME_MAX, max_t = 0;
 
 	if (option & OUTPUT_PORCELAIN) {
 		for (ent = sb->ent; ent; ent = ent->next) {
@@ -466,11 +546,24 @@ static void output(struct blame_scoreboard *sb, int option)
 		}
 	}
 
+	if (option & OUTPUT_SHOW_HIGHLIGHT) {
+		/* find oldest and youngest date for timestamp coloring */
+		struct commit_info ci;
+		for (ent = sb->ent; ent; ent = ent->next) {
+			get_commit_info(ent->suspect->commit, &ci, 1);
+			if (ci.author_time < min_t)
+				min_t = ci.author_time;
+			if (ci.author_time > max_t)
+				max_t = ci.author_time;
+		}
+	}
+
 	for (ent = sb->ent; ent; ent = ent->next) {
 		if (option & OUTPUT_PORCELAIN)
 			emit_porcelain(sb, ent, option);
 		else {
-			emit_other(sb, ent, option);
+			emit_other(sb, ent, prev, option, min_t, max_t);
+			prev = ent;
 		}
 	}
 }
@@ -681,6 +774,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('h', "highlights", &output_option, N_("darken redundancy from previous line; highlight dates (Default: off)"), OUTPUT_SHOW_HIGHLIGHT),
 
 		/*
 		 * The following two options are parsed by parse_revision_opt()
diff --git a/color.h b/color.h
index 90627650fc..bad2d7e29c 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"
-- 
2.14.0.rc0.3.g6c2e499285
Junio C Hamano· Jul 26, 2017, 23:29 UTC · re: Stefan Beller · lore

Re: [PATCHv2] builtin/blame: highlight interesting things

Stefan Beller <sbeller@google.com> writes:
Show 18 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) are repeated. A reader may not be interested in those, so darken
> (commit, author) information that is the same as in the previous line.
>
> Choose a different approach for dates and imitate a 'temperature cool down'
> for the dates. Compute the time range of all involved blamed commits
> and then color
>  * lines of old commits dark (aged 0-50% in that time range)
>  * lines of medium age normal (50-80%)
>  * lines of new age red (80-95%)
>  * lines just introduced bright yellow (95-100%)
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
>   I played around with it a bit more, using a different color scheme
>   for dates, http://i.imgur.com/redhaLi.png

I do agree with what this one tries to do, in that a block of lines tend to share the same metainfo as they come from the same commit and it is distracting to see them repeatedly---doing something to make their "these are in one group" nature stand out will give us a much better presentation.

But does this particular implementation work well for people who use black in on white background? "Darken to make it less distracting" may not work on both white-on-black and black-on-white users.

"Show the background only by replacing the letters with SP for metainfo that are same as previous line" would work for folks from either camp, I would imagine. And that should be a single feature, that can be enabled independently from the age based coloring.

The age coloring is much harder to make it work for folks from both camps at the same time with the same color selection. Yellow on white would be terribly unreadable for black-on-white folks, for example.

If you make "make it less distracting by blanking them out (not 'darken them')" feature without the age coloring, that can be usable immediately by folks from both camps, even if you cannot find a way to do the age coloring that would satisfy both groups. One group can just leave the knob off and not use the age coloring, while the other group can use it and people from both camps will be happier than the status quo.

Thanks.
Stefan Beller· Jul 26, 2017, 23:57 UTC · re: Junio C Hamano · lore

Re: [PATCHv2] builtin/blame: highlight interesting things

On Wed, Jul 26, 2017 at 4:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 26 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> When using git-blame lots of lines contain redundant information, for
>> example in hunks that consist of multiple lines, the metadata (commit name,
>> author) are repeated. A reader may not be interested in those, so darken
>> (commit, author) information that is the same as in the previous line.
>>
>> Choose a different approach for dates and imitate a 'temperature cool down'
>> for the dates. Compute the time range of all involved blamed commits
>> and then color
>>  * lines of old commits dark (aged 0-50% in that time range)
>>  * lines of medium age normal (50-80%)
>>  * lines of new age red (80-95%)
>>  * lines just introduced bright yellow (95-100%)
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>>   I played around with it a bit more, using a different color scheme
>>   for dates, http://i.imgur.com/redhaLi.png
>
> I do agree with what this one tries to do, in that a block of lines
> tend to share the same metainfo as they come from the same commit
> and it is distracting to see them repeatedly---doing something to
> make their "these are in one group" nature stand out will give us a
> much better presentation.

Well, we could also try a "zebra" as in sb/diff-color-move to show blocks with the same fancy border detection.

> But does this particular implementation work well for people who use
> black in on white background?  "Darken to make it less distracting"
> may not work on both white-on-black and black-on-white users.

correct. Once we have a shared understanding what the "interesting things" are and how to handle them, I would add color.blame.<slot> options to make it configurable.

> "Show the background only by replacing the letters with SP for
> metainfo that are same as previous line" would work for folks from
> either camp, I would imagine.  And that should be a single feature,
> that can be enabled independently from the age based coloring.

True, I had that as the very first step of this experiment, I lost the patch for it, but could redo it for presentation and discussion.

My impression was that this would remove _too_ much, e.g. if a commit spans more than one screen, you may not see the first line, but only blank space.

> The age coloring is much harder to make it work for folks from both
> camps at the same time with the same color selection.  Yellow on
> white would be terribly unreadable for black-on-white folks, for
> example.

Configuration is key here, I would think, both in the color space, as well as in the selection space. One could imagine that other people would rather have a defined time span, e.g. hard coding "2 weeks/one quarter/ more than a year" or relate that time span to the project history instead of the file history.

Show 7 quoted lines
> If you make "make it less distracting by blanking them out (not
> 'darken them')" feature without the age coloring, that can be usable
> immediately by folks from both camps, even if you cannot find a way
> to do the age coloring that would satisfy both groups.  One group
> can just leave the knob off and not use the age coloring, while the
> other group can use it and people from both camps will be happier
> than the status quo.
ok.

Thanks, Stefan

Junio C Hamano· Jul 27, 2017, 18:27 UTC · re: Stefan Beller · lore

Re: [PATCHv2] builtin/blame: highlight interesting things

Stefan Beller <sbeller@google.com> writes:
> Well, we could also try a "zebra"  as in sb/diff-color-move to show blocks
> with the same fancy border detection.

Luckily, blame output has places for metainfo on each line, unlike diff output, so there won't be a need for painting border differently. For example, if we decide that metainfo is shown only for the first line for each block, then by definition, the border between blocks is just before a line with metainfo and nowhere else. So in that sense, the problem should be a lot easier to solve ;-)

Unluckily, a block may span several pages, so "only at the first line" might not be very useful. I wonder if we can print in black ink on black background and let selection by mouse or in screen still reveal useful information?

Stefan Beller· Jul 27, 2017, 18:57 UTC · re: Junio C Hamano · lore

Re: [PATCHv2] builtin/blame: highlight interesting things

On Thu, Jul 27, 2017 at 11:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> Stefan Beller <sbeller@google.com> writes:
>
>> Well, we could also try a "zebra"  as in sb/diff-color-move to show blocks
>> with the same fancy border detection.
>
> Luckily, blame output has places for metainfo on each line, unlike
> diff output, so there won't be a need for painting border
> differently.
Right.
> For example, if we decide that metainfo is shown only
> for the first line for each block, then by definition, the border
> between blocks is just before a line with metainfo and nowhere else.
> So in that sense, the problem should be a lot easier to solve ;-)

Yes, also the code already provides structured blocks, so internally it is also easier.

The question is whether we decide that showing the first line is a good choice. We could also put it at the approximated middle and draw lines up and down.

In an advanced world, we would not use a dumb pager, but e.g. ncurses, such that the line displaying meta information may come along when you scroll down a page.

Maybe the discussion here can also feed back into the "machine readable move coloring" that Aevar seemed to be interested in.

Show 5 quoted lines
>
> Unluckily, a block may span several pages, so "only at the first
> line" might not be very useful.  I wonder if we can print in black
> ink on black background and let selection by mouse or in screen
> still reveal useful information?

This is what the dark color approximates, except you don't need a mouse selection to read?

← back to recent threads