Skip to contentSkip to conversation

Checkpatch: False positive

DISCUSSION Viresh Kumar 3 participants 5 messages Jul 16, 2015 Checkpatch

lore
Share & tools
lore

Viresh Kumar

author
#0 lore raw

Hi Andy/Joe,

I got a warning today for my cover-letter, and it looked like a false positive. Please have a look, based of v4.2-rc2.

-----------------------
0000-cover-letter.patch
-----------------------
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) #31:

 arch/x86/kernel/hpet.c         | 198 ++++++++++++++++++++++++++---------------
Signature · 1 line
viresh
maintainer
#1 lore raw
Quoted 10 lines · Viresh Kumar

Hi Andy/Joe,

I got a warning today for my cover-letter, and it looked like a false positive. Please have a look, based of v4.2-rc2.
-----------------------
0000-cover-letter.patch
-----------------------
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line) #31:

 arch/x86/kernel/hpet.c         | 198 ++++++++++++++++++++++++++---------------

There are a lot of other false positives for this test.

Maybe it's not worth checking for this condition and the test should be removed as "fixing" it may not be feasible.

maintainer
#2 lore raw
Quoted 2 lines · Joe Perches
Quoted 2 lines

#31:

 arch/x86/kernel/hpet.c         | 198 ++++++++++++++++++++++++++---------------

I guess those are in the limbo land between the end of message and beginning of the patch itself. Perhaps the test should at least stop at the end of header marker, at the '---'.

-apw

maintainer
#3 lore raw
Quoted 9 lines · Andy Whitcroft
Quoted 2 lines · Joe Perches
Quoted 2 lines

#31:

 arch/x86/kernel/hpet.c         | 198 ++++++++++++++++++++++++++---------------

I guess those are in the limbo land between the end of message and beginning of the patch itself. Perhaps the test should at least stop at the end of header marker, at the '---'.

-apw

Maybe, but the test already stops at signatures like
Signed-off-by: that should always be above the ---.

This might help, but there are _many_ false positives.

The other thing that might help is for people to take the warnings the script produces less seriously.

Maybe convert:

ERROR -> defect
WARNING -> unstylish
CHECK -> nitpick

or some such

---

 scripts/checkpatch.pl | 5 +++++
 1 file changed, 5 insertions(+)
scripts/checkpatch.pl +5 -0
index d5ce29a..5e7afa7 100755
@@ -2213,6 +2213,11 @@ sub process {
 			$in_commit_log = 0;
 		}
 
+# Check for patch separator
+		if ($line =~ /^---$/) {
+			$in_commit_log = 0;
+		}
+
 # 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*\|/) {
maintainer
#4 lore raw
Quoted 16 lines · Joe Perches
Quoted 9 lines · Andy Whitcroft
Quoted 2 lines · Joe Perches
Quoted 2 lines

#31:

 arch/x86/kernel/hpet.c         | 198 ++++++++++++++++++++++++++---------------

I guess those are in the limbo land between the end of message and beginning of the patch itself. Perhaps the test should at least stop at the end of header marker, at the '---'.

-apw

Maybe, but the test already stops at signatures like
Signed-off-by: that should always be above the ---.

This might help, but there are _many_ false positives.

Quoted 8 lines

The other thing that might help is for people to take the warnings the script produces less seriously.

Maybe convert:

ERROR -> defect
WARNING -> unstylish
CHECK -> nitpick

Heh, that has long been the main issue, please please believe your brain not checkpatch. But yes some less inflamitory words might, just might, reduce the noise.

-apw