Skip to contentSkip to conversation

checkpatch: enable MAINTAINERS warning only for --strict,--subjective

PATCH In review Brian Norris 6 participants 8 messages Dec 17, 2014 – Dec 19, 2014 Checkpatch

lore
Share & tools
lore

Brian Norris

author
#0 lore raw

PATCH 1/1 checkpatch: enable MAINTAINERS warning only for --strict,--subjective

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(-)
scripts/checkpatch.pl +1 -1
index f0bb6d60c07b..7057d1f54947 100755
@@ -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*\}/ &&
Signature · 1 line
1.9.1
maintainer
#1 lore raw
Quoted 3 lines · 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.

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.

#2 lore raw
Quoted 10 lines · Joe Perches
Quoted 3 lines · 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.

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?

#3 lore raw
Quoted 12 lines · Kevin Cernekee
Quoted 10 lines · Joe Perches
Quoted 3 lines · 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.

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.

Quoted 7 lines

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"

#4 lore raw

PATCH 1/1 checkpatch: Fix MAINTAINERS search pattern

In reply to Brian Norris

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(-)
scripts/checkpatch.pl +1 -1
index f0bb6d6..6f10ebb 100755
@@ -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;
 		}
Signature · 1 line
1.8.3.1
maintainer
#5 lore raw

Re: [PATCH] checkpatch: Fix MAINTAINERS search pattern

Quoted 3 lines · 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.

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.

#6 lore raw

RE: [PATCH] checkpatch: Fix MAINTAINERS search pattern

Quoted 5 lines · Derek Chickles
Quoted 3 lines

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.

Quoted 8 lines

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.

maintainer
#7 lore raw

Re: [PATCH] checkpatch: Fix MAINTAINERS search pattern

Quoted 10 lines · Derek
Quoted 10 lines · Derek Chickles
Quoted 3 lines

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.

Quoted 8 lines
Quoted 3 lines

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.