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

Alexander Strasser [off-list ref] 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:
...

quoted

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.

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

quoted

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.

quoted

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

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?

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.