New GitHub Merging

[Fishrock123]

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

[jasnell]

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:

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]
TestOrgPleaseIgnore/-TEST-#2, [2]
https://github.com/TestOrgPleaseIgnore/-TEST-/commits/master, and it
appears to do what we want. No merge commit.

[image: 🎉] [image: 🎉] [image: 🎉] [image: 🎉] [image:
🎉] [image: 🎉] [image: 🎉] [image: 🎉]

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 though.

@nodejs/collaborators https://github.com/orgs/nodejs/teams/collaborators

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly or view it on GitHub
#6002

[MylesBorins]

@Fishrock123 when you do these merges do the commits end up rebased and at the top of the tree

[Fishrock123]

@thealphanerd Hmm, let's find out.
https://github.com/Fishrock123/node/commits/test-new-github-merge

[Fishrock123]

@thealphanerd Yes, yes it does.

[MylesBorins]

omg

[jasnell]

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]

I will say, however, that turning off the merge commit option gets a huge hooray! from me.

[sam-github]

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]

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]

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 additional git merge; git push, @Fishrock123 ?

[jasnell]

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]

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.