Skip to content

Commit 54768f5

Browse files
Trottevanlucas
authored andcommitted
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. PR-URL: #15710 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Benedikt Meurer <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
1 parent 8dfd5a5 commit 54768f5

File tree

2 files changed

+33
-29
lines changed

2 files changed

+33
-29
lines changed

COLLABORATOR_GUIDE.md

+32-28
Original file line numberDiff line numberDiff line change
@@ -371,60 +371,45 @@ The TSC should serve as the final arbiter where required.
371371
* If more than one author has contributed to the PR, keep the most recent
372372
author when squashing.
373373

374-
Always modify the original commit message to include additional meta
375-
information regarding the change process:
376-
377-
- A `PR-URL:` line that references the *full* GitHub URL of the original
378-
pull request being merged so it's easy to trace a commit back to the
379-
conversation that led up to that change.
380-
- A `Fixes: X` line, where _X_ either includes the *full* GitHub URL
381-
for an issue, and/or the hash and commit message if the commit fixes
382-
a bug in a previous commit. Multiple `Fixes:` lines may be added if
383-
appropriate.
384-
- A `Refs:` line referencing a URL for any relevant background.
385-
- A `Reviewed-By: Name <email>` line for yourself and any
386-
other Collaborators who have reviewed the change.
387-
- Useful for @mentions / contact list if something goes wrong in the PR.
388-
- Protects against the assumption that GitHub will be around forever.
389-
390374
Review the commit message to ensure that it adheres to the guidelines outlined
391375
in the [contributing](./CONTRIBUTING.md#commit-message-guidelines) guide.
392376

377+
Add all necessary [metadata](#metadata) to commit messages before landing.
378+
393379
See the commit log for examples such as
394380
[this one](https://github.com/nodejs/node/commit/b636ba8186) if unsure
395381
exactly how to format your commit messages.
396382

397383
Additionally:
398384
- Double check PRs to make sure the person's _full name_ and email
399385
address are correct before merging.
400-
- Except when updating dependencies, all commits should be self
401-
contained (meaning every commit should pass all tests). This makes
402-
it much easier when bisecting to find a breaking change.
386+
- All commits should be self-contained (meaning every commit should pass all
387+
tests). This makes it much easier when bisecting to find a breaking change.
403388

404389
### Technical HOWTO
405390

406-
Clear any `am`/`rebase` that may already be underway.
391+
Clear any `am`/`rebase` that may already be underway:
407392

408393
```text
409394
$ git am --abort
410395
$ git rebase --abort
411396
```
412397

413-
Checkout proper target branch
398+
Checkout proper target branch:
414399

415400
```text
416401
$ git checkout master
417402
```
418403

419404
Update the tree (assumes your repo is set up as detailed in
420-
[CONTRIBUTING.md](CONTRIBUTING.md#step-1-fork))
405+
[CONTRIBUTING.md](CONTRIBUTING.md#step-1-fork)):
421406

422407
```text
423408
$ git fetch upstream
424409
$ git merge --ff-only upstream/master
425410
```
426411

427-
Apply external patches
412+
Apply external patches:
428413

429414
```text
430415
$ 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
442427
before landing. If the 3-way merge fails, then it is most likely that a conflicting
443428
PR has landed since the CI run and you will have to ask the author to rebase.
444429

445-
Check and re-review the changes
430+
Check and re-review the changes:
446431

447432
```text
448433
$ git diff upstream/master
449434
```
450435

451-
Check number of commits and commit messages
436+
Check number of commits and commit messages:
452437

453438
```text
454439
$ git log upstream/master...master
455440
```
456441

457-
If there are multiple commits that relate to the same feature or
458-
one with a feature and separate with a test for that feature,
459-
you'll need to use `squash` or `fixup`:
442+
Squash commits and add metadata:
460443

461444
```text
462445
$ git rebase -i upstream/master
@@ -512,9 +495,29 @@ Save the file and close the editor. You'll be asked to enter a new
512495
commit message for that commit. This is a good moment to fix incorrect
513496
commit logs, ensure that they are properly formatted, and add
514497
`Reviewed-By` lines.
498+
515499
* The commit message text must conform to the
516500
[commit message guidelines](./CONTRIBUTING.md#commit-message-guidelines).
517501

502+
<a name="metadata"></a>
503+
* Modify the original commit message to include additional metadata regarding
504+
the change process. (If you use Chrome or Edge, [`node-review`][] fetches
505+
the metadata for you.)
506+
507+
* Required: A `PR-URL:` line that references the *full* GitHub URL of the
508+
original pull request being merged so it's easy to trace a commit back to
509+
the conversation that led up to that change.
510+
* Optional: A `Fixes: X` line, where _X_ either includes the *full* GitHub URL
511+
for an issue, and/or the hash and commit message if the commit fixes
512+
a bug in a previous commit. Multiple `Fixes:` lines may be added if
513+
appropriate.
514+
* Optional: One or more `Refs:` lines referencing a URL for any relevant
515+
background.
516+
* Required: A `Reviewed-By: Name <email>` line for yourself and any
517+
other Collaborators who have reviewed the change.
518+
* Useful for @mentions / contact list if something goes wrong in the PR.
519+
* Protects against the assumption that GitHub will be around forever.
520+
518521
Run tests (`make -j4 test` or `vcbuild test`). Even though there was a
519522
successful continuous integration run, other changes may have landed on master
520523
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.
678681
[Stability Index]: doc/api/documentation.md#stability-index
679682
[Enhancement Proposal]: https://github.com/nodejs/node-eps
680683
[git-username]: https://help.github.com/articles/setting-your-username-in-git/
684+
[`node-review`]: https://github.com/evanlucas/node-review

doc/onboarding.md

+1-1
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ onboarding session.
158158
* After one or two approvals, land the PR.
159159
* Be sure to add the `PR-URL: <full-pr-url>` and appropriate `Reviewed-By:` metadata!
160160
* [`core-validate-commit`][] helps a lot with this – install and use it if you can!
161-
* If you use Chrome, [`node-review`][] fetches the metadata for you
161+
* If you use Chrome or Edge, [`node-review`][] fetches the metadata for you.
162162

163163
## Final notes
164164

0 commit comments

Comments
 (0)