Files
git/t/unit-tests/u-trailer.c
Kristoffer Haugsbakk 581183734c trailers: stop recognizing URLs as trailers
An HTTPS URL starts with an alphanumeric scheme followed by a colon.
That means that they will be recognized as trailers in a trailer block.
That turns out to be a problem in practice. Let’s stop recognizing these
as trailers by failing the trailer parsing when we:

1. find the separator;
2. the separator and the next two characters form `://`; and
3. we haven’t parsed any whitespace yet.

The simplest example of how this can be a problem is for people who do
not use trailers but may leave URLs at the end of the commit message.
Now, while these authors might not use trailers themselves, other
authors may have used trailers and this metadata confusion can become a
problem once someone tries to extract that metadata (and non-metadata).

Let’s now look at some examples in the Linux Kernel[1] to see how this
is a problem in practice.

There are commits which contain intended non-trailer lines which start
with URLs. These are comments. Example with just the trailers:[2]

    Signed-off-by: Shuai Xue <xueshuai@linux.alibaba.com>
    [bhelgaas: squash fixes:
    https://lore.kernel.org/r/20260108013956.14351-2-bagasdotme@gmail.com
    https://lore.kernel.org/r/20260108013956.14351-3-bagasdotme@gmail.com]
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
    Link: https://patch.msgid.link/20251210132907.58799-4-xueshuai@linux.alibaba.com

Those `[]` pairs delimit the “squash fixes” comment.

Now, any of these two commands:

     git log --format='%(trailers:only)' -1 <commit>
     git log -1 --format=%B <commit> |
         git interpret-trailers --only-trailers

Will both wrongly (according to the surmised user intent) include these
two URL lines as trailers and also mangle the URLs, e.g.:

    https: //lore.kernel.org/r/20260108013956.14351-2-bagasdotme@gmail.com

Because the `--only-trailers` mode (or `only` for the git-log(1) format)
normalizes the output to a colon and a space.

Another example is linewrapping mistakes; a `Link` trailer with a
URL where the URL ended up on the next line, presumably because the
user’s editor linewrapped the “too long” line. Example with just the
trailers:[3]

    Link: https://patch.msgid.link/20260216-work-xattr-socket-v1-4-c2efa4f74cb7@kernel.org
    Link:
    https://lore.kernel.org/3cnmtqmakpbb2uwhenrj7kdqu3uefykiykjllgfbtpkiwhaa4s@sghkevv7jned [1]
    Acked-by: Darrick J. Wong <djwong@kernel.org>
    Reviewed-by: Jan Kara <jack@suse.cz>
    Signed-off-by: Christian Brauner <brauner@kernel.org>

Now, this intended trailer is already ruined, but interpreting the URL
as a standalone trailer only compounds the mistake.

Yet another example is the trailer machinery normalizing the trailer
block before application, resulting in a `https` trailer key in the
commit message itself. Example with just the trailers:[4]

    https: //sashiko.dev/#/patchset/20260429114208.941011-1-holger.brunck%40hitachienergy.com
    Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC")
    Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>
    Link: https://patch.msgid.link/20260507155332.3452319-1-holger.brunck@hitachienergy.com
    Signed-off-by: Jakub Kicinski <kuba@kernel.org>

We have a helpful `Link` that points to the original patch.[5] Following
it we can see that that `https` trailer was indeed a URL
originally (again just the trailer block here):

    https://sashiko.dev/#/patchset/20260429114208.941011-1-holger.brunck%40hitachienergy.com
    Fixes: c19b6d246a35 ("drivers/net: support hdlc function for QE-UCC")
    Signed-off-by: Holger Brunck <holger.brunck@hitachienergy.com>

So how did it end up as a `https` trailer? My theory is that the trailer
block was normalized on patch application, causing a URL comment to be
wrongly normalized and cemented in the commit message as a trailer.[6]

† 1: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/
† 2: commit 8236fc613d44e59f6736d6c3e9efffaf26ab7f00
† 3: commit 5bd97f5c5f241a5610c4412d1b93995a26241f81
† 4: commit 496c0c4c53bbe1bad97e82cd12103df61a6e459d
† 5: https://patch.msgid.link/20260507155332.3452319-1-holger.brunck@hitachienergy.com
† 6: There are only four commits in the Linux Kernel of this kind, and
     three of them have the same recurring person in the signoff chain.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
2026-08-03 07:40:58 -07:00

373 lines
7.1 KiB
C

#define DISABLE_SIGN_COMPARE_WARNINGS
#include "unit-test.h"
#include "trailer.h"
struct contents {
const char *raw;
const char *key;
const char *val;
};
static void t_trailer_iterator(const char *msg, size_t num_expected,
struct contents *contents)
{
struct trailer_iterator iter;
size_t i = 0;
trailer_iterator_init(&iter, msg);
while (trailer_iterator_advance(&iter)) {
if (num_expected) {
cl_assert_equal_s(iter.raw, contents[i].raw);
cl_assert_equal_s(iter.key.buf, contents[i].key);
cl_assert_equal_s(iter.val.buf, contents[i].val);
}
i++;
}
trailer_iterator_release(&iter);
cl_assert_equal_i(i, num_expected);
}
void test_trailer__empty_input(void)
{
struct contents expected_contents[] = { 0 };
t_trailer_iterator("", 0, expected_contents);
}
void test_trailer__no_newline_start(void)
{
struct contents expected_contents[] = { 0 };
t_trailer_iterator("Fixes: x\n"
"Acked-by: x\n"
"Reviewed-by: x\n",
0,
expected_contents);
}
void test_trailer__newline_start(void)
{
struct contents expected_contents[] = {
{
.raw = "Fixes: x\n",
.key = "Fixes",
.val = "x",
},
{
.raw = "Acked-by: x\n",
.key = "Acked-by",
.val = "x",
},
{
.raw = "Reviewed-by: x\n",
.key = "Reviewed-by",
.val = "x",
},
{
0
},
};
t_trailer_iterator("\n"
"Fixes: x\n"
"Acked-by: x\n"
"Reviewed-by: x\n",
3,
expected_contents);
}
void test_trailer__no_body_text(void)
{
struct contents expected_contents[] = {
{
.raw = "Fixes: x\n",
.key = "Fixes",
.val = "x",
},
{
.raw = "Acked-by: x\n",
.key = "Acked-by",
.val = "x",
},
{
.raw = "Reviewed-by: x\n",
.key = "Reviewed-by",
.val = "x",
},
{
0
},
};
t_trailer_iterator("subject: foo bar\n"
"\n"
"Fixes: x\n"
"Acked-by: x\n"
"Reviewed-by: x\n",
3,
expected_contents);
}
void test_trailer__body_text_no_divider(void)
{
struct contents expected_contents[] = {
{
.raw = "Fixes: x\n",
.key = "Fixes",
.val = "x",
},
{
.raw = "Acked-by: x\n",
.key = "Acked-by",
.val = "x",
},
{
.raw = "Reviewed-by: x\n",
.key = "Reviewed-by",
.val = "x",
},
{
.raw = "Signed-off-by: x\n",
.key = "Signed-off-by",
.val = "x",
},
{
0
},
};
t_trailer_iterator("my subject\n"
"\n"
"my body which is long\n"
"and contains some special\n"
"chars like : = ? !\n"
"hello\n"
"\n"
"Fixes: x\n"
"Acked-by: x\n"
"Reviewed-by: x\n"
"Signed-off-by: x\n",
4,
expected_contents);
}
void test_trailer__body_no_divider_2nd_block(void)
{
struct contents expected_contents[] = {
{
.raw = "Helped-by: x\n",
.key = "Helped-by",
.val = "x",
},
{
.raw = "Signed-off-by: x\n",
.key = "Signed-off-by",
.val = "x",
},
{
0
},
};
t_trailer_iterator("my subject\n"
"\n"
"my body which is long\n"
"and contains some special\n"
"chars like : = ? !\n"
"hello\n"
"\n"
"Fixes: x\n"
"Acked-by: x\n"
"Reviewed-by: x\n"
"Signed-off-by: x\n"
"\n"
/*
* Because this is the last trailer block, it takes
* precedence over the first one encountered above.
*/
"Helped-by: x\n"
"Signed-off-by: x\n",
2,
expected_contents);
}
void test_trailer__body_and_divider(void)
{
struct contents expected_contents[] = {
{
.raw = "Signed-off-by: x\n",
.key = "Signed-off-by",
.val = "x",
},
{
0
},
};
t_trailer_iterator("my subject\n"
"\n"
"my body which is long\n"
"and contains some special\n"
"chars like : = ? !\n"
"hello\n"
"\n"
"---\n"
"\n"
/*
* This trailer still counts because the iterator
* always ignores the divider.
*/
"Signed-off-by: x\n",
1,
expected_contents);
}
void test_trailer__non_trailer_in_block(void)
{
struct contents expected_contents[] = {
{
.raw = "not a trailer line\n",
.key = "not a trailer line",
.val = "",
},
{
.raw = "not a trailer line\n",
.key = "not a trailer line",
.val = "",
},
{
.raw = "not a trailer line\n",
.key = "not a trailer line",
.val = "",
},
{
.raw = "Signed-off-by: x\n",
.key = "Signed-off-by",
.val = "x",
},
{
0
},
};
t_trailer_iterator("subject: foo bar\n"
"\n"
/*
* Even though this trailer block has a non-trailer line
* in it, it's still a valid trailer block because it's
* at least 25% trailers and is Git-generated (see
* git_generated_prefixes[] in trailer.c).
*/
"not a trailer line\n"
"not a trailer line\n"
"not a trailer line\n"
"Signed-off-by: x\n",
/*
* Even though there is only really 1 real "trailer"
* (Signed-off-by), we still have 4 trailer objects
* because we still want to iterate through the entire
* block.
*/
4,
expected_contents);
}
void test_trailer__too_many_non_trailers(void)
{
struct contents expected_contents[] = { 0 };
t_trailer_iterator("subject: foo bar\n"
"\n"
/*
* This block has only 20% trailers, so it's below the
* 25% threshold.
*/
"not a trailer line\n"
"not a trailer line\n"
"not a trailer line\n"
"not a trailer line\n"
"Signed-off-by: x\n",
0,
expected_contents);
}
void test_trailer__one_non_trailer_no_git_trailers(void)
{
struct contents expected_contents[] = { 0 };
t_trailer_iterator("subject: foo bar\n"
"\n"
/*
* This block has only 1 non-trailer out of 10 (IOW, 90%
* trailers) but is not considered a trailer block
* because the 25% threshold only applies to cases where
* there was a Git-generated trailer.
*/
"Reviewed-by: x\n"
"Reviewed-by: x\n"
"Reviewed-by: x\n"
"Helped-by: x\n"
"Helped-by: x\n"
"Helped-by: x\n"
"Acked-by: x\n"
"Acked-by: x\n"
"Acked-by: x\n"
"not a trailer line\n",
0,
expected_contents);
}
void test_trailer__URL(void)
{
struct contents expected_contents[] = { 0 };
t_trailer_iterator("Subject: foo bar\n"
"\n"
/*
* We do not want to match URLs as trailers.
*/
"https://www.example.org\n",
0,
expected_contents);
}
void test_trailer__not_a_URL_space_after_separator(void)
{
struct contents expected_contents[] = {
{ .raw = "https: //www.example.org\n",
.key = "https",
.val = "//www.example.org" },
{ 0 },
};
t_trailer_iterator("Subject: foo bar\n"
"\n"
/*
* This has a space after ':' so it's not a URL.
*/
"https: //www.example.org\n",
1,
expected_contents);
}
void test_trailer__not_a_URL_space_before_separator(void)
{
struct contents expected_contents[] = {
{ .raw = "https ://www.example.org\n",
.key = "https",
.val = "//www.example.org" },
{ 0 },
};
t_trailer_iterator("Subject: foo bar\n"
"\n"
/*
* This has a space before ':' so it's not a URL.
*/
"https ://www.example.org\n",
1,
expected_contents);
}