-
Notifications
You must be signed in to change notification settings - Fork 578
Guidance on squash-and-merge #18513
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
Guidance on squash-and-merge #18513
Conversation
pod/perlgit.pod
Outdated
branched from blead) via the GitHub.com interface. | ||
|
||
We have, however, noticed one peculiarity in this approach. If the branch you | ||
are merging into another has multiple commits, and if you select the "squash |
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.
In the case that I highlighted this problem occurring in (#18511), the source branch only had one commit so this is not specifically a problem to do with squashing multiple commits. I only used squash and merge because it was the default and I didn't think it would behave any differently from rebase and merge in this case.
squash commits within that branch, then either (a) merge into blead locally | ||
and push blead to origin; or (b) push the branch to origin and merge via the | ||
GitHub interface. | ||
|
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.
Given my comment above, would (b) actually have solved the problem? Squashing commits in a local checkout of the source branch would have been a no-op since the PR only contained one commit anyway. So the proposed solution then amounts solely to pushing the branch to origin and merging from that instead of directly from the original source branch. I don't know if that would solve the problem or not. (Note that the source branch in this case was in a branch on https://github.com/xenu/perl5, which is a fork of https://github.com/Perl/perl5.)
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.
The revised wording looks okay to me, though I won't know for sure whether the two proposed solutions really work until I try them out!
Another option would be to disable squash and merge (since it appears to be non-useful for our needs.) We could then document how to do it manually. |
+1. Squashing seems to be an occasionally desirable thing, but not the way GitHub does it. At the very least it shouldn't be the default option in the drop-down. And it might be nice if the currently disabled "Create a merge commit" was enabled -- as long as it behaves in a sensible manner and as long as the other option ("Rebase and merge") was made the default for anyone not aware of the issues, or not thinking about it too much. "Rebase and merge" and "Create a merge commit" seem to be the (only) two practices currently documented in the section "On merging and rebasing" in perlgit.pod. |
"Create a merge commit" doesn't do what we've typically wanted, ie. to make a merge with changes only on one side, which I'd guess could be called "Rebase and create a merge commit". |
Yeah, that is rather unfortunate |
We don't seem to have reached a consensus as to how to proceed, so I'm going to withdraw this pull request for the time being. |
No description provided.