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

Gerrit: messy merge commits created when merges are not fresh #38920

Open
FiloSottile opened this issue May 7, 2020 · 9 comments
Open

Gerrit: messy merge commits created when merges are not fresh #38920

FiloSottile opened this issue May 7, 2020 · 9 comments

Comments

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented May 7, 2020

Gerrit took it upon itself to create a merge commit to merge CL 227650, itself a merge commit, because there was a commit on the target branch after the parent of the merge under review.

392a746 was created by Gerrit because d41987b was on master after the parent of e067ce5.

* 49367815c7 - (dev.boringcrypto) [dev.boringcrypto] crypto/internal/boring: reject short signatures in VerifyRSAPKCS1v15 (4 weeks ago) <Filippo Valsorda>
*   392a746b15 - Merge "[dev.boringcrypto] all: merge master into dev.boringcrypto" into dev.boringcrypto (4 weeks ago) <Gerrit Code Review>
|\
| *   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>
| | * aa4d92b8aa - cmd/link: skip symbol references when looking for missing symbols (5 weeks ago) <Joel Sing>
| | [...]
| | * 72e043e48b - cmd/doc: show variables of unexported types for -all (6 months ago) <Agniva De Sarker>
| | * c2edcf4b12 - crypto/tls: take key size into account in signature algorithm selection (6 months ago) <Filippo Valsorda>
* | | d41987bb5b - [dev.boringcrypto] misc/boring: add new releases to RELEASES file (6 weeks ago) <Filippo Valsorda>
|/ /
* | 79284c2873 - [dev.boringcrypto] crypto/internal/boring: make accesses to RSA types with finalizers safer (9 weeks ago) <Filippo Valsorda>
* | 6c64b188a5 - [dev.boringcrypto] crypto/internal/boring: update BoringCrypto module to certificate 3318 (9 weeks ago) <Filippo Valsorda>

I am tempted to rewrite history, as the dev.boringcrypto branch is confusing enough as is, and we need it to be clean enough for it to be possible to easily flatten into a patchset.

Gerrit should definitely refuse to merge an out of date merge commit, or update it, not make a merge commit for the merge commit. Regrettably, this also happened a number of times on master for dev.link merges: f092be8, 00753d5, da33f9c, cd42fa5.

git log --graph --oneline master is now an unreadable maze, in part because of this and in part because it's unavoidable for long-running branches that get multiple merges from master before being merged multiple times into master. Not sure what we should do about that.

/cc @golang/osp-team

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 7, 2020

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.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented May 7, 2020

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?

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 7, 2020

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.

@FiloSottile
Copy link
Member Author

@FiloSottile FiloSottile commented May 7, 2020

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.

@FiloSottile
Copy link
Member Author

@FiloSottile FiloSottile commented May 7, 2020

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

@thanm thanm commented May 7, 2020

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.

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented May 7, 2020

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.

@FiloSottile
Copy link
Member Author

@FiloSottile FiloSottile commented May 7, 2020

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.

@FiloSottile
Copy link
Member Author

@FiloSottile FiloSottile commented May 7, 2020

The dev.boringcrypto history rewrite is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.