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 CL. Ideally, try to 114create smaller CLs so this isn't needed, but larger CLs are a practical reality. 115 116.. admonition:: **Yes**: Uses bulleted lists 117 :class: checkmark 118 119 .. code-block:: none 120 121 pw_complicated_module: Pre-work for refactor 122 123 Prepare for a bigger refactor by reworking some arguments before the larger 124 change. This change must land in downstream projects before the refactor to 125 enable a smooth transition to the new API. 126 127 - Add arguments to MyImportantClass::MyFunction 128 - Update MyImportantClass to handle precondition Y 129 - Add stub functions to be used during the transition 130 131.. admonition:: **No**: Long paragraph is hard to scan 132 :class: error 133 134 .. code-block:: none 135 136 pw_foo: Many things in a giant BWOT 137 138 This CL does A, B, and C. The commit message is a Big Wall Of Text 139 (BWOT), which we try to discourage in Pigweed. Also changes X and Y, 140 because Z and Q. Furthermore, in some cases, adds a new Foo (with Bar, 141 because we want to). Also refactors qux and quz. 142 143------------ 144Subject line 145------------ 146The subject line (first line of the commit) should take the form ``pw_<module>: 147Short description``. The module should not be capitalized, but the description 148should (unless the first word is an identifier). See below for the details. 149 150.. admonition:: **No**: Uses a non-standard ``[]`` to indicate module: 151 :class: error 152 153 .. code-block:: none 154 155 [pw_foo]: Do a thing 156 157.. admonition:: **No**: Has a period at the end of the subject 158 :class: error 159 160 .. code-block:: none 161 162 pw_bar: Do something great. 163 164.. admonition:: **No**: Puts extra stuff after the module which isn't a module. 165 :class: error 166 167 .. code-block:: none 168 169 pw_bar/byte_builder: Add more stuff to builder 170 171Multiple modules 172================ 173Sometimes it is necessary to change code across multiple modules. 174 175#. **2-5 modules**: Use ``{}`` syntax shown below 176#. **>5 modules changed** - Omit the module names entirely 177#. **Changes mostly in one module** - If the commit mostly changes the 178 code in a single module with some small changes elsewhere, only list the 179 module receiving most of the content 180 181.. admonition:: **Yes**: Small number of modules affected; use {} syntax. 182 :class: checkmark 183 184 .. code-block:: none 185 186 pw_{foo, bar, baz}: Change something in a few places 187 188 When changes cross a few modules, include them with the syntax shown 189 above. 190 191.. admonition:: **Yes**: Many modules changed 192 :class: checkmark 193 194 .. code-block:: none 195 196 Change convention for how errors are handled 197 198 When changes cross many modules, skip the module name entirely. 199 200.. admonition:: **No**: Too many modules changed for subject 201 :class: error 202 203 .. code-block:: none 204 205 pw_{a, b, c, d, e, f, g, h, i, j}: Change convention for how errors are handled 206 207 When changes cross many modules, skip the module name entirely. 208 209Non-standard modules 210==================== 211Most Pigweed modules follow the format of ``pw_<foo>``; however, some do not, 212such as targets. Targets are effectively modules, even though they're nested, so 213they get a ``/`` character. 214 215.. admonition:: **Yes**: 216 :class: checkmark 217 218 .. code-block:: none 219 220 targets/xyz123: Tweak support for XYZ's PQR 221 222 PQR is needed for reason ZXW; this adds a performant implementation. 223 224Capitalization 225============== 226The text after the ``:`` should be capitalized, provided the first word is not a 227case-sensitive symbol. 228 229.. admonition:: **No**: Doesn't capitalize the subject 230 :class: error 231 232 .. code-block:: none 233 234 pw_foo: do a thing 235 236 Above subject is incorrect, since it is a sentence style subject. 237 238.. admonition:: **Yes**: Doesn't capitalize the subject when subject's first 239 word is a lowercase identifier. 240 :class: checkmark 241 242 .. code-block:: none 243 244 pw_foo: std::unique_lock cleanup 245 246 This commit message demonstrates the subject when the subject has an 247 identifier for the first word. In that case, follow the identifier casing 248 instead of capitalizing. 249 250 However, imperative style subjects often have the identifier elsewhere in 251 the subject; for example: 252 253 .. code-block:: none 254 255 pw_foo: Improve use of std::unique_lock 256 257------ 258Footer 259------ 260We support a number of `git footers`_ in the commit message, such as ``Bug: 261123`` in the message below: 262 263.. code-block:: none 264 265 pw_something: Add foo and bar functions 266 267 Bug: 123 268 269The footer syntax is described in the `git documentation 270<https://git-scm.com/docs/git-interpret-trailers>`_. Note in particular that 271multi-line footers are supported: 272 273.. code-block::none 274 275 pw_something: Add foo and bar functions 276 277 Test: Carried out manual tests of pw_console 278 as described in the documentation. 279 280You are encouraged to use the following footers when appropriate: 281 282* ``Bug``: Associates this commit with a bug (issue in our `bug tracker`_). The 283 bug will be automatically updated when the change is submitted. When a change 284 is relevant to more than one bug, include multiple ``Bug`` lines, like so: 285 286 .. code-block:: none 287 288 pw_something: Add foo and bar functions 289 290 Bug: 123 291 Bug: 456 292 293* ``Fixed`` or ``Fixes``: Like ``Bug``, but automatically closes the bug when 294 submitted. 295 296 .. code-block:: none 297 298 pw_something: Fix incorrect use of foo 299 300 Fixes: 123 301 302* ``Test``: The author can use this field to tell the reviewer how the change 303 was tested. Typically, this will be some combination of writing new automated 304 tests, running automated tests, and manual testing. 305 306 Note: descriptions of manual testing procedures belong in module 307 documentation, not in the commit message. Use the ``Test`` field to attest 308 that tests were carried out, not to describe the procedures in detail. 309 310 .. code-block:: none 311 312 pw_something: Fix incorrect use of foo 313 314 Test: Added a regression unit test. 315 316In addition, we support all of the `Chromium CQ footers`_, but those are 317relatively rarely useful. 318 319.. _bug tracker: https://bugs.chromium.org/p/pigweed/issues/list 320.. _Chromium CQ footers: https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/infra/cq.md#options 321.. _git footers: https://commondatastorage.googleapis.com/chrome-infra-docs/flat/depot_tools/docs/html/git-footers.html 322