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

[Eric Sunshine <sunshine@sunshineco.com>]

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.

> @@ -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?

> @@ -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.

> +               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..."?

> 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"?

> +       git blame --abbrev=12 --color-lines hello.c >actual.raw &&
> +       test_decode_color <actual.raw >actual &&
> +       grep "<BOLD;BLACK>(F" actual > F.expect &&
> +       grep "<BOLD;BLACK>(H" actual > H.expect &&
> +       test_line_count = 2 F.expect &&
> +       test_line_count = 3 H.expect
> +'