{"thread":{"id":"49969","subject":"RFE: git-patch-id should handle patches without leading \"diff\"","startedAt":"2018-12-07T18:19:47Z","lastAt":"2018-12-08T06:02:07Z","messageCount":6,"participants":["Konstantin Ryabitsev","Ævar Arnfjörð Bjarmason","Jonathan Nieder","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"364749","messageId":"20181207181942.GA6411@pure.paranoia.local","threadId":"49969","inReplyTo":null,"subject":"RFE: git-patch-id should handle patches without leading \"diff\"","fromName":"Konstantin Ryabitsev","fromEmail":"konstantin@linuxfoundation.org","sentAt":"2018-12-07T18:19:42Z","receivedAt":"2018-12-07T18:19:47Z","isPatch":false,"sender":{"key":"konstantin@linuxfoundation.org","avatar":"https://gravatar.com/avatar/7cb8827c6de56e1bd2dea16508c6708aa43feed3bf3813bcdacecdf96ceadd79?d=mp&s=160"},"body":"Hi, all:\n\nEvery now and again I come across a patch sent to LKML without a leading\n\"diff a/foo b/foo\" -- usually produced by quilt. E.g.:\n\nhttps://lore.kernel.org/lkml/20181125185004.151077005@linutronix.de/\n\nI am guessing quilt does not bother including the leading \"diff a/foo\nb/foo\" because it's redundant with the next two lines, however this\nremains a valid patch recognized by git-am.\n\nIf you pipe that patch via git-patch-id, it produces nothing, but if I\nput in the leading \"diff\", like so:\n\ndiff a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c\n\nthen it properly returns \"fb3ae17451bc619e3d7f0dd647dfba2b9ce8992e\".\n\nCan we please teach git-patch-id to work without the leading diff a/foo\nb/foo, same as git-am?\n\nBest,\n-K\n"},{"id":"364752","messageId":"87va45xhoy.fsf@evledraar.gmail.com","threadId":"49969","inReplyTo":"20181207181942.GA6411@pure.paranoia.local","subject":"Re: RFE: git-patch-id should handle patches without leading \"diff\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-07T19:25:33Z","receivedAt":"2018-12-07T19:25:40Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 07 2018, Konstantin Ryabitsev wrote:\n\n> Hi, all:\n>\n> Every now and again I come across a patch sent to LKML without a leading\n> \"diff a/foo b/foo\" -- usually produced by quilt. E.g.:\n>\n> https://lore.kernel.org/lkml/20181125185004.151077005@linutronix.de/\n>\n> I am guessing quilt does not bother including the leading \"diff a/foo\n> b/foo\" because it's redundant with the next two lines, however this\n> remains a valid patch recognized by git-am.\n>\n> If you pipe that patch via git-patch-id, it produces nothing, but if I\n> put in the leading \"diff\", like so:\n>\n> diff a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c\n>\n> then it properly returns \"fb3ae17451bc619e3d7f0dd647dfba2b9ce8992e\".\n>\n> Can we please teach git-patch-id to work without the leading diff a/foo\n> b/foo, same as git-am?\n>\n> Best,\n> -K\n\nThe state machine is sensitive there being a \"diff\" line, then \"index\"\netc.\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 970d0d30b4..b99e4455fd 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -97,7 +97,9 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\t}\n\n \t\t/* Ignore commit comments */\n-\t\tif (!patchlen && !starts_with(line, \"diff \"))\n+\t\tif (!patchlen && starts_with(line, \"--- a/\"))\n+\t\t\t;\n+\t\telse if (!patchlen && !starts_with(line, \"diff \"))\n \t\t\tcontinue;\n\n \t\t/* Parsing diff header?  */\n\nThis would make it produce a patch-id for that input, however note that\nI've done \"--- a/\" there, with just \"--- \" (which is legit) we'd get\nconfused and start earlier before the diffstat.\n\nSo if you're interested in having this I leave it to you to run with\nthis & write tests for it, but more convincingly run it on the git &\nLKML archives and see that the output is the same (or just extra in case\nwhere we now find patches) with --stable etc.\n"},{"id":"364759","messageId":"20181207220116.GB73340@google.com","threadId":"49969","inReplyTo":"20181207181942.GA6411@pure.paranoia.local","subject":"Re: RFE: git-patch-id should handle patches without leading \"diff\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-12-07T22:01:16Z","receivedAt":"2018-12-07T22:01:21Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nKonstantin Ryabitsev wrote:\n\n> Every now and again I come across a patch sent to LKML without a leading\n> \"diff a/foo b/foo\" -- usually produced by quilt. E.g.:\n>\n> https://lore.kernel.org/lkml/20181125185004.151077005@linutronix.de/\n>\n> I am guessing quilt does not bother including the leading \"diff a/foo\n> b/foo\" because it's redundant with the next two lines, however this\n> remains a valid patch recognized by git-am.\n>\n> If you pipe that patch via git-patch-id, it produces nothing, but if I\n> put in the leading \"diff\", like so:\n>\n> diff a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c\n>\n> then it properly returns \"fb3ae17451bc619e3d7f0dd647dfba2b9ce8992e\".\n\nInteresting.  As Ævar mentioned, the relevant code is\n\n\t\t/* Ignore commit comments */\n\t\tif (!patchlen && !starts_with(line, \"diff \"))\n\t\t\tcontinue;\n\nwhich is trying to handle a case where a line that is special to the\nparser appears before the diff begins.\n\nThe patch-id appears to only care about the diff text, so it should be\nable to handle this.  So if we have a better heuristic for where the\ndiff starts, it would be good to use it.\n\n\"git apply\" uses apply.c::find_header, which is more permissive.\nMaybe it would be possible to unify these somehow.  (I haven't looked\nclosely enough to tell how painful that would be.)\n\nThanks and hope that helps,\nJonathan\n"},{"id":"364764","messageId":"87tvjpx9fy.fsf@evledraar.gmail.com","threadId":"49969","inReplyTo":"20181207220116.GB73340@google.com","subject":"Re: RFE: git-patch-id should handle patches without leading \"diff\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-12-07T22:23:45Z","receivedAt":"2018-12-07T22:23:50Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Dec 07 2018, Jonathan Nieder wrote:\n\n> Hi,\n>\n> Konstantin Ryabitsev wrote:\n>\n>> Every now and again I come across a patch sent to LKML without a leading\n>> \"diff a/foo b/foo\" -- usually produced by quilt. E.g.:\n>>\n>> https://lore.kernel.org/lkml/20181125185004.151077005@linutronix.de/\n>>\n>> I am guessing quilt does not bother including the leading \"diff a/foo\n>> b/foo\" because it's redundant with the next two lines, however this\n>> remains a valid patch recognized by git-am.\n>>\n>> If you pipe that patch via git-patch-id, it produces nothing, but if I\n>> put in the leading \"diff\", like so:\n>>\n>> diff a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c\n>>\n>> then it properly returns \"fb3ae17451bc619e3d7f0dd647dfba2b9ce8992e\".\n>\n> Interesting.  As Ævar mentioned, the relevant code is\n>\n> \t\t/* Ignore commit comments */\n> \t\tif (!patchlen && !starts_with(line, \"diff \"))\n> \t\t\tcontinue;\n>\n> which is trying to handle a case where a line that is special to the\n> parser appears before the diff begins.\n>\n> The patch-id appears to only care about the diff text, so it should be\n> able to handle this.  So if we have a better heuristic for where the\n> diff starts, it would be good to use it.\n\nNo, the patch-id doesn't just care about the diff, it cares about the\ncontext before the diff too.\n\nSee this patch:\n\n    $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~..\n    diff --git x/refs/files-backend.c y/refs/files-backend.c\n    index 9183875dad..dd8abe9185 100644\n    --- x/refs/files-backend.c\n    +++ y/refs/files-backend.c\n    @@ -180,7 +180,8 @@ static void files_reflog_path(struct files_ref_store *refs,\n                    break;\n            case REF_TYPE_OTHER_PSEUDOREF:\n            case REF_TYPE_MAIN_PSEUDOREF:\n    -               return files_reflog_path_other_worktrees(refs, sb, refname);\n    +               files_reflog_path_other_worktrees(refs, sb, refname);\n    +               break;\n            case REF_TYPE_NORMAL:\n                    strbuf_addf(sb, \"%s/logs/%s\", refs->gitcommondir, refname);\n                    break;\n\nObserve that the diff --git line matters, we hash it:\n\n    $ git diff-tree -p HEAD~.. | git patch-id\n    5870d115b7e2a9a936ab8fdc254932234413c710 0000000000000000000000000000000000000000\n    $ git diff-tree --src-prefix=a/ --dst-prefix=b/ -p HEAD~.. | git patch-id --stable\n    5870d115b7e2a9a936ab8fdc254932234413c710 0000000000000000000000000000000000000000\n    $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | git patch-id --stable\n    4cd136f2b98760150f700ac6a5b126389d6d05a7 0000000000000000000000000000000000000000\n\nThe thing it doesn't care about is the \"index\" between the \"diff\" and\npatch:\n\n    $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | grep -v ^index | git patch-id --stable\n    4cd136f2b98760150f700ac6a5b126389d6d05a7 0000000000000000000000000000000000000000\n\nWe also care about the +++ and --- lines:\n\n    $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | grep -v ^index | perl -pe 's/^(\\+\\+\\+|---).*/$1/g' | git patch-id\n    56985c2c38cce6079de2690082e1770a8e81214c 0000000000000000000000000000000000000000\n\nThen we normalize the @@ line, e.g.:\n\n    $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | grep -v ^index | git patch-id\n    4cd136f2b98760150f700ac6a5b126389d6d05a7 0000000000000000000000000000000000000000\n    $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | grep -v ^index | perl -pe 's/\\d+/123/g' | git patch-id\n    4cd136f2b98760150f700ac6a5b126389d6d05a7 0000000000000000000000000000000000000000\n\n\nThere's other caveats (see the code, e.g. \"strip space\") but to a first\napproximation a patch id is a hash of something that looks like this:\n    \n    diff --git x/refs/files-backend.c y/refs/files-backend.c\n    --- x/refs/files-backend.c\n    +++ y/refs/files-backend.c\n    @@ -123,123 +123,123 @@ static void files_reflog_path(struct files_ref_store *refs,\n                    break;\n            case REF_TYPE_OTHER_PSEUDOREF:\n            case REF_TYPE_MAIN_PSEUDOREF:\n    -               return files_reflog_path_other_worktrees(refs, sb, refname);\n    +               files_reflog_path_other_worktrees(refs, sb, refname);\n    +               break;\n            case REF_TYPE_NORMAL:\n                    strbuf_addf(sb, \"%s/logs/%s\", refs->gitcommondir, refname);\n                    break;\n\nWhich means that accepting a patch like this as input would actually\ngive you a different patch-id than if it had the proper header.\n\nSo it seems most sensible to me if this is going to be supported that we\ngo a bit beyond the call of duty and fake up the start of it, namely:\n\n    --- a/arch/x86/kernel/process.c\n    +++ b/arch/x86/kernel/process.c\n\nTo be:\n\n    diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c\n    --- a/arch/x86/kernel/process.c\n    +++ b/arch/x86/kernel/process.c\n\nIt'll make the state machine a bit more complex, but IMO it would suck\nmore if we generate a different hash depending on the tool generating\nthe diff. OTOH the \"diff --git\" line was never there, and it *does*\nmatter, so should we be faking it up? Maybe not, bah!\n\n> \"git apply\" uses apply.c::find_header, which is more permissive.\n> Maybe it would be possible to unify these somehow.  (I haven't looked\n> closely enough to tell how painful that would be.)\n>\n> Thanks and hope that helps,\n> Jonathan\n"},{"id":"364769","messageId":"20181207223406.GD73340@google.com","threadId":"49969","inReplyTo":"87tvjpx9fy.fsf@evledraar.gmail.com","subject":"Re: RFE: git-patch-id should handle patches without leading \"diff\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2018-12-07T22:34:06Z","receivedAt":"2018-12-07T22:34:11Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Fri, Dec 07 2018, Jonathan Nieder wrote:\n\n>> The patch-id appears to only care about the diff text, so it should be\n>> able to handle this.  So if we have a better heuristic for where the\n>> diff starts, it would be good to use it.\n>\n> No, the patch-id doesn't just care about the diff, it cares about the\n> context before the diff too.\n\nSorry, I did a bad job of communicating.  When I said \"diff text\", I was\nincluding context.\n\n[...]\n> Observe that the diff --git line matters, we hash it:\n>\n>     $ git diff-tree -p HEAD~.. | git patch-id\n>     5870d115b7e2a9a936ab8fdc254932234413c710 0000000000000000000000000000000000000000\n>     $ git diff-tree --src-prefix=a/ --dst-prefix=b/ -p HEAD~.. | git patch-id --stable\n>     5870d115b7e2a9a936ab8fdc254932234413c710 0000000000000000000000000000000000000000\n>     $ git diff-tree --src-prefix=x/ --dst-prefix=y/ -p HEAD~.. | git patch-id --stable\n>     4cd136f2b98760150f700ac6a5b126389d6d05a7 0000000000000000000000000000000000000000\n\nOh, hm.  That's unfortunate.\n\n[...]\n> So it seems most sensible to me if this is going to be supported that we\n> go a bit beyond the call of duty and fake up the start of it, namely:\n>\n>     --- a/arch/x86/kernel/process.c\n>     +++ b/arch/x86/kernel/process.c\n>\n> To be:\n>\n>     diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c\n>     --- a/arch/x86/kernel/process.c\n>     +++ b/arch/x86/kernel/process.c\n\nRight.  We may want to handle diff.mnemonicPrefix as well.\n\nJonathan\n"},{"id":"364790","messageId":"xmqqa7lg8sko.fsf@gitster-ct.c.googlers.com","threadId":"49969","inReplyTo":"20181207223406.GD73340@google.com","subject":"Re: RFE: git-patch-id should handle patches without leading \"diff\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-12-08T06:01:59Z","receivedAt":"2018-12-08T06:02:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n>> So it seems most sensible to me if this is going to be supported that we\n>> go a bit beyond the call of duty and fake up the start of it, namely:\n>>\n>>     --- a/arch/x86/kernel/process.c\n>>     +++ b/arch/x86/kernel/process.c\n>>\n>> To be:\n>>\n>>     diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c\n>>     --- a/arch/x86/kernel/process.c\n>>     +++ b/arch/x86/kernel/process.c\n>\n> Right.  We may want to handle diff.mnemonicPrefix as well.\n\nI definitely think under the --stable option, we should pretend as\nif the canonical a/ vs b/ prefixes were given with the \"diff --git\"\nheader, just like we try to reverse the effect of diff-orderfile,\netc.\n\nI am unsure what the right behaviour under --unstable is, though.\n\n\n"}]}