Re: [Linux-kernel-mentees] [PATCH] checkpatch: fix author Signed-off-by warning for split From: header

[Dwaipayan Ray <dwaipayanray1@gmail.com>]

On Fri, Sep 18, 2020 at 6:28 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>
>
> For the patch subject line, I actually think this patch is a new feature
> or extension, not a fix. It was not broken, just not supported before.
>
> So maybe: extend author Signed-off-by check for split From: header
>
>
> On Fri, 18 Sep 2020, Dwaipayan Ray wrote:
>
> > Checkpatch did not handle cases where the author From: header was
> > split into two lines. In those cases the author string went empty,
> > and checkpatch generated a false missing author signed-off-by
> > warning.
> >
> > This patch adds support for split From: headers and resolves those
> > false warnings.
> >
>
> You can drop 'This patch adds'. We see it is a patch, and we see that it
> adds something. Just use imperative tense:
>
> Support split From: headers in AUTHOR_SIGN_OFF check.
>
> (That is a good commit message header as well.)
>
> Can you provide some statistics on number of warnings before and after
> and maybe even in more detail, how many of the warnings disappeared with:
>
>   Missing Signed-off-by: line by nominal patch author ''
>
> Probably even new warnings appeared?
>
> > Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> > ---
> >  scripts/checkpatch.pl | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 504d2e431c60..8c4119ca7d17 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2347,6 +2347,7 @@ sub process {
> >       my $signoff = 0;
> >       my $author = '';
> >       my $authorsignoff = 0;
> > +     my $prevheader = 0;
> >       my $is_patch = 0;
> >       my $is_binding_patch = -1;
> >       my $in_header_lines = $file ? 0 : 1;
> > @@ -2658,12 +2659,22 @@ sub process {
> >                       }
> >               }
> >
> > +# Check the patch for a split From:
> > +             if ($author eq '' && decode("MIME-Header", $prevheader) =~ /^From:\s*(.*)/) {
>
> How about extending to check if $prevheader is not 0?
>
> > +                     $author = $1.$line;
> > +                     $author = encode("utf8", $author) if ($prevheader =~ /=\?utf-8\?/i);
> > +                     $author =~ s/"//g;
> > +                     $author = reformat_email($author);
> > +                     $prevheader = '';
> > +             }
> > +
> >  # Check the patch for a From:
> >               if (decode("MIME-Header", $line) =~ /^From:\s*(.*)/) {
> >                       $author = $1;
> >                       $author = encode("utf8", $author) if ($line =~ /=\?utf-8\?/i);
> >                       $author =~ s/"//g;
> >                       $author = reformat_email($author);
> > +                     $prevheader = $line;
> >               }
> >
>
> So here we see two almost identical parts of code now, right?
>
> Either use a small function or restructure the code such that the
> differences are in two branches and the common code is part of one common
> control flow. You are a good programmer, you can figure this out.
>
> Generally looks good. Let us know once you think it is ready to be tested :)
>
> We got some checkpatch.pl evaluation experts here and I am sure they are
> all happy to test your change and see the evaluation get better.
>
> Lukas

Hi,
I will get back with the changes you told once I do it in a clean enough way.

For now, I have generated the statistics for my patch. I ran my script on
all non merge commits between v5.7 and v5.8.

Before applying patch, no. of warnings:
NO_AUTHOR_SIGN_OFF: 278

After applying the patch:
NO_AUTHOR_SIGN_OFF: 251

Comparing the changes before and after applying the patch:
(Entries are given as no. of warnings of type NO_AUTHOR_SIGN_OFF
before and after applying the patch for given commit hash).

e33bcbab16d1: 1-> 0
6a5d6fd33262: 1-> 0
424c85e1ffea: 1-> 0
c7ff09f6e262: 1-> 0
148dd20602d5: 1-> 0
d5f74a1eff9a: 1-> 0
6c47660e3c3a: 1-> 0
9d9cc58aff46: 1-> 0
9a618e6f8cdd: 1-> 0
980f91778a2f: 1-> 0
b77da87c84f8: 1-> 0
15e3ae36f71e: 1-> 0
c5b4312bea5d: 1-> 0
41aef04524d3: 1-> 0
37d1e94692e0: 1-> 0
79eb8c7f015a: 1-> 0
8211d1e83ade: 1-> 0
9a42a5ff3dac: 1-> 0
440d7a6f7390: 1-> 0
6b6aeffc932d: 1-> 0
ce1d86dc9249: 1-> 0
f645e6256bd1: 1-> 0
770ae40cd6d2: 1-> 0
f4d12d8009d9: 1-> 0
f0a087a533b3: 1-> 0
772563b27c9f: 1-> 0
b03628b73564: 1-> 0

So, no new warnings have popped up, and the no. of warnings have
reduced significantly. :)

I will send in the new patch once I am done with it.

Thanks,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees