Messages in this thread | | | From | Lukas Bulwahn <> | Date | Mon, 23 Nov 2020 16:16:17 +0100 | Subject | Re: [PATCH v3] checkpatch: add fix and improve warning msg for Non-standard signature |
| |
On Mon, Nov 23, 2020 at 4:04 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 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@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> > --- > 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..f402c9c3958f 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", > + },
Maybe this reads better: Co-developed-by is the standard tag for patches developed by multiple authors.
> + "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 and Noticed-by are synonyms", > + },
Similarly:
Reported-by is the standard tag for denoting any notice or report.
> + "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", > + },
Similarly here.
> + "Okay-ished-by:" => { > + suggestion => "Acked-by:", > + rationale => "Acked-by is the standard signature tag for recording your approval", > + },
I think "your" is wrong here; the person that runs checkpatch may not be the person providing the approval.
So maybe:
- for recording one's approval.
or: - for recording acknowledgement.
> + "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", > + },
for recording the review.
> + "Proposed-by:" => { > + suggestion => "Suggested-by:", > + rationale => "Proposing changes is same as suggesting changes, so Suggested-by seems perfectly fine", > + },
Similarly, the sentence as above could be used.
> + "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 >
| |