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