trailers: --only-trailers normalizes URLs to trailers

trailers: --only-trailers normalizes URLs to trailers

[Kristoffer Haugsbakk]

The following is a bug that follows straightforwardly from the documented
or discussed behavior. In that sense it is not a bug. But it is a bug in
the sense that it makes things inconvenient and violates a design goal.

> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)

Ran what is the equivalent of

    git interpret-trailers --only-trailers

With

    git log --format="%(trailers:only)"

> What did you expect to happen? (Expected behavior)

For URLs like https://www.digsm.xyz/ to be left intact.

(Well, did I expect that? It follows from the discussed behavior...)

> What happened instead? (Actual behavior)

URLs on a line by themselves in eligible trailer blocks get
normalized/canonicalized to a “trailer” with key e.g. `https`:

    https: //www.digsm.xyz/

> What's different between what you expected and what actually happened?

In an ideal world to have some special-casing of URLs so that they are
not detected as trailers. Does anyone realistically want trailers like
this?:

    file: //...
    http: //...
    https: //...

Maybe a C-style comment?

    https: // I changed my mind about providing a URL here.
        This comment is a placeholder.
    Comment: // But next up we have a URL
    https: https://protocoltwiceover.net

And this is where my imagination ends.

Just special-casing `https` would go a long way.

> Anything else you want to add:

Yes, after this [System Info] part.

> Please review the rest of the bug report below.
> You can delete any lines you don't wish to share.

[System Info]
git version:
git version 2.54.0
cpu: x86_64
built from commit: 94f057755b7941b321fd11fec1b2e3ca5313a4e0
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
rust: disabled
gettext: enabled
libcurl: 7.81.0
OpenSSL: OpenSSL 3.0.2 15 Mar 2022
zlib: 1.2.11
SHA-1: SHA1_DC
SHA-256: SHA256_BLK
default-ref-format: files
default-hash: sha1
uname: Linux 6.8.0-117-generic #117~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Thu May  7 22:17:46 UTC  x86_64
compiler info: gnuc: 11.4
libc info: glibc: 2.35
$SHELL (typically, interactive shell): /bin/bash

[Enabled Hooks]
commit-msg
post-applypatch
post-commit
sendemail-validate

***

That things like `--format='%(trailers:only)'` normalize trailers is
known and has been discussed before.[1] There’s been discussion around
the key capitalization and prefix normalization. But this is not about
that. This is just about normalizing the separator part.

🔗 1: https://lore.kernel.org/git/87blk0rjob.fsf@0x63.nu/

One design goal for trailers (either by implementers or reviewers or both)
has been to avoid false positives.[2] That meant trying to avoid
detecting trailers that were not intended. For example:

    Everything was better in the past. Let me not even start on this
    rant: it is not good for my blood pressure.

This will not be picked up as a trailer block unless `rant` is configured
as a trailer key.

† 2: See e.g. Jonathan Tan’s series about among other things adding the
     25% rule

     https://lore.kernel.org/git/xmqq7f96sa9i.fsf@gitster.mtv.corp.google.com/

But that’s pretty innocuous. Just a misplaced rant. The topic of this
bug report is not a big deal either, but it is:

1. Structured data that gets mangled in this normalize mode
2. That can naturally go at the end of the message on its own line

And these two points are very relevant for people who never use
trailers. Or, wait. I guess it isn’t if they don’t use trailers and thus
will never normalize them. But it is relevant if they work on a project
where someone else does that.

IN INTENDED TRAILER BLOCKS [3]

And then there are things that can go wrong if you intend to write trailer blocks:

1. “Non-trailer lines” that are URLs get normalized as trailers (NTL for
   short)
2. User error line wrapping turns one trailer into an empty trailer plus
   a `https` trailer (LW for short)
3. Normalizing trailers along the way (as in patches in flight or
   something) introduces this strange lossiness (NL for short)

I did (2) (LW for short) four years ago it seems:

    See:
    https://digsm.yxz/blog/important-context/?bigtechtracker=86b0c5a1e2b73b08fd54c727f4458649ed9fe3ad1b6e8ac9460c070113509a1e

† 3: Are all-caps titles good or bad? Let me know.

IN THE LINUX KERNEL

There are some hits for the `http` and `https` trailers when trailers
are normalized. The baseline:

   $ git log --extended-regexp --grep='https?: //' --oneline | wc -l
   12

With normalization:

    $ git log --format='%(trailers:only)' |
          grep --extended-regexp '^https?: //' | wc -l
    245

Note that I have no idea how the Linux Kernel is run. But I don’t
imagine that there are uses for `https: //...` trailers.

And trailer usage is complicated. There are for example on-purpose
indented `Link` “trailers”, presumably for the purpose of *excluding*
them as `Link` trailers. See:

    commit d80a9cb1a64ab9c817b6262c7e4e433b6a3581a0

    <body>

    [ljs@kernel.org: avoid bisection hazard]
      Link: https://lkml.kernel.org/r/d0cc6161-77a4-42ba-a411-96c23c78df1b@lucifer.local
    Link: https://lkml.kernel.org/r/c2be872d64ef9573b80727d9ab5446cf002f17b5.1774029655.git.ljs@kernel.org
    Signed-off-by: Lorenzo Stoakes (Oracle) <ljs@kernel.org>
    [MORE BELOW]

Is that indented link for that `[]` comment? I dunno.

But what’s the main topic here are intended non-trailer lines which are
URLs that get treated as trailers (NTL). Like this invented example:

Reported-by: ...
https://digsm.xyz/?avastvirus=5891b5b522d5df086d0ff0b110fbd9d21bb4fc7163af34d08286a2e846f6be03
Signed-off-by: ...

Or this real example where the URLs are clearly part of a “comment”
non-trailer run.

    8236fc613d44e59f6736d6c3e9efffaf26ab7f00
    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

(These are shown as they are written in the commit message. Normalizing
the messages would create `https` trailers.)

Here are examples of line-wrapping mistake commits (LW) for `Link`,
`Closes`, or `Fixes` (sometimes these point to bug URLs and not
commits):

    5bd97f5c5f241a5610c4412d1b93995a26241f81
    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>

and:

• 24abe1f238e7d7ac56be6374c52a3c13dab84f69
• 27e21516914dc130a79aa895a5a26e18f0213a5a
• be3536a4bdda53ff5a91b7e542b167d12bddb317

Finally there is this commit which has a trailer in the commit message
itself with the key `https` (NL).

    commit 496c0c4c53bbe1bad97e82cd12103df61a6e459d
    ...
    ...

	net: wan: fsl_ucc_hdlc: free tx_skbuff in uhdlc_memclean

        <body>

	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>

How could this have happened? Follow the patch-id link.

https://patch.msgid.link/20260507155332.3452319-1-holger.brunck@hitachienergy.com

    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 it was just a non-trailer URL line as this person submitted it. But
presumably the person who applied it put the message through a round of
normalization.

Cheers, good night

-- 
Kristoffer

Re: trailers: --only-trailers normalizes URLs to trailers

[Jeff King]

On Thu, Jun 04, 2026 at 11:27:51PM +0200, Kristoffer Haugsbakk wrote:

> The following is a bug that follows straightforwardly from the documented
> or discussed behavior. In that sense it is not a bug. But it is a bug in
> the sense that it makes things inconvenient and violates a design goal.

Yeah, though if you'll allow me to nitpick your subject a moment: I
don't think --only-trailers is really the culprit here. It demonstrates
the problem because it normalizes the "trailer" it found. But the loose
trailer matching is the more fundamental issue. For example:

git interpret-trailers --trailer=foo=bar <<\EOF
subject

body

http://example.com
EOF

will stick the new "foo: bar" trailer right up against the (now-broken)
"http:" trailer. When it should come in its own stanza, which it would
if you added a line "other" at the end, since that tells us that "http:"
can't be a trailer.

> > What's different between what you expected and what actually happened?
> 
> In an ideal world to have some special-casing of URLs so that they are
> not detected as trailers. Does anyone realistically want trailers like
> this?:
> 
>     file: //...
>     http: //...
>     https: //...

I could even see those as trailers, if somebody really wanted to allow
arbitrary values that might just happen to start with "//". But without
the whitespace after the colon, it is quite questionable.

> Just special-casing `https` would go a long way.

Agreed, though I think a rule like: ":// (with no whitespace)" is not a
valid separator. Something like this:

diff --git a/trailer.c b/trailer.c
index 6d8ec7fa8d..342ed81c78 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)) {
+			/* special case to avoid accidental URL matches */
+			if (*c == ':' && c[1] == '/' && c[2] == '/')
+				return -1;
 			return c - line;
+		}
 		if (!whitespace_found && (isalnum(*c) || *c == '-'))
 			continue;
 		if (c != line && (*c == ' ' || *c == '\t')) {

-Peff

Re: trailers: --only-trailers normalizes URLs to trailers

[Kristoffer Haugsbakk]

On Tue, Jun 9, 2026, at 02:43, Jeff King wrote:
> On Thu, Jun 04, 2026 at 11:27:51PM +0200, Kristoffer Haugsbakk wrote:
>
>> The following is a bug that follows straightforwardly from the documented
>> or discussed behavior. In that sense it is not a bug. But it is a bug in
>> the sense that it makes things inconvenient and violates a design goal.
>
> Yeah, though if you'll allow me to nitpick your subject a moment: I
> don't think --only-trailers is really the culprit here. It demonstrates
> the problem because it normalizes the "trailer" it found. But the loose
> trailer matching is the more fundamental issue. For example:
>
>[snip]

Yeah, this is more precise. I focused a ton on the normalized output
because that’s what makes it obvious. But the fundamental problem is
interpreting URLs like trailers.

>
>> > What's different between what you expected and what actually happened?
>>
>> In an ideal world to have some special-casing of URLs so that they are
>> not detected as trailers. Does anyone realistically want trailers like
>> this?:
>>
>>     file: //...
>>     http: //...
>>     https: //...
>
> I could even see those as trailers, if somebody really wanted to allow
> arbitrary values that might just happen to start with "//". But without
> the whitespace after the colon, it is quite questionable.
>
>> Just special-casing `https` would go a long way.
>
> Agreed, though I think a rule like: ":// (with no whitespace)" is not a
> valid separator. Something like this:

Yes, matching on `://` strictly is a better proposal. No need to care
about `http`, `https`, `file`, etc. And both of these would *still* have
to be true for this change to be a false negative w.r.t. the user’s
intentions:

• They really input a trailer that looks like a URL, but it’s not meant
  to be a URL
• They really wanted the value to start with `//`

And again I don’t think that is likely to ever happen (with a knock
on wood).

Thanks!

>
> diff --git a/trailer.c b/trailer.c
> index 6d8ec7fa8d..342ed81c78 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)) {
> +			/* special case to avoid accidental URL matches */
> +			if (*c == ':' && c[1] == '/' && c[2] == '/')
> +				return -1;
>  			return c - line;
> +		}
>  		if (!whitespace_found && (isalnum(*c) || *c == '-'))
>  			continue;
>  		if (c != line && (*c == ' ' || *c == '\t')) {

Re: trailers: --only-trailers normalizes URLs to trailers

[Jeff King]

On Wed, Jun 10, 2026 at 04:21:29PM +0200, Kristoffer Haugsbakk wrote:

> > Yeah, though if you'll allow me to nitpick your subject a moment: I
> > don't think --only-trailers is really the culprit here. It demonstrates
> > the problem because it normalizes the "trailer" it found. But the loose
> > trailer matching is the more fundamental issue. For example:
> >
> >[snip]
> 
> Yeah, this is more precise. I focused a ton on the normalized output
> because that’s what makes it obvious. But the fundamental problem is
> interpreting URLs like trailers.

That makes sense. As the author of --only-trailers I immediately
wondered if I had introduced a bug in it, so I was partially motivated
by exonerating myself. ;) I agree that using it is the simplest way to
demonstrate the problem.

> > Agreed, though I think a rule like: ":// (with no whitespace)" is not a
> > valid separator. Something like this:
> 
> Yes, matching on `://` strictly is a better proposal. No need to care
> about `http`, `https`, `file`, etc. And both of these would *still* have
> to be true for this change to be a false negative w.r.t. the user’s
> intentions:
> 
> • They really input a trailer that looks like a URL, but it’s not meant
>   to be a URL
> • They really wanted the value to start with `//`
> 
> And again I don’t think that is likely to ever happen (with a knock
> on wood).

I didn't spend much effort on the patch I showed beyond running it once.
It would probably need tests and a doc update. I wasn't planning to run
with it, but if you feel like doing so, please feel free to use it as
you like.

-Peff

Re: trailers: --only-trailers normalizes URLs to trailers

[Kristoffer Haugsbakk]

On Thu, Jun 11, 2026, at 08:56, Jeff King wrote:
>>[snip]
>> 
>> And again I don’t think that is likely to ever happen (with a knock
>> on wood).
>
> I didn't spend much effort on the patch I showed beyond running it once.
> It would probably need tests and a doc update. I wasn't planning to run
> with it, but if you feel like doing so, please feel free to use it as
> you like.

Yeah, I want to add some tests on top
and make a sumbission (but sent a
message to first confirm that you weren't
cooking anything more ;) ). Thanks.

-- 
Sent from mobile

[PATCH] trailers: stop recognizing URLs as trailers

[kristofferhaugsbakk]

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.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    Topic name: trailers-no-urls
    
    Topic summary: Stop recognizing URLs in trailer blocks as trailers.
    
    Note to the maintainer: this is based on `master` with topic
    kh/doc-trailers merged into it.
    
    I used Peff’s suggestion from the previous email. I just shortened the
    comment, added the parentheses (://) and added the condition that
    whitespace has not been found for the case I discussed of someone writing
    out `<key>: //` (note the space). (Or for that matter: `<key> ://`.) I
    can’t imagine that that is a likely case, but I just want to avoid matching
    URLs, so we don’t have to reject this case.
    
    t/u-trailer.c: `expected_contents[]` is not formatted like the other ones
    in this file. But this is what clang-format(1) gave me.

 Documentation/git-interpret-trailers.adoc | 13 ++++--
 t/t7513-interpret-trailers.sh             | 19 +++++++++
 t/unit-tests/u-trailer.c                  | 52 +++++++++++++++++++++++
 trailer.c                                 |  7 ++-
 4 files changed, 87 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..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;
+		}
 		if (!whitespace_found && (isalnum(*c) || *c == '-'))
 			continue;
 		if (c != line && (*c == ' ' || *c == '\t')) {
-- 
2.54.0.22.g9e26862b904

Re: [PATCH] trailers: stop recognizing URLs as trailers

[Junio C Hamano]

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')) {

Re: [PATCH] trailers: stop recognizing URLs as trailers

[Kristoffer Haugsbakk]

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]

Re: [PATCH] trailers: stop recognizing URLs as trailers

[Jeff King]

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

Re: [PATCH] trailers: stop recognizing URLs as trailers

[Junio C Hamano]

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" ;-).

Re: [PATCH] trailers: stop recognizing URLs as trailers

[Kristoffer Haugsbakk]

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!