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

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

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

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

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

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

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

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

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

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

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

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

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

Likewise.

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

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

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

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

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