Re: [PATCH 3/4] diff --stat: report chmoded binary files like text files

[Junio C Hamano <gitster@pobox.com>]

Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:

> Binary files chmoded without content change were reported as if they
> were rewritten. At the same time, text files in the same situation
> were reported as "unchanged". Let's treat binary files like text files
> here, and simply say that they are unchanged.
>
> For text files, we knew that they were unchanged if the numbers of
> lines added and deleted were both 0. For binary files this metric does
> not make sense and is not calculated, so a new way of conveying this
> information is needed. A new flag is_unchanged is added in struct
> diffstat_t that is set if the contents of both files are identical.
> For consistency, this new flag is used both for text files and binary
> files.
>
> Output of --shortstat is modified in the same way.
>
> Reported-by: Martin Mareš <mj@ucw.cz>
> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
> ---
>  diff.c               |   28 +++++++++++++++++-----------
>  t/t4006-diff-mode.sh |    8 +-------
>  2 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 7da16c9..6eb2946 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1299,6 +1299,7 @@ struct diffstat_t {
>  		unsigned is_unmerged:1;
>  		unsigned is_binary:1;
>  		unsigned is_renamed:1;
> +		unsigned is_unchanged:1;

The name is somewhat misleading, as a filepair that consists of two blobs
with the same contents with different mode bits is still "changed", and
you are trying to say that they have the same contents.

> @@ -1471,7 +1472,7 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
>  		struct diffstat_file *file = data->files[i];
>  		uintmax_t change = file->added + file->deleted;
>  		if (!data->files[i]->is_renamed &&
> -			 (change == 0)) {
> +		    data->files[i]->is_unchanged) {

I am not sure if all these hunks are needed.  If you are going to show
only "  Bin\n" for a filepair with the same binary contents, perhaps it is
simpler to set added/deleted fields of such a filepair to 0?  Then most of
the hunks in this patch can disappear, no?

> @@ -2379,6 +2383,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
>  		return;
>  	}
>  
> +	data->is_unchanged = hashcmp(one->sha1, two->sha1) == 0;

Please write it as "!hashcmp(a, b)", not "hashcmp(a, b) == 0".

In any case, how about doing it like this instead?

 diff.c               |   38 +++++++++++++++++++++++---------------
 t/t4006-diff-mode.sh |    8 +-------
 2 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/diff.c b/diff.c
index 22288b0..338ef41 100644
--- a/diff.c
+++ b/diff.c
@@ -1583,8 +1583,12 @@ static void show_stats(struct diffstat_t *data, struct diff_options *options)
 		if (data->files[i]->is_binary) {
 			fprintf(options->file, "%s", line_prefix);
 			show_name(options->file, prefix, name, len);
-			fprintf(options->file, "  Bin ");
-			fprintf(options->file, "%s%"PRIuMAX"%s",
+			fprintf(options->file, "  Bin");
+			if (!added && !deleted) {
+				putc('\n', options->file);
+				continue;
+			}
+			fprintf(options->file, " %s%"PRIuMAX"%s",
 				del_c, deleted, reset);
 			fprintf(options->file, " -> ");
 			fprintf(options->file, "%s%"PRIuMAX"%s",
@@ -1657,17 +1661,16 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option
 		return;
 
 	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)) {
-				total_files--;
-			} else {
-				adds += added;
-				dels += deleted;
-			}
+		int added = data->files[i]->added;
+		int deleted= data->files[i]->deleted;
+
+		if (data->files[i]->is_unmerged)
+			continue;
+		if (!data->files[i]->is_renamed && (added + deleted == 0)) {
+			total_files--;
+		} else {
+			adds += added;
+			dels += deleted;
 		}
 	}
 	if (options->output_prefix) {
@@ -2377,8 +2380,13 @@ static void builtin_diffstat(const char *name_a, const char *name_b,
 
 	if (diff_filespec_is_binary(one) || diff_filespec_is_binary(two)) {
 		data->is_binary = 1;
-		data->added = diff_filespec_size(two);
-		data->deleted = diff_filespec_size(one);
+		if (!hashcmp(one->sha1, two->sha1)) {
+			data->added = 0;
+			data->deleted = 0;
+		} else {
+			data->added = diff_filespec_size(one);
+			data->deleted = diff_filespec_size(two);
+		}
 	}
 
 	else if (complete_rewrite) {
diff --git a/t/t4006-diff-mode.sh b/t/t4006-diff-mode.sh
index 392dfef..693bfc4 100755
--- a/t/t4006-diff-mode.sh
+++ b/t/t4006-diff-mode.sh
@@ -46,18 +46,12 @@ test_expect_success '--shortstat output after text chmod' '
 
 test_expect_success '--stat output after binary chmod' '
 	test_chmod +x binbin &&
-	cat >expect <<-EOF &&
-	 binbin |  Bin 1024 -> 1024 bytes
-	 1 file changed, 0 insertions(+), 0 deletions(-)
-	EOF
+	echo " 0 files changed" >expect &&
 	git diff HEAD --stat >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success '--shortstat output after binary chmod' '
-	cat >expect <<-EOF &&
-	 1 file changed, 0 insertions(+), 0 deletions(-)
-	EOF
 	git diff HEAD --shortstat >actual &&
 	test_cmp expect actual
 '