1.. _docs-pw-style-commit-message: 2 3==================== 4Commit message style 5==================== 6Pigweed commit message bodies and summaries are limited to 72 characters wide to 7improve readability, and should be prefixed with the name of the module that the 8commit is affecting. The commits should describe what is changed, and why. When 9writing long commit messages, consider whether the content should go in the 10documentation or code comments instead. For example: 11 12.. code-block:: none 13 14 pw_some_module: Short capitalized description 15 16 Details about the change here. Include a summary of what changed, and a clear 17 description of why the change is needed. Consider what parts of the commit 18 message are better suited for documentation or code. 19 20 - Added foo, to fix issue bar 21 - Improved speed of qux 22 - Refactored and extended qux's test suite 23 24----------------------------- 25Include both "what" and "why" 26----------------------------- 27It is important to include a "why" component in most commits. Sometimes, why is 28evident - for example, reducing memory usage, optimizing, or fixing a bug. 29Otherwise, err on the side of over-explaining why, not under-explaining why. 30 31When adding the "why" to a commit, also consider if that "why" content should go 32into the documentation or code comments. 33 34.. admonition:: **Yes**: Reasoning in commit message 35 :class: checkmark 36 37 .. code-block:: none 38 39 pw_sync_xrtos: Invoke deadlock detector 40 41 During locking, run the deadlock detector if there are enough cycles. 42 Though this costs performance, several bugs that went unnoticed would have 43 been caught by turning this on earlier. Take the small hit by default to 44 better catch issues going forward; see extended docs for details. 45 46.. admonition:: **No**: Reasoning omitted 47 :class: error 48 49 .. code-block:: none 50 51 pw_sync_xrtos: Invoke deadlock detector 52 53 During locking, run the deadlock detector if there are enough cycles. 54 55------------------ 56Present imperative 57------------------ 58Use present imperative style instead of passive descriptive. 59 60.. admonition:: **Yes**: Uses imperative style for subject and text. 61 :class: checkmark 62 63 .. code-block:: none 64 65 pw_something: Add foo and bar functions 66 67 This commit correctly uses imperative present-tense style. 68 69.. admonition:: **No**: Uses non-imperative style for subject and text. 70 :class: error 71 72 .. code-block:: none 73 74 pw_something: Adds more things 75 76 Use present tense imperative style for subjects and commit. The above 77 subject has a plural "Adds" which is incorrect; should be "Add". 78 79--------------------------------------- 80Documentation instead of commit content 81--------------------------------------- 82Consider whether any of the commit message content should go in the 83documentation or code comments and have the commit message reference it. 84Documentation and code comments are durable and discoverable; commit messages 85are rarely read after the change lands. 86 87.. admonition:: **Yes**: Created docs and comments 88 :class: checkmark 89 90 .. code-block:: none 91 92 pw_i2c: Add and enforce invariants 93 94 Precisely define the invariants around certain transaction states, and 95 extend the code to enforce them. See the newly extended documentation in 96 this change for details. 97 98.. admonition:: **No**: Important content only in commit message 99 :class: error 100 101 .. code-block:: none 102 103 pw_i2c: Add and enforce invariants 104 105 Add a new invariant such that before a transaction, the line must be high; 106 and after, the line must be low, due to XXX and YYY. Furthermore, users 107 should consider whether they could ever encounter XXX, and in that case 108 should ZZZ instead. 109 110--------------------------- 111Lists instead of paragraphs 112--------------------------- 113Use bulleted lists when multiple changes are in a single change. Ideally, try to 114create smaller changes so this isn't needed, but larger changes are a practical 115reality. 116 117.. admonition:: **Yes**: Uses bulleted lists 118 :class: checkmark 119 120 .. code-block:: none 121 122 pw_complicated_module: Pre-work for refactor 123 124 Prepare for a bigger refactor by reworking some arguments before the larger 125 change. This change must land in downstream projects before the refactor to 126 enable a smooth transition to the new API. 127 128 - Add arguments to MyImportantClass::MyFunction 129 - Update MyImportantClass to handle precondition Y 130 - Add stub functions to be used during the transition 131 132.. admonition:: **No**: Long paragraph is hard to scan 133 :class: error 134 135 .. code-block:: none 136 137 pw_foo: Many things in a giant BWOT 138 139 This change does A, B, and C. The commit message is a Big Wall Of Text 140 (BWOT), which we try to discourage in Pigweed. Also changes X and Y, 141 because Z and Q. Furthermore, in some cases, adds a new Foo (with Bar, 142 because we want to). Also refactors qux and quz. 143 144------------ 145Subject line 146------------ 147The subject line (first line of the commit) should take the form ``pw_<module>: 148Short description``. The module should not be capitalized, but the description 149should (unless the first word is an identifier). See below for the details. 150 151.. admonition:: **No**: Uses a non-standard ``[]`` to indicate module: 152 :class: error 153 154 .. code-block:: none 155 156 [pw_foo]: Do a thing 157 158.. admonition:: **No**: Has a period at the end of the subject 159 :class: error 160 161 .. code-block:: none 162 163 pw_bar: Do something great. 164 165.. admonition:: **No**: Puts extra stuff after the module which isn't a module. 166 :class: error 167 168 .. code-block:: none 169 170 pw_bar/byte_builder: Add more stuff to builder 171 172Multiple modules 173================ 174Sometimes it is necessary to change code across multiple modules. 175 176#. **2-5 modules**: Use ``{}`` syntax shown below 177#. **>5 modules changed** - Omit the module names entirely 178#. **Changes mostly in one module** - If the commit mostly changes the 179 code in a single module with some small changes elsewhere, only list the 180 module receiving most of the content 181 182.. admonition:: **Yes**: Small number of modules affected; use {} syntax. 183 :class: checkmark 184 185 .. code-block:: none 186 187 pw_{foo, bar, baz}: Change something in a few places 188 189 When changes cross a few modules, include them with the syntax shown 190 above. 191 192.. admonition:: **Yes**: Many modules changed 193 :class: checkmark 194 195 .. code-block:: none 196 197 Change convention for how errors are handled 198 199 When changes cross many modules, skip the module name entirely. 200 201.. admonition:: **No**: Too many modules changed for subject 202 :class: error 203 204 .. code-block:: none 205 206 pw_{a, b, c, d, e, f, g, h, i, j}: Change convention for how errors are handled 207 208 When changes cross many modules, skip the module name entirely. 209 210Non-standard modules 211==================== 212Most Pigweed modules follow the format of ``pw_<foo>``; however, some do not, 213such as targets. Targets are effectively modules, even though they're nested, so 214they get a ``/`` character. 215 216.. admonition:: **Yes**: 217 :class: checkmark 218 219 .. code-block:: none 220 221 targets/xyz123: Tweak support for XYZ's PQR 222 223 PQR is needed for reason ZXW; this adds a performant implementation. 224 225Capitalization 226============== 227The text after the ``:`` should be capitalized, provided the first word is not a 228case-sensitive symbol. 229 230.. admonition:: **No**: Doesn't capitalize the subject 231 :class: error 232 233 .. code-block:: none 234 235 pw_foo: do a thing 236 237 Above subject is incorrect, since it is a sentence style subject. 238 239.. admonition:: **Yes**: Doesn't capitalize the subject when subject's first 240 word is a lowercase identifier. 241 :class: checkmark 242 243 .. code-block:: none 244 245 pw_foo: std::unique_lock cleanup 246 247 This commit message demonstrates the subject when the subject has an 248 identifier for the first word. In that case, follow the identifier casing 249 instead of capitalizing. 250 251 However, imperative style subjects often have the identifier elsewhere in 252 the subject; for example: 253 254 .. code-block:: none 255 256 pw_foo: Improve use of std::unique_lock 257 258------ 259Footer 260------ 261We support a number of `git footers`_ in the commit message, such as ``Bug: 262123`` in the message below: 263 264.. code-block:: none 265 266 pw_something: Add foo and bar functions 267 268 Bug: 123 269 270The footer syntax is described in the `git documentation 271<https://git-scm.com/docs/git-interpret-trailers>`_. Note in particular that 272multi-line footers are supported: 273 274.. code-block::none 275 276 pw_something: Add foo and bar functions 277 278 Test: Carried out manual tests of pw_console 279 as described in the documentation. 280 281You are encouraged to use the following footers when appropriate: 282 283* ``Bug``: Associates this commit with a bug (issue in our `bug tracker`_). The 284 bug will be automatically updated when the change is submitted. When a change 285 is relevant to more than one bug, include multiple ``Bug`` lines, like so: 286 287 .. code-block:: none 288 289 pw_something: Add foo and bar functions 290 291 Bug: 123 292 Bug: 456 293 294* ``Fixed`` or ``Fixes``: Like ``Bug``, but automatically closes the bug when 295 submitted. 296 297 .. code-block:: none 298 299 pw_something: Fix incorrect use of foo 300 301 Fixes: 123 302 303* ``Test``: The author can use this field to tell the reviewer how the change 304 was tested. Typically, this will be some combination of writing new automated 305 tests, running automated tests, and manual testing. 306 307 Note: descriptions of manual testing procedures belong in module 308 documentation, not in the commit message. Use the ``Test`` field to attest 309 that tests were carried out, not to describe the procedures in detail. 310 311 .. code-block:: none 312 313 pw_something: Fix incorrect use of foo 314 315 Test: Added a regression unit test. 316 317In addition, we support all of the `Chromium CQ footers`_, but those are 318relatively rarely useful. 319 320.. _bug tracker: https://bugs.chromium.org/p/pigweed/issues/list 321.. _Chromium CQ footers: https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/infra/cq.md#options 322.. _git footers: https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-footers.html 323