checkpatch: allow tags between co-developed-by and their sign-off
Commit Message
Allow additional tags between Co-developed-by: and Signed-off-by:.
Removing the "immediately" word from the doc is a great summary of the
change - there is no need for the two tags to be glued together, barring
ease of checkpatch implementation.
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
the Submitter.
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 <mateusz.polchlopek@intel.com> has reported this to me.
Bump severity of missing SoB to ERROR, while that piece of code needs
touch anyway.
changelog:
v2: update also the doc, slight reword of commitmsg,
added workflows & doc MLs;
Links:
v1: https://lore.kernel.org/all/20231020132156.37882-1-przemyslaw.kitszel@intel.com
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
Signed-off-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
---
Documentation/process/5.Posting.rst | 2 +-
Documentation/process/submitting-patches.rst | 6 ++--
scripts/checkpatch.pl | 36 +++++++++++++-------
3 files changed, 28 insertions(+), 16 deletions(-)
Comments
+Mateusz
On Mon, Oct 23, 2023, Przemek Kitszel wrote:
> 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
> the Submitter.
...
> Mateusz Polchlopek <mateusz.polchlopek@intel.com> has reported this to me.
Heh, there's a tag for that...
Reported-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
And it's usually a good idea to Cc the reporter in case there are questions they
can help answer.
> @@ -509,16 +509,18 @@ Example of a patch submitted by the From: author::
> Signed-off-by: Second Co-Author <second@coauthor.example.org>
> Signed-off-by: From Author <from@author.example.org>
>
> -Example of a patch submitted by a Co-developed-by: author::
> +Example of a patch submitted by a Co-developed-by: author, who also collected
> +a Reviewed-by: tag posted for earlier version::
>
> From: From Author <from@author.example.org>
>
> <changelog>
>
> Co-developed-by: Random Co-Author <random@coauthor.example.org>
> Signed-off-by: Random Co-Author <random@coauthor.example.org>
> Signed-off-by: From Author <from@author.example.org>
> Co-developed-by: Submitting Co-Author <sub@coauthor.example.org>
> + Reviewed-by: Some Reviewer <srev@another.example.org>
> Signed-off-by: Submitting Co-Author <sub@coauthor.example.org>
This is silly. Allowing tags in-between Co-developed-by with Signed-off-by
unnecessarily complicates things, e.g. people already miss/forget the rule about
tightly coupling Co-developed-by with Signed-off-by.
And if we're being super pedantic about chronological history, arguably the
Reviewed-by should come before the Co-developed-by as adding the Reviewed-by is
a (trivial) modification to the patch that was done by the submitter.
Hi Przemek,
On Mon, Oct 23, 2023 at 12:29 PM Przemek Kitszel
<przemyslaw.kitszel@intel.com> wrote:
>
> Allow additional tags between Co-developed-by: and Signed-off-by:.
>
> Removing the "immediately" word from the doc is a great summary of the
> change - there is no need for the two tags to be glued together, barring
> ease of checkpatch implementation.
>
I think the currently suggested process of keeping Co-developed-by and
Signed-off-by glued together is good, and I see no reason why this
should be changed, nor do I see any drawbacks.
> 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
> the Submitter.
>
The other tags, Reviewed-by, etc., can go anywhere just not between
Co-developed-by and corresponding Signed-off-by. So, why do you have
this need to put it exactly there rather than putting it anywhere
else?
The commit message tells me what you are proposing, but there is no
rationale in the commit message and that is put up for discussion here
with the proposed change.
I see many potential areas of work for the checkpatch script, but in
my humble opinion, this really is not one of the rules that needs to
be improved.
Lukas
(...snipped the rest...)
On Mon, 2023-10-23 at 16:16 +0200, Lukas Bulwahn wrote:
> On Mon, Oct 23, 2023 at 12:29 PM Przemek Kitszel <przemyslaw.kitszel@intel.com> wrote:
> > Allow additional tags between Co-developed-by: and Signed-off-by:.
I think this unnecessary and not particularly useful as well.
>
> I see many potential areas of work for the checkpatch script
List them please.
On 10/23/23 16:02, Sean Christopherson wrote:
> +Mateusz
>
> On Mon, Oct 23, 2023, Przemek Kitszel wrote:
>> 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
>> the Submitter.
>
> ...
>
>> Mateusz Polchlopek <mateusz.polchlopek@intel.com> has reported this to me.
>
> Heh, there's a tag for that...
>
> Reported-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>
> And it's usually a good idea to Cc the reporter in case there are questions they
> can help answer.
Heh ;) then I would get a checkpatch warning for not providing Link: to
the report, somehow I wanted to avoid those for checkpatch contrib :)
>
>> @@ -509,16 +509,18 @@ Example of a patch submitted by the From: author::
>> Signed-off-by: Second Co-Author <second@coauthor.example.org>
>> Signed-off-by: From Author <from@author.example.org>
>>
>> -Example of a patch submitted by a Co-developed-by: author::
>> +Example of a patch submitted by a Co-developed-by: author, who also collected
>> +a Reviewed-by: tag posted for earlier version::
>>
>> From: From Author <from@author.example.org>
>>
>> <changelog>
>>
>> Co-developed-by: Random Co-Author <random@coauthor.example.org>
>> Signed-off-by: Random Co-Author <random@coauthor.example.org>
>> Signed-off-by: From Author <from@author.example.org>
>> Co-developed-by: Submitting Co-Author <sub@coauthor.example.org>
>> + Reviewed-by: Some Reviewer <srev@another.example.org>
>> Signed-off-by: Submitting Co-Author <sub@coauthor.example.org>
>
> This is silly. Allowing tags in-between Co-developed-by with Signed-off-by
> unnecessarily complicates things, e.g. people already miss/forget the rule about
> tightly coupling Co-developed-by with Signed-off-by.
Meh, I see that as a pure process simplification with proposed rule
being almost the same as the current one, thus as easy to remember or
forget.
>
> And if we're being super pedantic about chronological history, arguably the
> Reviewed-by should come before the Co-developed-by as adding the Reviewed-by is
> a (trivial) modification to the patch that was done by the submitter.
Tagging patches is not considered co-development by most people.
On 10/23/23 16:16, Lukas Bulwahn wrote:
> Hi Przemek,
>
> On Mon, Oct 23, 2023 at 12:29 PM Przemek Kitszel
> <przemyslaw.kitszel@intel.com> wrote:
>>
>> Allow additional tags between Co-developed-by: and Signed-off-by:.
>>
>> Removing the "immediately" word from the doc is a great summary of the
>> change - there is no need for the two tags to be glued together, barring
>> ease of checkpatch implementation.
>>
>
> I think the currently suggested process of keeping Co-developed-by and
> Signed-off-by glued together is good, and I see no reason why this
> should be changed, nor do I see any drawbacks.
>
>
>> 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
>> the Submitter.
>>
>
> The other tags, Reviewed-by, etc., can go anywhere just not between
> Co-developed-by and corresponding Signed-off-by. So, why do you have
> this need to put it exactly there rather than putting it anywhere
> else?
Multiple times during review it was odd for me to look at thw SoB of
submitter not being the last thing, and that's the result of the current
rule - co-dev authors put collected RB as last thing, only to keep their
CdB and SoB together.
>
> The commit message tells me what you are proposing, but there is no
> rationale in the commit message and that is put up for discussion here
> with the proposed change.
>
> I see many potential areas of work for the checkpatch script, but in
> my humble opinion, this really is not one of the rules that needs to
> be improved.
I started the other way, identified what was pissing me off, then tried
to fix that, despite of requirement of writing in perl.
>
> Lukas
>
> (...snipped the rest...)
On 24/10/2023 11:15, Przemek Kitszel wrote:
> On 10/23/23 16:02, Sean Christopherson wrote:
>> +Mateusz
>>
>> On Mon, Oct 23, 2023, Przemek Kitszel wrote:
>>> 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
>>> the Submitter.
>>
>> ...
>>
>>> Mateusz Polchlopek <mateusz.polchlopek@intel.com> has reported this to me.
>>
>> Heh, there's a tag for that...
>>
>> Reported-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>>
>> And it's usually a good idea to Cc the reporter in case there are questions they
>> can help answer.
>
> Heh ;) then I would get a checkpatch warning for not providing Link: to
> the report, somehow I wanted to avoid those for checkpatch contrib :)
You wanted Suggested-by. There is no bug here, so Reported-by is not
suitable.
Best regards,
Krzysztof
On 10/29/23 10:35, Krzysztof Kozlowski wrote:
> On 24/10/2023 11:15, Przemek Kitszel wrote:
>> On 10/23/23 16:02, Sean Christopherson wrote:
>>> +Mateusz
>>>
>>> On Mon, Oct 23, 2023, Przemek Kitszel wrote:
>>>> 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
>>>> the Submitter.
>>>
>>> ...
>>>
>>>> Mateusz Polchlopek <mateusz.polchlopek@intel.com> has reported this to me.
>>>
>>> Heh, there's a tag for that...
>>>
>>> Reported-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>>>
>>> And it's usually a good idea to Cc the reporter in case there are questions they
>>> can help answer.
>>
>> Heh ;) then I would get a checkpatch warning for not providing Link: to
>> the report, somehow I wanted to avoid those for checkpatch contrib :)
>
> You wanted Suggested-by. There is no bug here, so Reported-by is not
> suitable.
>
> Best regards,
> Krzysztof
>
I would really like to see Inspired-by: and use it a lot, for cases that
some talk ignites an idea, but it's Suggested-by
@@ -243,7 +243,7 @@ The tags in common use are:
- Co-developed-by: states that the patch was co-created by several developers;
it is a used to give attribution to co-authors (in addition to the author
attributed by the From: tag) when multiple people work on a single patch.
- Every Co-developed-by: must be immediately followed by a Signed-off-by: of
+ Every Co-developed-by: must be followed by a Signed-off-by: of
the associated co-author. Details and examples can be found in
:ref:`Documentation/process/submitting-patches.rst <submittingpatches>`.
@@ -489,7 +489,7 @@ have been included in the discussion.
Co-developed-by: states that the patch was co-created by multiple developers;
it is used to give attribution to co-authors (in addition to the author
attributed by the From: tag) when several people work on a single patch. Since
-Co-developed-by: denotes authorship, every Co-developed-by: must be immediately
+Co-developed-by: denotes authorship, every Co-developed-by: must be
followed by a Signed-off-by: of the associated co-author. Standard sign-off
procedure applies, i.e. the ordering of Signed-off-by: tags should reflect the
chronological history of the patch insofar as possible, regardless of whether
@@ -509,16 +509,18 @@ Example of a patch submitted by the From: author::
Signed-off-by: Second Co-Author <second@coauthor.example.org>
Signed-off-by: From Author <from@author.example.org>
-Example of a patch submitted by a Co-developed-by: author::
+Example of a patch submitted by a Co-developed-by: author, who also collected
+a Reviewed-by: tag posted for earlier version::
From: From Author <from@author.example.org>
<changelog>
Co-developed-by: Random Co-Author <random@coauthor.example.org>
Signed-off-by: Random Co-Author <random@coauthor.example.org>
Signed-off-by: From Author <from@author.example.org>
Co-developed-by: Submitting Co-Author <sub@coauthor.example.org>
+ Reviewed-by: Some Reviewer <srev@another.example.org>
Signed-off-by: Submitting Co-Author <sub@coauthor.example.org>
@@ -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();