-
Notifications
You must be signed in to change notification settings - Fork 927
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
Discuss PR merge strategies for 0.20-post-alpha #970
Comments
Regarding reproduction of initial EL 2.0 work: (ashamedly admitting that I didn't think of this sooner) 🤦 Some forks still have a treasure of old commits, mainly: Thx for keeping these commits around @francesca64 @Osspial! I'm pushing my archaeological-git-dig-in-progress at https://github.com/felixrabe/winit/branches/all?utf8=%E2%9C%93&query=el2 in case someone is interested. |
I'd be fine with moving to a EDIT: WRT the PRs you linked - I've got regrets about squashing those. I'd agree with your general assessments about when to squash and when to not. |
I prefer rebasing and merging, which does a rebase of the whole PR onto the master branch and then fast-forwards master without a separate merge commit. I find merge commits hard to navigate. (Case in point: The recent "web" branch merge trouble.) Of course, sometimes contributors will have to manually rebase and force-push their PRs, but this way, any merge conflicts have to be resolved only once. (I think so at least, not 100% sure.) |
@Osspial I see you started merging stuff into master. For the time being, I'm keeping a parallel "master-re" branch in my repository that is identical to "master" (different history, but identical content; but not when using GitHub compare for some reason :/ ), but with rebases instead of merges (easy for now, as there were no conflicts):
I keep my PRs rebased against the official master to avoid accidentally merging master-re into master. (That would be a merry mess :) ) |
@felixrabe Huh, I guess I wasn't entirely thinking there. I've gotten used to clicking "squash and merge", but the default there has changed, and I don't think there's any way to default to squash/merge if merge commits are enabled. I'm going to re-disable merge commits, since I think squash commits are generally better for small PRs and those are the majority of PRs we get. If we don't want to squash, we can plan to merge things manually. |
@Osspial I'm talking about "rebase merging": Would it be an option to default to that? |
@felixrabe How does rebase merging handle old commits that don't merge properly? I've tried to rebase those sorts of commits in the past, and the work that requires has always been really time-consuming and seemingly unnecessary. I feel like most PRs will fall into one of two categories:
Between those I don't see the place where rebase merging fits in, although I could absolutely be missing stuff here. |
I've also gone ahead and made a |
I'd like to start a discussion regarding how we merge PRs in the future (such as after we get to 0.20 beta, for example) to make commit history more readable.
Why: Mega-commits (resulting from the current stash-the-whole-PR-into-one-commit setting) are hard to read, especially when hunting down regressions. Examples:
On the other hand, I think squashing a handful of PR commits like "implemented foo" – "documented foo" – "oh, and changelog btw" into a single-commit merge is fine, as long as the resulting commit stays readable.
The text was updated successfully, but these errors were encountered: