* Re: [PATCH] trailers: stop recognizing URLs as trailers
2026-08-02 19:57 ` [PATCH] trailers: stop recognizing URLs as trailers kristofferhaugsbakk
@ 2026-08-02 22:36 ` Junio C Hamano
2026-08-03 12:11 ` Kristoffer Haugsbakk
2026-08-03 15:20 ` Jeff King
` (3 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2026-08-02 22:36 UTC (permalink / raw)
To: kristofferhaugsbakk; +Cc: git, Kristoffer Haugsbakk, Jeff King
kristofferhaugsbakk@fastmail.com writes:
> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> 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.
When I read the problem description, I would have expected you to
say "If we find <token>: at the beginning of the line, check <token>
against known URL schemes like https, ftp, etc. and declare that the
line is not a trailer, if it matches". Checking against "://" is
much more robust, as it is less likely to happen in random text, and
we avoid maintaining a whitelist of scheme names. You are certainly
smarter than I am ;-).
Shouldn't we restrict the token preceding "://" more strictly than
simply prohibiting whitespace?
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
> diff --git a/trailer.c b/trailer.c
> index 6d8ec7fa8d8..971ae459596 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -635,8 +635,13 @@ static ssize_t find_separator(const char *line, const char *separators)
> int whitespace_found = 0;
> const char *c;
> for (c = line; *c; c++) {
> - if (strchr(separators, *c))
> + if (strchr(separators, *c)) {
> + /* avoid accidental URL matches (://) */
> + if (*c == ':' && c[1] == '/' && c[2] == '/' &&
How do we know the references to c[1] and c[2] do not access an
unmapped piece of memory? The answer is that line[] is NUL
terminated, so c[0] == ':' guarantees that c[1] is safe to read and
unless it is NUL (and c[1] =='/' certainly means it is not NUL),
c[2] is safe to read.
OK. Makes sense to me.
Thanks.
> + !whitespace_found)
> + return -1;
> return c - line;
> + }
> if (!whitespace_found && (isalnum(*c) || *c == '-'))
> continue;
> if (c != line && (*c == ' ' || *c == '\t')) {
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] trailers: stop recognizing URLs as trailers
2026-08-02 22:36 ` Junio C Hamano
@ 2026-08-03 12:11 ` Kristoffer Haugsbakk
0 siblings, 0 replies; 17+ messages in thread
From: Kristoffer Haugsbakk @ 2026-08-03 12:11 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kristoffer Haugsbakk, Jeff King
On Mon, Aug 3, 2026, at 00:36, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
>
>> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>>
>> 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.
>
> When I read the problem description, I would have expected you to
> say "If we find <token>: at the beginning of the line, check <token>
> against known URL schemes like https, ftp, etc. and declare that the
> line is not a trailer, if it matches". Checking against "://" is
> much more robust, as it is less likely to happen in random text, and
> we avoid maintaining a whitelist of scheme names. You are certainly
> smarter than I am ;-).
The credit for being smart goes to Peff.
https://lore.kernel.org/git/20260609004340.GF358144@coredump.intra.peff.net/T/#m03ac1a456648090c04cdf5141b7a3e638f1213d1
> Shouldn't we restrict the token preceding "://" more strictly than
> simply prohibiting whitespace?
Right now (with this code) we know that:
1. We have either parsed only alphanumerics and hyphens (whitespace is
ruled out); or
2. We haven’t even parsed (1), but just found a line that starts with
`://`.
In both cases we bail out of the parsing with `-1`, i.e. “not a
trailer”.
Wikipedia[1] tells me that this current check *does* have a false positive:
A non-empty scheme component followed by a colon (:), consisting of
a sequence of characters beginning with a letter and followed by any
combination of letters, digits, plus (+), period (.), or hyphen (-).
🔗 1: https://en.wikipedia.org/wiki/Uniform_Resource_Identifier#Syntax
A URL *must* begin with a letter, but a trailer can just be a
digit. Which means that this is not the start of a URL:
1://
But the current code will reject it as a URL.
There are also other false positives like the strange but legal trailer
key `-`.
Other than that, the character set of trailers (alphanums and hyphens)
is a strict subset of URL <scheme>.
I also see that the git-interpret-trailers(1) doc update should say
alphanumerics and/or hyphens instead of just alphanums.
>
>> Helped-by: Jeff King <peff@peff.net>
>> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
>> ---
>
>> diff --git a/trailer.c b/trailer.c
>> index 6d8ec7fa8d8..971ae459596 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -635,8 +635,13 @@ static ssize_t find_separator(const char *line, const char *separators)
>> int whitespace_found = 0;
>> const char *c;
>> for (c = line; *c; c++) {
>> - if (strchr(separators, *c))
>> + if (strchr(separators, *c)) {
>> + /* avoid accidental URL matches (://) */
>> + if (*c == ':' && c[1] == '/' && c[2] == '/' &&
>
> How do we know the references to c[1] and c[2] do not access an
> unmapped piece of memory? The answer is that line[] is NUL
> terminated, so c[0] == ':' guarantees that c[1] is safe to read and
> unless it is NUL (and c[1] =='/' certainly means it is not NUL),
> c[2] is safe to read.
>
> OK. Makes sense to me.
I’m mostly a Java programmer so I had the same thought (non-didactically
;) ). Yes, because of sentinel `NUL` and boolean short-circuiting we can
incrementally peak one character ahead. This would be wrong in any
language without `NUL` terminating strings, but here it is
correct. Indeed, checking the length first (which you would need to do
in Java) would incur a linear cost since you need to scan the string
until you hit the `NUL` terminator.
>
> Thanks.
>[snip]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] trailers: stop recognizing URLs as trailers
2026-08-02 19:57 ` [PATCH] trailers: stop recognizing URLs as trailers kristofferhaugsbakk
2026-08-02 22:36 ` Junio C Hamano
@ 2026-08-03 15:20 ` Jeff King
2026-08-03 15:39 ` Junio C Hamano
2026-08-06 20:17 ` Kristoffer Haugsbakk
2026-08-20 14:38 ` Junio C Hamano
` (2 subsequent siblings)
4 siblings, 2 replies; 17+ messages in thread
From: Jeff King @ 2026-08-03 15:20 UTC (permalink / raw)
To: kristofferhaugsbakk; +Cc: Junio C Hamano, git, Kristoffer Haugsbakk
On Sun, Aug 02, 2026 at 09:57:17PM +0200, kristofferhaugsbakk@fastmail.com wrote:
> 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.
This example makes me wonder if we ought to be smarter about brackets.
I.e., could/should we realize that the opening bracket is a comment and
then ignore everything up to the closing one? That would help this case
and other weird cases like:
Signed-off-by: whomever
[peff: there's a really interesting thing going on
here: the comment is free-form text that happens to
use a colon in a sentence, but we'll interpret it
as a trailer with key "here"]
Signed-off-by: another unlucky soul
That said, I think there are cases without brackets that are also
confusing. Like:
Let me finish this commit message by telling you all about this
amazing url:
https://example.com
So I don't think that is a counter-argument against this URL
false-positive check, but just a possible direction for future
exploration.
> 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.
Yeah, this is another interesting example. I agree it is fundamentally
broken, but showing the "https" trailer is just making it worse.
> diff --git a/trailer.c b/trailer.c
> index 6d8ec7fa8d8..971ae459596 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -635,8 +635,13 @@ static ssize_t find_separator(const char *line, const char *separators)
> int whitespace_found = 0;
> const char *c;
> for (c = line; *c; c++) {
> - if (strchr(separators, *c))
> + if (strchr(separators, *c)) {
> + /* avoid accidental URL matches (://) */
> + if (*c == ':' && c[1] == '/' && c[2] == '/' &&
> + !whitespace_found)
> + return -1;
> return c - line;
> + }
As discussed elsewhere, we are free to match with short-circuiting
because of the NUL termination. But that also means we could write this
as:
if (starts_with(c, "://") && !whitespace_found)
which is perhaps a little more readable.
-Peff
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] trailers: stop recognizing URLs as trailers
2026-08-03 15:20 ` Jeff King
@ 2026-08-03 15:39 ` Junio C Hamano
2026-08-06 20:17 ` Kristoffer Haugsbakk
1 sibling, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2026-08-03 15:39 UTC (permalink / raw)
To: Jeff King; +Cc: kristofferhaugsbakk, git, Kristoffer Haugsbakk
Jeff King <peff@peff.net> writes:
> As discussed elsewhere, we are free to match with short-circuiting
> because of the NUL termination. But that also means we could write this
> as:
>
> if (starts_with(c, "://") && !whitespace_found)
>
> which is perhaps a little more readable.
"little more" -> "much more" ;-).
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] trailers: stop recognizing URLs as trailers
2026-08-03 15:20 ` Jeff King
2026-08-03 15:39 ` Junio C Hamano
@ 2026-08-06 20:17 ` Kristoffer Haugsbakk
1 sibling, 0 replies; 17+ messages in thread
From: Kristoffer Haugsbakk @ 2026-08-06 20:17 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
On Mon, Aug 3, 2026, at 17:20, Jeff King wrote:
> On Sun, Aug 02, 2026 at 09:57:17PM +0200,
> kristofferhaugsbakk@fastmail.com wrote:
>
>> 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.
>
> This example makes me wonder if we ought to be smarter about brackets.
> I.e., could/should we realize that the opening bracket is a comment and
> then ignore everything up to the closing one? That would help this case
> and other weird cases like: [snip]
I think that it makes a lot of sense to special-case brackets as
delimiting non-trailer runs.
(for other readers) This support for non-trailer lines grew out of Linux
Kernel practices. At least according to this thread:
https://lore.kernel.org/git/CA+55aFzN4SnenchxPScn61_apzitGAPtoYEd49iLZPxgK0KQGw@mail.gmail.com/
(See also in particular: https://lore.kernel.org/git/20150905000745.GC11443@sigill.intra.peff.net/ )
And part of the back-and-forth in that thread is an inherent tension:
the trailer format is loose. All you need is a some
alphanumerics/hyphens and a colon. So it is simple to accidentally slip
in a *real* trailer line along with all the cruft. Like Peff’s example
shows:
>
> Signed-off-by: whomever
> [peff: there's a really interesting thing going on
> here: the comment is free-form text that happens to
> use a colon in a sentence, but we'll interpret it
> as a trailer with key "here"]
> Signed-off-by: another unlucky soul
Imagine you had internalized the trailer parsing rules (which you
shouldn’t have to but anyway); it would still be easy to accidentally
write something like the above.
But with an additional `[]` rule you don’t have to worry:
• A run of non-trailer lines starts with regex `^[`
• And ends 0 or more lines later with regex `]$`
• In addition to the existing rules
And `[]` are illegal in trailer keys anyway.
This is a very Linux (and Git project) specific additional rule, but the
non-trailer lines rules were always like that.
>
> That said, I think there are cases without brackets that are also
> confusing. Like:
>
> Let me finish this commit message by telling you all about this
> amazing url:
>
> https://example.com
Yeah exactly.
>[snip]
>> diff --git a/trailer.c b/trailer.c
>> index 6d8ec7fa8d8..971ae459596 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -635,8 +635,13 @@ static ssize_t find_separator(const char *line, const char *separators)
>> int whitespace_found = 0;
>> const char *c;
>> for (c = line; *c; c++) {
>> - if (strchr(separators, *c))
>> + if (strchr(separators, *c)) {
>> + /* avoid accidental URL matches (://) */
>> + if (*c == ':' && c[1] == '/' && c[2] == '/' &&
>> + !whitespace_found)
>> + return -1;
>> return c - line;
>> + }
>
> As discussed elsewhere, we are free to match with short-circuiting
> because of the NUL termination. But that also means we could write this
> as:
>
> if (starts_with(c, "://") && !whitespace_found)
>
> which is perhaps a little more readable.
Oh for sure, much more readable.
Thanks!
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] trailers: stop recognizing URLs as trailers
2026-08-02 19:57 ` [PATCH] trailers: stop recognizing URLs as trailers kristofferhaugsbakk
2026-08-02 22:36 ` Junio C Hamano
2026-08-03 15:20 ` Jeff King
@ 2026-08-20 14:38 ` Junio C Hamano
2026-08-20 14:47 ` Kristoffer Haugsbakk
2026-08-20 20:00 ` [PATCH v2] " kristofferhaugsbakk
2026-08-21 5:26 ` [PATCH v3] " kristofferhaugsbakk
4 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2026-08-20 14:38 UTC (permalink / raw)
To: kristofferhaugsbakk; +Cc: git, Kristoffer Haugsbakk, Jeff King
kristofferhaugsbakk@fastmail.com writes:
> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> 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:
> ...
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
This has been on hold waiting for the base topic to settle, but now
that the base topic has graduated, the effort can be rebooted.
Can somebody summarize the outstanding issues on this topic (if
any)?
Thanks.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] trailers: stop recognizing URLs as trailers
2026-08-02 19:57 ` [PATCH] trailers: stop recognizing URLs as trailers kristofferhaugsbakk
` (2 preceding siblings ...)
2026-08-20 14:38 ` Junio C Hamano
@ 2026-08-20 20:00 ` kristofferhaugsbakk
2026-08-21 5:26 ` [PATCH v3] " kristofferhaugsbakk
4 siblings, 0 replies; 17+ messages in thread
From: kristofferhaugsbakk @ 2026-08-20 20:00 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, Junio C Hamano, Jeff King
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
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.
***
Note that this check has some benign false positives. A trailer key
can start with a digit, but a URL scheme can not start with a digit.
That means that a line that starts with `1://` will be rejected even
though it cannot be a URL. I don’t think this will reject any real
trailers, so I think the implementation simplicity is worth it.
And these false positives are just for a limited start fragment check;
a mere heuristic, not a URL parser.
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
Topic name (applied): trailers-no-urls
Topic summary: Stop recognizing URLs in trailer blocks as trailers.
Note to the maintainer: This has been rebased on `master` since the
dependent topic kh/doc-trailers has been merged thither.
§ Link to v1
https://lore.kernel.org/git/URLs_not_trailers.b13@msgid.xyz/
§ Changes in v2
• Use `starts_with` for readability:
https://lore.kernel.org/git/20260609004340.GF358144@coredump.intra.peff.net/T/#m74203c474c34f1028a7e3d389ff46fb7e579444c
• Since `starts_with` is a function, put the simpler conjunct
`whitespace_found` before it. It’s better to put the cheaper operations
first in a short-circuiting expression. Right?
• Explain in the commit message that you can technically get false positive
“URL” start fragments:
https://lore.kernel.org/git/20260609004340.GF358144@coredump.intra.peff.net/T/#m35047d5c7a79abd23c11f97e6b6a0364409805e3
Documentation/git-interpret-trailers.adoc | 13 ++++--
t/t7513-interpret-trailers.sh | 19 +++++++++
t/unit-tests/u-trailer.c | 52 +++++++++++++++++++++++
trailer.c | 6 ++-
4 files changed, 86 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-interpret-trailers.adoc b/Documentation/git-interpret-trailers.adoc
index b4988d39eab..903d598dcb0 100644
--- a/Documentation/git-interpret-trailers.adoc
+++ b/Documentation/git-interpret-trailers.adoc
@@ -123,9 +123,16 @@ OTHER RULES
What was covered in the previous section are the rules that are relevant
for regular use. The following points are included for completeness.
-This command ignores comment lines (see `core.commentString` in
-linkgit:git-config[1]). This is for use with the `prepare-commit-msg`
-and `commit-msg` hooks.
+--
+* This command ignores comment lines (see `core.commentString` in
+ linkgit:git-config[1]). This is for use with the `prepare-commit-msg`
+ and `commit-msg` hooks.
+
+* Candidate trailer lines that have `:` as the separator, that have no
+ whitespace before the value part, and that start with `//` are not
+ recognized as trailers. This is to avoid accidentally interpreting
+ URLs as trailers (e.g. lines that start with `https://`).
+--
OPTIONS
-------
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 818a8dafbd2..e3555b6d51d 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1989,4 +1989,23 @@ test_expect_success 'handling of --- lines in conjunction with cut-lines' '
test_cmp expected actual
'
+test_expect_success 'URLs and lines that are not quite URLs' '
+ cat >expect <<-\EOF &&
+ https: //www.a-trailer.org
+ https: //www.another-trailer.org
+ Signed-off-by: somebody <somebody@somewhere>
+ EOF
+ git interpret-trailers --only-trailers >actual <<-\EOF &&
+ subject
+
+ body
+
+ https://www.not-a-trailer.org
+ https ://www.a-trailer.org
+ https: //www.another-trailer.org
+ Signed-off-by: somebody <somebody@somewhere>
+ EOF
+ test_cmp expect actual
+'
+
test_done
diff --git a/t/unit-tests/u-trailer.c b/t/unit-tests/u-trailer.c
index 3d60ea1603d..7404b165fac 100644
--- a/t/unit-tests/u-trailer.c
+++ b/t/unit-tests/u-trailer.c
@@ -318,3 +318,55 @@ void test_trailer__one_non_trailer_no_git_trailers(void)
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);
+}
diff --git a/trailer.c b/trailer.c
index 6d8ec7fa8d8..10b1abebfbe 100644
--- a/trailer.c
+++ b/trailer.c
@@ -635,8 +635,12 @@ static ssize_t find_separator(const char *line, const char *separators)
int whitespace_found = 0;
const char *c;
for (c = line; *c; c++) {
- if (strchr(separators, *c))
+ if (strchr(separators, *c)) {
+ /* avoid accidental URL matches */
+ if (!whitespace_found && starts_with(c, "://"))
+ return -1;
return c - line;
+ }
if (!whitespace_found && (isalnum(*c) || *c == '-'))
continue;
if (c != line && (*c == ' ' || *c == '\t')) {
Range-diff against v1:
1: e7ba66a0ce3 ! 1: 2f8d10c1c6d trailers: stop recognizing URLs as trailers
@@ Commit message
† 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.
+ ***
+
+ Note that this check has some benign false positives. A trailer key
+ can start with a digit, but a URL scheme can not start with a digit.
+ That means that a line that starts with `1://` will be rejected even
+ though it cannot be a URL. I don’t think this will reject any real
+ trailers, so I think the implementation simplicity is worth it.
+
+ And these false positives are just for a limited start fragment check;
+ a mere heuristic, not a URL parser.
+
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
@@ trailer.c: static ssize_t find_separator(const char *line, const char *separator
for (c = line; *c; c++) {
- if (strchr(separators, *c))
+ if (strchr(separators, *c)) {
-+ /* avoid accidental URL matches (://) */
-+ if (*c == ':' && c[1] == '/' && c[2] == '/' &&
-+ !whitespace_found)
++ /* avoid accidental URL matches */
++ if (!whitespace_found && starts_with(c, "://"))
+ return -1;
return c - line;
+ }
base-commit: 1a3e64c6c4a623626ff0687008732a8e007e2a1c
--
2.55.0.13.g85d2d65e389
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH v3] trailers: stop recognizing URLs as trailers
2026-08-02 19:57 ` [PATCH] trailers: stop recognizing URLs as trailers kristofferhaugsbakk
` (3 preceding siblings ...)
2026-08-20 20:00 ` [PATCH v2] " kristofferhaugsbakk
@ 2026-08-21 5:26 ` kristofferhaugsbakk
2026-08-21 5:28 ` Kristoffer Haugsbakk
4 siblings, 1 reply; 17+ messages in thread
From: kristofferhaugsbakk @ 2026-08-21 5:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kristoffer Haugsbakk, Jeff King, git
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
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.
***
Note that this check has some benign false positives. A trailer key
can start with a digit, but a URL scheme can not start with a digit.
That means that a line that starts with `1://` will be rejected even
though it cannot be a URL. I don’t think this will reject any real
trailers, so I think the implementation simplicity is worth it.
And these false positives are just for a limited start fragment check;
a mere heuristic, not a URL parser.
Helped-by: Jeff King <peff@peff.net>
Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
Topic name (applied): trailers-no-urls
Topic summary: Stop recognizing URLs in trailer blocks as trailers.
§ Link to v2
https://lore.kernel.org/git/V2_URLs_not_trailers.bf3@msgid.xyz/
§ Changes in v2
• Add Ack https://lore.kernel.org/git/20260821004248.GA296777@coredump.intra.peff.net/
Documentation/git-interpret-trailers.adoc | 13 ++++--
t/t7513-interpret-trailers.sh | 19 +++++++++
t/unit-tests/u-trailer.c | 52 +++++++++++++++++++++++
trailer.c | 6 ++-
4 files changed, 86 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-interpret-trailers.adoc b/Documentation/git-interpret-trailers.adoc
index b4988d39eab..903d598dcb0 100644
--- a/Documentation/git-interpret-trailers.adoc
+++ b/Documentation/git-interpret-trailers.adoc
@@ -123,9 +123,16 @@ OTHER RULES
What was covered in the previous section are the rules that are relevant
for regular use. The following points are included for completeness.
-This command ignores comment lines (see `core.commentString` in
-linkgit:git-config[1]). This is for use with the `prepare-commit-msg`
-and `commit-msg` hooks.
+--
+* This command ignores comment lines (see `core.commentString` in
+ linkgit:git-config[1]). This is for use with the `prepare-commit-msg`
+ and `commit-msg` hooks.
+
+* Candidate trailer lines that have `:` as the separator, that have no
+ whitespace before the value part, and that start with `//` are not
+ recognized as trailers. This is to avoid accidentally interpreting
+ URLs as trailers (e.g. lines that start with `https://`).
+--
OPTIONS
-------
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 818a8dafbd2..e3555b6d51d 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1989,4 +1989,23 @@ test_expect_success 'handling of --- lines in conjunction with cut-lines' '
test_cmp expected actual
'
+test_expect_success 'URLs and lines that are not quite URLs' '
+ cat >expect <<-\EOF &&
+ https: //www.a-trailer.org
+ https: //www.another-trailer.org
+ Signed-off-by: somebody <somebody@somewhere>
+ EOF
+ git interpret-trailers --only-trailers >actual <<-\EOF &&
+ subject
+
+ body
+
+ https://www.not-a-trailer.org
+ https ://www.a-trailer.org
+ https: //www.another-trailer.org
+ Signed-off-by: somebody <somebody@somewhere>
+ EOF
+ test_cmp expect actual
+'
+
test_done
diff --git a/t/unit-tests/u-trailer.c b/t/unit-tests/u-trailer.c
index 3d60ea1603d..7404b165fac 100644
--- a/t/unit-tests/u-trailer.c
+++ b/t/unit-tests/u-trailer.c
@@ -318,3 +318,55 @@ void test_trailer__one_non_trailer_no_git_trailers(void)
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);
+}
diff --git a/trailer.c b/trailer.c
index 6d8ec7fa8d8..10b1abebfbe 100644
--- a/trailer.c
+++ b/trailer.c
@@ -635,8 +635,12 @@ static ssize_t find_separator(const char *line, const char *separators)
int whitespace_found = 0;
const char *c;
for (c = line; *c; c++) {
- if (strchr(separators, *c))
+ if (strchr(separators, *c)) {
+ /* avoid accidental URL matches */
+ if (!whitespace_found && starts_with(c, "://"))
+ return -1;
return c - line;
+ }
if (!whitespace_found && (isalnum(*c) || *c == '-'))
continue;
if (c != line && (*c == ' ' || *c == '\t')) {
Interdiff against v2:
Range-diff against v2:
1: 2f8d10c1c6d ! 1: 736610daf6e trailers: stop recognizing URLs as trailers
@@ Commit message
a mere heuristic, not a URL parser.
Helped-by: Jeff King <peff@peff.net>
+ Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
## Documentation/git-interpret-trailers.adoc ##
base-commit: 1a3e64c6c4a623626ff0687008732a8e007e2a1c
--
2.55.0.13.g85d2d65e389
^ permalink raw reply related [flat|nested] 17+ messages in thread