problem with parsing of patch files for patch-id

From: Rob Linden <hidden>
Date: 2024-06-21 10:33:28
Subsystem: the rest · Maintainer: Linus Torvalds

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

Attachments

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help