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

[Junio C Hamano]

Stefan Beller  writes:

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

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

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

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

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

Likewise.

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

[Stefan Beller]

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

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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..8128eee4f9 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1218,6 +1218,11 @@ color.status.::
status short-format), or
`unmerged` (files which have unmerged changes).
 
+color.blame.repeatedMeta::
+   Use the customized color for the part of git-blame output that
+   is repeated meta information per line (such as commit id,
+   author name, date and timezone). Defaults to cyan.
+
 color.ui::
This variable determines the default value for variables such
as `color.diff` and `color.grep` that control the use of color
diff --git a/builtin/blame.c b/builtin/blame.c
index db38c0b307..5190ff5df3 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -7,6 +7,7 @@
 
 #include "cache.h"
 #include "config.h"
+#include "color.h"
 #include "builtin.h"
 #include "commit.h"
 #include "diff.h"
@@ -46,6 +47,7 @@ static int xdl_opts;
 static int abbrev = -1;
 static int no_whole_file_rename;
 static int show_progress;
+static char repeated_meta_color[COLOR_MAXLEN];
 
 static struct date_mode blame_date_mode = { DATE_ISO8601 };
 static size_t blame_date_width;
@@ -316,10 +318,11 @@ static const char *format_time(timestamp_t time, const 
char *tz_str,
 #define OUTPUT_PORCELAIN   010
 #define OUTPUT_SHOW_NAME   020
 #define OUTPUT_SHOW_NUMBER 040
-#define OUTPUT_SHOW_SCORE  0100
-#define OUTPUT_NO_AUTHOR   0200
+#define OUTPUT_SHOW_SCORE  0100
+#define OUTPUT_NO_AUTHOR   0200
 #define OUTPUT_SHOW_EMAIL  0400
-#define OUTPUT_LINE_PORCELAIN 01000
+#define OUTPUT_LINE_PORCELAIN  01000
+#define OUTPUT_COLOR_LINE  02000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -375,6 +378,7 @@ static void emit_other(struct blame_scoreboard *sb, struct 
blame_entry *ent, int
struct commit_info ci;
char hex[GIT_MAX_HEXSZ + 1];
int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
+   const char *color = NULL, *reset = NULL;
 
get_commit_info(suspect->commit, , 1);
oid_to_hex_r(hex, >commit->object.oid);
@@ -384,6 +388,18 @@ static void emit_other(struct blame_scoreboard *sb, struct 
blame_entry *ent, int
char ch;
int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : 
abbrev;
 
+   if (opt & OUTPUT_COLOR_LINE) {
+   if (cnt > 0) {
+   color = repeated_meta_color;
+   reset = GIT_COLOR_RESET;
+   } else  {
+   color = "";
+   reset = "";
+   }
+   }
+   if (color)
+   fputs(color, stdout);
+
if (suspect->commit->object.flags & UNINTERESTING) {
if (blank_boundary)
memset(hex, ' ', length);
@@ -433,6 +449,8 @@ static void emit_other(struct blame_scoreboard *sb, struct 
blame_entry *ent, int
printf(" %*d) ",
   max_digits, ent->lno + 1 + cnt);
}
+   if (reset)
+   fputs(reset, stdout);
do {
ch = *cp++;
putchar(ch);
@@ -585,6 +603,8 @@ static const char *add_prefix(const char *prefix, const 
char *path)
 
 static int git_blame_config(const char *var, const char *value, void *cb)
 {
+   int *output_option = cb;
+
if (!strcmp(var, "blame.showroot")) {
show_root = git_config_bool(var, value);
return 0;
@@ -607,6 +627,13 @@ static int git_blame_config(const char *var, const char 
*value, void *cb)
parse_date_format(value, _date_mode);
return 0;
}
+   if (!strcmp(var, "color.blame.repeatedlines")) {
+   if (color_parse_mem(value, strlen(value), repeated_meta_color))
+   warning(_("invalid color '%s' in 
color.blame.repeatedLines"),
+   value);
+   *output_option |= OUTPUT_COLOR_LINE;
+   return 0;
+  

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

[Stefan Beller]

Hi Junio,

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

correct.

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

fixed

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

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

>> @@ -949,8 +979,12 @@ int cmd_blame(int argc, const char **argv, const char 
>> *prefix)
>>
>>   blame_coalesce();
>>
>> - if (!(output_option & OUTPUT_PORCELAIN))
>> + if (!(output_option & OUTPUT_PORCELAIN)) {
>>   find_alignment(, _option);
>> + if (!*repeated_meta_color &&
>> + (output_option & OUTPUT_COLOR_LINE))
>> + strcpy(repeated_meta_color, GIT_COLOR_DARK);
>> + }
>
> This code does not check OUTPUT_ANNOTATE_COMPAT because it assumes
> that OUTPUT_COLOR_LINE won't be in output_option when we are working
> in annotate compatibility mode.  The deeper codepaths we saw above
> should do the same.  It should be enough to drop color-line when
> anno-compat is set, or something like that, immediately after
> reading the config and overriding them from the command line.

Makes sense.

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

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

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

[Junio C Hamano]

Stefan Beller  writes:

> @@ -316,10 +318,11 @@ static const char *format_time(timestamp_t time, const 
> char *tz_str,
>  #define OUTPUT_PORCELAIN 010
>  #define OUTPUT_SHOW_NAME 020
>  #define OUTPUT_SHOW_NUMBER   040
> -#define OUTPUT_SHOW_SCORE  0100
> -#define OUTPUT_NO_AUTHOR   0200
> +#define OUTPUT_SHOW_SCORE0100
> +#define OUTPUT_NO_AUTHOR 0200

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

>  #define OUTPUT_SHOW_EMAIL0400
> -#define OUTPUT_LINE_PORCELAIN 01000
> +#define OUTPUT_LINE_PORCELAIN01000

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

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

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

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

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

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

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

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

Likewise.

> @@ -949,8 +979,12 @@ int cmd_blame(int argc, const char **argv, const char 
> *prefix)
>  
>   blame_coalesce();
>  
> - if (!(output_option & OUTPUT_PORCELAIN))
> + if (!(output_option & OUTPUT_PORCELAIN)) {
>   find_alignment(, _option);
> + if (!*repeated_meta_color &&
> + (output_option & OUTPUT_COLOR_LINE))
> + strcpy(repeated_meta_color, GIT_COLOR_DARK);
> + }

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

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

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

Traditionally, we use CYAN for lines that are less interesting than
others (e.g. hunk header), so reusing it might make the life easier
to the users, especially because I envision that we may want to
introduce another "logical" level to give another redirection
between the configuration keys like color.diff.frag and
color.blame.repeatedlines and the actual ANSI sequence like
"\033[36m".  I.e. we update the system so that these two
configuration keys take "uninteresting" (which is one of the
"logical" colors) by default, and 

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

[Stefan Beller]

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

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

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..a223232263 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1218,6 +1218,11 @@ color.status.::
status short-format), or
`unmerged` (files which have unmerged changes).
 
+color.blame.repeatedMeta::
+   Use the customized color for the part of git-blame output that
+   is repeated meta information per line (such as commit id,
+   author name, date and timezone). Defaults to dark gray.
+
 color.ui::
This variable determines the default value for variables such
as `color.diff` and `color.grep` that control the use of color
diff --git a/builtin/blame.c b/builtin/blame.c
index db38c0b307..b49fee70a9 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -7,6 +7,7 @@
 
 #include "cache.h"
 #include "config.h"
+#include "color.h"
 #include "builtin.h"
 #include "commit.h"
 #include "diff.h"
@@ -46,6 +47,7 @@ static int xdl_opts;
 static int abbrev = -1;
 static int no_whole_file_rename;
 static int show_progress;
+static char repeated_meta_color[COLOR_MAXLEN];
 
 static struct date_mode blame_date_mode = { DATE_ISO8601 };
 static size_t blame_date_width;
@@ -316,10 +318,11 @@ static const char *format_time(timestamp_t time, const 
char *tz_str,
 #define OUTPUT_PORCELAIN   010
 #define OUTPUT_SHOW_NAME   020
 #define OUTPUT_SHOW_NUMBER 040
-#define OUTPUT_SHOW_SCORE  0100
-#define OUTPUT_NO_AUTHOR   0200
+#define OUTPUT_SHOW_SCORE  0100
+#define OUTPUT_NO_AUTHOR   0200
 #define OUTPUT_SHOW_EMAIL  0400
-#define OUTPUT_LINE_PORCELAIN 01000
+#define OUTPUT_LINE_PORCELAIN  01000
+#define OUTPUT_COLOR_LINE  02000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -375,6 +378,7 @@ static void emit_other(struct blame_scoreboard *sb, struct 
blame_entry *ent, int
struct commit_info ci;
char hex[GIT_MAX_HEXSZ + 1];
int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
+   const char *color = "", *reset = "";
 
get_commit_info(suspect->commit, , 1);
oid_to_hex_r(hex, >commit->object.oid);
@@ -384,6 +388,19 @@ static void emit_other(struct blame_scoreboard *sb, struct 
blame_entry *ent, int
char ch;
int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : 
abbrev;
 
+   if (!(opt & OUTPUT_ANNOTATE_COMPAT)) {
+   if (opt & OUTPUT_COLOR_LINE) {
+   if (cnt > 0) {
+   color = repeated_meta_color;
+   reset = GIT_COLOR_RESET;
+   } else  {
+   color ="";
+   reset = "";
+   }
+   }
+   fputs(color, stdout);
+   }
+
if (suspect->commit->object.flags & UNINTERESTING) {
if (blank_boundary)
memset(hex, ' ', length);
@@ -433,6 +450,9 @@ static void emit_other(struct blame_scoreboard *sb, struct 
blame_entry *ent, int
printf(" %*d) ",
   max_digits, ent->lno + 1 + cnt);
}
+   if (!(opt & OUTPUT_ANNOTATE_COMPAT) &&
+   (opt & OUTPUT_COLOR_LINE))
+   fputs(reset, stdout);
do {
ch = *cp++;
putchar(ch);
@@ -585,6 +605,8 @@ static const char *add_prefix(const char *prefix, const 
char *path)
 
 static int git_blame_config(const char *var, const char *value, void *cb)
 {
+   int *output_option = cb;
+
if (!strcmp(var, "blame.showroot")) {
show_root = git_config_bool(var, value);
return 0;
@@ -607,6 +629,13 @@ static int git_blame_config(const char *var, const char 
*value, void *cb)
parse_date_format(value, _date_mode);
return 0;
}
+   if (!strcmp(var, "color.blame.repeatedlines")) {
+   if (color_parse_mem(value, strlen(value),