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 · 17 comments
Open

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

andybons opened this issue May 11, 2018 · 17 comments
Labels
Builders x/build issues (builders, bots, dashboards) help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@andybons
Copy link
Member

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

@andybons andybons added help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 11, 2018
@andybons andybons added this to the Unreleased milestone May 11, 2018
@gopherbot gopherbot added the Builders x/build issues (builders, bots, dashboards) label May 11, 2018
@FiloSottile
Copy link
Contributor

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
Copy link
Contributor

bcmills 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
Copy link
Member

rasky 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
Copy link
Member

mvdan 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
Copy link
Contributor

@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
Copy link
Member

mvdan 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
Copy link
Contributor

dmitshur 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
Copy link
Member Author

andybons 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
Copy link
Contributor

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

And /cc @rauls5382.

@bradfitz
Copy link
Contributor

@aclements
Copy link
Member

aclements 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
Copy link
Contributor

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
Copy link
Contributor

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

@bradfitz
Copy link
Contributor

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
Copy link
Contributor

bcmills commented Sep 13, 2019

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
Copy link
Contributor

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
Copy link
Contributor

bcmills commented Sep 13, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builders x/build issues (builders, bots, dashboards) help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

11 participants