checkpatch WARNING: patch prefix 'b' exists, appears to be a -p0 patch

PATCH Jonathan Cameron 4 participants 10 messages Oct 18, 2011 – Oct 19, 2011

Subsystems (5)

Arm (32-bit) Arm sub-architectures Input drivers PXA2xx/PXA3xx support +1 more subsystems

lore

Share & tools

lore

Jonathan Cameron

author

2011-10-18 15:03 UTC #0 lore raw

This started happening when I run checkpatch on pretty much anything.

triggered by the b in this snipped for example

arch/arm/mach-pxa/stargate2.c +0 -0

index 3f8d0af..62dc2a8 100644
@@ -54,6 +54,8 @@
 #include <linux/mfd/da903x.h>

Anyone else seeing this or have a clue what the heck is going on?

Jonathan

David Rientjes

2011-10-18 23:44 UTC #1 lore raw

Quoted 13 lines · Jonathan Cameron

This started happening when I run checkpatch on pretty much anything.
triggered by the b in this snipped for example
arch/arm/mach-pxa/stargate2.c +0 -0
index 3f8d0af..62dc2a8 100644
@@ -54,6 +54,8 @@
 #include <linux/mfd/da903x.h>
 
 

Quoted 2 lines

Anyone else seeing this or have a clue what the heck is going on?

I don't think anyone can say for sure if they're seeing what you're seeing since checkpatch will just complain that this is a corrupt patch. Could you send an actual diff that exhibits the problem?

Some people work internally with -p0-patches which has the danger that one forgets to convert them to -p1 before mainlining. Bitten myself and seen p0-patches in mailing lists occasionally, this patch adds a warning to checkpatch.pl in case a patch is -p0. If you really want, you can fool this check to generate false positives, this is why it just spits a warning. Making the check 100% proof is trickier than it looks, so let's start with a version which catches the cases of real use.

[[email protected]: update message language, handle null prefix, add tests]

Signed-off-by Wolfram Sang <[email protected]>

Signed-off-by Andy Whitcroft <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

Signed-off-by Linus Torvalds <[email protected]>

I've always ignored it.

David Rientjes

2011-10-19 01:27 UTC #3 lore raw

Quoted 12 lines · Joe Perches

Quoted 9 lines
Quoted 9 lines
This started happening when I run checkpatch on pretty much anything. triggered by the b in this snipped for example
arch/arm/mach-pxa/stargate2.c +0 -0
index 3f8d0af..62dc2a8 100644
@@ -54,6 +54,8 @@
 #include <linux/mfd/da903x.h>
Anyone else seeing this or have a clue what the heck is going on?
It's done this since:

Quoted 7 lines

$ git log -1 1e85572697b348b1a126520349a29654f2ae6a12
commit 1e85572697b348b1a126520349a29654f2ae6a12
Author: Wolfram Sang <[email protected]>
Date:   Tue Jan 6 14:41:24 2009 -0800

checkpatch: Add warning for p0-patches

Hmm, not sure how useful that is unless both prefixes ('a' and 'b') exist.

Jonathan Cameron

author

2011-10-19 08:27 UTC #4 lore raw

Quoted 19 lines · David Rientjes

Quoted 17 lines · Joe Perches
Quoted 9 lines
Quoted 9 lines
This started happening when I run checkpatch on pretty much anything. triggered by the b in this snipped for example
arch/arm/mach-pxa/stargate2.c +0 -0
index 3f8d0af..62dc2a8 100644
@@ -54,6 +54,8 @@
 #include <linux/mfd/da903x.h>
Anyone else seeing this or have a clue what the heck is going on?
It's done this since:
$ git log -1 1e85572697b348b1a126520349a29654f2ae6a12
commit 1e85572697b348b1a126520349a29654f2ae6a12
Author: Wolfram Sang <[email protected]>
Date:   Tue Jan 6 14:41:24 2009 -0800

Quoted 4 lines

Quoted 2 lines
checkpatch: Add warning for p0-patches
Hmm, not sure how useful that is unless both prefixes ('a' and 'b') exist.

Sorry, I missed one detail. This warning is firing on every patch including any I pull off mailing lists or produce with git format-patch

As you can imagine - this is rather tedious.

As a random example see the attached patch from yesterday.

Jonathan

Signed-off-by Ricardo Ribalda Delgado <[email protected]>

---
 drivers/input/misc/cma3000_d0x.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/input/misc/cma3000_d0x.c +1 -1

index bbda34c..96a46d4 100644
@@ -459,8 +459,8 @@ EXPORT_SYMBOL(cma3000_init);
 
 void cma3000_exit(struct cma3000_accl_data *data)
 {
-	free_irq(data->irq, data);
 	input_unregister_device(data->input_dev);
+	free_irq(data->irq, data);
 	kfree(data);
 }
 EXPORT_SYMBOL(cma3000_exit);

Signature · 1 line

1.7.7

Wolfram Sang

2011-10-19 10:26 UTC #5 lore raw

Quoted 3 lines

Sorry, I missed one detail. This warning is firing on every patch including any I pull off mailing lists or produce with git format-patch

So, I assume you have a directory named 'b' in the kernel-root-dir?

The p1-detection is heuristic and can always be fooled, so I don't think extending the logic will help. If too many users think it is annoying, it may be better to remove it. I do think, however, that it might help users sending in their first patches, so I'd prefer to keep it. Experienced users have the option to use something like "--ignore PATCH_PREFIX" in their .checkpatch.conf. I vote for improving the error message but won't oppose a removal if that is what is wanted.

Regards,

Wolfram

Signature · 2 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Jonathan Cameron

author

2011-10-19 11:04 UTC #6 lore raw

Quoted 5 lines · Wolfram Sang

Quoted 3 lines
Sorry, I missed one detail. This warning is firing on every patch including any I pull off mailing lists or produce with git format-patch
So, I assume you have a directory named 'b' in the kernel-root-dir?

File, but good call. Thanks. All back to normal now.
I'll have to be more creative with naming my random temporary diff files ;) (or remember to delete them afterwards).

Quoted 8 lines

The p1-detection is heuristic and can always be fooled, so I don't think extending the logic will help. If too many users think it is annoying, it may be better to remove it. I do think, however, that it might help users sending in their first patches, so I'd prefer to keep it. Experienced users have the option to use something like "--ignore PATCH_PREFIX" in their .checkpatch.conf. I vote for improving the error message but won't oppose a removal if that is what is wanted.

Maybe something as simple as a comment in the checkpatch source to say that such a file / directory can cause false positives?

Thanks again

Jonathan

Wolfram Sang

2011-10-19 11:22 UTC #7 lore raw

Quoted 2 lines

Maybe something as simple as a comment in the checkpatch source to say that such a file / directory can cause false positives?

I'd hope this error message would be clear enough, what do you think?

scripts/checkpatch.pl +1 -1

index 3dfc471..19e4de7 100755
@@ -1506,7 +1506,7 @@ sub process {
 			if (!$file && $tree && $p1_prefix ne '' &&
 			    -e "$root/$p1_prefix") {
 				WARN("PATCH_PREFIX",
-				     "patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
+				     "patch prefix '$p1_prefix' exists as file/directory. Make sure this isn't a -p0 patch\n");
 			}
 
 			if ($realfile =~ m@^include/asm/@) {

Signature · 2 lines

Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Jonathan Cameron

author Acked-by

2011-10-19 11:32 UTC #8 lore raw

Quoted 4 lines · Wolfram Sang

Quoted 2 lines
Maybe something as simple as a comment in the checkpatch source to say that such a file / directory can cause false positives?
I'd hope this error message would be clear enough, what do you think?

Would have told me what was going on!

Acked-by Jonathan Cameron <[email protected]>

Quoted 15 lines

scripts/checkpatch.pl +1 -1

index 3dfc471..19e4de7 100755
@@ -1506,7 +1506,7 @@ sub process {
 			if (!$file && $tree && $p1_prefix ne '' &&
 			    -e "$root/$p1_prefix") {
 				WARN("PATCH_PREFIX",
-				     "patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
+				     "patch prefix '$p1_prefix' exists as file/directory. Make sure this isn't a -p0 patch\n");
 			}
 
 			if ($realfile =~ m@^include/asm/@) {

Joe Perches

maintainer

2011-10-19 16:17 UTC #9 lore raw

In reply to Wolfram Sang

Quoted 11 lines · Wolfram Sang

Quoted 2 lines
Maybe something as simple as a comment in the checkpatch source to say that such a file / directory can cause false positives?
I'd hope this error message would be clear enough, what do you think?
scripts/checkpatch.pl +0 -0
index 3dfc471..19e4de7 100755

scripts/checkpatch.pl sub process { · line 1512

Quoted 8 lines

scripts/checkpatch.pl +1 -1

@@ -1506,7 +1506,7 @@ sub process {
 			if (!$file && $tree && $p1_prefix ne '' &&
 			    -e "$root/$p1_prefix") {
 				WARN("PATCH_PREFIX",
-				     "patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
+				     "patch prefix '$p1_prefix' exists as file/directory. Make sure this isn't a -p0 patch\n");
 			}
 
 			if ($realfile =~ m@^include/asm/@) {

How about:

 scripts/checkpatch.pl |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

scripts/checkpatch.pl +5 -5

index 2300964..6259b12 100755
@@ -1499,17 +1499,17 @@ sub process {
 			$realfile =~ s@^([^/]*)/@@;
 
 		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
+			my $patched_file = "$root/$1";
 			$realfile = $1;
 			$realfile =~ s@^([^/]*)/@@;
 
-			$p1_prefix = $1;
-			if (!$file && $tree && $p1_prefix ne '' &&
-			    -e "$root/$p1_prefix") {
+			if (!$file && $tree && $realfile ne '' &&
+			    -e "$patched_file") {
 				WARN("PATCH_PREFIX",
-				     "patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
+				     "patched file '$patched_file' exists, patch appears to be a -p0 patch\n");
 			}
 
-			if ($realfile =~ m@^include/asm/@) {
+			if ($patched_file =~ m@/include/asm/@) {
 				ERROR("MODIFIED_INCLUDE_ASM",
 				      "do not modify files in include/asm, change architecture specific files in include/asm-<architecture>\n" . "$here$rawline\n");
 			}