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

["Zbigniew Jędrzejewski-Szmek" <zbyszek@in.waw.pl>]

On 05/01/2012 08:27 PM, Junio C Hamano wrote:
> 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?
Yeah, this is much nicer.

On top of this, 4/4 becomes:
-       else {
+       else if (hashcmp(one->sha1, two->sha1)) {
and the time improvement is the same (0.8 vs 2.0 s).

Do you want me to resend with your replacement patch?

Zbyszek