Node: 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.

:tada: :tada: :tada: :tada: :tada: :tada: :tada: :tada:

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

[image: :tada:] [image: :tada:] [image: :tada:] [image: :tada:] [image:
:tada:] [image: :tada:] [image: :tada:] [image: :tada:]

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
https://github.com/nodejs/node/issues/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.

[Fishrock123]

@sam-github huh? what you fill into the merge box goes in the commit description and message, no?

[sam-github]

@Fishrock123 oh, it does? that is not so bad, then.

[cjihrig]

Would anyone mind if I landed https://github.com/nodejs/node/pull/6000 with the new button? I can force push over it quickly if something goes wrong.

[Fishrock123]

@cjihrig I'd say go for it.

[cjihrig]

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: 39de601e1c3eda92eb2e37eca4e6aa960f206f39

[Fishrock123]

Hmmm, it has no way of checking that, does it?

GitHub's desktop GUI checks the message length and auto-wraps the description.

[jasnell]

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]

Wouldn't squashing lead to loss of data?

[jasnell]

@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]

Hmmm, it has no way of checking that, does it?

@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]

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-revised

edit:

https://github.com/nodejs/node/commit/fd1314873d2f3f316136e157a64c41f6d4619552
vs
https://github.com/nodejs/node/commit/200f763c4384bafc9c931b02ddddcf46f54fb0d3

[Fishrock123]

@TheAlphaNerd That hasn't happened to me?

[MylesBorins]

I'll run a couple tests, I might have done something wrong. But it is a possible edge case to be aware of

[jbergstroem]

So, where is this issue supposed to lead? Are we updating documentation to reflect that we would be ok with github-squashed commits?

[Fishrock123]

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]

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]

Yeah, I agree for now. Closing, can reopen if necessary.