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@xxxxxxxxxx
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@xxxxxxxxxx
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@xxxxxxxxxx
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;
}
