threads / discuss / 19012

text in --stat is colored like color.diff.plain

Subject: text in --stat is colored like color.diff.plain

## tl;dr

6 messages between Apr 22, 2009 and Apr 25, 2009.

replies: 5people: 3as markdown or json

Markus Heidelberg· Apr 22, 2009, 21:13 UTC · lore

I noticed, that the filenames and the text "X files changed, Y insertions(+), Z deletions(-)" in the --stat output ended with an ESC[m, although they were not colored. Further investigation showed, that these parts of the output indeed can use colors from color.diff.plain.

Is this intention? Commit 785f743 (diff --stat: color output.,
2006-09-26) introduced it, but only mentioned colored output of the
+++++++---- diffstat graph in the commit message. color.diff.plain is
documented for coloring context text and I don't think it looks good
either as it is now, nor is this really "context text".
Note: --shortstat and --summary don't use colors like --stat. Or is this
really intention and I don't see the reason for it?

If not, should we remove the colors for --stat? After that maybe we could do some more color.diff.old/new coloring for "Y insertions(+)", "Z deletions(-)", "create mode ...", "delete mode ..."

Markus
Markus Heidelberg· Apr 24, 2009, 22:06 UTC · re: Markus Heidelberg · lore

[PATCH 1/2] diff: do not color --stat output like patch context

The diffstat used the color.diff.plain slot (context text) for coloring filenames and the whole summary line. This didn't look nice and the affected text isn't patch context at all.

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---
    Maybe it's easier to begin with a patch :)
 diff.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/diff.c b/diff.c
index 3ac7168..d581d4d 100644
--- a/diff.c
+++ b/diff.c
@@ -839,10 +839,9 @@ 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,
-		      const char *reset, const char *set)
+		      const char *prefix, const char *name, int len)
 {
-	fprintf(file, " %s%s%-*s%s |", set, prefix, len, name, reset);
+	fprintf(file, " %s%-*s |", prefix, len, name);
 }
 
 static void show_graph(FILE *file, char ch, int cnt, const char *set, const char *reset)
@@ -956,7 +955,7 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)
 		}
 
 		if (data->files[i]->is_binary) {
-			show_name(options->file, prefix, name, len, reset, set);
+			show_name(options->file, prefix, name, len);
 			fprintf(options->file, "  Bin ");
 			fprintf(options->file, "%s%d%s", del_c, deleted, reset);
 			fprintf(options->file, " -> ");
@@ -966,7 +965,7 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)
 			continue;
 		}
 		else if (data->files[i]->is_unmerged) {
-			show_name(options->file, prefix, name, len, reset, set);
+			show_name(options->file, prefix, name, len);
 			fprintf(options->file, "  Unmerged\n");
 			continue;
 		}
@@ -988,7 +987,7 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)
 			add = scale_linear(add, width, max_change);
 			del = scale_linear(del, width, max_change);
 		}
-		show_name(options->file, prefix, name, len, reset, set);
+		show_name(options->file, prefix, name, len);
 		fprintf(options->file, "%5d%s", added + deleted,
 				added + deleted ? " " : "");
 		show_graph(options->file, '+', add, add_c, reset);
@@ -996,8 +995,8 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)
 		fprintf(options->file, "\n");
 	}
 	fprintf(options->file,
-	       "%s %d files changed, %d insertions(+), %d deletions(-)%s\n",
-	       set, total_files, adds, dels, reset);
+	       " %d files changed, %d insertions(+), %d deletions(-)\n",
+	       total_files, adds, dels);
 }
 
 static void show_shortstats(struct diffstat_t* data, struct diff_options *options)

-- 
1.6.3.rc1.84.g1036b
Markus Heidelberg· Apr 24, 2009, 22:06 UTC · re: Markus Heidelberg · lore

[PATCH 2/2] diff: color statistics (stat, shortstat, numstat)

stat/shortstat: Color added and removed lines and the corresponding signs ('+' and '-') in the summary.

numstat: Color added and removed lines per file.

Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---
    I didn't consider --summary and --name-status. Also in --stat it
    would be possible to color filenames in renames and copies and the
    mode in mode changes. Not sure if it would be nice or distracting.
    At least it wouldn't look so consistent, I think.
 diff.c |   26 ++++++++++++++++++++------
 1 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/diff.c b/diff.c
index d581d4d..bc8377d 100644
--- a/diff.c
+++ b/diff.c
@@ -994,18 +994,24 @@ static void show_stats(struct diffstat_t* data, struct diff_options *options)
 		show_graph(options->file, '-', del, del_c, reset);
 		fprintf(options->file, "\n");
 	}
-	fprintf(options->file,
-	       " %d files changed, %d insertions(+), %d deletions(-)\n",
-	       total_files, adds, dels);
+	fprintf(options->file, " %d files changed, %s%d%s insertions(%s+%s), "
+						  "%s%d%s deletions(%s-%s)\n",
+				total_files, add_c, adds, reset, add_c, reset,
+					     del_c, dels, reset, del_c, reset);
 }
 
 static void show_shortstats(struct diffstat_t* data, struct diff_options *options)
 {
 	int i, adds = 0, dels = 0, total_files = data->nr;
+	const char *reset, *add_c, *del_c;
 
 	if (data->nr == 0)
 		return;
 
+	reset = diff_get_color_opt(options, DIFF_RESET);
+	add_c = diff_get_color_opt(options, DIFF_FILE_NEW);
+	del_c = diff_get_color_opt(options, DIFF_FILE_OLD);
+
 	for (i = 0; i < data->nr; i++) {
 		if (!data->files[i]->is_binary &&
 		    !data->files[i]->is_unmerged) {
@@ -1020,17 +1026,24 @@ static void show_shortstats(struct diffstat_t* data, struct diff_options *option
 			}
 		}
 	}
-	fprintf(options->file, " %d files changed, %d insertions(+), %d deletions(-)\n",
-	       total_files, adds, dels);
+	fprintf(options->file, " %d files changed, %s%d%s insertions(%s+%s), "
+						  "%s%d%s deletions(%s-%s)\n",
+				total_files, add_c, adds, reset, add_c, reset,
+					     del_c, dels, reset, del_c, reset);
 }
 
 static void show_numstat(struct diffstat_t* data, struct diff_options *options)
 {
 	int i;
+	const char *reset, *add_c, *del_c;
 
 	if (data->nr == 0)
 		return;
 
+	reset = diff_get_color_opt(options, DIFF_RESET);
+	add_c = diff_get_color_opt(options, DIFF_FILE_NEW);
+	del_c = diff_get_color_opt(options, DIFF_FILE_OLD);
+
 	for (i = 0; i < data->nr; i++) {
 		struct diffstat_file *file = data->files[i];
 
@@ -1038,7 +1051,8 @@ static void show_numstat(struct diffstat_t* data, struct diff_options *options)
 			fprintf(options->file, "-\t-\t");
 		else
 			fprintf(options->file,
-				"%d\t%d\t", file->added, file->deleted);
+				"%s%d%s\t%s%d%s\t", add_c, file->added, reset,
+						    del_c, file->deleted, reset);
 		if (options->line_termination) {
 			fill_print_name(file);
 			if (!file->is_renamed)

-- 
1.6.3.rc1.84.g1036b
Jeff King· Apr 24, 2009, 22:26 UTC · re: Markus Heidelberg · lore

Re: [PATCH 2/2] diff: color statistics (stat, shortstat, numstat)

On Sat, Apr 25, 2009 at 12:06:48AM +0200, Markus Heidelberg wrote:
> numstat:
> Color added and removed lines per file.

Somehow I thought numstat was about being easy to parse for plumbing. Are people really using it for human-readable output?

-Peff
Junio C Hamano· Apr 24, 2009, 23:01 UTC · re: Markus Heidelberg · lore

Re: [PATCH 2/2] diff: color statistics (stat, shortstat, numstat)

Markus Heidelberg <markus.heidelberg@web.de> writes:
Show 6 quoted lines
> stat/shortstat:
> Color added and removed lines and the corresponding signs ('+' and '-')
> in the summary.
>
> numstat:
> Color added and removed lines per file.

I find this extremely unreadable. Also numstat being for porcelain use, I do not see the point.

I think [1/2] that removes the (plain)coloring of the stat summary text is Ok. The code is painting the stat summary in the same color as the filenames in the stat graph, and the default "plain" color happens to be "do not color--just use the terminal default", so it probably does not have any practical difference.

Markus Heidelberg· Apr 25, 2009, 11:09 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/2] diff: color statistics (stat, shortstat, numstat)

Junio C Hamano, 25.04.2009:
Show 10 quoted lines
> Markus Heidelberg <markus.heidelberg@web.de> writes:
> 
> > stat/shortstat:
> > Color added and removed lines and the corresponding signs ('+' and '-')
> > in the summary.
> >
> > numstat:
> > Color added and removed lines per file.
> 
> I find this extremely unreadable.

I know one can overdo it with colors, but I don't think it's the case for the stat/shortstat summary line here. Of course this is very subjective and if the consensus is not to have colors there, then it's OK.

> Also numstat being for porcelain use, I
> do not see the point.

It gives you an information that --stat doesn't give you: the exact count of added and removed lines per file. Also for long path names you don't have to bother with --stat=x,y

Having said this, I have to admit that I didn't use it myself up to now.
> I think [1/2] that removes the (plain)coloring of the stat summary text is
> Ok.

Do you mean the whole patch 1/2 is OK or only the summary part and the filenames should stay with plain coloring? I think the former.

> The code is painting the stat summary in the same color as the
> filenames in the stat graph, and the default "plain" color happens to be
> "do not color--just use the terminal default", so it probably does not
> have any practical difference.
Eh, yes, for people setting color.diff.plain

← back to recent threads