* Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()] [not found] <20120607122149.GA3070@akuma> @ 2012-06-07 19:05 ` Zbigniew Jędrzejewski-Szmek 2012-06-07 20:04 ` Alexander Strasser 0 siblings, 1 reply; 5+ messages in thread From: Zbigniew Jędrzejewski-Szmek @ 2012-06-07 19:05 UTC (permalink / raw) To: Alexander Strasser, git, Junio C Hamano; +Cc: mj, Johannes Sixt On 06/07/2012 02:21 PM, Alexander Strasser wrote: > Hello Zbigniew, > > 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. > 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. > 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. > Signed-off-by: Alexander Strasser <eclipse7@gmx.net> Small note: normally the paragraphs are not indented. > --- > > I hope this does retain the original intent of e18872b while > not messing up the insertions/deletions output by --shortstat. > > Output of --stat was never affected AFAICT. > > diff.c | 2 +- > t/t4012-diff-binary.sh | 8 ++++++++ > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/diff.c b/diff.c > index 77edd50..1a594df 100644 > --- a/diff.c > +++ b/diff.c > @@ -1700,7 +1700,7 @@ static void show_shortstats(struct diffstat_t *data, struct diff_options *option > continue; > if (!data->files[i]->is_renamed && (added + deleted == 0)) { > total_files--; > - } else { > + } else if (!data->files[i]->is_binary) { /* don't count bytes */ > adds += added; > dels += deleted; > } > diff --git a/t/t4012-diff-binary.sh b/t/t4012-diff-binary.sh > index 8b4e80d..1a994f0 100755 > --- 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 ' > test_expect_success 'apply --numstat notices binary file change' ' > git diff >diff && > git apply --numstat <diff >current && Zbyszek ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()] 2012-06-07 19:05 ` [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()] Zbigniew Jędrzejewski-Szmek @ 2012-06-07 20:04 ` Alexander Strasser 2012-06-07 20:29 ` Junio C Hamano 0 siblings, 1 reply; 5+ messages in thread From: Alexander Strasser @ 2012-06-07 20:04 UTC (permalink / raw) To: Zbigniew Jędrzejewski-Szmek; +Cc: git, Junio C Hamano, mj, Johannes Sixt 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@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. > > 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. [...] > > --- 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()] 2012-06-07 20:04 ` Alexander Strasser @ 2012-06-07 20:29 ` Junio C Hamano 2012-06-14 19:07 ` Zbigniew Jędrzejewski-Szmek 0 siblings, 1 reply; 5+ messages in thread From: Junio C Hamano @ 2012-06-07 20:29 UTC (permalink / raw) To: Alexander Strasser Cc: Zbigniew Jędrzejewski-Szmek, git, mj, Johannes Sixt Alexander Strasser <eclipse7@gmx.net> 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: > ... >> > 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. >> 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. >> > 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. > > [...] >> > --- 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()] 2012-06-07 20:29 ` Junio C Hamano @ 2012-06-14 19:07 ` Zbigniew Jędrzejewski-Szmek 2012-06-14 20:28 ` Junio C Hamano 0 siblings, 1 reply; 5+ messages in thread From: Zbigniew Jędrzejewski-Szmek @ 2012-06-14 19:07 UTC (permalink / raw) To: Junio C Hamano; +Cc: Alexander Strasser, git, mj, Johannes Sixt On 06/07/2012 10:29 PM, Junio C Hamano wrote: >>>> >> > --- 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. Does this need a v2? Zbyszek ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()] 2012-06-14 19:07 ` Zbigniew Jędrzejewski-Szmek @ 2012-06-14 20:28 ` Junio C Hamano 0 siblings, 0 replies; 5+ messages in thread From: Junio C Hamano @ 2012-06-14 20:28 UTC (permalink / raw) To: Zbigniew Jędrzejewski-Szmek Cc: Alexander Strasser, git, mj, Johannes Sixt Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes: > On 06/07/2012 10:29 PM, Junio C Hamano wrote: > ... >> 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. > > Does this need a v2? > > Zbyszek That is a question to be asked with Alex on the To: line, not me, I would think. I saw "Ah, I see. I will try to do better next time." in his response to your review, but haven't seen the "next time" reroll yet. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-06-14 20:28 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20120607122149.GA3070@akuma> 2012-06-07 19:05 ` [eclipse7@gmx.net: [PATCH] diff: Only count lines in show_shortstats()] Zbigniew Jędrzejewski-Szmek 2012-06-07 20:04 ` Alexander Strasser 2012-06-07 20:29 ` Junio C Hamano 2012-06-14 19:07 ` Zbigniew Jędrzejewski-Szmek 2012-06-14 20:28 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).