* [Linux-kernel-mentees] [PATCH] checkpatch: fix author Signed-off-by warning for split From: header @ 2020-09-18 12:29 Dwaipayan Ray 2020-09-18 12:58 ` Lukas Bulwahn 0 siblings, 1 reply; 5+ messages in thread From: Dwaipayan Ray @ 2020-09-18 12:29 UTC (permalink / raw) To: lukas.bulwahn; +Cc: dwaipayanray1, linux-kernel-mentees 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. 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*(.*)/) { + $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; } # Check the patch for a signoff: -- 2.27.0 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: fix author Signed-off-by warning for split From: header 2020-09-18 12:29 [Linux-kernel-mentees] [PATCH] checkpatch: fix author Signed-off-by warning for split From: header Dwaipayan Ray @ 2020-09-18 12:58 ` Lukas Bulwahn 2020-09-18 15:05 ` Dwaipayan Ray 2020-09-19 5:57 ` Dwaipayan Ray 0 siblings, 2 replies; 5+ messages in thread From: Lukas Bulwahn @ 2020-09-18 12:58 UTC (permalink / raw) To: Dwaipayan Ray; +Cc: linux-kernel-mentees 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 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: fix author Signed-off-by warning for split From: header 2020-09-18 12:58 ` Lukas Bulwahn @ 2020-09-18 15:05 ` Dwaipayan Ray 2020-09-19 5:57 ` Dwaipayan Ray 1 sibling, 0 replies; 5+ messages in thread From: Dwaipayan Ray @ 2020-09-18 15:05 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: linux-kernel-mentees 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: fix author Signed-off-by warning for split From: header 2020-09-18 12:58 ` Lukas Bulwahn 2020-09-18 15:05 ` Dwaipayan Ray @ 2020-09-19 5:57 ` Dwaipayan Ray 2020-09-19 7:25 ` Lukas Bulwahn 1 sibling, 1 reply; 5+ messages in thread From: Dwaipayan Ray @ 2020-09-19 5:57 UTC (permalink / raw) To: Lukas Bulwahn; +Cc: linux-kernel-mentees > > 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. > Hi, I have changed the structure around a bit. diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 504d2e431c60..86975baead22 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1210,6 +1210,16 @@ sub reformat_email { return format_email($email_name, $email_address); } +sub format_author_email { + my ($email, $from) = @_; + + $email = encode("utf8", $email) if ($from =~ /=\?utf-8\?/i); + $email =~ s/"//g; + $email = reformat_email($email); + + return $email; +} + sub same_email_addresses { my ($email1, $email2) = @_; @@ -2347,6 +2357,7 @@ sub process { my $signoff = 0; my $author = ''; my $authorsignoff = 0; + my $prevheader = ''; my $is_patch = 0; my $is_binding_patch = -1; my $in_header_lines = $file ? 0 : 1; @@ -2658,12 +2669,21 @@ sub process { } } +# Check the patch for a split From: + if($prevheader ne '') { + if ($author eq '' && decode("MIME-Header", $prevheader) =~ /^From:\s*(.*)/) { + my $email = $1.$line; + $author = format_author_email($email, $prevheader); + } + $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); + $author = format_author_email($1, $line); + if($author eq '') { + $prevheader = $line; + } } # Check the patch for a signoff: Is it good to go? I shall mail it in then. Thanks, Dwaipayan. _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH] checkpatch: fix author Signed-off-by warning for split From: header 2020-09-19 5:57 ` Dwaipayan Ray @ 2020-09-19 7:25 ` Lukas Bulwahn 0 siblings, 0 replies; 5+ messages in thread From: Lukas Bulwahn @ 2020-09-19 7:25 UTC (permalink / raw) To: Dwaipayan Ray; +Cc: linux-kernel-mentees On Sat, 19 Sep 2020, Dwaipayan Ray wrote: > > > 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. > > > > Hi, > I have changed the structure around a bit. > Next time, please just send a proper PATCH v2 when you rework a patch. > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 504d2e431c60..86975baead22 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -1210,6 +1210,16 @@ sub reformat_email { > return format_email($email_name, $email_address); > } > > +sub format_author_email { > + my ($email, $from) = @_; > + > + $email = encode("utf8", $email) if ($from =~ /=\?utf-8\?/i); > + $email =~ s/"//g; > + $email = reformat_email($email); > + > + return $email; > +} > + > sub same_email_addresses { > my ($email1, $email2) = @_; > > @@ -2347,6 +2357,7 @@ sub process { > my $signoff = 0; > my $author = ''; > my $authorsignoff = 0; > + my $prevheader = ''; > my $is_patch = 0; > my $is_binding_patch = -1; > my $in_header_lines = $file ? 0 : 1; > @@ -2658,12 +2669,21 @@ sub process { > } > } > > +# Check the patch for a split From: > + if($prevheader ne '') { > + if ($author eq '' && decode("MIME-Header", $prevheader) =~ > /^From:\s*(.*)/) { > + my $email = $1.$line; > + $author = format_author_email($email, $prevheader); > + } > + $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); > + $author = format_author_email($1, $line); > + if($author eq '') { > + $prevheader = $line; > + } > } > > # Check the patch for a signoff: > > Is it good to go? I shall mail it in then. > Yes, I think it is good for a first submission. Please use ./scripts/get_maintainers.pl to find the developers to send this patch to. Also CC: me and the linux-kernel-mentees mailing list. Once, the list is on the linux-kernel mailing list, we will start reviewing and testing your patch. Lukas _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-19 7:25 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-18 12:29 [Linux-kernel-mentees] [PATCH] checkpatch: fix author Signed-off-by warning for split From: header Dwaipayan Ray 2020-09-18 12:58 ` Lukas Bulwahn 2020-09-18 15:05 ` Dwaipayan Ray 2020-09-19 5:57 ` Dwaipayan Ray 2020-09-19 7:25 ` Lukas Bulwahn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).