All of lore.kernel.org
 help / color / mirror / Atom feed
* [Linux-kernel-mentees] Fix for BAD_SIGN_OFF: non-standard signature
@ 2020-11-11 14:13 Aditya
  2020-11-11 20:04 ` Lukas Bulwahn
  0 siblings, 1 reply; 27+ messages in thread
From: Aditya @ 2020-11-11 14:13 UTC (permalink / raw)
  To: Lukas Bulwahn; +Cc: linux-kernel-mentees

Hi Sir
I have analyzed the checkpatch report for BAD_SIGN_OFF(over
v4.13..v5.8) for non-standard signature and generated reports for it.
Some mistakes are more frequent than others, whereas some mistakes
even have a frequency of 1.

Non-standard signatures occurring with their frequency:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/non_standard_signature/non_standard_signs.txt

Complete warning messages:
https://github.com/AdityaSrivast/kernel-tasks/blob/master/random/non_standard_signature/warn_msgs.txt

Should I implement the fix similar to TYPO_FIX, where we have a
separate file for common misspellings and corrected words? Or should I
make a hash of these misspellings in checkpatch.pl file as well?

Also should I include all these misspelled words in it? Or omit words
below certain frequency?

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

^ permalink raw reply	[flat|nested] 27+ messages in thread
* [Linux-kernel-mentees] [PATCH v3] checkpatch: add fix and improve warning msg for Non-standard signature
@ 2020-11-23 15:04 Aditya Srivastava
  2020-11-23 15:16 ` Lukas Bulwahn
  0 siblings, 1 reply; 27+ messages in thread
From: Aditya Srivastava @ 2020-11-23 15:04 UTC (permalink / raw)
  To: joe; +Cc: linux-kernel-mentees, linux-kernel, yashsri421

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",
+	},
+	"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",
+	},
+	"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

^ permalink raw reply related	[flat|nested] 27+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v2] checkpatch: add fix and improve warning msg for Non-standard signature
@ 2020-11-25  6:57 Lukas Bulwahn
  2020-11-25 11:25 ` [Linux-kernel-mentees] [PATCH v3] " Aditya Srivastava
  0 siblings, 1 reply; 27+ messages in thread
From: Lukas Bulwahn @ 2020-11-25  6:57 UTC (permalink / raw)
  To: Aditya Srivastava; +Cc: linux-kernel-mentees

On Tue, Nov 24, 2020 at 4:32 PM Aditya Srivastava <yashsri421@gmail.com> 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
> signoffs. An evaluation over v4.13..v5.8 revealed that out of 539
> warnings due to Non-standard signatures, 85 are due to typo mistakes.
>
> Provide a fix by calculating levenshtein distance for the signature tag
> over all the standard signatures and suggest a fix with signature, whose
> edit distance is less than or equal to 2.
>
> According to evaluation over v4.13..v5.8, following are the incorrect
> signature tags with typos and their corresponding suggested fixes
> (according to edit distace approach):
>
> 1)Reviwed-by (count: 19) => Reviewed-by
>
> 2)Reviewd-by (count: 9) => Reviewed-by
>
> 3)Singed-off-by (count: 8) => Signed-off-by
>
> 4)Signed-of-by (count: 6) => Signed-off-by
>
> 5)Rewieved-by (count: 3) => Reviewed-by
>
> 6)Signed-off--by (count: 3) => Signed-off-by
>
> 7)Revieved-by (count: 3) => Reviewed-by
>
> 8)Reivewed-by (count: 2) => Reviewed-by
>
> 9)Signef-off-by (count: 2) => Signed-off-by
>
> 10)Test-by (count: 2) => Tested-by
>
> 11)Acked_by (count: 2) => Acked-by
>
> 12)Signed-off-by-by (count: 2) => Signed-off-by
>
> 13)Reported-by-by (count: 1) => Reported-by
>
> 14)Reporetd-by (count: 1) => Reported-by
>
> 15)Reviewed--by (count: 1) => Reviewed-by
>
> 16)Sugested-by (count: 1) => Suggested-by
>
> 17)Suggested--by (count: 1) => Suggested-by
>
> 18)Repoted-by (count: 1) => Reported-by
>
> 19)Rported-by (count: 1) => Reported-by
>
> 20)eigned-off-by (count: 1) => Signed-off-by
>
> 21)Reveiwed-by (count: 1) => Reviewed-by
>
> 22)igned-off-by (count: 1) => Signed-off-by
>
> 23)Tested-by-by (count: 1) => Tested-by
>
> 24)Sugessted-by (count: 1) => Suggested-by
>
> 25)Rewiewed-by (count: 1) => Reviewed-by
>
> 26)Teste-by (count: 1) => Tested-by
>
> 27)Signee-off-by (count: 1) => Signed-off-by
>
> 28)-By (count: 1) => To
>
> 29)Signen-off-by (count: 1) => Signed-off-by
>
> 30)eported-by (count: 1) => Reported-by
>
> 31)Reviewedy-by (count: 1) => Signed-off-by
>
> 32)Siganed-off-by (count: 1) => Signed-off-by
>
> 33)Ackedy-by (count: 1) => Acked-by
>
> 34)Review-by (count: 1) => Reviewed-by
>
> 35)Tweeted-by (count: 1) => Tested-by
>
> 36)Ack-by (count: 1) => Acked-by
>
> 37)Reorted-by (count: 1) => Reported-by
>
> 38)Suggsted-by (count: 1) => Suggested-by
>
> 39)Reviwed-By (count: 1) => Reviewed-by
>

How about summarizing this?

Which are good corrections, which are "bad corrections"; what is the
total count of corrections, how many are good, and which are bad?

How many typo variants were corrected to each valid tag?

> Signed-off-by: Aditya Srivastava <yashsri421@gmail.com>
> ---
> changes in v2: modify commit message: replace specific example with overall evaluation, minor changes
> patch applies on next-20201120
>
>  scripts/checkpatch.pl | 85 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 83 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index fdfd5ec09be6..775a49a06179 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -506,6 +506,77 @@ our $signature_tags = qr{(?xi:
>         Cc:
>  )};
>
> +sub get_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) = @_;
> +       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 + get_min($dist1, $dist2, $dist3);
> +                       }
> +               }
> +       }
> +       return $distance[$len1][$len2];
> +}
> +
> +sub get_standard_signature {
> +       my ($sign_off) = @_;
> +       $sign_off = lc($sign_off);
> +       $sign_off =~ s/\-//g; # to match with formed hash
> +       my @standard_signature_tags = (
> +               'signed-off-by:', 'co-developed-by:', 'acked-by:', 'tested-by:',
> +               'reviewed-by:', 'reported-by:', 'suggested-by:', 'to:', 'cc:'
> +       );
> +       # setting default values
> +       my $standard_signature = 'signed-off-by';
> +       my $min_edit_distance = 20;
> +       my $edit_distance;
> +       foreach (@standard_signature_tags) {
> +               my $signature = $_;
> +               $_ =~ s/\-//g;
> +               $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) {
> +               return ucfirst($standard_signature);
> +        }
> +       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);
> +                               }
> +                               else {
> +                                       if (WARN("BAD_SIGN_OFF",
> +                                                "Non-standard signature: $sign_off. Please use '$suggested_signature' instead\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

^ permalink raw reply	[flat|nested] 27+ messages in thread
end of thread, other threads:[~2020-11-25 12:26 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-11 14:13 [Linux-kernel-mentees] Fix for BAD_SIGN_OFF: non-standard signature Aditya
2020-11-11 20:04 ` Lukas Bulwahn
2020-11-13 14:35   ` Aditya
2020-11-13 15:00     ` Aditya
2020-11-13 15:26       ` Lukas Bulwahn
2020-11-13 18:25         ` Aditya
2020-11-17 18:03           ` Aditya
2020-11-17 17:42             ` Lukas Bulwahn
2020-11-17 20:54               ` Aditya
2020-11-18 10:12                 ` Aditya
2020-11-18 19:17                   ` Lukas Bulwahn
2020-11-19  5:53                   ` Lukas Bulwahn
2020-11-19 14:09                     ` Aditya
2020-11-20 19:58                       ` [Linux-kernel-mentees] [PATCH] checkpatch: add fix and improve warning msg for Non-standard signature Aditya Srivastava
2020-11-20 20:03                         ` Aditya
2020-11-20 20:23                           ` Lukas Bulwahn
2020-11-20 21:30                             ` Aditya
2020-11-21  4:58                               ` [Linux-kernel-mentees] [PATCH v2] " Aditya Srivastava
2020-11-21  9:52                                 ` Lukas Bulwahn
2020-11-23 12:21                                   ` [Linux-kernel-mentees] [PATCH v3] " Aditya Srivastava
2020-11-23 13:09                                     ` Lukas Bulwahn
2020-11-23 15:16                                       ` Aditya
2020-11-23 15:18                                         ` Lukas Bulwahn
2020-11-23 15:04 Aditya Srivastava
2020-11-23 15:16 ` Lukas Bulwahn
2020-11-25  6:57 [Linux-kernel-mentees] [PATCH v2] " Lukas Bulwahn
2020-11-25 11:25 ` [Linux-kernel-mentees] [PATCH v3] " Aditya Srivastava
2020-11-25 12:26   ` Lukas Bulwahn

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.