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

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

[Aditya Srivastava]

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

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@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/;
 					}
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Aditya]

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@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@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.

Thanks
Aditya
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Lukas Bulwahn]

On Tue, Dec 1, 2020 at 10:00 AM Aditya <yashsri421@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@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@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.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Lukas Bulwahn]

On Tue, Dec 1, 2020 at 11:27 AM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>
> On Tue, Dec 1, 2020 at 10:00 AM Aditya <yashsri421@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@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@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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Aditya Srivastava]

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

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@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/;
 					}
-- 
2.17.1

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Joe Perches]

On Tue, 2020-12-01 at 16:59 +0530, 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:

I'm not a fan of this patch.

There is already a "non-standard" signature warning for
all of these cases since 2012, predating the range of this
retrospective evaluation by over 5 years and yet these
existing commits have been accepted.

The value in actual standardization and effectively
requiring specific signature style tags is quite low.

Anyone that signed a thing a particular way should be free
to sign the thing as they choose.

Most of these warnings would also still be in the tree in
the future in new patches as running checkpatch without
it emitting a message of any type isn't a requirement nor
should checkpatch use actually be required workflow.


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Lukas Bulwahn]

On Tue, Dec 1, 2020 at 6:24 PM Joe Perches <joe@perches.com> wrote:
>
> On Tue, 2020-12-01 at 16:59 +0530, 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:
>
> I'm not a fan of this patch.
>
> There is already a "non-standard" signature warning for
> all of these cases since 2012, predating the range of this
> retrospective evaluation by over 5 years and yet these
> existing commits have been accepted.
>
> The value in actual standardization and effectively
> requiring specific signature style tags is quite low.
>
> Anyone that signed a thing a particular way should be free
> to sign the thing as they choose.
>
> Most of these warnings would also still be in the tree in
> the future in new patches as running checkpatch without
> it emitting a message of any type isn't a requirement nor
> should checkpatch use actually be required workflow.
>

Can we scale this fixing feature down to the very obvious synonyms
that simply do not add anything but confusion?

Such as for those four here:

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

Then, we can probably also drop the rationale because it is pretty clear.

Of course, the impact might be really zero, given that it is unclear
if those authors did actually ever run checkpatch in the first place.

Joe, if you see no value in even such a minimal fix feature, let us
drop that idea and move on. There are enough other things to work on.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Joe Perches]

On Tue, 2020-12-01 at 19:21 +0100, Lukas Bulwahn wrote:
> On Tue, Dec 1, 2020 at 6:24 PM Joe Perches <joe@perches.com> wrote:
> > 
> > On Tue, 2020-12-01 at 16:59 +0530, 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:
> > 
> > I'm not a fan of this patch.
> > 
> > There is already a "non-standard" signature warning for
> > all of these cases since 2012, predating the range of this
> > retrospective evaluation by over 5 years and yet these
> > existing commits have been accepted.
> > 
> > The value in actual standardization and effectively
> > requiring specific signature style tags is quite low.
> > 
> > Anyone that signed a thing a particular way should be free
> > to sign the thing as they choose.
> > 
> > Most of these warnings would also still be in the tree in
> > the future in new patches as running checkpatch without
> > it emitting a message of any type isn't a requirement nor
> > should checkpatch use actually be required workflow.
> > 
> 
> Can we scale this fixing feature down to the very obvious synonyms
> that simply do not add anything but confusion?
> 
> Such as for those four here:
> 
> Co-authored-by (count: 43) => Co-developed-by

I've never been a big fan of "Co-developed-by" as a signature tag,
but a "this should be that" here could be ok.

> Reviewed-off-by (count: 5) => Reviewed-by

I don't see value.  If no one notices a BAD_SIGN_OFF
for the Reviewed-off-by:, I doubt this would add anything.

> Proposed-by (count: 5) => Suggested-by
> Suggestions-by (count: 3) => Suggested-by

Suggestions-by is not suggested-by as these suggestions could
have been in response to an initial patch proposal and the
author could have incorporated those suggestions.

> Then, we can probably also drop the rationale because it is pretty clear.
> 
> Of course, the impact might be really zero, given that it is unclear
> if those authors did actually ever run checkpatch in the first place.
> 
> Joe, if you see no value in even such a minimal fix feature, let us
> drop that idea and move on. There are enough other things to work on.

Maybe only add the Co-authored-by: -> Co-developed-by: check.

But IMO: none of this is particularly useful.


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Joe Perches]

On Sat, 2020-11-28 at 18:35 +0530, Aditya Srivastava wrote:
> Currently checkpatch warns for BAD_SIGN_OFF on non-standard signature
> styles.
> 
> A large number of these warnings occur because of typo mistakes in
> signature tags. An evaluation over v4.13..v5.8 showed that out of 539
> warnings due to non-standard signatures, 87 are due to typo mistakes.
> 
> Following are the standard signature tags which are often incorrectly
> used, along with their individual counts of incorrect use (over
> v4.13..v5.8):
> 
>  Reviewed-by: 42
>  Signed-off-by: 25
>  Reported-by: 6
>  Acked-by: 4
>  Tested-by: 4
>  Suggested-by: 4
> 
> Provide a fix by calculating levenshtein distance for the signature tag
> with all the standard signatures and suggest a fix with a signature, whose
> edit distance is less than or equal to 2 with the misspelled signature.
> 
> Out of the 86 mispelled signatures fixed with this approach, 85 were
> found to be good corrections and 1 was bad correction.
> 
> Following was found to be a bad correction:
>  Tweeted-by (count: 1) => Tested-by
> 
> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
> changes in v2: modify commit message: replace specific example with overall evaluation, minor changes
> 
> changes in v3: summarize commit message
> 
> changes in v4: improve commit message; remove signature suggestions of small length (ie 'cc' and 'to')

Seems OKish but this needs style modifications as there are
several whitespace uses that don't match the typical forms
and perhaps some new function naming could be improved.

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -506,6 +506,77 @@ our $signature_tags = qr{(?xi:
>  	Cc:
>  )};
>  
> 
> +sub get_min {

probably a poor name choice.  Maybe edit_distance_min

> +	my (@arr) = @_;
> +	my $len = scalar @arr;
> +	if((scalar @arr) < 1) {

space after if

> +		# if underflow, return
> +		return;
> +	}
> +	my $min = $arr[0];
> +	for my $i (0 .. ($len-1)) {
> +		if ($arr[$i] < $min) {
> +			$min = $arr[$i];
> +		}
> +	}
> +	return $min;
> +}
> +
> +sub get_edit_distance {
> +	my ($str1, $str2) = @_;

maybe lc($str) =~ s/-//g; here instead of the code in the caller

> +	my $len1 = length($str1);
> +	my $len2 = length($str2);
> +	# two dimensional array storing minimum edit distance
> +	my @distance;
> +	for my $i (0 .. $len1) {
> +		for my $j (0 .. $len2) {
> +			if ($i == 0) {
> +				$distance[$i][$j] = $j;
> +			}
> +			elsif ($j == 0) {

} elsif {

> +				$distance[$i][$j] = $i;
> +			}
> +			elsif (substr($str1, $i-1, 1) eq substr($str2, $j-1, 1)) {
> +				$distance[$i][$j] = $distance[$i - 1][$j - 1];
> +			}
> +			else {

} else {

> +				my $dist1 = $distance[$i][$j - 1]; #insert distance
> +				my $dist2 = $distance[$i - 1][$j]; # remove
> +				my $dist3 = $distance[$i - 1][$j - 1]; #replace
> +				$distance[$i][$j] = 1 + get_min($dist1, $dist2, $dist3);
> +			}
> +		}
> +	}
> +	return $distance[$len1][$len2];
> +}
> +
> +sub get_standard_signature {

find_standard_signature ?

> +	my ($sign_off) = @_;
> +	$sign_off = lc($sign_off);
> +	$sign_off =~ s/\-//g; # to match with formed hash

why not strip the dashes in get_edit_distance instead
of using this weird dance with dashes here?

> +	my @standard_signature_tags = (
> +		'signed-off-by:', 'co-developed-by:', 'acked-by:', 'tested-by:',
> +		'reviewed-by:', 'reported-by:', 'suggested-by:'
> +	);
> +	# setting default values
> +	my $standard_signature = 'signed-off-by';

why is does this need to be given a value?

> +	my $min_edit_distance = 20;
> +	my $edit_distance;
> +	foreach (@standard_signature_tags) {
> +		my $signature = $_;
> +		$_ =~ s/\-//g;

and this dancing here

> +		$edit_distance = get_edit_distance($sign_off, $_);
> +		if ($edit_distance < $min_edit_distance) {
> +			$min_edit_distance = $edit_distance;
> +			$standard_signature = $signature;
> +		}
> +	}
> +        if($min_edit_distance<=2) {

bad indentation, if (, spaces around test <=

> +		return ucfirst($standard_signature);
> +        }

bad indentation

> +	return "";
> +}
> +
>  our @typeListMisordered = (
>  	qr{char\s+(?:un)?signed},
>  	qr{int\s+(?:(?:un)?signed\s+)?short\s},
> @@ -2773,8 +2844,18 @@ 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 = get_standard_signature($sign_off);
> +				if ($suggested_signature eq "") {
> +					WARN("BAD_SIGN_OFF",
> +					"Non-standard signature: $sign_off\n" . $herecurr);

bad alignment

> +				}
> +				else {

} else {

> +					if (WARN("BAD_SIGN_OFF",
> +						 "Non-standard signature: $sign_off. Please use '$suggested_signature' instead\n" . $herecurr) &&

"perhaps" rather than "please use" or "likely typo of"

> +					    $fix) {
> +						$fixed[$fixlinenr] =~ s/$sign_off/$suggested_signature/;
> +					}
> +				}
>  			}
>  			if (defined $space_before && $space_before ne "") {
>  				if (WARN("BAD_SIGN_OFF",


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

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

[Aditya Srivastava]

Currently checkpatch warns for BAD_SIGN_OFF on non-standard signature
styles.

A large number of these warnings occur because of typo mistakes in
signature tags. An evaluation over v4.13..v5.8 showed that out of 539
warnings due to non-standard signatures, 87 are due to typo mistakes.

Following are the standard signature tags which are often incorrectly
used, along with their individual counts of incorrect use (over
v4.13..v5.8):

 Reviewed-by: 42
 Signed-off-by: 25
 Reported-by: 6
 Acked-by: 4
 Tested-by: 4
 Suggested-by: 4

Provide a fix by calculating levenshtein distance for the signature tag
with all the standard signatures and suggest a fix with a signature, whose
edit distance is less than or equal to 2 with the misspelled signature.

Out of the 86 mispelled signatures fixed with this approach, 85 were
found to be good corrections and 1 was bad correction.

Following was found to be a bad correction:
 Tweeted-by (count: 1) => Tested-by

Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
---
applies on next-20201120

changes in v2: modify commit message: replace specific example with overall evaluation, minor changes

changes in v3: summarize commit message

changes in v4: improve commit message; remove signature suggestions of small length (ie 'cc' and 'to')

changes in v5: modify coding styles: improve function names, whitespaces

 scripts/checkpatch.pl | 79 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 77 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index fdfd5ec09be6..e372d26d03dc 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -506,6 +506,72 @@ our $signature_tags = qr{(?xi:
 	Cc:
 )};
 
+sub edit_distance_min {
+	my (@arr) = @_;
+	my $len = scalar @arr;
+	if ((scalar @arr) < 1) {
+		# if underflow, return
+		return;
+	}
+	my $min = $arr[0];
+	for my $i (0 .. ($len-1)) {
+		if ($arr[$i] < $min) {
+			$min = $arr[$i];
+		}
+	}
+	return $min;
+}
+
+sub get_edit_distance {
+	my ($str1, $str2) = @_;
+	$str1 = lc($str1);
+	$str1 =~ s/-//g;
+	$str2 =~ s/-//g;
+	my $len1 = length($str1);
+	my $len2 = length($str2);
+	# two dimensional array storing minimum edit distance
+	my @distance;
+	for my $i (0 .. $len1) {
+		for my $j (0 .. $len2) {
+			if ($i == 0) {
+				$distance[$i][$j] = $j;
+			} elsif ($j == 0) {
+				$distance[$i][$j] = $i;
+			} elsif (substr($str1, $i-1, 1) eq substr($str2, $j-1, 1)) {
+				$distance[$i][$j] = $distance[$i - 1][$j - 1];
+			} else {
+				my $dist1 = $distance[$i][$j - 1]; #insert distance
+				my $dist2 = $distance[$i - 1][$j]; # remove
+				my $dist3 = $distance[$i - 1][$j - 1]; #replace
+				$distance[$i][$j] = 1 + edit_distance_min($dist1, $dist2, $dist3);
+			}
+		}
+	}
+	return $distance[$len1][$len2];
+}
+
+sub find_standard_signature {
+	my ($sign_off) = @_;
+	my @standard_signature_tags = (
+		'signed-off-by:', 'co-developed-by:', 'acked-by:', 'tested-by:',
+		'reviewed-by:', 'reported-by:', 'suggested-by:'
+	);
+	my $standard_signature;
+	my $min_edit_distance = 20; # setting default value
+	my $edit_distance;
+	foreach (@standard_signature_tags) {
+		$edit_distance = get_edit_distance($sign_off, $_);
+		if ($edit_distance < $min_edit_distance) {
+			$min_edit_distance = $edit_distance;
+			$standard_signature = $_;
+		}
+	}
+        if ($min_edit_distance <= 2) {
+		return ucfirst($standard_signature);
+	}
+	return "";
+}
+
 our @typeListMisordered = (
 	qr{char\s+(?:un)?signed},
 	qr{int\s+(?:(?:un)?signed\s+)?short\s},
@@ -2773,8 +2839,17 @@ 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 = find_standard_signature($sign_off);
+				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) &&
+					    $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

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

[Joe Perches]

On Sun, 2020-11-29 at 00:05 +0530, Aditya Srivastava wrote:
> Currently checkpatch warns for BAD_SIGN_OFF on non-standard signature
> styles.

Seems OK, but here are some last trivial notes:

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> +sub find_standard_signature {
> +	my ($sign_off) = @_;
> +	my @standard_signature_tags = (
> +		'signed-off-by:', 'co-developed-by:', 'acked-by:', 'tested-by:',
> +		'reviewed-by:', 'reported-by:', 'suggested-by:'

I would change this to the normal signatures:

	my @standard_signature_tags = (
		'Signed-off-by:', 'Co-developed-by:', 'Acked-by:', 'Tested-by:',
		'Reviewed-by:', 'Reported-by:', 'Suggested-by:'

> +	);
> +	my $standard_signature;
> +	my $min_edit_distance = 20; # setting default value

20 seems arbitrary, maybe (~0 << 1) ?

> +	my $edit_distance;

move this into the foreach (or maybe not use this at all)

> +	foreach (@standard_signature_tags) {

foreach style in this code uses foreach my $<something> and not $_

	foreach my $standard (@standard_signature_tags) {

> +		$edit_distance = get_edit_distance($sign_off, $_);

So:

		my $edit_distance = get_edit_distance($sign_off, $standard);

> +		if ($edit_distance < $min_edit_distance) {
> +			$min_edit_distance = $edit_distance;
> +			$standard_signature = $_;
> +		}
> +	}
> +        if ($min_edit_distance <= 2) {
> +		return ucfirst($standard_signature);

	return $standard;

Though maybe it's simpler to test in the loop if it's <= 2 as
the lowercase and dash strip is done inside get_edit_distance
so this seems rather simpler:

	foreach my $standard (@standard_signature_tags) {
		return $standard if (get_edit_distance($sign_off, $standard) <= 2);
	}

	return "";

> @@ -2773,8 +2839,17 @@ 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 = find_standard_signature($sign_off);
> +				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) &&

Please use consistent '' or nothing around signatures:

						"Non-standard signature: '$sign_off' - likely typo of '$suggested_signature'\n" . $herecurr) &&

> +					    $fix) {
> +						$fixed[$fixlinenr] =~ s/$sign_off/$suggested_signature/;
> +					}
> +				}
>  			}
>  			if (defined $space_before && $space_before ne "") {
>  				if (WARN("BAD_SIGN_OFF",


_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees