* [PATCH] checkpatch: enable MAINTAINERS warning only for --strict,--subjective
@ 2014-12-17 7:35 Brian Norris
2014-12-17 9:27 ` Joe Perches
2014-12-18 20:12 ` [PATCH] checkpatch: Fix MAINTAINERS search pattern Derek Chickles
0 siblings, 2 replies; 8+ messages in thread
From: Brian Norris @ 2014-12-17 7:35 UTC (permalink / raw)
To: Joe Perches; +Cc: Andy Whitcroft, Kevin Cernekee, Brian Norris, linux-kernel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] checkpatch: enable MAINTAINERS warning only for --strict,--subjective
2014-12-17 7:35 [PATCH] checkpatch: enable MAINTAINERS warning only for --strict,--subjective Brian Norris
@ 2014-12-17 9:27 ` Joe Perches
2014-12-17 15:53 ` Kevin Cernekee
2014-12-18 20:12 ` [PATCH] checkpatch: Fix MAINTAINERS search pattern Derek Chickles
1 sibling, 1 reply; 8+ messages in thread
From: Joe Perches @ 2014-12-17 9:27 UTC (permalink / raw)
To: Brian Norris; +Cc: Andy Whitcroft, Kevin Cernekee, linux-kernel
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] checkpatch: enable MAINTAINERS warning only for --strict,--subjective
2014-12-17 9:27 ` Joe Perches
@ 2014-12-17 15:53 ` Kevin Cernekee
2014-12-17 17:26 ` Joe Perches
0 siblings, 1 reply; 8+ messages in thread
From: Kevin Cernekee @ 2014-12-17 15:53 UTC (permalink / raw)
To: Joe Perches; +Cc: Brian Norris, Andy Whitcroft, linux-kernel
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?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] checkpatch: enable MAINTAINERS warning only for --strict,--subjective
2014-12-17 15:53 ` Kevin Cernekee
@ 2014-12-17 17:26 ` Joe Perches
0 siblings, 0 replies; 8+ messages in thread
From: Joe Perches @ 2014-12-17 17:26 UTC (permalink / raw)
To: Kevin Cernekee; +Cc: Brian Norris, Andy Whitcroft, linux-kernel
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"
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] checkpatch: Fix MAINTAINERS search pattern
2014-12-17 7:35 [PATCH] checkpatch: enable MAINTAINERS warning only for --strict,--subjective Brian Norris
2014-12-17 9:27 ` Joe Perches
@ 2014-12-18 20:12 ` Derek Chickles
2014-12-18 21:17 ` Joe Perches
1 sibling, 1 reply; 8+ messages in thread
From: Derek Chickles @ 2014-12-18 20:12 UTC (permalink / raw)
To: joe
Cc: apw, computersforpeace, linux-kernel, cernekee, Derek Chickles,
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] checkpatch: Fix MAINTAINERS search pattern
2014-12-18 20:12 ` [PATCH] checkpatch: Fix MAINTAINERS search pattern Derek Chickles
@ 2014-12-18 21:17 ` Joe Perches
2014-12-19 2:34 ` Chickles, Derek
0 siblings, 1 reply; 8+ messages in thread
From: Joe Perches @ 2014-12-18 21:17 UTC (permalink / raw)
To: Derek Chickles
Cc: apw, computersforpeace, linux-kernel, cernekee, Derek Chickles
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] checkpatch: Fix MAINTAINERS search pattern
2014-12-18 21:17 ` Joe Perches
@ 2014-12-19 2:34 ` Chickles, Derek
2014-12-19 3:57 ` Joe Perches
0 siblings, 1 reply; 8+ messages in thread
From: Chickles, Derek @ 2014-12-19 2:34 UTC (permalink / raw)
To: Joe Perches; +Cc: apw, computersforpeace, linux-kernel, cernekee
> 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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] checkpatch: Fix MAINTAINERS search pattern
2014-12-19 2:34 ` Chickles, Derek
@ 2014-12-19 3:57 ` Joe Perches
0 siblings, 0 replies; 8+ messages in thread
From: Joe Perches @ 2014-12-19 3:57 UTC (permalink / raw)
To: Chickles, Derek; +Cc: apw, computersforpeace, linux-kernel, cernekee
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-12-19 3:57 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-17 7:35 [PATCH] checkpatch: enable MAINTAINERS warning only for --strict,--subjective Brian Norris
2014-12-17 9:27 ` Joe Perches
2014-12-17 15:53 ` Kevin Cernekee
2014-12-17 17:26 ` Joe Perches
2014-12-18 20:12 ` [PATCH] checkpatch: Fix MAINTAINERS search pattern Derek Chickles
2014-12-18 21:17 ` Joe Perches
2014-12-19 2:34 ` Chickles, Derek
2014-12-19 3:57 ` Joe Perches
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.