<div dir="auto"><div>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.<br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">---------- Forwarded message ---------<br>From: <strong class="gmail_sendername" dir="auto">FMDF</strong> <span dir="auto"><<a href="mailto:fmdefrancesco@gmail.com">fmdefrancesco@gmail.com</a>></span><br>Date: Mon, 20 Sep 2021, 11:13<br>Subject: Re: Commit messages in a series of patches<br>To: Valdis Klētnieks <<a href="mailto:valdis.kletnieks@vt.edu">valdis.kletnieks@vt.edu</a>></div><div dir="auto"><div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, 17 Sep 2021, 23:58 Valdis Klētnieks, <<a href="mailto:valdis.kletnieks@vt.edu" target="_blank" rel="noreferrer">valdis.kletnieks@vt.edu</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Fri, 17 Sep 2021 11:12:45 +0200, FMDF said:<br>
<br>
> My question is: why "This patch is preparation for _io_ops [future]<br>
> structure removal." is good while "Eventually this function will be<br>
> deleted but some of the code will be reused later." is not.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">My fault. I should have provided more context to you: here I copy-paste the relevant part of the commit message...</div><div dir="auto"><br></div><div dir="auto">>"Clean up usbctrl_vendoreq () in usb_ops_linux.c. Eventually this function </div><div dir="auto">> will be deleted but some of the code will be reused later. This cleanup</div><div dir="auto">> makes code reuse easier.".</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The first is specific about what is being changed </blockquote></div></div><div dir="auto"><br></div><div dir="auto">This is about what is being changed: "Clean up usbctrl_vendoreq () in usb_ops_linux.c".</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">and why, </blockquote></div></div><div dir="auto"><br></div><div dir="auto"><div dir="auto" style="font-family:sans-serif">"This cleanup makes code reuse easier.".</div></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">and tells the<br>
reviewer what to watch for. </blockquote></div></div><div dir="auto"><br></div><div dir="auto"><div dir="auto" style="font-family:sans-serif">"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.".</div></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Also, the reviewer now knows where to look for<br>
justification - there is hopefully a 0/N patch that explains why and how this<br>
structure is being removed.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">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").</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The second doesn't say which "this function" is being removed,</blockquote></div></div><div dir="auto"><br></div><div dir="auto">Again, usbctrl_vendorreq(). Please re-read the commit message I pasted here if you have doubts.</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> why this is<br>
being done,</blockquote></div></div><div dir="auto"><br></div><div dir="auto">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?</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> or whether the changes were internal to the function, or in other<br>
functions. </blockquote></div></div><div dir="auto"><br></div><div dir="auto">It is pretty clear that, when we write "<span style="font-family:sans-serif">"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...".</span></div><div dir="auto"><span style="font-family:sans-serif">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?</span></div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> It also doesn't explain why or how code will be re-used.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">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"?</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
The distinction matters, because the biggest point of reviewing is "Does this<br>
commit do what was intended?"  Answering that question is a lot easier when<br>
it's clear what was intended.<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I'm sorry but, after all that has already been said, I am not able to understand the comment above.</div><div dir="auto"><br></div><div dir="auto">Thanks,</div><div dir="auto"><br></div><div dir="auto">Fabio</div></div>
</div></div></div>