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 <[email protected]>
---
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
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.
On Wed, Dec 17, 2014 at 1:27 AM, Joe Perches <[email protected]> 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?
On Wed, 2014-12-17 at 07:53 -0800, Kevin Cernekee wrote:
> On Wed, Dec 17, 2014 at 1:27 AM, Joe Perches <[email protected]> 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"
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 <[email protected]>
---
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
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.
> 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.
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.