{"thread":{"id":"56315","subject":"git format-patch produces invalid patch if the commit adds an empty file?","startedAt":"2021-08-17T18:50:51Z","lastAt":"2021-08-20T21:09:52Z","messageCount":6,"participants":["Adam Williamson","Junio C Hamano","Gwyneth Morgan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"433019","messageId":"02be6a48411fa100e7d1292fc312f7fcf571f334.camel@redhat.com","threadId":"56315","inReplyTo":null,"subject":"git format-patch produces invalid patch if the commit adds an empty file?","fromName":"Adam Williamson","fromEmail":"awilliam@redhat.com","sentAt":"2021-08-17T18:50:42Z","receivedAt":"2021-08-17T18:50:51Z","isPatch":false,"sender":{"key":"awilliam@redhat.com","avatar":"https://avatars.githubusercontent.com/u/916551?v=4"},"body":"Hi folks! So I ran into an odd issue with git today. I'm kinda\nsurprised I can't find any prior discussion of it, but oh well. The\nsituation is this: I ran git format-patch on a commit that adds three\nempty files to a repository - this commit:\nhttps://github.com/mesonbuild/meson/commit/5c87167a34c6ed703444af180fffd8a45a7928ee\nthe relevant lines from the patch file it produced look like this:\n\n===\n\ndiff --git a/test cases/common/56 array methods/a.txt b/test cases/common/56 array methods/a.txt\nnew file mode 100644\nindex 000000000..e69de29bb\ndiff --git a/test cases/common/56 array methods/b.txt b/test cases/common/56 array methods/b.txt\nnew file mode 100644\nindex 000000000..e69de29bb\ndiff --git a/test cases/common/56 array methods/c.txt b/test cases/common/56 array methods/c.txt\nnew file mode 100644\nindex 000000000..e69de29bb\n\n===\n\nbut `patch` actually chokes on that (when called in an RPM package build):\n\n===\n\n+ /usr/bin/cat /home/adamw/build/meson/0001-interpreter-Fix-list-contains-for-Holders-fixes-9020.patch\n+ /usr/bin/patch -p1 -s --fuzz=0 --no-backup-if-mismatch -f\nThe text leading up to this was:\n--------------------------\n|diff --git a/test cases/common/56 array methods/a.txt b/test cases/common/56 array methods/a.txt\n|new file mode 100644\n|index 000000000..e69de29bb\n--------------------------\nNo file to patch.  Skipping patch.\nThe text leading up to this was:\n--------------------------\n|diff --git a/test cases/common/56 array methods/b.txt b/test cases/common/56 array methods/b.txt\n|new file mode 100644\n|index 000000000..e69de29bb\n--------------------------\nNo file to patch.  Skipping patch.\nThe text leading up to this was:\n--------------------------\n|diff --git a/test cases/common/56 array methods/c.txt b/test cases/common/56 array methods/c.txt\n|new file mode 100644\n|index 000000000..e69de29bb\n--------------------------\nNo file to patch.  Skipping patch.\n\n===\n\nTo make the patch apply cleanly, I had to hand-edit it to add \"---\" and\n\"+++\" lines, like this:\n\n===\n\ndiff --git a/test cases/common/56 array methods/a.txt b/test cases/common/56 array methods/a.txt\nnew file mode 100644\nindex 000000000..e69de29bb\n--- /dev/null\n+++ b/test cases/common/56 array methods/a.txt\ndiff --git a/test cases/common/56 array methods/b.txt b/test cases/common/56 array methods/b.txt\nnew file mode 100644\nindex 000000000..e69de29bb\n--- /dev/null\n+++ b/test cases/common/56 array methods/b.txt\ndiff --git a/test cases/common/56 array methods/c.txt b/test cases/common/56 array methods/c.txt\nnew file mode 100644\nindex 000000000..e69de29bb\n--- /dev/null\n+++ b/test cases/common/56 array methods/c.txt\n\n===\n\nThis is with git-2.32.0-1.fc35.1.x86_64 in Fedora Rawhide.\n\nI'm not subscribed to the list, so please CC me directly on any replies. Thanks!\n-- \nAdam Williamson\nFedora QA\nIRC: adamw | Twitter: adamw_ha\nhttps://www.happyassassin.net\n\n\n"},{"id":"433171","messageId":"xmqq5yw1ywdk.fsf@gitster.g","threadId":"56315","inReplyTo":"02be6a48411fa100e7d1292fc312f7fcf571f334.camel@redhat.com","subject":"Re: git format-patch produces invalid patch if the commit adds an empty file?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-19T21:09:43Z","receivedAt":"2021-08-19T21:09:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Williamson <awilliam@redhat.com> writes:\n\n> Hi folks! So I ran into an odd issue with git today. I'm kinda\n> surprised I can't find any prior discussion of it, but oh well. The\n> situation is this: I ran git format-patch on a commit that adds three\n> empty files to a repository - this commit:\n> https://github.com/mesonbuild/meson/commit/5c87167a34c6ed703444af180fffd8a45a7928ee\n> the relevant lines from the patch file it produced look like this:\n>\n> ===\n>\n> diff --git a/test cases/common/56 array methods/a.txt b/test cases/common/56 array methods/a.txt\n> new file mode 100644\n> index 000000000..e69de29bb\n> diff --git a/test cases/common/56 array methods/b.txt b/test cases/common/56 array methods/b.txt\n> new file mode 100644\n> index 000000000..e69de29bb\n> diff --git a/test cases/common/56 array methods/c.txt b/test cases/common/56 array methods/c.txt\n> new file mode 100644\n> index 000000000..e69de29bb\n\nI do not have very ancient build of Git handy, but I know Git as old\nas v1.3.0 (which I consider is one of the two versions of historical\nimportance, the other being v1.5.3) behaved this way and we haven't\nchanged it ever since, so I am surprised too to learn that \"GNU\npatch\" cannot grok it.  Even though you didn't mention it, am I\ncorrect to assume that \"patch\" has a similar issue with a change\nthat removes an empty file?\n\nI do not think our patch injestion machinery in \"git apply\" minds if\nwe added the \"--- /dev/null\" + \"+++ b/<path>\" headers (and the\nreverse for removal of an empty file) to the current output, and I\nam not fundamentally opposed to such a change.\n\nBut because it is such a rare event (and a discouraged practice) to\nrecord a completely empty file, I wouldn't place a high priority on\ndoing so myself.\n\nThanks.\n"},{"id":"433172","messageId":"953c8ccbbb282850191b199345465dac485b933e.camel@redhat.com","threadId":"56315","inReplyTo":"xmqq5yw1ywdk.fsf@gitster.g","subject":"Re: git format-patch produces invalid patch if the commit adds an empty file?","fromName":"Adam Williamson","fromEmail":"awilliam@redhat.com","sentAt":"2021-08-19T21:25:57Z","receivedAt":"2021-08-19T21:26:04Z","isPatch":false,"sender":{"key":"awilliam@redhat.com","avatar":"https://avatars.githubusercontent.com/u/916551?v=4"},"body":"On Thu, 2021-08-19 at 14:09 -0700, Junio C Hamano wrote:\n> Adam Williamson <awilliam@redhat.com> writes:\n> \n> > Hi folks! So I ran into an odd issue with git today. I'm kinda\n> > surprised I can't find any prior discussion of it, but oh well. The\n> > situation is this: I ran git format-patch on a commit that adds three\n> > empty files to a repository - this commit:\n> > https://github.com/mesonbuild/meson/commit/5c87167a34c6ed703444af180fffd8a45a7928ee\n> > the relevant lines from the patch file it produced look like this:\n> > \n> > ===\n> > \n> > diff --git a/test cases/common/56 array methods/a.txt b/test cases/common/56 array methods/a.txt\n> > new file mode 100644\n> > index 000000000..e69de29bb\n> > diff --git a/test cases/common/56 array methods/b.txt b/test cases/common/56 array methods/b.txt\n> > new file mode 100644\n> > index 000000000..e69de29bb\n> > diff --git a/test cases/common/56 array methods/c.txt b/test cases/common/56 array methods/c.txt\n> > new file mode 100644\n> > index 000000000..e69de29bb\n> \n> I do not have very ancient build of Git handy, but I know Git as old\n> as v1.3.0 (which I consider is one of the two versions of historical\n> importance, the other being v1.5.3) behaved this way and we haven't\n> changed it ever since, so I am surprised too to learn that \"GNU\n> patch\" cannot grok it.  Even though you didn't mention it, am I\n> correct to assume that \"patch\" has a similar issue with a change\n> that removes an empty file?\n\nHi Junio!\n\nI didn't test that. It does seem likely, though.\n\n> I do not think our patch injestion machinery in \"git apply\" minds if\n> we added the \"--- /dev/null\" + \"+++ b/<path>\" headers (and the\n> reverse for removal of an empty file) to the current output, and I\n> am not fundamentally opposed to such a change.\n> \n> But because it is such a rare event (and a discouraged practice) to\n> record a completely empty file, I wouldn't place a high priority on\n> doing so myself.\n\nThanks.\n-- \nAdam Williamson\nFedora QA\nIRC: adamw | Twitter: adamw_ha\nhttps://www.happyassassin.net\n\n\n"},{"id":"433201","messageId":"YR9Iaj/FqAyCMade@tilde.club","threadId":"56315","inReplyTo":"xmqq5yw1ywdk.fsf@gitster.g","subject":"Re: git format-patch produces invalid patch if the commit adds an empty file?","fromName":"Gwyneth Morgan","fromEmail":"gwymor@tilde.club","sentAt":"2021-08-20T06:15:06Z","receivedAt":"2021-08-20T06:15:26Z","isPatch":false,"sender":{"key":"gwymor@tilde.club","avatar":"https://avatars.githubusercontent.com/u/87623694?v=4"},"body":"On 2021-08-19 14:09:43-0700, Junio C Hamano wrote:\n> I do not think our patch injestion machinery in \"git apply\" minds if\n> we added the \"--- /dev/null\" + \"+++ b/<path>\" headers (and the\n> reverse for removal of an empty file) to the current output, and I\n> am not fundamentally opposed to such a change.\n> \n> But because it is such a rare event (and a discouraged practice) to\n> record a completely empty file, I wouldn't place a high priority on\n> doing so myself.\n\nGNU patch chokes in this case with an unquoted filename with spaces.\nHowever if we output\n\n\tdiff --git \"a/test cases/common/56 array methods/a.txt\" \"b/test cases/common/56 array methods/a.txt\"\n\ninstead of\n\n\tdiff --git a/test cases/common/56 array methods/a.txt b/test cases/common/56 array methods/a.txt\n\nGNU patch (and Git) will read it correctly. Rather than adding the \"---\"\n\"+++\" lines, could we instead quote filenames in the \"diff --git\" line\nwhen they contain spaces?\n"},{"id":"433202","messageId":"2119ea2a7b8cd3cb3a84d69b9a9f4471f645667d.camel@redhat.com","threadId":"56315","inReplyTo":"YR9Iaj/FqAyCMade@tilde.club","subject":"Re: git format-patch produces invalid patch if the commit adds an empty file?","fromName":"Adam Williamson","fromEmail":"awilliam@redhat.com","sentAt":"2021-08-20T06:46:34Z","receivedAt":"2021-08-20T06:46:41Z","isPatch":false,"sender":{"key":"awilliam@redhat.com","avatar":"https://avatars.githubusercontent.com/u/916551?v=4"},"body":"On Fri, 2021-08-20 at 06:15 +0000, Gwyneth Morgan wrote:\n> On 2021-08-19 14:09:43-0700, Junio C Hamano wrote:\n> > I do not think our patch injestion machinery in \"git apply\" minds if\n> > we added the \"--- /dev/null\" + \"+++ b/<path>\" headers (and the\n> > reverse for removal of an empty file) to the current output, and I\n> > am not fundamentally opposed to such a change.\n> > \n> > But because it is such a rare event (and a discouraged practice) to\n> > record a completely empty file, I wouldn't place a high priority on\n> > doing so myself.\n> \n> GNU patch chokes in this case with an unquoted filename with spaces.\n> However if we output\n> \n> \tdiff --git \"a/test cases/common/56 array methods/a.txt\" \"b/test cases/common/56 array methods/a.txt\"\n> \n> instead of\n> \n> \tdiff --git a/test cases/common/56 array methods/a.txt b/test cases/common/56 array methods/a.txt\n> \n> GNU patch (and Git) will read it correctly. Rather than adding the \"---\"\n> \"+++\" lines, could we instead quote filenames in the \"diff --git\" line\n> when they contain spaces?\n\nAha, I did actually wonder about that, because even with the added\nlines, the patches don't apply (via `patch`) on Fedora 33 and 34 (and\nthe error message after adding the lines does seem to indicate the\nspaces in the filenames as the culprit). They only apply on Fedora 35\nand 36. I hadn't thought to just add quote marks, though of course it\nseems obvious now. So yeah, that seems likely to be the best fix. I'll\ntry and confirm your results tomorrow. Thanks!\n-- \nAdam Williamson\nFedora QA\nIRC: adamw | Twitter: adamw_ha\nhttps://www.happyassassin.net\n\n\n"},{"id":"433279","messageId":"xmqqbl5ru8kl.fsf@gitster.g","threadId":"56315","inReplyTo":"YR9Iaj/FqAyCMade@tilde.club","subject":"Re: git format-patch produces invalid patch if the commit adds an empty file?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-20T21:09:46Z","receivedAt":"2021-08-20T21:09:52Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Gwyneth Morgan <gwymor@tilde.club> writes:\n\n> GNU patch chokes in this case with an unquoted filename with spaces.\n\nWhen we settled what bytes (not characters) in a pathname will cause\nit to be quoted and how the quoting is done between us and GNU diff\nand patch maintainer back in Oct 2005, I thought that we excluded\nwhitespace from the bytes that need quoting [*].  And I do not\nrecall us changing the rule for pathname quoting since then (other\nthan introduction of core.quotepath to disable quoting bytes with\nthe 8th bit set).\n\nIt may be a \"recent\" change on the GNU patch side, and I do not\nthink we mind tweaking our diff output to be more accomodating iff\nthat observation is true.  I however understand that spaces in\npathnames are not so uncommon especially among non-programmers and\nthey may feel irritating having to see any pathname with spaces\nquoted.\n\n\n[Reference]\n\n* https://lore.kernel.org/git/Pine.LNX.4.64.0510111121030.14597@g5.osdl.org/\n"}]}