From 4a422be1acb4cd152e2f4276b82199a6af0d5099 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Sun, 1 Oct 2017 08:33:55 -0700 Subject: [PATCH] doc: reorganize COLLABORATOR_GUIDE.md Based on feedback during a recent collaborator onboarding, move the metadata description closer to where the instructions for editing a commit message are. Indicate which metadata are required and which are optional. Make the punctuation more consistent. Previously, sentences prior to instructions ended in a period, a colon, or no punctuation at all. Colon seems best, so changed the Technical How-To section to use colons. --- COLLABORATOR_GUIDE.md | 60 +++++++++++++++++++++++-------------------- doc/onboarding.md | 2 +- 2 files changed, 33 insertions(+), 29 deletions(-) diff --git a/COLLABORATOR_GUIDE.md b/COLLABORATOR_GUIDE.md index acbd3fa9b5d8c0..51fa6b01714cd8 100644 --- a/COLLABORATOR_GUIDE.md +++ b/COLLABORATOR_GUIDE.md @@ -371,25 +371,11 @@ The TSC should serve as the final arbiter where required. * If more than one author has contributed to the PR, keep the most recent author when squashing. -Always modify the original commit message to include additional meta -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. -- 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 `Refs:` line referencing a URL for any relevant background. -- 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. - Review the commit message to ensure that it adheres to the guidelines outlined in the [contributing](./CONTRIBUTING.md#commit-message-guidelines) guide. +Add all necessary [metadata](#metadata) to commit messages before landing. + See the commit log for examples such as [this one](https://github.com/nodejs/node/commit/b636ba8186) if unsure exactly how to format your commit messages. @@ -397,34 +383,33 @@ exactly how to format your commit messages. Additionally: - Double check PRs to make sure the person's _full name_ and email address are correct before merging. -- Except when updating dependencies, all commits should be self - contained (meaning every commit should pass all tests). This makes - it much easier when bisecting to find a breaking change. +- All commits should be self-contained (meaning every commit should pass all + tests). This makes it much easier when bisecting to find a breaking change. ### Technical HOWTO -Clear any `am`/`rebase` that may already be underway. +Clear any `am`/`rebase` that may already be underway: ```text $ git am --abort $ git rebase --abort ``` -Checkout proper target branch +Checkout proper target branch: ```text $ git checkout master ``` Update the tree (assumes your repo is set up as detailed in -[CONTRIBUTING.md](CONTRIBUTING.md#step-1-fork)) +[CONTRIBUTING.md](CONTRIBUTING.md#step-1-fork)): ```text $ git fetch upstream $ git merge --ff-only upstream/master ``` -Apply external patches +Apply external patches: ```text $ curl -L https://github.com/nodejs/node/pull/xxx.patch | git am --whitespace=fix @@ -442,21 +427,19 @@ against the original PR carefully and build/test on at least one platform before landing. If the 3-way merge fails, then it is most likely that a conflicting PR has landed since the CI run and you will have to ask the author to rebase. -Check and re-review the changes +Check and re-review the changes: ```text $ git diff upstream/master ``` -Check number of commits and commit messages +Check number of commits and commit messages: ```text $ git log upstream/master...master ``` -If there are multiple commits that relate to the same feature or -one with a feature and separate with a test for that feature, -you'll need to use `squash` or `fixup`: +Squash commits and add metadata: ```text $ git rebase -i upstream/master @@ -512,9 +495,29 @@ 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. + * The commit message text must conform to the [commit message guidelines](./CONTRIBUTING.md#commit-message-guidelines). + +* Modify the original commit message to include additional metadata regarding + the change process. (If you use Chrome or Edge, [`node-review`][] fetches + the metadata for you.) + + * Required: 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. + * Optional: 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. + * Optional: One or more `Refs:` lines referencing a URL for any relevant + background. + * Required: 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. + 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. @@ -678,3 +681,4 @@ LTS working group and the Release team. [Stability Index]: doc/api/documentation.md#stability-index [Enhancement Proposal]: https://github.com/nodejs/node-eps [git-username]: https://help.github.com/articles/setting-your-username-in-git/ +[`node-review`]: https://github.com/evanlucas/node-review diff --git a/doc/onboarding.md b/doc/onboarding.md index d028803aa0ef80..41df0d00bd93b3 100644 --- a/doc/onboarding.md +++ b/doc/onboarding.md @@ -158,7 +158,7 @@ onboarding session. * After one or two approvals, land the PR. * Be sure to add the `PR-URL: ` and appropriate `Reviewed-By:` metadata! * [`core-validate-commit`][] helps a lot with this – install and use it if you can! - * If you use Chrome, [`node-review`][] fetches the metadata for you + * If you use Chrome or Edge, [`node-review`][] fetches the metadata for you. ## Final notes