Allow additional tags between Co-developed-by: and Signed-off-by:.
Bump severity of missing SoB to ERROR.
Additional tags between Co-developed-by and corresponding Signed-off-by
could include Reviewed-by tags collected by Submitter, which is also
a Co-developer, but should sign-off at the very end of tags provided by
themself.
Missing SoB is promoted to error while that piece of code is touched.
Two sets of perl %hashes introduced to keep both (int) line numbers and
(string) messages handy for warning reporting, while keeping it correct
across 100+ line long commit messages.
Mateusz Polchlopek <[email protected]> has reported this to me.
Reviewed-by: Jacob Keller <[email protected]>
Signed-off-by: Przemek Kitszel <[email protected]>
---
scripts/checkpatch.pl | 36 +++++++++++++++++++++++-------------
1 file changed, 23 insertions(+), 13 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7d16f863edf1..0400bf092bfa 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2682,6 +2682,10 @@ sub process {
my $suppress_statement = 0;
my %signatures = ();
+ my %signoffs = ();
+ my %signoffs_msg = ();
+ my %codevs = ();
+ my %codevs_msg = ();
# Pre-scan the patch sanitizing the lines.
# Pre-scan the patch looking for any __setup documentation.
@@ -2967,11 +2971,13 @@ sub process {
if ($line =~ /^\s*signed-off-by:\s*(.*)/i) {
$signoff++;
$in_commit_log = 0;
+ my $ctx = $1;
+ $signoffs{$ctx} = $linenr;
+ $signoffs_msg{$ctx} = $herecurr;
if ($author ne '' && $authorsignoff != 1) {
- if (same_email_addresses($1, $author)) {
+ if (same_email_addresses($ctx, $author)) {
$authorsignoff = 1;
} else {
- my $ctx = $1;
my ($email_name, $email_comment, $email_address, $comment1) = parse_email($ctx);
my ($author_name, $author_comment, $author_address, $comment2) = parse_email($author);
@@ -3158,22 +3164,15 @@ sub process {
$signatures{$sig_nospace} = 1;
}
-# Check Co-developed-by: immediately followed by Signed-off-by: with same name and email
+# Collect Co-developed-by: to check if each is backed up by Signed-off-by: with
+# the same name and email. Checks are made after main loop.
if ($sign_off =~ /^co-developed-by:$/i) {
if ($email eq $author) {
WARN("BAD_SIGN_OFF",
"Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . $herecurr);
}
- if (!defined $lines[$linenr]) {
- WARN("BAD_SIGN_OFF",
- "Co-developed-by: must be immediately followed by Signed-off-by:\n" . $herecurr);
- } elsif ($rawlines[$linenr] !~ /^signed-off-by:\s*(.*)/i) {
- WARN("BAD_SIGN_OFF",
- "Co-developed-by: must be immediately followed by Signed-off-by:\n" . $herecurr . $rawlines[$linenr] . "\n");
- } elsif ($1 ne $email) {
- WARN("BAD_SIGN_OFF",
- "Co-developed-by and Signed-off-by: name/email do not match\n" . $herecurr . $rawlines[$linenr] . "\n");
- }
+ $codevs{$email} = $linenr;
+ $codevs_msg{$email} = $herecurr;
}
# check if Reported-by: is followed by a Closes: tag
@@ -7712,6 +7711,17 @@ sub process {
"From:/Signed-off-by: email subaddress mismatch: $sob_msg\n");
}
}
+ # check if each Co-developed-by tag is backed up by Sign-off,
+ # warn if Co-developed-by tag was put after a Signed-off-by tag
+ foreach my $codev (keys %codevs) {
+ if (!$signoffs{$codev}) {
+ ERROR("BAD_SIGN_OFF",
+ "Co-developed-by: must be followed by Signed-off-by:\n" . $codevs_msg{$codev});
+ } elsif ($signoffs{$codev} <= $codevs{$codev}) {
+ WARN("BAD_SIGN_OFF",
+ "Co-developed-by: must be followed by Signed-off-by:, but was placed after it\n" . $signoffs_msg{$codev} . $codevs_msg{$codev});
+ }
+ }
}
print report_dump();
--
2.38.1
On Fri, 2023-10-20 at 15:21 +0200, Przemek Kitszel wrote:
> Allow additional tags between Co-developed-by: and Signed-off-by:.
> Bump severity of missing SoB to ERROR.
>
> Additional tags between Co-developed-by and corresponding Signed-off-by
> could include Reviewed-by tags collected by Submitter, which is also
> a Co-developer, but should sign-off at the very end of tags provided by
> themself.
>
> Missing SoB is promoted to error while that piece of code is touched.
>
> Two sets of perl %hashes introduced to keep both (int) line numbers and
> (string) messages handy for warning reporting, while keeping it correct
> across 100+ line long commit messages.
>
> Mateusz Polchlopek <[email protected]> has reported this to me.
>
> Reviewed-by: Jacob Keller <[email protected]>
> Signed-off-by: Przemek Kitszel <[email protected]>
Unless this is accepted by various process folk,
and the documentation for it updated, I think this
should not be applied.
> ---
> scripts/checkpatch.pl | 36 +++++++++++++++++++++++-------------
> 1 file changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7d16f863edf1..0400bf092bfa 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2682,6 +2682,10 @@ sub process {
> my $suppress_statement = 0;
>
> my %signatures = ();
> + my %signoffs = ();
> + my %signoffs_msg = ();
> + my %codevs = ();
> + my %codevs_msg = ();
>
> # Pre-scan the patch sanitizing the lines.
> # Pre-scan the patch looking for any __setup documentation.
> @@ -2967,11 +2971,13 @@ sub process {
> if ($line =~ /^\s*signed-off-by:\s*(.*)/i) {
> $signoff++;
> $in_commit_log = 0;
> + my $ctx = $1;
> + $signoffs{$ctx} = $linenr;
> + $signoffs_msg{$ctx} = $herecurr;
> if ($author ne '' && $authorsignoff != 1) {
> - if (same_email_addresses($1, $author)) {
> + if (same_email_addresses($ctx, $author)) {
> $authorsignoff = 1;
> } else {
> - my $ctx = $1;
> my ($email_name, $email_comment, $email_address, $comment1) = parse_email($ctx);
> my ($author_name, $author_comment, $author_address, $comment2) = parse_email($author);
>
> @@ -3158,22 +3164,15 @@ sub process {
> $signatures{$sig_nospace} = 1;
> }
>
> -# Check Co-developed-by: immediately followed by Signed-off-by: with same name and email
> +# Collect Co-developed-by: to check if each is backed up by Signed-off-by: with
> +# the same name and email. Checks are made after main loop.
> if ($sign_off =~ /^co-developed-by:$/i) {
> if ($email eq $author) {
> WARN("BAD_SIGN_OFF",
> "Co-developed-by: should not be used to attribute nominal patch author '$author'\n" . $herecurr);
> }
> - if (!defined $lines[$linenr]) {
> - WARN("BAD_SIGN_OFF",
> - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . $herecurr);
> - } elsif ($rawlines[$linenr] !~ /^signed-off-by:\s*(.*)/i) {
> - WARN("BAD_SIGN_OFF",
> - "Co-developed-by: must be immediately followed by Signed-off-by:\n" . $herecurr . $rawlines[$linenr] . "\n");
> - } elsif ($1 ne $email) {
> - WARN("BAD_SIGN_OFF",
> - "Co-developed-by and Signed-off-by: name/email do not match\n" . $herecurr . $rawlines[$linenr] . "\n");
> - }
> + $codevs{$email} = $linenr;
> + $codevs_msg{$email} = $herecurr;
> }
>
> # check if Reported-by: is followed by a Closes: tag
> @@ -7712,6 +7711,17 @@ sub process {
> "From:/Signed-off-by: email subaddress mismatch: $sob_msg\n");
> }
> }
> + # check if each Co-developed-by tag is backed up by Sign-off,
> + # warn if Co-developed-by tag was put after a Signed-off-by tag
> + foreach my $codev (keys %codevs) {
> + if (!$signoffs{$codev}) {
> + ERROR("BAD_SIGN_OFF",
> + "Co-developed-by: must be followed by Signed-off-by:\n" . $codevs_msg{$codev});
> + } elsif ($signoffs{$codev} <= $codevs{$codev}) {
> + WARN("BAD_SIGN_OFF",
> + "Co-developed-by: must be followed by Signed-off-by:, but was placed after it\n" . $signoffs_msg{$codev} . $codevs_msg{$codev});
> + }
> + }
> }
>
> print report_dump();
On 10/21/23 00:34, Joe Perches wrote:
> On Fri, 2023-10-20 at 15:21 +0200, Przemek Kitszel wrote:
>> Allow additional tags between Co-developed-by: and Signed-off-by:.
>> Bump severity of missing SoB to ERROR.
>>
>> Additional tags between Co-developed-by and corresponding Signed-off-by
>> could include Reviewed-by tags collected by Submitter, which is also
>> a Co-developer, but should sign-off at the very end of tags provided by
>> themself.
>>
>> Missing SoB is promoted to error while that piece of code is touched.
>>
>> Two sets of perl %hashes introduced to keep both (int) line numbers and
>> (string) messages handy for warning reporting, while keeping it correct
>> across 100+ line long commit messages.
>>
>> Mateusz Polchlopek <[email protected]> has reported this to me.
>>
>> Reviewed-by: Jacob Keller <[email protected]>
>> Signed-off-by: Przemek Kitszel <[email protected]>
>
> Unless this is accepted by various process folk,
> and the documentation for it updated, I think this
> should not be applied.
I will post v2 with docs updated. Would make it clear in commit message
that immediateness of SoB after CdB was important for humans checking
presence of both manually, and checkpatch has adopted such requirement
for it's own comfort.
On Mon, 2023-10-23 at 11:24 +0200, Przemek Kitszel wrote:
> On 10/21/23 00:34, Joe Perches wrote:
> > On Fri, 2023-10-20 at 15:21 +0200, Przemek Kitszel wrote:
> > > Allow additional tags between Co-developed-by: and Signed-off-by:.
> > > Bump severity of missing SoB to ERROR.
[]
> > Unless this is accepted by various process folk,
> > and the documentation for it updated, I think this
> > should not be applied.
>
> I will post v2 with docs updated. Would make it clear in commit message
> that immediateness of SoB after CdB was important for humans checking
> presence of both manually, and checkpatch has adopted such requirement
> for it's own comfort.
checkpatch does not adopt stuff "for it's own comfort".