Skip to contentSkip to conversation

checkpatch: add new exception to repeated word check

PATCH Superseded Dwaipayan Ray 2 participants 8 messages Oct 14, 2020 – Oct 17, 2020 Checkpatch

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Dwaipayan Ray

author
#0 lore raw

PATCH v2 1/1 checkpatch: add new exception to repeated word check

Recently, commit 4f6ad8aa1eac ("checkpatch: move repeated word test") moved the repeated word test to check for more file types. But after this, if checkpatch.pl is run on MAINTAINERS, it generates several new warnings of the type:

WARNING: Possible repeated word: 'git'

For example:
WARNING: Possible repeated word: 'git'
+T: git git://git.kernel.org/pub/scm/linux/kernel/git/rw/uml.git

So, the pattern "git git://..." is a false positive in this case.

Add 'git' to the exception list for repeated word check. This effectively fixes all the newly generated false positives.

Fixes 4f6ad8aa1eac ("checkpatch: move repeated word test")
Link https://lore.kernel.org/linux-kernel-mentees/[email protected]/ open here
Suggested-by Joe Perches <[email protected]>
Suggested-by Lukas Bulwahn <[email protected]>
Signed-off-by Dwaipayan Ray <[email protected]>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
scripts/checkpatch.pl +1 -1
index f1a4e61917eb..b55d83360366 100755
@@ -3063,7 +3063,7 @@ sub process {
 				}
 
 				next if ($first ne $second);
-				next if ($first eq 'long');
+				next if ($first =~ /^(?:long|git)$/);
 
 				if (WARN("REPEATED_WORD",
 					 "Possible repeated word: '$first'\n" . $herecurr) &&
Signature · 1 line
2.27.0
maintainer
#1 lore raw
Quoted 4 lines · Dwaipayan Ray

Recently, commit 4f6ad8aa1eac ("checkpatch: move repeated word test") moved the repeated word test to check for more file types. But after this, if checkpatch.pl is run on MAINTAINERS, it generates several new warnings of the type:

Perhaps instead of adding more content checks so that word boundaries are not something like \S but also not punctuation so that content like

	git git://
	@size size

does not match?

author
#2 lore raw
Quoted 7 lines · Joe Perches
Quoted 4 lines · Dwaipayan Ray

Recently, commit 4f6ad8aa1eac ("checkpatch: move repeated word test") moved the repeated word test to check for more file types. But after this, if checkpatch.pl is run on MAINTAINERS, it generates several new warnings of the type:

Quoted 10 lines

Perhaps instead of adding more content checks so that word boundaries are not something like \S but also not punctuation so that content like

        git git://
        @size size

does not match?

Hi,
So currently the words are trimmed of non alphabets before the check:

while ($rawline =~ /\b($word_pattern) (?=($word_pattern))/g) { my $first = $1;
my $second = $2;

where, the word_pattern is:
my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';

So do you perhaps recommend modifying this word pattern to include the punctuation as well rather than trimming them off?

Thanks,
Dwaipayan.

maintainer
#3 lore raw
Quoted 20 lines · Dwaipayan Ray
Quoted 6 lines · Joe Perches
Quoted 4 lines · Dwaipayan Ray

Recently, commit 4f6ad8aa1eac ("checkpatch: move repeated word test") moved the repeated word test to check for more file types. But after this, if checkpatch.pl is run on MAINTAINERS, it generates several new warnings of the type:

Quoted 10 lines

Perhaps instead of adding more content checks so that word boundaries are not something like \S but also not punctuation so that content like

        git git://
        @size size

does not match?

Hi,
So currently the words are trimmed of non alphabets before the check:

Quoted 6 lines

while ($rawline =~ /\b($word_pattern) (?=($word_pattern))/g) { my $first = $1;
my $second = $2;

where, the word_pattern is:
my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';

I'm familiar.

Quoted 2 lines

So do you perhaps recommend modifying this word pattern to include the punctuation as well rather than trimming them off?

Not really, perhaps use the capture group position markers @- @+ or $-[1] $+[1] and $-[2] $+[2] with the substr could be used to see what characters are before and after the word matches.

Quoted 3 lines

Thanks,
Dwaipayan.

maintainer
#4 lore raw
Quoted 30 lines · Joe Perches
Quoted 20 lines · Dwaipayan Ray
Quoted 6 lines · Joe Perches
Quoted 4 lines · Dwaipayan Ray

Recently, commit 4f6ad8aa1eac ("checkpatch: move repeated word test") moved the repeated word test to check for more file types. But after this, if checkpatch.pl is run on MAINTAINERS, it generates several new warnings of the type:

Quoted 10 lines

Perhaps instead of adding more content checks so that word boundaries are not something like \S but also not punctuation so that content like

        git git://
        @size size

does not match?

Hi,
So currently the words are trimmed of non alphabets before the check:

Quoted 6 lines

while ($rawline =~ /\b($word_pattern) (?=($word_pattern))/g) { my $first = $1;
my $second = $2;

where, the word_pattern is:
my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';

I'm familiar.

Quoted 7 lines
Quoted 2 lines

So do you perhaps recommend modifying this word pattern to include the punctuation as well rather than trimming them off?

Not really, perhaps use the capture group position markers @- @+ or $-[1] $+[1] and $-[2] $+[2] with the substr could be used to see what characters are before and after the word matches.

Perhaps something like:

---
 scripts/checkpatch.pl | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
scripts/checkpatch.pl +11 -1
index fab38b493cef..a65eb40a5539 100755
@@ -3054,15 +3054,25 @@ sub process {
 
 				my $first = $1;
 				my $second = $2;
+				my $start_pos = $-[1];
+				my $end_pos = $+[2];
 
 				if ($first =~ /(?:struct|union|enum)/) {
 					pos($rawline) += length($first) + length($second) + 1;
 					next;
 				}
 
-				next if ($first ne $second);
+				next if (lc($first) ne lc($second));
 				next if ($first eq 'long');
 
+				my $start_char = "";
+				my $end_char = "";
+				$start_char = substr($rawline, $start_pos - 1, 1) if ($start_pos > 0);
+				$end_char = substr($rawline, $end_pos, 1) if (length($rawline) > $end_pos);
+
+				next if ($start_char =~ /^\S$/);
+				next if ($end_char !~ /^[\.\,\s]?$/);
+
 				if (WARN("REPEATED_WORD",
 					 "Possible repeated word: '$first'\n" . $herecurr) &&
 				    $fix) {

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

author
#5 lore raw
Quoted 68 lines · Joe Perches
Quoted 30 lines · Joe Perches
Quoted 20 lines · Dwaipayan Ray
Quoted 6 lines · Joe Perches
Quoted 4 lines · Dwaipayan Ray

Recently, commit 4f6ad8aa1eac ("checkpatch: move repeated word test") moved the repeated word test to check for more file types. But after this, if checkpatch.pl is run on MAINTAINERS, it generates several new warnings of the type:

Quoted 10 lines

Perhaps instead of adding more content checks so that word boundaries are not something like \S but also not punctuation so that content like

        git git://
        @size size

does not match?

Hi,
So currently the words are trimmed of non alphabets before the check:

Quoted 6 lines

while ($rawline =~ /\b($word_pattern) (?=($word_pattern))/g) { my $first = $1;
my $second = $2;

where, the word_pattern is:
my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';

I'm familiar.

Quoted 7 lines
Quoted 2 lines

So do you perhaps recommend modifying this word pattern to include the punctuation as well rather than trimming them off?

Not really, perhaps use the capture group position markers @- @+ or $-[1] $+[1] and $-[2] $+[2] with the substr could be used to see what characters are before and after the word matches.

Perhaps something like:

---
 scripts/checkpatch.pl | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
scripts/checkpatch.pl +4 -1
index fab38b493cef..a65eb40a5539 100755
@@ -3054,15 +3054,25 @@ sub process {
 
                                my $first = $1;
                                my $second = $2;
+                               my $start_pos = $-[1];
+                               my $end_pos = $+[2];
 
                                if ($first =~ /(?:struct|union|enum)/) {
                                        pos($rawline) += length($first) + length($second) + 1;
                                        next;
                                }
 
-                               next if ($first ne $second);
+                               next if (lc($first) ne lc($second));
                                next if ($first eq 'long');
 
+                               my $start_char = "";

scripts/checkpatch.pl sub process { · line 3078

Quoted 10 lines
scripts/checkpatch.pl +7 -0
+                               my $end_char = "";
+                               $start_char = substr($rawline, $start_pos - 1, 1) if ($start_pos > 0);
+                               $end_char = substr($rawline, $end_pos, 1) if (length($rawline) > $end_pos);
+
+                               next if ($start_char =~ /^\S$/);
+                               next if ($end_char !~ /^[\.\,\s]?$/);
+
                                if (WARN("REPEATED_WORD",
                                         "Possible repeated word: '$first'\n" . $herecurr) &&
                                    $fix) {

Hi Joe,
Thank you for the insight. I was also doing something similar:

---
 scripts/checkpatch.pl | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
scripts/checkpatch.pl +16 -0
index f1a4e61917eb..82497a71ac96 100755
@@ -595,6 +595,7 @@ our @mode_permission_funcs = (
 );
 
 my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';
+my $punctuation_chars = '[,:;@\.\-]';
 
 #Create a search pattern for all these functions to speed up a loop below
 our $mode_perms_search = "";

@@ -3065,6 +3066,21 @@ sub process {
  next if ($first ne $second);
  next if ($first eq 'long');
 
+ # check for character before and after the word matches
+ my $ca_first = substr($rawline, $-[1]-1, 1);
+ my $cb_first = substr($rawline, $+[1], 1);
+ my $ca_second = substr($rawline, $-[2]-1, 1);
+ my $cb_second = substr($rawline, $+[2], 1);
+
+ if ($ca_first ne $ca_second || $cb_first ne $cb_second) {
+ if ($ca_first =~ /$punctuation_chars/ ||
+     $ca_second =~ /$punctuation_chars/ ||
+     $cb_first =~ /$punctuation_chars/ ||
+     $cb_second =~ /$punctuation_chars/) {
+ next;
+ }
+ }
+
  if (WARN("REPEATED_WORD",
  "Possible repeated word: '$first'\n" . $herecurr) &&
      $fix) {

Does it look okay to you??

Thanks,
Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

maintainer
#6 lore raw
Quoted 121 lines · Dwaipayan Ray
Quoted 67 lines · Joe Perches
Quoted 30 lines · Joe Perches
Quoted 20 lines · Dwaipayan Ray
Quoted 6 lines · Joe Perches
Quoted 4 lines · Dwaipayan Ray

Recently, commit 4f6ad8aa1eac ("checkpatch: move repeated word test") moved the repeated word test to check for more file types. But after this, if checkpatch.pl is run on MAINTAINERS, it generates several new warnings of the type:

Quoted 10 lines

Perhaps instead of adding more content checks so that word boundaries are not something like \S but also not punctuation so that content like

        git git://
        @size size

does not match?

Hi,
So currently the words are trimmed of non alphabets before the check:

Quoted 6 lines

while ($rawline =~ /\b($word_pattern) (?=($word_pattern))/g) { my $first = $1;
my $second = $2;

where, the word_pattern is:
my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';

I'm familiar.

Quoted 7 lines
Quoted 2 lines

So do you perhaps recommend modifying this word pattern to include the punctuation as well rather than trimming them off?

Not really, perhaps use the capture group position markers @- @+ or $-[1] $+[1] and $-[2] $+[2] with the substr could be used to see what characters are before and after the word matches.

Perhaps something like:

---
 scripts/checkpatch.pl | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
scripts/checkpatch.pl +4 -1
index fab38b493cef..a65eb40a5539 100755
@@ -3054,15 +3054,25 @@ sub process {
 
                                my $first = $1;
                                my $second = $2;
+                               my $start_pos = $-[1];
+                               my $end_pos = $+[2];
 
                                if ($first =~ /(?:struct|union|enum)/) {
                                        pos($rawline) += length($first) + length($second) + 1;
                                        next;
                                }
 
-                               next if ($first ne $second);
+                               next if (lc($first) ne lc($second));
                                next if ($first eq 'long');
 
+                               my $start_char = "";

scripts/checkpatch.pl sub process { · line 3078

Quoted 10 lines
scripts/checkpatch.pl +7 -0
+                               my $end_char = "";
+                               $start_char = substr($rawline, $start_pos - 1, 1) if ($start_pos > 0);
+                               $end_char = substr($rawline, $end_pos, 1) if (length($rawline) > $end_pos);
+
+                               next if ($start_char =~ /^\S$/);
+                               next if ($end_char !~ /^[\.\,\s]?$/);
+
                                if (WARN("REPEATED_WORD",
                                         "Possible repeated word: '$first'\n" . $herecurr) &&
                                    $fix) {

Hi Joe,
Thank you for the insight. I was also doing something similar:

---
 scripts/checkpatch.pl | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
scripts/checkpatch.pl +16 -0
index f1a4e61917eb..82497a71ac96 100755
@@ -595,6 +595,7 @@ our @mode_permission_funcs = (
 );
 
 my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';
+my $punctuation_chars = '[,:;@\.\-]';
 
 #Create a search pattern for all these functions to speed up a loop below
 our $mode_perms_search = "";

@@ -3065,6 +3066,21 @@ sub process {
  next if ($first ne $second);
  next if ($first eq 'long');
 
+ # check for character before and after the word matches
+ my $ca_first = substr($rawline, $-[1]-1, 1);
+ my $cb_first = substr($rawline, $+[1], 1);
+ my $ca_second = substr($rawline, $-[2]-1, 1);
+ my $cb_second = substr($rawline, $+[2], 1);
+
+ if ($ca_first ne $ca_second || $cb_first ne $cb_second) {
+ if ($ca_first =~ /$punctuation_chars/ ||
+     $ca_second =~ /$punctuation_chars/ ||
+     $cb_first =~ /$punctuation_chars/ ||
+     $cb_second =~ /$punctuation_chars/) {
+ next;
+ }
+ }
+
  if (WARN("REPEATED_WORD",
  "Possible repeated word: '$first'\n" . $herecurr) &&
      $fix) {
Quoted 1 line

Does it look okay to you??

Not really, as ca_second and cb_first are both known to be the same position and known to be a single space.

_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

author
#7 lore raw
Quoted 123 lines · Joe Perches
Quoted 121 lines · Dwaipayan Ray
Quoted 67 lines · Joe Perches
Quoted 30 lines · Joe Perches
Quoted 20 lines · Dwaipayan Ray
Quoted 6 lines · Joe Perches
Quoted 4 lines · Dwaipayan Ray

Recently, commit 4f6ad8aa1eac ("checkpatch: move repeated word test") moved the repeated word test to check for more file types. But after this, if checkpatch.pl is run on MAINTAINERS, it generates several new warnings of the type:

Quoted 10 lines

Perhaps instead of adding more content checks so that word boundaries are not something like \S but also not punctuation so that content like

        git git://
        @size size

does not match?

Hi,
So currently the words are trimmed of non alphabets before the check:

Quoted 6 lines

while ($rawline =~ /\b($word_pattern) (?=($word_pattern))/g) { my $first = $1;
my $second = $2;

where, the word_pattern is:
my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';

I'm familiar.

Quoted 7 lines
Quoted 2 lines

So do you perhaps recommend modifying this word pattern to include the punctuation as well rather than trimming them off?

Not really, perhaps use the capture group position markers @- @+ or $-[1] $+[1] and $-[2] $+[2] with the substr could be used to see what characters are before and after the word matches.

Perhaps something like:

---
 scripts/checkpatch.pl | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)
scripts/checkpatch.pl +4 -1
index fab38b493cef..a65eb40a5539 100755
@@ -3054,15 +3054,25 @@ sub process {
 
                                my $first = $1;
                                my $second = $2;
+                               my $start_pos = $-[1];
+                               my $end_pos = $+[2];
 
                                if ($first =~ /(?:struct|union|enum)/) {
                                        pos($rawline) += length($first) + length($second) + 1;
                                        next;
                                }
 
-                               next if ($first ne $second);
+                               next if (lc($first) ne lc($second));
                                next if ($first eq 'long');
 
+                               my $start_char = "";

scripts/checkpatch.pl sub process { · line 3078

Quoted 10 lines
scripts/checkpatch.pl +7 -0
+                               my $end_char = "";
+                               $start_char = substr($rawline, $start_pos - 1, 1) if ($start_pos > 0);
+                               $end_char = substr($rawline, $end_pos, 1) if (length($rawline) > $end_pos);
+
+                               next if ($start_char =~ /^\S$/);
+                               next if ($end_char !~ /^[\.\,\s]?$/);
+
                                if (WARN("REPEATED_WORD",
                                         "Possible repeated word: '$first'\n" . $herecurr) &&
                                    $fix) {

Hi Joe,
Thank you for the insight. I was also doing something similar:

---
 scripts/checkpatch.pl | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
scripts/checkpatch.pl +16 -0
index f1a4e61917eb..82497a71ac96 100755
@@ -595,6 +595,7 @@ our @mode_permission_funcs = (
 );
 
 my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';
+my $punctuation_chars = '[,:;@\.\-]';
 
 #Create a search pattern for all these functions to speed up a loop below
 our $mode_perms_search = "";

@@ -3065,6 +3066,21 @@ sub process {
  next if ($first ne $second);
  next if ($first eq 'long');
 
+ # check for character before and after the word matches
+ my $ca_first = substr($rawline, $-[1]-1, 1);
+ my $cb_first = substr($rawline, $+[1], 1);
+ my $ca_second = substr($rawline, $-[2]-1, 1);
+ my $cb_second = substr($rawline, $+[2], 1);
+
+ if ($ca_first ne $ca_second || $cb_first ne $cb_second) {
+ if ($ca_first =~ /$punctuation_chars/ ||
+     $ca_second =~ /$punctuation_chars/ ||
+     $cb_first =~ /$punctuation_chars/ ||
+     $cb_second =~ /$punctuation_chars/) {
+ next;
+ }
+ }
+
  if (WARN("REPEATED_WORD",
  "Possible repeated word: '$first'\n" . $herecurr) &&
      $fix) {
Quoted 6 lines
Quoted 1 line

Does it look okay to you??

Not really, as ca_second and cb_first are both known to be the same position and known to be a single space.

Oh right, my bad. I will adjust it with your suggestions and send in a V2 then.

Thanks for your time.

Dwaipayan.
_______________________________________________
Linux-kernel-mentees mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees