checkpatch: improve email parsing
PATCH v4 1/1 checkpatch: improve email parsing
checkpatch doesn't report warnings for many common mistakes in emails. Some of which are trailing commas and incorrect use of email comments.
At the same time several false positives are reported due to incorrect handling of mail comments. The most common of which is due to the pattern:
<[email protected]> # X.X
Improve email parsing in checkpatch.
Some general email rules are defined:
- Multiple name comments should not be allowed.
- Comments inside address should not be allowed.
- In general comments should be enclosed within parentheses. Relaxation is given to comments beginning with #.
- Stable addresses should not begin with a name.
- Comments in stable addresses should begin only with a #.
Improvements to parsing:
- Detect and report unexpected content after email.
- Quoted names are excluded from comment parsing.
- Trailing dots, commas or quotes in email are removed during formatting. Correspondingly a BAD_SIGN_OFF warning is emitted.
- Improperly quoted email like '"name <address>"' are now warned about.
In addition, added fixes for all the possible rules.
--- scripts/checkpatch.pl | 97 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 80 insertions(+), 17 deletions(-)
scripts/checkpatch.pl +80 -17
@@ -1152,6 +1152,7 @@ sub parse_email { my ($formatted_email) = @_; my $name = ""; + my $quoted = ""; my $name_comment = ""; my $address = ""; my $comment = "";
@@ -1183,14 +1184,20 @@ sub parse_email { } } - $comment = trim($comment); - $name = trim($name); - $name =~ s/^\"|\"$//g; - if ($name =~ s/(\s*\([^\)]+\))\s*//) { - $name_comment = trim($1); + # Extract comments from names excluding quoted parts + # "John A. (Kennedy)" - Do not extract + if ($name =~ s/\"(.+)\"//) { + $quoted = $1; + } + while ($name =~ s/\s*($balanced_parens)\s*/ /) { + $name_comment .= trim($1); } + $name =~ s/^[ \"]+|[ \"]+$//g; + $name = trim("$quoted $name"); + $address = trim($address); $address =~ s/^\<|\>$//g; + $comment = trim($comment); if ($name =~ /[^\w \-]/i) { ##has "must quote" chars $name =~ s/(?<!\\)"/\\"/g; ##escape quotes
@@ -1205,17 +1212,20 @@ sub format_email { my $formatted_email; - $name_comment = trim($name_comment); - $comment = trim($comment); - $name = trim($name); - $name =~ s/^\"|\"$//g; + $name =~ s/^[ \"]+|[ \"]+$//g; $address = trim($address); + $address =~ s/(?:\.|\,|\")+$//; ##trailing commas, dots or quotes if ($name =~ /[^\w \-]/i) { ##has "must quote" chars $name =~ s/(?<!\\)"/\\"/g; ##escape quotes $name = "\"$name\""; } + $name_comment = trim($name_comment); + $name_comment = " $name_comment" if length($name_comment) > 0; + $comment = trim($comment); + $comment = " $comment" if length($comment) > 0; + if ("$name" eq "") { $formatted_email = "$address"; } else {
@@ -1233,15 +1243,11 @@ sub reformat_email { } sub same_email_addresses { - my ($email1, $email2, $match_comment) = @_; + my ($email1, $email2) = @_; my ($email1_name, $name1_comment, $email1_address, $comment1) = parse_email($email1); my ($email2_name, $name2_comment, $email2_address, $comment2) = parse_email($email2); - if ($match_comment != 1) { - return $email1_name eq $email2_name && - $email1_address eq $email2_address; - } return $email1_name eq $email2_name && $email1_address eq $email2_address && $name1_comment eq $name2_comment &&
@@ -2704,7 +2710,7 @@ sub process { $signoff++; $in_commit_log = 0; if ($author ne '' && $authorsignoff != 1) { - if (same_email_addresses($1, $author, 1)) { + if (same_email_addresses($1, $author)) { $authorsignoff = 1; } else { my $ctx = $1;
@@ -2800,9 +2806,66 @@ sub process { $dequoted =~ s/" </ </; # Don't force email to have quotes # Allow just an angle bracketed address - if (!same_email_addresses($email, $suggested_email, 0)) { + if (!same_email_addresses($email, $suggested_email)) { + if (WARN("BAD_SIGN_OFF", + "email address '$email' might be better as '$suggested_email'\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/\Q$email\E/$suggested_email/; + } + } + + # Address part shouldn't have comments + my $stripped_address = $email_address; + $stripped_address =~ s/\([^\(\)]*\)//g; + if ($email_address ne $stripped_address) { + if (WARN("BAD_SIGN_OFF", + "address part of email should not have comments: '$email_address'\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/\Q$email_address\E/$stripped_address/; + } + } + + # Only one name comment should be allowed + my $comment_count = () = $name_comment =~ /\([^\)]+\)/g; + if ($comment_count > 1) { WARN("BAD_SIGN_OFF", - "email address '$email' might be better as '$suggested_email'\n" . $herecurr); + "Use a single name comment in email: '$email'\n" . $herecurr); + } + + + # [email protected] or [email protected] shouldn't + # have an email name. In addition commments should strictly + # begin with a # + if ($email =~ /^.*stable\@(?:vger\.)?kernel\.org/) { + if ($sign_off =~ /cc:$/i && (($comment ne "" && $comment !~ /^#.+/) || + ($email_name ne ""))) { + my $cur_name = $email_name; + my $new_comment = $comment; + + $cur_name =~ s/[a-zA-Z\s\-\"]+//g; + $new_comment =~ s/^[\s\#\(\[]+|[\s\)\]]+$//g; + $new_comment = trim("$new_comment $cur_name") if $cur_name ne $new_comment; + $new_comment = " # $new_comment" if length($new_comment) > 0; + my $new_email = "$email_address$new_comment"; + + if (WARN("BAD_SIGN_OFF", + "Invalid email format for stable: '$email', prefer '$new_email'\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/\Q$email\E/$new_email/; + } + } + } else { + if ($comment ne "" && $comment !~ /^(?:#.+|\(.+\))$/) { + if (WARN("BAD_SIGN_OFF", + "Unexpected content after email: '$email'\n" . $herecurr) && + $fix) { + my $new_comment = $comment; + $new_comment =~ s/^(?:\/\*|\.|\,)//g; + $new_comment =~ s/^[\s\{\[]+|[\s\}\]]+$//g; + $new_comment = " ($new_comment)" if length($new_comment) > 0; + $fixed[$fixlinenr] =~ s/\s*\Q$comment\E$/$new_comment/; + } + } } }
Signature · 1 line
2.27.0
Joe Perches
maintainerQuoted 3 lines · Dwaipayan Ray
checkpatch doesn't report warnings for many common mistakes in emails. Some of which are trailing commas and incorrect use of email comments.
Assuming it all works, this looks good. I haven't tested it.
How did you test the $fix bits?
Trivial notes:
scripts/checkpatch.pl
Quoted 1 line
scripts/checkpatch.pl +0
-0
[]
scripts/checkpatch.pl sub process { · line 2839
Quoted 4 lines
scripts/checkpatch.pl +4
-0+ # [email protected] or [email protected] shouldn't + # have an email name. In addition commments should strictly + # begin with a # + if ($email =~ /^.*stable\@(?:vger\.)?kernel\.org/) {
Likely better to test with a case insensitive match so [email protected] and such are still warned.
if ($email =~ /\bstable\@(?:vger\.)?kernel\.org\b/i) {scripts/checkpatch.pl sub process { · line 2841
Quoted 2 lines
scripts/checkpatch.pl +2
-0+ if ($sign_off =~ /cc:$/i && (($comment ne "" && $comment !~ /^#.+/) || + ($email_name ne ""))) {
|| $sign_off !~ /^cc:/i ?
scripts/checkpatch.pl sub process { · line 2852
Quoted 11 lines
scripts/checkpatch.pl +11
-0+ my $cur_name = $email_name; + my $new_comment = $comment; + + $cur_name =~ s/[a-zA-Z\s\-\"]+//g; + $new_comment =~ s/^[\s\#\(\[]+|[\s\)\]]+$//g; + $new_comment = trim("$new_comment $cur_name") if $cur_name ne $new_comment; + $new_comment = " # $new_comment" if length($new_comment) > 0; + my $new_email = "$email_address$new_comment"; + + if (WARN("BAD_SIGN_OFF", + "Invalid email format for stable: '$email', prefer '$new_email'\n" . $herecurr) &&
These message lines should be aligned to the next open parenthesis (7 tabs, 1 space)
scripts/checkpatch.pl sub process { · line 2855
Quoted 3 lines
scripts/checkpatch.pl +3
-0+ $fix) { + $fixed[$fixlinenr] =~ s/\Q$email\E/$new_email/; + }
As a cc to stable isn't a sign-off, perhaps this should be a different "BAD_<FOO>" like "BAD_STABLE_ADDRESS_STYLE" or such.
scripts/checkpatch.pl
Quoted 2 lines
scripts/checkpatch.pl +2
-0+ } + } else {
elsif ?
scripts/checkpatch.pl sub process { · line 2860
Quoted 3 lines
scripts/checkpatch.pl +3
-0+ if ($comment ne "" && $comment !~ /^(?:#.+|\(.+\))$/) { + if (WARN("BAD_SIGN_OFF", + "Unexpected content after email: '$email'\n" . $herecurr) &&
7 tabs, 1 space
Quoted 12 lines
scripts/checkpatch.pl +8
-0+ $fix) { + my $new_comment = $comment; + $new_comment =~ s/^(?:\/\*|\.|\,)//g; + $new_comment =~ s/^[\s\{\[]+|[\s\}\]]+$//g; + $new_comment = " ($new_comment)" if length($new_comment) > 0; + $fixed[$fixlinenr] =~ s/\s*\Q$comment\E$/$new_comment/; + } + }}
}
Quoted 10 lines · Joe Perches
Quoted 3 lines · Dwaipayan Ray
checkpatch doesn't report warnings for many common mistakes in emails. Some of which are trailing commas and incorrect use of email comments.
Assuming it all works, this looks good. I haven't tested it.
How did you test the $fix bits?
Hi,
I actually dumped about 17k unique emails from git log, put it in one of my previous patches, and ran checkpatch with --fix on it.
I checked the diff and most of the cases looked pretty good to me. I could send the diff output if you like?
Quoted 11 lines
Trivial notes:
scripts/checkpatch.pl
Quoted 1 line
scripts/checkpatch.pl +0
-0[]
scripts/checkpatch.pl sub process { · line 2839
Quoted 4 lines
scripts/checkpatch.pl +4
-0+ # [email protected] or [email protected] shouldn't + # have an email name. In addition commments should strictly + # begin with a # + if ($email =~ /^.*stable\@(?:vger\.)?kernel\.org/) {Likely better to test with a case insensitive match so [email protected] and such are still warned.
Sure, I will do that.
Quoted 7 lines
if ($email =~ /\bstable\@(?:vger\.)?kernel\.org\b/i) {scripts/checkpatch.pl sub process { · line 2841
Quoted 2 lines
scripts/checkpatch.pl +2
-0+ if ($sign_off =~ /cc:$/i && (($comment ne "" && $comment !~ /^#.+/) || + ($email_name ne ""))) {|| $sign_off !~ /^cc:/i ?
I actually had a doubt about that one. Only the stable address with
Cc: should be checked right? Or something else?
What about those stable addresses with tags other than Cc: ? Should a change be suggested?
Thanks,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
Joe Perches
maintainerQuoted 9 lines · Dwaipayan Ray
Quoted 8 lines · Joe Perches
Quoted 3 lines · Dwaipayan Ray
checkpatch doesn't report warnings for many common mistakes in emails. Some of which are trailing commas and incorrect use of email comments.
Assuming it all works, this looks good. I haven't tested it.
Quoted 7 lines
Quoted 2 lines
How did you test the $fix bits?
Hi,
I actually dumped about 17k unique emails from git log, put it in one of my previous patches, and ran checkpatch with --fix on it.
I checked the diff and most of the cases looked pretty good to me. I could send the diff output if you like?
Please. Likely just to me as I imagine it's not interesting to most.
Quoted 16 lines
Quoted 11 lines
Trivial notes:
scripts/checkpatch.pl
Quoted 1 line
scripts/checkpatch.pl +0
-0[]
scripts/checkpatch.pl sub process { · line 2839
Quoted 4 lines
scripts/checkpatch.pl +4
-0+ # [email protected] or [email protected] shouldn't + # have an email name. In addition commments should strictly + # begin with a # + if ($email =~ /^.*stable\@(?:vger\.)?kernel\.org/) {Likely better to test with a case insensitive match so [email protected] and such are still warned.
Sure, I will do that.
Quoted 3 lines
if ($email =~ /\bstable\@(?:vger\.)?kernel\.org\b/i) {
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
scripts/checkpatch.pl +2
-0+ if ($sign_off =~ /cc:$/i && (($comment ne "" && $comment !~ /^#.+/) || + ($email_name ne ""))) {Quoted 2 lines
$sign_off !~ /^cc:/i ?
I actually had a doubt about that one. Only the stable address with
Cc: should be checked right? Or something else?
yes.
Quoted 2 lines
What about those stable addresses with tags other than Cc: ? Should a change be suggested?
Ideally yes, but there were very few of those in the git commit history so it's probably not a big deal one way or another.