xref: /aosp_15_r20/external/pigweed/docs/style/commit_message.rst (revision 61c4878ac05f98d0ceed94b57d316916de578985)
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