From d42d56f41c5f5a3204cf7dd5a694a759da1ffcd0 Mon Sep 17 00:00:00 2001 From: jessicaquynh Date: Tue, 11 Oct 2016 21:37:29 -0400 Subject: [PATCH 1/2] doc: explain why GitHub merge button is not used Adds documentation and explicit reasons on why the GitHub web interface button is not used. This was explained in the referenced issue by @TheAlphaNerd. Fixes: https://github.com/nodejs/node/issues/8893 --- doc/onboarding.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/doc/onboarding.md b/doc/onboarding.md index e9d6a4bd244b9e..32ec44000c4f25 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -164,8 +164,14 @@ onboarding session. ## Landing PRs: Details -* Please never use GitHub's green "Merge Pull Request" button. +* Please never use GitHub's green ["Merge Pull Request"](https://help.github.com/articles/merging-a-pull-request/#merging-a-pull-request-using-the-github-web-interface) button. * If you do, please force-push removing the merge. + * Reasons for not using the web interface button: + * The old merge method will write an ugly commit message. + * The old rebase & merge method adds metadata to the commit title. + * The latest rebase method changes the author. + * The squash & merge method has been known to add metadata to the commit title. + Update your `master` branch (or whichever branch you are landing on, almost always `master`) From 177d880ffcb1cae11cdb954941b88109327cec6e Mon Sep 17 00:00:00 2001 From: jessicaquynh Date: Tue, 11 Oct 2016 21:38:06 -0400 Subject: [PATCH 2/2] doc: explains why Reviewed-By is added in PRs Adds verbose reasons to the documentation on why the Reviewed-By metadata on a pull request is important. This was loosely mentioned as an issue in the referenced issue below, and answered by @addaleax. Ref: https://github.com/nodejs/node/issues/8893 change wording on documentation update Changes the initial commit to the recommended, and more accurate wording. Removed time qualifiers on documentation for git merge removes the ugly wording add a new reason why autosquashing is prohibited --- COLLABORATOR_GUIDE.md | 2 ++ doc/onboarding.md | 7 ++++--- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/COLLABORATOR_GUIDE.md b/COLLABORATOR_GUIDE.md index 2bab2e203145f0..5d0bfbfbb4339c 100644 --- a/COLLABORATOR_GUIDE.md +++ b/COLLABORATOR_GUIDE.md @@ -101,6 +101,8 @@ information regarding the change process: - A `Reviewed-By: Name ` line for yourself and any other Collaborators who have reviewed the change. + - Useful for @mentions / contact list if something goes wrong in the PR. + - Protects against the assumption that GitHub will be around forever. - A `PR-URL:` line that references the *full* GitHub URL of the original pull request being merged so it's easy to trace a commit back to the conversation that led up to that change. diff --git a/doc/onboarding.md b/doc/onboarding.md index 32ec44000c4f25..e22c876893fb20 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -167,10 +167,11 @@ onboarding session. * Please never use GitHub's green ["Merge Pull Request"](https://help.github.com/articles/merging-a-pull-request/#merging-a-pull-request-using-the-github-web-interface) button. * If you do, please force-push removing the merge. * Reasons for not using the web interface button: - * The old merge method will write an ugly commit message. - * The old rebase & merge method adds metadata to the commit title. - * The latest rebase method changes the author. + * The merge method will add an unnecessary merge commit. + * The rebase & merge method adds metadata to the commit title. + * The rebase method changes the author. * The squash & merge method has been known to add metadata to the commit title. + * If more than one author has contributed to the PR, only the latest author will be considered during the squashing. Update your `master` branch (or whichever branch you are landing on, almost always `master`)