Bug 266028 – `Co-authored-by` doesn’t work because (non-trailer) `Canonical link` gets added by the merge queue

• Status: NEW
• Component: Tools / Tests

[Antoine Quint]

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.

[Alexey Proskuryakov]

AFAIK our tooling requires for canonical link to be the last line, so it wouldn't be easy to change.

[Radar WebKit Bug Importer]

<rdar://problem/119691076>

[Sam Sneddon [:gsnedders]]

(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.

[Elliott Williams]

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.

[Sam Sneddon [:gsnedders]]

(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.

[Sam Sneddon [:gsnedders]]

(Changing the title so this is more easily found searching for "trailer")

[Sam Sneddon [:gsnedders]]

We currently have "Canonical link" hardcoded in:

Tools/CISupport/ews-build/steps.py
Tools/CISupport/ews-build/steps_unittest.py
Tools/Scripts/git-webkit
Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program/canonicalize/message.py
Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program/command.py
Tools/Scripts/libraries/webkitscmpy/webkitscmpy/test/canonicalize_unittest.py
Tools/Scripts/libraries/webkitscmpy/webkitscmpy/test/land_unittest.py
Tools/Scripts/webkitpy/tool/commands/suggestnominations.py
Tools/Scripts/webkitpy/tool/commands/suggestnominations_unittest.py
Tools/glib/apply-build-revision-to-files.py
Websites/perf.webkit.org/tools/sync-commits.py

Some of these are probably pretty easy to change and have confidence the behaviour is correct, others are likely harder.

[Adrian Perez]

(In reply to Sam Sneddon [:gsnedders] from comment #7)
> We currently have "Canonical link" hardcoded in:
> 
> [...]
> Tools/glib/apply-build-revision-to-files.py

This one does not need changing: it only reads the commit log messages
to extract the revision identifier from the "Canonical link:" entry.