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

Lukas Bulwahn lukas.bulwahn at gmail.com
Wed Nov 25 12:26:13 UTC 2020


On Wed, Nov 25, 2020 at 12:25 PM Aditya Srivastava <yashsri421 at 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

It is not a sign-off, it is a signature tag.

> warnings due to Non-standard signatures, 87 are due to typo mistakes.
>
why do you write "non-standard" capitalized?

> Following are the standard signature tags which are often incorrectly
> used with their counts(over v4.13..v5.8):
>

This sentence above is not clear.

> 1) Reviewed-by => 42
> 2) Signed-off-by => 25
> 3) Reported-by => 6
> 4) Acked-by => 4
> 5) Tested-by => 4
> 6) Suggested-by => 4
>

You can probably drop the 1) to 6); simply indent it; I think "tag:
count" or "tag     count" (where the spacing is aligned) is better
than "tag => count"

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

s/misspelt/misspelled/

> Out of the 87 misspelt signatures fixed with this approach, 85 were
> found to be good corrections and 2 were bad corrections.
>
> The signature tags which are good corrections using this approach are:
>
> 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)Signen-off-by (count: 1) => Signed-off-by
> 29)Reviwed-By (count: 1) => Reviewed-by
> 30)eported-by (count: 1) => Reported-by
> 31)Reviewedy-by (count: 1) => Reviewed-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)Suggsted-by (count: 1) => Suggested-by
> 36)Ack-by (count: 1) => Acked-by
> 37)Reorted-by (count: 1) => Reported-by
>

I do not think the enumeration of all those typos is interesting for
the commit message. Maybe Joe will ask, but anyone else can imagine
how such typos look like.

If you think they should be kept, I would just sort it this way:

Typo variants of the tags were:

Signed-off-by:
  then list all variants

Acked-by:
  then list all variants

Reported-by:
  then list all variants

etc.

> Following were found to be bad corrections:
>
> 1)-By (count: 1) => To

How about not correcting to To: (It is simply too short).

The explanation is getting better, give it another try. Then we might
be in a good state for Joe and lkml.

Lukas

> 2)Tweeted-by (count: 1) => Tested-by
>
> Signed-off-by: Aditya Srivastava <yashsri421 at gmail.com>
> ---
> changes in v2: modify commit message: replace specific example with overall evaluation, minor changes
>
> changes in v3: summarize commit message
>
>  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
>


More information about the Linux-kernel-mentees mailing list