266028 2023-12-07 15:21:52 -0800 `Co-authored-by` doesn’t work because (non-trailer) `Canonical link` gets added by the merge queue 2025-11-11 07:32:06 -0800 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified NEW https://bugs.webkit.org/show_bug.cgi?id=282792 https://bugs.webkit.org/show_bug.cgi?id=282952 InRadar P2 Normal --- 301444 301469 301471 1 graouts webkit-unassigned aperez ap bfan2 emw gsnedders jbedard repstein ryanhaddad slewis webkit-bug-importer oldest_to_newest 1997799 0 graouts 2023-12-07 15:21:52 -0800 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. 1998773 1 ap 2023-12-11 13:15:58 -0800 AFAIK our tooling requires for canonical link to be the last line, so it wouldn't be easy to change. 1999741 2 webkit-bug-importer 2023-12-14 15:22:14 -0800 <rdar://problem/119691076> 2018003 3 gsnedders 2024-03-01 06:55:51 -0800 (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. 2018047 4 emw 2024-03-01 10:39:44 -0800 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. 2018090 5 gsnedders 2024-03-01 12:55:17 -0800 (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. 2052401 6 gsnedders 2024-08-13 10:49:22 -0700 (Changing the title so this is more easily found searching for "trailer") 2073636 7 gsnedders 2024-11-07 16:05:46 -0800 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. 2157921 8 aperez 2025-11-11 07:32:06 -0800 (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.