{"thread":{"id":"46846","subject":"diffstat summary mode change bug","startedAt":"2017-09-27T18:15:10Z","lastAt":"2017-09-28T04:14:31Z","messageCount":19,"participants":["Linus Torvalds","Stefan Beller","Jeff King","Jonathan Nieder","Ramsay Jones","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"329050","messageId":"CA+55aFzThd6cayUVuv39k=sb8KCphLkWGCV4n2HUjfn7Pxkd3g@mail.gmail.com","threadId":"46846","inReplyTo":null,"subject":"diffstat summary mode change bug","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-09-27T18:15:04Z","receivedAt":"2017-09-27T18:15:10Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"Current git shows file-mode changes incorrectly in the diffstat\nsummary, as I just noted from a pull request I did on the kernel.\n\nThe pull request *should* have resulted in a summary like this:\n\n ...\n 21 files changed, 247 insertions(+), 67 deletions(-)\n mode change 100644 => 100755 tools/testing/selftests/memfd/run_tests.sh\n create mode 100644 tools/testing/selftests/net/reuseaddr_conflict.c\n\nbut instead that \"mode change\" line didn't have a newline at the end, and I got\n\n ...\n 21 files changed, 247 insertions(+), 67 deletions(-)\n mode change 100644 => 100755\ntools/testing/selftests/memfd/run_tests.sh create mode 100644\ntools/testing/selftests/net/reuseaddr_conflict.c\n\n(ok, linewrapping in this email may make that look wrong - but the\n\"mode change\" land the \"create mode\" are both on the same line.\n\nBisecting it got me:\n\n 146fdb0dfe445464fa438f3835557c58a01d85d7 is the first bad commit\n commit 146fdb0dfe445464fa438f3835557c58a01d85d7\n Author: Stefan Beller \n Date: Thu Jun 29 17:07:05 2017 -0700\n\n diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY\n\n Signed-off-by: Stefan Beller \n Signed-off-by: Junio C Hamano \n\nand the reason seems to be that the '\\n' at the end got dropped as the\nold code was very confusing (the old code had two different '\\n' cases\nfor the \"show filename or not\").\n\nI think the right fix is this whitespace-damaged trivial one-liner:\n\n diff --git a/diff.c b/diff.c\n index 3c6a3e0fa..653bb2e72 100644\n --- a/diff.c\n +++ b/diff.c\n @@ -5272,6 +5272,7 @@ static void show_mode_change(struct\ndiff_options *opt, struct diff_filepair *p,\n strbuf_addch(&sb, ' ');\n quote_c_style(p->two->path, &sb, NULL, 0);\n }\n + strbuf_addch(&sb, '\\n');\n emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,\n sb.buf, sb.len, 0);\n strbuf_release(&sb);\n\nbut somebody should double-check that.\n\n Linus\n"},{"id":"329058","messageId":"CAGZ79kZF5w7ucbUCKJAuAbz2KBoMJjWneAxFcjOPeted8SBNmg@mail.gmail.com","threadId":"46846","inReplyTo":"CA+55aFzThd6cayUVuv39k=sb8KCphLkWGCV4n2HUjfn7Pxkd3g@mail.gmail.com","subject":"Re: diffstat summary mode change bug","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-27T20:40:30Z","receivedAt":"2017-09-27T20:40:36Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 27, 2017 at 11:15 AM, Linus Torvalds\n wrote:\n\n> (ok, linewrapping in this email may make that look wrong - but the\n> \"mode change\" land the \"create mode\" are both on the same line.\n\nThanks for the bug report.\n\n> and the reason seems to be that the '\\n' at the end got dropped as the\n> old code was very confusing (the old code had two different '\\n' cases\n> for the \"show filename or not\").\n\nI disagree with this analysis, as the fix you propose adds the\nnew line unconditionally, i.e. this code path would be broken\nregardless of \"show filename or not\".\n\nBoth search for \"create mode\" and \"mode change\" results in\nhits in the test suite, so we should have caught this.\nI wonder why our tests failed to tell us about this.\n\nSpecifically we have t4100/t-apply-4.expect\n mode change 100644 => 100755 t/t0000-basic.sh\n mode change 100644 => 100755 t/test-lib.sh\nwhich would seem to exercise this code path.\n\nThe code *looks* correct, but I am full of doubts now.\n\nStefan\n\n> I think the right fix is this whitespace-damaged trivial one-liner:\n>\n> diff --git a/diff.c b/diff.c\n> index 3c6a3e0fa..653bb2e72 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -5272,6 +5272,7 @@ static void show_mode_change(struct\n> diff_options *opt, struct diff_filepair *p,\n> strbuf_addch(&sb, ' ');\n> quote_c_style(p->two->path, &sb, NULL, 0);\n> }\n> + strbuf_addch(&sb, '\\n');\n> emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,\n> sb.buf, sb.len, 0);\n> strbuf_release(&sb);\n>\n> but somebody should double-check that.\n>\n> Linus\n"},{"id":"329060","messageId":"CA+55aFxwNOLaoeg64CYb-a6c89ceVbfc0b5vG_WCzyNBwynwfQ@mail.gmail.com","threadId":"46846","inReplyTo":"CAGZ79kZF5w7ucbUCKJAuAbz2KBoMJjWneAxFcjOPeted8SBNmg@mail.gmail.com","subject":"Re: diffstat summary mode change bug","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-09-27T21:02:48Z","receivedAt":"2017-09-27T21:02:56Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Sep 27, 2017 at 1:40 PM, Stefan Beller wrote:\n>\n> I disagree with this analysis, as the fix you propose adds the\n> new line unconditionally, i.e. this code path would be broken\n> regardless of \"show filename or not\".\n\nRight. Because it is what we want.\n\nThe old code (before that commit) used to have two different cases:\n\n fprintf(file, \"%s mode change %06o => %06o%c\",\nline_prefix, p->one->mode,\n p->two->mode, show_name ? ' ' : '\\n');\n\nie if \"show_name\" was set, it would *not* print a newline, and print a\nspace instead.\n\nBut then on the very next line, it used to do:\n\n if (show_name) {\n write_name_quoted(p->two->path, file, '\\n');\n\nie now it prints the filename, and then prints the newline.\n\nEnd result: it used to *always* print the newline. Either it printed\nit at the end of the mode (for the non-show_name case), or it printed\nit at the end of the filename (for the show_name case).\n\nYour patch removed the '\\n' entirely.\n\nMy patch makes it unconditional, which it was before your patch (it\nwas \"conditional\" only in where it was printed, not _whether_ it was\nprinted).\n\n> I wonder why our tests failed to tell us about this.\n>\n> Specifically we have t4100/t-apply-4.expect\n> mode change 100644 => 100755 t/t0000-basic.sh\n> mode change 100644 => 100755 t/test-lib.sh\n> which would seem to exercise this code path.\n\nThat only tests \"git apply --stat --summary\".\n\nIt doesn't test \"git diff\" at all.\n\nAnd the \"mode change\" printout is entirely different code (see apply.c\nvs diff.c).\n\n Linus\n"},{"id":"329062","messageId":"CAGZ79kbCgTNaunfneWHP3xkLeXwW72CZZWs7Sv8A7QL_UYE_HQ@mail.gmail.com","threadId":"46846","inReplyTo":"CA+55aFxwNOLaoeg64CYb-a6c89ceVbfc0b5vG_WCzyNBwynwfQ@mail.gmail.com","subject":"Re: diffstat summary mode change bug","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-27T21:09:20Z","receivedAt":"2017-09-27T21:09:28Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 27, 2017 at 2:02 PM, Linus Torvalds\n wrote:\n> On Wed, Sep 27, 2017 at 1:40 PM, Stefan Beller wrote:\n>>\n>> I disagree with this analysis, as the fix you propose adds the\n>> new line unconditionally, i.e. this code path would be broken\n>> regardless of \"show filename or not\".\n>\n> Right. Because it is what we want.\n>\n> The old code (before that commit) used to have two different cases:\n>\n> fprintf(file, \"%s mode change %06o => %06o%c\",\n> line_prefix, p->one->mode,\n> p->two->mode, show_name ? ' ' : '\\n');\n>\n> ie if \"show_name\" was set, it would *not* print a newline, and print a\n> space instead.\n>\n> But then on the very next line, it used to do:\n>\n> if (show_name) {\n> write_name_quoted(p->two->path, file, '\\n');\n>\n> ie now it prints the filename, and then prints the newline.\n>\n> End result: it used to *always* print the newline. Either it printed\n> it at the end of the mode (for the non-show_name case), or it printed\n> it at the end of the filename (for the show_name case).\n>\n> Your patch removed the '\\n' entirely.\n>\n> My patch makes it unconditional, which it was before your patch (it\n> was \"conditional\" only in where it was printed, not _whether_ it was\n> printed).\n\nI agree with this.\n\n>\n>> I wonder why our tests failed to tell us about this.\n>>\n>> Specifically we have t4100/t-apply-4.expect\n>> mode change 100644 => 100755 t/t0000-basic.sh\n>> mode change 100644 => 100755 t/test-lib.sh\n>> which would seem to exercise this code path.\n>\n> That only tests \"git apply --stat --summary\".\n>\n> It doesn't test \"git diff\" at all.\n>\n> And the \"mode change\" printout is entirely different code (see apply.c\n> vs diff.c).\n\nWhy doesn't this surprise me at all?\n(Whenever I write emails I assume the best of Gits code base, such\nas non-duplicated code. \"It's all in the mighty diff machinery\".)\n\nIn that case let me write a test for this and resubmit\nyour fix without whitespace mangling.\n\nThanks,\nStefan\n"},{"id":"329066","messageId":"20170927215852.20163-1-sbeller@google.com","threadId":"46846","inReplyTo":"CAGZ79kbCgTNaunfneWHP3xkLeXwW72CZZWs7Sv8A7QL_UYE_HQ@mail.gmail.com","subject":"[PATCH] diff: correct newline in summary for renamed files","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-27T21:58:52Z","receivedAt":"2017-09-27T21:59:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Linus Torvalds \n\nIn 146fdb0dfe (diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY,\n2017-06-29), the conversion from direct printing to the symbol emission\ndropped the new line character for renamed, copied and rewritten files.\n\nAdd the emission of a newline, add a test for this case.\n\nSigned-off-by: Linus Torvalds \nSigned-off-by: Stefan Beller \n---\n\n Linus, I assumed your sign off for the original patch. Thanks for spotting.\n \n Adding the mode change to t4016 seems like the easiest way to test it.\n \n Thanks,\n Stefan\n \n diff.c | 1 +\n t/t4016-diff-quote.sh | 2 ++\n 2 files changed, 3 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 3c6a3e0faa..653bb2e72e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5272,6 +5272,7 @@ static void show_mode_change(struct diff_options *opt, struct diff_filepair *p,\n \t\t\tstrbuf_addch(&sb, ' ');\n \t\t\tquote_c_style(p->two->path, &sb, NULL, 0);\n \t\t}\n+\t\tstrbuf_addch(&sb, '\\n');\n \t\temit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,\n \t\t\t\t sb.buf, sb.len, 0);\n \t\tstrbuf_release(&sb);\ndiff --git a/t/t4016-diff-quote.sh b/t/t4016-diff-quote.sh\nindex 9c48e5c2c9..514056dd10 100755\n--- a/t/t4016-diff-quote.sh\n+++ b/t/t4016-diff-quote.sh\n@@ -30,6 +30,7 @@ test_expect_success setup '\n \tgit add . &&\n \tgit commit -m initial &&\n \tgit mv \"$P0.0\" \"R$P0.0\" &&\n+\tchmod a+x \"R$P0.0\" &&\n \tgit mv \"$P0.1\" \"R$P1.0\" &&\n \tgit mv \"$P0.2\" \"R$P2.0\" &&\n \tgit mv \"$P0.3\" \"R$P3.0\" &&\n@@ -47,6 +48,7 @@ cat >expect <<\\EOF\n rename pathname.2 => Rpathname with SP.0 (100%)\n rename \"pathname\\twith HT.2\" => Rpathname with SP.1 (100%)\n rename pathname.0 => Rpathname.0 (100%)\n+ mode change 100644 => 100755\n rename \"pathname\\twith HT.0\" => Rpathname.1 (100%)\n EOF\n '\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"329067","messageId":"CA+55aFxPZ8+MrB5mbad76aPo8v9S-Q9p3zvK7=j1m2e7o2dFFA@mail.gmail.com","threadId":"46846","inReplyTo":"20170927215852.20163-1-sbeller@google.com","subject":"Re: [PATCH] diff: correct newline in summary for renamed files","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2017-09-27T22:04:16Z","receivedAt":"2017-09-27T22:04:21Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Sep 27, 2017 at 2:58 PM, Stefan Beller wrote:\n>\n> Linus, I assumed your sign off for the original patch. Thanks for spotting.\n>\n> Adding the mode change to t4016 seems like the easiest way to test it.\n\nLooks good to me, and you don't need to give me authorship credit.\nJust a \"Reported-by:\" is fine by me.\n\nBut yes, you can also consider the patch signed off by me, although\nwith your test update, _most_ of the patch is yours so it feels kind\nof stupid to mark me as author.\n\n Linus\n"},{"id":"329069","messageId":"20170927220925.u622metv2fcdtcfq@sigill.intra.peff.net","threadId":"46846","inReplyTo":"20170927215852.20163-1-sbeller@google.com","subject":"Re: [PATCH] diff: correct newline in summary for renamed files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T22:09:25Z","receivedAt":"2017-09-27T22:09:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 27, 2017 at 02:58:52PM -0700, Stefan Beller wrote:\n\n> From: Linus Torvalds \n> \n> In 146fdb0dfe (diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY,\n> 2017-06-29), the conversion from direct printing to the symbol emission\n> dropped the new line character for renamed, copied and rewritten files.\n> \n> Add the emission of a newline, add a test for this case.\n> \n> Signed-off-by: Linus Torvalds \n> Signed-off-by: Stefan Beller \n\nThe overall substance looks good, but...\n\n> diff --git a/t/t4016-diff-quote.sh b/t/t4016-diff-quote.sh\n> index 9c48e5c2c9..514056dd10 100755\n> --- a/t/t4016-diff-quote.sh\n> +++ b/t/t4016-diff-quote.sh\n> @@ -30,6 +30,7 @@ test_expect_success setup '\n> \tgit add . &&\n> \tgit commit -m initial &&\n> \tgit mv \"$P0.0\" \"R$P0.0\" &&\n> +\tchmod a+x \"R$P0.0\" &&\n> \tgit mv \"$P0.1\" \"R$P1.0\" &&\n> \tgit mv \"$P0.2\" \"R$P2.0\" &&\n> \tgit mv \"$P0.3\" \"R$P3.0\" &&\n\nWon't this chmod be a problem for platforms without an executable bit?\nI think you'd need to use \"update-index --chmod=+x\" here, or require the\nFILEMODE prereq.\n\nThe whole script is marked as !MINGW, so that makes it less of a\nproblem, but it's still possible have !FILEMODE on a Linux system, if\nyou're on a funny filesystem. That also seems like a good reason to make\nsure this is in a script which is run more widely, since Windows folks\nwould want to run this test, too.\n\n-Peff\n"},{"id":"329071","messageId":"20170927223239.t6gvfztkdjn2vltb@sigill.intra.peff.net","threadId":"46846","inReplyTo":"20170927220925.u622metv2fcdtcfq@sigill.intra.peff.net","subject":"Re: [PATCH] diff: correct newline in summary for renamed files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T22:32:39Z","receivedAt":"2017-09-27T22:32:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 27, 2017 at 06:09:25PM -0400, Jeff King wrote:\n\n> > diff --git a/t/t4016-diff-quote.sh b/t/t4016-diff-quote.sh\n> > index 9c48e5c2c9..514056dd10 100755\n> > --- a/t/t4016-diff-quote.sh\n> > +++ b/t/t4016-diff-quote.sh\n> > @@ -30,6 +30,7 @@ test_expect_success setup '\n> > \tgit add . &&\n> > \tgit commit -m initial &&\n> > \tgit mv \"$P0.0\" \"R$P0.0\" &&\n> > +\tchmod a+x \"R$P0.0\" &&\n> > \tgit mv \"$P0.1\" \"R$P1.0\" &&\n> > \tgit mv \"$P0.2\" \"R$P2.0\" &&\n> > \tgit mv \"$P0.3\" \"R$P3.0\" &&\n> \n> Won't this chmod be a problem for platforms without an executable bit?\n> I think you'd need to use \"update-index --chmod=+x\" here, or require the\n> FILEMODE prereq.\n> \n> The whole script is marked as !MINGW, so that makes it less of a\n> problem, but it's still possible have !FILEMODE on a Linux system, if\n> you're on a funny filesystem. That also seems like a good reason to make\n> sure this is in a script which is run more widely, since Windows folks\n> would want to run this test, too.\n\nThe most appropriate place seems like t4013. I tried adding to its big\nlist of tested formats, but it's quite fragile. The patch below is what\nI came up with, but it still needs updated to cover the cases which call\n\"log --all\".\n\nI think we'd do better to just do a set of new tests at the end (or even\na new test script for diffing mode changes in various formats).\n\n-- >8 --\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex d09acfe48e..c515e3e53f 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -90,6 +90,14 @@ test_expect_success setup '\n \tgit commit -m \"Rearranged lines in dir/sub\" &&\n \tgit checkout master &&\n \n+\tGIT_AUTHOR_DATE=\"2006-06-26 00:06:00 +0000\" &&\n+\tGIT_COMMITTER_DATE=\"2006-06-26 00:06:00 +0000\" &&\n+\texport GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&\n+\tgit checkout -b mode initial &&\n+\tgit update-index --chmod=+x file0 &&\n+\tgit commit -m \"update mode\" &&\n+\tgit checkout -f master &&\n+\n \tgit config diff.renames false &&\n \n \tgit show-branch\n@@ -192,6 +200,10 @@ diff-tree --pretty side\n diff-tree --pretty -p side\n diff-tree --pretty --patch-with-stat side\n \n+diff-tree initial mode\n+diff-tree --stat initial mode\n+diff-tree --summary initial mode\n+\n diff-tree master\n diff-tree -p master\n diff-tree -p -m master\ndiff --git a/t/t4013/diff.diff-tree_--stat_initial_mode b/t/t4013/diff.diff-tree_--stat_initial_mode\nnew file mode 100644\nindex 0000000000..0e5943c2c6\n--- /dev/null\n+++ b/t/t4013/diff.diff-tree_--stat_initial_mode\n@@ -0,0 +1,4 @@\n+$ git diff-tree --stat initial mode\n+ file0 | 0\n+ 1 file changed, 0 insertions(+), 0 deletions(-)\n+$\ndiff --git a/t/t4013/diff.diff-tree_--summary_initial_mode b/t/t4013/diff.diff-tree_--summary_initial_mode\nnew file mode 100644\nindex 0000000000..25846b6af8\n--- /dev/null\n+++ b/t/t4013/diff.diff-tree_--summary_initial_mode\n@@ -0,0 +1,3 @@\n+$ git diff-tree --summary initial mode\n+ mode change 100644 => 100755 file0\n+$\ndiff --git a/t/t4013/diff.diff-tree_initial_mode b/t/t4013/diff.diff-tree_initial_mode\nnew file mode 100644\nindex 0000000000..c47c09423e\n--- /dev/null\n+++ b/t/t4013/diff.diff-tree_initial_mode\n@@ -0,0 +1,3 @@\n+$ git diff-tree initial mode\n+:100644 100755 01e79c32a8c99c557f0757da7cb6d65b3414466d 01e79c32a8c99c557f0757da7cb6d65b3414466d M\tfile0\n+$\n"},{"id":"329072","messageId":"CAGZ79kbqLqPBbzH_c6=YhjSNr+4QAAf=JD7QUNY=aJQ355w8Cw@mail.gmail.com","threadId":"46846","inReplyTo":"20170927220925.u622metv2fcdtcfq@sigill.intra.peff.net","subject":"Re: [PATCH] diff: correct newline in summary for renamed files","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-27T22:34:24Z","receivedAt":"2017-09-27T22:34:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 27, 2017 at 3:09 PM, Jeff King wrote:\n> On Wed, Sep 27, 2017 at 02:58:52PM -0700, Stefan Beller wrote:\n>\n>> From: Linus Torvalds \n>>\n>> In 146fdb0dfe (diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY,\n>> 2017-06-29), the conversion from direct printing to the symbol emission\n>> dropped the new line character for renamed, copied and rewritten files.\n>>\n>> Add the emission of a newline, add a test for this case.\n>>\n>> Signed-off-by: Linus Torvalds \n>> Signed-off-by: Stefan Beller \n>\n> The overall substance looks good, but...\n>\n>> diff --git a/t/t4016-diff-quote.sh b/t/t4016-diff-quote.sh\n>> index 9c48e5c2c9..514056dd10 100755\n>> --- a/t/t4016-diff-quote.sh\n>> +++ b/t/t4016-diff-quote.sh\n>> @@ -30,6 +30,7 @@ test_expect_success setup '\n>> git add . &&\n>> git commit -m initial &&\n>> git mv \"$P0.0\" \"R$P0.0\" &&\n>> + chmod a+x \"R$P0.0\" &&\n>> git mv \"$P0.1\" \"R$P1.0\" &&\n>> git mv \"$P0.2\" \"R$P2.0\" &&\n>> git mv \"$P0.3\" \"R$P3.0\" &&\n>\n> Won't this chmod be a problem for platforms without an executable bit?\n> I think you'd need to use \"update-index --chmod=+x\" here, or require the\n> FILEMODE prereq.\n\nI was experimenting with git add --chmod=+x for this patch, but as this\ntest runs \"diff --summary -M HEAD\", we need it change don the fs.\n\nSo let's find another test. Changing the setup of t4013 is a lot of work,\nbut maybe worth it? Looking into t4031, that would also work.\n\nTo be fs agnostic, we can only compare commits against each other.\n\n> The whole script is marked as !MINGW, so that makes it less of a\n> problem, but it's still possible have !FILEMODE on a Linux system, if\n> you're on a funny filesystem. That also seems like a good reason to make\n> sure this is in a script which is run more widely, since Windows folks\n> would want to run this test, too.\n>\n> -Peff\n"},{"id":"329073","messageId":"CAGZ79kYAfii=vwXwSkx-QE+tXSfPE5h1c39gZSB=c5Asfq9P3Q@mail.gmail.com","threadId":"46846","inReplyTo":"20170927223239.t6gvfztkdjn2vltb@sigill.intra.peff.net","subject":"Re: [PATCH] diff: correct newline in summary for renamed files","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-27T22:39:00Z","receivedAt":"2017-09-27T22:39:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Sep 27, 2017 at 3:32 PM, Jeff King wrote:\n\n> The most appropriate place seems like t4013. I tried adding to its big\n> list of tested formats, but it's quite fragile. The patch below is what\n> I came up with, but it still needs updated to cover the cases which call\n> \"log --all\".\n>\n> I think we'd do better to just do a set of new tests at the end (or even\n> a new test script for diffing mode changes in various formats).\n\nThanks for providing this patch, I think it is actually reasonable to\ndo it here. I can fixup the rest if you don't mind.\n"},{"id":"329074","messageId":"20170927224909.25dfojhx6wzaqwt7@sigill.intra.peff.net","threadId":"46846","inReplyTo":"CAGZ79kYAfii=vwXwSkx-QE+tXSfPE5h1c39gZSB=c5Asfq9P3Q@mail.gmail.com","subject":"Re: [PATCH] diff: correct newline in summary for renamed files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-27T22:49:10Z","receivedAt":"2017-09-27T22:49:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 27, 2017 at 03:39:00PM -0700, Stefan Beller wrote:\n\n> On Wed, Sep 27, 2017 at 3:32 PM, Jeff King wrote:\n> \n> > The most appropriate place seems like t4013. I tried adding to its big\n> > list of tested formats, but it's quite fragile. The patch below is what\n> > I came up with, but it still needs updated to cover the cases which call\n> > \"log --all\".\n> >\n> > I think we'd do better to just do a set of new tests at the end (or even\n> > a new test script for diffing mode changes in various formats).\n> \n> Thanks for providing this patch, I think it is actually reasonable to\n> do it here. I can fixup the rest if you don't mind.\n\nOK. Please go ahead, then.\n\n-Peff\n"},{"id":"329075","messageId":"20170927225126.4836-1-sbeller@google.com","threadId":"46846","inReplyTo":"20170927224909.25dfojhx6wzaqwt7@sigill.intra.peff.net","subject":"[PATCH] diff: correct newline in summary for renamed files","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-27T22:51:26Z","receivedAt":"2017-09-27T22:51:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In 146fdb0dfe (diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY,\n2017-06-29), the conversion from direct printing to the symbol emission\ndropped the new line character for renamed, copied and rewritten files.\n\nAdd the emission of a newline, add a test for this case.\n\nReported-by: Linus Torvalds \nHelped-by: Jeff King \nSigned-off-by: Stefan Beller \n---\n\n Peff, I am undecided about the added 'diff --stat' call as that\n uses a completely different code path and would not show the mode\n change, but I guess we can just use it to document the current state.\n \n Thanks,\n Stefan\n\n diff.c | 1 +\n t/t4013-diff-various.sh | 12 ++++++++++++\n t/t4013/diff.diff-tree_--stat_initial_mode | 4 ++++\n t/t4013/diff.diff-tree_--summary_initial_mode | 3 +++\n t/t4013/diff.diff-tree_initial_mode | 3 +++\n t/t4013/diff.log_--decorate=full_--all | 6 ++++++\n t/t4013/diff.log_--decorate_--all | 6 ++++++\n 7 files changed, 35 insertions(+)\n create mode 100644 t/t4013/diff.diff-tree_--stat_initial_mode\n create mode 100644 t/t4013/diff.diff-tree_--summary_initial_mode\n create mode 100644 t/t4013/diff.diff-tree_initial_mode\n\ndiff --git a/diff.c b/diff.c\nindex 3c6a3e0faa..653bb2e72e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5272,6 +5272,7 @@ static void show_mode_change(struct diff_options *opt, struct diff_filepair *p,\n \t\t\tstrbuf_addch(&sb, ' ');\n \t\t\tquote_c_style(p->two->path, &sb, NULL, 0);\n \t\t}\n+\t\tstrbuf_addch(&sb, '\\n');\n \t\temit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,\n \t\t\t\t sb.buf, sb.len, 0);\n \t\tstrbuf_release(&sb);\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex d09acfe48e..c515e3e53f 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -90,6 +90,14 @@ test_expect_success setup '\n \tgit commit -m \"Rearranged lines in dir/sub\" &&\n \tgit checkout master &&\n \n+\tGIT_AUTHOR_DATE=\"2006-06-26 00:06:00 +0000\" &&\n+\tGIT_COMMITTER_DATE=\"2006-06-26 00:06:00 +0000\" &&\n+\texport GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&\n+\tgit checkout -b mode initial &&\n+\tgit update-index --chmod=+x file0 &&\n+\tgit commit -m \"update mode\" &&\n+\tgit checkout -f master &&\n+\n \tgit config diff.renames false &&\n \n \tgit show-branch\n@@ -192,6 +200,10 @@ diff-tree --pretty side\n diff-tree --pretty -p side\n diff-tree --pretty --patch-with-stat side\n \n+diff-tree initial mode\n+diff-tree --stat initial mode\n+diff-tree --summary initial mode\n+\n diff-tree master\n diff-tree -p master\n diff-tree -p -m master\ndiff --git a/t/t4013/diff.diff-tree_--stat_initial_mode b/t/t4013/diff.diff-tree_--stat_initial_mode\nnew file mode 100644\nindex 0000000000..0e5943c2c6\n--- /dev/null\n+++ b/t/t4013/diff.diff-tree_--stat_initial_mode\n@@ -0,0 +1,4 @@\n+$ git diff-tree --stat initial mode\n+ file0 | 0\n+ 1 file changed, 0 insertions(+), 0 deletions(-)\n+$\ndiff --git a/t/t4013/diff.diff-tree_--summary_initial_mode b/t/t4013/diff.diff-tree_--summary_initial_mode\nnew file mode 100644\nindex 0000000000..25846b6af8\n--- /dev/null\n+++ b/t/t4013/diff.diff-tree_--summary_initial_mode\n@@ -0,0 +1,3 @@\n+$ git diff-tree --summary initial mode\n+ mode change 100644 => 100755 file0\n+$\ndiff --git a/t/t4013/diff.diff-tree_initial_mode b/t/t4013/diff.diff-tree_initial_mode\nnew file mode 100644\nindex 0000000000..c47c09423e\n--- /dev/null\n+++ b/t/t4013/diff.diff-tree_initial_mode\n@@ -0,0 +1,3 @@\n+$ git diff-tree initial mode\n+:100644 100755 01e79c32a8c99c557f0757da7cb6d65b3414466d 01e79c32a8c99c557f0757da7cb6d65b3414466d M\tfile0\n+$\ndiff --git a/t/t4013/diff.log_--decorate=full_--all b/t/t4013/diff.log_--decorate=full_--all\nindex b345b2ebfa..2afe91f116 100644\n--- a/t/t4013/diff.log_--decorate=full_--all\n+++ b/t/t4013/diff.log_--decorate=full_--all\n@@ -1,4 +1,10 @@\n $ git log --decorate=full --all\n+commit b7e0bc69303b488b47deca799a7d723971dfa6cd (refs/heads/mode)\n+Author: A U Thor \n+Date: Mon Jun 26 00:06:00 2006 +0000\n+\n+ update mode\n+\n commit cd4e72fd96faed3f0ba949dc42967430374e2290 (refs/heads/rearrange)\n Author: A U Thor \n Date: Mon Jun 26 00:06:00 2006 +0000\ndiff --git a/t/t4013/diff.log_--decorate_--all b/t/t4013/diff.log_--decorate_--all\nindex 3aa16a9e42..d0f308ab2b 100644\n--- a/t/t4013/diff.log_--decorate_--all\n+++ b/t/t4013/diff.log_--decorate_--all\n@@ -1,4 +1,10 @@\n $ git log --decorate --all\n+commit b7e0bc69303b488b47deca799a7d723971dfa6cd (mode)\n+Author: A U Thor \n+Date: Mon Jun 26 00:06:00 2006 +0000\n+\n+ update mode\n+\n commit cd4e72fd96faed3f0ba949dc42967430374e2290 (rearrange)\n Author: A U Thor \n Date: Mon Jun 26 00:06:00 2006 +0000\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"329076","messageId":"20170927231516.GG19555@aiede.mtv.corp.google.com","threadId":"46846","inReplyTo":"20170927225126.4836-1-sbeller@google.com","subject":"Re: [PATCH] diff: correct newline in summary for renamed files","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-27T23:15:16Z","receivedAt":"2017-09-27T23:15:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> In 146fdb0dfe (diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY,\n> 2017-06-29), the conversion from direct printing to the symbol emission\n> dropped the new line character for renamed, copied and rewritten files.\n>\n> Add the emission of a newline, add a test for this case.\n>\n> Reported-by: Linus Torvalds \n> Helped-by: Jeff King \n> Signed-off-by: Stefan Beller \n> ---\n> diff.c | 1 +\n> t/t4013-diff-various.sh | 12 ++++++++++++\n> t/t4013/diff.diff-tree_--stat_initial_mode | 4 ++++\n> t/t4013/diff.diff-tree_--summary_initial_mode | 3 +++\n> t/t4013/diff.diff-tree_initial_mode | 3 +++\n> t/t4013/diff.log_--decorate=full_--all | 6 ++++++\n> t/t4013/diff.log_--decorate_--all | 6 ++++++\n> 7 files changed, 35 insertions(+)\n> create mode 100644 t/t4013/diff.diff-tree_--stat_initial_mode\n> create mode 100644 t/t4013/diff.diff-tree_--summary_initial_mode\n> create mode 100644 t/t4013/diff.diff-tree_initial_mode\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"329078","messageId":"094ebbd4-1bc6-8360-222f-55a4a16a6ca3@ramsayjones.plus.com","threadId":"46846","inReplyTo":"20170927225126.4836-1-sbeller@google.com","subject":"Re: [PATCH] diff: correct newline in summary for renamed files","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-09-27T23:49:36Z","receivedAt":"2017-09-27T23:49:43Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 27/09/17 23:51, Stefan Beller wrote:\n> In 146fdb0dfe (diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY,\n> 2017-06-29), the conversion from direct printing to the symbol emission\n> dropped the new line character for renamed, copied and rewritten files.\n> \n> Add the emission of a newline, add a test for this case.\n> \n> Reported-by: Linus Torvalds \n> Helped-by: Jeff King \n> Signed-off-by: Stefan Beller \n> ---\n> \n> Peff, I am undecided about the added 'diff --stat' call as that\n> uses a completely different code path and would not show the mode\n> change, but I guess we can just use it to document the current state.\n> \n> Thanks,\n> Stefan\n> \n> diff.c | 1 +\n> t/t4013-diff-various.sh | 12 ++++++++++++\n> t/t4013/diff.diff-tree_--stat_initial_mode | 4 ++++\n> t/t4013/diff.diff-tree_--summary_initial_mode | 3 +++\n> t/t4013/diff.diff-tree_initial_mode | 3 +++\n> t/t4013/diff.log_--decorate=full_--all | 6 ++++++\n> t/t4013/diff.log_--decorate_--all | 6 ++++++\n> 7 files changed, 35 insertions(+)\n> create mode 100644 t/t4013/diff.diff-tree_--stat_initial_mode\n> create mode 100644 t/t4013/diff.diff-tree_--summary_initial_mode\n> create mode 100644 t/t4013/diff.diff-tree_initial_mode\n> \n> diff --git a/diff.c b/diff.c\n> index 3c6a3e0faa..653bb2e72e 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -5272,6 +5272,7 @@ static void show_mode_change(struct diff_options *opt, struct diff_filepair *p,\n> \t\t\tstrbuf_addch(&sb, ' ');\n> \t\t\tquote_c_style(p->two->path, &sb, NULL, 0);\n> \t\t}\n> +\t\tstrbuf_addch(&sb, '\\n');\n> \t\temit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,\n> \t\t\t\t sb.buf, sb.len, 0);\n> \t\tstrbuf_release(&sb);\n> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\n> index d09acfe48e..c515e3e53f 100755\n> --- a/t/t4013-diff-various.sh\n> +++ b/t/t4013-diff-various.sh\n> @@ -90,6 +90,14 @@ test_expect_success setup '\n> \tgit commit -m \"Rearranged lines in dir/sub\" &&\n> \tgit checkout master &&\n> \n> +\tGIT_AUTHOR_DATE=\"2006-06-26 00:06:00 +0000\" &&\n> +\tGIT_COMMITTER_DATE=\"2006-06-26 00:06:00 +0000\" &&\n> +\texport GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&\n> +\tgit checkout -b mode initial &&\n> +\tgit update-index --chmod=+x file0 &&\n\nwould 'test_chmod +x file0 &&' work here?\n\nATB,\nRamsay Jones\n\n"},{"id":"329079","messageId":"CAGZ79kYAzcjECZ0Pt3weyxE0V7J0WCM=M-MvM+HM9DgBDcJQTA@mail.gmail.com","threadId":"46846","inReplyTo":"094ebbd4-1bc6-8360-222f-55a4a16a6ca3@ramsayjones.plus.com","subject":"Re: [PATCH] diff: correct newline in summary for renamed files","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-09-27T23:57:40Z","receivedAt":"2017-09-27T23:57:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">> + GIT_AUTHOR_DATE=\"2006-06-26 00:06:00 +0000\" &&\n>> + GIT_COMMITTER_DATE=\"2006-06-26 00:06:00 +0000\" &&\n>> + export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&\n>> + git checkout -b mode initial &&\n>> + git update-index --chmod=+x file0 &&\n>\n> would 'test_chmod +x file0 &&' work here?\n\nThat is what I was looking for in my previous solution,\nthis one doesn't care about the on-disk things at all (regarding\nthe mode of files).\n\nSo I would argue that test_chmod may be overkill\n(well we could drop the force flag from the following\ncheckout... not sure if that is a good trade off)\n"},{"id":"329081","messageId":"20170928003518.z5zccj762tov4moj@sigill.intra.peff.net","threadId":"46846","inReplyTo":"20170927225126.4836-1-sbeller@google.com","subject":"Re: [PATCH] diff: correct newline in summary for renamed files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-28T00:35:18Z","receivedAt":"2017-09-28T00:35:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 27, 2017 at 03:51:26PM -0700, Stefan Beller wrote:\n\n> In 146fdb0dfe (diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY,\n> 2017-06-29), the conversion from direct printing to the symbol emission\n> dropped the new line character for renamed, copied and rewritten files.\n> \n> Add the emission of a newline, add a test for this case.\n> \n> Reported-by: Linus Torvalds \n> Helped-by: Jeff King \n> Signed-off-by: Stefan Beller \n> ---\n> \n> Peff, I am undecided about the added 'diff --stat' call as that\n> uses a completely different code path and would not show the mode\n> change, but I guess we can just use it to document the current state.\n\nYeah, I agree it isn't showing much. I was mostly thinking it just made\nsense to test a bunch of formats across a mode change to make sure they\nall showed something reasonable. But it's a rather unlikely bug that\n\"--stat\" would _start_ showing something that it's not supposed.\n\nI suppose that \"--patch\" should also possibly be included, though I'd be\nsurprised if that isn't covered elsewhere (but then, I was surprised\nthat --summary wasn't covered either).\n\n-Peff\n"},{"id":"329082","messageId":"20170928003915.rlexebnpj4e5dsk7@sigill.intra.peff.net","threadId":"46846","inReplyTo":"CAGZ79kYAzcjECZ0Pt3weyxE0V7J0WCM=M-MvM+HM9DgBDcJQTA@mail.gmail.com","subject":"Re: [PATCH] diff: correct newline in summary for renamed files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-28T00:39:15Z","receivedAt":"2017-09-28T00:39:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 27, 2017 at 04:57:40PM -0700, Stefan Beller wrote:\n\n> >> + GIT_AUTHOR_DATE=\"2006-06-26 00:06:00 +0000\" &&\n> >> + GIT_COMMITTER_DATE=\"2006-06-26 00:06:00 +0000\" &&\n> >> + export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&\n> >> + git checkout -b mode initial &&\n> >> + git update-index --chmod=+x file0 &&\n> >\n> > would 'test_chmod +x file0 &&' work here?\n\nOh, I forgot we had that.\n\n> That is what I was looking for in my previous solution,\n> this one doesn't care about the on-disk things at all (regarding\n> the mode of files).\n\nIt still wouldn't have helped there. It does an on-disk chmod and an\nindex update, with the assumption that the chmod will be a noop on some\nsystems. So it's good if you want to put the executable bit in the index\nand you'd like the working tree to match if it can, or have its change\nignored otherwise.\n\nBut if you care about seeing a diff between the working tree and index,\nthat still wouldn't work.\n\n> So I would argue that test_chmod may be overkill\n> (well we could drop the force flag from the following\n> checkout... not sure if that is a good trade off)\n\nThe \"-f\" certainly caught me (I only added it after seeing the test\nfail). So maybe it's worth doing. I doubt it's all that big a deal\neither way.\n\n-Peff\n"},{"id":"329088","messageId":"xmqqd16b31i0.fsf@gitster.mtv.corp.google.com","threadId":"46846","inReplyTo":"CA+55aFzThd6cayUVuv39k=sb8KCphLkWGCV4n2HUjfn7Pxkd3g@mail.gmail.com","subject":"Re: diffstat summary mode change bug","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-28T04:12:23Z","receivedAt":"2017-09-28T04:12:30Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> and the reason seems to be that the '\\n' at the end got dropped as the\n> old code was very confusing (the old code had two different '\\n' cases\n> for the \"show filename or not\").\n>\n> I think the right fix is this whitespace-damaged trivial one-liner:\n>\n> diff --git a/diff.c b/diff.c\n> index 3c6a3e0fa..653bb2e72 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -5272,6 +5272,7 @@ static void show_mode_change(struct\n> diff_options *opt, struct diff_filepair *p,\n> strbuf_addch(&sb, ' ');\n> quote_c_style(p->two->path, &sb, NULL, 0);\n> }\n> + strbuf_addch(&sb, '\\n');\n> emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,\n> sb.buf, sb.len, 0);\n> strbuf_release(&sb);\n>\n> but somebody should double-check that.\n>\n> Linus\n\nThanks for being extra gentle by mentioning \"old code was very\nconfusing\", but it was not necessary. We should have caught this\nbreakage during the review, but I somehow failed to spot it. My\nbad.\n\nThanks for a fix.\n"},{"id":"329089","messageId":"xmqq8tgz31eo.fsf@gitster.mtv.corp.google.com","threadId":"46846","inReplyTo":"094ebbd4-1bc6-8360-222f-55a4a16a6ca3@ramsayjones.plus.com","subject":"Re: [PATCH] diff: correct newline in summary for renamed files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-28T04:14:23Z","receivedAt":"2017-09-28T04:14:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n>> +\tgit checkout -b mode initial &&\n>> +\tgit update-index --chmod=+x file0 &&\n>\n> would 'test_chmod +x file0 &&' work here?\n\nThis one wants to set +x only in the index, leaving the working tree\nversion without the executable bit. test_chmod is about setting\nboth, so it would do something different from what we want to do\nhere.\n"}]}