[RFC] Colorize 'commit' lines in log ui

[RFC] Colorize 'commit' lines in log ui

[Jeff King]

When paging through the output of git-whatchanged, the color cues help to
visually navigate within a diff. However, it is difficult to notice when a
new commit starts, because the commit and log are shown in the "normal"
color. This patch colorizes the 'commit' line, customizable through
diff.colors.commit and defaulting to yellow.

As a side effect, some of the diff color engine (slot enum, get_color) has
become accessible outside of diff.c.

Signed-off-by: Jeff King <peff@peff.net>
---

I think the visual cue makes the git-whatchanged -p output much easier
to read.

I originally thought of making a "colorized commit" patch where you
could independently set commit.color and commit.color.*. However, it was
clearly much simpler to start by using the diff engine's code.
  1. Do people really want to colorize the commit outside of colorizing
     the diffs, anyway? It makes sense for the code to go together.
  2. Should we call it something other than diff_colors (since now it's
     handling the commit log, too)?

It currently just colorizes the 'commit' line. We could also do the
'Author' and 'Date' lines (either as the same color, or independently),
as well as the log text.

It colorizes all formats given to show_log() except CMIT_FMT_EMAIL (so,
e.g., the sha1's in CMIT_FMT_ONELINE will be yellow). Is that what we
want?

 diff.c     |   28 +++++++++++-----------------
 diff.h     |   11 +++++++++++
 log-tree.c |    5 ++++-
 3 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/diff.c b/diff.c
index 8b44756..6a71376 100644
--- a/diff.c
+++ b/diff.c
@@ -17,15 +17,6 @@ static int diff_detect_rename_default = 
 static int diff_rename_limit_default = -1;
 static int diff_use_color_default = 0;
 
-enum color_diff {
-	DIFF_RESET = 0,
-	DIFF_PLAIN = 1,
-	DIFF_METAINFO = 2,
-	DIFF_FRAGINFO = 3,
-	DIFF_FILE_OLD = 4,
-	DIFF_FILE_NEW = 5,
-};
-
 /* "\033[1;38;5;2xx;48;5;2xxm\0" is 23 bytes */
 static char diff_colors[][24] = {
 	"\033[m",	/* reset */
@@ -33,7 +24,8 @@ static char diff_colors[][24] = {
 	"\033[1m",	/* bold */
 	"\033[36m",	/* cyan */
 	"\033[31m",	/* red */
-	"\033[32m"	/* green */
+	"\033[32m",	/* green */
+	"\033[33m"	/* yellow */
 };
 
 static int parse_diff_color_slot(const char *var, int ofs)
@@ -48,6 +40,8 @@ static int parse_diff_color_slot(const c
 		return DIFF_FILE_OLD;
 	if (!strcasecmp(var+ofs, "new"))
 		return DIFF_FILE_NEW;
+	if (!strcasecmp(var+ofs, "commit"))
+		return DIFF_COMMIT;
 	die("bad config variable '%s'", var);
 }
 
@@ -370,7 +364,7 @@ struct emit_callback {
 	const char **label_path;
 };
 
-static inline const char *get_color(int diff_use_color, enum color_diff ix)
+const char *diff_get_color(int diff_use_color, enum color_diff ix)
 {
 	if (diff_use_color)
 		return diff_colors[ix];
@@ -381,8 +375,8 @@ static void fn_out_consume(void *priv, c
 {
 	int i;
 	struct emit_callback *ecbdata = priv;
-	const char *set = get_color(ecbdata->color_diff, DIFF_METAINFO);
-	const char *reset = get_color(ecbdata->color_diff, DIFF_RESET);
+	const char *set = diff_get_color(ecbdata->color_diff, DIFF_METAINFO);
+	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
 
 	if (ecbdata->label_path[0]) {
 		printf("%s--- %s%s\n", set, ecbdata->label_path[0], reset);
@@ -397,7 +391,7 @@ static void fn_out_consume(void *priv, c
 		;
 	if (2 <= i && i < len && line[i] == ' ') {
 		ecbdata->nparents = i - 1;
-		set = get_color(ecbdata->color_diff, DIFF_FRAGINFO);
+		set = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
 	}
 	else if (len < ecbdata->nparents)
 		set = reset;
@@ -410,7 +404,7 @@ static void fn_out_consume(void *priv, c
 			else if (line[i] == '+')
 				color = DIFF_FILE_NEW;
 		}
-		set = get_color(ecbdata->color_diff, color);
+		set = diff_get_color(ecbdata->color_diff, color);
 	}
 	if (len > 0 && line[len-1] == '\n')
 		len--;
@@ -767,8 +761,8 @@ static void builtin_diff(const char *nam
 	mmfile_t mf1, mf2;
 	const char *lbl[2];
 	char *a_one, *b_two;
-	const char *set = get_color(o->color_diff, DIFF_METAINFO);
-	const char *reset = get_color(o->color_diff, DIFF_RESET);
+	const char *set = diff_get_color(o->color_diff, DIFF_METAINFO);
+	const char *reset = diff_get_color(o->color_diff, DIFF_RESET);
 
 	a_one = quote_two("a/", name_a);
 	b_two = quote_two("b/", name_b);
diff --git a/diff.h b/diff.h
index a06f959..2cced53 100644
--- a/diff.h
+++ b/diff.h
@@ -69,6 +69,17 @@ struct diff_options {
 	add_remove_fn_t add_remove;
 };
 
+enum color_diff {
+	DIFF_RESET = 0,
+	DIFF_PLAIN = 1,
+	DIFF_METAINFO = 2,
+	DIFF_FRAGINFO = 3,
+	DIFF_FILE_OLD = 4,
+	DIFF_FILE_NEW = 5,
+	DIFF_COMMIT = 6,
+};
+const char *diff_get_color(int diff_use_color, enum color_diff ix);
+
 extern const char mime_boundary_leader[];
 
 extern void diff_tree_setup_paths(const char **paths, struct diff_options *);
diff --git a/log-tree.c b/log-tree.c
index 9d8d46f..b71cf9b 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -129,7 +129,8 @@ void show_log(struct rev_info *opt, cons
 			opt->diffopt.stat_sep = buffer;
 		}
 	} else {
-		printf("%s%s",
+		printf("%s%s%s",
+		       diff_get_color(opt->diffopt.color_diff, DIFF_COMMIT),
 		       opt->commit_format == CMIT_FMT_ONELINE ? "" : "commit ",
 		       diff_unique_abbrev(commit->object.sha1, abbrev_commit));
 		if (opt->parents)
@@ -139,6 +140,8 @@ void show_log(struct rev_info *opt, cons
 			       diff_unique_abbrev(parent->object.sha1,
 						  abbrev_commit));
 		putchar(opt->commit_format == CMIT_FMT_ONELINE ? ' ' : '\n');
+		printf("%s",
+		       diff_get_color(opt->diffopt.color_diff, DIFF_RESET));
 	}
 
 	/*
-- 
1.4.2.rc1.gc470-dirty

Re: [RFC] Colorize 'commit' lines in log ui

[Jan-Benedict Glaw]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

On Sun, 2006-07-23 05:24:18 -0400, Jeff King <peff@peff.net> wrote:
> @@ -33,7 +24,8 @@ static char diff_colors[][24] = {
>  	"\033[1m",	/* bold */
>  	"\033[36m",	/* cyan */
>  	"\033[31m",	/* red */
> -	"\033[32m"	/* green */
> +	"\033[32m",	/* green */
> +	"\033[33m"	/* yellow */
>  };
>  
>  static int parse_diff_color_slot(const char *var, int ofs)

If people only were used to *always* place a comma behind the last
array element, the diff would be shorter by one line each time...

MfG, JBG

-- 
       Jan-Benedict Glaw       jbglaw@lug-owl.de                +49-172-7608481
 Signature of:                     ...und wenn Du denkst, es geht nicht mehr,
 the second  :                            kommt irgendwo ein Lichtlein her.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [RFC] Colorize 'commit' lines in log ui

[Jeff King]

On Sun, Jul 23, 2006 at 01:24:22PM +0200, Jan-Benedict Glaw wrote:

> If people only were used to *always* place a comma behind the last
> array element, the diff would be shorter by one line each time...

I agree that it's much more convenient; I left it off because it wasn't
there before and I thought there were some standardization issues.

As it turns out, the situation is quite ridiculous. C89 allowed trailing
commas in initialization lists but not in enums. Most compilers (gcc,
sun) accepted it anyway, but some (aix xlc) did not.

Do we care? I think probably not, since the color_diff enum already had
a trailing comma. It seems like the primary non-gcc compiler that has
been mentioned is sun cc. I don't know what other compilers are being
used to compile git.

-Peff

Re: [RFC] Colorize 'commit' lines in log ui

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I think the visual cue makes the git-whatchanged -p output much easier
> to read.

Although I personally do not do much colors (instead I do
"/^commit [0-9a-f]*" under less), I think this makes sense and I
do not think of any obvious downside.