checkpatch: extend author Signed-off-by check for split From: header
PATCH 1/1 checkpatch: extend author Signed-off-by check for split From: header
Checkpatch did not handle cases where the author From: header was split into two lines. The author string went empty and checkpatch generated a false NO_AUTHOR_SIGN_OFF warning.
Support split From: headers in AUTHOR_SIGN_OFF check by adding an additional clause to resolve author identity in such cases.
--- scripts/checkpatch.pl | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)
scripts/checkpatch.pl +24 -4
@@ -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:
Signature · 1 line
2.27.0
Joe Perches
maintainerQuoted 3 lines · Dwaipayan Ray
Checkpatch did not handle cases where the author From: header was split into two lines. The author string went empty and checkpatch generated a false
NO_AUTHOR_SIGN_OFFwarning.
It's good to provide an example where the current code doesn't work.
It likely would be better to do this by searching forward for any extension lines after a "^From:' rather than searching backwards as there can be any number of extension lines.
Quoted 63 lines
Support split From: headers in
AUTHOR_SIGN_OFFcheck by adding an additional clause to resolve author identity in such cases.Signed-off-by Dwaipayan Ray <[email protected]>--- scripts/checkpatch.pl | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)scripts/checkpatch.pl +24
-4@@ -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:
Quoted 8 lines · Joe Perches
Quoted 3 lines · Dwaipayan Ray
Checkpatch did not handle cases where the author From: header was split into two lines. The author string went empty and checkpatch generated a false
NO_AUTHOR_SIGN_OFFwarning.It's good to provide an example where the current code doesn't work.
Joe, as this is a linux-kernel-mentees patch, we discussed that before reaching out to you; you can find Dwaipayan's own evaluation here:
Dwaipayan, Joe's comment is still valid; it would be good to describe the reasons why patches might have split lines (as far as see, long encodings for non-ascii names).
I will run my own evaluation of checkpatch.pl before and after patch application on Monday and then check if I can confirm Dwaipayan's results.
Quoted 4 lines
It likely would be better to do this by searching forward for any extension lines after a "^From:' rather than searching backwards as there can be any number of extension lines.
Just to sure what you are talking about...
You mean just to access the next line through the lines array, rather than using prevheader and trying to decode that one line twice.
I agree the logic is a bit redundant and complicated at the moment.
Once prevheader is non-empty, it already clear that author is '' and prevheader decodes with that match, because that is the only way to make prevheader non-empty in the first place; at least as far I see it right now.
Lukas
Quoted 62 lines
Quoted 62 lines
Support split From: headers in
AUTHOR_SIGN_OFFcheck by adding an additional clause to resolve author identity in such cases.Signed-off-by Dwaipayan Ray <[email protected]>--- scripts/checkpatch.pl | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)scripts/checkpatch.pl +24
-4@@ -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; + } }
Quoted 3 lines
scripts/checkpatch.pl sub process { · line 2689
Quoted 1 line
scripts/checkpatch.pl +0
-0# Check the patch for a signoff:
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
Joe Perches
maintainerQuoted 34 lines · Lukas Bulwahn
Quoted 8 lines · Joe Perches
Quoted 3 lines · Dwaipayan Ray
Checkpatch did not handle cases where the author From: header was split into two lines. The author string went empty and checkpatch generated a false
NO_AUTHOR_SIGN_OFFwarning.It's good to provide an example where the current code doesn't work.
Joe, as this is a linux-kernel-mentees patch, we discussed that before reaching out to you; you can find Dwaipayan's own evaluation here:
Dwaipayan, Joe's comment is still valid; it would be good to describe the reasons why patches might have split lines (as far as see, long encodings for non-ascii names).
I will run my own evaluation of checkpatch.pl before and after patch application on Monday and then check if I can confirm Dwaipayan's results.
Quoted 4 lines
It likely would be better to do this by searching forward for any extension lines after a "^From:' rather than searching backwards as there can be any number of extension lines.
Just to sure what you are talking about...
You mean just to access the next line through the lines array, rather than using prevheader and trying to decode that one line twice.
Quoted 6 lines
I agree the logic is a bit redundant and complicated at the moment.
Once prevheader is non-empty, it already clear that author is '' and prevheader decodes with that match, because that is the only way to make prevheader non-empty in the first place; at least as far I see it right now.
Yeah, something like this (completely untested):
--- scripts/checkpatch.pl | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
scripts/checkpatch.pl +6 -2
@@ -2679,9 +2679,13 @@ sub process { } # Check the patch for a From: - if (decode("MIME-Header", $line) =~ /^From:\s*(.*)/) { + if ($line =~ /^From:\s*(.*)/i) { $author = $1; - $author = encode("utf8", $author) if ($line =~ /=\?utf-8\?/i); + my $curline = $linenr; + while (defined($rawlines[$curline] && $rawlines[$curline++] =~ /^ \s*(.*)/) { + $author .= $1; + } + $author = encode("utf8", $author) if ($author =~ /=\?utf-8\?/i); $author =~ s/"//g; $author = reformat_email($author); }
Quoted 58 lines · Joe Perches
Quoted 34 lines · Lukas Bulwahn
Quoted 8 lines · Joe Perches
Quoted 3 lines · Dwaipayan Ray
Checkpatch did not handle cases where the author From: header was split into two lines. The author string went empty and checkpatch generated a false
NO_AUTHOR_SIGN_OFFwarning.It's good to provide an example where the current code doesn't work.
Joe, as this is a linux-kernel-mentees patch, we discussed that before reaching out to you; you can find Dwaipayan's own evaluation here:
Dwaipayan, Joe's comment is still valid; it would be good to describe the reasons why patches might have split lines (as far as see, long encodings for non-ascii names).
I will run my own evaluation of checkpatch.pl before and after patch application on Monday and then check if I can confirm Dwaipayan's results.
Quoted 4 lines
It likely would be better to do this by searching forward for any extension lines after a "^From:' rather than searching backwards as there can be any number of extension lines.
Just to sure what you are talking about...
You mean just to access the next line through the lines array, rather than using prevheader and trying to decode that one line twice.
Quoted 6 lines
I agree the logic is a bit redundant and complicated at the moment.
Once prevheader is non-empty, it already clear that author is '' and prevheader decodes with that match, because that is the only way to make prevheader non-empty in the first place; at least as far I see it right now.
Yeah, something like this (completely untested):
--- scripts/checkpatch.pl | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)scripts/checkpatch.pl +1
-1@@ -2679,9 +2679,13 @@ sub process { } # Check the patch for a From: - if (decode("MIME-Header", $line) =~ /^From:\s*(.*)/) { + if ($line =~ /^From:\s*(.*)/i) {
scripts/checkpatch.pl sub process { · line 2691
Quoted 10 lines
scripts/checkpatch.pl +5
-1$author = $1; - $author = encode("utf8", $author) if ($line =~ /=\?utf-8\?/i); + my $curline = $linenr; + while (defined($rawlines[$curline] && $rawlines[$curline++] =~ /^ \s*(.*)/) { + $author .= $1; + } + $author = encode("utf8", $author) if ($author =~ /=\?utf-8\?/i); $author =~ s/"//g; $author = reformat_email($author); }
Yeah, I get how you would like to see that being implemented. I will work with Dwaipayan to get that properly implemented, properly described and tested.
But let us keep the fun of that task to Dwaipayan... that is what a mentorship is all about :)
Lukas
Quoted 59 lines · Lukas Bulwahn
Quoted 54 lines · Joe Perches
Quoted 34 lines · Lukas Bulwahn
Quoted 8 lines · Joe Perches
Quoted 3 lines · Dwaipayan Ray
Checkpatch did not handle cases where the author From: header was split into two lines. The author string went empty and checkpatch generated a false
NO_AUTHOR_SIGN_OFFwarning.It's good to provide an example where the current code doesn't work.
Joe, as this is a linux-kernel-mentees patch, we discussed that before reaching out to you; you can find Dwaipayan's own evaluation here:
Dwaipayan, Joe's comment is still valid; it would be good to describe the reasons why patches might have split lines (as far as see, long encodings for non-ascii names).
I will run my own evaluation of checkpatch.pl before and after patch application on Monday and then check if I can confirm Dwaipayan's results.
Quoted 4 lines
It likely would be better to do this by searching forward for any extension lines after a "^From:' rather than searching backwards as there can be any number of extension lines.
Just to sure what you are talking about...
You mean just to access the next line through the lines array, rather than using prevheader and trying to decode that one line twice.
Quoted 6 lines
I agree the logic is a bit redundant and complicated at the moment.
Once prevheader is non-empty, it already clear that author is '' and prevheader decodes with that match, because that is the only way to make prevheader non-empty in the first place; at least as far I see it right now.
Yeah, something like this (completely untested):
--- scripts/checkpatch.pl | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)scripts/checkpatch.pl +0
-0@@ -2679,9 +2679,13 @@ sub process { }
Quoted 15 lines
Quoted 4 lines
scripts/checkpatch.pl +1
-1# Check the patch for a From: - if (decode("MIME-Header", $line) =~ /^From:\s*(.*)/) { + if ($line =~ /^From:\s*(.*)/i) {scripts/checkpatch.pl sub process { · line 2691
Quoted 10 lines
scripts/checkpatch.pl +5
-1$author = $1; - $author = encode("utf8", $author) if ($line =~ /=\?utf-8\?/i); + my $curline = $linenr; + while (defined($rawlines[$curline] && $rawlines[$curline++] =~ /^ \s*(.*)/) { + $author .= $1; + } + $author = encode("utf8", $author) if ($author =~ /=\?utf-8\?/i); $author =~ s/"//g; $author = reformat_email($author); }
Hi,
Yeah I think the backwards checking was pretty redundant after all. If the extended encoding went too long, the From: header would be split into more than two lines and my proposed solution would fail.
Thanks for the heads up, Joe!
Quoted 8 lines
Yeah, I get how you would like to see that being implemented. I will work with Dwaipayan to get that properly implemented, properly described and tested.
But let us keep the fun of that task to Dwaipayan... that is what a mentorship is all about :)
Lukas
Yes definitely, the task is interesting for me, and I would like to solve it in a proper way.
As for the fix, shouldn't we stop the author string concatenation once an email address is found? something like:
last if $rawlines[$curline] = ~/^\s*(\S+\@\S+)\s*/
I will update the patch and sync up with Lukas on this.
Thanks,
Dwaipayan.
Joe Perches
maintainerQuoted 84 lines · Dwaipayan Ray
Quoted 51 lines · Lukas Bulwahn
Quoted 50 lines · Joe Perches
Quoted 30 lines · Lukas Bulwahn
Quoted 8 lines · Joe Perches
Quoted 3 lines · Dwaipayan Ray
Checkpatch did not handle cases where the author From: header was split into two lines. The author string went empty and checkpatch generated a false
NO_AUTHOR_SIGN_OFFwarning.It's good to provide an example where the current code doesn't work.
Joe, as this is a linux-kernel-mentees patch, we discussed that before reaching out to you; you can find Dwaipayan's own evaluation here:
Dwaipayan, Joe's comment is still valid; it would be good to describe the reasons why patches might have split lines (as far as see, long encodings for non-ascii names).
I will run my own evaluation of checkpatch.pl before and after patch application on Monday and then check if I can confirm Dwaipayan's results.
Quoted 4 lines
It likely would be better to do this by searching forward for any extension lines after a "^From:' rather than searching backwards as there can be any number of extension lines.
Just to sure what you are talking about...
You mean just to access the next line through the lines array, rather than using prevheader and trying to decode that one line twice.
Quoted 6 lines
I agree the logic is a bit redundant and complicated at the moment.
Once prevheader is non-empty, it already clear that author is '' and prevheader decodes with that match, because that is the only way to make prevheader non-empty in the first place; at least as far I see it right now.
Yeah, something like this (completely untested):
--- scripts/checkpatch.pl | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)scripts/checkpatch.pl +0
-0@@ -2679,9 +2679,13 @@ sub process { }Quoted 14 lines
Quoted 4 lines
scripts/checkpatch.pl +1
-1# Check the patch for a From: - if (decode("MIME-Header", $line) =~ /^From:\s*(.*)/) { + if ($line =~ /^From:\s*(.*)/i) {scripts/checkpatch.pl sub process { · line 2691
Quoted 10 lines
scripts/checkpatch.pl +5
-1$author = $1; - $author = encode("utf8", $author) if ($line =~ /=\?utf-8\?/i); + my $curline = $linenr; + while (defined($rawlines[$curline] && $rawlines[$curline++] =~ /^ \s*(.*)/) { + $author .= $1; + } + $author = encode("utf8", $author) if ($author =~ /=\?utf-8\?/i); $author =~ s/"//g; $author = reformat_email($author); }Hi,
Yeah I think the backwards checking was pretty redundant after all. If the extended encoding went too long, the From: header would be split into more than two lines and my proposed solution would fail.
Thanks for the heads up, Joe!
Quoted 8 lines
Yeah, I get how you would like to see that being implemented. I will work with Dwaipayan to get that properly implemented, properly described and tested.
But let us keep the fun of that task to Dwaipayan... that is what a mentorship is all about :)
Lukas
Quoted 7 lines
Yes definitely, the task is interesting for me, and I would like to solve it in a proper way.
As for the fix, shouldn't we stop the author string concatenation once an email address is found? something like:
last if $rawlines[$curline] = ~/^\s*(\S+\@\S+)\s*/
Probably not.
I think it should follow the rfc standard with extension lines starting with a space.
See rfc 5322, 2.2.3 Long Header Fields
Quoted 1 line
I will update the patch and sync up with Lukas on this.
Enjoy.
I believe I now have a working version, we can compare later.
cheers, Joe