Node: Reasons for not using GitHub's green button are unconvincing

[seishun]

The Landing Pull Requests section in COLLABORATOR_GUIDE.md admonishes against using the button for merging, but the cited reasons are arguable:

If you do, please force-push removing the merge.

Obsolete advice. This feature is disabled in this repo, so it's impossible to create a merge commit using the button in the first place.

The merge method will add an unnecessary merge commit.

See above. If anything, this should explain why it's disabled, rather than why one shouldn't use it.

The rebase & merge method adds metadata to the commit title.

Does "commit title" mean the first line of the commit message? If so, it's not clear how one can add metadata to the first line of text. Anyway, the commit messages for all commits in the PR are left unmodified.

The rebase method changes the author.

Doesn't.

The squash & merge method has been known to add metadata to the commit title.

Same question as above. It does add the PR number to the first line of the commit message, but you can remove it before confirming.

If more than one author has contributed to the PR, only the latest author will be considered during the squashing.

Not relevant for most PRs. Besides, how would you have multiple authors for a single commit anyway?

---

My proposal: replace this whole section with a warning about the automatically added PR number.

[mscdex]

If more than one author has contributed to the PR, only the latest author will be considered during the squashing.

Not relevant for most PRs. Besides, how would you have multiple authors for a single commit anyway?

Perhaps it's referring to multiple authors of multiple commits in the PR? That would be pretty common...

[seishun]

Perhaps it's referring to multiple authors of multiple commits in the PR? That would be pretty common...

That paragraph refers to "Squash and merge". You can't have multiple commits after squashing.

[mscdex]

But if you squash multiple commits from multiple authors, attribution is lost (except for the author of the last commit), which I think is what the sentence is getting at?

[seishun]

That's an argument against squashing, but not an argument against using the GitHub feature when you want to squash.

[gibfahn]

Last discussed in https://github.com/nodejs/node/issues/8893#issuecomment-250955448, cc/ @MylesBorins @evanlucas @Fishrock123 .

---

If you do, please force-push removing the merge.

Obsolete advice. This feature is disabled in this repo, so it's impossible to create a merge commit using the button in the first place.

Squash and merge and Rebase and merge are also part of "Github's green Pull Request button", so I don't think this is obsolete.

---

The rebase & merge method adds metadata to the commit title.

Does "commit title" mean the first line of the commit message? If so, it's not clear how one can add metadata to the first line of text. Anyway, the commit messages for all commits in the PR are left unmodified.

I think it was Myles who said that metadata was added, @MylesBorins can you provide an example?

Does Rebase and merge allow you to add metadata to the commits? If not then we should include that as a reason not to use it.

---

The rebase method changes the author.

Doesn't.

@evanlucas could you provide an example of Rebase and merge changing the author (or adding metadata)?

---

If more than one author has contributed to the PR, only the latest author will be considered during the squashing.

Not relevant for most PRs. Besides, how would you have multiple authors for a single commit anyway?

That's an argument against squashing, but not an argument against using the GitHub feature when you want to squash.

I think the warning about the issue with squashing is valid (yes it's an argument against squashing of any kind, but still a valid thing for Collaborators to consider IMO).

If we were going to allow squashing for PRs where it makes sense, then I think the most important advice would be to remember to add metadata and ensure CI has run (and delete the PR number from the title). I personally prefer using the Terminal to land commits, I'm not sure I'd check things as thoroughly if it was just a matter of clicking a button (e.g. running make lint to double-check before landing).

[seishun]

Squash and merge and Rebase and merge are also part of "Github's green Pull Request button", so I don't think this is obsolete.

Yeah but they don't create merge commits.

Does Rebase and merge allow you to add metadata to the commits? If not then we should include that as a reason not to use it.

No, it doesn't let you modify the commits in any way. If by metadata you mean "Reviewed-By:" etc, then you'd have to make sure it's added beforehand. That does make the feature less useful, but not "bad".

yes it's an argument against squashing of any kind, but still a valid thing for Collaborators to consider IMO

Possibly, but it doesn't belong under the "green button" section anyway. It's not specific to the button.

[lpinca]

A lot of PRs can be safely merged using the "squash and merge" button imo by only making sure that:

1. PR number is removed from title.
2. Metadata (PR-URL:, Reviewed-By:, etc.) is correctly added.
3. "Landed in" is added after merge.

This greatly simplifies the process of landing a PR which is, for good reasons, quite cumbersome.

[seishun]

Close the pull request with a "Landed in " comment. If your pull request shows the purple merged status then you should still add the "Landed in .." comment if you added multiple commits.

That implies that if there's purple merged status with a single commit, the "Landed in..." comment is unnecessary.

[gibfahn]

Squash and merge and Rebase and merge are also part of "Github's green Pull Request button", so I don't think this is obsolete.

Yeah but they don't create merge commits.

Force push removing the merge implies that you should undo the commits that were merged in, not just the merge commit (rebase & fast-forward merge is still a merge). If you Rebase and merge three commits, you should remove those three merged commits.

---

Close the pull request with a "Landed in " comment. If your pull request shows the purple merged status then you should still add the "Landed in .." comment if you added multiple commits.

That implies that if there's purple merged status with a single commit, the "Landed in..." comment is unnecessary.

Yes, that's true. I'm sure it was discussed somewhere, but I can't find it.

---

No, it doesn't let you modify the commits in any way. If by metadata you mean "Reviewed-By:" etc, then you'd have to make sure it's added beforehand.

If you're going through and adding metadata (from the command line), isn't it easier just to rebase and git merge -ff-only pr-branch master than going through multiple commits and checking that all the metadata is right?

That does make the feature less useful, but not "bad".

I don't think our docs say that it's "bad", just that it's not useful (and thus should be avoided).

---

I think the Squash and merge feature makes sense to use for single-commit PRs, and it makes sense to document it (assuming the metadata being added isn't happening any more). But if we're going to say the other two don't make sense in this project then we should disable them from the project settings.

[seishun]

Force push removing the merge implies that you should undo the commits that were merged in, not just the merge commit (rebase & fast-forward merge is still a merge).

Oh, so that's what that sentence actually means. That makes more sense.

If you're going through and adding metadata (from the command line), isn't it easier just to rebase and git merge -ff-only pr-branch master than going through multiple commits and checking that all the metadata is right?

I agree, but maybe someone prefers using the button for the actual merging for whatever reason. Does git merge -ff-only pr-branch master effect the purple merged status? If not, then that's one reason to prefer the button.

I don't think our docs say that it's "bad", just that it's not useful (and thus should be avoided).

Well it says above "Please never use GitHub's green "Merge Pull Request" button." and tells you to revert it if you accidentally do. That kinda implies that it's bad.

But if we're going to say the other two don't make sense in this project then we should disable them from the project settings.

Agreed. The current documentation is inconsistent with the project settings.

[gibfahn]

Does git merge -ff-only pr-branch master effect the purple merged status? If not, then that's one reason to prefer the button.

Yes it does. If you:

• Rebase on master and add all the metadata to your commits.
• Force-push to your branch
• git checkout master && git merge --ff-only mybranch

then GitHub works out that the commit hash of the head of your PR branch and master are the same, and adds the merged commit xyz into master message. If you look at closed PRs you can see the purple merged ones.

_EDIT:_ Just realised that this is covered in COLLABORATOR_GUIDE.md#L452-L463.

[gibfahn]

PR raised: https://github.com/nodejs/node/pull/11686

[addaleax]

Fwiw, the Squash & merge button does not change the author of a commit, but it does change the committer (sets it to GitHub <[email protected]> instead of your usual name/email combination). That’s not a horrible thing or anything, but for me personally it’s reason enough to avoid it.

[lpinca]

@addaleax I've run git show --pretty=fuller <object> on a couple of commits I've merged with the "Squash and merge" button on other repositories and it correctly reports me as the committer. Am I missing something?

[addaleax]

Hm – I was looking at 0a44b71c50835a21539334e6dfa5fe83e4e084d1 and assumed that that”s what @seishun did.

But you’re right, it actually seems to set the committer field to your primary Github email.

[seishun]

@lpinca @addaleax Apparently it sets the committer to GitHub if you're merging your own PR. Otherwise it sets it to your email.

[lpinca]

I guess that committed with <committer> is what is used as committer.

[jasnell]

@nodejs/ctc ... general thoughts on this? Using the squash and merge seems to be ok at this point.... or, at the very least, I have yet to see any serious issues associated with it's use.

[bnoordhuis]

I have no problem with it if there is no observable difference. People using the button would need to make sure they show up as the committer though.

[indutny]

Does green button validate line lengths in commit message?

[seishun]

People using the button would need to make sure they show up as the committer though.

That means we can't use "Squash and merge" for merging our own PRs. That's unfortunate.

Does green button validate line lengths in commit message?

Nope.

[indutny]

How would a committer validate line lengths then? I guess we can write a Chrome Extension, but overall it may not end up being a comfortable workflow.

[seishun]

Using a separate text editor I guess. A Chrome extension would be cool.

[Trott]

Aside: I lean heavily on @evanlucas's core-validate-commit tool when landing stuff and wish there was a way to have it be a pre-receive hook on the server side, but alas, that's a GitHub limitation.

[lpinca]

@indutny how is this different from the current workflow? You still have to validate line lengths in vim or your editor.

[indutny]

@lpinca yes, but I don't have to leave to the console at the moment. Furthermore since I check it in vim and use the contents of vim buffer as a commit message, I don't have to copy and paste it anywhere.

[lpinca]

@indutny I agree, it might be more error prone but I think it's not a valid reason to not use the "Squash and merge" button.

I usually copy/paste anyway, especially when adding metadata.

[bnoordhuis]

I'd count "more mistakes" as an observable difference, though.

[indutny]

@bnoordhuis that's my point too.

[Trott]

Should this remain open?

[lpinca]

@Trott I don't think so.

[Trott]

Closing due to inactivity. Feel free to re-open or comment if you think this should be open and the conversation should continue.

[seishun]

It still lists the same unconvincing reasons, so reopening.

[tniessen]

You still have to validate line lengths in vim or your editor.

That's where @Trott's suggestion to always use core-validate-commit comes in handy.

I'd count "more mistakes" as an observable difference, though.

I fear this might be true. Our current workflow more or less ensures that people pay a lot of attention to what they are doing, and I am not sure that will be true when using GitHub's button.

Don't forget this section of the guide, at least when it comes to code changes:

Run tests (make -j4 test or vcbuild test). Even though there was a successful continuous integration run, other changes may have landed on master since then, so running the tests one last time locally is a good practice.

Sure, we could as well run the tests using CI before merging the changes, but that won't detect problems caused by rebasing and - as CI can take quite some time - it will also miss anything caused by other changes to the respective branch (even though local testing takes time as well). We could as well run CI after pushing to master, but at that point the damage is already done.

[lpinca]

Sure, we could as well run the tests using CI before merging the changes, but that won't detect problems caused by rebasing and - as CI can take quite some time - it will also miss anything caused by other changes to the respective branch (even though local testing takes time as well).

This isn't a problem in practice. CI already rebases onto the target branch and chances for new commits to be added to target branch before merging are low. Patches are _usually_ not landed at such high rate (every 20 min or so).

[trevnorris]

_Summary_: While using the green button isn't "bad", it can only be used with a subset of PRs and introduces a greater margin of error while also adding unnecessary noise to the commit graph (if using the "merge" option). So maintaining that using the green button is off limits is reasonable and isn't necessary to discuss further.

---

@seishun
I created a patch w/ the author/committer as someone else then landed it using "Create a merge commit". Here's the git graph of what it looks like:

*   5984393 (HEAD -> tmp-master, origin/tmp-master) Merge pull request #3 from trevnorris/test-pr
|\  
| * e55a88a (origin/test-pr) test patch
|/  
* 8dae1d2 (tag: 0.2.2, origin/master, master) Release 0.2.2

Here's the commit information from the PR:

$ git log -1 --show-signature --pretty=fuller e55a88a
commit e55a88a5cc2536d5ba5692bc5858a9b95fc7b02b
gpg: Signature made Thu 27 Jul 2017 05:57:39 AM MDT using RSA key ID 820DC7F3
gpg: Good signature from "Trevor Norris (@trevnorris) <[email protected]>"
Author:     foo bar <[email protected]>
AuthorDate: Thu Jul 27 05:50:19 2017 -0600
Commit:     foo bar <[email protected]>
CommitDate: Thu Jul 27 05:57:37 2017 -0600

    test patch

And from the "merge commit":

$ git log -1 --show-signature --pretty=fuller 5984393
commit 5984393d5056ed84bae62c4acf41a9c03df6f4f1
Merge: 8dae1d2 e55a88a
Author:     Trevor Norris <[email protected]>
AuthorDate: Thu Jul 27 05:58:49 2017 -0600
Commit:     GitHub <[email protected]>
CommitDate: Thu Jul 27 05:58:49 2017 -0600

    Merge pull request #3 from trevnorris/test-pr

    test patch

    PR-URL: https://github.com/trevnorris/threx/pull/3

Issues from using this:

1. The metadata in the PR's (i.e. PR-URL) isn't added. It's only added to the merge commit.
2. A new commit is created for the merge. Even if the branch could have been fast-forwarded.
3. The user merging the commit has no way of signing it.

I realize you addressed (1) in the following comment:

If by metadata you mean "Reviewed-By:" etc, then you'd have to make sure it's added beforehand. That does make the feature less useful, but not "bad".

It might not be "bad", but it is more error prone. Adding more possibility for error isn't a step in the right direction.

As explained in (2) using the button only adds noise to the commit log. As a follow up I landed two PRs side-by-side using merge and this is the resulting graph:

*   5a750bf (HEAD -> tmp-master, origin/tmp-master) Merge pull request #4 from trevnorris/test-pr1
|\  
| * 494ef07 (origin/test-pr1) bye!
* |   3ffee27 Merge pull request #5 from trevnorris/test-pr2
|\ \  
| |/  
|/|   
| * cea7b8e (origin/test-pr2) what the?
|/  
*   5984393 Merge pull request #3 from trevnorris/test-pr
|\  
| * e55a88a (origin/test-pr) test patch
|/  
* 8dae1d2 (tag: 0.2.2, origin/master, master) Release 0.2.2

All of these commits could have been fast-forwarded, but instead we now have this interwoven mess to look at when reviewing our git history. Why should we say that's alright?

As for (3), I hope we'd work towards more developers signing their commits. So why add an option to a pipeline that would make a goal impossible?

The other two options require a rebase, and I've verified that both of these remove all signatures in the PR. You might say "just check if any of the commits in the PR are signed" but I ask again, why allow an option that is more error prone, and makes a goal of increasing the number of signed commits more difficult?

NOTE: There is no stated goal that more commits should be signed, but with our increasing security measures (e.g. enforcing 2FA) I think it's a safe assumption that increasing the number of signed commits is a desirable goal.

@tniessen

That's where @Trott's suggestion to always use core-validate-commit comes in handy.

Problem is we can't enforce it, and @Trott even follows up with:

wish there was a way to have it be a pre-receive hook on the server side, but alas, that's a GitHub limitation.

So there's no way to enforce commit message format, and because github does things like shorten URLs in the commit messages it's necessary to look at the <pr_url>.patch link to validate the commit message. From here I find it difficult to argue that merging from the command line is more cumbersome and/or equally or more error prone.

[lpinca]

@trevnorris the "merge commit" option has been discarded and marked as not viable from the beginning. The discussion was on the "squash and merge" option.

P.S. I noticed that we now have only the "merge commit" option enabled.

I'm pretty sure we disabled it and kept only the "squash and merge" option to reduce the risk of creating merge commits. Not sure when that changed.

[gibfahn]

@lpinca, that's not quite right, we kept the Create a merge commit button because we thought that would be obviously not something you should use. If we only kept the Squash and Merge option then people might start to use it.

Discussion: https://github.com/nodejs/node/pull/11686#issuecomment-289383925

[gibfahn]

It still lists the same unconvincing reasons, so reopening.

For anyone reading this, this is (AIUI) a documentation request, not a "we should revisit this discussion" request.

[lpinca]

@gibfahn thank you. I guess I stopped reading that thread when it was closed.

[BridgeAR]

Closing as it seems somewhat resolved (there are reasons not to use them and we now have better tools to land PRs).