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

Hi,

Zbigniew Jędrzejewski-Szmek wrote:

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

quoted

  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.

quoted

  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.

quoted

  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.

quoted

Signed-off-by: Alexander Strasser <redacted>

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.

[...]

quoted

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