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

[Lukas Bulwahn <lukas.bulwahn@gmail.com>]

On Mon, Nov 23, 2020 at 4:16 PM Aditya <yashsri421@gmail.com> wrote:
>
> On 23/11/20 6:39 pm, Lukas Bulwahn wrote:
> > On Mon, Nov 23, 2020 at 1:21 PM Aditya Srivastava <yashsri421@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@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@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?
>

Let us give Joe time to review the patch first; then if accepted, we
can continue the discussion on those tags above; you can already
propose a solution you have in mind and think about where the
documentation, checkpatch etc. needs to be adjusted.

> > 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
> >>
>
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees