Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
andybons opened this issue May 11, 2018 · 14 comments

Comments

Projects
None yet
10 participants
@andybons
Copy link
Member

commented May 11, 2018

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

This comment has been minimized.

Copy link
Member

commented May 11, 2018

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

This comment has been minimized.

Copy link
Member

commented May 12, 2018

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

This comment has been minimized.

Copy link
Member

commented May 12, 2018

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

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

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

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

@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

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

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

This comment has been minimized.

Copy link
Member

commented Aug 30, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Sep 2, 2018

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

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

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

And /cc @rauls5382.

@aclements

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

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

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 17, 2019

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. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.