Re: [PATCH 2/4] builtin/blame: dim uninteresting metadata
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Dec 28, 2017, 22:29 UTC
- Message-ID
- <CAPig+cTkxmiJZwLqYhKhvApZubJLTLufv2Yo6D5nLMLmrUc2cg@mail.gmail.com>
- In-Reply-To
- <20171228210345.205300-3-sbeller@google.com>
On Thu, Dec 28, 2017 at 4:03 PM, Stefan Beller <sbeller@google.com> wrote:
Show 31 quoted lines
> 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.
Show 5 quoted lines
> @@ -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?
Show 5 quoted lines
> @@ -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.
Show 7 quoted lines
> + 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..."?
Show 11 quoted lines
> 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"?
Show 7 quoted lines
> + 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 > +'