[PATCH UI experiment] diffstat: annotate/highlight new or removed files

[PATCH UI experiment] diffstat: annotate/highlight new or removed files

[Nguyễn Thái Ngọc Duy]

diffstat does not show whether a file is added or deleted. I know
--summary does. But the problem with --summary is it makes me look for
information of a file in two places: diffstat and summary. And with a
commit that adds/removes a lot, showing both --stat --summary can be
long.

This patch adds "(new)", "(gone)" or "(new mode)" to diffstat, with
highlight, to easily catch file additions/removals. The extra text is
chosen to be short enough so that it won't take up too much space for
path names:

 .gitignore                  |   1 +
 Makefile                    |   3 +
 t/t3070-wildmatch.sh (new)  | 188 ++++++++++++++++++++++++
 t/t3070/wildtest.txt (gone) | 165 ---------------------
 test-wildmatch.c (new)      |  14 ++
 wildmatch.c                 |   5 +-
 6 files changed, 210 insertions(+), 166 deletions(-)

I don't put creation modes in there too because most of the time it
does not matter much to me and I could look down to --summary for mode
verification. But we could put "(new+x)" for 0755 and just "(new)" for
0644. "(new mode)" then could become "(+x)", "(-x)" or something like
that.

Coloring is to me an improvement over --summary. Probably the main
point. Without it, perhaps it's not worth putting extra text to
diffstat.

Single patch for easy testing. Test suite probably breaks.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 diff.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++-----------------
 diff.h |  3 ++-
 utf8.c | 33 +++++++++++++++++++++++++++
 utf8.h |  2 ++
 4 files changed, 97 insertions(+), 22 deletions(-)

diff --git a/diff.c b/diff.c
index 35d3f07..f9217e6 100644
--- a/diff.c
+++ b/diff.c
@@ -45,6 +45,7 @@ static char diff_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_YELLOW,	/* COMMIT */
 	GIT_COLOR_BG_RED,	/* WHITESPACE */
 	GIT_COLOR_NORMAL,	/* FUNCINFO */
+	GIT_COLOR_YELLOW,	/* STATUSINFO */
 };
 
 static int parse_diff_color_slot(const char *var, int ofs)
@@ -65,6 +66,8 @@ static int parse_diff_color_slot(const char *var, int ofs)
 		return DIFF_WHITESPACE;
 	if (!strcasecmp(var+ofs, "func"))
 		return DIFF_FUNCINFO;
+	if (!strcasecmp(var+ofs, "status"))
+		return DIFF_STATUSINFO;
 	return -1;
 }
 
@@ -1223,7 +1226,8 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 	}
 }
 
-static char *pprint_rename(const char *a, const char *b)
+static char *pprint_rename(struct diff_options *options,
+			   const char *a, const char *b)
 {
 	const char *old = a;
 	const char *new = b;
@@ -1237,7 +1241,9 @@ static char *pprint_rename(const char *a, const char *b)
 
 	if (qlen_a || qlen_b) {
 		quote_c_style(a, &name, NULL, 0);
-		strbuf_addstr(&name, " => ");
+		strbuf_addf(&name, " %s=>%s ",
+			    diff_get_color_opt(options, DIFF_STATUSINFO),
+			    diff_get_color_opt(options, DIFF_RESET));
 		quote_c_style(b, &name, NULL, 0);
 		return strbuf_detach(&name, NULL);
 	}
@@ -1278,13 +1284,19 @@ static char *pprint_rename(const char *a, const char *b)
 	strbuf_grow(&name, pfx_length + a_midlen + b_midlen + sfx_length + 7);
 	if (pfx_length + sfx_length) {
 		strbuf_add(&name, a, pfx_length);
-		strbuf_addch(&name, '{');
+		strbuf_addf(&name, "%s{%s",
+			    diff_get_color_opt(options, DIFF_STATUSINFO),
+			    diff_get_color_opt(options, DIFF_RESET));
 	}
 	strbuf_add(&name, a + pfx_length, a_midlen);
-	strbuf_addstr(&name, " => ");
+	strbuf_addf(&name, " %s=>%s ",
+		    diff_get_color_opt(options, DIFF_STATUSINFO),
+		    diff_get_color_opt(options, DIFF_RESET));
 	strbuf_add(&name, b + pfx_length, b_midlen);
 	if (pfx_length + sfx_length) {
-		strbuf_addch(&name, '}');
+		strbuf_addf(&name, "%s}%s",
+			    diff_get_color_opt(options, DIFF_STATUSINFO),
+			    diff_get_color_opt(options, DIFF_RESET));
 		strbuf_add(&name, a + len_a - sfx_length, sfx_length);
 	}
 	return strbuf_detach(&name, NULL);
@@ -1300,6 +1312,7 @@ struct diffstat_t {
 		unsigned is_unmerged:1;
 		unsigned is_binary:1;
 		unsigned is_renamed:1;
+		char status;
 		uintmax_t added, deleted;
 	} **files;
 };
@@ -1357,7 +1370,8 @@ static int scale_linear(int it, int width, int max_change)
 static void show_name(FILE *file,
 		      const char *prefix, const char *name, int len)
 {
-	fprintf(file, " %s%-*s |", prefix, len, name);
+	fprintf(file, " %s%-*s |", prefix,
+		len + strlen_ansi(name), name);
 }
 
 static void show_graph(FILE *file, char ch, int cnt, const char *set, const char *reset)
@@ -1370,7 +1384,8 @@ static void show_graph(FILE *file, char ch, int cnt, const char *set, const char
 	fprintf(file, "%s", reset);
 }
 
-static void fill_print_name(struct diffstat_file *file)
+static void fill_print_name(struct diff_options *options,
+			    struct diffstat_file *file)
 {
 	char *pname;
 
@@ -1379,14 +1394,32 @@ static void fill_print_name(struct diffstat_file *file)
 
 	if (!file->is_renamed) {
 		struct strbuf buf = STRBUF_INIT;
-		if (quote_c_style(file->name, &buf, NULL, 0)) {
+		if (quote_c_style(file->name, &buf, NULL, 0) ||
+		    file->status) {
+			const char *str = NULL;
+			switch (file->status) {
+			case DIFF_STATUS_ADDED:
+				str = "new";
+				break;
+			case DIFF_STATUS_DELETED:
+				str = "gone";
+				break;
+			case DIFF_STATUS_TYPE_CHANGED:
+				str = "new mode";
+				break;
+			}
+			if (str)
+				strbuf_addf(&buf, " (%s%s%s)",
+					    diff_get_color_opt(options, DIFF_STATUSINFO),
+					    str,
+					    diff_get_color_opt(options, DIFF_RESET));
 			pname = strbuf_detach(&buf, NULL);
 		} else {
 			pname = file->name;
 			strbuf_release(&buf);
 		}
 	} else {
-		pname = pprint_rename(file->from_name, file->name);
+		pname = pprint_rename(options, file->from_name, file->name);
 	}
 	file->print_name = pname;
 }
@@ -1474,8 +1507,8 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 			count++; /* not shown == room for one more */
 			continue;
 		}
-		fill_print_name(file);
-		len = strlen(file->print_name);
+		fill_print_name(options, file);
+		len = strlen_no_ansi(file->print_name);
 		if (max_len < len)
 			max_len = len;
 
@@ -1599,7 +1632,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 		 * "scale" the filename
 		 */
 		len = name_width;
-		name_len = strlen(name);
+		name_len = strlen_no_ansi(name);
 		if (name_width < name_len) {
 			char *slash;
 			prefix = "...";
@@ -1737,7 +1770,7 @@ static void show_numstat(struct diffstat_t *data, struct diff_options *options)
 				"%"PRIuMAX"\t%"PRIuMAX"\t",
 				file->added, file->deleted);
 		if (options->line_termination) {
-			fill_print_name(file);
+			fill_print_name(options, file);
 			if (!file->is_renamed)
 				write_name_quoted(file->name, options->file,
 						  options->line_termination);
@@ -2397,13 +2430,15 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 			     struct diff_filespec *two,
 			     struct diffstat_t *diffstat,
 			     struct diff_options *o,
-			     int complete_rewrite)
+			     int complete_rewrite,
+			     char status)
 {
 	mmfile_t mf1, mf2;
 	struct diffstat_file *data;
 	int same_contents;
 
 	data = diffstat_add(diffstat, name_a, name_b);
+	data->status = status;
 
 	if (!one || !two) {
 		data->is_unmerged = 1;
@@ -3118,7 +3153,8 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o,
 
 	if (DIFF_PAIR_UNMERGED(p)) {
 		/* unmerged */
-		builtin_diffstat(p->one->path, NULL, NULL, NULL, diffstat, o, 0);
+		builtin_diffstat(p->one->path, NULL, NULL, NULL,
+				 diffstat, o, 0, 0);
 		return;
 	}
 
@@ -3133,7 +3169,8 @@ static void run_diffstat(struct diff_filepair *p, struct diff_options *o,
 
 	if (p->status == DIFF_STATUS_MODIFIED && p->score)
 		complete_rewrite = 1;
-	builtin_diffstat(name, other, p->one, p->two, diffstat, o, complete_rewrite);
+	builtin_diffstat(name, other, p->one, p->two, diffstat,
+			 o, complete_rewrite, p->status);
 }
 
 static void run_checkdiff(struct diff_filepair *p, struct diff_options *o)
@@ -4118,10 +4155,12 @@ static void show_mode_change(FILE *file, struct diff_filepair *p, int show_name,
 	}
 }
 
-static void show_rename_copy(FILE *file, const char *renamecopy, struct diff_filepair *p,
-			const char *line_prefix)
+static void show_rename_copy(FILE *file, const char *renamecopy,
+			     struct diff_filepair *p,
+			     const char *line_prefix,
+			     struct diff_options *options)
 {
-	char *names = pprint_rename(p->one->path, p->two->path);
+	char *names = pprint_rename(options, p->one->path, p->two->path);
 
 	fprintf(file, " %s %s (%d%%)\n", renamecopy, names, similarity_index(p));
 	free(names);
@@ -4149,11 +4188,11 @@ static void diff_summary(struct diff_options *opt, struct diff_filepair *p)
 		break;
 	case DIFF_STATUS_COPIED:
 		fputs(line_prefix, file);
-		show_rename_copy(file, "copy", p, line_prefix);
+		show_rename_copy(file, "copy", p, line_prefix, opt);
 		break;
 	case DIFF_STATUS_RENAMED:
 		fputs(line_prefix, file);
-		show_rename_copy(file, "rename", p, line_prefix);
+		show_rename_copy(file, "rename", p, line_prefix, opt);
 		break;
 	default:
 		if (p->score) {
diff --git a/diff.h b/diff.h
index a658f85..c609512 100644
--- a/diff.h
+++ b/diff.h
@@ -167,7 +167,8 @@ enum color_diff {
 	DIFF_FILE_NEW = 5,
 	DIFF_COMMIT = 6,
 	DIFF_WHITESPACE = 7,
-	DIFF_FUNCINFO = 8
+	DIFF_FUNCINFO = 8,
+	DIFF_STATUSINFO = 9
 };
 const char *diff_get_color(int diff_use_color, enum color_diff ix);
 #define diff_get_color_opt(o, ix) \
diff --git a/utf8.c b/utf8.c
index a544f15..10823fd 100644
--- a/utf8.c
+++ b/utf8.c
@@ -317,6 +317,39 @@ static size_t display_mode_esc_sequence_len(const char *s)
 	return p - s;
 }
 
+size_t strlen_no_ansi(const char *s)
+{
+	size_t len = 0;
+	for (;;) {
+		size_t skip;
+
+		while ((skip = display_mode_esc_sequence_len(s)))
+			s += skip;
+		if (!*s)
+			break;
+		len++;
+		s++;
+	}
+	return len;
+}
+
+size_t strlen_ansi(const char *s)
+{
+	size_t len = 0;
+	for (;;) {
+		size_t skip;
+
+		while ((skip = display_mode_esc_sequence_len(s))) {
+			s += skip;
+			len += skip;
+		}
+		if (!*s)
+			break;
+		s++;
+	}
+	return len;
+}
+
 /*
  * Wrap the text, if necessary. The variable indent is the indent for the
  * first line, indent2 is the indent for all other lines.
diff --git a/utf8.h b/utf8.h
index 3c0ae76..fd4cfca 100644
--- a/utf8.h
+++ b/utf8.h
@@ -3,6 +3,8 @@
 
 typedef unsigned int ucs_char_t;  /* assuming 32bit int */
 
+size_t strlen_no_ansi(const char *s);
+size_t strlen_ansi(const char *s);
 int utf8_width(const char **start, size_t *remainder_p);
 int utf8_strwidth(const char *string);
 int is_utf8(const char *text);
-- 
1.8.0.rc2.11.g2b79d01

Re: [PATCH UI experiment] diffstat: annotate/highlight new or removed files

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> diffstat does not show whether a file is added or deleted. I know
> --summary does. But the problem with --summary is it makes me look for
> information of a file in two places: diffstat and summary. And with a
> commit that adds/removes a lot, showing both --stat --summary can be
> long.
>
> This patch adds "(new)", "(gone)" or "(new mode)" to diffstat, with
> highlight, to easily catch file additions/removals. The extra text is
> chosen to be short enough so that it won't take up too much space for
> path names:
>
>  .gitignore                  |   1 +
>  Makefile                    |   3 +
>  t/t3070-wildmatch.sh (new)  | 188 ++++++++++++++++++++++++
>  t/t3070/wildtest.txt (gone) | 165 ---------------------
>  test-wildmatch.c (new)      |  14 ++
>  wildmatch.c                 |   5 +-
>  6 files changed, 210 insertions(+), 166 deletions(-)
>
> I don't put creation modes in there too because most of the time it
> does not matter much to me and I could look down to --summary for mode
> verification. But we could put "(new+x)" for 0755 and just "(new)" for
> 0644. "(new mode)" then could become "(+x)", "(-x)" or something like
> that.
>
> Coloring is to me an improvement over --summary. Probably the main
> point. Without it, perhaps it's not worth putting extra text to
> diffstat.

It is kind of surprising that you did not choose to paint new in
green and gone in red, and rather paint everything in yellow.

I personally think the above in monochrome is fairly easy to read;
with coloring, it might become too distracting, though.

Just a nit, "new mode" is too similar to "new".  Everything is "new"
in the sense that they have "new contents"; it may be better phrased
without saying "new" but giving a stress on "changed".

Thanks for a fun patch.  I am not strongly against it.

Re: [PATCH UI experiment] diffstat: annotate/highlight new or removed files

[Nguyen Thai Ngoc Duy]

On Tue, Oct 16, 2012 at 4:03 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Coloring is to me an improvement over --summary. Probably the main
>> point. Without it, perhaps it's not worth putting extra text to
>> diffstat.
>
> It is kind of surprising that you did not choose to paint new in
> green and gone in red, and rather paint everything in yellow.

The colors of line addition and deletion? Nice. I just wanted to make
sure these stand out, or at least not easily mistaken as part of path
names.

> I personally think the above in monochrome is fairly easy to read;
> with coloring, it might become too distracting, though.

Hmm.. maybe. I'm probably too excited to see the distraction just yet.
I will try it out for longer time, see if I change my mind.

> Just a nit, "new mode" is too similar to "new".  Everything is "new"
> in the sense that they have "new contents"; it may be better phrased
> without saying "new" but giving a stress on "changed".

I think "new mode" should be replaced by the actual mode change (e.g.
"+x", "-x", or "mode +x", "mode -x"). Not too long and quite clear
what it does.
-- 
Duy