Re: [PATCH 3/4] diff --stat: report chmoded binary files like text files
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 1, 2012, 18:27 UTC
- Message-ID
- <7vvckf92pp.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1335892215-21331-4-git-send-email-zbyszek@in.waw.pl>
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(-)
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 '