x/build/cmd/gerritbot: commit message source confuses a lot of people

[andybons]

Mapping from the title/first comment -> Gerrit commit message is confusing to almost everyone. It would be nice if this were either more clear or the commit message was determined by a more user-friendly method. Since PRs can have multiple commits, squashing all commit messages into one for the final commit message is one solution.

Open to thoughts on others.

/cc @bradfitz @bcmills @FiloSottile

[FiloSottile]

Squashing is almost guaranteed not to lead to a commit message we want to submit. This is hard.

Maybe put something like

### Write commit message below following https://github.com/golang/go/wiki/CommitMessage
net/http: frob the quux before blarfing

[longer description here in the body]

Fixes #nnnn
### End of commit message

in the PR template and crop on those lines?

[bcmills]

The first commit in the sequence is likely to contain the real purpose of the PR, but that's hard to edit as the change progresses.

OTOH, we can fairly easily edit the commit message in Gerrit, right?

So maybe start with the title of the PR as the first line and the contents of the first commit message as the body, and after that retain whatever is in Gerrit instead of trying to keep the message in sync as the PR evolves?

[rasky]

Another option would be to add the following features to GerritBot:

• De-markdown the PR description while importing into Gerrit, and word-wrap it while we're at it. With a good markdown parser that parses into an AST, this should be possible and work in most cases.
• Alert with a comment if the title isn't in the right format.

I think this would fix most cases seen in current PRs.

[mvdan]

I agree that this is very confusing. Yet another contributor is fighting gerritbot today: https://go-review.googlesource.com/c/go/+/126897

How about using the commit message if there's exactly one commit, but using the PR title+body otherwise? That seems to be the best of both worlds. If someone knows how to squash/rebase commits, they very likely know how to update commit messages that way. If someone doesn't know how to do that, they can use the existing mechanism.

[bradfitz]

@mvdan, that would be my preference too (if 1 commit, use it).

The one downside is that it precludes us from easily fixing their commit message before submitting, which we often do to save a round of back-and-forth. At least, we'd need to a way to flag a CL as "Gerrit-owned-and-frozen" so Gerritbot stops trying to update it so we can tweak the commit message in Gerrit & then submit.

[mvdan]

What if gerritbot didn't undo commit message changes, if they had been updated by a gerrit user with submit privileges?

Of course, if the author updates the PR, the change would be lost. But it should work if all you need is to update the commit message and have it temporarily stick until you hit the submit button.

[dmitshur]

A property of the current strategy that I think is favorable is that it (indirectly) encourages people to write high quality PR titles/descriptions. That's good both inside the Go project and outside.

How about using the commit message if there's exactly one commit, but using the PR title+body otherwise? That seems to be the best of both worlds.

A trade-off with that approach we should consider is that it would make the strategy more complex. The current scheme is not very intuitive, but it is simple and consistent. It doesn't change behavior when going from 1 to more commits.

At least, we'd need to a way to flag a CL as "Gerrit-owned-and-frozen" so Gerritbot stops trying to update it so we can tweak the commit message in Gerrit & then submit.

What if gerritbot didn't undo commit message changes, if they had been updated by a gerrit user with submit privileges?

Of course, if the author updates the PR, the change would be lost. But it should work if all you need is to update the commit message and have it temporarily stick until you hit the submit button.

I'll want to revisit the original design doc on the PR mirroring and see if I'm missing something, but right now I don't see why we can't implement 2-way syncing for commit messages. If someone edits the commit message in Gerrit, gerritbot could resolve the mismatch by editing the PR title/description to match, couldn't it?

[andybons]

I'll want to revisit the original design doc on the PR mirroring and see if I'm missing something, but right now I don't see why we can't implement 2-way syncing for commit messages. If someone edits the commit message in Gerrit, gerritbot could resolve the mismatch by editing the PR title/description to match, couldn't it?

It’s not unreasonable (implementing 2-way syncing), but naturally adds complexity to the implementation. One of reasons for this is that we have three data sources—maintner, GitHub, and Gerrit. Maintner is eventually consistent, so to avoid posting duplicate messages or repeating a patch upload, gerritbot still needs to keep track of which changes are “dirty” and should be ignored until the maintner corpus catches up. It also still needs to make calls to GitHub’s API since maintner doesn’t support PRs yet.

It’s very easy to say “this commit message doesn’t match the one in Gerrit, so upload a new one so that it matches.” This is how it works now. 2-way syncing is not infeasible, just more logic branches to consider.

I’m wary of spending more time on this until we can gather more data on the number of PRs, how many were merged, and any other info we can collect on how effective this has been in attracting quality contributions. I want to carefully consider the cost/benefit ratio in an objective way before putting more engineering effort in due to the large number of other things we have to do.

If someone from the community wants to pick this up and own it, however, I’d be all for it.

[bradfitz]

@aclements was just complaining about this to me today, so copying him here too.

And /cc @rauls5382.

[bradfitz]

Existing docs:

• https://github.com/golang/go/wiki/CommitMessage#github-pull-requests
• https://github.com/golang/go/wiki/GerritBot#how-does-gerritbot-determine-the-final-commit-message

[aclements]

This is causing me problems on CL 158337. @rauls5382 left the template text in his description. I tried editing it in Gerrit, but GerritBot quickly undid my edit, so I asked @rauls5382 to update his commit message, but of course that didn't work either.

Is there even a work-around for this? I suggested that @rauls5382 try editing the first message in the "conversation" for that PR, but I don't know if that will work.

[bradfitz]

Is there even a work-around for this? I suggested that @rauls5382 try editing the first message in the "conversation" for that PR, but I don't know if that will work.

Yes. Just edit the PR description on GitHub.

I often do it for users when I don't want to wait for a round-trip, but I'll let @rauls5382 do it here.

[bradfitz]

Oh, and yes... the "first message in the conversation" is the PR description.

[bradfitz]

Btw, the instructions do include the text:

+ Delete these instructions once you have read and applied them

So one benefit of the current system is that it indicates whether people have read the instructions. :-)

[bcmills]

Just edit the PR description on GitHub.
[…]
the "first message in the conversation" is the PR description.

That appears to use the raw text of the message, not the GitHub-rendered text. Since the rendered text is what people actually see, that's still pretty confusing.

[dmitshur]

What do you mean by "GitHub-rendered text"? GitHub renders the PR description (written in GitHub Flavored Markdown format) into HTML, and then displays HTML (with CSS applied).

Are you saying part of the confusion is because people expect the rendered HTML to be what we use in Gerrit, but Gerrit just displays the raw markdown without rendering it to HTML?

[bcmills]

Are you saying part of the confusion is because people expect the rendered HTML to be what we use in Gerrit, but Gerrit just displays the raw markdown without rendering it to HTML?

Yes, exactly. Particularly when the GitHub rendering changes an https://github.com/golang/go/issues/[…] URL into a #[…] shortlink.