[PATCH RFC 0/2] docs: process: submitting-patches: clarify imperative mood suggestion

[PATCH RFC 0/2] docs: process: submitting-patches: clarify imperative mood suggestion

[Ahmad Fatoum]

Many commit message bodies start off with some background information,
before explaining how they address the situation. This can be arguably
easier to follow than having the imperative in the commit message title
be followed directly by another differently worded or more verbose
imperative in the commit message body and then at the end an
", because ..." with an explanation why things were done this way.

Yet, while the documentation talks about use of imperative mood, it does
not fully explain why, which IMO makes it prone to misunderstanding[1][2].

Therefore adapt the documentation to clarify the intent of the imperative
mood and give an example for how a good commit message can look like.

[1]: https://lore.kernel.org/all/f085aa33-f0b7-49e7-bbfc-d3728d3e3e8c@pengutronix.de/#t
[2]: https://lore.kernel.org/all/Z2RzA5S%2Fch1YDdUD@lizhi-Precision-Tower-5810/

---
Ahmad Fatoum (2):
      docs: process: submitting-patches: split canonical patch format section
      docs: process: submitting-patches: clarify imperative mood suggestion

 Documentation/process/submitting-patches.rst | 74 +++++++++++++++++++---------
 1 file changed, 52 insertions(+), 22 deletions(-)
---
base-commit: 78d4f34e2115b517bcbfe7ec0d018bbbb6f9b0b8
change-id: 20241219-submitting-patches-imperative-248413781db1

Best regards,
-- 
Ahmad Fatoum <a.fatoum@pengutronix.de>

[PATCH RFC 1/2] docs: process: submitting-patches: split canonical patch format section

[Ahmad Fatoum]

To make it easier to reference specific parts of the patch format,
let's add some headings for different parts.

Doing that, it becomes clear that backtraces in commit message is out of
place being after Reply-To Headers, so move it next to the commit
message body subsubsection.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 Documentation/process/submitting-patches.rst | 56 +++++++++++++++++-----------
 1 file changed, 34 insertions(+), 22 deletions(-)

diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index 1518bd57adab501f7a7cf2fdfb9ac3f3a870766e..1474c84b3ac562f5806d85e8ef5b6e9d23572e59 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -610,6 +610,9 @@ that, if you have your patches stored in a ``git`` repository, proper patch
 formatting can be had with ``git format-patch``.  The tools cannot create
 the necessary text, though, so read the instructions below anyway.
 
+Subject Line
+^^^^^^^^^^^^
+
 The canonical patch subject line is::
 
     Subject: [PATCH 001/123] subsystem: summary phrase
@@ -683,6 +686,9 @@ Here are some good example Subjects::
     Subject: [PATCH v2] sub/sys: Condensed patch summary
     Subject: [PATCH v2 M/N] sub/sys: Condensed patch summary
 
+From Line
+^^^^^^^^^
+
 The ``from`` line must be the very first line in the message body,
 and has the form:
 
@@ -693,6 +699,9 @@ patch in the permanent changelog.  If the ``from`` line is missing,
 then the ``From:`` line from the email header will be used to determine
 the patch author in the changelog.
 
+Explanation Body
+^^^^^^^^^^^^^^^^
+
 The explanation body will be committed to the permanent source
 changelog, so should make sense to a competent reader who has long since
 forgotten the immediate details of the discussion that might have led to
@@ -708,6 +717,31 @@ _all_ of the compile failures; just enough that it is likely that
 someone searching for the patch can find it. As in the ``summary
 phrase``, it is important to be both succinct as well as descriptive.
 
+.. _backtraces:
+
+Backtraces in commit messages
+"""""""""""""""""""""""""""""
+
+Backtraces help document the call chain leading to a problem. However,
+not all backtraces are helpful. For example, early boot call chains are
+unique and obvious. Copying the full dmesg output verbatim, however,
+adds distracting information like timestamps, module lists, register and
+stack dumps.
+
+Therefore, the most useful backtraces should distill the relevant
+information from the dump, which makes it easier to focus on the real
+issue. Here is an example of a well-trimmed backtrace::
+
+  unchecked MSR access error: WRMSR to 0xd51 (tried to write 0x0000000000000064)
+  at rIP: 0xffffffffae059994 (native_write_msr+0x4/0x20)
+  Call Trace:
+  mba_wrmsr
+  update_domains
+  rdtgroup_mkdir
+
+Commentary
+^^^^^^^^^^
+
 The ``---`` marker line serves the essential purpose of marking for
 patch handling tools where the changelog message ends.
 
@@ -746,28 +780,6 @@ patch::
 See more details on the proper patch format in the following
 references.
 
-.. _backtraces:
-
-Backtraces in commit messages
-^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-
-Backtraces help document the call chain leading to a problem. However,
-not all backtraces are helpful. For example, early boot call chains are
-unique and obvious. Copying the full dmesg output verbatim, however,
-adds distracting information like timestamps, module lists, register and
-stack dumps.
-
-Therefore, the most useful backtraces should distill the relevant
-information from the dump, which makes it easier to focus on the real
-issue. Here is an example of a well-trimmed backtrace::
-
-  unchecked MSR access error: WRMSR to 0xd51 (tried to write 0x0000000000000064)
-  at rIP: 0xffffffffae059994 (native_write_msr+0x4/0x20)
-  Call Trace:
-  mba_wrmsr
-  update_domains
-  rdtgroup_mkdir
-
 .. _explicit_in_reply_to:
 
 Explicit In-Reply-To headers

-- 
2.39.5

Re: [PATCH RFC 1/2] docs: process: submitting-patches: split canonical patch format section

[Jonathan Corbet]

Ahmad Fatoum <a.fatoum@pengutronix.de> writes:

> To make it easier to reference specific parts of the patch format,
> let's add some headings for different parts.
>
> Doing that, it becomes clear that backtraces in commit message is out of
> place being after Reply-To Headers, so move it next to the commit
> message body subsubsection.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  Documentation/process/submitting-patches.rst | 56 +++++++++++++++++-----------
>  1 file changed, 34 insertions(+), 22 deletions(-)

This seems like an improvement - I've applied it, thanks.

jon

[PATCH RFC 2/2] docs: process: submitting-patches: clarify imperative mood suggestion

[Ahmad Fatoum]

While we expect commit message titles to use the imperative mood,
it's ok for commit message bodies to first include a blurb describing
the background of the patch, before delving into what's being done
to address the situation.

Make this clearer by adding a clarification after the imperative mood
suggestion as well as listing Rob Herring's commit 52bb69be6790
("dt-bindings: ata: pata-common: Add missing additionalProperties on
child nodes") as a good example commit message.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
I liked Rob's commit message, because:

  - It has multiple subsystem prefixes
  - Uses imperative mood in the commit message title
  - Doesn't use it in the commit message body showing that it's
    not a hard requirement
  - Is short and gives a succinct background
  - Explains not only why to do the change, but also why it's ok
    to do it

Admittedly though, a C example may be easier to grok for a general
audience. I can search for one if that's preferred (or maybe someone
has a suitable commit already they wish to suggest?)
---
 Documentation/process/submitting-patches.rst | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/process/submitting-patches.rst b/Documentation/process/submitting-patches.rst
index 1474c84b3ac562f5806d85e8ef5b6e9d23572e59..b10534e460aa30f2751208bd1eca242841ba5edb 100644
--- a/Documentation/process/submitting-patches.rst
+++ b/Documentation/process/submitting-patches.rst
@@ -96,6 +96,11 @@ instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
 to do frotz", as if you are giving orders to the codebase to change
 its behaviour.
 
+The goal of the imperative mood is to make especially commit message
+titles (the :ref:`patch_subject_line`) succinct and to the point.
+The explanation body should be more detailed and take care to explain
+the background motivating the change. (see :ref:`patch_explanation_body`).
+
 If you want to refer to a specific commit, don't just refer to the
 SHA-1 ID of the commit. Please also include the oneline summary of
 the commit, to make it easier for reviewers to know what it is about.
@@ -610,6 +615,8 @@ that, if you have your patches stored in a ``git`` repository, proper patch
 formatting can be had with ``git format-patch``.  The tools cannot create
 the necessary text, though, so read the instructions below anyway.
 
+.. _patch_subject_line:
+
 Subject Line
 ^^^^^^^^^^^^
 
@@ -699,6 +706,8 @@ patch in the permanent changelog.  If the ``from`` line is missing,
 then the ``From:`` line from the email header will be used to determine
 the patch author in the changelog.
 
+.. _patch_explanation_body:
+
 Explanation Body
 ^^^^^^^^^^^^^^^^
 
@@ -717,6 +726,15 @@ _all_ of the compile failures; just enough that it is likely that
 someone searching for the patch can find it. As in the ``summary
 phrase``, it is important to be both succinct as well as descriptive.
 
+Here is one example of a good commit message::
+
+  dt-bindings: ata: pata-common: Add missing additionalProperties on child nodes
+
+  The PATA child node schema is missing constraints to prevent unknown
+  properties. As none of the users of this common binding extend the child
+  nodes with additional properties, adding "additionalProperties: false"
+  here is sufficient.
+
 .. _backtraces:
 
 Backtraces in commit messages

-- 
2.39.5

Re: [PATCH RFC 2/2] docs: process: submitting-patches: clarify imperative mood suggestion

[Jonathan Corbet]

Ahmad Fatoum <a.fatoum@pengutronix.de> writes:

> While we expect commit message titles to use the imperative mood,
> it's ok for commit message bodies to first include a blurb describing
> the background of the patch, before delving into what's being done
> to address the situation.
>
> Make this clearer by adding a clarification after the imperative mood
> suggestion as well as listing Rob Herring's commit 52bb69be6790
> ("dt-bindings: ata: pata-common: Add missing additionalProperties on
> child nodes") as a good example commit message.
>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

I'm rather less convinced about this one.  We already have a whole
section on describing changes.  Given that this crucial document is
already long and hard enough to get through, I don't really think that
adding some duplicate information - and the noise of more labels - is
going to improve things.

Thanks,

jon

Re: [PATCH RFC 2/2] docs: process: submitting-patches: clarify imperative mood suggestion

[Ahmad Fatoum]

Hello Jon,

On 30.12.24 19:40, Jonathan Corbet wrote:
> Ahmad Fatoum <a.fatoum@pengutronix.de> writes:
> 
>> While we expect commit message titles to use the imperative mood,
>> it's ok for commit message bodies to first include a blurb describing
>> the background of the patch, before delving into what's being done
>> to address the situation.
>>
>> Make this clearer by adding a clarification after the imperative mood
>> suggestion as well as listing Rob Herring's commit 52bb69be6790
>> ("dt-bindings: ata: pata-common: Add missing additionalProperties on
>> child nodes") as a good example commit message.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> 
> I'm rather less convinced about this one.  We already have a whole
> section on describing changes.  Given that this crucial document is
> already long and hard enough to get through, I don't really think that
> adding some duplicate information - and the noise of more labels - is
> going to improve things.

Do you agree with the content of the patch in principle?

My changes were motivated by a disagreement about the necessity of having
to use the imperative mood throughout as I described in my cover letter,
so I still think think that a clarification is appropriate.

Would a v2 without the example at the end be acceptable?

Thanks,
Ahmad




> 
> Thanks,
> 
> jon
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH RFC 2/2] docs: process: submitting-patches: clarify imperative mood suggestion

[Jonathan Corbet]

Ahmad Fatoum <a.fatoum@pengutronix.de> writes:

> Hello Jon,
>
> On 30.12.24 19:40, Jonathan Corbet wrote:
>> Ahmad Fatoum <a.fatoum@pengutronix.de> writes:
>> 
>>> While we expect commit message titles to use the imperative mood,
>>> it's ok for commit message bodies to first include a blurb describing
>>> the background of the patch, before delving into what's being done
>>> to address the situation.
>>>
>>> Make this clearer by adding a clarification after the imperative mood
>>> suggestion as well as listing Rob Herring's commit 52bb69be6790
>>> ("dt-bindings: ata: pata-common: Add missing additionalProperties on
>>> child nodes") as a good example commit message.
>>>
>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> 
>> I'm rather less convinced about this one.  We already have a whole
>> section on describing changes.  Given that this crucial document is
>> already long and hard enough to get through, I don't really think that
>> adding some duplicate information - and the noise of more labels - is
>> going to improve things.
>
> Do you agree with the content of the patch in principle?
>
> My changes were motivated by a disagreement about the necessity of having
> to use the imperative mood throughout as I described in my cover letter,
> so I still think think that a clarification is appropriate.
>
> Would a v2 without the example at the end be acceptable?

I will always consider a patch, but the example isn't the concern,
really.  The information you are trying to add to an already too-long
document is already present there; I think that repeating it, and making
this crucial document that much more unapproachable, would actively make
things worse.

Thanks,

jon

Re: [PATCH RFC 2/2] docs: process: submitting-patches: clarify imperative mood suggestion

[Ahmad Fatoum]

Hello Jon,

On 06.01.25 15:57, Jonathan Corbet wrote:
> Ahmad Fatoum <a.fatoum@pengutronix.de> writes:
> 
>> Hello Jon,
>>
>> On 30.12.24 19:40, Jonathan Corbet wrote:
>>> Ahmad Fatoum <a.fatoum@pengutronix.de> writes:
>>>
>>>> While we expect commit message titles to use the imperative mood,
>>>> it's ok for commit message bodies to first include a blurb describing
>>>> the background of the patch, before delving into what's being done
>>>> to address the situation.
>>>>
>>>> Make this clearer by adding a clarification after the imperative mood
>>>> suggestion as well as listing Rob Herring's commit 52bb69be6790
>>>> ("dt-bindings: ata: pata-common: Add missing additionalProperties on
>>>> child nodes") as a good example commit message.
>>>>
>>>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>>>
>>> I'm rather less convinced about this one.  We already have a whole
>>> section on describing changes.  Given that this crucial document is
>>> already long and hard enough to get through, I don't really think that
>>> adding some duplicate information - and the noise of more labels - is
>>> going to improve things.
>>
>> Do you agree with the content of the patch in principle?
>>
>> My changes were motivated by a disagreement about the necessity of having
>> to use the imperative mood throughout as I described in my cover letter,
>> so I still think think that a clarification is appropriate.
>>
>> Would a v2 without the example at the end be acceptable?
> 
> I will always consider a patch, but the example isn't the concern,
> really.  The information you are trying to add to an already too-long
> document is already present there; I think that repeating it, and making
> this crucial document that much more unapproachable, would actively make
> things worse.

Ok, thanks for the prompt response.

Cheers,
Ahmad

> 
> Thanks,
> 
> jon
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |