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

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

[Stefan Beller]

When using git-blame lots of lines contain redundant information, for
example in hunks that consist of multiple lines, the metadata (commit name,
author, 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(-)

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

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

[Junio C Hamano]

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.  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?

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

[Stefan Beller]

On Tue, Jun 13, 2017 at 8:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

> 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

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

[Junio C Hamano]

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.

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

[Stefan Beller]

On Tue, Jun 13, 2017 at 10:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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:

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

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

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

[Junio C Hamano]

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.

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?

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

[Stefan Beller]

On Tue, Jun 13, 2017 at 10:19 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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:

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

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

[Junio C Hamano]

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?

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

[Stefan Beller]

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.

So ideally we could tell the user this

>          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

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

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

[Stefan Beller]

On Tue, Jun 13, 2017 at 10:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

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

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

[Jonathan Tan]

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.)

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.

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

[Stefan Beller]

On Tue, Jun 13, 2017 at 4:42 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
> 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)

[PATCHv2] builtin/blame: highlight interesting things

[Stefan Beller]

When using git-blame lots of lines contain redundant information, for
example in hunks that consist of multiple lines, the metadata (commit name,
author) 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(-)

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

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

[Junio C Hamano]

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.

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.

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

[Stefan Beller]

On Wed, Jul 26, 2017 at 4:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

> 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

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

[Junio C Hamano]

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?

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

[Stefan Beller]

On Thu, Jul 27, 2017 at 11:27 AM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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