Re: [PATCH 1/2] builtin/blame: dim uninteresting metadata lines
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 17, 2018, 03:17 UTC
- Message-ID
- <xmqqk1t6v7w4.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20180416220955.46163-2-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 8 quoted lines
> @@ -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.
Show 23 quoted lines
> +#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'.
Show 5 quoted lines
> + 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.
Show 7 quoted lines
> @@ -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.
Show 11 quoted lines
> @@ -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.
Show 12 quoted lines
> 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.