-
-
Notifications
You must be signed in to change notification settings - Fork 320
Bring over changes from the patch release #1250
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
Bring over changes from the patch release #1250
Conversation
Many of the changes in master for the patch release have not been applied to draft-next. This is an attempt to merge those changes.
I'll read this more carefully tomorrow, but I take it it's not easy to cherry pick or merge stuff from master to this branch? Doing |
Could a rebase might have been more appropriate? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather this a rebase to keep the commit history.
@Relequestual I'm a big fan of rebasing, but I don't think it's appropriate in this case. Remember that a rebase rewrites history meaning that everyone who currently has the I originally did this as a merge, but the commit history was really wacky, so I dropped the merge commit and just added the changes in a new commit. The intention is that Rebasing is an option, but there are a few challenges associated with it. With a rebase, there are will be no PR which means it's more difficult to review and there's no record that it happened. Then we need to warn anyone who has the branch checked out and possibly assist them with the git-fu necessary to get their branch up-to-date with the changes. |
@jdesrosiers I think that maybe what @Relequestual meant was to rebase the commits from the patch release onto draft-next instead of either merging or just reconstructing into a flat commit? The rebased commits are then PR'd like any other set of commits appended to a branch. |
@handrews Do you mean applying commits from |
If you cherry pick, which it sounds like is what you're describing, git will know not to merge twice when the branch is later merged. |
@Julian I'm not sure how that's possible with commits that had conflicts. It can't just skip the commit or it misses the merge conflicts. So, even if git is smart enough to not have a conflict, there would have to be duplicate commits in the history. |
I'm ok with doing this however we all agree we want to do it. What are everyone's preference? |
I had an idea that I think will work well.
|
Rebasing draft-next will rewrite the commits, meaning it won't be possible to see what the commits originally were (and PRs that were made to the draft-next branch will have their history altered as well). The only proper way to handle a situation like this is to merge, where the merge commit is not empty (as it is for uneventful merges), and all the conflicts resolved in that merge commit (which hopefully should be small). This can be done fairly easily by saving a copy of the files as you have them here in this PR, and using that content as the resolved form of the files in the merge commit. The diffs from old-main to the head commit, and from draft-next to the head commit, should both make intuitive sense, when viewed separately (which is a good thing to do when reviewing this change). It's not nice as a PR though - so I suggest making a new branch (say "new-main"), making the merge there, then pushing the branch and commenting in this PR what you called it so everyone can review using their favourite tools. Then when it's approved, simply |
Closing in favor of #1251 |
Many of the changes in master for the patch release have not been applied to draft-next. This is an attempt to merge those changes. Weird things may have happened with the merge, so please review carefully.