From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: ** X-Spam-Status: No, score=2.5 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 06E0CC433F5 for ; Mon, 20 Sep 2021 09:22:38 +0000 (UTC) Received: from shelob.surriel.com (shelob.surriel.com [96.67.55.147]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0E02760F56 for ; Mon, 20 Sep 2021 09:22:36 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 0E02760F56 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kernelnewbies.org Received: from localhost ([::1] helo=shelob.surriel.com) by shelob.surriel.com with esmtp (Exim 4.94.2) (envelope-from ) id 1mSFQi-0000PT-VK; Mon, 20 Sep 2021 05:17:40 -0400 Received: from mail-oo1-xc2e.google.com ([2607:f8b0:4864:20::c2e]) by shelob.surriel.com with esmtps (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1mSFQe-0000PN-2j for kernelnewbies@kernelnewbies.org; Mon, 20 Sep 2021 05:17:36 -0400 Received: by mail-oo1-xc2e.google.com with SMTP id g4-20020a4ab044000000b002900bf3b03fso5643476oon.1 for ; Mon, 20 Sep 2021 02:17:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=es8G9ucZ6NzcbMM4m3i7UkDaTIBUrdtUszPJ6sUUKGY=; b=crQirq3Iya2qWdVGZRagSamfzEuCDZ4OSXcX4167kpVgzY9SKiZx17GnSBcnBZHitu Gr5ArmvgtL20LrV+aAHknWTfYie0zO9Uit+O0dtyclmDMHA5TI468kn4zZ92TpWL2I6C kQItqDH6JnwJd614o4FYiWThBSUmOj9reD4OMjXnOyp5YUnVLx5JALhSX3y1gV5Xmvrx njgL7w2rD6M3qkwlBMQx6J4oSSL2SSxt58UkgpOzMXLIHIezGCmyJhym/xIRBXyt7w3S WKjOMVaxWpMzkEfasb6Jwl7fMBVhJZ8wgFNGSQq7TWcjZQmo7djjzYKzeM0QrWU1PIXk CXJg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=es8G9ucZ6NzcbMM4m3i7UkDaTIBUrdtUszPJ6sUUKGY=; b=npfybwQ0xx6ee9uAzGbKOlH0e1cvozuIq4a4qRx6RMZ5MXMgp5oOP9TiZ74DIXYJTw cPcDFzTdSsJr+jw1OqIVd4cAvdOMs9vk7S0ZukvjTSDm1dUrdiUT2Sv+Ah+Pp6M6DPIV lUA9aqAX9W5dD2c9H097fOEIb7DNrtzBBEPKO5Q9bHlWACVb43JGCuVc2kb1g+UrpYLX mIbMI0egkGLvO10oJSPc7uQ9Ixe0I9KnaYCKCyPC4cHvuTH+ZckCry+cylZxnjZueJbI 4yg0YD2bImhsrCZZA8BMh4oSYvgGiljcvwAClFBohHHNqbwZjSfbxunnd/VvU6tMzgUV nxhA== X-Gm-Message-State: AOAM532OJ/3b89nhAQ2VBuUlQD6HP9J90otU1XP97Iz6KbAElkGHKCBE qbHVcOgfOsbT2f4BNRsXpl56t5ZxbzXzupJyEQ4= X-Google-Smtp-Source: ABdhPJz2aN4sWVJR0t6kGDbUOFItdsjrgXaFZXpPYPNE5UhrZ3QpX9aGNTNiYyomYe2zjymuKsn7GnAhtNnbYJct+Bk= X-Received: by 2002:a4a:e0c4:: with SMTP id e4mr6120963oot.24.1632129453286; Mon, 20 Sep 2021 02:17:33 -0700 (PDT) MIME-Version: 1.0 References: <90319.1631915928@turing-police> In-Reply-To: From: FMDF Date: Mon, 20 Sep 2021 11:17:22 +0200 Message-ID: Subject: Fwd: Commit messages in a series of patches To: =?UTF-8?Q?Valdis_Kl=C4=93tnieks?= , kernelnewbies X-BeenThere: kernelnewbies@kernelnewbies.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Learn about the Linux kernel List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: multipart/mixed; boundary="===============0047888473070447074==" Errors-To: kernelnewbies-bounces@kernelnewbies.org --===============0047888473070447074== Content-Type: multipart/alternative; boundary="0000000000004a66e305cc69c0ee" --0000000000004a66e305cc69c0ee Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 Date: Mon, 20 Sep 2021, 11:13 Subject: Re: Commit messages in a series of patches To: Valdis Kl=C4=93tnieks On Fri, 17 Sep 2021, 23:58 Valdis Kl=C4=93tnieks, 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 wh= en > 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 --0000000000004a66e305cc69c0ee Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
I'm sorry but, for some reason, the address of k= ernelnewbies 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=C4=93tnieks <valdis.kletnieks@vt.edu>

On Fri, 17 Sep 2021= , 23:58 Valdis Kl=C4=93tnieks, <valdis.kletnieks@vt.edu> wro= te:
On Fri, 17 Sep 2021 11:12:45 +0= 200, FMDF said:

> My question is: why "This patch is preparation for _io_ops [futur= e]
> structure removal." is good while "Eventually this function = will be
> deleted but some of the code will be reused later." is not.

My fau= lt. I should have provided more context to you: here I copy-paste the relev= ant part of the commit message...

>"Clean up usbctrl_vendoreq () in usb_ops_linux.c. Eventu= ally this function=C2=A0
> will be deleted but so= me of the code will be reused later. This cleanup
&g= t; makes code reuse easier.".

The first is specific about what is being changed
=

This is about what is being c= hanged: "Clean up usbctrl_vendoreq () in usb_ops_linux.c".
<= div dir=3D"auto">
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 u= sbctrl_vendorreq()] will be deleted but some of the code will be reused lat= er.".

Also, the reviewer now k= nows where to look for
justification - there is hopefully a 0/N patch that explains why and how th= is
structure is being removed.
<= br>
Yes, patch 14/19 delete that _io_ops structure a= nd explains why . In the same way, 18/19 has the deletion of usbctrl_vendor= req() and the commit message explains why. That should provide the "wh= y and how this structure is being removed" (obviously, replace "s= tructure" with "function").


Aga= in, 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/1= 9 is also explained why _io_ops structure is removed. I can't see any c= onceptual difference, can you?

or whet= her the changes were internal to the function, or in other
functions.=C2=A0

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 co= de will be reused later.", we make it clear that the cleaned-up code c= annot 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 i= nterested in our future plans", so he asks for the removal of the phra= se "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 a= lso=C2=A0 interested in [future] plans about _io_ops?

It also doesn't explain why or how code will be r= e-used.

Do you mean that I should have mentioned the names of the two new fu= nctions 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 "Do= es this
commit do what was intended?"=C2=A0 Answering that question is a lot e= asier when
it's clear what was intended.

I'm sorry but, after all that has alre= ady been said, I am not able to understand the comment above.

Thanks,

Fabio
--0000000000004a66e305cc69c0ee-- --===============0047888473070447074== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies --===============0047888473070447074==--