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

On Sun, Aug 02, 2026 at 09:57:17PM +0200, [email protected] 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 <[email protected]>
>     [bhelgaas: squash fixes:
>     https://lore.kernel.org/r/[email protected]
>     https://lore.kernel.org/r/[email protected]]
>     Signed-off-by: Bjorn Helgaas <[email protected]>
>     Reviewed-by: Ilpo Järvinen <[email protected]>
>     Link: https://patch.msgid.link/[email protected]
> 
> 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/[email protected]
>     Link:
>     https://lore.kernel.org/3cnmtqmakpbb2uwhenrj7kdqu3uefykiykjllgfbtpkiwhaa4s@sghkevv7jned [1]
>     Acked-by: Darrick J. Wong <[email protected]>
>     Reviewed-by: Jan Kara <[email protected]>
>     Signed-off-by: Christian Brauner <[email protected]>
> 
> 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