From eb9412624ab7d5af9ed7e4a1547252456541f309 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Tue, 26 Apr 2016 00:01:41 -0700 Subject: [PATCH 1/3] doc: note that PR-URL should be first metadata Convention has coalesced around putting the PR-URL as the first metadata item in pull requests, so much so that collaborators are now flagging other collaborators' commit messages when they don't do that. So let's make it official and document that the PR-URL should be first. Refs: https://github.com/nodejs/build/issues/325 Refs: https://github.com/nodejs/node/pull/6385 --- COLLABORATOR_GUIDE.md | 7 ++++--- doc/onboarding.md | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/COLLABORATOR_GUIDE.md b/COLLABORATOR_GUIDE.md index 90d73e80e9679b..b721302c9d0be0 100644 --- a/COLLABORATOR_GUIDE.md +++ b/COLLABORATOR_GUIDE.md @@ -91,11 +91,12 @@ The CTC should serve as the final arbiter where required. Always modify the original commit message to include additional meta information regarding the change process: -- A `Reviewed-By: Name ` line for yourself and any - other Collaborators who have reviewed the change. - 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. + conversation that led up to that change. Please be sure that the `PR-URL` + line appears first, above the `Reviewed-By` lines and any `Fixes` lines. +- A `Reviewed-By: Name ` line for yourself and any + other Collaborators who have reviewed the change. - A `Fixes: X` line, where _X_ either includes the *full* GitHub URL for an issue, and/or the hash and commit message if the commit fixes a bug in a previous commit. Multiple `Fixes:` lines may be added if diff --git a/doc/onboarding.md b/doc/onboarding.md index 1646080f3b3bbf..1cb5c3ebfebccc 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -164,11 +164,11 @@ Landing a PR * commits should follow `subsystem[,subsystem]: small description\n\nbig description\n\n` * first line 50 columns, all others 72 * add metadata: + * `PR-URL: ` (this line should always be the first metadata line!) * `Fixes: ` * `Reviewed-By: human ` * Easiest to use `git log` then do a search * (`/Name` + `enter` (+ `n` as much as you need to) in vim) - * `PR-URL: ` * `git push upstream master` * close the original PR with "Landed in ``". @@ -180,7 +180,7 @@ Landing a PR * Collaborators in alphabetical order by username * Label your pull request with the `doc` subsystem label * If you would like to run CI on your PR, feel free to - * Make sure to added the `PR-URL: `! + * Make sure to add the `PR-URL: `! ### final notes: From 89cadec7112ee051cdca34e4fdb7c4f0aeab5eda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johan=20Bergstr=C3=B6m?= Date: Tue, 26 Apr 2016 17:25:59 +1000 Subject: [PATCH 2/3] fix: reorder fixes/reviewed-by --- COLLABORATOR_GUIDE.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/COLLABORATOR_GUIDE.md b/COLLABORATOR_GUIDE.md index b721302c9d0be0..85cb87ab7a8223 100644 --- a/COLLABORATOR_GUIDE.md +++ b/COLLABORATOR_GUIDE.md @@ -95,12 +95,12 @@ information regarding the change process: pull request being merged so it's easy to trace a commit back to the conversation that led up to that change. Please be sure that the `PR-URL` line appears first, above the `Reviewed-By` lines and any `Fixes` lines. -- A `Reviewed-By: Name ` line for yourself and any - other Collaborators who have reviewed the change. - A `Fixes: X` line, where _X_ either includes the *full* GitHub URL for an issue, and/or the hash and commit message if the commit fixes a bug in a previous commit. Multiple `Fixes:` lines may be added if appropriate. +- A `Reviewed-By: Name ` line for yourself and any + other Collaborators who have reviewed the change. Review the commit message to ensure that it adheres to the guidelines outlined in the [contributing](https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit) guide. From eb41b3f0be8cfe64c3f36cd1c212d453d83e0632 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Johan=20Bergstr=C3=B6m?= Date: Tue, 26 Apr 2016 17:30:16 +1000 Subject: [PATCH 3/3] feature: order applies to all metadata values --- COLLABORATOR_GUIDE.md | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/COLLABORATOR_GUIDE.md b/COLLABORATOR_GUIDE.md index 85cb87ab7a8223..a360497f7fd44a 100644 --- a/COLLABORATOR_GUIDE.md +++ b/COLLABORATOR_GUIDE.md @@ -93,8 +93,7 @@ information regarding the change process: - 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. Please be sure that the `PR-URL` - line appears first, above the `Reviewed-By` lines and any `Fixes` lines. + conversation that led up to that change. - A `Fixes: X` line, where _X_ either includes the *full* GitHub URL for an issue, and/or the hash and commit message if the commit fixes a bug in a previous commit. Multiple `Fixes:` lines may be added if @@ -102,6 +101,9 @@ information regarding the change process: - A `Reviewed-By: Name ` line for yourself and any other Collaborators who have reviewed the change. +Make sure that the order of metadata follows the outline above; meaning +`PR-URL`, followed by `Fixes` and so on. + Review the commit message to ensure that it adheres to the guidelines outlined in the [contributing](https://github.com/nodejs/node/blob/master/CONTRIBUTING.md#step-3-commit) guide. @@ -213,8 +215,8 @@ fixup 7d6f433 test for feature B Save the file and close the editor. You'll be asked to enter a new commit message for that commit. This is a good moment to fix incorrect -commit logs, ensure that they are properly formatted, and add -`Reviewed-By` lines. +commit logs, ensure that they are properly formatted, and add metadata as +appropriate. Time to push it: