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

Lukas Bulwahn lukas.bulwahn at gmail.com
Tue Dec 1 11:34:21 UTC 2020


On Tue, Dec 1, 2020 at 11:27 AM Lukas Bulwahn <lukas.bulwahn at gmail.com> wrote:
>
> On Tue, Dec 1, 2020 at 10:00 AM Aditya <yashsri421 at gmail.com> wrote:
> >
> > On 29/11/20 1:44 pm, Aditya Srivastava 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 acknowledgment 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 acknowledgment
> > > for the same
> > > E.g., Pointed-at-by: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> > >
> > > 14) Suggestions-by (count: 3) => Suggested-by
> > >
> > > Provide a fix by replacing the non-standard signature with its standard
> > > equivalent.
> > >
> > > Also, improve warning messages correspondingly, providing suitable
> > > rationale to the user for the suggestion made.
> > >
> > > Signed-off-by: Aditya Srivastava <yashsri421 at gmail.com>
> > > ---
> > > applies on next-20201120 and my last patch at Link: https://lore.kernel.org/linux-kernel-mentees/db1195235752685fc85fb52ecb1b1af3f35b5394.camel@perches.com/T/#u
> > >
> > > 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
> > >
> > > changes in v4: modify rationale for certain suggestions
> > >
> > > changes in v5: remove the tag deletion suggestions, ie "Generated-by" and "Celebrated-by"; rebase on last accepted changes; modify commit message
> > >
> > >  scripts/checkpatch.pl | 73 ++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 72 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index 4a026926139f..d0c2f189272f 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -563,6 +563,72 @@ sub find_standard_signature {
> > >
> > >       return "";
> > >  }
> > > +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 is the standard signature tag to attribute multiple authors for a patch",
> > > +     },
> > > +     "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 Development, so 'Co-developed-by' is perfectly fine, even if contributor did not create code",
> > > +     },
> > > +     "Noticed-by:" => {
> > > +             suggestion => "Reported-by:",
> > > +             rationale => "Reported-by is the standard signature tag for acknowledging user who noticed or reported any bug(s)",
> > > +     },
> > > +     "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 is the standard signature tag to attribute user for verifying/testing the patch",
> > > +     },
> > > +     "Okay-ished-by:" => {
> > > +             suggestion => "Acked-by:",
> > > +             rationale => "Acked-by is the standard signature tag for recording one's approval",
> > > +     },
> > > +     "Acked-for-MFD-by:" => {
> > > +             suggestion => "Acked-by:",
> > > +             rationale => "Acked-by is the standard signature tag for recording one's approval",
> > > +     },
> > > +     "Reviewed-off-by:" => {
> > > +             suggestion => "Reviewed-by:",
> > > +             rationale => "Reviewed-by is the standard signature tag to indicate that the patch has been reviewed",
> > > +     },
> > > +     "Proposed-by:" => {
> > > +             suggestion => "Suggested-by:",
> > > +             rationale => "Suggested-by is the standard signature tag for acknowledging user for their suggestions",
> > > +     },
> > > +     "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",
> > > +     },
> > > +);
> > >
> > >  our @typeListMisordered = (
> > >       qr{char\s+(?:un)?signed},
> > > @@ -2832,12 +2898,17 @@ sub process {
> > >
> > >                       if ($sign_off !~ /$signature_tags/) {
> > >                               my $suggested_signature = find_standard_signature($sign_off);
> > > +                             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);
> > >                               } else {
> > >                                       if (WARN("BAD_SIGN_OFF",
> > > -                                              "Non-standard signature: '$sign_off' - perhaps '$suggested_signature'?\n" . $herecurr) &&
> > > +                                              "Non-standard signature: '$sign_off' - perhaps '$suggested_signature'? $rationale\n" . $herecurr) &&
> > >                                           $fix) {
> > >                                               $fixed[$fixlinenr] =~ s/$sign_off/$suggested_signature/;
> > >                                       }
> > >
> >
> > Hi Lukas
> > You probably missed this patch. Actually I wanted to get a quick
> > feedback from you before sending it to Joe again.
> >
>
> Sorry, looks good and can go to Joe. It could be that Joe is more
> picky; so, we might just choose the very obvious mappings after the
> discussion with him.
>

Maybe it is better to just start with a patch on the very obvious
"replacements".

E.g. only keep a very minimal set such as:

Co-authored-by (count: 43) => Co-developed-by
Reviewed-off-by (count: 5) => Reviewed-by
Proposed-by (count: 5) => Suggested-by
Suggestions-by (count: 3) => Suggested-by

But let us see what Joe thinks... if he does not accept the full list,
we can possibly agree on a smaller list like this one.

Lukas


More information about the Linux-kernel-mentees mailing list