Re: [PATCHv3 1/3] diff.c: omit hidden entries from namelen calculation with --stat

Michael J Gruber [off-list ref] writes:

Currently, --stat calculates the longest name from all items but then
drops some (mode changes) from the output later on.

Instead, drop them from the namelen generation and calculation.

Signed-off-by: Michael J Gruber <redacted>
---
This optimizes (tightens) the display potentially, but we never had tests
which are sensitive to that.

More importantly I think this is a good first step in the right direction
to reduce the duplicated code that need to implement the same logic.

However, you missed another instance in show_shortstats() no?

I am wondering if it would make it easier to both read and maintain if you
add a boolean field "changed" to "struct diffstat_file", and set it at the
end of builtin_diffstat(), and have these places all check that field.

A possible alternative is not to put such an unchanged entry in the struct
diffstat_t in the first place so that nobody has to worry about it. Right
now, show_numstat() does show 0/0 entries (i.e. only mode change), while
shortstat and normal stat do not, but I somehow have a feeling that this
difference is not by design but by accident.  Besides, --numstat that only
says 0/0 is not useful in practice without --summary.

    $ edit diff.c
    $ chmod +x Makefile
    $ git diff --stat
     diff.c |   26 ++++++++++++++------------
    $ git diff --numstat
    0       0       Makefile
    14      12      diff.c
    $ git diff --numstat --summary
    0       0       Makefile
    14      12      diff.c
     mode change 100644 => 100755 Makefile

Getting rid of an unchanged entry from the diffstat array upfront will
change the behaviour of numstat, but I suspect that it change things in a
better way, making the result consistent with the textual version.

The patch below, diff.c shows that approach, while ndiff.c shows the extra
boolean "changed" approach.

What do you think?

 diff.c            |    7 +++++++
 diff.c => ndiff.c |   26 ++++++++++++++------------
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/diff.c b/diff.c
index 8f4815b..e2ee70a 100644
--- a/diff.c
+++ b/diff.c

@@ -2267,6 +2267,13 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 			      &xpp, &xecfg);
 	}
 
+	if (!data->is_renamed && !data->added && !data->deleted) {
+		diffstat->nr--;
+		free(data->name);
+		free(data->from_name);
+		free(data);
+	}
+
 	diff_free_filespec_data(one);
 	diff_free_filespec_data(two);
 }

diff --git a/diff.c b/ndiff.c
similarity index 99%
copy from diff.c
copy to ndiff.c
index 8f4815b..1620053 100644
--- a/diff.c
+++ b/ndiff.c

@@ -1222,6 +1222,7 @@ struct diffstat_t {
 		unsigned is_unmerged:1;
 		unsigned is_binary:1;
 		unsigned is_renamed:1;
+		unsigned changed:1;
 		uintmax_t added, deleted;
 	} **files;
 };

@@ -1350,6 +1351,9 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 	for (i = 0; i < data->nr; i++) {
 		struct diffstat_file *file = data->files[i];
 		uintmax_t change = file->added + file->deleted;
+
+		if (!file->changed)
+			continue;
 		fill_print_name(file);
 		len = strlen(file->print_name);
 		if (max_len < len)

@@ -1381,6 +1385,11 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 		uintmax_t deleted = data->files[i]->deleted;
 		int name_len;
 
+		if (!data->files[i]->changed) {
+			total_files--;
+			continue;
+		}
+
 		/*
 		 * "scale" the filename
 		 */

@@ -1415,11 +1424,6 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 			fprintf(options->file, "  Unmerged\n");
 			continue;
 		}
-		else if (!data->files[i]->is_renamed &&
-			 (added + deleted == 0)) {
-			total_files--;
-			continue;
-		}
 
 		/*
 		 * scale the add/delete

@@ -1457,15 +1461,12 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option
 	for (i = 0; i < data->nr; i++) {
 		if (!data->files[i]->is_binary &&
 		    !data->files[i]->is_unmerged) {
-			int added = data->files[i]->added;
-			int deleted= data->files[i]->deleted;
-			if (!data->files[i]->is_renamed &&
-			    (added + deleted == 0)) {
+			if (!data->files[i]->changed) {
 				total_files--;
-			} else {
-				adds += added;
-				dels += deleted;
+				continue;
 			}
+			adds += data->files[i]->added;
+			dels += data->files[i]->deleted;
 		}
 	}
 	if (options->output_prefix) {

@@ -2266,6 +2267,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 		xdi_diff_outf(&mf1, &mf2, diffstat_consume, diffstat,
 			      &xpp, &xecfg);
 	}
+	data->changed = data->is_renamed || data->added || data->deleted;
 
 	diff_free_filespec_data(one);
 	diff_free_filespec_data(two);

Junio C Hamano venit, vidit, dixit 27.05.2011 19:43:

Michael J Gruber [off-list ref] writes:

quoted

Currently, --stat calculates the longest name from all items but then
drops some (mode changes) from the output later on.

Instead, drop them from the namelen generation and calculation.

Signed-off-by: Michael J Gruber <redacted>
---
This optimizes (tightens) the display potentially, but we never had tests
which are sensitive to that.

More importantly I think this is a good first step in the right direction
to reduce the duplicated code that need to implement the same logic.

However, you missed another instance in show_shortstats() no?

I'm sorry I don't understand this remark. For all different formats, the
respective functions show_stat(), show_shortstats() etc. do their own
formatting, and I did not change this at all. So, what did I miss?

Also, show_shortstats() does not need the names, so what similar change
should one have made there?

I am wondering if it would make it easier to both read and maintain if you
add a boolean field "changed" to "struct diffstat_file", and set it at the
end of builtin_diffstat(), and have these places all check that field.

Sounds fine.

A possible alternative is not to put such an unchanged entry in the struct
diffstat_t in the first place so that nobody has to worry about it. Right
now, show_numstat() does show 0/0 entries (i.e. only mode change), while
shortstat and normal stat do not, but I somehow have a feeling that this
difference is not by design but by accident.  Besides, --numstat that only
says 0/0 is not useful in practice without --summary.

    $ edit diff.c
    $ chmod +x Makefile
    $ git diff --stat
     diff.c |   26 ++++++++++++++------------
    $ git diff --numstat
    0       0       Makefile
    14      12      diff.c
    $ git diff --numstat --summary
    0       0       Makefile
    14      12      diff.c
     mode change 100644 => 100755 Makefile

Getting rid of an unchanged entry from the diffstat array upfront will
change the behaviour of numstat, but I suspect that it change things in a
better way, making the result consistent with the textual version.

That sounds good to me.

I don't know that codepath very well, but hasn't that diffstat been
added earlier in builtin_diffstat() already? But maybe the
diffstat->nr-- takes care of that.

Also, I have no idea how builtin_diffstat() influences show_stat() which
my patch was about, sorry.

or do this dance only here (with ...->changed) instead of in the first
loop, because in 2/3 (when limited by stat_count), the second loop would
be done all the way up to nr (as 2 separate loops), but the first one
only up to count.

Does show_stats() get the data as modified by builtin_diffstat()? Then
this is fine.

Again, I'm sorry I'm not seeing through the call chain here.

Michael