* [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).