* [PATCH 3/4] builtin/blame: add option to color metadata fields separately
2017-11-10 1:09 [RFC PATCH 0/4] blame: (dim rep. metadata lines or fields, decay date coloring) Stefan Beller
@ 2017-11-10 1:10 ` Stefan Beller
0 siblings, 0 replies; 13+ messages in thread
From: Stefan Beller @ 2017-11-10 1:10 UTC (permalink / raw)
To: git; +Cc: Stefan Beller
Unlike the previous commit, this dims colors for each
metadata field individually.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/blame.c | 82 +++++++++++++++++++++++++++++++++++++++++++------
t/t8012-blame-colors.sh | 38 +++++++++++++++++++++++
2 files changed, 111 insertions(+), 9 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index d15e901357..9d460597a2 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -323,6 +323,7 @@ static const char *format_time(timestamp_t time, const char *tz_str,
#define OUTPUT_SHOW_EMAIL 0400
#define OUTPUT_LINE_PORCELAIN 01000
#define OUTPUT_COLOR_LINE 02000
+#define OUTPUT_COLOR_FIELDS 04000
static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
{
@@ -370,6 +371,33 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
putchar('\n');
}
+static int had_same_field_previously(int opt, int field,
+ struct blame_entry *ent,
+ struct blame_entry *prev)
+{
+ struct commit_info ci, prev_ci;
+
+ switch (field) {
+ case OUTPUT_SHOW_SCORE:
+ return ent->score == prev->score;
+ case OUTPUT_SHOW_NAME:
+ return prev->suspect &&
+ !strcmp(ent->suspect->path, prev->suspect->path);
+ case OUTPUT_SHOW_NUMBER:
+ return ent->s_lno == prev->s_lno + prev->num_lines - 1;
+
+ case OUTPUT_NO_AUTHOR:
+ get_commit_info(ent->suspect->commit, &ci, 1);
+ get_commit_info(prev->suspect->commit, &prev_ci, 1);
+ return ((opt & OUTPUT_SHOW_EMAIL) &&
+ !strcmp(ci.author_mail.buf, prev_ci.author_mail.buf)) ||
+ !strcmp(ci.author.buf, prev_ci.author.buf);
+ default:
+ BUG("unknown field");
+ }
+ return 0;
+}
+
static inline void colors_unset(const char **use_color, const char **reset_color)
{
*use_color = "";
@@ -382,17 +410,36 @@ static inline void colors_set(const char **use_color, const char **reset_color)
*reset_color = GIT_COLOR_RESET;
}
+static void setup_field_color(int opt, int cnt, int field,
+ struct blame_entry *ent,
+ struct blame_entry *prev,
+ const char **use_color,
+ const char **reset_color)
+{
+ if (!(opt & OUTPUT_COLOR_FIELDS))
+ return;
+
+ if ((cnt > 0 ||
+ (prev && had_same_field_previously(opt, field, ent, prev))))
+ colors_set(use_color, reset_color);
+ else
+ colors_unset(use_color, reset_color);
+}
+
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)
+ if ((opt & (OUTPUT_COLOR_LINE | OUTPUT_COLOR_FIELDS)) && cnt > 0)
colors_set(use_color, reset_color);
}
-static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int opt)
+static void emit_other(struct blame_scoreboard *sb,
+ struct blame_entry *ent,
+ struct blame_entry *prev,
+ int opt)
{
int cnt;
const char *cp;
@@ -432,18 +479,27 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
show_raw_time),
ent->lno + 1 + cnt);
} else {
- if (opt & OUTPUT_SHOW_SCORE)
+ if (opt & OUTPUT_SHOW_SCORE) {
+ setup_field_color(opt, cnt, OUTPUT_SHOW_SCORE,
+ ent, prev, &col, &rcol);
printf(" %s%*d %02d%s", col,
max_score_digits, ent->score,
ent->suspect->refcnt, rcol);
- if (opt & OUTPUT_SHOW_NAME)
+ }
+ if (opt & OUTPUT_SHOW_NAME) {
+ setup_field_color(opt, cnt, OUTPUT_SHOW_NAME,
+ ent, prev, &col, &rcol);
printf(" %s%-*.*s%s", col, longest_file,
longest_file,
suspect->path,
rcol);
- if (opt & OUTPUT_SHOW_NUMBER)
+ }
+ if (opt & OUTPUT_SHOW_NUMBER) {
+ setup_field_color(opt, cnt, OUTPUT_SHOW_NUMBER,
+ ent, prev, &col, &rcol);
printf(" %s%*d%s", col, max_orig_digits,
ent->s_lno + 1 + cnt, rcol);
+ }
if (!(opt & OUTPUT_NO_AUTHOR)) {
const char *name;
int pad;
@@ -452,6 +508,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
else
name = ci.author.buf;
pad = longest_author - utf8_strwidth(name);
+ setup_field_color(opt, cnt, OUTPUT_NO_AUTHOR,
+ ent, prev, &col, &rcol);
printf(" %s(%s%*s %10s%s", col,
name, pad, "",
format_time(ci.author_time,
@@ -477,7 +535,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
static void output(struct blame_scoreboard *sb, int option)
{
- struct blame_entry *ent;
+ struct blame_entry *ent, *prev = NULL;
if (option & OUTPUT_PORCELAIN) {
for (ent = sb->ent; ent; ent = ent->next) {
@@ -499,7 +557,8 @@ static void output(struct blame_scoreboard *sb, int option)
if (option & OUTPUT_PORCELAIN)
emit_porcelain(sb, ent, option);
else {
- emit_other(sb, ent, option);
+ emit_other(sb, ent, prev, option);
+ prev = ent;
}
}
}
@@ -717,6 +776,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
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),
+ OPT_BIT(0, "color-fields", &output_option, N_("color redundant metadata fields from previous line"), OUTPUT_COLOR_FIELDS),
/*
* The following two options are parsed by parse_revision_opt()
@@ -775,6 +835,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
DIFF_OPT_CLR(&revs.diffopt, FOLLOW_RENAMES);
argc = parse_options_end(&ctx);
+ if ((output_option & OUTPUT_COLOR_LINE) &&
+ (output_option & OUTPUT_COLOR_FIELDS))
+ die(_("cannot ask for colored lines and fields at the same time"));
+
if (incremental || (output_option & OUTPUT_PORCELAIN)) {
if (show_progress > 0)
die(_("--progress can't be used with --incremental or porcelain formats"));
@@ -978,8 +1042,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
if (!(output_option & OUTPUT_PORCELAIN)) {
find_alignment(&sb, &output_option);
- if ((output_option & OUTPUT_COLOR_LINE) &&
- !repeated_meta_color)
+ if (!repeated_meta_color &&
+ (output_option & (OUTPUT_COLOR_LINE | OUTPUT_COLOR_FIELDS)))
repeated_meta_color = GIT_COLOR_DARK;
}
diff --git a/t/t8012-blame-colors.sh b/t/t8012-blame-colors.sh
index b2de03b777..be491bde9a 100755
--- a/t/t8012-blame-colors.sh
+++ b/t/t8012-blame-colors.sh
@@ -15,4 +15,42 @@ test_expect_success 'colored blame colors continuous lines' '
test_line_count = 3 H.expect
'
+test_expect_success 'colored blame colors continuous fields' '
+
+ git mv hello.c world.c &&
+ git commit -a -m "moved file" &&
+ cat <<-EOF >> world.c &&
+ void world()
+ {
+ puts("world");
+ }
+ EOF
+ git add world.c &&
+ GIT_AUTHOR_NAME="F" GIT_AUTHOR_EMAIL="F@test.git" \
+ git commit -m "forgot to add changes to moved file" &&
+
+ git blame --abbrev=12 --color-fields world.c >actual.raw &&
+ test_decode_color <actual.raw >actual &&
+
+ grep "<BOLD;BLACK>hello.c" actual > colored_hello.expect &&
+ grep "hello.c" actual > all_hello.expect &&
+ test_line_count = 9 colored_hello.expect &&
+ test_line_count = 10 all_hello.expect &&
+
+ grep "<BOLD;BLACK>world.c" actual > colored_world.expect &&
+ grep "world.c" actual > all_world.expect &&
+ test_line_count = 3 colored_world.expect &&
+ test_line_count = 4 all_world.expect &&
+
+ grep "(F" actual > all_F.expect &&
+ grep "<BOLD;BLACK>(F" actual > colored_F.expect &&
+ test_line_count = 8 all_F.expect &&
+ test_line_count = 5 colored_F.expect &&
+
+ grep "(H" actual > all_H.expect &&
+ grep "<BOLD;BLACK>(H" actual > colored_H.expect &&
+ test_line_count = 5 all_H.expect &&
+ test_line_count = 3 colored_H.expect
+'
+
test_done
--
2.15.0.128.gcadd42da22
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCHv2 0/4] blame: (dim rep. metadata lines or fields, decay date coloring)
@ 2017-12-28 21:03 Stefan Beller
2017-12-28 21:03 ` [PATCH 1/4] color.h: document and modernize header Stefan Beller
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Stefan Beller @ 2017-12-28 21:03 UTC (permalink / raw)
To: git; +Cc: Stefan Beller
This is picking up [1], but presenting it in another approach,
as I realized these are orthogonal features:
* dimming repeated lines/fields of information
* giving a quick visual information how old (as a proxy for 'well tested')
a line of code is.
Both features are configurable.
Changes from sending it out in November:
* better commit messages
* rebased on master
Any feedback welcome.
Thanks,
Stefan
[1] https://public-inbox.org/git/20171110011002.10179-1-sbeller@google.com/
Stefan Beller (4):
color.h: document and modernize header
builtin/blame: dim uninteresting metadata
builtin/blame: add option to color metadata fields separately
builtin/blame: highlight recently changed lines
Documentation/config.txt | 23 +++++
builtin/blame.c | 216 ++++++++++++++++++++++++++++++++++++++++++-----
color.c | 2 -
color.h | 49 ++++++++---
t/t8012-blame-colors.sh | 56 ++++++++++++
5 files changed, 311 insertions(+), 35 deletions(-)
create mode 100755 t/t8012-blame-colors.sh
--
2.15.1.620.gb9897f4670-goog
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/4] color.h: document and modernize header
2017-12-28 21:03 [PATCHv2 0/4] blame: (dim rep. metadata lines or fields, decay date coloring) Stefan Beller
@ 2017-12-28 21:03 ` Stefan Beller
2017-12-28 22:00 ` Eric Sunshine
2017-12-28 21:03 ` [PATCH 2/4] builtin/blame: dim uninteresting metadata Stefan Beller
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Stefan Beller @ 2017-12-28 21:03 UTC (permalink / raw)
To: git; +Cc: Stefan Beller
Add documentation explaining the functions in color.h.
While at it, mark them extern.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
color.c | 2 --
color.h | 48 +++++++++++++++++++++++++++++++++++-------------
2 files changed, 35 insertions(+), 15 deletions(-)
diff --git a/color.c b/color.c
index d48dd947c9..c4dc1cb989 100644
--- a/color.c
+++ b/color.c
@@ -399,8 +399,6 @@ static int color_vfprintf(FILE *fp, const char *color, const char *fmt,
return r;
}
-
-
int color_fprintf(FILE *fp, const char *color, const char *fmt, ...)
{
va_list args;
diff --git a/color.h b/color.h
index fd2b688dfb..6cd632c0d8 100644
--- a/color.h
+++ b/color.h
@@ -72,26 +72,48 @@ extern int color_stdout_is_tty;
* Use the first one if you need only color config; the second is a convenience
* if you are just going to change to git_default_config, too.
*/
-int git_color_config(const char *var, const char *value, void *cb);
-int git_color_default_config(const char *var, const char *value, void *cb);
+extern int git_color_config(const char *var, const char *value, void *cb);
+extern int git_color_default_config(const char *var, const char *value, void *cb);
/*
- * 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
* default color variables.
*/
-void color_set(char *dst, const char *color_bytes);
+extern void color_set(char *dst, const char *color_bytes);
-int git_config_colorbool(const char *var, const char *value);
-int want_color(int var);
-int color_parse(const char *value, char *dst);
-int color_parse_mem(const char *value, int len, char *dst);
+/*
+ * 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);
+
+/* Is the output supposed to be colored? Resolve and cache the 'auto' setting */
+extern int want_color(int var);
+
+/*
+ * 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);
+
+/*
+ * Print the format string `fmt`, encapsulated by setting and resetting the
+ * color. Omits the color encapsulation if `color` is NULL.
+ * The `color_fprintf_ln` prints a new line after resetting the color.
+ * The `color_print_strbuf` prints the given pre-formatted strbuf instead.
+ */
__attribute__((format (printf, 3, 4)))
-int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
+extern int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
__attribute__((format (printf, 3, 4)))
-int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
-void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
+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);
-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.
+ */
+extern int color_is_nil(const char *color);
#endif /* COLOR_H */
--
2.15.1.620.gb9897f4670-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/4] builtin/blame: dim uninteresting metadata
2017-12-28 21:03 [PATCHv2 0/4] blame: (dim rep. metadata lines or fields, decay date coloring) Stefan Beller
2017-12-28 21:03 ` [PATCH 1/4] color.h: document and modernize header Stefan Beller
@ 2017-12-28 21:03 ` Stefan Beller
2017-12-28 22:29 ` Eric Sunshine
2017-12-28 21:03 ` [PATCH 3/4] builtin/blame: add option to color metadata fields separately Stefan Beller
2017-12-28 21:03 ` [PATCH 4/4] builtin/blame: highlight recently changed lines Stefan Beller
3 siblings, 1 reply; 13+ messages in thread
From: Stefan Beller @ 2017-12-28 21:03 UTC (permalink / raw)
To: git; +Cc: 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 <sbeller@google.com>
---
Documentation/config.txt | 5 ++++
builtin/blame.c | 68 ++++++++++++++++++++++++++++++++++++++----------
color.h | 1 +
t/t8012-blame-colors.sh | 18 +++++++++++++
4 files changed, 78 insertions(+), 14 deletions(-)
create mode 100755 t/t8012-blame-colors.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index b18c0f97fe..2c211b6e7d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1216,6 +1216,11 @@ color.status.<slot>::
status short-format), or
`unmerged` (files which have unmerged changes).
+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.
+
color.ui::
This variable determines the default value for variables such
as `color.diff` and `color.grep` that control the use of color
diff --git a/builtin/blame.c b/builtin/blame.c
index 005f55aaa2..09afb4d3ea 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -7,6 +7,7 @@
#include "cache.h"
#include "config.h"
+#include "color.h"
#include "builtin.h"
#include "commit.h"
#include "diff.h"
@@ -46,6 +47,7 @@ static int xdl_opts;
static int abbrev = -1;
static int no_whole_file_rename;
static int show_progress;
+static char *repeated_meta_color;
static struct date_mode blame_date_mode = { DATE_ISO8601 };
static size_t blame_date_width;
@@ -316,10 +318,11 @@ static const char *format_time(timestamp_t time, const char *tz_str,
#define OUTPUT_PORCELAIN 010
#define OUTPUT_SHOW_NAME 020
#define OUTPUT_SHOW_NUMBER 040
-#define OUTPUT_SHOW_SCORE 0100
-#define OUTPUT_NO_AUTHOR 0200
+#define OUTPUT_SHOW_SCORE 0100
+#define OUTPUT_NO_AUTHOR 0200
#define OUTPUT_SHOW_EMAIL 0400
-#define OUTPUT_LINE_PORCELAIN 01000
+#define OUTPUT_LINE_PORCELAIN 01000
+#define OUTPUT_COLOR_LINE 02000
static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
{
@@ -367,6 +370,28 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
putchar('\n');
}
+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);
+}
+
static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int opt)
{
int cnt;
@@ -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;
if (suspect->commit->object.flags & UNINTERESTING) {
if (blank_boundary)
@@ -393,7 +419,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
}
}
- printf("%.*s", length, hex);
+ setup_line_color(opt, cnt, &col, &rcol);
+ printf("%s%.*s%s", col, length, hex, rcol);
if (opt & OUTPUT_ANNOTATE_COMPAT) {
const char *name;
if (opt & OUTPUT_SHOW_EMAIL)
@@ -406,16 +433,17 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
ent->lno + 1 + cnt);
} else {
if (opt & OUTPUT_SHOW_SCORE)
- printf(" %*d %02d",
+ printf(" %s%*d %02d%s", col,
max_score_digits, ent->score,
- ent->suspect->refcnt);
+ ent->suspect->refcnt, rcol);
if (opt & OUTPUT_SHOW_NAME)
- printf(" %-*.*s", longest_file, longest_file,
- suspect->path);
+ printf(" %s%-*.*s%s", col, longest_file,
+ longest_file,
+ suspect->path,
+ rcol);
if (opt & OUTPUT_SHOW_NUMBER)
- printf(" %*d", max_orig_digits,
- ent->s_lno + 1 + cnt);
-
+ printf(" %s%*d%s", col, max_orig_digits,
+ ent->s_lno + 1 + cnt, rcol);
if (!(opt & OUTPUT_NO_AUTHOR)) {
const char *name;
int pad;
@@ -424,11 +452,12 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
else
name = ci.author.buf;
pad = longest_author - utf8_strwidth(name);
- printf(" (%s%*s %10s",
+ printf(" %s(%s%*s %10s%s", col,
name, pad, "",
format_time(ci.author_time,
ci.author_tz.buf,
- show_raw_time));
+ show_raw_time),
+ rcol);
}
printf(" %*d) ",
max_digits, ent->lno + 1 + cnt);
@@ -607,6 +636,12 @@ static int git_blame_config(const char *var, const char *value, void *cb)
parse_date_format(value, &blame_date_mode);
return 0;
}
+ 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);
+ return 0;
+ }
if (git_diff_heuristic_config(var, value, cb) < 0)
return -1;
@@ -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),
/*
* The following two options are parsed by parse_revision_opt()
@@ -940,8 +976,12 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
blame_coalesce(&sb);
- if (!(output_option & OUTPUT_PORCELAIN))
+ if (!(output_option & OUTPUT_PORCELAIN)) {
find_alignment(&sb, &output_option);
+ if ((output_option & OUTPUT_COLOR_LINE) &&
+ !repeated_meta_color)
+ repeated_meta_color = GIT_COLOR_DARK;
+ }
output(&sb, output_option);
free((void *)sb.final_buf);
diff --git a/color.h b/color.h
index 6cd632c0d8..fce5588df9 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"
diff --git a/t/t8012-blame-colors.sh b/t/t8012-blame-colors.sh
new file mode 100755
index 0000000000..b2de03b777
--- /dev/null
+++ 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' '
+ 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
+'
+
+test_done
--
2.15.1.620.gb9897f4670-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/4] builtin/blame: add option to color metadata fields separately
2017-12-28 21:03 [PATCHv2 0/4] blame: (dim rep. metadata lines or fields, decay date coloring) Stefan Beller
2017-12-28 21:03 ` [PATCH 1/4] color.h: document and modernize header Stefan Beller
2017-12-28 21:03 ` [PATCH 2/4] builtin/blame: dim uninteresting metadata Stefan Beller
@ 2017-12-28 21:03 ` Stefan Beller
2017-12-28 21:03 ` [PATCH 4/4] builtin/blame: highlight recently changed lines Stefan Beller
3 siblings, 0 replies; 13+ messages in thread
From: Stefan Beller @ 2017-12-28 21:03 UTC (permalink / raw)
To: git; +Cc: Stefan Beller
Unlike the previous commit, this dims colors for each
metadata field individually.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/blame.c | 82 +++++++++++++++++++++++++++++++++++++++++++------
t/t8012-blame-colors.sh | 38 +++++++++++++++++++++++
2 files changed, 111 insertions(+), 9 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 09afb4d3ea..39e8c0b222 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -323,6 +323,7 @@ static const char *format_time(timestamp_t time, const char *tz_str,
#define OUTPUT_SHOW_EMAIL 0400
#define OUTPUT_LINE_PORCELAIN 01000
#define OUTPUT_COLOR_LINE 02000
+#define OUTPUT_COLOR_FIELDS 04000
static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
{
@@ -370,6 +371,33 @@ static void emit_porcelain(struct blame_scoreboard *sb, struct blame_entry *ent,
putchar('\n');
}
+static int had_same_field_previously(int opt, int field,
+ struct blame_entry *ent,
+ struct blame_entry *prev)
+{
+ struct commit_info ci, prev_ci;
+
+ switch (field) {
+ case OUTPUT_SHOW_SCORE:
+ return ent->score == prev->score;
+ case OUTPUT_SHOW_NAME:
+ return prev->suspect &&
+ !strcmp(ent->suspect->path, prev->suspect->path);
+ case OUTPUT_SHOW_NUMBER:
+ return ent->s_lno == prev->s_lno + prev->num_lines - 1;
+
+ case OUTPUT_NO_AUTHOR:
+ get_commit_info(ent->suspect->commit, &ci, 1);
+ get_commit_info(prev->suspect->commit, &prev_ci, 1);
+ return ((opt & OUTPUT_SHOW_EMAIL) &&
+ !strcmp(ci.author_mail.buf, prev_ci.author_mail.buf)) ||
+ !strcmp(ci.author.buf, prev_ci.author.buf);
+ default:
+ BUG("unknown field");
+ }
+ return 0;
+}
+
static inline void colors_unset(const char **use_color, const char **reset_color)
{
*use_color = "";
@@ -382,17 +410,36 @@ static inline void colors_set(const char **use_color, const char **reset_color)
*reset_color = GIT_COLOR_RESET;
}
+static void setup_field_color(int opt, int cnt, int field,
+ struct blame_entry *ent,
+ struct blame_entry *prev,
+ const char **use_color,
+ const char **reset_color)
+{
+ if (!(opt & OUTPUT_COLOR_FIELDS))
+ return;
+
+ if ((cnt > 0 ||
+ (prev && had_same_field_previously(opt, field, ent, prev))))
+ colors_set(use_color, reset_color);
+ else
+ colors_unset(use_color, reset_color);
+}
+
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)
+ if ((opt & (OUTPUT_COLOR_LINE | OUTPUT_COLOR_FIELDS)) && cnt > 0)
colors_set(use_color, reset_color);
}
-static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int opt)
+static void emit_other(struct blame_scoreboard *sb,
+ struct blame_entry *ent,
+ struct blame_entry *prev,
+ int opt)
{
int cnt;
const char *cp;
@@ -432,18 +479,27 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
show_raw_time),
ent->lno + 1 + cnt);
} else {
- if (opt & OUTPUT_SHOW_SCORE)
+ if (opt & OUTPUT_SHOW_SCORE) {
+ setup_field_color(opt, cnt, OUTPUT_SHOW_SCORE,
+ ent, prev, &col, &rcol);
printf(" %s%*d %02d%s", col,
max_score_digits, ent->score,
ent->suspect->refcnt, rcol);
- if (opt & OUTPUT_SHOW_NAME)
+ }
+ if (opt & OUTPUT_SHOW_NAME) {
+ setup_field_color(opt, cnt, OUTPUT_SHOW_NAME,
+ ent, prev, &col, &rcol);
printf(" %s%-*.*s%s", col, longest_file,
longest_file,
suspect->path,
rcol);
- if (opt & OUTPUT_SHOW_NUMBER)
+ }
+ if (opt & OUTPUT_SHOW_NUMBER) {
+ setup_field_color(opt, cnt, OUTPUT_SHOW_NUMBER,
+ ent, prev, &col, &rcol);
printf(" %s%*d%s", col, max_orig_digits,
ent->s_lno + 1 + cnt, rcol);
+ }
if (!(opt & OUTPUT_NO_AUTHOR)) {
const char *name;
int pad;
@@ -452,6 +508,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
else
name = ci.author.buf;
pad = longest_author - utf8_strwidth(name);
+ setup_field_color(opt, cnt, OUTPUT_NO_AUTHOR,
+ ent, prev, &col, &rcol);
printf(" %s(%s%*s %10s%s", col,
name, pad, "",
format_time(ci.author_time,
@@ -477,7 +535,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
static void output(struct blame_scoreboard *sb, int option)
{
- struct blame_entry *ent;
+ struct blame_entry *ent, *prev = NULL;
if (option & OUTPUT_PORCELAIN) {
for (ent = sb->ent; ent; ent = ent->next) {
@@ -499,7 +557,8 @@ static void output(struct blame_scoreboard *sb, int option)
if (option & OUTPUT_PORCELAIN)
emit_porcelain(sb, ent, option);
else {
- emit_other(sb, ent, option);
+ emit_other(sb, ent, prev, option);
+ prev = ent;
}
}
}
@@ -717,6 +776,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
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),
+ OPT_BIT(0, "color-fields", &output_option, N_("color redundant metadata fields from previous line"), OUTPUT_COLOR_FIELDS),
/*
* The following two options are parsed by parse_revision_opt()
@@ -775,6 +835,10 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
revs.diffopt.flags.follow_renames = 0;
argc = parse_options_end(&ctx);
+ if ((output_option & OUTPUT_COLOR_LINE) &&
+ (output_option & OUTPUT_COLOR_FIELDS))
+ die(_("cannot ask for colored lines and fields at the same time"));
+
if (incremental || (output_option & OUTPUT_PORCELAIN)) {
if (show_progress > 0)
die(_("--progress can't be used with --incremental or porcelain formats"));
@@ -978,8 +1042,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
if (!(output_option & OUTPUT_PORCELAIN)) {
find_alignment(&sb, &output_option);
- if ((output_option & OUTPUT_COLOR_LINE) &&
- !repeated_meta_color)
+ if (!repeated_meta_color &&
+ (output_option & (OUTPUT_COLOR_LINE | OUTPUT_COLOR_FIELDS)))
repeated_meta_color = GIT_COLOR_DARK;
}
diff --git a/t/t8012-blame-colors.sh b/t/t8012-blame-colors.sh
index b2de03b777..be491bde9a 100755
--- a/t/t8012-blame-colors.sh
+++ b/t/t8012-blame-colors.sh
@@ -15,4 +15,42 @@ test_expect_success 'colored blame colors continuous lines' '
test_line_count = 3 H.expect
'
+test_expect_success 'colored blame colors continuous fields' '
+
+ git mv hello.c world.c &&
+ git commit -a -m "moved file" &&
+ cat <<-EOF >> world.c &&
+ void world()
+ {
+ puts("world");
+ }
+ EOF
+ git add world.c &&
+ GIT_AUTHOR_NAME="F" GIT_AUTHOR_EMAIL="F@test.git" \
+ git commit -m "forgot to add changes to moved file" &&
+
+ git blame --abbrev=12 --color-fields world.c >actual.raw &&
+ test_decode_color <actual.raw >actual &&
+
+ grep "<BOLD;BLACK>hello.c" actual > colored_hello.expect &&
+ grep "hello.c" actual > all_hello.expect &&
+ test_line_count = 9 colored_hello.expect &&
+ test_line_count = 10 all_hello.expect &&
+
+ grep "<BOLD;BLACK>world.c" actual > colored_world.expect &&
+ grep "world.c" actual > all_world.expect &&
+ test_line_count = 3 colored_world.expect &&
+ test_line_count = 4 all_world.expect &&
+
+ grep "(F" actual > all_F.expect &&
+ grep "<BOLD;BLACK>(F" actual > colored_F.expect &&
+ test_line_count = 8 all_F.expect &&
+ test_line_count = 5 colored_F.expect &&
+
+ grep "(H" actual > all_H.expect &&
+ grep "<BOLD;BLACK>(H" actual > colored_H.expect &&
+ test_line_count = 5 all_H.expect &&
+ test_line_count = 3 colored_H.expect
+'
+
test_done
--
2.15.1.620.gb9897f4670-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 4/4] builtin/blame: highlight recently changed lines
2017-12-28 21:03 [PATCHv2 0/4] blame: (dim rep. metadata lines or fields, decay date coloring) Stefan Beller
` (2 preceding siblings ...)
2017-12-28 21:03 ` [PATCH 3/4] builtin/blame: add option to color metadata fields separately Stefan Beller
@ 2017-12-28 21:03 ` Stefan Beller
2017-12-28 22:45 ` Eric Sunshine
3 siblings, 1 reply; 13+ messages in thread
From: Stefan Beller @ 2017-12-28 21:03 UTC (permalink / raw)
To: git; +Cc: Stefan Beller
Choose a different color for dates and imitate a 'temperature cool down'
for the dates.
Originally I had planned to have the temperature cooldown dependent on
the age of the project or file for example, as that might scale better,
but that can be added on top of this commit, e.g. instead of giving a
date, you could imagine giving a percentage that would be the linearly
interpolated between now and the beginning of the file.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Documentation/config.txt | 18 ++++++++++++
builtin/blame.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 91 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2c211b6e7d..52f27b2ced 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1221,6 +1221,24 @@ color.blame.repeatedMeta::
is repeated meta information per line (such as commit id,
author name, date and timezone). Defaults to dark gray.
+color.blame.highlightRecent::
+ This can be used to color the author and date of a blame line.
+ This overrides `color.blame.repeatedMeta` setting, which colors
+ repetitions.
++
+This setting should be set to a comma-separated list of color and date settings,
+starting and ending with a color, the dates should be set from oldest to newest.
+The metadata will be colored given the colors if the the line was introduced
+before the given timestamp, overwriting older timestamped colors.
++
+Instead of an absolute timestamp relative timestamps work as well, e.g.
+2.weeks.ago is valid to address anything older than 2 weeks.
++
+It defaults to "blue,12 month ago,white,1 month ago,red", which colors
+everything older than one year blue, recent changes between one month and
+one year old are kept white, and lines introduced within the last month are
+colored red.
+
color.ui::
This variable determines the default value for variables such
as `color.diff` and `color.grep` that control the use of color
diff --git a/builtin/blame.c b/builtin/blame.c
index 39e8c0b222..b23e98e2ac 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -24,6 +24,7 @@
#include "dir.h"
#include "progress.h"
#include "blame.h"
+#include "string-list.h"
static char blame_usage[] = N_("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>");
@@ -324,6 +325,7 @@ static const char *format_time(timestamp_t time, const char *tz_str,
#define OUTPUT_LINE_PORCELAIN 01000
#define OUTPUT_COLOR_LINE 02000
#define OUTPUT_COLOR_FIELDS 04000
+#define OUTPUT_HEATED_LINES 010000
static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
{
@@ -436,6 +438,61 @@ static void setup_line_color(int opt, int cnt,
colors_set(use_color, reset_color);
}
+static struct color_field {
+ timestamp_t hop;
+ char col[COLOR_MAXLEN];
+} *colorfield;
+static int colorfield_nr, colorfield_alloc;
+
+static void parse_color_fields(const char *s)
+{
+ struct string_list l = STRING_LIST_INIT_DUP;
+ struct string_list_item *item;
+ enum { EXPECT_DATE, EXPECT_COLOR } next = EXPECT_COLOR;
+
+ /* Ideally this would be stripped and split at the same time? */
+ string_list_split(&l, s, ',', -1);
+ ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
+
+ for_each_string_list_item(item, &l) {
+ switch (next) {
+ case EXPECT_DATE:
+ colorfield[colorfield_nr].hop = approxidate(item->string);
+ next = EXPECT_COLOR;
+ colorfield_nr++;
+ ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
+ break;
+ case EXPECT_COLOR:
+ if (color_parse(item->string, colorfield[colorfield_nr].col))
+ die(_("expecting a color: %s"), item->string);
+ next = EXPECT_DATE;
+ break;
+ }
+ }
+
+ if (next == EXPECT_COLOR)
+ die (_("must end with a color"));
+
+ colorfield[colorfield_nr].hop = TIME_MAX;
+}
+
+static void setup_default_colorfield(void)
+{
+ parse_color_fields("blue,12 month ago,white,1 month ago,red");
+}
+
+static void determine_line_heat(struct blame_entry *ent, const char **dest_color)
+{
+ int i = 0;
+ struct commit_info ci;
+ get_commit_info(ent->suspect->commit, &ci, 1);
+
+ while (i < colorfield_nr && ci.author_time > colorfield[i].hop)
+ i++;
+
+ *dest_color = colorfield[i].col;
+}
+
static void emit_other(struct blame_scoreboard *sb,
struct blame_entry *ent,
struct blame_entry *prev,
@@ -443,6 +500,7 @@ static void emit_other(struct blame_scoreboard *sb,
{
int cnt;
const char *cp;
+ const char *heatcolor = NULL;
struct blame_origin *suspect = ent->suspect;
struct commit_info ci;
char hex[GIT_MAX_HEXSZ + 1];
@@ -452,6 +510,10 @@ static void emit_other(struct blame_scoreboard *sb,
oid_to_hex_r(hex, &suspect->commit->object.oid);
cp = blame_nth_line(sb, ent->lno);
+
+ if (opt & OUTPUT_HEATED_LINES)
+ determine_line_heat(ent, &heatcolor);
+
for (cnt = 0; cnt < ent->num_lines; cnt++) {
char ch;
int length = (opt & OUTPUT_LONG_OBJECT_NAME) ? GIT_SHA1_HEXSZ : abbrev;
@@ -503,6 +565,7 @@ static void emit_other(struct blame_scoreboard *sb,
if (!(opt & OUTPUT_NO_AUTHOR)) {
const char *name;
int pad;
+ int hcolor = opt & OUTPUT_HEATED_LINES;
if (opt & OUTPUT_SHOW_EMAIL)
name = ci.author_mail.buf;
else
@@ -510,7 +573,10 @@ static void emit_other(struct blame_scoreboard *sb,
pad = longest_author - utf8_strwidth(name);
setup_field_color(opt, cnt, OUTPUT_NO_AUTHOR,
ent, prev, &col, &rcol);
- printf(" %s(%s%*s %10s%s", col,
+ if (hcolor)
+ rcol = GIT_COLOR_RESET;
+ printf(" %s(%s%*s %10s%s",
+ hcolor ? heatcolor : col,
name, pad, "",
format_time(ci.author_time,
ci.author_tz.buf,
@@ -701,6 +767,10 @@ static int git_blame_config(const char *var, const char *value, void *cb)
value);
return 0;
}
+ if (!strcmp(var, "color.blame.highlightrecent")) {
+ parse_color_fields(value);
+ return 0;
+ }
if (git_diff_heuristic_config(var, value, cb) < 0)
return -1;
@@ -777,6 +847,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
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),
OPT_BIT(0, "color-fields", &output_option, N_("color redundant metadata fields from previous line"), OUTPUT_COLOR_FIELDS),
+ OPT_BIT(0, "heated-lines", &output_option, N_("color lines by date"), OUTPUT_HEATED_LINES),
/*
* The following two options are parsed by parse_revision_opt()
@@ -801,6 +872,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
unsigned int range_i;
long anchor;
+ setup_default_colorfield();
git_config(git_blame_config, &output_option);
init_revisions(&revs, NULL);
revs.date_mode = blame_date_mode;
--
2.15.1.620.gb9897f4670-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] color.h: document and modernize header
2017-12-28 21:03 ` [PATCH 1/4] color.h: document and modernize header Stefan Beller
@ 2017-12-28 22:00 ` Eric Sunshine
2018-01-04 19:49 ` Stefan Beller
0 siblings, 1 reply; 13+ messages in thread
From: Eric Sunshine @ 2017-12-28 22:00 UTC (permalink / raw)
To: Stefan Beller; +Cc: Git List
On Thu, Dec 28, 2017 at 4:03 PM, Stefan Beller <sbeller@google.com> wrote:
> Add documentation explaining the functions in color.h.
> While at it, mark them extern.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/color.h b/color.h
> @@ -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
> * 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.
> +/*
> + * 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/
> +/* 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.
> +/*
> + * 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.
> +/*
> + * 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).
> + * 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?)
> + */
> __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);
>
> -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/
> + */
> +extern int color_is_nil(const char *color);
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] builtin/blame: dim uninteresting metadata
2017-12-28 21:03 ` [PATCH 2/4] builtin/blame: dim uninteresting metadata Stefan Beller
@ 2017-12-28 22:29 ` Eric Sunshine
2018-01-04 22:10 ` Stefan Beller
0 siblings, 1 reply; 13+ messages in thread
From: Eric Sunshine @ 2017-12-28 22:29 UTC (permalink / raw)
To: Stefan Beller; +Cc: Git List
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
> +'
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 4/4] builtin/blame: highlight recently changed lines
2017-12-28 21:03 ` [PATCH 4/4] builtin/blame: highlight recently changed lines Stefan Beller
@ 2017-12-28 22:45 ` Eric Sunshine
0 siblings, 0 replies; 13+ messages in thread
From: Eric Sunshine @ 2017-12-28 22:45 UTC (permalink / raw)
To: Stefan Beller; +Cc: Git List
On Thu, Dec 28, 2017 at 4:03 PM, Stefan Beller <sbeller@google.com> wrote:
> Choose a different color for dates and imitate a 'temperature cool down'
> for the dates.
s/for the dates/depending upon age/
> [...]
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> diff --git a/builtin/blame.c b/builtin/blame.c
> @@ -777,6 +847,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
> + OPT_BIT(0, "heated-lines", &output_option, N_("color lines by date"), OUTPUT_HEATED_LINES),
s/date/age/
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/4] color.h: document and modernize header
2017-12-28 22:00 ` Eric Sunshine
@ 2018-01-04 19:49 ` Stefan Beller
0 siblings, 0 replies; 13+ messages in thread
From: Stefan Beller @ 2018-01-04 19:49 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List
On Thu, Dec 28, 2017 at 2:00 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Dec 28, 2017 at 4:03 PM, Stefan Beller <sbeller@google.com> wrote:
>> Add documentation explaining the functions in color.h.
>> While at it, mark them extern.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> diff --git a/color.h b/color.h
>> @@ -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
>> * 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.
Right. This description is bad.
It's implementation is just
xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
Apparently this function is only ever used by grep.c which uses
it to fill in struct grep_opt {
...
char color_context[COLOR_MAXLEN];
char color_filename[COLOR_MAXLEN];
char color_function[COLOR_MAXLEN];
char color_lineno[COLOR_MAXLEN];
char color_match_context[COLOR_MAXLEN];
char color_match_selected[COLOR_MAXLEN];
char color_selected[COLOR_MAXLEN];
char color_sep[COLOR_MAXLEN];
...
}
I guess I'll drop the documentation for color_set, and put a NEEDSWORK
comment there as I'd think we don't need to copy around the colors
using snprintf, but either can keep pointers or use xmemdup.
>
>> +/*
>> + * 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?
Maybe I should fix the code as well. Currently it only returns one of
0 (=GIT_COLOR_NEVER), 1 (=GIT_COLOR_ALWAYS) and
GIT_COLOR_AUTO.
> 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?
done.
>
> Finally, for grammatical consistency with other comments:
> s/Parses/Parse
> s/Returns/Return/
done
>
>> +/* 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.
done
>
>> +/*
>> + * 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.
>
>> +/*
>> + * 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).
sounds good.
>
>> + * 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?)
added.
>
>> + */
>> __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);
>>
>> -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/
done
>
>> + */
>> +extern int color_is_nil(const char *color);
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] builtin/blame: dim uninteresting metadata
2017-12-28 22:29 ` Eric Sunshine
@ 2018-01-04 22:10 ` Stefan Beller
2018-01-06 8:26 ` Eric Sunshine
0 siblings, 1 reply; 13+ messages in thread
From: Stefan Beller @ 2018-01-04 22:10 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] builtin/blame: dim uninteresting metadata
2018-01-04 22:10 ` Stefan Beller
@ 2018-01-06 8:26 ` Eric Sunshine
2018-01-08 19:38 ` Stefan Beller
0 siblings, 1 reply; 13+ messages in thread
From: Eric Sunshine @ 2018-01-06 8:26 UTC (permalink / raw)
To: Stefan Beller; +Cc: Git List
On Thu, Jan 4, 2018 at 5:10 PM, Stefan Beller <sbeller@google.com> wrote:
> 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:
>>> +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;
>>> +}
>>
>> 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.
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/4] builtin/blame: dim uninteresting metadata
2018-01-06 8:26 ` Eric Sunshine
@ 2018-01-08 19:38 ` Stefan Beller
0 siblings, 0 replies; 13+ messages in thread
From: Stefan Beller @ 2018-01-08 19:38 UTC (permalink / raw)
To: Eric Sunshine; +Cc: Git List
On Sat, Jan 6, 2018 at 12:26 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Jan 4, 2018 at 5:10 PM, Stefan Beller <sbeller@google.com> wrote:
>> 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:
>>>> +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;
>>>> +}
>>>
>>> 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.
>
> 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.
I asked the question before rewriting and resending, and now I agree
that we do not want to have these small helpers.
Thanks,
Stefan
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-01-08 19:38 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-28 21:03 [PATCHv2 0/4] blame: (dim rep. metadata lines or fields, decay date coloring) Stefan Beller
2017-12-28 21:03 ` [PATCH 1/4] color.h: document and modernize header Stefan Beller
2017-12-28 22:00 ` Eric Sunshine
2018-01-04 19:49 ` Stefan Beller
2017-12-28 21:03 ` [PATCH 2/4] builtin/blame: dim uninteresting metadata Stefan Beller
2017-12-28 22:29 ` Eric Sunshine
2018-01-04 22:10 ` Stefan Beller
2018-01-06 8:26 ` Eric Sunshine
2018-01-08 19:38 ` Stefan Beller
2017-12-28 21:03 ` [PATCH 3/4] builtin/blame: add option to color metadata fields separately Stefan Beller
2017-12-28 21:03 ` [PATCH 4/4] builtin/blame: highlight recently changed lines Stefan Beller
2017-12-28 22:45 ` Eric Sunshine
-- strict thread matches above, loose matches on Subject: below --
2017-11-10 1:09 [RFC PATCH 0/4] blame: (dim rep. metadata lines or fields, decay date coloring) Stefan Beller
2017-11-10 1:10 ` [PATCH 3/4] builtin/blame: add option to color metadata fields separately Stefan Beller
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).