{"thread":{"id":"61666","subject":"problem with parsing of patch files for patch-id","startedAt":"2024-06-21T10:33:28Z","lastAt":"2024-07-13T16:25:28Z","messageCount":5,"participants":["Rob Linden","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"497477","messageId":"CAP0H_AEd1jFNB_dO=HRjwEUKzFqnjntss_1wskKU6hE1VmBs+A@mail.gmail.com","threadId":"61666","inReplyTo":null,"subject":"problem with parsing of patch files for patch-id","fromName":"Rob Linden","fromEmail":"rlinden@redhat.com","sentAt":"2024-06-21T10:33:10Z","receivedAt":"2024-06-21T10:33:28Z","isPatch":false,"sender":{"key":"rlinden@redhat.com","avatar":null},"body":"Hello!\n\nWe noticed a problem with the parsing of a patch file for \"git\npatch-id\". I understand that the patch file format is very difficult\nand unpredictable and probably it's not even possible to correctly\nparse all of them (mostly due to missing restrictions on or escaping\nof commit messages).\n\nBut in our specific case it can be improved to handle it correctly.\n\nI attached an example patch file. When feeding that to \"git patch-id\"\n(with git version 2.45.2.561.g66ac6e4bcd) the output is:\n\n068a8a30a5b0e55b93fdc16b2a7dcd6886e420f3\n1111111111111111111111111111111111111111\n818756276fff2c6075da8effe36c65d25e6ed592\ndcc59dffa5116bf96618065cd60742cb660224b8\nb033e3eca8a60741bb414689ddfe00a0c1a09de5\n3333333333333333333333333333333333333333\n\nBut it should be:\n\n068a8a30a5b0e55b93fdc16b2a7dcd6886e420f3\n1111111111111111111111111111111111111111\n818756276fff2c6075da8effe36c65d25e6ed592\n2222222222222222222222222222222222222222\nb033e3eca8a60741bb414689ddfe00a0c1a09de5\n3333333333333333333333333333333333333333\n\nThe reason is that the commit message of the second patch contains\ncommit hashes which are parsed as if they were the commit hash for the\npatch, and not just some message.\n\nThis patch (also attached) fixes it by only considering commit hashes\nin a \"From xxxxx...\" line:\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 583099cacf..4b8a41bde8 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -86,7 +86,7 @@ static int get_one_patchid(struct object_id\n*next_oid, struct object_id *result,\n continue;\n }\n\n- if (!get_oid_hex(p, next_oid)) {\n+ if (starts_with(p-5, \"From \") && !get_oid_hex(p, next_oid)) {\n found_next = 1;\n break;\n }\n\nBut I'm not sure it is ok in all other cases [which are handled\ncorrectly now, i.e. it only makes it better for cases like ours,\nwithout making it worse for anything else). The unit-tests pass ok but\nI didn't check how comprehensive they are.\nCan somebody please have a look and tell me what they think about\npatch file parsing?\n\nThanks & all the best,\nrob\n\n\nFrom 1111111111111111111111111111111111111111 Mon Sep 17 00:00:00 2001\nFrom: rlinden@redhat.com\nDate: Wed, 24 Jan 2024 14:49:21 +0100\nSubject: [PATCH 1/3] foo foo\n\nResolves: bug-123\n---\n test/units/testsuite-07.main-PID-change.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/test/units/testsuite-07.main-PID-change.sh b/test/units/testsuite-07.main-PID-change.sh\nindex be4631f10d..bd5a63a272 100755\n--- a/test/units/testsuite-07.main-PID-change.sh\n+++ b/test/units/testsuite-07.main-PID-change.sh\n@@ -151,6 +151,8 @@ systemd-run --unit=test-mainpidsh3.service \\\n -p RuntimeDirectory=mainpidsh3 \\\n -p PIDFile=/run/mainpidsh3/pid \\\n -p DynamicUser=1 \\\n+ `# Make sanitizers happy when DynamicUser=1 pulls in instrumented systemd NSS modules` \\\n+ -p EnvironmentFile=-/usr/lib/systemd/systemd-asan-env \\\n -p TimeoutStartSec=2s \\\n /dev/shm/test-mainpid3.sh \\\n && { echo 'unexpected success'; exit 1; }\n\nFrom 2222222222222222222222222222222222222222 Mon Sep 17 00:00:00 2001\nFrom: rlinden@redhat.com\nDate: Wed, 24 Jan 2024 15:49:21 +0100\nSubject: [PATCH 2/3] bar bar\n\nwe should not mention these other commits in our commit message like this:\n55d337de1940076855c1687ffd588498d068724e\ndcc59dffa5116bf96618065cd60742cb660224b8\n\n---\n test/units/testsuite-07.main-PID-change.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/test/units/testsuite-07.main-PID-change.sh b/test/units/testsuite-07.main-PID-change.sh\nindex be4631f10d..bd5a63a272 100755\n--- a/test/units/testsuite-07.main-PID-change.sh\n+++ b/test/units/testsuite-07.main-PID-change.sh\n@@ -151,6 +151,8 @@ systemd-run --unit=test-mainpidsh3.service \\\n -p RuntimeDirectory=mainpidsh3 \\\n -p PIDFile=/run/mainpidsh3/pid \\\n -p DynamicUser=1 \\\n+ `# Make sanitizers happy when DynamicUser=1 pulls in instrumented systemd NSS modules` \\\n+ -p EnvironmentFile=-/usr/share/lib/systemd/systemd-asan-env \\\n -p TimeoutStartSec=2s \\\n /dev/shm/test-mainpid3.sh \\\n && { echo 'unexpected success'; exit 1; }\n\nFrom 3333333333333333333333333333333333333333 Mon Sep 17 00:00:00 2001\nFrom: rlinden@redhat.com\nDate: Wed, 24 Jan 2024 16:49:21 +0100\nSubject: [PATCH 3/3] baz baz\n\nthat's it\n---\n test/units/testsuite-07.main-PID-change.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/test/units/testsuite-07.main-PID-change.sh b/test/units/testsuite-07.main-PID-change.sh\nindex be4631f10d..bd5a63a272 100755\n--- a/test/units/testsuite-07.main-PID-change.sh\n+++ b/test/units/testsuite-07.main-PID-change.sh\n@@ -151,6 +151,8 @@ systemd-run --unit=test-mainpidsh3.service \\\n -p RuntimeDirectory=mainpidsh3 \\\n -p PIDFile=/run/mainpidsh3/pid \\\n -p DynamicUser=1 \\\n+ `# Make sanitizers happy when DynamicUser=1 pulls in instrumented systemd NSS modules` \\\n+ -p EnvironmentFile=-/usr/local/lib/systemd/systemd-asan-env \\\n -p TimeoutStartSec=2s \\\n /dev/shm/test-mainpid3.sh \\\n && { echo 'unexpected success'; exit 1; }\n\n\ndiff --git a/builtin/patch-id.c b/builtin/patch-id.c\nindex 583099cacf..4b8a41bde8 100644\n--- a/builtin/patch-id.c\n+++ b/builtin/patch-id.c\n@@ -86,7 +86,7 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,\n \t\t\tcontinue;\n \t\t}\n \n-\t\tif (!get_oid_hex(p, next_oid)) {\n+\t\tif (starts_with(p-5, \"From \") && !get_oid_hex(p, next_oid)) {\n \t\t\tfound_next = 1;\n \t\t\tbreak;\n \t\t}\n"},{"id":"497503","messageId":"xmqqbk3uns4s.fsf@gitster.g","threadId":"61666","inReplyTo":"CAP0H_AEd1jFNB_dO=HRjwEUKzFqnjntss_1wskKU6hE1VmBs+A@mail.gmail.com","subject":"Re: problem with parsing of patch files for patch-id","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-21T17:05:23Z","receivedAt":"2024-06-21T17:05:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rob Linden writes:\n\n> This patch (also attached) fixes it by only considering commit hashes\n> in a \"From xxxxx...\" line:\n\nIf I am not mistaken, \"git patch-id\" was designed to read from\n\n git rev-list ... commit range ... | git diff-tree --stdin -p\n\nwhere we see\n\n 9005149a4a77e2d3409c6127bf4fd1a0893c3495\n diff --git a/path b/path\n index ...\n ... patch text here ...\n\nso I would suspect that limiting the commit object names only to\nthose that follow \"From \" (i.e. the format-patch output or output\nwith the \"--format=email\" option) would break existing use cases big\ntime.\n"},{"id":"497511","messageId":"CAP0H_AGw2R6JPxd=8gAn0w1Fy6iY71LRkH+F7uHL0PVmE04B_g@mail.gmail.com","threadId":"61666","inReplyTo":"xmqqbk3uns4s.fsf@gitster.g","subject":"Re: problem with parsing of patch files for patch-id","fromName":"Rob Linden","fromEmail":"rlinden@redhat.com","sentAt":"2024-06-21T18:15:47Z","receivedAt":"2024-06-21T18:16:02Z","isPatch":false,"sender":{"key":"rlinden@redhat.com","avatar":null},"body":"Hello Junio!\nThanks for the clue, You're right... I only work with the email format\nso I didn't think of that.\nMy solution doesn't work then...\nI had a different idea first: to check if we already got an oid and\nonly read a new one once\nthe current diff is finished (and wasn't empty so far). The other one\nseemed just simpler.\nI will try again...\nThanks & all the best,\nrob\n\nOn Fri, Jun 21, 2024 at 7:05 PM Junio C Hamano wrote:\n>\n> Rob Linden writes:\n>\n> > This patch (also attached) fixes it by only considering commit hashes\n> > in a \"From xxxxx...\" line:\n>\n> If I am not mistaken, \"git patch-id\" was designed to read from\n>\n> git rev-list ... commit range ... | git diff-tree --stdin -p\n>\n> where we see\n>\n> 9005149a4a77e2d3409c6127bf4fd1a0893c3495\n> diff --git a/path b/path\n> index ...\n> ... patch text here ...\n>\n> so I would suspect that limiting the commit object names only to\n> those that follow \"From \" (i.e. the format-patch output or output\n> with the \"--format=email\" option) would break existing use cases big\n> time.\n>\n\n"},{"id":"497516","messageId":"xmqq7ceiktzj.fsf@gitster.g","threadId":"61666","inReplyTo":"xmqqbk3uns4s.fsf@gitster.g","subject":"Re: problem with parsing of patch files for patch-id","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-21T18:53:36Z","receivedAt":"2024-06-21T18:53:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Rob Linden writes:\n>\n>> This patch (also attached) fixes it by only considering commit hashes\n>> in a \"From xxxxx...\" line:\n>\n> If I am not mistaken, \"git patch-id\" was designed to read from\n>\n> git rev-list ... commit range ... | git diff-tree --stdin -p\n>\n> where we see\n>\n> 9005149a4a77e2d3409c6127bf4fd1a0893c3495\n> diff --git a/path b/path\n> index ...\n> ... patch text here ...\n>\n> so I would suspect that limiting the commit object names only to\n> those that follow \"From \" (i.e. the format-patch output or output\n> with the \"--format=email\" option) would break existing use cases big\n> time.\n\nLet's do this to make sure we have a baseline that we will not\nbreak.\n\n------- >8 ------------- >8 ------------- >8 -------\n[PATCH] t4204: patch-id supports various input format\n\n\"git patch-id\" was first developed to read from \"git diff-tree\n--stdin -p\" output. Later it was enhanced to read from \"git\ndiff-tree --stdin -p -v\", which was the downstream of an early\nimitation of \"git log\" (\"git rev-list\" run in the upstream of a pipe\nto feed the \"diff-tree\"). These days, we also read from \"git\nformat-patch\".\n\nTheir output begins slightly differently, but the patch-id computed\nover them for the same commit should be the same. Ensure that we\nwon't accidentally break this expectation.\n\nSigned-off-by: Junio C Hamano \n---\n t/t4204-patch-id.sh | 23 +++++++++++++++++++++++\n 1 file changed, 23 insertions(+)\n\ndiff --git c/t/t4204-patch-id.sh w/t/t4204-patch-id.sh\nindex 605faea0c7..ebce72b2ce 100755\n--- c/t/t4204-patch-id.sh\n+++ w/t/t4204-patch-id.sh\n@@ -114,6 +114,29 @@ test_expect_success 'patch-id supports git-format-patch output' '\n \ttest \"$2\" = $(git rev-parse HEAD)\n '\n \n+test_expect_success 'patch-id computes the same for various formats' '\n+\t# This test happens to consider \"git log -p -1\" output\n+\t# the canonical input format, so use it as the norm.\n+\tgit log -1 -p same >log-p.output &&\n+\tgit patch-id expect &&\n+\n+\t# format-patch begins with \"From \"\n+\tgit format-patch -1 --stdout same >format-patch.output &&\n+\tgit patch-id actual &&\n+\ttest_cmp actual expect &&\n+\n+\t# \"diff-tree --stdin -p\" begins with \"\"\n+\tsame=$(git rev-parse same) &&\n+\techo $same | git diff-tree --stdin -p >diff-tree.output &&\n+\tgit patch-id actual &&\n+\ttest_cmp actual expect &&\n+\n+\t# \"diff-tree --stdin -v -p\" begins with \"commit \"\n+\techo $same | git diff-tree --stdin -p -v >diff-tree-v.output &&\n+\tgit patch-id actual &&\n+\ttest_cmp actual expect\n+'\n+\n test_expect_success 'whitespace is irrelevant in footer' '\n \tget_patch_id main &&\n \tgit checkout same &&\n"},{"id":"498628","messageId":"xmqqh6ctb6oa.fsf@gitster.g","threadId":"61666","inReplyTo":"CAP0H_AGw2R6JPxd=8gAn0w1Fy6iY71LRkH+F7uHL0PVmE04B_g@mail.gmail.com","subject":"Re: problem with parsing of patch files for patch-id","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-13T16:25:25Z","receivedAt":"2024-07-13T16:25:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rob Linden writes:\n\n>> Rob Linden writes:\n>>\n>> > This patch (also attached) fixes it by only considering commit hashes\n>> > in a \"From xxxxx...\" line:\n>>\n>> If I am not mistaken, \"git patch-id\" was designed to read from\n>>\n>> git rev-list ... commit range ... | git diff-tree --stdin -p\n>>\n>> where we see\n>>\n>> 9005149a4a77e2d3409c6127bf4fd1a0893c3495\n>> diff --git a/path b/path\n>> index ...\n>> ... patch text here ...\n>>\n>> so I would suspect that limiting the commit object names only to\n>> those that follow \"From \" (i.e. the format-patch output or output\n>> with the \"--format=email\" option) would break existing use cases big\n>> time.\n> ...\n> Hello Junio!\n> Thanks for the clue, You're right... I only work with the email format\n> so I didn't think of that.\n> My solution doesn't work then...\n> I had a different idea first: to check if we already got an oid and\n> only read a new one once\n> the current diff is finished (and wasn't empty so far). The other one\n> seemed just simpler.\n> I will try again...\n> Thanks & all the best,\n> rob\n\nSince then I sent a series [*] that was designed to address the\nissue you raised here, but unfortunately nobody seems to have paid\nattention and the patches are left hanging. It is part of my 'seen'\nbranch and in the broken-out format parked on the jc/patch-id branch\nin the https://github.com/gitster/git/ repository, ending with the\ncommit 3226bd87 (patch-id: tighten code to detect the patch header,\n2024-06-21).\n\nIf you can test (and if possible code review) them, that may help\nthe series to move forward.\n\nThanks.\n\n\n\n[Reference]\n\n * https://lore.kernel.org/git/20240621231826.3280338-1-gitster@pobox.com/\n"}]}