[Linux-kernel-mentees] [PATCH RFC] checkpatch: add new exceptions to repeated word check

[Linux-kernel-mentees] [PATCH RFC] checkpatch: add new exceptions to repeated word check

[Dwaipayan Ray]

The repeated word check generated false positives for
the following pattern:

"git git://..."

This resulted in lots of warnings in the MAINTAINERS file
which shouldn't have been generated.

Fixed this issue by adding "git" to the exception list
for repeated word check.

In addition, modified $word_pattern to capture words which
may contain a single letter like "a".
This ensures that repeated word warning in cases like
"This is a a repetition" are also generated.

Link: https://lore.kernel.org/linux-kernel-mentees/b6cd81b936671a8868fe98536d7c80771bdfd61c.camel@perches.com/

Suggested-by: Joe Perches <joe@perches.com>
Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
---
 scripts/checkpatch.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 124ff9432b51..0ef75d2fb5cf 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -588,7 +588,7 @@ our @mode_permission_funcs = (
 	["__ATTR", 2],
 );
 
-my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';
+my $word_pattern = '\b[A-Z]?[a-z]{1,}\b';
 
 #Create a search pattern for all these functions to speed up a loop below
 our $mode_perms_search = "";
@@ -3364,7 +3364,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) &&
-- 
2.27.0

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH RFC] checkpatch: add new exceptions to repeated word check

[Dwaipayan Ray]

On Sun, Oct 11, 2020 at 12:14 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>
> The repeated word check generated false positives for
> the following pattern:
>
> "git git://..."
>
> This resulted in lots of warnings in the MAINTAINERS file
> which shouldn't have been generated.
>
> Fixed this issue by adding "git" to the exception list
> for repeated word check.
>
> In addition, modified $word_pattern to capture words which
> may contain a single letter like "a".
> This ensures that repeated word warning in cases like
> "This is a a repetition" are also generated.
>
> Link: https://lore.kernel.org/linux-kernel-mentees/b6cd81b936671a8868fe98536d7c80771bdfd61c.camel@perches.com/
>
> Suggested-by: Joe Perches <joe@perches.com>
> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> ---
>  scripts/checkpatch.pl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 124ff9432b51..0ef75d2fb5cf 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -588,7 +588,7 @@ our @mode_permission_funcs = (
>         ["__ATTR", 2],
>  );
>
> -my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';
> +my $word_pattern = '\b[A-Z]?[a-z]{1,}\b';
>
>  #Create a search pattern for all these functions to speed up a loop below
>  our $mode_perms_search = "";
> @@ -3364,7 +3364,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) &&
> --
> 2.27.0
>

One more additional improvement to this would be to make
comparison case neutral.

Like:

 -  next if ($first ne $second);
+  next if (lc($first) ne lc($second));

What do you think?
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH RFC] checkpatch: add new exceptions to repeated word check

[Lukas Bulwahn]

On Sun, 11 Oct 2020, Dwaipayan Ray wrote:

> The repeated word check generated false positives for
> the following pattern:
> 
> "git git://..."
> 
> This resulted in lots of warnings in the MAINTAINERS file
> which shouldn't have been generated.
> 
> Fixed this issue by adding "git" to the exception list
> for repeated word check.
> 
> In addition, modified $word_pattern to capture words which
> may contain a single letter like "a".
> This ensures that repeated word warning in cases like
> "This is a a repetition" are also generated.
> 
> Link: https://lore.kernel.org/linux-kernel-mentees/b6cd81b936671a8868fe98536d7c80771bdfd61c.camel@perches.com/
> 
> Suggested-by: Joe Perches <joe@perches.com>
> Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>

I do not have any explicit comments.

Just two comments:

1. A patch should do/change ONE thing.

You are changing two here; just split the patch in two changes; the kernel 
community can handle many many patches.

2. For the reducing it to single-letter repetitions, I suggest to do a 
proper evaluation first. I can imagine that there are many more false 
positives than true positives in that case. Please evaluate.


Lukas

> ---
>  scripts/checkpatch.pl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 124ff9432b51..0ef75d2fb5cf 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -588,7 +588,7 @@ our @mode_permission_funcs = (
>  	["__ATTR", 2],
>  );
>  
> -my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';
> +my $word_pattern = '\b[A-Z]?[a-z]{1,}\b';
>  
>  #Create a search pattern for all these functions to speed up a loop below
>  our $mode_perms_search = "";
> @@ -3364,7 +3364,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) &&
> -- 
> 2.27.0
> 
> 
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH RFC] checkpatch: add new exceptions to repeated word check

[Lukas Bulwahn]

On Sun, 11 Oct 2020, Dwaipayan Ray wrote:

> On Sun, Oct 11, 2020 at 12:14 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
> >
> > The repeated word check generated false positives for
> > the following pattern:
> >
> > "git git://..."
> >
> > This resulted in lots of warnings in the MAINTAINERS file
> > which shouldn't have been generated.
> >
> > Fixed this issue by adding "git" to the exception list
> > for repeated word check.
> >
> > In addition, modified $word_pattern to capture words which
> > may contain a single letter like "a".
> > This ensures that repeated word warning in cases like
> > "This is a a repetition" are also generated.
> >
> > Link: https://lore.kernel.org/linux-kernel-mentees/b6cd81b936671a8868fe98536d7c80771bdfd61c.camel@perches.com/
> >
> > Suggested-by: Joe Perches <joe@perches.com>
> > Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> > ---
> >  scripts/checkpatch.pl | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 124ff9432b51..0ef75d2fb5cf 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -588,7 +588,7 @@ our @mode_permission_funcs = (
> >         ["__ATTR", 2],
> >  );
> >
> > -my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';
> > +my $word_pattern = '\b[A-Z]?[a-z]{1,}\b';
> >
> >  #Create a search pattern for all these functions to speed up a loop below
> >  our $mode_perms_search = "";
> > @@ -3364,7 +3364,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) &&
> > --
> > 2.27.0
> >
> 
> One more additional improvement to this would be to make
> comparison case neutral.
> 
> Like:
> 
>  -  next if ($first ne $second);
> +  next if (lc($first) ne lc($second));
> 
> What do you think?
> 

Sounds good. Please evaluate though. Then, we can really judge better.

Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH RFC] checkpatch: add new exceptions to repeated word check

[Dwaipayan Ray]

On Sun, Oct 11, 2020 at 11:07 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
>
>
>
> On Sun, 11 Oct 2020, Dwaipayan Ray wrote:
>
> > On Sun, Oct 11, 2020 at 12:14 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
> > >
> > > The repeated word check generated false positives for
> > > the following pattern:
> > >
> > > "git git://..."
> > >
> > > This resulted in lots of warnings in the MAINTAINERS file
> > > which shouldn't have been generated.
> > >
> > > Fixed this issue by adding "git" to the exception list
> > > for repeated word check.
> > >
> > > In addition, modified $word_pattern to capture words which
> > > may contain a single letter like "a".
> > > This ensures that repeated word warning in cases like
> > > "This is a a repetition" are also generated.
> > >
> > > Link: https://lore.kernel.org/linux-kernel-mentees/b6cd81b936671a8868fe98536d7c80771bdfd61c.camel@perches.com/
> > >
> > > Suggested-by: Joe Perches <joe@perches.com>
> > > Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > > Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> > > ---
> > >  scripts/checkpatch.pl | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index 124ff9432b51..0ef75d2fb5cf 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -588,7 +588,7 @@ our @mode_permission_funcs = (
> > >         ["__ATTR", 2],
> > >  );
> > >
> > > -my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';
> > > +my $word_pattern = '\b[A-Z]?[a-z]{1,}\b';
> > >
> > >  #Create a search pattern for all these functions to speed up a loop below
> > >  our $mode_perms_search = "";
> > > @@ -3364,7 +3364,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) &&
> > > --
> > > 2.27.0
> > >
> >
> > One more additional improvement to this would be to make
> > comparison case neutral.
> >
> > Like:
> >
> >  -  next if ($first ne $second);
> > +  next if (lc($first) ne lc($second));
> >
> > What do you think?
> >
>
> Sounds good. Please evaluate though. Then, we can really judge better.
>

Sure, I will run both the evaluations:
1) For case insenstitive matching
2) For single letter words

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

Re: [Linux-kernel-mentees] [PATCH RFC] checkpatch: add new exceptions to repeated word check

[Dwaipayan Ray]

On Sun, Oct 11, 2020 at 11:12 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
>
> On Sun, Oct 11, 2020 at 11:07 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> >
> >
> >
> > On Sun, 11 Oct 2020, Dwaipayan Ray wrote:
> >
> > > On Sun, Oct 11, 2020 at 12:14 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
> > > >
> > > > The repeated word check generated false positives for
> > > > the following pattern:
> > > >
> > > > "git git://..."
> > > >
> > > > This resulted in lots of warnings in the MAINTAINERS file
> > > > which shouldn't have been generated.
> > > >
> > > > Fixed this issue by adding "git" to the exception list
> > > > for repeated word check.
> > > >
> > > > In addition, modified $word_pattern to capture words which
> > > > may contain a single letter like "a".
> > > > This ensures that repeated word warning in cases like
> > > > "This is a a repetition" are also generated.
> > > >
> > > > Link: https://lore.kernel.org/linux-kernel-mentees/b6cd81b936671a8868fe98536d7c80771bdfd61c.camel@perches.com/
> > > >
> > > > Suggested-by: Joe Perches <joe@perches.com>
> > > > Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > > > Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> > > > ---
> > > >  scripts/checkpatch.pl | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > > index 124ff9432b51..0ef75d2fb5cf 100755
> > > > --- a/scripts/checkpatch.pl
> > > > +++ b/scripts/checkpatch.pl
> > > > @@ -588,7 +588,7 @@ our @mode_permission_funcs = (
> > > >         ["__ATTR", 2],
> > > >  );
> > > >
> > > > -my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';
> > > > +my $word_pattern = '\b[A-Z]?[a-z]{1,}\b';
> > > >
> > > >  #Create a search pattern for all these functions to speed up a loop below
> > > >  our $mode_perms_search = "";
> > > > @@ -3364,7 +3364,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) &&
> > > > --
> > > > 2.27.0
> > > >
> > >
> > > One more additional improvement to this would be to make
> > > comparison case neutral.
> > >
> > > Like:
> > >
> > >  -  next if ($first ne $second);
> > > +  next if (lc($first) ne lc($second));
> > >
> > > What do you think?
> > >
> >
> > Sounds good. Please evaluate though. Then, we can really judge better.
> >
>
> Sure, I will run both the evaluations:
> 1) For case insenstitive matching
> 2) For single letter words
>
> Thanks,
> Dwaipayan.

I did run an evaulation for the single letter case
and though it caught one of the lurking 'a's, like in:

commit 8d90822643ad ("crypto: engine - support for batch requests")

WARNING:REPEATED_WORD: Possible repeated word: 'a'
#53: FILE: crypto/crypto_engine.c:472:
+ * @cbk_do_batch: pointer to a callback function to be invoked when executing a
+ *                a batch of requests.


But yikes, it introduced new false positive:

commit 6514b25d3fba ("lpfc: Refactor Send LS Request support")

WARNING:REPEATED_WORD: Possible repeated word: 'x'
#156: FILE: drivers/scsi/lpfc/lpfc_nvme.c:614:
+ "Data: x%x x%x  rc x%x\n",

It's probably a bad idea to allow single lettered checking. :-[

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

Re: [Linux-kernel-mentees] [PATCH RFC] checkpatch: add new exceptions to repeated word check

[Lukas Bulwahn]

On Tue, 13 Oct 2020, Dwaipayan Ray wrote:

> On Sun, Oct 11, 2020 at 11:12 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
> >
> > On Sun, Oct 11, 2020 at 11:07 PM Lukas Bulwahn <lukas.bulwahn@gmail.com> wrote:
> > >
> > >
> > >
> > > On Sun, 11 Oct 2020, Dwaipayan Ray wrote:
> > >
> > > > On Sun, Oct 11, 2020 at 12:14 PM Dwaipayan Ray <dwaipayanray1@gmail.com> wrote:
> > > > >
> > > > > The repeated word check generated false positives for
> > > > > the following pattern:
> > > > >
> > > > > "git git://..."
> > > > >
> > > > > This resulted in lots of warnings in the MAINTAINERS file
> > > > > which shouldn't have been generated.
> > > > >
> > > > > Fixed this issue by adding "git" to the exception list
> > > > > for repeated word check.
> > > > >
> > > > > In addition, modified $word_pattern to capture words which
> > > > > may contain a single letter like "a".
> > > > > This ensures that repeated word warning in cases like
> > > > > "This is a a repetition" are also generated.
> > > > >
> > > > > Link: https://lore.kernel.org/linux-kernel-mentees/b6cd81b936671a8868fe98536d7c80771bdfd61c.camel@perches.com/
> > > > >
> > > > > Suggested-by: Joe Perches <joe@perches.com>
> > > > > Suggested-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > > > > Signed-off-by: Dwaipayan Ray <dwaipayanray1@gmail.com>
> > > > > ---
> > > > >  scripts/checkpatch.pl | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > > > index 124ff9432b51..0ef75d2fb5cf 100755
> > > > > --- a/scripts/checkpatch.pl
> > > > > +++ b/scripts/checkpatch.pl
> > > > > @@ -588,7 +588,7 @@ our @mode_permission_funcs = (
> > > > >         ["__ATTR", 2],
> > > > >  );
> > > > >
> > > > > -my $word_pattern = '\b[A-Z]?[a-z]{2,}\b';
> > > > > +my $word_pattern = '\b[A-Z]?[a-z]{1,}\b';
> > > > >
> > > > >  #Create a search pattern for all these functions to speed up a loop below
> > > > >  our $mode_perms_search = "";
> > > > > @@ -3364,7 +3364,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) &&
> > > > > --
> > > > > 2.27.0
> > > > >
> > > >
> > > > One more additional improvement to this would be to make
> > > > comparison case neutral.
> > > >
> > > > Like:
> > > >
> > > >  -  next if ($first ne $second);
> > > > +  next if (lc($first) ne lc($second));
> > > >
> > > > What do you think?
> > > >
> > >
> > > Sounds good. Please evaluate though. Then, we can really judge better.
> > >
> >
> > Sure, I will run both the evaluations:
> > 1) For case insenstitive matching
> > 2) For single letter words
> >
> > Thanks,
> > Dwaipayan.
> 
> I did run an evaulation for the single letter case
> and though it caught one of the lurking 'a's, like in:
> 
> commit 8d90822643ad ("crypto: engine - support for batch requests")
> 
> WARNING:REPEATED_WORD: Possible repeated word: 'a'
> #53: FILE: crypto/crypto_engine.c:472:
> + * @cbk_do_batch: pointer to a callback function to be invoked when executing a
> + *                a batch of requests.
> 
> 
> But yikes, it introduced new false positive:
> 
> commit 6514b25d3fba ("lpfc: Refactor Send LS Request support")
> 
> WARNING:REPEATED_WORD: Possible repeated word: 'x'
> #156: FILE: drivers/scsi/lpfc/lpfc_nvme.c:614:
> + "Data: x%x x%x  rc x%x\n",
> 
> It's probably a bad idea to allow single lettered checking. :-[
>

Yeah, I feared that...

I agree single letter checking is probably not a good idea if we do not 
refine that even more.

Maybe, we put that aside for now and look at other potential improvements.


Lukas
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees

Re: [Linux-kernel-mentees] [PATCH RFC] checkpatch: add new exceptions to repeated word check

[Dwaipayan Ray]

> > I did run an evaulation for the single letter case
> > and though it caught one of the lurking 'a's, like in:
> >
> > commit 8d90822643ad ("crypto: engine - support for batch requests")
> >
> > WARNING:REPEATED_WORD: Possible repeated word: 'a'
> > #53: FILE: crypto/crypto_engine.c:472:
> > + * @cbk_do_batch: pointer to a callback function to be invoked when executing a
> > + *                a batch of requests.
> >
> >
> > But yikes, it introduced new false positive:
> >
> > commit 6514b25d3fba ("lpfc: Refactor Send LS Request support")
> >
> > WARNING:REPEATED_WORD: Possible repeated word: 'x'
> > #156: FILE: drivers/scsi/lpfc/lpfc_nvme.c:614:
> > + "Data: x%x x%x  rc x%x\n",
> >
> > It's probably a bad idea to allow single lettered checking. :-[
> >
>
> Yeah, I feared that...
>
> I agree single letter checking is probably not a good idea if we do not
> refine that even more.
>
> Maybe, we put that aside for now and look at other potential improvements.
>
>
> Lukas

Yeah sure, so just quickly sending out the small patch for git modification.

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