Gerrit should definitely refuse to merge an out of date merge commit

This makes merging operation much harder. I also don't like the automatic merge commits, and try to avoid it if possible. On the other hand, making a merge commit and testing and running trybots takes time. It races with other people submitting CLs. If it rejects merge commit when some other CL went in in the interim, I guess it means it's almost impossible to do a merge during busy hours, and I have to do it in the weekend or midnight (Pacific time?). Sometimes I can do it, but it is not always feasible.

Regrettably, this also happened a number of times on master for dev.link merges: f092be8, 00753d5, da33f9c, cd42fa5.

Another unfortunate property of those commits is that it is somewhat difficult to go from commit to the Gerrit CL where code review took place, and to see the description of the (large) change.

Taking commit f092be8 as an example, it has no commit message body linking to a Gerrit CL, nor does it have a Change-Id line. Searching for the hash in Gerrit returns no results:

https://go-review.googlesource.com/q/f092be8fd839f5e61745c1b7f3b5990b4b8d6565

It requires viewing one of the parent commits, bed9325, which in turn has a commit message body describing the change, and a Change-Id line that makes it possible to find where code review happened:

https://go-review.googlesource.com/q/I38516d6c4b41021bc61c1b9886e701de5fa2b0f1

I agree we should try to find a way to avoid this for future merges, if possible to do so without significant compromises to other favorable properties of the current approach.

This issue has NeedsDecision state. What exactly is being decided, is it about re-writing history? On which branch(es)? Or is it about preventing this from happening in the future?

Taking commit f092be8 as an example, it has no commit message body linking to a Gerrit CL, nor does it have a Change-Id line. Searching for the hash in Gerrit returns no results:

https://go-review.googlesource.com/q/f092be8fd839f5e61745c1b7f3b5990b4b8d6565

Yeah, this is unfortunate, and is the part I don't really like. I wish Gerrit can link back the original CL from such commits.

On the other hand, making a merge commit and testing and running trybots takes time.

I see no problem with running the TryBots, and then rebasing the merge and immediately submitting. The result is the same, TryBots don't run on the Gerrit generated merge, and in general the Go tree doesn't operate with a submit queue, so the risk of in-flight collisions is accepted.

This issue has NeedsDecision state. What exactly is being decided, is it about re-writing history? On which branch(es)? Or is it about preventing this from happening in the future?

Both about rewriting dev.boringcrypto, and about avoiding this in the future.

This is how the rewrite looks like

$ git checkout -b filippo/boringcrypto/rewrite e067ce5225
$ git cherry-pick d41987bb5b
$ git cherry-pick 49367815c7
$ git cherry-pick 1d671675a5
$ git diff origin/dev.boringcrypto
$ git log --oneline --graph | head
* c19c0a047b - [dev.boringcrypto] misc/boring: add new releases to RELEASES file (66 seconds ago) <Filippo Valsorda>
* 36c94f8421 - [dev.boringcrypto] crypto/internal/boring: reject short signatures in VerifyRSAPKCS1v15 (2 minutes ago) <Filippo Valsorda>
* ee159d2f35 - [dev.boringcrypto] misc/boring: add new releases to RELEASES file (3 minutes ago) <Filippo Valsorda>
*   e067ce5225 - [dev.boringcrypto] all: merge master into dev.boringcrypto (4 weeks ago) <Filippo Valsorda>
|\
| * 9baafabac9 - crypto/rsa: refactor RSA-PSS signing and verification (5 weeks ago) <Filippo Valsorda>

Does gerrit have any sort of support for temporarily disabling commits to a given branch? If so then perhaps we could "turn off" commits to master while someone is preparing and submitting an important merge.

I don't think we want to disable submitting CLs. It's good that everyone could submit CLs at their convenient time. A submit queue would be a better approach.

I'm not convinced the Go tree has high traffic enough to require a fancier solution than rebase (as in reparent the merge) + submit immediately, which matches what we do with every CL.

The dev.boringcrypto history rewrite is done.