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

> Michael J Gruber <git@drmicha.warpmail.net> 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 <git@drmicha.warpmail.net>
>> ---
>> 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?

> 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);