Fwd: Commit messages in a series of patches

[FMDF <fmdefrancesco@gmail.com>]

[-- 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