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

[Lukas Bulwahn]

On Sat, 17 Oct 2020, Aditya wrote:

> 
> On Wed, Oct 14, 2020 at 10:33 PM Aditya <yashsri421 at 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 at 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 at aculab.com (big 
> > endian
> > >> system concerns)' might be better as 'David.Laight at 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 at 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