* Commit messages in a series of patches @ 2021-09-17 9:12 FMDF 2021-09-17 21:58 ` Valdis Klētnieks 0 siblings, 1 reply; 3+ messages in thread From: FMDF @ 2021-09-17 9:12 UTC (permalink / raw) To: kernelnewbies 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: Commit messages in a series of patches 2021-09-17 9:12 Commit messages in a series of patches FMDF @ 2021-09-17 21:58 ` Valdis Klētnieks [not found] ` <CAPj211ufxfjritLEc8n11=OLXFFjGSAMacvgOAb2usOeS0LEwg@mail.gmail.com> 0 siblings, 1 reply; 3+ messages in thread From: Valdis Klētnieks @ 2021-09-17 21:58 UTC (permalink / raw) To: FMDF; +Cc: kernelnewbies [-- 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
[parent not found: <CAPj211ufxfjritLEc8n11=OLXFFjGSAMacvgOAb2usOeS0LEwg@mail.gmail.com>]
* Fwd: Commit messages in a series of patches [not found] ` <CAPj211ufxfjritLEc8n11=OLXFFjGSAMacvgOAb2usOeS0LEwg@mail.gmail.com> @ 2021-09-20 9:17 ` FMDF 0 siblings, 0 replies; 3+ messages in thread From: FMDF @ 2021-09-20 9:17 UTC (permalink / raw) To: Valdis Klētnieks, kernelnewbies [-- 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-09-20 9:22 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-09-17 9:12 Commit messages in a series of patches FMDF
2021-09-17 21:58 ` Valdis Klētnieks
[not found] ` <CAPj211ufxfjritLEc8n11=OLXFFjGSAMacvgOAb2usOeS0LEwg@mail.gmail.com>
2021-09-20 9:17 ` Fwd: " FMDF
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).