Merge #19494: doc: CONTRIBUTING.md improvements
b03697b68e
doc: CONTRIBUTING.md improvements (Jon Atack) Pull request description: The motivation here was to add a mention of hygienic commits following a discussion today, e.g. something along the lines of: *Make sure each individual commit is hygienic, building successfully on its own without warnings, errors, or regressions, and that all tests pass.* While here, made various fixups. They are optional and can be omitted. ACKs for top commit: harding: ACKb03697b68e
Locally reviewed the word diff. MarcoFalke: ACKb03697b68e
🚌 practicalswift: ACKb03697b68e
hebasto: ACKb03697b68e
, I have reviewed the changes and they look OK, I agree they can be merged. Tree-SHA512: 6fb56219c311d914ec18fcf5d50fdbe3a51e4743a8cace93e348cb4a10c83b6fce631518f1455a1804d1fc81558b235bef58a8be6ccb1a010f46aa4143b1ebf5
This commit is contained in:
commit
89fa736783
1 changed files with 62 additions and 52 deletions
114
CONTRIBUTING.md
114
CONTRIBUTING.md
|
@ -6,27 +6,28 @@ welcome to contribute towards development in the form of peer review, testing
|
||||||
and patches. This document explains the practical process and guidelines for
|
and patches. This document explains the practical process and guidelines for
|
||||||
contributing.
|
contributing.
|
||||||
|
|
||||||
Firstly in terms of structure, there is no particular concept of "Core
|
First, in terms of structure, there is no particular concept of "Bitcoin Core
|
||||||
developers" in the sense of privileged people. Open source often naturally
|
developers" in the sense of privileged people. Open source often naturally
|
||||||
revolves around meritocracy where longer term contributors gain more trust from
|
revolves around a meritocracy where contributors earn trust from the developer
|
||||||
the developer community. However, some hierarchy is necessary for practical
|
community over time. Nevertheless, some hierarchy is necessary for practical
|
||||||
purposes. As such there are repository "maintainers" who are responsible for
|
purposes. As such, there are repository "maintainers" who are responsible for
|
||||||
merging pull requests as well as a "lead maintainer" who is responsible for the
|
merging pull requests, as well as a "lead maintainer" who is responsible for the
|
||||||
release cycle, overall merging, moderation and appointment of maintainers.
|
release cycle as well as overall merging, moderation and appointment of
|
||||||
|
maintainers.
|
||||||
|
|
||||||
Getting Started
|
Getting Started
|
||||||
---------------
|
---------------
|
||||||
|
|
||||||
New contributors are very welcome and needed.
|
New contributors are very welcome and needed.
|
||||||
|
|
||||||
Reviewing and testing is the most effective way you can contribute as a new
|
Reviewing and testing is highly valued and the most effective way you can contribute
|
||||||
contributor, and it also will teach you much more about the code and process
|
as a new contributor. It also will teach you much more about the code and
|
||||||
than opening PRs. Please refer to the section [peer review](#peer-review) later
|
process than opening pull requests. Please refer to the [peer review](#peer-review)
|
||||||
in this document.
|
section below.
|
||||||
|
|
||||||
Before you start contributing, familiarize yourself with the Bitcoin Core build
|
Before you start contributing, familiarize yourself with the Bitcoin Core build
|
||||||
system and tests. Refer to the documentation in the repository on how to build
|
system and tests. Refer to the documentation in the repository on how to build
|
||||||
Bitcoin Core and how to run the unit and functional tests.
|
Bitcoin Core and how to run the unit tests, functional tests, and fuzz tests.
|
||||||
|
|
||||||
There are many open issues of varying difficulty waiting to be fixed.
|
There are many open issues of varying difficulty waiting to be fixed.
|
||||||
If you're looking for somewhere to start contributing, check out the
|
If you're looking for somewhere to start contributing, check out the
|
||||||
|
@ -62,7 +63,7 @@ history logs can be found
|
||||||
on [http://www.erisian.com.au/bitcoin-core-dev/](http://www.erisian.com.au/bitcoin-core-dev/)
|
on [http://www.erisian.com.au/bitcoin-core-dev/](http://www.erisian.com.au/bitcoin-core-dev/)
|
||||||
and [http://gnusha.org/bitcoin-core-dev/](http://gnusha.org/bitcoin-core-dev/).
|
and [http://gnusha.org/bitcoin-core-dev/](http://gnusha.org/bitcoin-core-dev/).
|
||||||
|
|
||||||
Discussion about code base improvements happens in GitHub issues and on pull
|
Discussion about codebase improvements happens in GitHub issues and pull
|
||||||
requests.
|
requests.
|
||||||
|
|
||||||
The developer
|
The developer
|
||||||
|
@ -75,7 +76,7 @@ Contributor Workflow
|
||||||
--------------------
|
--------------------
|
||||||
|
|
||||||
The codebase is maintained using the "contributor workflow" where everyone
|
The codebase is maintained using the "contributor workflow" where everyone
|
||||||
without exception contributes patch proposals using "pull requests". This
|
without exception contributes patch proposals using "pull requests" (PRs). This
|
||||||
facilitates social contribution, easy testing and peer review.
|
facilitates social contribution, easy testing and peer review.
|
||||||
|
|
||||||
To contribute a patch, the workflow is as follows:
|
To contribute a patch, the workflow is as follows:
|
||||||
|
@ -113,6 +114,9 @@ In general, [commits should be atomic](https://en.wikipedia.org/wiki/Atomic_comm
|
||||||
and diffs should be easy to read. For this reason, do not mix any formatting
|
and diffs should be easy to read. For this reason, do not mix any formatting
|
||||||
fixes or code moves with actual code changes.
|
fixes or code moves with actual code changes.
|
||||||
|
|
||||||
|
Make sure each individual commit is hygienic: that it builds successfully on its
|
||||||
|
own without warnings, errors, regressions, or test failures.
|
||||||
|
|
||||||
Commit messages should be verbose by default consisting of a short subject line
|
Commit messages should be verbose by default consisting of a short subject line
|
||||||
(50 chars max), a blank line and detailed explanatory text as separate
|
(50 chars max), a blank line and detailed explanatory text as separate
|
||||||
paragraph(s), unless the title alone is self-explanatory (like "Corrected typo
|
paragraph(s), unless the title alone is self-explanatory (like "Corrected typo
|
||||||
|
@ -124,7 +128,7 @@ If a particular commit references another issue, please add the reference. For
|
||||||
example: `refs #1234` or `fixes #4321`. Using the `fixes` or `closes` keywords
|
example: `refs #1234` or `fixes #4321`. Using the `fixes` or `closes` keywords
|
||||||
will cause the corresponding issue to be closed when the pull request is merged.
|
will cause the corresponding issue to be closed when the pull request is merged.
|
||||||
|
|
||||||
Commit messages should never contain any `@` mentions.
|
Commit messages should never contain any `@` mentions (usernames prefixed with "@").
|
||||||
|
|
||||||
Please refer to the [Git manual](https://git-scm.com/doc) for more information
|
Please refer to the [Git manual](https://git-scm.com/doc) for more information
|
||||||
about Git.
|
about Git.
|
||||||
|
@ -158,10 +162,16 @@ Examples:
|
||||||
qt: Add feed bump button
|
qt: Add feed bump button
|
||||||
log: Fix typo in log message
|
log: Fix typo in log message
|
||||||
|
|
||||||
The body of the pull request should contain enough description about what the
|
The body of the pull request should contain sufficient description of *what* the
|
||||||
patch does together with any justification/reasoning. You should include
|
patch does, and even more importantly, *why*, with justification and reasoning.
|
||||||
references to any discussions (for example other tickets or mailing list
|
You should include references to any discussions (for example, other issues or
|
||||||
discussions).
|
mailing list discussions).
|
||||||
|
|
||||||
|
The description for a new pull request should not contain any `@` mentions. The
|
||||||
|
PR description will be included in the commit message when the PR is merged and
|
||||||
|
any users mentioned in the description will be annoyingly notified each time a
|
||||||
|
fork of Bitcoin Core copies the merge. Instead, make any username mentions in a
|
||||||
|
subsequent comment to the PR.
|
||||||
|
|
||||||
### Translation changes
|
### Translation changes
|
||||||
|
|
||||||
|
@ -197,13 +207,13 @@ before it will be merged. The basic squashing workflow is shown below.
|
||||||
# Save and quit.
|
# Save and quit.
|
||||||
git push -f # (force push to GitHub)
|
git push -f # (force push to GitHub)
|
||||||
|
|
||||||
Please update the resulting commit message if needed. It should read as a
|
Please update the resulting commit message, if needed. It should read as a
|
||||||
coherent message. In most cases, this means that you should not just list the
|
coherent message. In most cases, this means not just listing the interim
|
||||||
interim commits.
|
commits.
|
||||||
|
|
||||||
If you have problems with squashing (or other workflows with `git`), you can
|
If you have problems with squashing or other git workflows, you can enable
|
||||||
alternatively enable "Allow edits from maintainers" in the right GitHub
|
"Allow edits from maintainers" in the right-hand sidebar of the GitHub web
|
||||||
sidebar and ask for help in the pull request.
|
interface and ask for help in the pull request.
|
||||||
|
|
||||||
Please refrain from creating several pull requests for the same change.
|
Please refrain from creating several pull requests for the same change.
|
||||||
Use the pull request that is already open (or was created earlier) to amend
|
Use the pull request that is already open (or was created earlier) to amend
|
||||||
|
@ -287,8 +297,8 @@ In general, all pull requests must:
|
||||||
|
|
||||||
- Have a clear use case, fix a demonstrable bug or serve the greater good of
|
- Have a clear use case, fix a demonstrable bug or serve the greater good of
|
||||||
the project (for example refactoring for modularisation);
|
the project (for example refactoring for modularisation);
|
||||||
- Be well peer reviewed;
|
- Be well peer-reviewed;
|
||||||
- Have unit tests and functional tests where appropriate;
|
- Have unit tests, functional tests, and fuzz tests, where appropriate;
|
||||||
- Follow code style guidelines ([C++](doc/developer-notes.md), [functional tests](test/functional/README.md));
|
- Follow code style guidelines ([C++](doc/developer-notes.md), [functional tests](test/functional/README.md));
|
||||||
- Not break the existing test suite;
|
- Not break the existing test suite;
|
||||||
- Where bugs are fixed, where possible, there should be unit tests
|
- Where bugs are fixed, where possible, there should be unit tests
|
||||||
|
@ -315,7 +325,7 @@ spread out over GitHub, mailing list and IRC discussions).
|
||||||
#### Conceptual Review
|
#### Conceptual Review
|
||||||
|
|
||||||
A review can be a conceptual review, where the reviewer leaves a comment
|
A review can be a conceptual review, where the reviewer leaves a comment
|
||||||
* `Concept (N)ACK`, meaning "I do (not) agree in the general goal of this pull
|
* `Concept (N)ACK`, meaning "I do (not) agree with the general goal of this pull
|
||||||
request",
|
request",
|
||||||
* `Approach (N)ACK`, meaning `Concept ACK`, but "I do (not) agree with the
|
* `Approach (N)ACK`, meaning `Concept ACK`, but "I do (not) agree with the
|
||||||
approach of this change".
|
approach of this change".
|
||||||
|
@ -325,30 +335,28 @@ NACKs without accompanying reasoning may be disregarded.
|
||||||
|
|
||||||
#### Code Review
|
#### Code Review
|
||||||
|
|
||||||
After conceptual agreement on the change, code review can be provided. It is
|
After conceptual agreement on the change, code review can be provided. A review
|
||||||
starting with `ACK BRANCH_COMMIT`, where `BRANCH_COMMIT` is the top of the
|
begins with `ACK BRANCH_COMMIT`, where `BRANCH_COMMIT` is the top of the PR
|
||||||
topic branch. The review is followed by a description of how the reviewer did
|
branch, followed by a description of how the reviewer did the review. The
|
||||||
the review. The following
|
following language is used within pull request comments:
|
||||||
language is used within pull-request comments:
|
|
||||||
|
|
||||||
- "I have tested the code", involving
|
- "I have tested the code", involving change-specific manual testing in
|
||||||
change-specific manual testing in addition to running the unit and functional
|
addition to running the unit, functional, or fuzz tests, and in case it is
|
||||||
tests, and in case it is not obvious how the manual testing was done, it should
|
not obvious how the manual testing was done, it should be described;
|
||||||
be described;
|
|
||||||
- "I have not tested the code, but I have reviewed it and it looks
|
- "I have not tested the code, but I have reviewed it and it looks
|
||||||
OK, I agree it can be merged";
|
OK, I agree it can be merged";
|
||||||
- Nit refers to trivial, often non-blocking issues.
|
- A "nit" refers to a trivial, often non-blocking issue.
|
||||||
|
|
||||||
Project maintainers reserve the right to weigh the opinions of peer reviewers
|
Project maintainers reserve the right to weigh the opinions of peer reviewers
|
||||||
using common sense judgement and also may weight based on meritocracy: Those
|
using common sense judgement and may also weigh based on merit. Reviewers that
|
||||||
that have demonstrated a deeper commitment and understanding towards the project
|
have demonstrated a deeper commitment and understanding of the project over time
|
||||||
(over time) or have clear domain expertise may naturally have more weight, as
|
or who have clear domain expertise may naturally have more weight, as one would
|
||||||
one would expect in all walks of life.
|
expect in all walks of life.
|
||||||
|
|
||||||
Where a patch set affects consensus critical code, the bar will be set much
|
Where a patch set affects consensus-critical code, the bar will be much
|
||||||
higher in terms of discussion and peer review requirements, keeping in mind that
|
higher in terms of discussion and peer review requirements, keeping in mind that
|
||||||
mistakes could be very costly to the wider community. This includes refactoring
|
mistakes could be very costly to the wider community. This includes refactoring
|
||||||
of consensus critical code.
|
of consensus-critical code.
|
||||||
|
|
||||||
Where a patch set proposes to change the Bitcoin consensus, it must have been
|
Where a patch set proposes to change the Bitcoin consensus, it must have been
|
||||||
discussed extensively on the mailing list and IRC, be accompanied by a widely
|
discussed extensively on the mailing list and IRC, be accompanied by a widely
|
||||||
|
@ -365,7 +373,7 @@ about:
|
||||||
|
|
||||||
- It may be because of a feature freeze due to an upcoming release. During this time,
|
- It may be because of a feature freeze due to an upcoming release. During this time,
|
||||||
only bug fixes are taken into consideration. If your pull request is a new feature,
|
only bug fixes are taken into consideration. If your pull request is a new feature,
|
||||||
it will not be prioritized until the release is over. Wait for release.
|
it will not be prioritized until after the release. Wait for the release.
|
||||||
- It may be because the changes you are suggesting do not appeal to people. Rather than
|
- It may be because the changes you are suggesting do not appeal to people. Rather than
|
||||||
nits and critique, which require effort and means they care enough to spend time on your
|
nits and critique, which require effort and means they care enough to spend time on your
|
||||||
contribution, thundering silence is a good sign of widespread (mild) dislike of a given change
|
contribution, thundering silence is a good sign of widespread (mild) dislike of a given change
|
||||||
|
@ -375,16 +383,18 @@ about:
|
||||||
[developer notes](doc/developer-notes.md), is dangerous or insecure, is messily written, etc.
|
[developer notes](doc/developer-notes.md), is dangerous or insecure, is messily written, etc.
|
||||||
Identify and address any of the issues you find. Then ask e.g. on IRC if someone could give
|
Identify and address any of the issues you find. Then ask e.g. on IRC if someone could give
|
||||||
their opinion on the concept itself.
|
their opinion on the concept itself.
|
||||||
- It may be because your code is too complex for all but a few people. And those people
|
- It may be because your code is too complex for all but a few people, and those people
|
||||||
may not have realized your pull request even exists. A great way to find people who
|
may not have realized your pull request even exists. A great way to find people who
|
||||||
are qualified and care about the code you are touching is the
|
are qualified and care about the code you are touching is the
|
||||||
[Git Blame feature](https://help.github.com/articles/tracing-changes-in-a-file/). Simply
|
[Git Blame feature](https://help.github.com/articles/tracing-changes-in-a-file/). Simply
|
||||||
find the person touching the code you are touching before you and see if you can find
|
look up who last modified the code you are changing and see if you can find
|
||||||
them and give them a nudge. Don't be incessant about the nudging though.
|
them and give them a nudge. Don't be incessant about the nudging, though.
|
||||||
- Finally, if all else fails, ask on IRC or elsewhere for someone to give your pull request
|
- Finally, if all else fails, ask on IRC or elsewhere for someone to give your pull request
|
||||||
a look. If you think you've been waiting an unreasonably long amount of time (month+) for
|
a look. If you think you've been waiting for an unreasonably long time (say,
|
||||||
no particular reason (few lines changed, etc), this is totally fine. Try to return the favor
|
more than a month) for no particular reason (a few lines changed, etc.),
|
||||||
when someone else is asking for feedback on their code, and universe balances out.
|
this is totally fine. Try to return the favor when someone else is asking
|
||||||
|
for feedback on their code, and the universe balances out.
|
||||||
|
- Remember that the best thing you can do while waiting is give review to others!
|
||||||
|
|
||||||
|
|
||||||
Backporting
|
Backporting
|
||||||
|
@ -393,11 +403,11 @@ Backporting
|
||||||
Security and bug fixes can be backported from `master` to release
|
Security and bug fixes can be backported from `master` to release
|
||||||
branches.
|
branches.
|
||||||
If the backport is non-trivial, it may be appropriate to open an
|
If the backport is non-trivial, it may be appropriate to open an
|
||||||
additional PR, to backport the change, only after the original PR
|
additional PR to backport the change, but only after the original PR
|
||||||
has been merged.
|
has been merged.
|
||||||
Otherwise, backports will be done in batches and
|
Otherwise, backports will be done in batches and
|
||||||
the maintainers will use the proper `Needs backport (...)` labels
|
the maintainers will use the proper `Needs backport (...)` labels
|
||||||
when needed (the original author does not need to worry).
|
when needed (the original author does not need to worry about it).
|
||||||
|
|
||||||
A backport should contain the following metadata in the commit body:
|
A backport should contain the following metadata in the commit body:
|
||||||
|
|
||||||
|
|
Loading…
Reference in a new issue