blame: (dim rep. metadata lines or fields, decay date coloring)

Add documentation explaining the functions in color.h. While at it, mark them extern.

Signed-off-by Stefan Beller <[email protected]>

---

color.h +2 -0

+ */
+extern int color_is_nil(const char *color);

Quoted 12 lines · Stefan Beller

Add documentation explaining the functions in color.h. While at it, mark them extern.

Signed-off-by Stefan Beller <[email protected]>

---

color.h +2 -2

@@ -72,26 +72,48 @@ extern int color_stdout_is_tty;
 /*
- * Set the color buffer (which must be COLOR_MAXLEN bytes)
- * to the raw color bytes; this is useful for initializing
+ * Set the color buffer `dst` (which must be COLOR_MAXLEN bytes)
+ * to the raw color bytes `color_bytes`; this is useful for initializing

color.h extern int color_stdout_is_tty; · line 83

Quoted 4 lines

color.h +1 -1

  * default color variables.
  */
-void color_set(char *dst, const char *color_bytes);
+extern void color_set(char *dst, const char *color_bytes);

I don't see an explanation of what "color bytes" are. From where does one obtain such bytes? How is this function used? The function comment does not particularly answer these questions.

color.h extern int color_stdout_is_tty; · line 89

Quoted 6 lines

color.h +5 -0

+/*
+ * Parses a config option, which can be a boolean or one of
+ * "never", "auto", "always". Returns the constant for the given setting.
+ */
+extern int git_config_colorbool(const char *var, const char *value);

I suppose that "constant for the given setting" means one of GIT_COLOR_NEVER , GIT_COLOR_AUTO, GIT_COLOR_ALWAYS? Perhaps say so explicitly?

Would it also make sense to say that boolean "true" ("yes", etc.) results in GIT_COLOR_ALWAYS and "false" ("no", etc.)" results in GIT_COLOR_NEVER?

Finally, for grammatical consistency with other comments:
s/Parses/Parse
s/Returns/Return/

color.h extern int color_stdout_is_tty; · line 92

Quoted 3 lines

color.h +2 -0

+/* Is the output supposed to be colored? Resolve and cache the 'auto' setting */
+extern int want_color(int var);

What is the 'var' argument? How is it interpreted? (...goes and checks implementation...) I guess this documentation should explain that the caller would pass in the result of git_config_colorbool().

Also, the meaning of "Resolve and cache 'auto' setting" stumped me for a while since it's not clear why it's here (or why it's missing the full stop), but I eventually realized that it's describing an implementation detail, which probably doesn't belong in API documentation.

color.h extern int color_stdout_is_tty; · line 99

Quoted 7 lines

color.h +6 -0

+/*
+ * Translate the human readable color string from `value` and into
+ * terminal color codes and store them in `dst`
+ */
+extern int color_parse(const char *value, char *dst);
+extern int color_parse_mem(const char *value, int len, char *dst);

What does "human readable" mean in this context? Is it talking about color names or RGB(A) tuples or what?

Also, how does the caller know how large to make 'dst'? At minimum, you should say something about COLOR_MAXLEN.

Finally, for the 'len' case, what happens if 'dst' is too small? This should be documented.

And, the return value of these functions should be discussed.

color.h extern int color_stdout_is_tty; · line 103

Quoted 3 lines

color.h +3 -0

+/*
+ * Print the format string `fmt`, encapsulated by setting and resetting the
+ * color. Omits the color encapsulation if `color` is NULL.

The "encapsulated by setting and resetting the color" bit is hard to grok. Perhaps instead say something along the lines of:

Output the formatted string in the specified color (and then reset to normal color so subsequent output is uncolored).

color.h extern int color_stdout_is_tty; · line 105

Quoted 3 lines

color.h +2 -0

+ * The `color_fprintf_ln` prints a new line after resetting the color.
+ * The `color_print_strbuf` prints the given pre-formatted strbuf instead.

Should the strbuf variation warn that it only outputs content up to the first embedded NUL? (Or should that bug/misfeature be fixed?)

Quoted 8 lines

color.h +4 -0

+ */
 __attribute__((format (printf, 3, 4)))
+extern int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
+extern int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
+extern void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
 

color.h extern int color_stdout_is_tty; · line 115

Quoted 4 lines

color.h +3 -1

-int color_is_nil(const char *color);
+/*
+ * Check if the given color is GIT_COLOR_NIL that means "no color selected".
+ * The application needs to replace the color with the actual desired color.

Maybe: s/application/caller/

Quoted 3 lines

color.h +2 -0

+ */
+extern int color_is_nil(const char *color);

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 <[email protected]>

---

builtin/blame.c +11 -0

@@ -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;
+}

t/t8012-blame-colors.sh +7 -0

+       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
+'

Quoted 21 lines · Stefan Beller

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 <[email protected]>

---

builtin/blame.c +11 -0

@@ -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;
+}

builtin/blame.c static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent, · line 390

Quoted 10 lines

builtin/blame.c +10 -0

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

builtin/blame.c static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int · line 411

Quoted 6 lines

builtin/blame.c +1 -0

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

builtin/blame.c static int git_blame_config(const char *var, const char *value, void *cb) · line 639

Quoted 6 lines

builtin/blame.c +4 -0

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

builtin/blame.c int cmd_blame(int argc, const char **argv, const char *prefix) · line 719

Quoted 8 lines

builtin/blame.c +3 -0

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

Quoted 5 lines

t/t8012-blame-colors.sh +3 -0

@@ -0,0 +1,18 @@
+#!/bin/sh
+
+test_description='colored git blame'

t/t8012-blame-colors.sh line 9

Quoted 6 lines

t/t8012-blame-colors.sh +6 -0

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

Quoted 13 lines · Eric Sunshine

builtin/blame.c static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent, · line 383

Quoted 11 lines · Stefan Beller

builtin/blame.c +11 -0

+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;
+}

Quoted 8 lines

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.

Quoted 23 lines · Stefan Beller

Quoted 13 lines · Eric Sunshine

builtin/blame.c static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent, · line 383

Quoted 11 lines · Stefan Beller

builtin/blame.c +11 -0

+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;
+}

Quoted 8 lines

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.

Quoted 3 lines

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.

I did take the other patch into consideration when making the observation, and I still found the code more difficult to reason about than if these minor bits of code had merely been inlined into the callers. I neglected to mention previously that part of the problem may be that these function names do not do a good job of conveying what is being done, thus I repeatedly had to consult the function implementations while reading callers in order to understand what was going on.

Choose a different color for dates and imitate a 'temperature cool down' for the dates.