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@xxxxxxxxx, 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@xxxxxxx> > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html