checkpatch: extend author Signed-off-by check for split From: header
PATCH v2 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 multiple lines. The author identity could not be resolved and checkpatch generated a false NO_AUTHOR_SIGN_OFF warning.
A typical example is Commit e33bcbab16d1 ("tee: add support for session's client UUID generation"). When checkpatch was run on this commit, it displayed:
"WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author ''"
This was due to split header lines not being handled properly and the author himself wrote in Commit cd2614967d8b ("checkpatch: warn if missing author Signed-off-by"):
"Split From: headers are not fully handled: only the first part is compared."
Support split From: headers by correctly parsing the header extension lines. RFC 2822, Section-2.2.3 stated that each extended line must start with a WSP character (a space or htab). The solution was therefore to concatenate the lines which start with a WSP to get the correct long header.
--- scripts/checkpatch.pl | 4 ++++ 1 file changed, 4 insertions(+)
scripts/checkpatch.pl +4 -0
@@ -2661,6 +2661,10 @@ sub process { # Check the patch for a From: if (decode("MIME-Header", $line) =~ /^From:\s*(.*)/) { $author = $1; + my $curline = $linenr; + while(defined($rawlines[$curline]) && ($rawlines[$curline++] =~ /^[ \t]\s*(.*)/)) { + $author .= $1; + } $author = encode("utf8", $author) if ($line =~ /=\?utf-8\?/i); $author =~ s/"//g; $author = reformat_email($author);
Signature · 1 line
2.27.0
Joe Perches
maintainerQuoted 4 lines · Dwaipayan Ray
Checkpatch did not handle cases where the author From: header was split into multiple lines. The author identity could not be resolved and checkpatch generated a false
NO_AUTHOR_SIGN_OFFwarning.
Hi Dwaipayan.
Quoted 11 lines
A typical example is Commit
e33bcbab16d1("tee: add support for session's client UUID generation"). When checkpatch was run on this commit, it displayed:"WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author ''"
This was due to split header lines not being handled properly and the author himself wrote in Commit
cd2614967d8b("checkpatch: warn if missing author Signed-off-by"):
Quoted 8 lines
"Split From: headers are not fully handled: only the first part is compared."
Support split From: headers by correctly parsing the header extension lines. RFC 2822, Section-2.2.3 stated that each extended line must start with a WSP character (a space or htab). The solution was therefore to concatenate the lines which start with a WSP to get the correct long header.
This is a good commit message, though I believe the latest rfc is 5322. I'm not sure there is any real difference in the referenced section though.
While your patch seems to work for git format-email, other emailers seem to set headers that have multiple whitespace chars that should be collapsed into a single space.
I think you'll find that the eliding all whitespace after header folding causes mismatches for emails.
For instance:
From: "=?UTF-8?q?Christian=20K=C3=B6nig?="
<[email protected]>
Always inserting a single space if there is any whitespace after the folding WSP might be better otherwise this is decoded as
From: "Christian König"<[email protected]>
What I have does a bit more by saving any post-folding
"From: <name and email address>"
and comparing that to any "name and perhaps different email address" in a Signed-off-by: line.
A new message is emitted if the name matches but the email address is different.
Perhaps it's reasonable to apply your patch and then update it with something like the below:
--- scripts/checkpatch.pl | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-)
scripts/checkpatch.pl +28 -4
@@ -1240,6 +1240,15 @@ sub same_email_addresses { $email1_address eq $email2_address; } +sub same_email_names { + 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); + + return $email1_name eq $email2_name; +} + sub which { my ($bin) = @_;
@@ -2679,20 +2688,32 @@ 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(\s+)?(.*)/) { + $author .= ' ' if (defined($1)); + $author .= "$2"; + } + if ($author =~ /=\?utf-8\?/i) { + $author = decode("MIME-Header", $author); + $author = encode("utf8", $author); + } + $author =~ s/"//g; $author = reformat_email($author); } # Check the patch for a signoff: if ($line =~ /^\s*signed-off-by:\s*(.*)/i) { + my $sig = $1; $signoff++; $in_commit_log = 0; if ($author ne '') { - if (same_email_addresses($1, $author)) { - $authorsignoff = 1; + if (same_email_addresses($sig, $author)) { + $authorsignoff = "1"; + } elsif (same_email_names($sig, $author)) { + $authorsignoff = $sig; } } }
@@ -6937,6 +6958,9 @@ sub process { } elsif (!$authorsignoff) { WARN("NO_AUTHOR_SIGN_OFF", "Missing Signed-off-by: line by nominal patch author '$author'\n"); + } elsif ($authorsignoff ne "1") { + WARN("NO_AUTHOR_SIGN_OFF", + "From:/SoB: email address mismatch: 'From: $author' != 'Signed-off-by: $authorsignoff'\n"); } }
Quoted 43 lines · Joe Perches
Quoted 4 lines · Dwaipayan Ray
Checkpatch did not handle cases where the author From: header was split into multiple lines. The author identity could not be resolved and checkpatch generated a false
NO_AUTHOR_SIGN_OFFwarning.Hi Dwaipayan.
Quoted 11 lines
A typical example is Commit
e33bcbab16d1("tee: add support for session's client UUID generation"). When checkpatch was run on this commit, it displayed:"WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author ''"
This was due to split header lines not being handled properly and the author himself wrote in Commit
cd2614967d8b("checkpatch: warn if missing author Signed-off-by"):Quoted 8 lines
"Split From: headers are not fully handled: only the first part is compared."
Support split From: headers by correctly parsing the header extension lines. RFC 2822, Section-2.2.3 stated that each extended line must start with a WSP character (a space or htab). The solution was therefore to concatenate the lines which start with a WSP to get the correct long header.
This is a good commit message, though I believe the latest rfc is 5322. I'm not sure there is any real difference in the referenced section though.
While your patch seems to work for git format-email, other emailers seem to set headers that have multiple whitespace chars that should be collapsed into a single space.
I think you'll find that the eliding all whitespace after header folding causes mismatches for emails.
For instance:
Quoted 9 lines
From: "=?UTF-8?q?Christian=20K=C3=B6nig?="
<[email protected]>Always inserting a single space if there is any whitespace after the folding WSP might be better otherwise this is decoded as
From: "Christian König"<[email protected]>
Hi,
I think eliding all whitespaces shouldn't cause an issue because at the end of the From: header parser block, there is a call to reformat_email($author).
$author =~ s/"//g; $author = reformat_email($author);
The subroutine reformat_email reparses the author string such that the correct name <address> format is maintainined.
In revision b3b33d3c43bb,
line 1206:
sub reformat_email {
my ($email) = @_;
my ($email_name, $name_comment, $email_address,
$comment) = parse_email($email);
return format_email($email_name, $email_address);
}And I also checked the format_email subroutine:
line 1997:
if ("$name" eq "") {
$formatted_email = "$address";
} else {
$formatted_email = "$name <$address>";
}return $formatted_email;
So I think the author string is basically reconstructed to maintain the correct format.
As you pointed out, at first the author string might be:
"Christian König"<[email protected]>
But after reformat_email is called, $author should be:
Christian König <[email protected]>
So, I think there won't be any problem. Is my
observation correct?
Quoted 75 lines
What I have does a bit more by saving any post-folding
"From: <name and email address>"
and comparing that to any "name and perhaps different email address" in a Signed-off-by: line.
A new message is emitted if the name matches but the email address is different.
Perhaps it's reasonable to apply your patch and then update it with something like the below:
--- scripts/checkpatch.pl | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-)scripts/checkpatch.pl +25
-4@@ -1240,6 +1240,15 @@ sub same_email_addresses { $email1_address eq $email2_address; } +sub same_email_names { + 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); + + return $email1_name eq $email2_name; +} + sub which { my ($bin) = @_;@@ -2679,20 +2688,32 @@ 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(\s+)?(.*)/) { + $author .= ' ' if (defined($1)); + $author .= "$2"; + } + if ($author =~ /=\?utf-8\?/i) { + $author = decode("MIME-Header", $author); + $author = encode("utf8", $author); + } + $author =~ s/"//g; $author = reformat_email($author); } # Check the patch for a signoff: if ($line =~ /^\s*signed-off-by:\s*(.*)/i) { + my $sig = $1; $signoff++; $in_commit_log = 0; if ($author ne '') { - if (same_email_addresses($1, $author)) { - $authorsignoff = 1; + if (same_email_addresses($sig, $author)) { + $authorsignoff = "1"; + } elsif (same_email_names($sig, $author)) { + $authorsignoff = $sig; } } }
scripts/checkpatch.pl sub process { · line 6966
Quoted 9 lines
scripts/checkpatch.pl +3
-0@@ -6937,6 +6958,9 @@ sub process { } elsif (!$authorsignoff) { WARN("NO_AUTHOR_SIGN_OFF", "Missing Signed-off-by: line by nominal patch author '$author'\n"); + } elsif ($authorsignoff ne "1") { + WARN("NO_AUTHOR_SIGN_OFF", + "From:/SoB: email address mismatch: 'From: $author' != 'Signed-off-by: $authorsignoff'\n"); } }
Yes, this is definitely more logical !
I was actually hoping to talk with you on this.
The code you sent better handles name mismatches when email addresses are same. But I also have found several such commits in which the author have signed off using a different email address than the one which he/she used to send the patch.
For example, Lukas checked commits between v5.4 and v5.8 and he found:
175 Missing Signed-off-by: line by nominal patch author 'Daniel Vetter <[email protected]>'
Infact in all of those commits he signed off using a different mail, Daniel Vetter <[email protected]>.
So is it possible to resolve these using perhaps .mailmap entries? Or should only the name mismatch part be better handled? Or perhaps both?
Also, I would like to know if there are any more changes required for the current patch or if it is good to go?
Thanks,
Dwaipayan.
Joe Perches
maintainerQuoted 9 lines · Dwaipayan Ray
Quoted 6 lines · Joe Perches
Quoted 4 lines · Dwaipayan Ray
Checkpatch did not handle cases where the author From: header was split into multiple lines. The author identity could not be resolved and checkpatch generated a false
NO_AUTHOR_SIGN_OFFwarning.I think there won't be any problem. Is my
observation correct?
Likely true, it probably doesn't matter. Still, I'd imagine it doesn't hurt either.
Quoted 79 lines
Quoted 79 lines
What I have does a bit more by saving any post-folding
"From: <name and email address>"
and comparing that to any "name and perhaps different email address" in a Signed-off-by: line.
A new message is emitted if the name matches but the email address is different.
Perhaps it's reasonable to apply your patch and then update it with something like the below:
--- scripts/checkpatch.pl | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-)scripts/checkpatch.pl +27
-4@@ -1240,6 +1240,15 @@ sub same_email_addresses { $email1_address eq $email2_address; } +sub same_email_names { + 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); + + return $email1_name eq $email2_name; +} + sub which { my ($bin) = @_;@@ -2679,20 +2688,32 @@ 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(\s+)?(.*)/) { + $author .= ' ' if (defined($1)); + $author .= "$2"; + } + if ($author =~ /=\?utf-8\?/i) { + $author = decode("MIME-Header", $author); + $author = encode("utf8", $author); + } + $author =~ s/"//g; $author = reformat_email($author); } # Check the patch for a signoff: if ($line =~ /^\s*signed-off-by:\s*(.*)/i) { + my $sig = $1; $signoff++; $in_commit_log = 0; if ($author ne '') { - if (same_email_addresses($1, $author)) { - $authorsignoff = 1; + if (same_email_addresses($sig, $author)) { + $authorsignoff = "1"; + } elsif (same_email_names($sig, $author)) { + $authorsignoff = $sig; } } }@@ -6937,6 +6958,9 @@ sub process { } elsif (!$authorsignoff) { WARN("NO_AUTHOR_SIGN_OFF", "Missing Signed-off-by: line by nominal patch author '$author'\n"); + } elsif ($authorsignoff ne "1") { + WARN("NO_AUTHOR_SIGN_OFF",
Quoted 8 lines
scripts/checkpatch.pl sub process { · line 6966
Quoted 5 lines
scripts/checkpatch.pl +1
-0+ "From:/SoB: email address mismatch: 'From: $author' != 'Signed-off-by: $authorsignoff'\n"); } }Yes, this is definitely more logical !
I was actually hoping to talk with you on this.
Hope granted, but only via email... (smile)
Quoted 11 lines
The code you sent better handles name mismatches when email addresses are same. But I also have found several such commits in which the author have signed off using a different email address than the one which he/she used to send the patch.
For example, Lukas checked commits between v5.4 and v5.8 and he found:
175 Missing Signed-off-by: line by nominal patch authorrep 'Daniel Vetter <[email protected]>'
Quoted 6 lines
Infact in all of those commits he signed off using a different mail, Daniel Vetter <[email protected]>.
So is it possible to resolve these using perhaps .mailmap entries? Or should only the name mismatch part be better handled? Or perhaps both?
Dunno. It certainly can be improved...
Try adding some more logic and see what you come up with.
btw:
The most frequent NO_AUTHOR_SIGN_OFF messages for v5.7..v5.8 are
98 WARNING: From:/SoB: email address mismatch: 'From: Daniel Vetter <[email protected]>' != 'Signed-off-by: Daniel Vetter <[email protected]>' 24 WARNING: From:/SoB: email address mismatch: 'From: Thinh Nguyen <[email protected]>' != 'Signed-off-by: Thinh Nguyen <[email protected]>' 19 WARNING: From:/SoB: email address mismatch: 'From: Wolfram Sang <[email protected]>' != 'Signed-off-by: Wolfram Sang <[email protected]>' 11 WARNING: From:/SoB: email address mismatch: 'From: Luke Nelson <[email protected]>' != 'Signed-off-by: Luke Nelson <[email protected]>' 8 WARNING: From:/SoB: email address mismatch: 'From: Christophe Leroy <[email protected]>' != 'Signed-off-by: Christophe Leroy <[email protected]>' 6 WARNING: From:/SoB: email address mismatch: 'From: Davidlohr Bueso <[email protected]>' != 'Signed-off-by: Davidlohr Bueso <[email protected]>' 5 WARNING: Missing Signed-off-by: line by nominal patch author '"Paul A. Clarke" <[email protected]>' 4 WARNING: Missing Signed-off-by: line by nominal patch author 'Huang Ying <[email protected]>' 3 WARNING: Missing Signed-off-by: line by nominal patch author '"Stephan Müller" <[email protected]>'
For the Missing Signed-off-by: lines above, even after decoding, the email matches but the names do not.
From: "Paul A. Clarke" <[email protected]>
From: Huang Ying <[email protected]>
From: =?UTF-8?q?Stephan=20M=C3=BCller?= <[email protected]>
Quoted 2 lines
Also, I would like to know if there are any more changes required for the current patch or if it is good to go?
I think it's fine.
cheers, Joe
Quoted 129 lines · Joe Perches
Quoted 9 lines · Dwaipayan Ray
Quoted 6 lines · Joe Perches
Quoted 4 lines · Dwaipayan Ray
Checkpatch did not handle cases where the author From: header was split into multiple lines. The author identity could not be resolved and checkpatch generated a false
NO_AUTHOR_SIGN_OFFwarning.I think there won't be any problem. Is my
observation correct?Likely true, it probably doesn't matter. Still, I'd imagine it doesn't hurt either.
Quoted 79 lines
Quoted 79 lines
What I have does a bit more by saving any post-folding
"From: <name and email address>"
and comparing that to any "name and perhaps different email address" in a Signed-off-by: line.
A new message is emitted if the name matches but the email address is different.
Perhaps it's reasonable to apply your patch and then update it with something like the below:
--- scripts/checkpatch.pl | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-)scripts/checkpatch.pl +27
-4@@ -1240,6 +1240,15 @@ sub same_email_addresses { $email1_address eq $email2_address; } +sub same_email_names { + 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); + + return $email1_name eq $email2_name; +} + sub which { my ($bin) = @_;@@ -2679,20 +2688,32 @@ 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(\s+)?(.*)/) { + $author .= ' ' if (defined($1)); + $author .= "$2"; + } + if ($author =~ /=\?utf-8\?/i) { + $author = decode("MIME-Header", $author); + $author = encode("utf8", $author); + } + $author =~ s/"//g; $author = reformat_email($author); } # Check the patch for a signoff: if ($line =~ /^\s*signed-off-by:\s*(.*)/i) { + my $sig = $1; $signoff++; $in_commit_log = 0; if ($author ne '') { - if (same_email_addresses($1, $author)) { - $authorsignoff = 1; + if (same_email_addresses($sig, $author)) { + $authorsignoff = "1"; + } elsif (same_email_names($sig, $author)) { + $authorsignoff = $sig; } } }@@ -6937,6 +6958,9 @@ sub process { } elsif (!$authorsignoff) { WARN("NO_AUTHOR_SIGN_OFF", "Missing Signed-off-by: line by nominal patch author '$author'\n"); + } elsif ($authorsignoff ne "1") { + WARN("NO_AUTHOR_SIGN_OFF",Quoted 8 lines
scripts/checkpatch.pl sub process { · line 6966
Quoted 5 lines
scripts/checkpatch.pl +1
-0+ "From:/SoB: email address mismatch: 'From: $author' != 'Signed-off-by: $authorsignoff'\n"); } }Yes, this is definitely more logical !
I was actually hoping to talk with you on this.Hope granted, but only via email... (smile)
Quoted 11 lines
The code you sent better handles name mismatches when email addresses are same. But I also have found several such commits in which the author have signed off using a different email address than the one which he/she used to send the patch.
For example, Lukas checked commits between v5.4 and v5.8 and he found:
175 Missing Signed-off-by: line by nominal patch authorrep 'Daniel Vetter <[email protected]>'Quoted 6 lines
Infact in all of those commits he signed off using a different mail, Daniel Vetter <[email protected]>.
So is it possible to resolve these using perhaps .mailmap entries? Or should only the name mismatch part be better handled? Or perhaps both?
Dunno. It certainly can be improved...
Try adding some more logic and see what you come up with.btw:
The most frequent
NO_AUTHOR_SIGN_OFFmessages for v5.7..v5.8 are
Quoted 10 lines
98 WARNING: From:/SoB: email address mismatch: 'From: Daniel Vetter <[email protected]>' != 'Signed-off-by: Daniel Vetter <[email protected]>' 24 WARNING: From:/SoB: email address mismatch: 'From: Thinh Nguyen <[email protected]>' != 'Signed-off-by: Thinh Nguyen <[email protected]>' 19 WARNING: From:/SoB: email address mismatch: 'From: Wolfram Sang <[email protected]>' != 'Signed-off-by: Wolfram Sang <[email protected]>' 11 WARNING: From:/SoB: email address mismatch: 'From: Luke Nelson <[email protected]>' != 'Signed-off-by: Luke Nelson <[email protected]>' 8 WARNING: From:/SoB: email address mismatch: 'From: Christophe Leroy <[email protected]>' != 'Signed-off-by: Christophe Leroy <[email protected]>' 6 WARNING: From:/SoB: email address mismatch: 'From: Davidlohr Bueso <[email protected]>' != 'Signed-off-by: Davidlohr Bueso <[email protected]>' 5 WARNING: Missing Signed-off-by: line by nominal patch author '"Paul A. Clarke" <[email protected]>' 4 WARNING: Missing Signed-off-by: line by nominal patch author 'Huang Ying <[email protected]>' 3 WARNING: Missing Signed-off-by: line by nominal patch author '"Stephan Müller" <[email protected]>'
Great minds think alike.
I did a similar investigation on Friday after the first discussion with Dwaipayan:
I hope Dwaipayan can come up with a '.AUTHOR_SIGN_OFF.mailmap' file that we can use to distinguish the known developers that knowingly and intentionally use different identities vs. the 'newbies' that should validly be warned.
We will see if Dwaipayan can come up with a good idea how to handle that.
Lukas
Quoted 24 lines
For the Missing Signed-off-by: lines above, even after decoding, the email matches but the names do not.
From: "Paul A. Clarke" <[email protected]>
[...]Signed-off-by Paul Clarke <[email protected]>From: Huang Ying <[email protected]>
[...]Signed-off-by "Huang, Ying" <[email protected]>From: =?UTF-8?q?Stephan=20M=C3=BCller?= <[email protected]>
[...]Signed-off-by Stephan Mueller <[email protected]>Quoted 2 lines
Also, I would like to know if there are any more changes required for the current patch or if it is good to go?
I think it's fine.
cheers, Joe
Joe Perches
maintainerQuoted 2 lines · Lukas Bulwahn
Quoted 1 line · Joe Perches
On Sun, 2020-09-20 at 21:52 +0530, Dwaipayan Ray wrote:
[]
Quoted 19 lines
Quoted 19 lines
Quoted 11 lines
For example, Lukas checked commits between v5.4 and v5.8 and he found:
175 Missing Signed-off-by: line by nominal patch authorrep 'Daniel Vetter <[email protected]>'Infact in all of those commits he signed off using a different mail, Daniel Vetter <[email protected]>.
So is it possible to resolve these using perhaps .mailmap entries? Or should only the name mismatch part be better handled? Or perhaps both?
Dunno. It certainly can be improved...
Try adding some more logic and see what you come up with.btw:
The most frequent
NO_AUTHOR_SIGN_OFFmessages for v5.7..v5.8 are
Quoted 9 lines
Quoted 9 lines
98 WARNING: From:/SoB: email address mismatch: 'From: Daniel Vetter <[email protected]>' != 'Signed-off-by: Daniel Vetter <[email protected]>' 24 WARNING: From:/SoB: email address mismatch: 'From: Thinh Nguyen <[email protected]>' != 'Signed-off-by: Thinh Nguyen <[email protected]>' 19 WARNING: From:/SoB: email address mismatch: 'From: Wolfram Sang <[email protected]>' != 'Signed-off-by: Wolfram Sang <[email protected]>' 11 WARNING: From:/SoB: email address mismatch: 'From: Luke Nelson <[email protected]>' != 'Signed-off-by: Luke Nelson <[email protected]>' 8 WARNING: From:/SoB: email address mismatch: 'From: Christophe Leroy <[email protected]>' != 'Signed-off-by: Christophe Leroy <[email protected]>' 6 WARNING: From:/SoB: email address mismatch: 'From: Davidlohr Bueso <[email protected]>' != 'Signed-off-by: Davidlohr Bueso <[email protected]>' 5 WARNING: Missing Signed-off-by: line by nominal patch author '"Paul A. Clarke" <[email protected]>' 4 WARNING: Missing Signed-off-by: line by nominal patch author 'Huang Ying <[email protected]>' 3 WARNING: Missing Signed-off-by: line by nominal patch author '"Stephan Müller" <[email protected]>'
[]
Quoted 4 lines
I hope Dwaipayan can come up with a '.AUTHOR_SIGN_OFF.mailmap' file that we can use to distinguish the known developers that knowingly and intentionally use different identities vs. the 'newbies' that should validly be warned.
I hope not. One .mailmap should be enough.
Quoted 25 lines · Dwaipayan Ray
Checkpatch did not handle cases where the author From: header was split into multiple lines. The author identity could not be resolved and checkpatch generated a false
NO_AUTHOR_SIGN_OFFwarning.A typical example is Commit
e33bcbab16d1("tee: add support for session's client UUID generation"). When checkpatch was run on this commit, it displayed:"WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author ''"
This was due to split header lines not being handled properly and the author himself wrote in Commit
cd2614967d8b("checkpatch: warn if missing author Signed-off-by"):"Split From: headers are not fully handled: only the first part is compared."
Support split From: headers by correctly parsing the header extension lines. RFC 2822, Section-2.2.3 stated that each extended line must start with a WSP character (a space or htab). The solution was therefore to concatenate the lines which start with a WSP to get the correct long header.
Quoted 3 lines
Quoted 19 lines
--- scripts/checkpatch.pl | 4 ++++ 1 file changed, 4 insertions(+)scripts/checkpatch.pl +4
-0@@ -2661,6 +2661,10 @@ sub process { # Check the patch for a From: if (decode("MIME-Header", $line) =~ /^From:\s*(.*)/) { $author = $1; + my $curline = $linenr; + while(defined($rawlines[$curline]) && ($rawlines[$curline++] =~ /^[ \t]\s*(.*)/)) { + $author .= $1; + } $author = encode("utf8", $author) if ($line =~ /=\?utf-8\?/i); $author =~ s/"//g; $author = reformat_email($author);
Dwaipayan, just a quick nitpick:
Your subject line has two spaces in front of 'From:'
Quoted 6 lines · Dwaipayan Ray
Checkpatch did not handle cases where the author From: header was split into multiple lines. The author identity could not be resolved and checkpatch generated a false
NO_AUTHOR_SIGN_OFFwarning.A typical example is Commit
e33bcbab16d1("tee: add support for
You can write 'Commit' lowercase as 'commit'.
Quoted 10 lines
session's client UUID generation"). When checkpatch was run on this commit, it displayed:
"WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author ''"
This was due to split header lines not being handled properly and the author himself wrote in Commit
cd2614967d8b("checkpatch: warn if missing author Signed-off-by"):
Same here.
Quoted 9 lines
"Split From: headers are not fully handled: only the first part is compared."
Support split From: headers by correctly parsing the header extension lines. RFC 2822, Section-2.2.3 stated that each extended line must start with a WSP character (a space or htab). The solution was therefore to concatenate the lines which start with a WSP to get the correct long header.
Quoted 3 lines
On v5.4..v5.7 using data from a previous evaluation, I found 116 commits with
the error message "Missing Signed-off-by: line by nominal patch author ''",
when running ./scripts/checkpatch.pl on v5.9-rc6.
After this patch application, all 116 warnings disappeared with "Missing
Signed-off-by: line by nominal patch author ''"
and these three new warnings appeared:
322f6a3182d42df18059a89c53b09d33919f755e:35: WARNING: Missing Signed-off-by: line by nominal patch author 'Johnson CH Chen <[email protected]>' 18977008f44c66bdd6d20bb432adf71372589303:37: WARNING: Missing Signed-off-by: line by nominal patch author '"Marek Szyprowski via Linux.Kernel.Org" <[email protected]>' ed3520427f57327f581de0cc28c1c30df08f0103:51: WARNING: Missing Signed-off-by: line by nominal patch author 'Chengguang Xu via Linux-f2fs-devel <[email protected]>'
With that said, I think am happy to add my tags:
Dwaipayan, you can fix the minor commit message issues above, add the tags from Joe and me to the end of your commit message and send the patch as v3 out to Andrew Morton with everyone sofar as CC. Andrew Morton will pick up the patch and make it travel to Linus Torvalds.
Good job so far!
After you did that, let us develop and discuss a plan to refine the 'trickier' part of false AUTHOR_SIGN_OFF warnings for developer and maintainers with some known special author and sign-off procedures.
Lukas
Quoted 23 lines
--- scripts/checkpatch.pl | 4 ++++ 1 file changed, 4 insertions(+)scripts/checkpatch.pl +4
-0@@ -2661,6 +2661,10 @@ sub process { # Check the patch for a From: if (decode("MIME-Header", $line) =~ /^From:\s*(.*)/) { $author = $1; + my $curline = $linenr; + while(defined($rawlines[$curline]) && ($rawlines[$curline++] =~ /^[ \t]\s*(.*)/)) { + $author .= $1; + } $author = encode("utf8", $author) if ($line =~ /=\?utf-8\?/i); $author =~ s/"//g; $author = reformat_email($author);Signature · 1 line
2.27.0
Quoted 93 lines · Lukas Bulwahn
Dwaipayan, just a quick nitpick:
Your subject line has two spaces in front of 'From:'
Quoted 6 lines · Dwaipayan Ray
Checkpatch did not handle cases where the author From: header was split into multiple lines. The author identity could not be resolved and checkpatch generated a false
NO_AUTHOR_SIGN_OFFwarning.A typical example is Commit
e33bcbab16d1("tee: add support forYou can write 'Commit' lowercase as 'commit'.
Quoted 10 lines
session's client UUID generation"). When checkpatch was run on this commit, it displayed:
"WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author ''"
This was due to split header lines not being handled properly and the author himself wrote in Commit
cd2614967d8b("checkpatch: warn if missing author Signed-off-by"):Same here.
Quoted 9 lines
"Split From: headers are not fully handled: only the first part is compared."
Support split From: headers by correctly parsing the header extension lines. RFC 2822, Section-2.2.3 stated that each extended line must start with a WSP character (a space or htab). The solution was therefore to concatenate the lines which start with a WSP to get the correct long header.
Quoted 3 lines
On v5.4..v5.7 using data from a previous evaluation, I found 116 commits with
the error message "Missing Signed-off-by: line by nominal patch author ''",
when running./scripts/checkpatch.plon v5.9-rc6.After this patch application, all 116 warnings disappeared with "Missing
Signed-off-by: line by nominal patch author ''"
and these three new warnings appeared:322f6a3182d42df18059a89c53b09d33919f755e:35: WARNING: Missing Signed-off-by: line by nominal patch author 'Johnson CH Chen <[email protected]>' 18977008f44c66bdd6d20bb432adf71372589303:37: WARNING: Missing Signed-off-by: line by nominal patch author '"Marek Szyprowski via Linux.Kernel.Org" <[email protected]>' ed3520427f57327f581de0cc28c1c30df08f0103:51: WARNING: Missing Signed-off-by: line by nominal patch author 'Chengguang Xu via Linux-f2fs-devel <[email protected]>'
With that said, I think am happy to add my tags:
Reviewed-by Lukas Bulwahn <[email protected]>Tested-by Lukas Bulwahn <[email protected]>Dwaipayan, you can fix the minor commit message issues above, add the tags from Joe and me to the end of your commit message and send the patch as v3 out to Andrew Morton with everyone sofar as CC. Andrew Morton will pick up the patch and make it travel to Linus Torvalds.
Good job so far!
After you did that, let us develop and discuss a plan to refine the 'trickier' part of false
AUTHOR_SIGN_OFFwarnings for developer and maintainers with some known special author and sign-off procedures.Lukas
Quoted 15 lines
--- scripts/checkpatch.pl | 4 ++++ 1 file changed, 4 insertions(+)scripts/checkpatch.pl +3
-0@@ -2661,6 +2661,10 @@ sub process { # Check the patch for a From: if (decode("MIME-Header", $line) =~ /^From:\s*(.*)/) { $author = $1; + my $curline = $linenr; + while(defined($rawlines[$curline]) && ($rawlines[$curline++] =~ /^[ \t]\s*(.*)/)) { + $author .= $1;
Quoted 8 lines
Quoted 8 lines
scripts/checkpatch.pl +1
-1+ } $author = encode("utf8", $author) if ($line =~ /=\?utf-8\?/i); $author =~ s/"//g; $author = reformat_email($author); --2.27.0
Hi,
Sure! I will do that and get back to you.
Thanks,
Dwaipayan.