Bug 241208 - [GitHub] Reconcile the top message of the PR with the git commit message
Summary: [GitHub] Reconcile the top message of the PR with the git commit message
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 239082
  Show dependency treegraph
 
Reported: 2022-06-01 17:22 PDT by Myles C. Maxfield
Modified: 2022-06-07 21:35 PDT (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2022-06-01 17:22:16 PDT
https://github.com/WebKit/WebKit/pull/892 is an example of someone trying to contribute, but (I think) getting mixed up between the contents of the top message of the pull request and the ChangeLog. If there is reviewer information in the PR, we should be able to insert it into the commit message.
Comment 1 Radar WebKit Bug Importer 2022-06-01 17:23:38 PDT
<rdar://problem/94256972>
Comment 2 Alexey Proskuryakov 2022-06-01 19:18:24 PDT
I don't think that I'm following what happened. Maybe Jonathan will, but it just looks like the commit message doesn't follow our format - it's just this:

>The source code and comment seem to have detached from each other, where the comment retains an old syntax.

I don't think that automation should be able to re-format a sentence like this to a proper commit message.

Not sure how the formatted commit message ended up in the top comment though.
Comment 3 Mehmet Oguz Derin 2022-06-02 09:03:18 PDT
> Not sure how the formatted commit message ended up in the top comment though.

In this instance, while authoring the PR, I took a look at the other PRs while writing the PR body message but not the commit message itself. It could be more accessible for people to adjust their PR if the top comment was the basis of merges akin to squash merges. Thanks!
Comment 4 Myles C. Maxfield 2022-06-02 11:39:05 PDT
(In reply to Alexey Proskuryakov from comment #2)
> it just looks like the commit message doesn't follow our format

That's true. However, the top post in the PR thread _does_ follow our format.

> 
> I don't think that automation should be able to re-format a sentence like
> this to a proper commit message.
> 
> Not sure how the formatted commit message ended up in the top comment though.

The git commit message said this:

```
[WGSL] Adjust comment of test to match the tested source
The source code and comment seem to have detached from each other, where the comment retains an old syntax.
```

The top post in the PR said this:

```
[WGSL] Adjust comment of test to match the tested source
https://bugs.webkit.org/show_bug.cgi?id=240773

Reviewed by Myles C. Maxfield.

* WGSLUnitTests/WGSLLexerTests.mm
```

And yet we failed to merge. Which is a bit contributor-hostile - they've told us everything we need to know to merge. They've even formatted it correctly! It's just in place B rather than in place A. We seem to just be insisting that the information come from place A rather than place B.

We should allow the information to come from either place. The whole point of the move to GitHub is to make it easier for contributors to contribute. If a contributor goes through the trouble of formatting the entry but putting it in the top post in the PR instead of the commit message, we shouldn't punish them but should instead graciously accept the information they have given us - because they've given us all the information we need.
Comment 5 Alexey Proskuryakov 2022-06-02 12:02:29 PDT
Yes, the suggestion is clear now. I'm still not sure about whether this is a one-off case, or something that many people will expect.

Merging works on commits, and never pulls commit message from top comment in other GitHub projects AFAIK. So it seems like it would be super unusual, and could confuse people more than help them. Or are my facts wrong?
Comment 6 Myles C. Maxfield 2022-06-02 14:52:50 PDT
(In reply to Alexey Proskuryakov from comment #5)
> Yes, the suggestion is clear now. I'm still not sure about whether this is a
> one-off case, or something that many people will expect.
> 
> Merging works on commits, and never pulls commit message from top comment in
> other GitHub projects AFAIK. So it seems like it would be super unusual, and
> could confuse people more than help them. Or are my facts wrong?

Well, we already have code that knows how to parse ChangeLog entries. A first pass at this could be "if one parses as a ChangeLog entry and the other one doesn't, use the first one." (We could certainly do better in the future, and try to merge stuff together, but this simple first idea would have solved this issue.)
Comment 7 Jonathan Bedard 2022-06-06 09:00:44 PDT
It is possible to construct a commit message from the PR description, but I'm a bit reluctant to do so at the moment.

The reason this gets a bit complicated is that if we decide to require commit signatures (which is decently likely in the coming months), we can't modify commit messages as we land changes and have to decide where the merge-commit's message comes from. The PR description is a leading candidate for that, but we then have to decide where the PR description comes from in the first place, and it may not be the commit message.
Comment 8 Myles C. Maxfield 2022-06-07 21:35:03 PDT
(In reply to Jonathan Bedard from comment #7)
> require commit signatures

How will this be able to rewrite the commit message to change the "Reviewed by NOBODY (OOPS)" to "Reviewed by SomePerson." in that scenario?