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

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

[Markus Heidelberg]

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

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

[Markus Heidelberg]

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

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

[Markus Heidelberg]

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

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

[Jeff King]

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

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

[Junio C Hamano]

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

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

[Markus Heidelberg]

Junio C Hamano, 25.04.2009:
> 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