Bug 282792 – Non-git-trailers in "Canonical link" block

• Status: NEW
• Component: Tools / Tests

[Sam Sneddon [:gsnedders]]

For Bug 266028, we really want to move towards using actual git trailers for our "Canonical link".

It would be nice if, at the moment, the only non-trailer line in the trailer block was our "Canonical link" line.

Per the git-interpret-trailers man page:

> Existing trailers are extracted from the input by looking for a group of one or more lines that (i) is all trailers, or (ii) contains at least one Git-generated or user-configured trailer and consists of at least 25% trailers. The group must be preceded by one or more empty (or whitespace-only) lines. The group must either be at the end of the input or be the last non-whitespace lines before a line that starts with --- (followed by a space or the end of the line).

We do however have a number of commits where we have other non-trailer lines in the trailer block:

286098@main, 286074@main, 285122@main, 285092@main, 284409@main, 284248@main, 283054@main, 282969@main, 282950@main, 282415@main, 282327@main, 282204@main, 281380@main, 281218@main, 280897@main, 280750@main, 280635@main, 278661@main, 277268@main, 277054@main, 276989@main, 276928@main, 276721@main, 275313@main, 274905@main, 274777@main, 273007@main, 269958@main, 269186@main, 268926@main, 268819@main, 267564@main, 267441@main, 266913@main, 266711@main, 253704@main

Specifically, we see the following lines in our pseudo-trailer block:

* .gitignore: Add BenchmarkTemp to .gitignore
* .submitproject-tools: Added. * .submitproject: Skip more ThirdParty to match installsrc a little better.
* .submitproject: Added.
* .submitproject: Exclude Source/ThirdParty/skia
* Websites/webkit.org/wp-content/themes/webkit/images/twitter-card.png: Added. * Websites/webkit.org/wp-content/plugins/social-meta.php:
* filenames: Removed.
* hello.txt: Added.
* modified:   LayoutTests/accessibility/aria-current-expected.txt * modified:   LayoutTests/accessibility/aria-current.html
* tmp: Added.
014e584f75 Vulkan: Separate out XFB data from ShaderInterfaceVariableInfo
10217bb0 libsysprof-capture: fix set_fd_blocking() c228634f libsysprof-capture: handle unwind length < 0 gracefully
11724133cd Metal: Separate vars with normal types
1e269594df ssci: canonicalize / backfill dependencies managed by DEPS
2d4a027dc7 Tests: Add Poppy Playtime Trace 8482f44fab android_helper cleanup: Inline test package name 318e5e0260 Vulkan: Update EGL_EXT_buffer_age implementation
319907d2cd Vulkan: Propagate the support of shaderInt16/64 feature 62cc379056 Metal: Remove UseResource old SDK fallbacks e90cd0c412 Vulkan: Add check for int8 extension support 78dd9a8eec Metal: Support compiling without OpenGL on macOS 2f28264ea2 Trace perf: add origin/main revision to summary if != HEAD 579a58552f Vulkan: Add query for 64bit fp support feature check
319907d2cd Vulkan: Propagate the support of shaderInt16/64 feature 62cc379056 Metal: Remove UseResource old SDK fallbacks e90cd0c412 Vulkan: Add check for int8 extension support 78dd9a8eec Metal: Support compiling without OpenGL on macOS 2f28264ea2 Trace perf: add origin/main revision to summary if != HEAD 579a58552f Vulkan: Add query for 64bit fp support feature check
Drive-by fix to spelling: deafult -> default
Drive-by fix: Unified sources.
Drive-by fix: always call the completion handler on the main thread.
Drive-by fix: remove an unnecessary header import for `FixedVector.h`.
Fix `error: equality comparison operator can only be defaulted in a class definition`.
Fix typo: `uint` -> `uint8_t`
Import of revision: 7fe9e78691410d5e2eb9bd78e79f14988e5b5643
Mark the button as `will-change: opacity` to give it its own layer.
Store active document in CapturingData: https://w3c.github.io/pointerevents/#dfn-active-document
Test expectation update: removed occlusion layers.
Upstream commit: https://github.com/web-platform-tests/wpt/commit/4e3b5de2eb8218cf18a1674618994efeb96e2cc0
Upstream commit: https://github.com/web-platform-tests/wpt/commit/b2528beea2a54778f7314768d2352ccee8b7993f
[1]: https://w3c.github.io/mathml-core/spec.html#prescripts-and-tensor-indices-mmultiscripts#:~:text=that%20is%20not%20an%20mprescripts%20element
[2]: https://github.com/WebKit/WebKit/commit/b70287103be9d7a826cb2697e7a951536dc2a04b [1]: https://github.com/WebKit/WebKit/commit/d8b1589e4915042608bb1a1a55bd291bcec76d51#diff-955991178e0e7971ad17ceface65532b503e98975aec31d0fc7093e0d80c3dc5R167-R169
`appearance: -apple-pay-button`.
`vertical-rl`: Left and right arrow keys can be used for next and previous respectively. `vertical-lr`: Right and left arrow keys can be used for next and previous respectively.
b380ed1f98 Vulkan: Add EGL_ANGLE_global_fence_sync
ef78e57015 Revert "Vulkan: disable warmUpPipelineCacheAtLink for Venus"
ef78e57015 Revert "Vulkan: disable warmUpPipelineCacheAtLink for Venus"
of the line and `line-break: after-white-space` is used.

We should make our canonicalization stricter to match the git-interpret-trailers parser _aside_ from "Canonical link", because these are all clearly _not_ trailers.

[Elliott Williams]

Sounds like a potential (easy) solution is for canonicalization to always ensure a blank line at the end of the committer's message. But this would be complicated by the things that may appear in commit messages which really _are_ trailers, such as Co-authored-by and Originally-landed-as.

Maybe canonicalization needs to look for a trailer group in the commit message, and if one does NOT exist, ensure an empty line before adding its own.

[Sam Sneddon [:gsnedders]]

(In reply to Elliott Williams from comment #1)
> Maybe canonicalization needs to look for a trailer group in the commit
> message, and if one does NOT exist, ensure an empty line before adding its
> own.

It tries to do this; it's just overly lax about parsing trailers, and thus picks up basically any line with a colon in it as a trailer.

I'm gonna post a PR which progresses this, which just alters our regex we use to find trailers. This doesn't solve many of the more fundamental bugs (we don't support folded trailers, we don't deal with comments in the commit message), but it does stop us from doing surprising reformatting of commit messages that end in many seemingly reasonable lines.

e.g., currently:

```
Subject

not a trailer: line
just: a line
```

Turns into:

```
Subject

not a trailer: line

just: a line
Canonical link: xxx
```

[Sam Sneddon [:gsnedders]]

Pull request: https://github.com/WebKit/WebKit/pull/36498

[Radar WebKit Bug Importer]

<rdar://problem/139923051>