mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH v4] checkpatch: add fix and improve warning msg for non-standard signature
@ 2020-11-28 13:05 Aditya Srivastava
  2020-11-28 15:40 ` Joe Perches
  0 siblings, 1 reply; 12+ messages in thread
From: Aditya Srivastava @ 2020-11-28 13:05 UTC (permalink / raw)
  To: joe; +Cc: yashsri421, lukas.bulwahn, linux-kernel-mentees, linux-kernel

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')

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index fdfd5ec09be6..2b1afd763d8d 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:'
+	);
+	# 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


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH v3] checkpatch: add fix and improve warning msg for Non-standard signature
@ 2020-11-23 15:16 Lukas Bulwahn
  2020-11-23 17:24 ` [PATCH v4] " Aditya Srivastava
  0 siblings, 1 reply; 12+ messages in thread
From: Lukas Bulwahn @ 2020-11-23 15:16 UTC (permalink / raw)
  To: Aditya Srivastava
  Cc: Joe Perches, linux-kernel-mentees, Linux Kernel Mailing List

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
>

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

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-28 13:05 [PATCH v4] checkpatch: add fix and improve warning msg for non-standard signature Aditya Srivastava
2020-11-28 15:40 ` Joe Perches
2020-11-28 18:35   ` [PATCH v5] " Aditya Srivastava
2020-11-28 19:12     ` Joe Perches
2020-11-28 20:43       ` [PATCH v6] " Aditya Srivastava
2020-11-28 20:57         ` Joe Perches
  -- strict thread matches above, loose matches on Subject: below --
2020-11-23 15:16 [PATCH v3] checkpatch: add fix and improve warning msg for Non-standard signature Lukas Bulwahn
2020-11-23 17:24 ` [PATCH v4] " Aditya Srivastava
2020-11-23 17:33   ` Joe Perches
2020-11-24  3:12     ` Aditya
2020-11-24  6:54     ` Lukas Bulwahn
2020-11-24  7:26       ` Joe Perches
2020-11-24  7:44         ` Lukas Bulwahn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®