[PATCH] checkpatch: enable MAINTAINERS warning only for --strict,--subjective

[PATCH] checkpatch: enable MAINTAINERS warning only for --strict,--subjective

[Brian Norris]

The rule which delivers this warning is very prone to errors:

  "added, moved or deleted file(s), does MAINTAINERS need updating?"

so it should not be enabled by default.

The current checkpatch rule doesn't check:
 1. whether other patches in the same series update MAINTAINERS
 2. whether MAINTAINERS already contains a sufficient entry (e.g., with
    an appropriate wildcard pattern)

Given that #1 is impossible to fix and #2 is highly subjective (what's
"appropriate"? is the subsystem maintainer fine with maintaining a new
small driver? or should the driver author be adding himself/herself for
a niche driver?), I'd be just as happy to remove this rule entirely.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f0bb6d60c07b..7057d1f54947 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2189,7 +2189,7 @@ sub process {
 		}
 
 # Check for added, moved or deleted files
-		if (!$reported_maintainer_file && !$in_commit_log &&
+		if ($check && !$reported_maintainer_file && !$in_commit_log &&
 		    ($line =~ /^(?:new|deleted) file mode\s*\d+\s*$/ ||
 		     $line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
 		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
-- 
1.9.1

Re: [PATCH] checkpatch: enable MAINTAINERS warning only for --strict,--subjective

[Joe Perches]

On Tue, 2014-12-16 at 23:35 -0800, Brian Norris wrote:
> The rule which delivers this warning is very prone to errors:
>   "added, moved or deleted file(s), does MAINTAINERS need updating?"
> so it should not be enabled by default.

I don't think so.

It's _far_ more common for people to forget to
update MAINTAINERS so I think it's quite useful
even if it's slightly noisy.

Re: [PATCH] checkpatch: enable MAINTAINERS warning only for --strict,--subjective

[Kevin Cernekee]

On Wed, Dec 17, 2014 at 1:27 AM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2014-12-16 at 23:35 -0800, Brian Norris wrote:
>> The rule which delivers this warning is very prone to errors:
>>   "added, moved or deleted file(s), does MAINTAINERS need updating?"
>> so it should not be enabled by default.
>
> I don't think so.
>
> It's _far_ more common for people to forget to
> update MAINTAINERS so I think it's quite useful
> even if it's slightly noisy.

In a recent submission I saw a bunch of these warnings too, even
though I had already added MAINTAINERS entries.

If this is just a friendly reminder ("hey, please double-check X
before you send") rather than an indication that a suspected problem
was detected in the patch, perhaps it would be better if it lived in
Documentation/SubmitChecklist?

Re: [PATCH] checkpatch: enable MAINTAINERS warning only for --strict,--subjective

[Joe Perches]

On Wed, 2014-12-17 at 07:53 -0800, Kevin Cernekee wrote:
> On Wed, Dec 17, 2014 at 1:27 AM, Joe Perches <joe@perches.com> wrote:
> > On Tue, 2014-12-16 at 23:35 -0800, Brian Norris wrote:
> >> The rule which delivers this warning is very prone to errors:
> >>   "added, moved or deleted file(s), does MAINTAINERS need updating?"
> >> so it should not be enabled by default.
> >
> > I don't think so.
> >
> > It's _far_ more common for people to forget to
> > update MAINTAINERS so I think it's quite useful
> > even if it's slightly noisy.
> 
> In a recent submission I saw a bunch of these warnings too, even
> though I had already added MAINTAINERS entries.
> 
> If this is just a friendly reminder ("hey, please double-check X
> before you send") rather than an indication that a suspected problem
> was detected in the patch, perhaps it would be better if it lived in
> Documentation/SubmitChecklist?

An entry might help.

Submit a patch to SubmitChecklist and see if the
overall MAINTAINERS patterns error rate changes
over a few release cycles.

Another thing that might help you not see these
by creating a .checkpatch.conf file and adding:
"--ignore=FILE_PATH_CHANGES"

[PATCH] checkpatch: Fix MAINTAINERS search pattern

[Derek Chickles]

This fixes the check that was supposed to prevent the MAINTAINERS
warning from appearing when you run checkpatch.pl on a patch that
includes new files/directories and an updated MAINTAINERS file.

Signed-off-by: Derek Chickles <derek.chickles@caviumnetworks.com>
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f0bb6d6..6f10ebb 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2085,7 +2085,7 @@ sub process {
 
 # Check if MAINTAINERS is being updated.  If so, there's probably no need to
 # emit the "does MAINTAINERS need updating?" message on file add/move/delete
-		if ($line =~ /^\s*MAINTAINERS\s*\|/) {
+		if ($realfile =~ /MAINTAINERS/) {
 			$reported_maintainer_file = 1;
 		}
 
-- 
1.8.3.1

Re: [PATCH] checkpatch: Fix MAINTAINERS search pattern

[Joe Perches]

On Thu, 2014-12-18 at 12:12 -0800, Derek Chickles wrote:
> This fixes the check that was supposed to prevent the MAINTAINERS
> warning from appearing when you run checkpatch.pl on a patch that
> includes new files/directories and an updated MAINTAINERS file.

This suggested patch doesn't work.

$realfile isn't set until the file shows up
in the patch itself.

The current test looks at the changelog
before the files are modified and is correct.

RE: [PATCH] checkpatch: Fix MAINTAINERS search pattern

[Chickles, Derek]

> On Thu, 2014-12-18 at 12:12 -0800, Derek Chickles wrote:
> > This fixes the check that was supposed to prevent the MAINTAINERS
> > warning from appearing when you run checkpatch.pl on a patch that
> > includes new files/directories and an updated MAINTAINERS file.
> 
> This suggested patch doesn't work.
> 
> $realfile isn't set until the file shows up
> in the patch itself.
> 
> The current test looks at the changelog
> before the files are modified and is correct.
> 

Thanks, you're right. My mistake was trying to run checkpatch.pl on a diff.
My patch works on diffs while the original code does not. This is by chance
really because MAINTAINERS is early in the patch. Using format-patch on
something that is committed is clearly the correct solution.

Re: [PATCH] checkpatch: Fix MAINTAINERS search pattern

[Joe Perches]

On Fri, 2014-12-19 at 02:34 +0000, Chickles, Derek wrote:
> > On Thu, 2014-12-18 at 12:12 -0800, Derek Chickles wrote:
> > > This fixes the check that was supposed to prevent the MAINTAINERS
> > > warning from appearing when you run checkpatch.pl on a patch that
> > > includes new files/directories and an updated MAINTAINERS file.
> > 
> > This suggested patch doesn't work.
> > 
> > $realfile isn't set until the file shows up
> > in the patch itself.
> > 
> > The current test looks at the changelog
> > before the files are modified and is correct.
> > 
> 
> Thanks, you're right. My mistake was trying to run checkpatch.pl on a diff.
> My patch works on diffs while the original code does not. This is by chance
> really because MAINTAINERS is early in the patch. Using format-patch on
> something that is committed is clearly the correct solution.

Both tests can be used.

Send another patch without deleting the first test
and that'd be good to me.