Re: [PATCH 3/4] diff --stat: report chmoded binary files like text files
Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
Show 31 quoted lines
> 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.
Show 6 quoted lines
> @@ -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?
Show 5 quoted lines
> @@ -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(-)
Show changes to 2 files +24 −22
diff.c, t/t4006-diff-mode.sh
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
'
Zbigniew Jędrzejewski-Szmek
