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 #8893 (comment), 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: #11686