Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()]

From: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
Date: Jun 7, 2012, 19:05 UTC
Message-ID: <4FD0FB75.4090906@in.waw.pl>
In-Reply-To: <20120607122149.GA3070@akuma>

On 06/07/2012 02:21 PM, Alexander Strasser wrote:

> Hello Zbigniew,
> 
>   could you have a look at the patch below? I submitted to it to the
> Git mailing list and you could probably comment there?

Hi Alexander, sure, thanks for finding (and fixing) the bug.

>   I think I should have put you in CC. But I am not so sure about
> Git patch submission policies.

The policy is to CC everyone who might be interested, and also to add TO:gitster@pobox.com, if the patch is intended for merging, as yours is. So basically taking the address list from the discussion of e18872b would be the simplest and most effective choice.

>   Do not mix byte and line counts. Binary files have byte counts;
> skip them when accumulating line insertions/deletions.
> 
>   The regression was introduced in e18872b.

Yeah, it seems that the condition for !binary was lost in the refactoring of the code.

> Signed-off-by: Alexander Strasser <eclipse7@gmx.net>

Small note: normally the paragraphs are not indented.

Show 40 quoted lines

> ---
> 
>   I hope this does retain the original intent of e18872b while
> not messing up the insertions/deletions output by --shortstat.
> 
>   Output of --stat was never affected AFAICT.
> 
>  diff.c                 | 2 +-
>  t/t4012-diff-binary.sh | 8 ++++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/diff.c b/diff.c
> index 77edd50..1a594df 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1700,7 +1700,7 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option
>  			continue;
>  		if (!data->files[i]->is_renamed && (added + deleted == 0)) {
>  			total_files--;
> -		} else {
> +		} else if (!data->files[i]->is_binary) { /* don't count bytes */
>  			adds += added;
>  			dels += deleted;
>  		}
> diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh
> index 8b4e80d..1a994f0 100755
> --- a/t/t4012-diff-binary.sh
> +++ b/t/t4012-diff-binary.sh
> @@ -36,6 +36,14 @@ test_expect_success '"apply --stat" output for binary file change' '
>  	test_i18ncmp expected current
>  '
>  
> +cat > expected <<\EOF
> + 4 files changed, 2 insertions(+), 2 deletions(-)
> +EOF
> +test_expect_success 'diff with --shortstat' '
> +	git diff --shortstat >current &&
> +	test_cmp expected current
> +'
> +

The test is OK, and follows the style of surrounding tests, but current
style is slightly different:
- no space after '>'
- expected output is inlined if it is short
- test_i18ncmp is used, even if the message is not yet i18n-ized

Something like this:
test_expect_success 'diff --shortstat output for binary file change' '
	echo " 4 files changed, 2 insertions(+), 2 deletions(-)" >expect &&
	git diff --shortstat >current &&
	test_i18ncmp expect current
'

>  test_expect_success 'apply --numstat notices binary file change' '
>  	git diff >diff &&
>  	git apply --numstat <diff >current &&

Zbyszek