problem with parsing of patch files for patch-id

From: Rob Linden <rlinden@redhat.com>
Date: Jun 21, 2024, 10:33 UTC
Message-ID: <CAP0H_AEd1jFNB_dO=HRjwEUKzFqnjntss_1wskKU6hE1VmBs+A@mail.gmail.com>

Hello!

We noticed a problem with the parsing of a patch file for "git patch-id". I understand that the patch file format is very difficult and unpredictable and probably it's not even possible to correctly parse all of them (mostly due to missing restrictions on or escaping of commit messages).

But in our specific case it can be improved to handle it correctly.

I attached an example patch file. When feeding that to "git patch-id" (with git version 2.45.2.561.g66ac6e4bcd) the output is:

068a8a30a5b0e55b93fdc16b2a7dcd6886e420f3 1111111111111111111111111111111111111111 818756276fff2c6075da8effe36c65d25e6ed592 dcc59dffa5116bf96618065cd60742cb660224b8 b033e3eca8a60741bb414689ddfe00a0c1a09de5 3333333333333333333333333333333333333333

But it should be:

068a8a30a5b0e55b93fdc16b2a7dcd6886e420f3 1111111111111111111111111111111111111111 818756276fff2c6075da8effe36c65d25e6ed592 2222222222222222222222222222222222222222 b033e3eca8a60741bb414689ddfe00a0c1a09de5 3333333333333333333333333333333333333333

The reason is that the commit message of the second patch contains commit hashes which are parsed as if they were the commit hash for the patch, and not just some message.

This patch (also attached) fixes it by only considering commit hashes in a "From xxxxx..." line:

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 583099cacf..4b8a41bde8 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -86,7 +86,7 @@ static int get_one_patchid(struct object_id
*next_oid, struct object_id *result,
                        continue;
                }

-               if (!get_oid_hex(p, next_oid)) {
+               if (starts_with(p-5, "From ") && !get_oid_hex(p, next_oid)) {
                        found_next = 1;
                        break;
                }

But I'm not sure it is ok in all other cases [which are handled
correctly now, i.e. it only makes it better for cases like ours,
without making it worse for anything else). The unit-tests pass ok but
I didn't check how comprehensive they are.
Can somebody please have a look and tell me what they think about
patch file parsing?

Thanks & all the best,
rob


From 1111111111111111111111111111111111111111 Mon Sep 17 00:00:00 2001
From: rlinden@redhat.com
Date: Wed, 24 Jan 2024 14:49:21 +0100
Subject: [PATCH 1/3] foo foo

Resolves: bug-123
---
 test/units/testsuite-07.main-PID-change.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/test/units/testsuite-07.main-PID-change.sh b/test/units/testsuite-07.main-PID-change.sh
index be4631f10d..bd5a63a272 100755
--- a/test/units/testsuite-07.main-PID-change.sh
+++ b/test/units/testsuite-07.main-PID-change.sh
@@ -151,6 +151,8 @@ systemd-run --unit=test-mainpidsh3.service \
             -p RuntimeDirectory=mainpidsh3 \
             -p PIDFile=/run/mainpidsh3/pid \
             -p DynamicUser=1 \
+            `# Make sanitizers happy when DynamicUser=1 pulls in instrumented systemd NSS modules` \
+            -p EnvironmentFile=-/usr/lib/systemd/systemd-asan-env \
             -p TimeoutStartSec=2s \
             /dev/shm/test-mainpid3.sh \
     && { echo 'unexpected success'; exit 1; }

From 2222222222222222222222222222222222222222 Mon Sep 17 00:00:00 2001
From: rlinden@redhat.com
Date: Wed, 24 Jan 2024 15:49:21 +0100
Subject: [PATCH 2/3] bar bar

we should not mention these other commits in our commit message like this:
55d337de1940076855c1687ffd588498d068724e
dcc59dffa5116bf96618065cd60742cb660224b8

---
 test/units/testsuite-07.main-PID-change.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/test/units/testsuite-07.main-PID-change.sh b/test/units/testsuite-07.main-PID-change.sh
index be4631f10d..bd5a63a272 100755
--- a/test/units/testsuite-07.main-PID-change.sh
+++ b/test/units/testsuite-07.main-PID-change.sh
@@ -151,6 +151,8 @@ systemd-run --unit=test-mainpidsh3.service \
             -p RuntimeDirectory=mainpidsh3 \
             -p PIDFile=/run/mainpidsh3/pid \
             -p DynamicUser=1 \
+            `# Make sanitizers happy when DynamicUser=1 pulls in instrumented systemd NSS modules` \
+            -p EnvironmentFile=-/usr/share/lib/systemd/systemd-asan-env \
             -p TimeoutStartSec=2s \
             /dev/shm/test-mainpid3.sh \
     && { echo 'unexpected success'; exit 1; }

From 3333333333333333333333333333333333333333 Mon Sep 17 00:00:00 2001
From: rlinden@redhat.com
Date: Wed, 24 Jan 2024 16:49:21 +0100
Subject: [PATCH 3/3] baz baz

that's it
---
 test/units/testsuite-07.main-PID-change.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/test/units/testsuite-07.main-PID-change.sh b/test/units/testsuite-07.main-PID-change.sh
index be4631f10d..bd5a63a272 100755
--- a/test/units/testsuite-07.main-PID-change.sh
+++ b/test/units/testsuite-07.main-PID-change.sh
@@ -151,6 +151,8 @@ systemd-run --unit=test-mainpidsh3.service \
             -p RuntimeDirectory=mainpidsh3 \
             -p PIDFile=/run/mainpidsh3/pid \
             -p DynamicUser=1 \
+            `# Make sanitizers happy when DynamicUser=1 pulls in instrumented systemd NSS modules` \
+            -p EnvironmentFile=-/usr/local/lib/systemd/systemd-asan-env \
             -p TimeoutStartSec=2s \
             /dev/shm/test-mainpid3.sh \
     && { echo 'unexpected success'; exit 1; }


diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index 583099cacf..4b8a41bde8 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -86,7 +86,7 @@ static int get_one_patchid(struct object_id *next_oid, struct object_id *result,
 			continue;
 		}
 
-		if (!get_oid_hex(p, next_oid)) {
+		if (starts_with(p-5, "From ") && !get_oid_hex(p, next_oid)) {
 			found_next = 1;
 			break;
 		}