x/build/cmd/gerritbot: Gerrit edits are immediately overwritten by older GitHub commits

[bradfitz]

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

[andybons]

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

[FiloSottile]

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]

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]

@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.

[bep]

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]

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

[bradfitz]

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]

@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]

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".

[dmitshur]

From #35464 (comment):

When I want to take over a CL in Gerrit, I can forge the author and work in peace.

If I want to take over a CL in Gerrit that originated from Gerritbot, I need to race and get my change +2'ed and submitted before Gerritbot wakes up and overwrites my work.

@bradfitz Since you haven't mentioned it, I wanted to point out that it's also possible to take over the Pull Request on GitHub, as long as the author hasn't disallowed that by unchecking the "Allow edits from maintainers" checkbox.

[bradfitz]

@dmitshur, I'd rather stay in Gerrit, ideally.

[stamblerre]

I've also been bit by this a number of times - I'll take a look at implementing @rsc's suggestion above.

[FiloSottile]

This got me again in https://golang.org/cl/263277.

[ianlancetaylor]

Another case: https://golang.org/cl/257817.

[thepudds]

It seems from some very brief poking that gerritbot at one point might have had behavior that was closer to what Russ described as desirable in #24887 (comment), but it was changed in https://go.dev/cl/96999:

cmd/gerritbot: use full commit message to determine staleness

Instead of checking only against the last revision of a GitHub PR,
check to see if the Gerrit commit message matches that of the one
determined by the PR, which is the source of truth due to
GerritBot’s one-way nature.

(Perhaps that's off base or not useful, but a thread to pull on if anyone looks at this issue in the future. Changing to Russ' suggested behavior would not be as simple as just reverting that CL, and I'm not sure what data is available for comparison, but maybe the pr.UpdatedAt field could also be examined, or ____).