[Linux-kernel-mentees] [PATCH v3] checkpatch: add fix and improve warning msg for Non-standard signature

Aditya yashsri421 at gmail.com
Mon Nov 23 15:16:06 UTC 2020


On 23/11/20 6:39 pm, Lukas Bulwahn wrote:
> On Mon, Nov 23, 2020 at 1:21 PM Aditya Srivastava <yashsri421 at gmail.com> wrote:
>>
>> Currently, checkpatch.pl warns for BAD_SIGN_OFF on non-standard signature
>> styles.
>>
>> This warning occurs because of incorrect use of signature tags,
>> e.g. an evaluation on v4.13..v5.8 showed the use of following incorrect
>> signature tags, which may seem correct, but are not standard:
>>
>> 1) Requested-by (count: 48) => Suggested-by
>> Rationale: In an open-source project, there are no 'requests', just
>> 'suggestions' to convince a maintainer to accept your patch
>>
>> 2) Co-authored-by (count: 43) => Co-developed-by
>> Rationale: Co-developed-by and Co-authored-by are synonyms
>>
>> 3) Analyzed-by (count: 22) / Analysed-by (count: 5) => Co-developed-by
>> Rationale: Analyzing is a part of Software Development, so
>> 'Co-developed-by' is perfectly fine, even if contributor did not create
>> code
>>
>> 4) Improvements-by (count: 19) => Co-developed-by
>>
>> 5) Noticed-by (count: 11) => Reported-by
>>
>> 6) Inspired-by (count: 11) => Suggested-by
>>
>> 7) Verified-by (count: 8) => Tested-by
>> Rationale: Used by a single user. On reading mailing list, it seems
>> Tested-by might be a suitable alternative
>>
>> 8) Okay-ished-by (count: 8) => Acked-by
>> Rationale: Used by a single user. On reading mailing list, it seems
>> Acked-by must be suitable alternative
>>
>> 9) Acked-for-MFD-by (count: 6) => Acked-by
>>
>> 10) Reviewed-off-by (count: 5) => Reviewed-by
>>
>> 11) Proposed-by (count: 5) => Suggested-by
>> Rationale: On observing the mailing list, this tag is always used for a
>> maintainer. It seems that the changes might have been suggested by them
>> and the tag is used as acknowledgement for the same
>>
>> 12) Fixed-by (count: 3) => Co-developed-by
>> Rationale: Fixing bug is a part of Software Development, so
>> 'Co-developed-by' is perfectly fine, even if contributor did not create
>> code
>>
>> 13) Pointed-out-by (count: 3) / Pointed-at-by (count: 2) => Suggested-by
>> Rationale: The tags are used for maintainers. It seems that the changes
>> might have been suggested by them and the tag is used as acknowledgement
>> for the same
>> E.g., Pointed-at-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
>>
>> 14) Suggestions-by (count: 3) => Suggested-by
>>
>> 15) Generated-by (count: 17) => remove the tag
>> On observing the mailing list, this tag is always used for quoting the
>> tool or script, which might have been used to generate the patch.
>> E.g. Generated-by: scripts/coccinelle/api/alloc/kzalloc-simple.cocci
>>
>> 16) Celebrated-by (count: 3) => remove the tag
>> This tag was used for only one commit. On observing mailing list, it seem
>> like the celebration for a particular patch and changes.
>>
>> Provide a fix by:
>> 1) replacing the non-standard signature with its standard equivalent
>> 2) removing the signature if it is not required
>>
>> Also, improve warning messages correspondingly, providing users
>> suggestions to either replace or remove the signature. Also provide
>> suitable rationale to the user for the suggestion made.
>>
>> Signed-off-by: Aditya Srivastava <yashsri421 at gmail.com>
> 
> 
> Looks good to me. Let us propose this to Joe for review.
> 
Sent it. We probably still need to discuss about the remaining signatures:
1) "Debugged-by", 61
2) "Originally-by", 39
3) "Bisected-by", 20
4) "Diagnosed-by", 11
5) "Original-patch-by", 11
6) "Based-on-patch-by", 7
7) "Based-on-a-patch-by", 8
8) "Root-caused-by", 6
9) "Original-by", 6
10) "Based-on-patches-by", 5
11) "Based-on-work-by", 5

Should I send this list as a follow-up mail?

> Also, you can already start working on the related feature for
> providing fixes based on the edit distance.
> 
Okay.

Thanks
Aditya
> With both features, we can probably fix all non-standard signatures...
> 
> Lukas
> 
>> ---
>> changes in v2: replace commit specific example with brief evaluation
>>
>> changes in v3: provide rationale to users for every signature tag suggestion;
>> modify commit message describing arrival to conclusion in a structured way
>>
>>  scripts/checkpatch.pl | 101 +++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 99 insertions(+), 2 deletions(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index fdfd5ec09be6..575ff8efb0eb 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -506,6 +506,81 @@ our $signature_tags = qr{(?xi:
>>         Cc:
>>  )};
>>
>> +our %standard_signature_fix = (
>> +       "Requested-by:" => {
>> +               suggestion => "Suggested-by:",
>> +               rationale => "In an open-source project, there are no 'requests', just 'suggestions' to convince a maintainer to accept your patch",
>> +       },
>> +       "Co-authored-by:" => {
>> +               suggestion => "Co-developed-by:",
>> +               rationale => "Co-developed-by and Co-authored-by are synonyms",
>> +       },
>> +       "Analyzed-by:" => {
>> +               suggestion => "Co-developed-by:",
>> +               rationale => "Analyzing is a part of Software Development, so 'Co-developed-by' is perfectly fine, even if contributor did not create code",
>> +       },
>> +       "Analysed-by:" => {
>> +               suggestion => "Co-developed-by:",
>> +               rationale => "Analysing is a part of Software Development, so 'Co-developed-by' is perfectly fine, even if contributor did not create code",
>> +       },
>> +       "Improvements-by:" => {
>> +               suggestion => "Co-developed-by:",
>> +               rationale => "Performing improvements are a part of Software Developement, so 'Co-developed-by' is perfectly fine, even if contributor did not create code",
>> +       },
>> +       "Noticed-by:" => {
>> +               suggestion => "Reported-by:",
>> +               rationale => "Reported-by and Noticed-by are synonyms",
>> +       },
>> +       "Inspired-by:" => {
>> +               suggestion => "Suggested-by:",
>> +               rationale => "Suggested-by is the standard signature tag for acknowledging user for their suggestions",
>> +       },
>> +       "Verified-by:" => {
>> +               suggestion => "Tested-by:",
>> +               rationale => "Tested-by and Verified-by are synonyms",
>> +       },
>> +       "Okay-ished-by:" => {
>> +               suggestion => "Acked-by:",
>> +               rationale => "Acked-by is the standard signature tag for recording your approval",
>> +       },
>> +       "Acked-for-MFD-by:" => {
>> +               suggestion => "Acked-by:",
>> +               rationale => "Acked-by is the standard signature tag for recording your approval",
>> +       },
>> +       "Reviewed-off-by:" => {
>> +               suggestion => "Reviewed-by:",
>> +               rationale => "Reviewed-by is the standard signature tag for recording your approval",
>> +       },
>> +       "Proposed-by:" => {
>> +               suggestion => "Suggested-by:",
>> +               rationale => "Proposing changes is same as suggesting changes, so Suggested-by seems perfectly fine",
>> +       },
>> +       "Fixed-by:" => {
>> +               suggestion => "Co-developed-by:",
>> +               rationale => "Fixing bug is a part of Software Development, so 'Co-developed-by' is perfectly fine, even if contributor did not create code",
>> +       },
>> +       "Pointed-out-by:" => {
>> +               suggestion => "Suggested-by:",
>> +               rationale => "Pointing out certain changes is synonymous to suggesting changes, so Suggested-by seems perfectly fine",
>> +       },
>> +       "Pointed-at-by:" => {
>> +               suggestion => "Suggested-by:",
>> +               rationale => "Pointing at certain changes is synonymous to suggesting changes, so Suggested-by seems perfectly fine",
>> +       },
>> +       "Suggestions-by:" => {
>> +               suggestion => "Suggested-by:",
>> +               rationale => "Suggested-by is the standard signature tag for acknowledging user for their suggestions",
>> +       },
>> +       "Generated-by:" => {
>> +               suggestion => "remove",
>> +               rationale => "Signature tags are used to acknowledge users for their contributions. It is advised to describe about tools in commit description instead",
>> +       },
>> +       "Celebrated-by:" => {
>> +               suggestion => "remove",
>> +               rationale => "Signature tags are used to acknowledge users for their contributions. This tag may not be required at all",
>> +       },
>> +);
>> +
>>  our @typeListMisordered = (
>>         qr{char\s+(?:un)?signed},
>>         qr{int\s+(?:(?:un)?signed\s+)?short\s},
>> @@ -2773,8 +2848,30 @@ sub process {
>>                         my $ucfirst_sign_off = ucfirst(lc($sign_off));
>>
>>                         if ($sign_off !~ /$signature_tags/) {
>> -                               WARN("BAD_SIGN_OFF",
>> -                                    "Non-standard signature: $sign_off\n" . $herecurr);
>> +                               my $suggested_signature = "";
>> +                               my $rationale = "";
>> +                               if (exists($standard_signature_fix{$sign_off})) {
>> +                                       $suggested_signature = $standard_signature_fix{$sign_off}{'suggestion'};
>> +                                       $rationale = $standard_signature_fix{$sign_off}{'rationale'};
>> +                               }
>> +                               if ($suggested_signature eq "") {
>> +                                       WARN("BAD_SIGN_OFF",
>> +                                            "Non-standard signature: $sign_off\n" . $herecurr);
>> +                               }
>> +                               elsif ($suggested_signature eq "remove") {
>> +                                       if (WARN("BAD_SIGN_OFF",
>> +                                               "Non-standard signature: $sign_off. Please consider removing this signature tag. $rationale\n" . $herecurr) &&
>> +                                       $fix) {
>> +                                               fix_delete_line($fixlinenr, $rawline);
>> +                                       }
>> +                               }
>> +                               else {
>> +                                       if (WARN("BAD_SIGN_OFF",
>> +                                               "Non-standard signature: $sign_off. Please use '$suggested_signature' instead. $rationale\n" . $herecurr) &&
>> +                                       $fix) {
>> +                                               $fixed[$fixlinenr] =~ s/$sign_off/$suggested_signature/;
>> +                                       }
>> +                               }
>>                         }
>>                         if (defined $space_before && $space_before ne "") {
>>                                 if (WARN("BAD_SIGN_OFF",
>> --
>> 2.17.1
>>



More information about the Linux-kernel-mentees mailing list