Bug 266028 - `Co-authored-by` doesn’t work because (non-trailer) `Canonical link` gets added by the merge queue
Summary: `Co-authored-by` doesn’t work because (non-trailer) `Canonical link` gets add...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2023-12-07 15:21 PST by Antoine Quint
Modified: 2024-08-13 10:49 PDT (History)
9 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2023-12-07 15:21:52 PST
A recent patch of mine (271692@main) was meant to credit not just me as the author but a colleague as well. We used `Co-authored-by` as specified in https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors and this looked fine in the PR’s commits tab, but once the patch landed, the last line of the commit became the `Canonical link` and thus the co-author credit was no longer picked up.
Comment 1 Alexey Proskuryakov 2023-12-11 13:15:58 PST
AFAIK our tooling requires for canonical link to be the last line, so it wouldn't be easy to change.
Comment 2 Radar WebKit Bug Importer 2023-12-14 15:22:14 PST
<rdar://problem/119691076>
Comment 3 Sam Sneddon [:gsnedders] 2024-03-01 06:55:51 PST
(In reply to Alexey Proskuryakov from comment #1)
> AFAIK our tooling requires for canonical link to be the last line, so it
> wouldn't be easy to change.

It isn't quite a simple as "the last line", because e.g. git-svn-id appears after it (see 200001@main). I think we might just treat git-svn-id as a special-case though?

At this point when everything is using git, we should just be using git-interpret-trailers, which would also solve the underlying problem.

*However*, git changed their behaviour to disallow whitespace in trailer names in 2016, https://github.com/git/git/commit/e4319562bc2834096fade432fd90c985b96476db, in git 2.12.0, though wasn't properly documented until 2022, https://github.com/git/git/commit/b46dd1726c139c930d7b4b7c3262e3f2699987d3, git v2.38.0, thus our "apparent" trailer of "Canonical Link" is no longer a valid trailer name.

Our path forward here is probably a multi-step one:

1. Allow *either* a final line of "Canonical Link: XXX" (or followed by a git-svn-id line?) _or_ a "Canonical-Link" trailer in the trailers block.

2. Migrate to outputting a Canonical-Link trailer once we believe (1) has landed long enough ago.

Long term, this would allow us to do things like `git log --pretty='format:%(trailers:key=Canonical-Link)'` once we rarely care about the history commits with a space.

That said, different vendors may have their own tooling hard-coding "Canonical Link" which would also need changed (e.g. Apple's perf infrastructure apparently does this), so this probably does need some broader communication *especially* before step 2.
Comment 4 Elliott Williams 2024-03-01 10:39:44 PST
I'm not sure whether we intended the "Canonical Link" line to be a trailer and got the format wrong, or whether we did not design it with trailers in mind. But regardless, it's *so close* to a well-formed trailer that I would appreciate it being fixed up.

That said, I don't think we have ever made a "breaking" change to the commit metadata format since the project migrated to Git. It's possible that this isn't useful enough to merit such a break, or that it should wait until we have other changes we want to make to the formatting.

We do already have "Originally-landed-as" which is used during the merge-back process and is a well-formed trailer.

> Our path forward here is probably a multi-step one:
> 
> 1. Allow *either* a final line of "Canonical Link: XXX" (or followed by a git-svn-id line?) _or_ a "Canonical-Link" trailer in the trailers block.
> 
> 2. Migrate to outputting a Canonical-Link trailer once we believe (1) has landed long enough ago.

One thing I'd suggest adding is a period when we write *both* a "Canonical Link" line and a "Canonical-Link" trailer, to help tooling that looks for the former.
Comment 5 Sam Sneddon [:gsnedders] 2024-03-01 12:55:17 PST
(In reply to Elliott Williams from comment #4)
> One thing I'd suggest adding is a period when we write *both* a "Canonical
> Link" line and a "Canonical-Link" trailer, to help tooling that looks for
> the former.

The trailers by definition come at the end, and we also require the "Canonical Link" to come at the end, so we can't really do both. One of them will fail to parse as a result.
Comment 6 Sam Sneddon [:gsnedders] 2024-08-13 10:49:22 PDT
(Changing the title so this is more easily found searching for "trailer")