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

all: fix history of dev.regabi and dev.typeparams branches #43147

Closed
mdempsky opened this issue Dec 12, 2020 · 11 comments
Closed

all: fix history of dev.regabi and dev.typeparams branches #43147

mdempsky opened this issue Dec 12, 2020 · 11 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Soon This needs to be done soon. (regressions, serious bugs, outages)
Milestone

Comments

@mdempsky
Copy link
Member

@cherrymui noticed that https://go-review.googlesource.com/c/go/+/277032 (merge master into dev.regabi) was actually committed as a regular commit (one parent ref), rather than a merge commit (two parent refs). It's also been subsequently merged into dev.typeparams.

We should rewrite the history of these two branches so that "git bisect" later on will continue to work reliably in the future. Filing as an issue since my understanding is this is the Go release team's responsibility.

/cc @dmitshur @griesemer @dr2chase

@mdempsky mdempsky added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 12, 2020
@mdempsky mdempsky added this to the Go1.17 milestone Dec 12, 2020
@griesemer
Copy link
Contributor

FWI, I rewound (checked out) dev.regabi locally just before the most recent merge, merged again, and then cherry-picked the handful of changes on top. It's 5min of work (the merge is clean except for two files, cmd/compile/internal/ssa.go and cmd/compile/internal/builtin.go - so make a copy of these files beforehand and just replace them during the merge).

The main issue is that we will force rewrite history and invalidate clients. But I think it's ok if we communicate beforehand.

I will happily do the dev.typeparams branch as well if we agree that this is what we should do. Since there's a lot master changes that went into the merge, being able to dissect on each individual change (rather than the one huge CL) would be good, in my mind.

(Thanks to Cherry for finding this.)

@FiloSottile
Copy link
Contributor

This happens with some regularity, we should probably reprioritize #26201.

I think we should rewrite the history. Squashed merges often confuse git when doing later merges and will break blame.

Force pushes are destructive operations, so they require a Release or Security team member to obtain administrator privileges. Please reach out internally on Monday (or through my phone number if it needs to happen sooner).

@griesemer
Copy link
Contributor

cc: @findleyr who is also working on dev.typeparams.
cc: @rsc in case he has pending changes

@aclements
Copy link
Member

cc @golang/osp-team

@toothrot
Copy link
Contributor

I can take care of this.

@toothrot toothrot self-assigned this Dec 14, 2020
@dmitshur dmitshur added the Soon This needs to be done soon. (regressions, serious bugs, outages) label Dec 14, 2020
@toothrot
Copy link
Contributor

toothrot commented Dec 14, 2020

dev.regabi has been updated:

8ce37e4110 (HEAD -> dev.regabi, sso-direct/dev.regabi) [dev.regabi] cmd/compile: fix noopt builder
7e17b46c58 [dev.regabi] cmd/compile/internal/types: add IsScalar query method
2b76429eb0 [dev.regabi] cmd/compile: refactor type initialization code into helper
9c5241e520 [dev.regabi] cmd/compile: remove unnecessary String methods
267975dc47 Merge branch 'master' into dev.regabi
6c64b6db68 (master) cmd/compile: don't constant fold divide by zero

It looks like @rsc had landed some changes this morning while I was working on this. We'll coordinate on getting those back on the branch.

@mdempsky
Copy link
Member Author

It looks like the rewrite merged 6c64b6d, rather than 6d2b335 (what @dr2chase 's CL was attempting to merge). I think this should be fine (they're only a few commits apart), but thought I'd point out.

@mdempsky
Copy link
Member Author

mdempsky commented Dec 14, 2020

I've confirmed though that 6d2b335 merges cleanly on top of 267975d (@toothrot 's merge), and produces a tree identical to 2a1cf9d (@dr2chase 's merge). So the merge conflict resolutions appear to have been faithfully reproduced.

Thanks @toothrot and all!

Edit: Steps to reproduce yourself:

git checkout 267975d             # checkout @toothrot's merge
git merge -q -m merge 6d2b335    # merge up to the same commit @dr2chase merged
git diff 2a1cf9d                 # diff against @dr2chase's merge; should be empty output

@dmitshur
Copy link
Contributor

Thank you for verifying that @mdempsky.

@toothrot
Copy link
Contributor

Thanks for verifying, @mdempsky!

@griesemer and myself have corrected history for the dev.typeparams branch, and @griesemer is working on getting the changes that had previously landed re-submitted. I will leave this issue open until we've caught up.

@griesemer
Copy link
Contributor

With commit 8ec9e89, the dev.typeparams branch has caught up again.

@golang golang locked and limited conversation to collaborators Dec 14, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Soon This needs to be done soon. (regressions, serious bugs, outages)
Projects
None yet
Development

No branches or pull requests

7 participants