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

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

[Zbigniew Jędrzejewski-Szmek]

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.

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

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

[Alexander Strasser]

Hi,

Zbigniew Jędrzejewski-Szmek wrote:
> On 06/07/2012 02:21 PM, Alexander Strasser wrote:
> >   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.

  thank you very much for the review.

> >   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.

  Ah, I see. I will try to do better next time. Thanks for the good
explanation.

> >   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.

  Yes, seems so. I was seeing changing line counts in GitStats output
compared to older and newer Git versions. I found the exact commit with
"git bisect" which was a big help.

> > Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
> Small note: normally the paragraphs are not indented.

  Noted. I probably should have also dropped the () in the subject. After
submitting I noticed this notation was not used in analog log messages.

[...]
> > --- 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
> '

  Should I rewrite the test for this patch? Or should it be changed for the
whole file at once?

[...]

  Alexander

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

[Junio C Hamano]

Alexander Strasser <eclipse7@gmx.net> writes:

Administrivia.

Please do not use Mail-Followup-To header to deflect direct response
to you away to other people.  When I want to reply to you and Cc
others, I do not want to see other people's name on To field for me
to edit and correct, and when somebody else wants to reply to you, I
do not want to see my name on its To line, as such a message may not
be of immediate interest for me.

> Zbigniew Jędrzejewski-Szmek wrote:
> ...
>> >   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.

Correction.  It is not "is intended for merging", but only when it
is *ready* to be merged, when stakeholders are happy with the patch.

>> So basically taking the address list from the discussion of e18872b
>> would be the simplest and most effective choice.

>   Yes, seems so. I was seeing changing line counts in GitStats output
> compared to older and newer Git versions. I found the exact commit with
> "git bisect" which was a big help.

Thanks.

>> > Signed-off-by: Alexander Strasser <eclipse7@gmx.net>
>> Small note: normally the paragraphs are not indented.
>
>   Noted. I probably should have also dropped the () in the subject. After
> submitting I noticed this notation was not used in analog log messages.
>
> [...]
>> > --- 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
>> '
>
>   Should I rewrite the test for this patch? Or should it be changed for the
> whole file at once?

Please keep a bugfix patch to only fixes with tests.  Style fixes
should be done later after dust from more important changes (e.g. a
bugfix) settles.

Thanks.

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

[Zbigniew Jędrzejewski-Szmek]

On 06/07/2012 10:29 PM, Junio C Hamano wrote:
>>>> >> > --- 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
>>> >> '
>> >
>> >   Should I rewrite the test for this patch? Or should it be changed for the
>> > whole file at once?
> Please keep a bugfix patch to only fixes with tests.  Style fixes
> should be done later after dust from more important changes (e.g. a
> bugfix) settles.
> 
> Thanks.
Does this need a v2?

Zbyszek

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

[Junio C Hamano]

Zbigniew Jędrzejewski-Szmek  <zbyszek@in.waw.pl> writes:

> On 06/07/2012 10:29 PM, Junio C Hamano wrote:
> ...
>> Please keep a bugfix patch to only fixes with tests.  Style fixes
>> should be done later after dust from more important changes (e.g. a
>> bugfix) settles.
>> 
>> Thanks.
>
> Does this need a v2?
>
> Zbyszek

That is a question to be asked with Alex on the To: line, not me, I
would think.  I saw "Ah, I see. I will try to do better next time."
in his response to your review, but haven't seen the "next time"
reroll yet.