* Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()]
[not found] <20120607122149.GA3070@akuma>
@ 2012-06-07 19:05 ` Zbigniew Jędrzejewski-Szmek
2012-06-07 20:04 ` Alexander Strasser
0 siblings, 1 reply; 5+ messages in thread
From: Zbigniew Jędrzejewski-Szmek @ 2012-06-07 19:05 UTC (permalink / raw)
To: Alexander Strasser, git, Junio C Hamano; +Cc: mj, Johannes Sixt
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()]
2012-06-07 19:05 ` [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()] Zbigniew Jędrzejewski-Szmek
@ 2012-06-07 20:04 ` Alexander Strasser
2012-06-07 20:29 ` Junio C Hamano
0 siblings, 1 reply; 5+ messages in thread
From: Alexander Strasser @ 2012-06-07 20:04 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, Junio C Hamano, mj, Johannes Sixt
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()]
2012-06-07 20:04 ` Alexander Strasser
@ 2012-06-07 20:29 ` Junio C Hamano
2012-06-14 19:07 ` Zbigniew Jędrzejewski-Szmek
0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2012-06-07 20:29 UTC (permalink / raw)
To: Alexander Strasser
Cc: Zbigniew Jędrzejewski-Szmek, git, mj, Johannes Sixt
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()]
2012-06-07 20:29 ` Junio C Hamano
@ 2012-06-14 19:07 ` Zbigniew Jędrzejewski-Szmek
2012-06-14 20:28 ` Junio C Hamano
0 siblings, 1 reply; 5+ messages in thread
From: Zbigniew Jędrzejewski-Szmek @ 2012-06-14 19:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Alexander Strasser, git, mj, Johannes Sixt
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()]
2012-06-14 19:07 ` Zbigniew Jędrzejewski-Szmek
@ 2012-06-14 20:28 ` Junio C Hamano
0 siblings, 0 replies; 5+ messages in thread
From: Junio C Hamano @ 2012-06-14 20:28 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek
Cc: Alexander Strasser, git, mj, Johannes Sixt
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-06-14 20:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20120607122149.GA3070@akuma>
2012-06-07 19:05 ` [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()] Zbigniew Jędrzejewski-Szmek
2012-06-07 20:04 ` Alexander Strasser
2012-06-07 20:29 ` Junio C Hamano
2012-06-14 19:07 ` Zbigniew Jędrzejewski-Szmek
2012-06-14 20:28 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).