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

[Aditya]

On 4/11/20 12:27 pm, Lukas Bulwahn wrote:
> 
> 
> On Tue, 3 Nov 2020, Aditya wrote:
> 
>> 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/
>>
> 
> Yes, and it suggest to fix patchwork :)
> 
> How are your python programming skills?
> 
> Do you want to try fixing patchwork for this issue?
> 
> Lukas
> 

Sure, I'd like to try. Please guide me for it.

Thanks
Aditya