Skip to content

Commit

Permalink
doc: reorganize COLLABORATOR_GUIDE.md
Browse files Browse the repository at this point in the history
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.

PR-URL: #15710
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Benedikt Meurer <benedikt.meurer@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
  • Loading branch information
Trott authored and MylesBorins committed Nov 28, 2017
1 parent a8cff7a commit 4583f1b
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 29 deletions.
60 changes: 32 additions & 28 deletions COLLABORATOR_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -365,60 +365,45 @@ 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 <email>` 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.

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
Expand All @@ -436,21 +421,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
Expand Down Expand Up @@ -506,9 +489,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).

<a name="metadata"></a>
* 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 <email>` 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.
Expand Down Expand Up @@ -672,3 +675,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
2 changes: 1 addition & 1 deletion doc/onboarding.md
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ onboarding session.
* After one or two approvals, land the PR.
* Be sure to add the `PR-URL: <full-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

Expand Down

0 comments on commit 4583f1b

Please sign in to comment.