Presence of hexadecimal address or symbol results in false warning
message by checkpatch.pl.
For example, running checkpatch on commit b8ad540dd4e4 ("mptcp: fix
memory leak in mptcp_subflow_create_socket()") results in warning:
WARNING:REPEATED_WORD: Possible repeated word: 'ff'
00 00 00 00 00 00 00 00 00 2f 30 0a 81 88 ff ff ........./0.....
Similarly, the presence of list command output in commit results in
an unnecessary warning.
For example, running checkpatch on commit 899e5ffbf246 ("perf record:
Introduce --switch-output-event") gives:
WARNING:REPEATED_WORD: Possible repeated word: 'root'
dr-xr-x---. 12 root root 4096 Apr 27 17:46 ..
Here, it reports 'ff' and 'root to be repeated, but it is in fact part
of some address or code, where it has to be repeated.
In these cases, the intent of the warning to find stylistic issues in
commit messages is not met and the warning is just completely wrong in
this case.
To avoid these warnings, add additional regex check for the
directory permission pattern and avoid checking the line for this
class of warning. Similarly, to avoid hex pattern, check if the word
consists of hex symbols and skip this warning if it is not among the
common english words formed using hex letters.
A quick evaluation on v5.6..v5.8 showed that this fix reduces
REPEATED_WORD warnings from 2797 to 907.
A quick manual check found all cases are related to hex output or
list command outputs in commit messages.
Signed-off-by: Aditya Srivastava <[email protected]>
---
scripts/checkpatch.pl | 15 ++++++++++++++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7e505688257a..1d42d08d520b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3049,7 +3049,9 @@ sub process {
}
# check for repeated words separated by a single space
- if ($rawline =~ /^\+/ || $in_commit_log) {
+# avoid false positive from list command eg, '-rw-r--r-- 1 root root'
+ if (($rawline =~ /^\+/ || $in_commit_log) &&
+ $rawline !~ /[bcCdDlMnpPs\?-][rwxsStT-]{9}/) {
pos($rawline) = 1 if (!$in_commit_log);
while ($rawline =~ /\b($word_pattern) (?=($word_pattern))/g) {
@@ -3074,6 +3076,17 @@ sub process {
next if ($start_char =~ /^\S$/);
next if (index(" \t.,;?!", $end_char) == -1);
+ # avoid repeating hex occurrences like 'ff ff fe 09 ...'
+ my %allow_repeated_words = (
+ add => '',
+ added => '',
+ bad => '',
+ be => '',
+ );
+ if ($first =~ /\b[0-9a-f]{2,}\b/) {
+ next if (!exists($allow_repeated_words{lc($first)}));
+ }
+
if (WARN("REPEATED_WORD",
"Possible repeated word: '$first'\n" . $herecurr) &&
$fix) {
--
2.17.1
On 24/10/20 5:38 am, Aditya Srivastava wrote:
> Presence of hexadecimal address or symbol results in false warning
> message by checkpatch.pl.
>
> For example, running checkpatch on commit b8ad540dd4e4 ("mptcp: fix
> memory leak in mptcp_subflow_create_socket()") results in warning:
>
> WARNING:REPEATED_WORD: Possible repeated word: 'ff'
> 00 00 00 00 00 00 00 00 00 2f 30 0a 81 88 ff ff ........./0.....
>
> Similarly, the presence of list command output in commit results in
> an unnecessary warning.
>
> For example, running checkpatch on commit 899e5ffbf246 ("perf record:
> Introduce --switch-output-event") gives:
>
> WARNING:REPEATED_WORD: Possible repeated word: 'root'
> dr-xr-x---. 12 root root 4096 Apr 27 17:46 ..
>
> Here, it reports 'ff' and 'root to be repeated, but it is in fact part
> of some address or code, where it has to be repeated.
>
> In these cases, the intent of the warning to find stylistic issues in
> commit messages is not met and the warning is just completely wrong in
> this case.
>
> To avoid these warnings, add additional regex check for the
> directory permission pattern and avoid checking the line for this
> class of warning. Similarly, to avoid hex pattern, check if the word
> consists of hex symbols and skip this warning if it is not among the
> common english words formed using hex letters.
>
> A quick evaluation on v5.6..v5.8 showed that this fix reduces
> REPEATED_WORD warnings from 2797 to 907.
>
> A quick manual check found all cases are related to hex output or
> list command outputs in commit messages.
>
> Signed-off-by: Aditya Srivastava <[email protected]>
> ---
> scripts/checkpatch.pl | 15 ++++++++++++++-
> 1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7e505688257a..1d42d08d520b 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3049,7 +3049,9 @@ sub process {
> }
>
> # check for repeated words separated by a single space
> - if ($rawline =~ /^\+/ || $in_commit_log) {
> +# avoid false positive from list command eg, '-rw-r--r-- 1 root root'
> + if (($rawline =~ /^\+/ || $in_commit_log) &&
> + $rawline !~ /[bcCdDlMnpPs\?-][rwxsStT-]{9}/) {
> pos($rawline) = 1 if (!$in_commit_log);
> while ($rawline =~ /\b($word_pattern) (?=($word_pattern))/g) {
>
> @@ -3074,6 +3076,17 @@ sub process {
> next if ($start_char =~ /^\S$/);
> next if (index(" \t.,;?!", $end_char) == -1);
>
> + # avoid repeating hex occurrences like 'ff ff fe 09 ...'
> + my %allow_repeated_words = (
> + add => '',
> + added => '',
> + bad => '',
> + be => '',
> + );
> + if ($first =~ /\b[0-9a-f]{2,}\b/) {
> + next if (!exists($allow_repeated_words{lc($first)}));
> + }
> +
> if (WARN("REPEATED_WORD",
> "Possible repeated word: '$first'\n" . $herecurr) &&
> $fix) {
>
Changes made in v4:
- Fix indentation
- Reduce the dictionary size for allowed words
- Apply changes over Dwaipayan's patch
(https://lore.kernel.org/linux-kernel-mentees/[email protected]/)
On Sat, 2020-10-24 at 05:38 +0530, Aditya Srivastava wrote:
> Presence of hexadecimal address or symbol results in false warning
> message by checkpatch.pl.
>
> For example, running checkpatch on commit b8ad540dd4e4 ("mptcp: fix
> memory leak in mptcp_subflow_create_socket()") results in warning:
>
> WARNING:REPEATED_WORD: Possible repeated word: 'ff'
> ????00 00 00 00 00 00 00 00 00 2f 30 0a 81 88 ff ff ........./0.....
>
> Similarly, the presence of list command output in commit results in
> an unnecessary warning.
>
> For example, running checkpatch on commit 899e5ffbf246 ("perf record:
> Introduce --switch-output-event") gives:
>
> WARNING:REPEATED_WORD: Possible repeated word: 'root'
> ??dr-xr-x---. 12 root root 4096 Apr 27 17:46 ..
>
> Here, it reports 'ff' and 'root to be repeated, but it is in fact part
'root'
> of some address or code, where it has to be repeated.
>
> In these cases, the intent of the warning to find stylistic issues in
> commit messages is not met and the warning is just completely wrong in
> this case.
>
> To avoid these warnings, add additional regex check for the
add an
> directory permission pattern and avoid checking the line for this
> class of warning. Similarly, to avoid hex pattern, check if the word
> consists of hex symbols and skip this warning if it is not among the
> common english words formed using hex letters.
>
> A quick evaluation on v5.6..v5.8 showed that this fix reduces
> REPEATED_WORD warnings from 2797 to 907.
How many of these 907 remaining are still false positive?
> A quick manual check found all cases are related to hex output or
> list command outputs in commit messages.
You mean 1890 of the 2797 are now no longer reported and all 1890
were false positives yes?
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3049,7 +3049,9 @@ sub process {
> ? }
> ?
>
> ?# check for repeated words separated by a single space
> - if ($rawline =~ /^\+/ || $in_commit_log) {
> +# avoid false positive from list command eg, '-rw-r--r-- 1 root root'
> + if (($rawline =~ /^\+/ || $in_commit_log) &&
> + $rawline !~ /[bcCdDlMnpPs\?-][rwxsStT-]{9}/) {
Use maximal tab indentation and spaces to align please.
2 tabs, 4 spaces
> ? pos($rawline) = 1 if (!$in_commit_log);
> ? while ($rawline =~ /\b($word_pattern) (?=($word_pattern))/g) {
> ?
>
> @@ -3074,6 +3076,17 @@ sub process {
> ? next if ($start_char =~ /^\S$/);
> ? next if (index(" \t.,;?!", $end_char) == -1);
> ?
>
> + # avoid repeating hex occurrences like 'ff ff fe 09 ...'
> + my %allow_repeated_words = (
> + add => '',
> + added => '',
> + bad => '',
> + be => '',
> + );
If perl caches this local hash declaration, fine,
but I think it better to use 'our %allow_repeated_words'
and move it so it's only declared using the file scope.
> + if ($first =~ /\b[0-9a-f]{2,}\b/) {
This regex matches only lower case so it wouldn't match "Add".
I think this regex would be clearer using
/^[0-9a-f]+$/i or /^[A-Fa-f0-9]+$/
On 24/10/20 7:07 am, Joe Perches wrote:
> On Sat, 2020-10-24 at 05:38 +0530, Aditya Srivastava wrote:
>> A quick evaluation on v5.6..v5.8 showed that this fix reduces
>> REPEATED_WORD warnings from 2797 to 907.
>
> How many of these 907 remaining are still false positive?
>
>> A quick manual check found all cases are related to hex output or
>> list command outputs in commit messages.
>
> You mean 1890 of the 2797 are now no longer reported and all 1890
> were false positives yes?
>
Yes. In v5.6..5.8, there were 2797 warnings for REPEATED_WORD, after
these changes, they are reduced to 907.
However, many among these 907 must have been fixed by Dwaipayan's
patch. I'll replace it with 1890 instead, for the better.
>> pos($rawline) = 1 if (!$in_commit_log);
>> while ($rawline =~ /\b($word_pattern) (?=($word_pattern))/g) {
>>
>>
>> @@ -3074,6 +3076,17 @@ sub process {
>> next if ($start_char =~ /^\S$/);
>> next if (index(" \t.,;?!", $end_char) == -1);
>>
>>
>> + # avoid repeating hex occurrences like 'ff ff fe 09 ...'
>> + my %allow_repeated_words = (
>> + add => '',
>> + added => '',
>> + bad => '',
>> + be => '',
>> + );
>
> If perl caches this local hash declaration, fine,
> but I think it better to use 'our %allow_repeated_words'
> and move it so it's only declared using the file scope.
>
I ran checkpatch over few commits, it was working fine. But I'll move
it to file scope, using 'our'. That should do as well.
>> + if ($first =~ /\b[0-9a-f]{2,}\b/) {
>
> This regex matches only lower case so it wouldn't match "Add".
>
> I think this regex would be clearer using
> /^[0-9a-f]+$/i or /^[A-Fa-f0-9]+$/
>
>
Missed it. Will do.
Thanks
Aditya