Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit ccd3646

Browse filesBrowse files
TrottMylesBorins
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 <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>
1 parent 29efb02 commit ccd3646
Copy full SHA for ccd3646

File tree

Expand file treeCollapse file tree

2 files changed

+33
-29
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

2 files changed

+33
-29
lines changed
Open diff view settings
Collapse file

‎COLLABORATOR_GUIDE.md‎

Copy file name to clipboardExpand all lines: COLLABORATOR_GUIDE.md
+32-28Lines changed: 32 additions & 28 deletions
  • Display the source diff
  • Display the rich diff
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
Collapse file

‎doc/onboarding.md‎

Copy file name to clipboardExpand all lines: doc/onboarding.md
+1-1Lines changed: 1 addition & 1 deletion
  • Display the source diff
  • Display the rich diff
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 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.