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: Gerrit edits are immediately overwritten by older GitHub commits #24887

Open
bradfitz opened this issue Apr 16, 2018 · 9 comments

Comments

Projects
None yet
7 participants
@bradfitz
Copy link
Member

commented Apr 16, 2018

I tried to edit a commit message on Gerrit, but gopherbot fought me and reverted it:

https://go-review.googlesource.com/c/sys/+/107302
https://go-review.googlesource.com/c/sys/+/107302/2..3

I'd only expect it to be reverted if there was a new patchset on GitHub's side.

/cc @andybons

@bradfitz bradfitz added the NeedsFix label Apr 16, 2018

@gopherbot gopherbot added this to the Unreleased milestone Apr 16, 2018

@gopherbot gopherbot added the Builders label Apr 16, 2018

@andybons

This comment has been minimized.

Copy link
Member

commented Apr 16, 2018

Yep. A consequence of the one-way nature of GerritBot v0.

@andybons andybons changed the title x/build/cmd/gerritbot: doesn't permit editing commit messages in Gerrit x/build/cmd/gerritbot: edits are unidirectional (GitHub → Gerrit only) Apr 20, 2018

@FiloSottile

This comment has been minimized.

Copy link
Member

commented May 2, 2018

More fighting. https://go-review.googlesource.com/c/crypto/+/110796

I think the 80/20 way to fix this would be to implement Gerrit → GitHub for the commit message, and just send a warning in a comment when a new patchset is uploaded. We can see if that causes rollbacks.

@andybons

This comment has been minimized.

Copy link
Member

commented May 2, 2018

That's fine with me. The issue is one of complexity. Synchronization of Gerrit/GitHub/Maintner is a non-trivial part of the tool. Something to watch out for.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 13, 2018

@bradfitz's original report and my #28713, which was de-dup'ed into this issue, are NOT about what @andybons retitled this as.

GerritBot v0 could be one-way and still NOT overwrite Gerrit changes made after the latest GitHub PR was pushed to Gerrit. That should be trivial to do: before re-pushing something from GitHub to Gerrit, see if that exact commit has already been pushed as an earlier patch set. If so, don't push it again. Everything is still one-way from GitHub to Gerrit, but local Gerrit changes would no longer be immediately overwritten.

In addition to the example Brad gave from April and the example I gave from a few days ago, here's one I stumbled over from July: https://golang.org/cl/124775.

I retitled this to make clear that the issue here is NOT about two-way sync, just about one-way sync not being quite so insistent on overwriting new Gerrit commits with old GitHub commits.

@rsc rsc changed the title x/build/cmd/gerritbot: edits are unidirectional (GitHub → Gerrit only) x/build/cmd/gerritbot: Gerrit edits are immediately overwritten by older GitHub commits Nov 13, 2018

@andybons andybons assigned andybons and unassigned julieqiu Feb 15, 2019

@bep

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

I suspect I stumbled upon this issue in https://go-review.googlesource.com/c/go/+/164958 -- where I have no idea how to edit my commit message of my first code contribution to this project. I felt rather stupid there for some time. If this is how it behaves for every PR, I would suggest you turn off that integration until this is fixed.

@bep

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2019

Seems it is possible to edit the first comment in the PR ... Which isnt' obvious if you don't know it...

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

I agree people are getting confused by this but in the bot's defense, it commented on the PR with a link to info which you didn't read. :)

@bep

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

@bradfitz I read it, but this particular thing wasn't clear to me. But to my defense, I'm from Norway, and English isn't my first language :-)

@bradfitz

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

Ideally the bot would work intuitively and we thus wouldn't need any documentation. The behavior is more obvious when there's exactly 1 commit in a PR. It's less clear what to do when we have 2+ commits in a PR and how to form their commit message. I think just using the first might be good enough. Follow-up commit messages are probably just "fix things" or "address feedback from reviewer".

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.