diff options
Diffstat (limited to 'docs/style/commit_message.rst')
| -rw-r--r-- | docs/style/commit_message.rst | 296 |
1 files changed, 296 insertions, 0 deletions
diff --git a/docs/style/commit_message.rst b/docs/style/commit_message.rst new file mode 100644 index 000000000..5b4dbaff7 --- /dev/null +++ b/docs/style/commit_message.rst @@ -0,0 +1,296 @@ +.. _docs-pw-style-commit-message: + +==================== +Commit message style +==================== +Pigweed commit message bodies and summaries are limited to 72 characters wide to +improve readability, and should be prefixed with the name of the module that the +commit is affecting. The commits should describe what is changed, and why. When +writing long commit messages, consider whether the content should go in the +documentation or code comments instead. For example: + +.. code-block:: none + + pw_some_module: Short capitalized description + + Details about the change here. Include a summary of what changed, and a clear + description of why the change is needed. Consider what parts of the commit + message are better suited for documentation or code. + + - Added foo, to fix issue bar + - Improved speed of qux + - Refactored and extended qux's test suite + +----------------------------- +Include both "what" and "why" +----------------------------- +It is important to include a "why" component in most commits. Sometimes, why is +evident - for example, reducing memory usage, optimizing, or fixing a bug. +Otherwise, err on the side of over-explaining why, not under-explaining why. + +When adding the "why" to a commit, also consider if that "why" content should go +into the documentation or code comments. + +.. admonition:: **Yes**: Reasoning in commit message + :class: checkmark + + .. code-block:: none + + pw_sync_xrtos: Invoke deadlock detector + + During locking, run the deadlock detector if there are enough cycles. + Though this costs performance, several bugs that went unnoticed would have + been caught by turning this on earlier. Take the small hit by default to + better catch issues going forward; see extended docs for details. + +.. admonition:: **No**: Reasoning omitted + :class: error + + .. code-block:: none + + pw_sync_xrtos: Invoke deadlock detector + + During locking, run the deadlock detector if there are enough cycles. + +------------------ +Present imperative +------------------ +Use present imperative style instead of passive descriptive. + +.. admonition:: **Yes**: Uses imperative style for subject and text. + :class: checkmark + + .. code-block:: none + + pw_something: Add foo and bar functions + + This commit correctly uses imperative present-tense style. + +.. admonition:: **No**: Uses non-imperative style for subject and text. + :class: error + + .. code-block:: none + + pw_something: Adds more things + + Use present tense imperative style for subjects and commit. The above + subject has a plural "Adds" which is incorrect; should be "Add". + +--------------------------------------- +Documentation instead of commit content +--------------------------------------- +Consider whether any of the commit message content should go in the +documentation or code comments and have the commit message reference it. +Documentation and code comments are durable and discoverable; commit messages +are rarely read after the change lands. + +.. admonition:: **Yes**: Created docs and comments + :class: checkmark + + .. code-block:: none + + pw_i2c: Add and enforce invariants + + Precisely define the invariants around certain transaction states, and + extend the code to enforce them. See the newly extended documentation in + this change for details. + +.. admonition:: **No**: Important content only in commit message + :class: error + + .. code-block:: none + + pw_i2c: Add and enforce invariants + + Add a new invariant such that before a transaction, the line must be high; + and after, the line must be low, due to XXX and YYY. Furthermore, users + should consider whether they could ever encounter XXX, and in that case + should ZZZ instead. + +--------------------------- +Lists instead of paragraphs +--------------------------- +Use bulleted lists when multiple changes are in a single CL. Ideally, try to +create smaller CLs so this isn't needed, but larger CLs are a practical reality. + +.. admonition:: **Yes**: Uses bulleted lists + :class: checkmark + + .. code-block:: none + + pw_complicated_module: Pre-work for refactor + + Prepare for a bigger refactor by reworking some arguments before the larger + change. This change must land in downstream projects before the refactor to + enable a smooth transition to the new API. + + - Add arguments to MyImportantClass::MyFunction + - Update MyImportantClass to handle precondition Y + - Add stub functions to be used during the transition + +.. admonition:: **No**: Long paragraph is hard to scan + :class: error + + .. code-block:: none + + pw_foo: Many things in a giant BWOT + + This CL does A, B, and C. The commit message is a Big Wall Of Text + (BWOT), which we try to discourage in Pigweed. Also changes X and Y, + because Z and Q. Furthermore, in some cases, adds a new Foo (with Bar, + because we want to). Also refactors qux and quz. + +------------ +Subject line +------------ +The subject line (first line of the commit) should take the form ``pw_<module>: +Short description``. The module should not be capitalized, but the description +should (unless the first word is an identifier). See below for the details. + +.. admonition:: **No**: Uses a non-standard ``[]`` to indicate module: + :class: error + + .. code-block:: none + + [pw_foo]: Do a thing + +.. admonition:: **No**: Has a period at the end of the subject + :class: error + + .. code-block:: none + + pw_bar: Do something great. + +.. admonition:: **No**: Puts extra stuff after the module which isn't a module. + :class: error + + .. code-block:: none + + pw_bar/byte_builder: Add more stuff to builder + +Multiple modules +================ +Sometimes it is necessary to change code across multiple modules. + +#. **2-5 modules**: Use ``{}`` syntax shown below +#. **>5 modules changed** - Omit the module names entirely +#. **Changes mostly in one module** - If the commit mostly changes the + code in a single module with some small changes elsewhere, only list the + module receiving most of the content + +.. admonition:: **Yes**: Small number of modules affected; use {} syntax. + :class: checkmark + + .. code-block:: none + + pw_{foo, bar, baz}: Change something in a few places + + When changes cross a few modules, include them with the syntax shown + above. + +.. admonition:: **Yes**: Many modules changed + :class: checkmark + + .. code-block:: none + + Change convention for how errors are handled + + When changes cross many modules, skip the module name entirely. + +.. admonition:: **No**: Too many modules changed for subject + :class: error + + .. code-block:: none + + pw_{a, b, c, d, e, f, g, h, i, j}: Change convention for how errors are handled + + When changes cross many modules, skip the module name entirely. + +Non-standard modules +==================== +Most Pigweed modules follow the format of ``pw_<foo>``; however, some do not, +such as targets. Targets are effectively modules, even though they're nested, so +they get a ``/`` character. + +.. admonition:: **Yes**: + :class: checkmark + + .. code-block:: none + + targets/xyz123: Tweak support for XYZ's PQR + + PQR is needed for reason ZXW; this adds a performant implementation. + +Capitalization +============== +The text after the ``:`` should be capitalized, provided the first word is not a +case-sensitive symbol. + +.. admonition:: **No**: Doesn't capitalize the subject + :class: error + + .. code-block:: none + + pw_foo: do a thing + + Above subject is incorrect, since it is a sentence style subject. + +.. admonition:: **Yes**: Doesn't capitalize the subject when subject's first + word is a lowercase identifier. + :class: checkmark + + .. code-block:: none + + pw_foo: std::unique_lock cleanup + + This commit message demonstrates the subject when the subject has an + identifier for the first word. In that case, follow the identifier casing + instead of capitalizing. + + However, imperative style subjects often have the identifier elsewhere in + the subject; for example: + + .. code-block:: none + + pw_foo: Improve use of std::unique_lock + +------ +Footer +------ +We support a number of `git footers`_ in the commit message, such as ``Bug: +123`` in the message below: + +.. code-block:: none + + pw_something: Add foo and bar functions + + Bug: 123 + +You are encouraged to use the following footers when appropriate: + +* ``Bug``: Associates this commit with a bug (issue in our `bug tracker`_). The + bug will be automatically updated when the change is submitted. When a change + is relevant to more than one bug, include multiple ``Bug`` lines, like so: + + .. code-block:: none + + pw_something: Add foo and bar functions + + Bug: 123 + Bug: 456 + +* ``Fixed`` or ``Fixes``: Like ``Bug``, but automatically closes the bug when + submitted. + + .. code-block:: none + + pw_something: Fix incorrect use of foo + + Fixes: 123 + +In addition, we support all of the `Chromium CQ footers`_, but those are +relatively rarely useful. + +.. _bug tracker: https://bugs.chromium.org/p/pigweed/issues/list +.. _Chromium CQ footers: https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/infra/cq.md#options +.. _git footers: https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-footers.html |