Repository navigation
New GitHub Merging #6002
Description
GitHub has a new merge button feature, allowing it do a "squash-merge". See: https://github.com/blog/2141-squash-your-commits
I've done a little test [1], [2], and it appears to do what we want. No merge commit.
🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉
I have disabled the old-style merge button use on the core repo here already. We should double check and make sure this meets our merge standards before using the new option though.
@nodejs/collaborators
Activity
jasnell commentedon Apr 1, 2016
First Microsoft does Bash and now this?! What is this future alternate
universe I'm living in?!
On Apr 1, 2016 12:41 PM, "Jeremiah Senkpiel" notifications@github.com
wrote:
MylesBorins commentedon Apr 1, 2016
@Fishrock123 when you do these merges do the commits end up rebased and at the top of the tree
Fishrock123 commentedon Apr 1, 2016
@thealphanerd Hmm, let's find out.
https://github.com/Fishrock123/node/commits/test-new-github-merge
doc: test new github merging
Fishrock123 commentedon Apr 1, 2016
@thealphanerd Yes, yes it does.
MylesBorins commentedon Apr 1, 2016
omg
jasnell commentedon Apr 1, 2016
It's definitely an improvement. I can imagine that if we enable this we'll have to keep a close eye on making sure that the commit metadata is still being added correctly to the commits. It also appears that this does not present the option of selecting which commits get squashed (we regularly end up with a few PRs whose commits should NOT be squashed on landing). This is certainly a step in the right direction but I'm not certain it's exactly what we need.
jasnell commentedon Apr 1, 2016
I will say, however, that turning off the merge commit option gets a huge hooray! from me.
sam-github commentedon Apr 1, 2016
That's sucky... they finally do what we want, almost then mess it up? It actually squashes, so your nice PR that carefully introduces a set of changes as incremental commits gets squashed into one.... and some devs crappy PR that is full of little commits with useless names also gets squashed into one with a bullet list of all the crappy commit messages? Just a tick box for "no merge commit for PRs" would have been handy, as a project wide setting. That it does an implicit rebase is pretty cool, though. But again, I wish I could make all PRs on my projects do the implicit rebase, AND leave the merge commit (not for node, of course).
Fishrock123 commentedon Apr 1, 2016
It will work for at least some portion of our commits by the looks of it.
Notable exceptions being: PRs with more than one commit, and those we need to amend nits on.
It'd be awesome of we could get a markdown template for the message box, but this is great incremental step.
sam-github commentedon Apr 1, 2016
It doesn't allow amending the commit metadata,
Reviewed-By: ..., so it only works if you pre-prepare the branch to have a commit with that info. And if you are at the CLI because you just did that... why not just do an additionalgit merge; git push, @Fishrock123 ?jasnell commentedon Apr 1, 2016
As @Fishrock123 says, it is at least a step in the right direction, just not sure how often we'd be able to use it.
sam-github commentedon Apr 1, 2016
Maybe its a step on a good path, possibly there will be no more steps after this. Though it is a sign that some devs are getting what they ask for, and that is good.
Fishrock123 commentedon Apr 1, 2016
@sam-github huh? what you fill into the merge box goes in the commit description and message, no?
2 remaining items
Fishrock123 commentedon Apr 1, 2016
@cjihrig I'd say go for it.
cjihrig commentedon Apr 1, 2016
So, that worked fine. I had to force push over it, but that's only because I had a long line in the commit message (human error).
Original: 0879dfe11ca0ea8ec8b4c26ad7775eaa66270330
Force push: 39de601
Fishrock123 commentedon Apr 1, 2016
Hmmm, it has no way of checking that, does it?
GitHub's desktop GUI checks the message length and auto-wraps the description.
jasnell commentedon Apr 1, 2016
Happy that it worked but definitely concerned that this will make it easier to overlook those kinds of errors. We'd have to make sure that all collaborators landing commits were disciplined in double checking the log.
drgroot commentedon Apr 1, 2016
Wouldn't squashing lead to loss of data?
jasnell commentedon Apr 1, 2016
@drgroot ... that depends on what gets squashed. We already as a common practice squash minor commits when landing PRs in order to keep the commit history fairly sane. However, quite often we will logically separate multiple commits in a single PR. It really depends on the individual PR. In this case, using the green button to squash ALL commits all the time is going to be problematic because it does not give us enough control.
cjihrig commentedon Apr 1, 2016
@Fishrock123 that was my own stupidity. It was originally wrapped like I had it in my local editor. When it available for me to edit, it had extra lines between all of the lines. When I was removing the extra lines, I unwrapped that line.
MylesBorins commentedon Apr 1, 2016
So I just checked it myself on staging and it would appear that github is adding meta data to the top of the commit (the merge data)
Merge pull request #5989 from npm/npm-2.15.1-node-4-revisededit:
fd13148
vs
200f763
Fishrock123 commentedon Apr 1, 2016
@thealphanerd That hasn't happened to me?
MylesBorins commentedon Apr 1, 2016
I'll run a couple tests, I might have done something wrong. But it is a possible edge case to be aware of
jbergstroem commentedon Apr 5, 2016
So, where is this issue supposed to lead? Are we updating documentation to reflect that we would be ok with github-squashed commits?
Fishrock123 commentedon Apr 5, 2016
Hmm, I'm not sure. We unfortunately still need to prefer the old merging for many issues.
@nodejs/ctc should we update the documentation to say this using the github button is ok under simple circumstances?
cjihrig commentedon Apr 5, 2016
I'd say don't document it. If someone can use the button without anyone noticing, then more power to them. Otherwise, just stick to the directions.
Fishrock123 commentedon Apr 5, 2016
Yeah, I agree for now. Closing, can reopen if necessary.