Re: [PATCH 1/7] xz: add fall-through comments to a switch statement

From: Jan Beulich <jbeulich@suse.com>
To: Julien Grall <julien@xen.org>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH 1/7] xz: add fall-through comments to a switch statement
Date: Fri, 26 Nov 2021 10:12:49 +0100	[thread overview]
Message-ID: <0b808ce0-23a2-65ae-dfb3-b167d5565b31@suse.com> (raw)
In-Reply-To: <c2d312c2-c413-4e07-1c0c-8652cab40784@xen.org>

On 26.11.2021 10:03, Julien Grall wrote:
> On 26/11/2021 07:37, Jan Beulich wrote:
>> On 25.11.2021 18:13, Julien Grall wrote:
>>> On 25/11/2021 17:03, Jan Beulich wrote:
>>>> On 25.11.2021 17:54, Julien Grall wrote:
>>>>> On 25/11/2021 16:49, Julien Grall wrote:
>>>>>> On 19/11/2021 10:21, Jan Beulich wrote:
>>>>>>> From: Lasse Collin <lasse.collin@tukaani.org>
>>>>>>>
>>>>>>> It's good style. I was also told that GCC 7 is more strict and might
>>>>>>> give a warning when such comments are missing.
>>>>>>>
>>>>>>> Suggested-by: Andrei Borzenkov <arvidjaar@gmail.com>
>>>>>>> Signed-off-by: Lasse Collin <lasse.collin@tukaani.org>
>>>>>
>>>>> Actually, any reason why there are some signed-off-by missing?
>>>>
>>>> I often keep the author's, but drop ones which clearly got there only
>>>> because of the path a patch has taken through trees.
>>>
>>> This might be clear for you. For me, as a reviewer, I have to do extra
>>> work to check whether you keeped the relevant signed-off-by.
>>>
>>>> These aren't
>>>> relevant imo when pulling over the change;
>>>
>>> They are technically part of the "chain of approval".
>>
>> But the Linux chain of approval is precisely what is of no interest to
>> us. We need to approve the change ourselves; Linux having had it
>> approved is merely a data point.
> 
> I can understand this point of view. But as I wrote above, a reviewer as 
> to do extra work to check you correctly propagated the signed-off-by 
> (see more below).
> 
>>
>>>> I could as well take the
>>>> email submission as my basis, after all, where just the single S-o-b
>>>> would be there.
>>>
>>> That's a fair point. That said, you took the commit-as-is from linus.git
>>
>> How would you be able to tell?
> 
> That's easy. You wrote in your commit message:
> 
> [Linux commit: 5a244f48ecbbd03a11eb84819c5c599db81823ee]
> 
> That's indicating you used the Linux commit rather than the one on the 
> ML. So I will tend to diff the commit and the what's different.

I don't view this as such an indication. I could have taken the submission
and merely have looked up the corresponding commit to provide a reference.

I think our re-using of Linux submissions should be indistinguishable from
their authors, if they were aware of and cared about our cloned code,
submitting their changes separately to xen-devel.

> Anyway, I think it would save time for everyone (you had to manually 
> delete signed-off-by after all) if you just copy the commit (including 
> all the signed-off-by) message as-is.

I don't think I see why you found it necessary to verify the S-o-b set.

Also note that, for things to be useful in our tree, I may also edit
commit messages in mechanical ways (e.g. to change file or function
names). I don't think you can expect a 1:1 match in any event. Review
of such submissions would normally mainly mean making sure that
everything was transformed correctly (besides the question whether the
patch is applicable to us in the first place), not that everything
matches up directly.

Jan