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

[Stefan Beller <sbeller@google.com>]

On Thu, Dec 28, 2017 at 2:29 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Dec 28, 2017 at 4:03 PM, 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, 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.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> diff --git a/builtin/blame.c b/builtin/blame.c
>> @@ -367,6 +370,28 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
>> +static inline void colors_unset(const char **use_color, const char **reset_color)
>> +{
>> +       *use_color = "";
>> +       *reset_color = "";
>> +}
>> +
>> +static inline void colors_set(const char **use_color, const char **reset_color)
>> +{
>> +       *use_color = repeated_meta_color;
>> +       *reset_color = GIT_COLOR_RESET;
>> +}
>> +
>> +static void setup_line_color(int opt, int cnt,
>> +                            const char **use_color,
>> +                            const char **reset_color)
>> +{
>> +       colors_unset(use_color, reset_color);
>> +
>> +       if ((opt & OUTPUT_COLOR_LINE) && cnt > 0)
>> +               colors_set(use_color, reset_color);
>> +}
>
> I'm not convinced that this colors_unset() / colors_set() /
> setup_line_color() abstraction is buying much. With this abstraction,
> I found the code more difficult to reason about than if the colors
> were just set/unset manually in the code which needs the colors. I
> *could* perhaps imagine setup_line_color() existing as a separate
> function since it is slightly more complex than the other two, but as
> it has only a single caller through all patches, even that may not be
> sufficient to warrant its existence.

Have you viewed this patch in context of the following patch?
Originally I was convinced an abstraction was not needed, but
as the next patch shows, a helper per field seems handy.

>
>> @@ -383,6 +408,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
>>         for (cnt = 0; cnt < ent->num_lines; cnt++) {
>>                 char ch;
>>                 int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : abbrev;
>> +               const char *col, *rcol;
>
> I can't help but read these as "column" and "[r]column"; the former,
> especially, is just too ingrained to interpret it any other way.
> Perhaps call these "color" and "reset" instead?

will fix.

>
>> @@ -607,6 +636,12 @@ static int git_blame_config(const char *var, const char *value, void *cb)
>> +       if (!strcmp(var, "color.blame.repeatedmeta")) {
>> +               if (color_parse_mem(value, strlen(value), repeated_meta_color))
>> +                       warning(_("ignore invalid color '%s' in color.blame.repeatedMeta"),
>> +                               value);
>
> Does this need to say "ignore"? If you drop that word, you still have
> a valid warning message.

dropped 'ignore'.

>
>> +               return 0;
>> +       }
>> @@ -681,6 +716,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(0, "color-lines", &output_option, N_("color redundant metadata from previous line"), OUTPUT_COLOR_LINE),
>
> Not sure what this help message means. Do you mean "color redundant
> ... _differently_ ..."? Or "_dim_ redundant..."?

Originally (in a patch set a couple months back) I had 'dim', but Junio
seems to have an interesting color setup, such that he would not call
it dimming IIRC, hence I think I wanted to say color _differently_. Fixed.

>> diff --git a/t/t8012-blame-colors.sh b/t/t8012-blame-colors.sh
>> @@ -0,0 +1,18 @@
>> +#!/bin/sh
>> +
>> +test_description='colored git blame'
>> +. ./test-lib.sh
>> +
>> +PROG='git blame -c'
>> +. "$TEST_DIRECTORY"/annotate-tests.sh
>> +
>> +test_expect_success 'colored blame colors continuous lines' '
>
> What are "continuous lines"? Did you mean "contiguous"?

Thanks for hinting at the subtle difference!

Thanks for the review!
(I will drop the abstraction and see how it goes)

Stefan