{"thread":{"id":"30740","subject":"Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()]","startedAt":"2012-06-07T19:05:25Z","lastAt":"2012-06-14T20:28:16Z","messageCount":5,"participants":["Zbigniew Jędrzejewski-Szmek","Alexander Strasser","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"193099","messageId":"4FD0FB75.4090906@in.waw.pl","threadId":"30740","inReplyTo":"20120607122149.GA3070@akuma","subject":"Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()]","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-06-07T19:05:25Z","receivedAt":"2012-06-07T19:05:25Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"On 06/07/2012 02:21 PM, Alexander Strasser wrote:\n> Hello Zbigniew,\n> \n> could you have a look at the patch below? I submitted to it to the\n> Git mailing list and you could probably comment there?\nHi Alexander,\nsure, thanks for finding (and fixing) the bug.\n\n> I think I should have put you in CC. But I am not so sure about\n> Git patch submission policies.\nThe policy is to CC everyone who might be interested, and also to add\nTO:gitster@pobox.com, if the patch is intended for merging, as yours is.\nSo basically taking the address list from the discussion of e18872b\nwould be the simplest and most effective choice.\n\n> Do not mix byte and line counts. Binary files have byte counts;\n> skip them when accumulating line insertions/deletions.\n> \n> The regression was introduced in e18872b.\nYeah, it seems that the condition for !binary was lost in the refactoring\nof the code.\n\n> Signed-off-by: Alexander Strasser \nSmall note: normally the paragraphs are not indented.\n\n> ---\n> \n> I hope this does retain the original intent of e18872b while\n> not messing up the insertions/deletions output by --shortstat.\n> \n> Output of --stat was never affected AFAICT.\n> \n> diff.c | 2 +-\n> t/t4012-diff-binary.sh | 8 ++++++++\n> 2 files changed, 9 insertions(+), 1 deletion(-)\n> \n> diff --git a/diff.c b/diff.c\n> index 77edd50..1a594df 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1700,7 +1700,7 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option\n> \t\t\tcontinue;\n> \t\tif (!data->files[i]->is_renamed && (added + deleted == 0)) {\n> \t\t\ttotal_files--;\n> -\t\t} else {\n> +\t\t} else if (!data->files[i]->is_binary) { /* don't count bytes */\n> \t\t\tadds += added;\n> \t\t\tdels += deleted;\n> \t\t}\n> diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh\n> index 8b4e80d..1a994f0 100755\n> --- a/t/t4012-diff-binary.sh\n> +++ b/t/t4012-diff-binary.sh\n> @@ -36,6 +36,14 @@ test_expect_success '\"apply --stat\" output for binary file change' '\n> \ttest_i18ncmp expected current\n> '\n> \n> +cat > expected <<\\EOF\n> + 4 files changed, 2 insertions(+), 2 deletions(-)\n> +EOF\n> +test_expect_success 'diff with --shortstat' '\n> +\tgit diff --shortstat >current &&\n> +\ttest_cmp expected current\n> +'\n> +\nThe test is OK, and follows the style of surrounding tests, but current\nstyle is slightly different:\n- no space after '>'\n- expected output is inlined if it is short\n- test_i18ncmp is used, even if the message is not yet i18n-ized\n\nSomething like this:\ntest_expect_success 'diff --shortstat output for binary file change' '\n\techo \" 4 files changed, 2 insertions(+), 2 deletions(-)\" >expect &&\n\tgit diff --shortstat >current &&\n\ttest_i18ncmp expect current\n'\n\n> test_expect_success 'apply --numstat notices binary file change' '\n> \tgit diff >diff &&\n> \tgit apply --numstat current &&\n\nZbyszek\n"},{"id":"193104","messageId":"20120607200434.GA2965@akuma","threadId":"30740","inReplyTo":"4FD0FB75.4090906@in.waw.pl","subject":"Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()]","fromName":"Alexander Strasser","fromEmail":"eclipse7@gmx.net","sentAt":"2012-06-07T20:04:34Z","receivedAt":"2012-06-07T20:04:34Z","isPatch":true,"sender":{"key":"eclipse7@gmx.net","avatar":"https://avatars.githubusercontent.com/u/4342576?v=4"},"body":"Hi,\n\nZbigniew Jędrzejewski-Szmek wrote:\n> On 06/07/2012 02:21 PM, Alexander Strasser wrote:\n> > could you have a look at the patch below? I submitted to it to the\n> > Git mailing list and you could probably comment there?\n> Hi Alexander,\n> sure, thanks for finding (and fixing) the bug.\n\n thank you very much for the review.\n\n> > I think I should have put you in CC. But I am not so sure about\n> > Git patch submission policies.\n> The policy is to CC everyone who might be interested, and also to add\n> TO:gitster@pobox.com, if the patch is intended for merging, as yours is.\n> So basically taking the address list from the discussion of e18872b\n> would be the simplest and most effective choice.\n\n Ah, I see. I will try to do better next time. Thanks for the good\nexplanation.\n\n> > Do not mix byte and line counts. Binary files have byte counts;\n> > skip them when accumulating line insertions/deletions.\n> > \n> > The regression was introduced in e18872b.\n> Yeah, it seems that the condition for !binary was lost in the refactoring\n> of the code.\n\n Yes, seems so. I was seeing changing line counts in GitStats output\ncompared to older and newer Git versions. I found the exact commit with\n\"git bisect\" which was a big help.\n\n> > Signed-off-by: Alexander Strasser \n> Small note: normally the paragraphs are not indented.\n\n Noted. I probably should have also dropped the () in the subject. After\nsubmitting I noticed this notation was not used in analog log messages.\n\n[...]\n> > --- a/t/t4012-diff-binary.sh\n> > +++ b/t/t4012-diff-binary.sh\n> > @@ -36,6 +36,14 @@ test_expect_success '\"apply --stat\" output for binary file change' '\n> > \ttest_i18ncmp expected current\n> > '\n> > \n> > +cat > expected <<\\EOF\n> > + 4 files changed, 2 insertions(+), 2 deletions(-)\n> > +EOF\n> > +test_expect_success 'diff with --shortstat' '\n> > +\tgit diff --shortstat >current &&\n> > +\ttest_cmp expected current\n> > +'\n> > +\n> The test is OK, and follows the style of surrounding tests, but current\n> style is slightly different:\n> - no space after '>'\n> - expected output is inlined if it is short\n> - test_i18ncmp is used, even if the message is not yet i18n-ized\n> \n> Something like this:\n> test_expect_success 'diff --shortstat output for binary file change' '\n> \techo \" 4 files changed, 2 insertions(+), 2 deletions(-)\" >expect &&\n> \tgit diff --shortstat >current &&\n> \ttest_i18ncmp expect current\n> '\n\n Should I rewrite the test for this patch? Or should it be changed for the\nwhole file at once?\n\n[...]\n\n Alexander\n"},{"id":"193105","messageId":"7vk3zig92n.fsf@alter.siamese.dyndns.org","threadId":"30740","inReplyTo":"20120607200434.GA2965@akuma","subject":"Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-07T20:29:04Z","receivedAt":"2012-06-07T20:29:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexander Strasser writes:\n\nAdministrivia.\n\nPlease do not use Mail-Followup-To header to deflect direct response\nto you away to other people. When I want to reply to you and Cc\nothers, I do not want to see other people's name on To field for me\nto edit and correct, and when somebody else wants to reply to you, I\ndo not want to see my name on its To line, as such a message may not\nbe of immediate interest for me.\n\n> Zbigniew Jędrzejewski-Szmek wrote:\n> ...\n>> > I think I should have put you in CC. But I am not so sure about\n>> > Git patch submission policies.\n>> The policy is to CC everyone who might be interested, and also to add\n>> TO:gitster@pobox.com, if the patch is intended for merging, as yours is.\n\nCorrection. It is not \"is intended for merging\", but only when it\nis *ready* to be merged, when stakeholders are happy with the patch.\n\n>> So basically taking the address list from the discussion of e18872b\n>> would be the simplest and most effective choice.\n\n> Yes, seems so. I was seeing changing line counts in GitStats output\n> compared to older and newer Git versions. I found the exact commit with\n> \"git bisect\" which was a big help.\n\nThanks.\n\n>> > Signed-off-by: Alexander Strasser \n>> Small note: normally the paragraphs are not indented.\n>\n> Noted. I probably should have also dropped the () in the subject. After\n> submitting I noticed this notation was not used in analog log messages.\n>\n> [...]\n>> > --- a/t/t4012-diff-binary.sh\n>> > +++ b/t/t4012-diff-binary.sh\n>> > @@ -36,6 +36,14 @@ test_expect_success '\"apply --stat\" output for binary file change' '\n>> > \ttest_i18ncmp expected current\n>> > '\n>> > \n>> > +cat > expected <<\\EOF\n>> > + 4 files changed, 2 insertions(+), 2 deletions(-)\n>> > +EOF\n>> > +test_expect_success 'diff with --shortstat' '\n>> > +\tgit diff --shortstat >current &&\n>> > +\ttest_cmp expected current\n>> > +'\n>> > +\n>> The test is OK, and follows the style of surrounding tests, but current\n>> style is slightly different:\n>> - no space after '>'\n>> - expected output is inlined if it is short\n>> - test_i18ncmp is used, even if the message is not yet i18n-ized\n>> \n>> Something like this:\n>> test_expect_success 'diff --shortstat output for binary file change' '\n>> \techo \" 4 files changed, 2 insertions(+), 2 deletions(-)\" >expect &&\n>> \tgit diff --shortstat >current &&\n>> \ttest_i18ncmp expect current\n>> '\n>\n> Should I rewrite the test for this patch? Or should it be changed for the\n> whole file at once?\n\nPlease keep a bugfix patch to only fixes with tests. Style fixes\nshould be done later after dust from more important changes (e.g. a\nbugfix) settles.\n\nThanks.\n"},{"id":"193670","messageId":"4FDA3668.3000900@in.waw.pl","threadId":"30740","inReplyTo":"7vk3zig92n.fsf@alter.siamese.dyndns.org","subject":"Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()]","fromName":"Zbigniew Jędrzejewski-Szmek","fromEmail":"zbyszek@in.waw.pl","sentAt":"2012-06-14T19:07:20Z","receivedAt":"2012-06-14T19:07:20Z","isPatch":true,"sender":{"key":"zbyszek@in.waw.pl","avatar":"https://avatars.githubusercontent.com/u/349618?v=4"},"body":"On 06/07/2012 10:29 PM, Junio C Hamano wrote:\n>>>> >> > --- a/t/t4012-diff-binary.sh\n>>>> >> > +++ b/t/t4012-diff-binary.sh\n>>>> >> > @@ -36,6 +36,14 @@ test_expect_success '\"apply --stat\" output for binary file change' '\n>>>> >> > \ttest_i18ncmp expected current\n>>>> >> > '\n>>>> >> > \n>>>> >> > +cat > expected <<\\EOF\n>>>> >> > + 4 files changed, 2 insertions(+), 2 deletions(-)\n>>>> >> > +EOF\n>>>> >> > +test_expect_success 'diff with --shortstat' '\n>>>> >> > +\tgit diff --shortstat >current &&\n>>>> >> > +\ttest_cmp expected current\n>>>> >> > +'\n>>>> >> > +\n>>> >> The test is OK, and follows the style of surrounding tests, but current\n>>> >> style is slightly different:\n>>> >> - no space after '>'\n>>> >> - expected output is inlined if it is short\n>>> >> - test_i18ncmp is used, even if the message is not yet i18n-ized\n>>> >> \n>>> >> Something like this:\n>>> >> test_expect_success 'diff --shortstat output for binary file change' '\n>>> >> \techo \" 4 files changed, 2 insertions(+), 2 deletions(-)\" >expect &&\n>>> >> \tgit diff --shortstat >current &&\n>>> >> \ttest_i18ncmp expect current\n>>> >> '\n>> >\n>> > Should I rewrite the test for this patch? Or should it be changed for the\n>> > whole file at once?\n> Please keep a bugfix patch to only fixes with tests. Style fixes\n> should be done later after dust from more important changes (e.g. a\n> bugfix) settles.\n> \n> Thanks.\nDoes this need a v2?\n\nZbyszek\n"},{"id":"193673","messageId":"7vaa05eizj.fsf@alter.siamese.dyndns.org","threadId":"30740","inReplyTo":"4FDA3668.3000900@in.waw.pl","subject":"Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()]","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-14T20:28:16Z","receivedAt":"2012-06-14T20:28:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Zbigniew Jędrzejewski-Szmek writes:\n\n> On 06/07/2012 10:29 PM, Junio C Hamano wrote:\n> ...\n>> Please keep a bugfix patch to only fixes with tests. Style fixes\n>> should be done later after dust from more important changes (e.g. a\n>> bugfix) settles.\n>> \n>> Thanks.\n>\n> Does this need a v2?\n>\n> Zbyszek\n\nThat is a question to be asked with Alex on the To: line, not me, I\nwould think. I saw \"Ah, I see. I will try to do better next time.\"\nin his response to your review, but haven't seen the \"next time\"\nreroll yet.\n"}]}