Commit messages in a series of patches

Commit messages in a series of patches

[FMDF]

Hi all,

I've sent a series of 19 patches (co-developed by Pavel Skripkin). Now
it is v7 and we hope it is the latest revision. :)

Few days ago, when it was still at v5, Dan Carpenter reviewed the
whole series and asked us to remove "irrelevant" information from the
commit message of 15/19: [1]

"This relates to the discussion we had about reviewing patches one at a time
in the order they arrive.  Every patch should be self contained.  It
should not refer to the past except in the case of explaining the Fixes
tag and it should not refer to the future except in the case where it
needs to excuse adding unused infrastructure.  Reviewing is stateless.
We don't want to know about your plans.".

I decided that he was right and so I removed that extra information
(they were about plans to remove the function that I'm cleaning up in
the next patch 18/19.

The clean-ups were somewhat necessary because part of the code of the
old function is re-used in new functions introduced in  17/19 and
18/19 and it is then deleted forever, with a strange side effect that
at least one reviewer (David Laight) thought that we were renaming
variables and that renamings should go to separate patches.

However they were _not_ renamings (as I explain above). I thought that
preventive real renaming of the variables that I reuse in the next
patches within _new_ functions would have helped other developers to
review the patches 16-19/19 while avoiding that at a quick read they
could appear as renaming. Actually then David wrote that now the
patches are more easily readable.

In the meantime, Dan C. granted his "Reviewed-by" tag to the first 14
patches of 19. All patches from 2/19 to 12/19 have the following
phrase in the commit messages: "This patch is in preparation for the
_io_ops structure removal.". [2]

My question is: why "This patch is preparation for _io_ops [future]
structure removal." is good while "Eventually this function will be
deleted but some of the code will be reused later." is not.

I'm not really interested in this specific case. I'd like to have
general guidelines that I can understand and use for my future
patches.

Sorry for this long email, I wasn't able to make it shorter :(

Thanks in advance to whoever has more experience than I have and wants
to make this topic clearer to me.

Fabio

[1] https://lore.kernel.org/lkml/20210915135301.GF2088@kadam/
[2] https://lore.kernel.org/lkml/20210915124149.27543-3-fmdefrancesco@gmail.com/

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Re: Commit messages in a series of patches

[Valdis Klētnieks]

[-- Attachment #1.1: Type: text/plain, Size: 914 bytes --]

On Fri, 17 Sep 2021 11:12:45 +0200, FMDF said:

> My question is: why "This patch is preparation for _io_ops [future]
> structure removal." is good while "Eventually this function will be
> deleted but some of the code will be reused later." is not.

The first is specific about what is being changed and why, and tells the
reviewer what to watch for. Also, the reviewer now knows where to look for
justification - there is hopefully a 0/N patch that explains why and how this
structure is being removed.

The second doesn't say which "this function" is being removed, why this is
being done, or whether the changes were internal to the function, or in other
functions.  It also doesn't explain why or how code will be re-used.

The distinction matters, because the biggest point of reviewing is "Does this
commit do what was intended?"  Answering that question is a lot easier when
it's clear what was intended.


[-- Attachment #1.2: Type: application/pgp-signature, Size: 494 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

Fwd: Commit messages in a series of patches

[FMDF]

[-- Attachment #1.1: Type: text/plain, Size: 3680 bytes --]

I'm sorry but, for some reason, the address of kernelnewbies list was
missing in my reply to Valdis. So I had to resend it.

---------- Forwarded message ---------
From: FMDF <fmdefrancesco@gmail.com>
Date: Mon, 20 Sep 2021, 11:13
Subject: Re: Commit messages in a series of patches
To: Valdis Klētnieks <valdis.kletnieks@vt.edu>

On Fri, 17 Sep 2021, 23:58 Valdis Klētnieks, <valdis.kletnieks@vt.edu>
wrote:

> On Fri, 17 Sep 2021 11:12:45 +0200, FMDF said:
>
> > My question is: why "This patch is preparation for _io_ops [future]
> > structure removal." is good while "Eventually this function will be
> > deleted but some of the code will be reused later." is not.
>

My fault. I should have provided more context to you: here I copy-paste the
relevant part of the commit message...

>"Clean up usbctrl_vendoreq () in usb_ops_linux.c. Eventually this function
> will be deleted but some of the code will be reused later. This cleanup
> makes code reuse easier.".

The first is specific about what is being changed


This is about what is being changed: "Clean up usbctrl_vendoreq () in
usb_ops_linux.c".

and why,


"This cleanup makes code reuse easier.".

and tells the
> reviewer what to watch for.


"Eventually this function [it is clear we're still talking about
usbctrl_vendorreq()] will be deleted but some of the code will be reused
later.".

Also, the reviewer now knows where to look for
> justification - there is hopefully a 0/N patch that explains why and how
> this
> structure is being removed.
>

Yes, patch 14/19 delete that _io_ops structure and explains why . In the
same way, 18/19 has the deletion of usbctrl_vendorreq() and the commit
message explains why. That should provide the "why and how this structure
is being removed" (obviously, replace "structure" with "function").

The second doesn't say which "this function" is being removed,


Again, usbctrl_vendorreq(). Please re-read the commit message I pasted here
if you have doubts.

why this is
> being done,


Again, this is explained later in 18/19, exactly how (later) in 14/19 is
also explained why _io_ops structure is removed. I can't see any conceptual
difference, can you?

or whether the changes were internal to the function, or in other
> functions.


It is pretty clear that, when we write ""Eventually this function [it is
clear we're still talking about usbctrl_vendorreq()] will be deleted but
some of the code will be reused later.", we make it clear that the
cleaned-up code cannot be reused in the original function because it is
going to be deleted. Anyway this is not the point: the Reviewer writes that
"[he] is not interested in our future plans", so he asks for the removal of
the phrase "Eventually this function will be deleted...".
Why didn't he ask also for the removal of "This patch is in preparation of
the _io_ops removal"? That is the question: why didn't he say that he is
not also  interested in [future] plans about _io_ops?

It also doesn't explain why or how code will be re-used.
>

Do you mean that I should have mentioned the names of the two new functions
that we create in later patches? I mean those functions that reuse part of
the code of the deleted usbctrl_vendorreq? If so, what about "we are not
interested in your future plans"?

The distinction matters, because the biggest point of reviewing is "Does
> this
> commit do what was intended?"  Answering that question is a lot easier when
> it's clear what was intended.
>

I'm sorry but, after all that has already been said, I am not able to
understand the comment above.

Thanks,

Fabio

[-- Attachment #1.2: Type: text/html, Size: 7040 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Kernelnewbies mailing list
Kernelnewbies@kernelnewbies.org
https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies