doc: document use of the Squash and merge button #11686

gibfahn · 2017-03-04T15:30:36Z

The Create a Merge Commit and Rebase and Merge buttons are not
currently suitable for merging Pull Requests, and are thus disabled.
Squashing is okay, so we should document its use.

Fixes: #11674

Checklist

documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

doc

cc/ @seishun @MylesBorins @evanlucas @mscdex @lpinca

The `Create a Merge Commit` and `Rebase and Merge` buttons are not currently suitable for merging Pull Requests, and are thus disabled. Squashing is okay, so we should document its use. Fixes: nodejs#11674

gibfahn · 2017-03-04T15:31:28Z

If the consensus is that the Squash and merge button should not be used, then we should disable it in the repo settings.

seishun · 2017-03-04T15:34:02Z

COLLABORATOR_GUIDE.md

+button, as long as you:
+ * Remove the PR number, e.g. `(#1048)`, from the commit message title.
+ * Ensure that the commit message is correct, remove any fixup commit messages.
+ * Make sure CI was run on the latest update to the Pull Request.


Is this something specific to using the button?

@seishun in a certain sense it is as you shouldn't press the button without making sure that CI is passing. I would keep it.

@lpinca you should also not press the button until someone has reviewed the PR. Should this be mentioned here too?

I think not, that is already known by collaborators and is part of the on-boarding process. You can argue that landing a PR after ensuring that CI is passing is also part of the on-boarding process but I would still keep this sentence as it doesn't harm and is not off-topic.

This is just my opinion though.

@seishun also that is "included" in the next line:

Add the correct metadata (see below).

That means that PR must be reviewed.

I tried to give a complete list, as I assumed someone using the button to make life easier wouldn't necessarily want to read through the command line instructions to work out which rules still applied.

jasnell · 2017-03-04T17:18:02Z

COLLABORATOR_GUIDE.md

+["Squash and merge"](https://help.github.com/articles/about-pull-request-merges/#squash-and-merge-your-pull-request-commits)
+button, as long as you:
+ * Remove the PR number, e.g. `(#1048)`, from the commit message title.
+ * Ensure that the commit message is correct, remove any fixup commit messages.


Perhaps just say that the commit message needs to be updated to conform to the contributions guidelines?

seishun · 2017-03-05T19:16:17Z

Following @bnoordhuis's comment (#11674 (comment)), if we want to be strict about the committer field, then we should document that the "Squash and merge" button can only be used to merge others' PRs.

Fishrock123 · 2017-03-06T15:29:14Z

We should encourage people to not fully squash when it would make more sense to keep some separate commits though.

I'm still in favor of never using it because then at least everyone always gets taught how to do it and has full control each time.

seishun · 2017-03-06T15:32:25Z

I'm still in favor of never using it because then at least everyone always gets taught how to do it and has full control each time.

I'm fine either way, but in this case we should disable the button altogether.

lpinca · 2017-03-06T15:32:31Z

Ok then I think it doesn't make sense to have the merge buttons enabled. We can disable all of them and update this PR to completely remove the section about the merge button.

gibfahn · 2017-03-06T15:39:12Z

+1 to disabling the button if we're not using it, but I don't think we should remove the section altogether, as I recall @Trott's original reason for adding it was that "why can't we use it" is a question that got asked at every onboarding. I can update this PR to update the reasons though (as @seishun says, the current reasons aren't very accurate).

lpinca · 2017-03-06T15:45:17Z

I guess the question is asked after the collaborator is instructed to never use the button. If there is no button there is no need to tell the collaborator to not use it and also probably no question, but I agree that it is still useful to know why it is disabled.

targos · 2017-03-06T17:20:05Z

It is not possible to completely disable the button because at least one option must be selected:

seishun · 2017-03-07T09:33:48Z

I would support completely disabling the button despite the fact that it has some use, simply to prevent someone from using it incorrectly through accident or ignorance. But if we can't disable it completely, then I don't think we should prohibit its usage. Instead, we should explain why it should be avoided and let the collaborators decide for themselves.

fhinkel · 2017-03-26T10:00:35Z

Have we agreed on whether we can use the button or not? 🤔

jasnell · 2017-03-26T19:21:32Z

Ping @nodejs/ctc ... please take a moment to review this issue.

bnoordhuis · 2017-03-26T19:43:17Z

Per #11686 (comment), it would be an if a then b else c kind of rule, which is easier to get wrong than our current don't do b rule. -1 on changing it.

gibfahn · 2017-03-26T21:10:32Z

To summarize, the issues with the Squash and Merge button are:

It sets the Committer field to GitHub <noreply@github.com> if you apply it to your own commits. It works as expected for others commits.
You don't get the line length checking, or any of the other benefits that core-validate-commit gives you, which will probably lead to more mistakes being made using it.

We currently can't disable it, as you can only disable two of the three merge options (and we should probably choose to disable Create a merge commit and Rebase and merge as they are more problematic. I've emailed GitHub and they said they're looking at it FWIW.

I don't think we should prohibit its usage. Instead, we should explain why it should be avoided and let the collaborators decide for themselves.

I don't think we want to get into the situation where it's officially forbidden or discouraged, but some people use it.

If there was a browser extension that did what core-validate-commit does for the Squash and Merge button (and maybe also checked that CI was run) then maybe we could live with the "committed by Github means landed by whoever authored the commit" issue.

Trott · 2017-03-26T21:21:59Z

@gibfahn Should this PR be closed? Updated? Something else?

mscdex · 2017-03-26T21:32:51Z

@gibfahn I think another issue with it is it may encourage new contributors to squash commits that should be kept separate?

gibfahn · 2017-03-26T22:16:40Z

@Trott I think this can be closed, given feedback. However I think someone with access should probably disable the Allow rebase merging button in https://github.com/nodejs/node/settings.

@silverwind if you still think the justification for not using the buttons should be updated, feel free.

jasnell · 2017-03-27T01:16:59Z

Closing given the status of the conversation. Will bring up disabling the Allow rebase merging on the next CTC call to see if there are any objections.

Trott · 2017-03-27T04:44:51Z

Closing given the status of the conversation. Will bring up disabling the Allow rebase merging on the next CTC call to see if there are any objections.

Since it's so easy to undo, something that no one uses, and something that we don't want people to use, how about do it and leave a comment here @-mentioning CTC letting them know it happened and to comment if they have concerns. (Then we can focus on the Buffer thing at the CTC meeting, which feels like it could easily take the whole hour by itself...)

bnoordhuis · 2017-03-27T08:14:28Z

I've set it to 'Allow merge commits' for now.

Trott · 2017-03-27T08:44:23Z

@gibfahn Any chance you meant to say leave "Allow rebase commits" enabled but instead disable "Allow merge commits"? I think the former is less problematic than the latter, but maybe there's something I'm missing?

bnoordhuis · 2017-03-27T08:50:22Z

'Allow merge commits' seems logical to me. There can be no confusion whether it's okay to use the green button now because we never use merge commits; with the squash & rebase option it's less clear cut.

Trott · 2017-03-27T09:10:14Z

@nodejs/ctc The settings on the site have been updated (thanks @bnoordhuis) to disable the "Allow rebase commits" option. The reasoning is above. If you have any questions or concerns, comment here. Thanks!

seishun · 2017-03-27T12:25:17Z

I guess the guide needs to explain why they are disabled then.

doc: document use of the Squash and merge button

913c365

The `Create a Merge Commit` and `Rebase and Merge` buttons are not currently suitable for merging Pull Requests, and are thus disabled. Squashing is okay, so we should document its use. Fixes: nodejs#11674

gibfahn added the doc Issues and PRs related to the documentations. label Mar 4, 2017

gibfahn mentioned this pull request Mar 4, 2017

Reasons for not using GitHub's green button are unconvincing #11674

Closed

seishun reviewed Mar 4, 2017

View reviewed changes

lpinca approved these changes Mar 4, 2017

View reviewed changes

jasnell reviewed Mar 4, 2017

View reviewed changes

mscdex added the meta Issues and PRs related to the general management of the project. label Mar 4, 2017

jasnell added the ctc-review label Mar 26, 2017

Trott mentioned this pull request Mar 26, 2017

Node.js Foundation Core Technical Committee (CTC) Meeting 2017-03-29 nodejs/CTC#85

Closed

jasnell closed this Mar 27, 2017

gibfahn deleted the merge-button branch March 27, 2017 08:10

Trott removed the ctc-review label Apr 3, 2017

aliscco mentioned this pull request Jun 23, 2023

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#571

Open

leonardoadame mentioned this pull request Dec 1, 2023

[Snyk] Fix for 1 vulnerabilities leonardoadame/node#377

Open

aliscco mentioned this pull request Dec 2, 2023

[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#1015

Open

aliscco mentioned this pull request Jan 9, 2024

[Snyk] Security upgrade webpack from 4.47.0 to 5.1.1 aliscco/alisco-node#1197

Open

leonardoadame mentioned this pull request Jan 10, 2024

[Snyk] Security upgrade webpack from 4.47.0 to 5.1.1 leonardoadame/node#621

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc: document use of the Squash and merge button #11686

doc: document use of the Squash and merge button #11686

gibfahn commented Mar 4, 2017 •

edited

Loading

gibfahn commented Mar 4, 2017

seishun Mar 4, 2017

lpinca Mar 4, 2017

seishun Mar 4, 2017

lpinca Mar 4, 2017

lpinca Mar 4, 2017 •

edited

Loading

gibfahn Mar 4, 2017

jasnell Mar 4, 2017

indutny Mar 26, 2017

seishun commented Mar 5, 2017 •

edited

Loading

Fishrock123 commented Mar 6, 2017

seishun commented Mar 6, 2017

lpinca commented Mar 6, 2017

gibfahn commented Mar 6, 2017

lpinca commented Mar 6, 2017 •

edited

Loading

targos commented Mar 6, 2017

seishun commented Mar 7, 2017

fhinkel commented Mar 26, 2017

jasnell commented Mar 26, 2017

bnoordhuis commented Mar 26, 2017

gibfahn commented Mar 26, 2017 •

edited

Loading

Trott commented Mar 26, 2017

mscdex commented Mar 26, 2017

gibfahn commented Mar 26, 2017

jasnell commented Mar 27, 2017

Trott commented Mar 27, 2017

bnoordhuis commented Mar 27, 2017

Trott commented Mar 27, 2017

bnoordhuis commented Mar 27, 2017

Trott commented Mar 27, 2017

seishun commented Mar 27, 2017

doc: document use of the Squash and merge button #11686

doc: document use of the Squash and merge button #11686

Conversation

gibfahn commented Mar 4, 2017 • edited Loading

Checklist

Affected core subsystem(s)

gibfahn commented Mar 4, 2017

seishun Mar 4, 2017

Choose a reason for hiding this comment

lpinca Mar 4, 2017

Choose a reason for hiding this comment

seishun Mar 4, 2017

Choose a reason for hiding this comment

lpinca Mar 4, 2017

Choose a reason for hiding this comment

lpinca Mar 4, 2017 • edited Loading

Choose a reason for hiding this comment

gibfahn Mar 4, 2017

Choose a reason for hiding this comment

jasnell Mar 4, 2017

Choose a reason for hiding this comment

indutny Mar 26, 2017

Choose a reason for hiding this comment

seishun commented Mar 5, 2017 • edited Loading

Fishrock123 commented Mar 6, 2017

seishun commented Mar 6, 2017

lpinca commented Mar 6, 2017

gibfahn commented Mar 6, 2017

lpinca commented Mar 6, 2017 • edited Loading

targos commented Mar 6, 2017

seishun commented Mar 7, 2017

fhinkel commented Mar 26, 2017

jasnell commented Mar 26, 2017

bnoordhuis commented Mar 26, 2017

gibfahn commented Mar 26, 2017 • edited Loading

Trott commented Mar 26, 2017

mscdex commented Mar 26, 2017

gibfahn commented Mar 26, 2017

jasnell commented Mar 27, 2017

Trott commented Mar 27, 2017

bnoordhuis commented Mar 27, 2017

Trott commented Mar 27, 2017

bnoordhuis commented Mar 27, 2017

Trott commented Mar 27, 2017

seishun commented Mar 27, 2017

gibfahn commented Mar 4, 2017 •

edited

Loading

lpinca Mar 4, 2017 •

edited

Loading

seishun commented Mar 5, 2017 •

edited

Loading

lpinca commented Mar 6, 2017 •

edited

Loading

gibfahn commented Mar 26, 2017 •

edited

Loading