Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()]
Hi,
Zbigniew Jędrzejewski-Szmek wrote:
Show 5 quoted lines
> 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.
Show 6 quoted lines
> > 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.
Show 6 quoted lines
> > 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.
[...]
Show 26 quoted lines
> > --- 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
Alexander Strasser <eclipse7@gmx.net>