[Linux-kernel-mentees] [PATCH] checkpatch: add fixes for BAD_SIGN_OFF

[Aditya]

On 3/11/20 9:04 pm, Lukas Bulwahn wrote:
> On Tue, Nov 3, 2020 at 3:47 PM Aditya <yashsri421 at gmail.com> wrote:
>>
>> On 3/11/20 7:09 pm, Lukas Bulwahn wrote:
>>> On Tue, Nov 3, 2020 at 2:31 PM Aditya Srivastava <yashsri421 at gmail.com> wrote:
>>>>
>>>> Currently, checkpatch warns if the author signs-off two or more times
>>>> for the same role.
>>>> E.g., for commit 6dd47d9754ff ("mac80211: fix
>>>> ieee80211_txq_setup_flows() failure path") we get warning:
>>>>
>>>> WARNING: Duplicate signature
>>>> Signed-off-by: Johannes Berg <johannes.berg at intel.com>
>>>>
>>>> Here there are two Signed-off-by lines present for the same user. So,
>>>> we get a warning to remove one.
>>>>
>>>> Similarly, we get warning if the author of the commit signs-off under
>>>> co-developed-by.
>>>> E.g. for commit 6e88559470f5 ("Documentation: Add section about
>>>> CPU vulnerabilities for Spectre") we get:
>>>>
>>>> WARNING: Co-developed-by: should not be used to attribute nominal
>>>> patch author 'Tim Chen <tim.c.chen at linux.intel.com>'
>>>> Co-developed-by: Tim Chen <tim.c.chen at linux.intel.com>
>>>>
>>>> Provide fixes by removing the duplicate signature line and the
>>>> co-developed-by line from the commit
>>>>
>>>
>>>
>>> The duplicate signature fix is simply wrong, because the indication of
>>> the duplicate signature is imprecise.
>>>
>>> Probably, we would first really need to understand the valid cases of
>>> duplicate signature warnings compared to the invalid ones.
>>>
>>> Please rework.
>>>
>>>
>>> Lukas
>>>
>>
>> Oh, Alright. I get it now.
>> I ran a script over WARNING: Duplicate signature.
>> I found out that it is occurring with these prefixes and their
>> frequencies:
>>
>> Acked-by -- 6
>> Signed-off-by -- 218
>> Tested-by -- 3
>> Reported-by -- 1
>> Reviewed-by -- 8
>> Cc -- 28
>>
>> IMO largely Signed-off-by warnings in this frequency might be false
>> positives. This is so because we are simply checking for duplicate
>> occurrences in the sign-offs in checkpatch.pl
>>
>>> Probably, we would first really need to understand the valid cases
>>> of duplicate signature warnings compared to the invalid ones.
>>>
>>
>> Is there any way I can know that the second sign-off is intended?
>>
>> In my observation, this particular commit was prefixed by '^Link:.*$'
>> Alternatively, we could use maintainers list to ensure that the signer
>> is maintainer and avoid this warning.
>>
> 
> That is somehow difficult to say, but probably it is best to
> deactivate this multiple Signed-off-by: warning on git commits.
> 
> On patches it makes sense to warn about duplicate Signed-off-by: ; on
> commits, not so much.
> 
>> How about other prefixes. Can we remove them?
>>
> 
> Can you show the duplicate Tested-by, Acked-by, Reported-by,
> Reviewed-by, Cc in some structured format to quickly check those?
> 
> Probably for those, we can really assess them to be all true positives
> (maybe we can find out the reasons as well) and hence a fix for them
> would make sense.
> 

I generated git logs of all such commits, which might help us get some
idea about mailing list or sign offs as well.

Acked-by:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/acked_by.txt

Reported-by:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/reported_by.txt

Reviewed by:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/reviewed_by.txt

Tested-by:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/tested_by.txt

Cc: https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task5/cc.txt


>> I observed one of the 'Tested-by' warnings and it was because of these
>> sign offs:
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl at googlemail.com>
>> Tested-by:Liang Yang <liang.yang at amlogic.com>
>> Acked-by: Liang Yang <liang.yang at amlogic.com>
>> Tested-by:Liang Yang <liang.yang at amlogic.com>
>> Acked-by: Liang Yang <liang.yang at amlogic.com>
>> Signed-off-by: Miquel Raynal <miquel.raynal at bootlin.com>
>>
>> Here tested-by is occurring 2 times.
>>
> 
> That example seems to be a clear true positive. It would be
> interesting to see how that happened to be added twice in a commit.
> What was the state on the mailing list and what did the maintainer do
> to add these tags twice (was this done manually or is there some
> automatic tool involved?).
> 

I found the mailing list state corresponding to that day. It indeed
seems like a true positive.

Link1:
https://lore.kernel.org/linux-mtd/CAFBinCB9Z_H4wX3_g2hVL-XnS6Oq-KtPHa4Z1ws79FGp5jCapQ@mail.gmail.com/

Link2 (reply):
https://lore.kernel.org/linux-mtd/8ff232c3-f325-5b91-4de1-a39e63939df2@amlogic.com/

Thanks
Aditya