Re: [PATCH v4 15/18] staging: r8188eu: hal: Clean up usbctrl_vendorreq()

[Fabio M. De Francesco]

On Tuesday, September 14, 2021 11:24:05 AM CEST Dan Carpenter wrote:
> On Mon, Sep 13, 2021 at 08:09:59PM +0200, Fabio M. De Francesco wrote:
> > Clean up usbctrl_vendorreq () in usb_ops_linux.c because some
> > of its code will be reused in this series. This cleanup is in
> > preparation for shortening the call chains of rtw_read{8,16,32}()
> > and rtw_write{8,16,32,N}(). More insights about the reasons why at
> > https://lore.kernel.org/lkml/5319192.FrU0QrjFp7@localhost.localdomain/
> > 
> 
> This commit message is quite bad.
> 
> This patch has nothing to do with reusing the code or shortening call
> chains.

It has to do, in a certain sense. Let me explain please...

Some days ago, David Laight made the review of "Shorten calls chain of 
rtw_write8/16/32/n()" version 3. 

In that patch he noticed some lines of usb_read() that I had created with the 
help of reusing some lines of the code of usbctrl_vendorreq() that is deleted 
in the same patch. 

He thought that they were clean-ups and renames and so he suggested to make 
those "clean-ups" in a separate patch.

However they were _not_ renames or other clean-ups, because usb_read() was 
not touched in that patch and, above all, it was a new function. 

I am sure that when I write new functions I can use whatever name of 
variables I like, even if people may think I'm renaming the variables that 
were in a old function that now is deleted. Am I not permitted?

However, because I also think that readability of the diffs matters, I 
decided to do some clean-up of the code I'm about to reuse in the new 
functions. It improves readability of the above-mentioned patch that is also 
the 18/18 of this series.

That is the reason why I'm cleaning up a function that is going to be deleted 
in the last patch of the series.

> Don't use a link like that in the commit message especially when it's a
> link to an email you wrote.  If it's someone else's email you can say,
> something like "As <name> points out in <his/her> email <url>.  Blah
> blah blah."  That way you give credit to the other person but all the
> information is in the commit message.

I agree with you. I'll redo the commit message for in order to summarize in 
few lines why I'm doing clean-ups of functions that must be deleted in 18/18.
The same for 16/18. I think that a short explanation like the one that I gave 
you above should suffice (much shorter, obviously).

I hope that I've been clear now. Please let me know if you have more 
suggestions about this patch and the next (16/18).

Regards,

Fabio