{"thread":{"id":"38669","subject":"[PATCH] diff --shortstat --dirstat: remove duplicate output","startedAt":"2015-02-28T13:19:27Z","lastAt":"2015-03-05T21:38:21Z","messageCount":13,"participants":["Mårten Kongstad","Johan Herland","Junio C Hamano","Torsten Bögershausen","Michael J Gruber","SZEDER Gábor","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"256776","messageId":"1425129567-18693-1-git-send-email-marten.kongstad@gmail.com","threadId":"38669","inReplyTo":null,"subject":"[PATCH] diff --shortstat --dirstat: remove duplicate output","fromName":"Mårten Kongstad","fromEmail":"marten.kongstad@gmail.com","sentAt":"2015-02-28T13:19:27Z","receivedAt":"2015-02-28T13:19:27Z","isPatch":true,"sender":{"key":"marten.kongstad@gmail.com","avatar":"https://avatars.githubusercontent.com/u/567558?v=4"},"body":"When --shortstat is used in conjunction with --dirstat=changes, git diff will\noutput the dirstat information twice: first as calculated by the 'lines'\nalgorithm, then as calculated by the 'changes' algorithm:\n\n    $ git diff --dirstat=changes,10 --shortstat v2.2.0..v2.2.1\n     23 files changed, 453 insertions(+), 54 deletions(-)\n      33.5% Documentation/RelNotes/\n      26.2% t/\n      46.6% Documentation/RelNotes/\n      16.6% t/\n\nThe same duplication happens for --shortstat together with --dirstat=files, but\nnot for --shortstat together with --dirstat=lines.\n\nLimit output to only include one dirstat part, calculated as specified\nby the --dirstat parameter. Also, add test for this.\n\nSigned-off-by: Mårten Kongstad <marten.kongstad@gmail.com>\n---\n diff.c                  |  2 +-\n t/t4047-diff-dirstat.sh | 52 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 53 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex d1bd534..abc32c8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4541,7 +4541,7 @@ void diff_flush(struct diff_options *options)\n \t\t\tshow_stats(&diffstat, options);\n \t\tif (output_format & DIFF_FORMAT_SHORTSTAT)\n \t\t\tshow_shortstats(&diffstat, options);\n-\t\tif (output_format & DIFF_FORMAT_DIRSTAT)\n+\t\tif (output_format & DIFF_FORMAT_DIRSTAT && dirstat_by_line)\n \t\t\tshow_dirstat_by_line(&diffstat, options);\n \t\tfree_diffstat_info(&diffstat);\n \t\tseparator++;\ndiff --git a/t/t4047-diff-dirstat.sh b/t/t4047-diff-dirstat.sh\nindex ed7e093..208760e 100755\n--- a/t/t4047-diff-dirstat.sh\n+++ b/t/t4047-diff-dirstat.sh\n@@ -973,4 +973,56 @@ test_expect_success 'diff.dirstat=future_param,0,lines should warn, but still wo\n \ttest_i18ngrep -q \"diff\\\\.dirstat\" actual_error\n '\n \n+cat <<EOF >expect_diff_shortstat_dirstat_changes\n+ 11 files changed, 62 insertions(+), 32 deletions(-)\n+  10.8% dst/copy/changed/\n+  10.8% dst/copy/rearranged/\n+  10.8% dst/copy/unchanged/\n+  10.8% dst/move/changed/\n+  10.8% dst/move/rearranged/\n+  10.8% dst/move/unchanged/\n+  10.8% src/move/changed/\n+  10.8% src/move/rearranged/\n+  10.8% src/move/unchanged/\n+EOF\n+\n+cat <<EOF >expect_diff_shortstat_dirstat_lines\n+ 11 files changed, 62 insertions(+), 32 deletions(-)\n+  10.6% dst/copy/changed/\n+  10.6% dst/copy/rearranged/\n+  10.6% dst/copy/unchanged/\n+  10.6% dst/move/changed/\n+  10.6% dst/move/rearranged/\n+  10.6% dst/move/unchanged/\n+  10.6% src/move/changed/\n+  10.6% src/move/rearranged/\n+  10.6% src/move/unchanged/\n+EOF\n+\n+cat <<EOF >expect_diff_shortstat_dirstat_files\n+ 11 files changed, 62 insertions(+), 32 deletions(-)\n+   9.0% changed/\n+   9.0% dst/copy/changed/\n+   9.0% dst/copy/rearranged/\n+   9.0% dst/copy/unchanged/\n+   9.0% dst/move/changed/\n+   9.0% dst/move/rearranged/\n+   9.0% dst/move/unchanged/\n+   9.0% rearranged/\n+   9.0% src/move/changed/\n+   9.0% src/move/rearranged/\n+   9.0% src/move/unchanged/\n+EOF\n+\n+test_expect_success '--shortstat --dirstat should output only one dirstat' '\n+\tgit diff --shortstat --dirstat=changes HEAD^..HEAD >actual_diff_shortstat_dirstat_changes &&\n+\ttest_cmp expect_diff_shortstat_dirstat_changes actual_diff_shortstat_dirstat_changes &&\n+\n+\tgit diff --shortstat --dirstat=lines HEAD^..HEAD >actual_diff_shortstat_dirstat_lines &&\n+\ttest_cmp expect_diff_shortstat_dirstat_lines actual_diff_shortstat_dirstat_lines &&\n+\n+\tgit diff --shortstat --dirstat=files HEAD^..HEAD >actual_diff_shortstat_dirstat_files &&\n+\ttest_cmp expect_diff_shortstat_dirstat_files actual_diff_shortstat_dirstat_files\n+'\n+\n test_done\n-- \n1.9.1\n"},{"id":"256777","messageId":"CALKQrgcozE4y6ZWezvaedQqhATaQCtrxUScq_etJVDdkuG55kw@mail.gmail.com","threadId":"38669","inReplyTo":"1425129567-18693-1-git-send-email-marten.kongstad@gmail.com","subject":"Re: [PATCH] diff --shortstat --dirstat: remove duplicate output","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2015-02-28T14:21:58Z","receivedAt":"2015-02-28T14:21:58Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Sat, Feb 28, 2015 at 2:19 PM, Mårten Kongstad\n<marten.kongstad@gmail.com> wrote:\n[...]\n> Signed-off-by: Mårten Kongstad <marten.kongstad@gmail.com>\n\nAcked-by: Johan Herland <johan@herland.net>\n\n-- \nJohan Herland, <johan@herland.net>\nwww.herland.net\n"},{"id":"256792","messageId":"xmqqegp9gyof.fsf@gitster.dls.corp.google.com","threadId":"38669","inReplyTo":"CALKQrgcozE4y6ZWezvaedQqhATaQCtrxUScq_etJVDdkuG55kw@mail.gmail.com","subject":"Re: [PATCH] diff --shortstat --dirstat: remove duplicate output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-01T03:54:24Z","receivedAt":"2015-03-01T03:54:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johan Herland <johan@herland.net> writes:\n\n> On Sat, Feb 28, 2015 at 2:19 PM, Mårten Kongstad\n> <marten.kongstad@gmail.com> wrote:\n> [...]\n>> Signed-off-by: Mårten Kongstad <marten.kongstad@gmail.com>\n>\n> Acked-by: Johan Herland <johan@herland.net>\n\nInteresting.  So nobody in real life uses --dirstat and --shortstat\ntogether?\n\nI am not very happy with the added tests that hardcode exact numbers\nthat are shown, as the counting algorithm can be improved.  Can't we\ndo better?\n\nThanks.\n"},{"id":"256796","messageId":"1425195546-15637-1-git-send-email-marten.kongstad@gmail.com","threadId":"38669","inReplyTo":"xmqqegp9gyof.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2] diff --shortstat --dirstat: remove duplicate output","fromName":"Mårten Kongstad","fromEmail":"marten.kongstad@gmail.com","sentAt":"2015-03-01T07:39:06Z","receivedAt":"2015-03-01T07:39:06Z","isPatch":true,"sender":{"key":"marten.kongstad@gmail.com","avatar":"https://avatars.githubusercontent.com/u/567558?v=4"},"body":"When --shortstat is used in conjunction with --dirstat=changes, git diff will\noutput the dirstat information twice: first as calculated by the 'lines'\nalgorithm, then as calculated by the 'changes' algorithm:\n\n    $ git diff --dirstat=changes,10 --shortstat v2.2.0..v2.2.1\n     23 files changed, 453 insertions(+), 54 deletions(-)\n      33.5% Documentation/RelNotes/\n      26.2% t/\n      46.6% Documentation/RelNotes/\n      16.6% t/\n\nThe same duplication happens for --shortstat together with --dirstat=files, but\nnot for --shortstat together with --dirstat=lines.\n\nLimit output to only include one dirstat part, calculated as specified\nby the --dirstat parameter. Also, add test for this.\n\nSigned-off-by: Mårten Kongstad <marten.kongstad@gmail.com>\n---\nGood point about hardcoded values in the tests. How about instead we check that\na specific directory appears exactly once in the output?\n\n diff.c                  |  2 +-\n t/t4047-diff-dirstat.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex d1bd534..abc32c8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4541,7 +4541,7 @@ void diff_flush(struct diff_options *options)\n \t\t\tshow_stats(&diffstat, options);\n \t\tif (output_format & DIFF_FORMAT_SHORTSTAT)\n \t\t\tshow_shortstats(&diffstat, options);\n-\t\tif (output_format & DIFF_FORMAT_DIRSTAT)\n+\t\tif (output_format & DIFF_FORMAT_DIRSTAT && dirstat_by_line)\n \t\t\tshow_dirstat_by_line(&diffstat, options);\n \t\tfree_diffstat_info(&diffstat);\n \t\tseparator++;\ndiff --git a/t/t4047-diff-dirstat.sh b/t/t4047-diff-dirstat.sh\nindex ed7e093..128f7bf 100755\n--- a/t/t4047-diff-dirstat.sh\n+++ b/t/t4047-diff-dirstat.sh\n@@ -973,4 +973,15 @@ test_expect_success 'diff.dirstat=future_param,0,lines should warn, but still wo\n \ttest_i18ngrep -q \"diff\\\\.dirstat\" actual_error\n '\n \n+test_expect_success '--shortstat --dirstat should output only one dirstat' '\n+\tgit diff --shortstat --dirstat=changes HEAD^..HEAD >actual_diff_shortstat_dirstat_changes &&\n+\ttest $(grep -c \" dst/copy/changed/$\" actual_diff_shortstat_dirstat_changes) = 1 &&\n+\n+\tgit diff --shortstat --dirstat=lines HEAD^..HEAD >actual_diff_shortstat_dirstat_lines &&\n+\ttest $(grep -c \" dst/copy/changed/$\" actual_diff_shortstat_dirstat_lines) = 1 &&\n+\n+\tgit diff --shortstat --dirstat=files HEAD^..HEAD >actual_diff_shortstat_dirstat_files &&\n+\ttest $(grep -c \" dst/copy/changed/$\" actual_diff_shortstat_dirstat_files) = 1\n+'\n+\n test_done\n-- \n1.9.1\n"},{"id":"256799","messageId":"54F2E931.7020200@web.de","threadId":"38669","inReplyTo":"1425195546-15637-1-git-send-email-marten.kongstad@gmail.com","subject":"Re: [PATCH v2] diff --shortstat --dirstat: remove duplicate output","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-03-01T10:25:53Z","receivedAt":"2015-03-01T10:25:53Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-03-01 08.39, Mårten Kongstad wrote:\n[]\n index ed7e093..128f7bf 100755\n> --- a/t/t4047-diff-dirstat.sh\n> +++ b/t/t4047-diff-dirstat.sh\n> @@ -973,4 +973,15 @@ test_expect_success 'diff.dirstat=future_param,0,lines should warn, but still wo\n>  \ttest_i18ngrep -q \"diff\\\\.dirstat\" actual_error\n>  '\n>  \n> +test_expect_success '--shortstat --dirstat should output only one dirstat' '\n> +\tgit diff --shortstat --dirstat=changes HEAD^..HEAD >actual_diff_shortstat_dirstat_changes &&\n> +\ttest $(grep -c \" dst/copy/changed/$\" actual_diff_shortstat_dirstat_changes) = 1 &&\nHow portable is the \"grep -c\" usage ?\n(I don't now it either, do we have other opinions ?), but the following seems to be more \"Git-style\":\n\ntest_expect_success '--shortstat --dirstat should output only one dirstat' '\n\tgit diff --shortstat --dirstat=changes HEAD^..HEAD >actual_diff_shortstat_dirstat_changes &&\n\tgrep \" dst/copy/changed/$\" actual_diff_shortstat_dirstat_changes >actual &&\n\ttest_line_count = 1 actual\n"},{"id":"256803","messageId":"54F320E9.1000006@drmicha.warpmail.net","threadId":"38669","inReplyTo":"54F2E931.7020200@web.de","subject":"Re: [PATCH v2] diff --shortstat --dirstat: remove duplicate output","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2015-03-01T14:23:37Z","receivedAt":"2015-03-01T14:23:37Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Torsten Bögershausen venit, vidit, dixit 01.03.2015 11:25:\n> On 2015-03-01 08.39, Mårten Kongstad wrote:\n> []\n>  index ed7e093..128f7bf 100755\n>> --- a/t/t4047-diff-dirstat.sh\n>> +++ b/t/t4047-diff-dirstat.sh\n>> @@ -973,4 +973,15 @@ test_expect_success 'diff.dirstat=future_param,0,lines should warn, but still wo\n>>  \ttest_i18ngrep -q \"diff\\\\.dirstat\" actual_error\n>>  '\n>>  \n>> +test_expect_success '--shortstat --dirstat should output only one dirstat' '\n>> +\tgit diff --shortstat --dirstat=changes HEAD^..HEAD >actual_diff_shortstat_dirstat_changes &&\n>> +\ttest $(grep -c \" dst/copy/changed/$\" actual_diff_shortstat_dirstat_changes) = 1 &&\n> How portable is the \"grep -c\" usage ?\n> (I don't now it either, do we have other opinions ?), but the following seems to be more \"Git-style\":\n> \n> test_expect_success '--shortstat --dirstat should output only one dirstat' '\n> \tgit diff --shortstat --dirstat=changes HEAD^..HEAD >actual_diff_shortstat_dirstat_changes &&\n> \tgrep \" dst/copy/changed/$\" actual_diff_shortstat_dirstat_changes >actual &&\n> \ttest_line_count = 1 actual\n> \n\nIf I would have had to guess from the documentation: What does \"git diff\n--dirstat --shortstat\" do? I would have answered: It displays both the\ndirstat and the shortstat.\n\nSo, is what you are trying to \"fix\" a peculiarity of\n\"--dirstat=changes\", or do you simplify prefer --dirstat and --shortstat\nto override each other?\n\nMaybe I'm overlooking something (and that's not a rhetorical\nconditional), but if you specify both options when you want the output\nof only one them, the answer would be the obvious one, not a patch,\nwouldn't it?\n\nIf there is indeed a good reason to change the behavior it should be\ndocumented.\n\nMichael\n"},{"id":"256804","messageId":"20150301155818.GA5307@laptop","threadId":"38669","inReplyTo":"54F2E931.7020200@web.de","subject":"Re: [PATCH v2] diff --shortstat --dirstat: remove duplicate output","fromName":"Mårten Kongstad","fromEmail":"marten.kongstad@gmail.com","sentAt":"2015-03-01T15:58:18Z","receivedAt":"2015-03-01T15:58:18Z","isPatch":true,"sender":{"key":"marten.kongstad@gmail.com","avatar":"https://avatars.githubusercontent.com/u/567558?v=4"},"body":"On Sun, Mar 01, 2015 at 11:25:53AM +0100, Torsten Bögershausen wrote:\n> On 2015-03-01 08.39, Mårten Kongstad wrote:\n> []\n> > +test_expect_success '--shortstat --dirstat should output only one dirstat' '\n> > +\tgit diff --shortstat --dirstat=changes HEAD^..HEAD >actual_diff_shortstat_dirstat_changes &&\n> > +\ttest $(grep -c \" dst/copy/changed/$\" actual_diff_shortstat_dirstat_changes) = 1 &&\n> How portable is the \"grep -c\" usage ?\n> (I don't now it either, do we have other opinions ?), but the following seems to be more \"Git-style\":\n> \n> test_expect_success '--shortstat --dirstat should output only one dirstat' '\n> \tgit diff --shortstat --dirstat=changes HEAD^..HEAD >actual_diff_shortstat_dirstat_changes &&\n> \tgrep \" dst/copy/changed/$\" actual_diff_shortstat_dirstat_changes >actual &&\n> \ttest_line_count = 1 actual\n> \nFrom what I can see, both 'grep -c' and 'grep >file && test_line_count'\nare used in the tests.\n\n'grep -c' is used in these tests:\n- t3404-rebase-interactive.sh\n- t3507-cherry-pick-conflict.sh\n- t4036-format-patch-signer-mime.sh\n- t4150-am.sh\n- t7810-grep.sh\n- t8003-blame-corner-cases.sh\n- t9350-fast-export.sh\n\n'grep >file && test_line_count' is used in this test:\n- t9400-git-cvsserver-server.sh\n\nAnd to make matters more confusing, both 'grep -c' and 'grep >file &&\ntest_line_count' is used in this test:\n- t9001-send-email.sh\n\nGranted I didn't miss anything while trawling the tests for the above\nnumbers, it feels like the 'grep -c' option is more in line with the\nexisting tests. That said, I don't know if there is an ongoing trend to\ndeprecate 'grep -c' in favour of 'test_line_count'.\n"},{"id":"256805","messageId":"20150301160125.GB5307@laptop","threadId":"38669","inReplyTo":"54F320E9.1000006@drmicha.warpmail.net","subject":"Re: [PATCH v2] diff --shortstat --dirstat: remove duplicate output","fromName":"Mårten Kongstad","fromEmail":"marten.kongstad@gmail.com","sentAt":"2015-03-01T16:01:25Z","receivedAt":"2015-03-01T16:01:25Z","isPatch":true,"sender":{"key":"marten.kongstad@gmail.com","avatar":"https://avatars.githubusercontent.com/u/567558?v=4"},"body":"On Sun, Mar 01, 2015 at 03:23:37PM +0100, Michael J Gruber wrote:\n[]\n> If I would have had to guess from the documentation: What does \"git diff\n> --dirstat --shortstat\" do? I would have answered: It displays both the\n> dirstat and the shortstat.\n> \n> So, is what you are trying to \"fix\" a peculiarity of\n> \"--dirstat=changes\", or do you simplify prefer --dirstat and --shortstat\n> to override each other?\n> \n> Maybe I'm overlooking something (and that's not a rhetorical\n> conditional), but if you specify both options when you want the output\n> of only one them, the answer would be the obvious one, not a patch,\n> wouldn't it?\n> \n> If there is indeed a good reason to change the behavior it should be\n> documented.\nI interpret the documentation the same way as you do. The problem is\nthat the dirstat is displayed twice for --dirstat=changes (or\n--dirstat=files):\n\n$ git diff --dirstat=changes,10 --shortstat v2.2.0..v2.2.1\n 23 files changed, 453 insertions(+), 54 deletions(-)\n  33.5% Documentation/RelNotes/\n  26.2% t/\n  46.6% Documentation/RelNotes/\n  16.6% t/\n\nbut only once for --dirstat=lines:\n\n$ git diff --dirstat=lines,10 --shortstat v2.2.0..v2.2.1\n 23 files changed, 453 insertions(+), 54 deletions(-)\n  33.5% Documentation/RelNotes/\n  26.2% t/\n\nThis behaviour is either a bug, or an inconsistency not immediately apparent to\nthe user.\n\nThe proposed patch will make the 'changes' and 'files' cases behave like\n'lines', i.e. output one shortstat and (only) one dirstat:\n\n$ patched-version-of-git diff --dirstat=changes,10 --shortstat v2.2.0..v2.2.1\n 23 files changed, 453 insertions(+), 54 deletions(-)\n  46.6% Documentation/RelNotes/\n  16.6% t/\n"},{"id":"256806","messageId":"54F33983.5090700@drmicha.warpmail.net","threadId":"38669","inReplyTo":"20150301160125.GB5307@laptop","subject":"Re: [PATCH v2] diff --shortstat --dirstat: remove duplicate output","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2015-03-01T16:08:35Z","receivedAt":"2015-03-01T16:08:35Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Mårten Kongstad venit, vidit, dixit 01.03.2015 17:01:\n> On Sun, Mar 01, 2015 at 03:23:37PM +0100, Michael J Gruber wrote:\n> []\n>> If I would have had to guess from the documentation: What does \"git diff\n>> --dirstat --shortstat\" do? I would have answered: It displays both the\n>> dirstat and the shortstat.\n>>\n>> So, is what you are trying to \"fix\" a peculiarity of\n>> \"--dirstat=changes\", or do you simplify prefer --dirstat and --shortstat\n>> to override each other?\n>>\n>> Maybe I'm overlooking something (and that's not a rhetorical\n>> conditional), but if you specify both options when you want the output\n>> of only one them, the answer would be the obvious one, not a patch,\n>> wouldn't it?\n>>\n>> If there is indeed a good reason to change the behavior it should be\n>> documented.\n> I interpret the documentation the same way as you do. The problem is\n> that the dirstat is displayed twice for --dirstat=changes (or\n> --dirstat=files):\n> \n> $ git diff --dirstat=changes,10 --shortstat v2.2.0..v2.2.1\n>  23 files changed, 453 insertions(+), 54 deletions(-)\n>   33.5% Documentation/RelNotes/\n>   26.2% t/\n>   46.6% Documentation/RelNotes/\n>   16.6% t/\n> \n> but only once for --dirstat=lines:\n> \n> $ git diff --dirstat=lines,10 --shortstat v2.2.0..v2.2.1\n>  23 files changed, 453 insertions(+), 54 deletions(-)\n>   33.5% Documentation/RelNotes/\n>   26.2% t/\n> \n> This behaviour is either a bug, or an inconsistency not immediately apparent to\n> the user.\n> \n> The proposed patch will make the 'changes' and 'files' cases behave like\n> 'lines', i.e. output one shortstat and (only) one dirstat:\n> \n> $ patched-version-of-git diff --dirstat=changes,10 --shortstat v2.2.0..v2.2.1\n>  23 files changed, 453 insertions(+), 54 deletions(-)\n>   46.6% Documentation/RelNotes/\n>   16.6% t/\n> \n\nThanks for the clarification. That looks worthwhile.\n\nMichael\n"},{"id":"256815","messageId":"20150302020009.Horde.9sATpKnsrWQkGadaRTvxkA3@webmail.informatik.kit.edu","threadId":"38669","inReplyTo":"20150301155818.GA5307@laptop","subject":"Re: [PATCH v2] diff --shortstat --dirstat: remove duplicate output","fromName":"SZEDER Gábor","fromEmail":"szeder@ira.uka.de","sentAt":"2015-03-02T01:00:09Z","receivedAt":"2015-03-02T01:00:09Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\nHi,\n\nQuoting Mårten Kongstad <marten.kongstad@gmail.com>:\n\n> On Sun, Mar 01, 2015 at 11:25:53AM +0100, Torsten Bögershausen wrote:\n>> On 2015-03-01 08.39, Mårten Kongstad wrote:\n>> []\n>>> +test_expect_success '--shortstat --dirstat should output only\n>> one dirstat' '\n>>> +\tgit diff --shortstat --dirstat=changes HEAD^..HEAD\n>>> actual_diff_shortstat_dirstat_changes &&\n>>> +\ttest $(grep -c \" dst/copy/changed/$\"\n>> actual_diff_shortstat_dirstat_changes) = 1 &&\n>> How portable is the \"grep -c\" usage ?\n>> (I don't now it either, do we have other opinions ?), but the\n>> following seems to be more \"Git-style\":\n>>\n>> test_expect_success '--shortstat --dirstat should output only one dirstat' '\n>> \tgit diff --shortstat --dirstat=changes HEAD^..HEAD\n>>> actual_diff_shortstat_dirstat_changes &&\n>> \tgrep \" dst/copy/changed/$\" actual_diff_shortstat_dirstat_changes >actual &&\n>> \ttest_line_count = 1 actual\n>>\n\n\n> Granted I didn't miss anything while trawling the tests for the above\n> numbers, it feels like the 'grep -c' option is more in line with the\n> existing tests. That said, I don't know if there is an ongoing trend to\n> deprecate 'grep -c' in favour of 'test_line_count'.\n\nIt's not just 'grep -c' but the 'test' checking its output as well.\n\nIf something goes wrong and the line count doesn't match expectations\n'test' fails silently leaving the developer clueless as to what went\nwrong.\n\n'test_line_count', on the other hand, produces useful output in case\nof a failure:\n\n    $ printf 'foo\\nbar\\n' >actual\n    $ test_line_count = 1 actual\n    test_line_count: line count for actual != 1\n    foo\n    bar\n\nSince the name of the file in question is included in the output and\nsince there are three separate checks in this test, I would also\nsuggest writing 'grep's output into separate files\n'actual_{changes,lines,files}'.\n\nGábor\n"},{"id":"256845","messageId":"1425308739-13082-1-git-send-email-marten.kongstad@gmail.com","threadId":"38669","inReplyTo":"20150302020009.Horde.9sATpKnsrWQkGadaRTvxkA3@webmail.informatik.kit.edu","subject":"[PATCH v3] diff --shortstat --dirstat: remove duplicate output","fromName":"Mårten Kongstad","fromEmail":"marten.kongstad@gmail.com","sentAt":"2015-03-02T15:05:39Z","receivedAt":"2015-03-02T15:05:39Z","isPatch":true,"sender":{"key":"marten.kongstad@gmail.com","avatar":"https://avatars.githubusercontent.com/u/567558?v=4"},"body":"When --shortstat is used in conjunction with --dirstat=changes, git diff will\noutput the dirstat information twice: first as calculated by the 'lines'\nalgorithm, then as calculated by the 'changes' algorithm:\n\n    $ git diff --dirstat=changes,10 --shortstat v2.2.0..v2.2.1\n     23 files changed, 453 insertions(+), 54 deletions(-)\n      33.5% Documentation/RelNotes/\n      26.2% t/\n      46.6% Documentation/RelNotes/\n      16.6% t/\n\nThe same duplication happens for --shortstat together with --dirstat=files, but\nnot for --shortstat together with --dirstat=lines.\n\nLimit output to only include one dirstat part, calculated as specified\nby the --dirstat parameter. Also, add test for this.\n\nSigned-off-by: Mårten Kongstad <marten.kongstad@gmail.com>\n---\nv3: change how tests count (part of) the dirstat number of lines: instead of\n'grep -c', use 'grep >filename && test_line_count'. Thanks to Torsten\nBögershausen and SZEDER Gábor for pointing out how to improve the tests.\n\n diff.c                  |  2 +-\n t/t4047-diff-dirstat.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex d1bd534..abc32c8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4541,7 +4541,7 @@ void diff_flush(struct diff_options *options)\n \t\t\tshow_stats(&diffstat, options);\n \t\tif (output_format & DIFF_FORMAT_SHORTSTAT)\n \t\t\tshow_shortstats(&diffstat, options);\n-\t\tif (output_format & DIFF_FORMAT_DIRSTAT)\n+\t\tif (output_format & DIFF_FORMAT_DIRSTAT && dirstat_by_line)\n \t\t\tshow_dirstat_by_line(&diffstat, options);\n \t\tfree_diffstat_info(&diffstat);\n \t\tseparator++;\ndiff --git a/t/t4047-diff-dirstat.sh b/t/t4047-diff-dirstat.sh\nindex ed7e093..065d74f 100755\n--- a/t/t4047-diff-dirstat.sh\n+++ b/t/t4047-diff-dirstat.sh\n@@ -973,4 +973,15 @@ test_expect_success 'diff.dirstat=future_param,0,lines should warn, but still wo\n \ttest_i18ngrep -q \"diff\\\\.dirstat\" actual_error\n '\n \n+test_expect_success '--shortstat --dirstat should output only one dirstat' '\n+\tgit diff --shortstat --dirstat=changes HEAD^..HEAD | grep \" dst/copy/changed/$\" >actual_diff_shortstat_dirstat_changes &&\n+\ttest_line_count = 1 actual_diff_shortstat_dirstat_changes &&\n+\n+\tgit diff --shortstat --dirstat=lines HEAD^..HEAD | grep \" dst/copy/changed/$\" >actual_diff_shortstat_dirstat_lines &&\n+\ttest_line_count = 1 actual_diff_shortstat_dirstat_lines &&\n+\n+\tgit diff --shortstat --dirstat=files HEAD^..HEAD | grep \" dst/copy/changed/$\" >actual_diff_shortstat_dirstat_files &&\n+\ttest_line_count = 1 actual_diff_shortstat_dirstat_files\n+'\n+\n test_done\n-- \n1.9.1\n"},{"id":"256977","messageId":"20150304090734.GA31245@peff.net","threadId":"38669","inReplyTo":"20150302020009.Horde.9sATpKnsrWQkGadaRTvxkA3@webmail.informatik.kit.edu","subject":"Re: [PATCH v2] diff --shortstat --dirstat: remove duplicate output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-04T09:07:35Z","receivedAt":"2015-03-04T09:07:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 02, 2015 at 02:00:09AM +0100, SZEDER Gábor wrote:\n\n> It's not just 'grep -c' but the 'test' checking its output as well.\n> \n> If something goes wrong and the line count doesn't match expectations\n> 'test' fails silently leaving the developer clueless as to what went\n> wrong.\n> \n> 'test_line_count', on the other hand, produces useful output in case\n> of a failure:\n> \n>    $ printf 'foo\\nbar\\n' >actual\n>    $ test_line_count = 1 actual\n>    test_line_count: line count for actual != 1\n>    foo\n>    bar\n\nSince we have test_line_count, I think it makes sense to use it. But for\nreference, I recently introduced the `verbose` function to test-lib.sh,\nwhich lets you write:\n\n  $ verbose test 1 = 2\n  command failed:  'test' '1' '=' '2'\n\nYou can use it with any command that might fail without printing a\nuseful error message. The big downside is that it sees only the\narguments to the command, so if you write:\n\n  $ test \"$(do_something)\" = 123\n\nyou will only see:\n\n  command failed:  'test '456' '=' '123'\n\nwith no notion that $(do_something) was involved. So purpose-built\nhelpers like test_line_count will produce better output.\n\nYou may also be introduced in the \"-x\" option I recently introduced,\nwhich can help with \"quiet\" failures. E.g.:\n\n  $ ./t0001-init.sh -x --verbose-only=15\n  [...]\n  ok 13 - GIT_DIR & GIT_WORK_TREE (2)\n  ok 14 - reinit\n  \n  expecting success: \n          mkdir template-source &&\n          echo content >template-source/file &&\n          git init --template=../template-source template-custom &&\n          test_cmp template-source/file template-custom/.git/file\n  \n  + mkdir template-source\n  + echo content\n  + git init --template=../template-source template-custom\n  Initialized empty Git repository in /home/peff/compile/git/t/trash directory.t0001-init/template-custom/.git/\n  + test_cmp template-source/file template-custom/.git/file\n  + diff -u template-source/file template-custom/.git/file\n  ok 15 - init with --template\n  ok 16 - init with --template (blank)\n\n(ok, it's not that interesting because the test didn't fail, but\nhopefully you get the point).\n\nNow I'll stop hijacking your thread to advertise random test-lib\nfeatures. :)\n\n-Peff\n"},{"id":"257108","messageId":"xmqq8ufb2khe.fsf@gitster.dls.corp.google.com","threadId":"38669","inReplyTo":"1425308739-13082-1-git-send-email-marten.kongstad@gmail.com","subject":"Re: [PATCH v3] diff --shortstat --dirstat: remove duplicate output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-05T21:38:21Z","receivedAt":"2015-03-05T21:38:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mårten Kongstad <marten.kongstad@gmail.com> writes:\n\n> v3: change how tests count (part of) the dirstat number of lines: instead of\n> 'grep -c', use 'grep >filename && test_line_count'. Thanks to Torsten\n> Bögershausen and SZEDER Gábor for pointing out how to improve the tests.\n\nThanks.\n\nI'd squash the following on top before queuing it.\n\nThe overlong lines that ignores the exit status from \"git diff\"\nlooked problematic to me.\n\n t/t4047-diff-dirstat.sh | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t4047-diff-dirstat.sh b/t/t4047-diff-dirstat.sh\nindex 065d74f..0d50dce 100755\n--- a/t/t4047-diff-dirstat.sh\n+++ b/t/t4047-diff-dirstat.sh\n@@ -974,13 +974,16 @@ test_expect_success 'diff.dirstat=future_param,0,lines should warn, but still wo\n '\n \n test_expect_success '--shortstat --dirstat should output only one dirstat' '\n-\tgit diff --shortstat --dirstat=changes HEAD^..HEAD | grep \" dst/copy/changed/$\" >actual_diff_shortstat_dirstat_changes &&\n+\tgit diff --shortstat --dirstat=changes HEAD^..HEAD >out &&\n+\tgrep \" dst/copy/changed/$\" out >actual_diff_shortstat_dirstat_changes &&\n \ttest_line_count = 1 actual_diff_shortstat_dirstat_changes &&\n \n-\tgit diff --shortstat --dirstat=lines HEAD^..HEAD | grep \" dst/copy/changed/$\" >actual_diff_shortstat_dirstat_lines &&\n+\tgit diff --shortstat --dirstat=lines HEAD^..HEAD >out &&\n+\tgrep \" dst/copy/changed/$\" out >actual_diff_shortstat_dirstat_lines &&\n \ttest_line_count = 1 actual_diff_shortstat_dirstat_lines &&\n \n-\tgit diff --shortstat --dirstat=files HEAD^..HEAD | grep \" dst/copy/changed/$\" >actual_diff_shortstat_dirstat_files &&\n+\tgit diff --shortstat --dirstat=files HEAD^..HEAD >out &&\n+\tgrep \" dst/copy/changed/$\" out >actual_diff_shortstat_dirstat_files &&\n \ttest_line_count = 1 actual_diff_shortstat_dirstat_files\n '\n \n"}]}