* [Linux-kernel-mentees] [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; 14+ messages in thread
From: Aditya Srivastava @ 2020-11-28 13:05 UTC (permalink / raw)
To: joe; +Cc: linux-kernel-mentees, linux-kernel, yashsri421
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
_______________________________________________
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] 14+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v4] checkpatch: add fix and improve warning msg for non-standard signature
2020-11-28 13:05 [Linux-kernel-mentees] [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 ` [Linux-kernel-mentees] [PATCH v5] " Aditya Srivastava
0 siblings, 1 reply; 14+ messages in thread
From: Joe Perches @ 2020-11-28 15:40 UTC (permalink / raw)
To: Aditya Srivastava; +Cc: linux-kernel-mentees, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Linux-kernel-mentees] [PATCH v5] checkpatch: add fix and improve warning msg for non-standard signature
2020-11-28 15:40 ` Joe Perches
@ 2020-11-28 18:35 ` Aditya Srivastava
2020-11-28 19:12 ` Joe Perches
0 siblings, 1 reply; 14+ messages in thread
From: Aditya Srivastava @ 2020-11-28 18:35 UTC (permalink / raw)
To: joe; +Cc: linux-kernel-mentees, linux-kernel, yashsri421
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v5] checkpatch: add fix and improve warning msg for non-standard signature
2020-11-28 18:35 ` [Linux-kernel-mentees] [PATCH v5] " Aditya Srivastava
@ 2020-11-28 19:12 ` Joe Perches
2020-11-28 20:43 ` [Linux-kernel-mentees] [PATCH v6] " Aditya Srivastava
0 siblings, 1 reply; 14+ messages in thread
From: Joe Perches @ 2020-11-28 19:12 UTC (permalink / raw)
To: Aditya Srivastava; +Cc: linux-kernel-mentees, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Linux-kernel-mentees] [PATCH v6] checkpatch: add fix and improve warning msg for non-standard signature
2020-11-28 19:12 ` Joe Perches
@ 2020-11-28 20:43 ` Aditya Srivastava
2020-11-28 20:57 ` Joe Perches
0 siblings, 1 reply; 14+ messages in thread
From: Aditya Srivastava @ 2020-11-28 20:43 UTC (permalink / raw)
To: joe; +Cc: linux-kernel-mentees, linux-kernel, yashsri421
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
changes in v6: Simplify foreach loop; change standard signature tag values to normal ucfirst; modify warning message
scripts/checkpatch.pl | 71 +++++++++++++++++++++++++++++++++++++++++--
1 file changed, 69 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index fdfd5ec09be6..4a026926139f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -506,6 +506,64 @@ 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);
+ $str2 = lc($str2);
+ $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:'
+ );
+ foreach my $signature (@standard_signature_tags) {
+ return $signature if (get_edit_distance($sign_off, $signature) <= 2);
+ }
+
+ return "";
+}
+
our @typeListMisordered = (
qr{char\s+(?:un)?signed},
qr{int\s+(?:(?:un)?signed\s+)?short\s},
@@ -2773,8 +2831,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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v6] checkpatch: add fix and improve warning msg for non-standard signature
2020-11-28 20:43 ` [Linux-kernel-mentees] [PATCH v6] " Aditya Srivastava
@ 2020-11-28 20:57 ` Joe Perches
0 siblings, 0 replies; 14+ messages in thread
From: Joe Perches @ 2020-11-28 20:57 UTC (permalink / raw)
To: Aditya Srivastava, Andrew Morton; +Cc: linux-kernel-mentees, linux-kernel
On Sun, 2020-11-29 at 02:13 +0530, Aditya Srivastava wrote:
> Currently checkpatch warns for BAD_SIGN_OFF on non-standard signature
> styles.
Thanks.
Acked-by: Joe Perches <joe@perches.com>
> 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
>
> changes in v6: Simplify foreach loop; change standard signature tag values to normal ucfirst; modify warning message
>
> scripts/checkpatch.pl | 71 +++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 69 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index fdfd5ec09be6..4a026926139f 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -506,6 +506,64 @@ 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);
> + $str2 = lc($str2);
> + $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:'
> + );
> + foreach my $signature (@standard_signature_tags) {
> + return $signature if (get_edit_distance($sign_off, $signature) <= 2);
> + }
> +
> + return "";
> +}
> +
> our @typeListMisordered = (
> qr{char\s+(?:un)?signed},
> qr{int\s+(?:(?:un)?signed\s+)?short\s},
> @@ -2773,8 +2831,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",
_______________________________________________
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] 14+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v5] checkpatch: add fix and improve warning msg for Non-standard signature
2020-12-01 18:21 ` Lukas Bulwahn
@ 2020-12-01 18:39 ` Joe Perches
0 siblings, 0 replies; 14+ messages in thread
From: Joe Perches @ 2020-12-01 18:39 UTC (permalink / raw)
To: Lukas Bulwahn
Cc: linux-kernel-mentees, Linux Kernel Mailing List, Aditya Srivastava
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v5] checkpatch: add fix and improve warning msg for Non-standard signature
2020-12-01 17:24 ` Joe Perches
@ 2020-12-01 18:21 ` Lukas Bulwahn
2020-12-01 18:39 ` Joe Perches
0 siblings, 1 reply; 14+ messages in thread
From: Lukas Bulwahn @ 2020-12-01 18:21 UTC (permalink / raw)
To: Joe Perches
Cc: linux-kernel-mentees, Linux Kernel Mailing List, Aditya Srivastava
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v5] checkpatch: add fix and improve warning msg for Non-standard signature
2020-12-01 11:29 Aditya Srivastava
@ 2020-12-01 17:24 ` Joe Perches
2020-12-01 18:21 ` Lukas Bulwahn
0 siblings, 1 reply; 14+ messages in thread
From: Joe Perches @ 2020-12-01 17:24 UTC (permalink / raw)
To: Aditya Srivastava; +Cc: linux-kernel-mentees, linux-kernel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v5] checkpatch: add fix and improve warning msg for Non-standard signature
2020-12-01 10:27 ` Lukas Bulwahn
@ 2020-12-01 11:34 ` Lukas Bulwahn
0 siblings, 0 replies; 14+ messages in thread
From: Lukas Bulwahn @ 2020-12-01 11:34 UTC (permalink / raw)
To: Aditya; +Cc: linux-kernel-mentees
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Linux-kernel-mentees] [PATCH v5] checkpatch: add fix and improve warning msg for Non-standard signature
@ 2020-12-01 11:29 Aditya Srivastava
2020-12-01 17:24 ` Joe Perches
0 siblings, 1 reply; 14+ messages in thread
From: Aditya Srivastava @ 2020-12-01 11:29 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
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v5] checkpatch: add fix and improve warning msg for Non-standard signature
2020-12-01 9:00 ` Aditya
@ 2020-12-01 10:27 ` Lukas Bulwahn
2020-12-01 11:34 ` Lukas Bulwahn
0 siblings, 1 reply; 14+ messages in thread
From: Lukas Bulwahn @ 2020-12-01 10:27 UTC (permalink / raw)
To: Aditya; +Cc: linux-kernel-mentees
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Linux-kernel-mentees] [PATCH v5] checkpatch: add fix and improve warning msg for Non-standard signature
2020-11-29 8:14 [Linux-kernel-mentees] [PATCH v5] checkpatch: add fix and improve warning msg for Non-standard signature Aditya Srivastava
@ 2020-12-01 9:00 ` Aditya
2020-12-01 10:27 ` Lukas Bulwahn
0 siblings, 1 reply; 14+ messages in thread
From: Aditya @ 2020-12-01 9:00 UTC (permalink / raw)
To: lukas.bulwahn; +Cc: linux-kernel-mentees
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Linux-kernel-mentees] [PATCH v5] checkpatch: add fix and improve warning msg for Non-standard signature
@ 2020-11-29 8:14 Aditya Srivastava
2020-12-01 9:00 ` Aditya
0 siblings, 1 reply; 14+ messages in thread
From: Aditya Srivastava @ 2020-11-29 8:14 UTC (permalink / raw)
To: lukas.bulwahn; +Cc: linux-kernel-mentees, 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
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-12-01 18:39 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-28 13:05 [Linux-kernel-mentees] [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 ` [Linux-kernel-mentees] [PATCH v5] " Aditya Srivastava
2020-11-28 19:12 ` Joe Perches
2020-11-28 20:43 ` [Linux-kernel-mentees] [PATCH v6] " Aditya Srivastava
2020-11-28 20:57 ` Joe Perches
2020-11-29 8:14 [Linux-kernel-mentees] [PATCH v5] checkpatch: add fix and improve warning msg for Non-standard signature Aditya Srivastava
2020-12-01 9:00 ` Aditya
2020-12-01 10:27 ` Lukas Bulwahn
2020-12-01 11:34 ` Lukas Bulwahn
2020-12-01 11:29 Aditya Srivastava
2020-12-01 17:24 ` Joe Perches
2020-12-01 18:21 ` Lukas Bulwahn
2020-12-01 18:39 ` Joe Perches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).