Re: [PATCHv3 2/4] builtin/blame: dim uninteresting metadata

From: Stefan Beller <sbeller@google.com>
Date: Feb 8, 2018, 21:19 UTC
Message-ID: <CAGZ79kZKaKuvF2mp2r7Vt_3r73m7BmFqc_VPctKezR7gXQTx_A@mail.gmail.com>
In-Reply-To: <xmqqk1ws87qw.fsf@gitster.mtv.corp.google.com>

On Mon, Jan 8, 2018 at 11:34 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 27 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> +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.
>> +
>> ...
>
> "Dark gray on default background" may alleviate worrries from those
> who prefer black ink on white paper display by hinting that both
> foreground and background colors can be configured.
>
> Do we want to make this overridable from the command line,
> i.e. --color-repeated-meta=gray?
>
>> +#define OUTPUT_COLOR_LINE    02000
>
> The name of the macro implies that this is (or at least can be) a
> lot more generic UI request than merely "paint the same metadata on
> adjacent lines in a different color".
>
>> +             OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line differently"), OUTPUT_COLOR_LINE),
>
> Should this eventually become "--color=<yes,no,auto>" that is more
> usual and generic, possibly defaulting to "auto" in the future, I
> wonder?

This sounds like a good change to make, though what does "yes" mean here? The following patches introduce other modes of colorization, such that --color=<no, auto, lines, fields, decayed> would make sense to me, with "lines" as the mode of this patch, "fields" implemented in the next patch and "decayed" having the meaning of the heating algorithm presented in the last patch.

However each of these (except no and auto) would want to have extra parameters, which as mentioned above could go into its own separate parameters, such that

  git blame --color=line --color-repeated-meta=cyan

would seem like a good UI. This might look like it could be a good idea to have --color-repeated-meta imply --color=<line,auto> if no --color is given. But as different coloring modes will have different arguments to provide extra color information, I wonder what we would do with

  git blame --color="mode1" --extra-color-arg-for-mode-2

So I don't think the idea of having separate options would be a good idea as we'd have to think about the implications as mentioned above. So maybe

  git blame "--color=lines,cyan yellow"

would work, if you want "cyan yellow" as the color configuration in the "lines" mode? (bad choice of a background color btw)

Show 17 quoted lines

>> diff --git a/color.h b/color.h
>> index 2e768a10c6..2df2f86698 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"
>
> How about using CYAN just like "diff" output uses it to paint the
> least interesting lines?  That way we will keep the "uninteresting
> is cyan" consistency for the default settings without adding a new
> color to the palette.

sounds good, I'll take that suggestion once we reach consensus on the UI for the user.

Thanks, Stefan