Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: fix missing whitespace in formatted email

[Aditya <yashsri421@gmail.com>]

On 18/10/20 1:13 pm, Lukas Bulwahn wrote:
> 
> 
> On Sat, 17 Oct 2020, Aditya wrote:
> 
>>
>> On Wed, Oct 14, 2020 at 10:33 PM Aditya <yashsri421@gmail.com> wrote:
>>
>>>
>>> On 13/10/20 11:46 am, Lukas Bulwahn wrote:
>>>>
>>>> On Tue, 13 Oct 2020, Dwaipayan Ray wrote:
>>>>
>>>>> Commit 0c01921e56f9 ("checkpatch: add new warnings to author signoff
>>>>> checks.") introduced new checks for author sign off. The format_email
>>>>> procedure was modified to add comment blocks to the formatted email. But
>>>>> no space was added between the email address and mail comment.
>>>>>
>>>>> This causes wrong email formatting in cases where the email is in the 
>>> form
>>>>> "author@example.com (Comment block)". The space between the address and
>>>>> comment block is removed, which causes incorrect parsing of the
>>>>> formatted email.
>>>>>
>>>>> An evaluation on checkpatch brought up this case. For example,
>>>>> on commit 1129d31b55d5 ("ima: Fix ima digest hash table key 
>>> calculation"),
>>>>> the following warning was reported:
>>>>>
>>>>> WARNING:BAD_SIGN_OFF: email address 'David.Laight@aculab.com (big 
>>> endian
>>>>> system concerns)' might be better as 'David.Laight@aculab.com(big 
>>> endian
>>>>> system concerns)'
>>>>>
>>>>> Add a single space in between the address and comment when the
>>>>> extracted comment is not empty.
>>>>>
>>>> Dwaipayan, looks good to me.
>>>>
>>>> So, how about a 'Fixes:' tag?
>>>>
>>>> Aditya, can you rerun your evaluation with this fix patch applied on top?
>>>>
>>>> Then, we need a comparison for:
>>>> 1. completely before vs. after the two patches, and
>>>> 2. after the first patch vs. after the two patches (to see that the fix 
>>>> works)
>>>>
>>>> More support on evaluation from others interested to become mentees are 
>>> of 
>>>> course welcome.
>>>>
>>>> Lukas
>>>>  
>>>>> Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
>>>>> ---
>>>>>  scripts/checkpatch.pl | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>>>> index fab38b493cef..f1a4e61917eb 100755
>>>>> --- a/scripts/checkpatch.pl
>>>>> +++ b/scripts/checkpatch.pl
>>>>> @@ -1221,7 +1221,9 @@ sub format_email {
>>>>>      } else {
>>>>>              $formatted_email = "$name$name_comment <$address>";
>>>>>      }
>>>>> -    $formatted_email .= "$comment";
>>>>> +    if ("$comment" ne "") {
>>>>> +            $formatted_email .= " $comment";
>>>>> +    }
>>>>>      return $formatted_email;
>>>>>  }
>>>>>  
>>>>> -- 
>>>>> 2.27.0
>>>>>
>>>>>
>>> Hi Sir
>>>
>>> I have analyzed the reports using this patch and made comparison. The 
>>> links are as follows:
>>>
>>> 1. completely before vs. after this patch: 
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task2/reports/analysis/relative_change/before_v_second/summary_relative.txt
>>> 2. after the first patch vs. after the two patches: 
>>> https://github.com/AdityaSrivast/kernel-tasks/blob/master/Task2/reports/analysis/relative_change/first_v_second/summary_relative.txt
>>>
>>> Kindly let me know if you have any questions.
>>>
>>> Aditya
>>>
>>>
>>
>> Hi Sir
>> I have completed all the assigned tasks. What should be my next steps to participate as a mentee in the community bridge program?
>> Kindly guide me regarding it.
>>
> 
> (your email client is broken; it should not spread lines so long...)
> 
> Aditya, you are on a good way to be towards acceptance for a kernel 
> mentorship.
> 
> If I am not mistaken, you have not created yet a non-trivial patch to 
> checkpatch.pl and got your patch accepted.
> 
> 
> Look at your own checkpatch.pl evaluation, find a relevant case where 
> checkpatch.pl reports a large class of false positives and think about how 
> to improve checkpatch.pl.
> 
> Especially, when looking at checkpatch.pl complaining about suspicious 
> code patterns or style and formatting, you might want to compare to the 
> capabilities of compiler static analysers (clang-tidy) or clang-format.
> 
> Improving those tools, making them the warning-free for the specific 
> class and supporting to make them default tools might also be a good 
> extension to the goals of checkpatch.
> 
> Let us know which kind of false positive class you found and would like to 
> improve.
> 
> Dwaipayan and I can help if you really do not find anything at all...
> 
> Good luck :)
> 
> 
> Lukas
> 

Hi Sir,
I observed the reports as you suggested.
I found out that "REPEATED_WORD" is the third most frequent warning in
v6..v8 with frequency of 2797. Although this does give correct
messages most of the times, but a large proportion(probably more than
50%) of this error comes out of hexadecimal being a part of the commit
or code.
For eg, WARNING:REPEATED_WORD: Possible repeated word: 'ff'
#21:
Code: 00 77 27 48 81 ff 00 00 01 00 76 07 0f b7 d7 ed 0f c8 c3 55 48
c7 c6 3b ee d5 9f 48 89 e5 e8 67 fc ff ff b8 ff ff ff ff 5d c3 <8b> 07
0f c8 c3 66 66 2e 0f 1f 84 00 00 00 00 00 48 81 fe ff ff 03

Now, here it reports 'ff' to be repeated many times. But I believe
that this was not the expected outcome from this warning.

I think that such warnings should be avoided from this class tag of
warning.

My approach: Use regex to know if the repeated words are of '2' length
and occur among such pattern which consists of 2 lettered words
repeated 5-6 times,ie. size(/[a-f0-9]{2}\s/)>6, eg. "04 19 00 00 c3 48
83 ec 08 e8 0a fa ff ff 48 89 04 24 b8 02 00 00 00 0f".
As normally such occurrences do not occur in English, it can be an
effective method to avoid such hexadecimal occurrences.

Kindly let me know if I can proceed with this. Also, please suggest if
there is a better way to counter this issue.

Thank You
Sincerely
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees