{"thread":{"id":"44753","subject":"builtin difftool parsing issue","startedAt":"2016-12-22T04:53:11Z","lastAt":"2017-01-04T01:16:16Z","messageCount":5,"participants":["Paul Sbarra","Johannes Schindelin","Stefan Beller","Jacob Keller"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"308237","messageId":"CAGf+dShpkPvsC8wQN6mWmYeMZ3=i-ZOzDNSM1aa0rinKW6+-+g@mail.gmail.com","threadId":"44753","inReplyTo":null,"subject":"builtin difftool parsing issue","fromName":"Paul Sbarra","fromEmail":"sbarra.paul@gmail.com","sentAt":"2016-12-22T04:53:04Z","receivedAt":"2016-12-22T04:53:11Z","isPatch":false,"sender":{"key":"sbarra.paul@gmail.com","avatar":null},"body":"Sadly, I haven't been able to figure out how to get the mbox file from\nthis tread into gmail, but wanted to report a parsing issue I've found\nwith the builtin difftool.\n\nOriginal Patch:\nhttps://public-inbox.org/git/ac91e4818cfb5c5af6b5874662dbeb61cde1f69d.1480019834.git.johannes.schindelin@gmx.de/#t\n\n> + *status = *++p;\n> + if (!status || p[1])\n> + return error(\"unexpected trailer: '%s'\", p);\n> + return 0;\n\nThe p[1] null check assumes the status is only one character long, but\ngit-diff's raw output format shows that a numeric value can follow in\nthe copy-edit and rename-edit cases.\n\nI'm looking forward to seeing the builtin difftool land.  I came across it\nwhile investigating adding --submodule=diff (expanding on diff's\nrecent addition) support and this looks more promising then the perl\nscript.  Hopefully I will make some progress.  Any tips/pointers would\nbe greatly appreciated.\n\nThanks\n"},{"id":"308674","messageId":"alpine.DEB.2.20.1701021712010.3469@virtualbox","threadId":"44753","inReplyTo":"CAGf+dShpkPvsC8wQN6mWmYeMZ3=i-ZOzDNSM1aa0rinKW6+-+g@mail.gmail.com","subject":"Re: builtin difftool parsing issue","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-02T16:12:57Z","receivedAt":"2017-01-02T16:13:23Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Paul,\n\nOn Wed, 21 Dec 2016, Paul Sbarra wrote:\n\n> Sadly, I haven't been able to figure out how to get the mbox file from\n> this tread into gmail, but wanted to report a parsing issue I've found\n> with the builtin difftool.\n> \n> Original Patch:\n> https://public-inbox.org/git/ac91e4818cfb5c5af6b5874662dbeb61cde1f69d.1480019834.git.johannes.schindelin@gmx.de/#t\n> \n> > + *status = *++p;\n> > + if (!status || p[1])\n> > + return error(\"unexpected trailer: '%s'\", p);\n> > + return 0;\n> \n> The p[1] null check assumes the status is only one character long, but\n> git-diff's raw output format shows that a numeric value can follow in\n> the copy-edit and rename-edit cases.\n\nThank you for the report! I fixed it locally.\n\n> I'm looking forward to seeing the builtin difftool land.  I came across it\n> while investigating adding --submodule=diff (expanding on diff's\n> recent addition) support and this looks more promising then the perl\n> script.  Hopefully I will make some progress.  Any tips/pointers would\n> be greatly appreciated.\n\nI would have expected `git difftool --submodule=diff ...` to work... What\nare the problems?\n\nCiao,\nJohannes\n"},{"id":"308691","messageId":"CAGf+dSjM9nuroeSM9mkQmO3ho4XcZhLo1CR76q-jbeQ-WNGG+Q@mail.gmail.com","threadId":"44753","inReplyTo":"alpine.DEB.2.20.1701021712010.3469@virtualbox","subject":"Re: builtin difftool parsing issue","fromName":"Paul Sbarra","fromEmail":"sbarra.paul@gmail.com","sentAt":"2017-01-02T19:05:11Z","receivedAt":"2017-01-02T19:19:00Z","isPatch":false,"sender":{"key":"sbarra.paul@gmail.com","avatar":null},"body":"> I would have expected `git difftool --submodule=diff ...` to work... What\n> are the problems?\n\nThe docs for difftool state...\n\"git difftool is a frontend to git diff and accepts the same options\nand arguments.\"\nwhich could lead a user to expect passing --submodule=diff to have a\nsimilar behavior for difftool.  It would be especially useful when\ncombined with --dir-diff.\n\nUnfortunately, due to the way the left/right directories are built up,\ndifftool needs to handle this option itself.  Currently a file\nrepresenting the submodule directory is created that contains the\nhash.\n\nif (S_ISGITLINK(lmode) || S_ISGITLINK(rmode)) {\n   strbuf_reset(&buf);\n   strbuf_addf(&buf, \"Subproject commit %s\", oid_to_hex(&loid));\n   add_left_or_right(&submodules, src_path, buf.buf, 0);\n   strbuf_reset(&buf);\n   strbuf_addf(&buf, \"Subproject commit %s\", oid_to_hex(&roid));\n   if (!oidcmp(&loid, &roid))\n      strbuf_addstr(&buf, \"-dirty\");\n   add_left_or_right(&submodules, dst_path, buf.buf, 1);\n   continue;\n}\n\nTo achieve the desired behavior a diff command would need to be run\nwithin the submodule.  A further complication is whether submodules\nshould be processed recursively.  I'm not sure whether or not diff\nhandles them recursively.  I believe the logic to parse and build up\nthe files would need to be factored out such that it could be called\nfor the super-project as well as each submodule change.\n\nThis is all out of scope for your effort as the existing (perl-based)\ndifftool doesn't do this either.  However, it's a feature that would\nprovide a significant simplification to the workflow used at the\noffice to review changes.\n"},{"id":"308712","messageId":"CAGZ79kaMnFUefiMagU82euxjeQdnc9KdgSwyDvkDp--QT-MbCA@mail.gmail.com","threadId":"44753","inReplyTo":"CAGf+dSjM9nuroeSM9mkQmO3ho4XcZhLo1CR76q-jbeQ-WNGG+Q@mail.gmail.com","subject":"Re: builtin difftool parsing issue","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-03T18:47:23Z","receivedAt":"2017-01-03T18:47:54Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jan 2, 2017 at 11:05 AM, Paul Sbarra <sbarra.paul@gmail.com> wrote:\n>> I would have expected `git difftool --submodule=diff ...` to work... What\n>> are the problems?\n>\n> The docs for difftool state...\n> \"git difftool is a frontend to git diff and accepts the same options\n> and arguments.\"\n\nI think such a sentence in the man page is dangerous, as nobody\nwas caught this issue until now. There have been numerous authors\nand reviewers that touched \"diff --submodule=<format>, but as there\nis no back-reference, hinting that the patch is only half done, and the\ndifftool also needs to implement such an option.\n\nWe should reword the man page either as\n\n  \"git difftool is a frontend to git diff and accepts the most(?) options\n  and arguments.\"\n\nor even be explicit and list the arguments instead. There we could also\ndescribe differences if any (e.g. the formats available might be different\nfor --submodule=<format>)\n"},{"id":"308754","messageId":"CA+P7+xqaOXagpXb53raPcD19uZRgYrOYH6O9pDPsAxE6TsMkvg@mail.gmail.com","threadId":"44753","inReplyTo":"CAGZ79kaMnFUefiMagU82euxjeQdnc9KdgSwyDvkDp--QT-MbCA@mail.gmail.com","subject":"Re: builtin difftool parsing issue","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2017-01-04T01:05:57Z","receivedAt":"2017-01-04T01:16:16Z","isPatch":false,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Tue, Jan 3, 2017 at 10:47 AM, Stefan Beller <sbeller@google.com> wrote:\n> On Mon, Jan 2, 2017 at 11:05 AM, Paul Sbarra <sbarra.paul@gmail.com> wrote:\n>>> I would have expected `git difftool --submodule=diff ...` to work... What\n>>> are the problems?\n>>\n>> The docs for difftool state...\n>> \"git difftool is a frontend to git diff and accepts the same options\n>> and arguments.\"\n>\n> I think such a sentence in the man page is dangerous, as nobody\n> was caught this issue until now. There have been numerous authors\n> and reviewers that touched \"diff --submodule=<format>, but as there\n> is no back-reference, hinting that the patch is only half done, and the\n> difftool also needs to implement such an option.\n>\n> We should reword the man page either as\n>\n>   \"git difftool is a frontend to git diff and accepts the most(?) options\n>   and arguments.\"\n>\n> or even be explicit and list the arguments instead. There we could also\n> describe differences if any (e.g. the formats available might be different\n> for --submodule=<format>)\n\nI agree with updating the documentation. Difftool would require\nextensive work to support the --submodule format, and I'm not sure\nit's worth it.\n\nThanks,\nJake\n"}]}