LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH][RESEND] checkpatch: Add warning for p0-patches
@ 2008-10-31 15:36 Wolfram Sang
  2008-11-12 13:55 ` Andy Whitcroft
  0 siblings, 1 reply; 7+ messages in thread
From: Wolfram Sang @ 2008-10-31 15:36 UTC (permalink / raw)
  To: apw; +Cc: linux-kernel, Wolfram Sang

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.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 scripts/checkpatch.pl |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f88bb3e..dae5854 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1051,6 +1051,7 @@ sub process {
 	my $in_comment = 0;
 	my $comment_edge = 0;
 	my $first_line = 0;
+	my $p1_prefix = '';
 
 	my $prev_values = 'E';
 
@@ -1196,7 +1197,12 @@ sub process {
 		# extract the filename as it passes
 		if ($line=~/^\+\+\+\s+(\S+)/) {
 			$realfile = $1;
-			$realfile =~ s@^[^/]*/@@;
+			$realfile =~ s@^([^/]*)/@@;
+
+			$p1_prefix = $1;
+			if ($tree && -e "$root/$p1_prefix") {
+				WARN("Patch prefix '$p1_prefix' exists. Is it maybe a p0-patch?\n");
+			}
 
 			if ($realfile =~ m@^include/asm/@) {
 				ERROR("do not modify files in include/asm, change architecture specific files in include/asm-<architecture>\n" . "$here$rawline\n");
-- 
1.5.6.5


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH][RESEND] checkpatch: Add warning for p0-patches
  2008-10-31 15:36 [PATCH][RESEND] checkpatch: Add warning for p0-patches Wolfram Sang
@ 2008-11-12 13:55 ` Andy Whitcroft
  2008-11-12 14:23   ` Borislav Petkov
  0 siblings, 1 reply; 7+ messages in thread
From: Andy Whitcroft @ 2008-11-12 13:55 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: linux-kernel

On Fri, Oct 31, 2008 at 04:36:10PM +0100, Wolfram Sang wrote:
> 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.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> ---
>  scripts/checkpatch.pl |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f88bb3e..dae5854 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1051,6 +1051,7 @@ sub process {
>  	my $in_comment = 0;
>  	my $comment_edge = 0;
>  	my $first_line = 0;
> +	my $p1_prefix = '';
>  
>  	my $prev_values = 'E';
>  
> @@ -1196,7 +1197,12 @@ sub process {
>  		# extract the filename as it passes
>  		if ($line=~/^\+\+\+\s+(\S+)/) {
>  			$realfile = $1;
> -			$realfile =~ s@^[^/]*/@@;
> +			$realfile =~ s@^([^/]*)/@@;
> +
> +			$p1_prefix = $1;
> +			if ($tree && -e "$root/$p1_prefix") {
> +				WARN("Patch prefix '$p1_prefix' exists. Is it maybe a p0-patch?\n");
> +			}
>  
>  			if ($realfile =~ m@^include/asm/@) {
>  				ERROR("do not modify files in include/asm, change architecture specific files in include/asm-<architecture>\n" . "$here$rawline\n");

Looks reasonable.  Committed this with a few mods to my tree.  Will be
in the next batch of updates.

-apw

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH][RESEND] checkpatch: Add warning for p0-patches
  2008-11-12 13:55 ` Andy Whitcroft
@ 2008-11-12 14:23   ` Borislav Petkov
  2008-11-13 22:55     ` Wolfram Sang
  0 siblings, 1 reply; 7+ messages in thread
From: Borislav Petkov @ 2008-11-12 14:23 UTC (permalink / raw)
  To: Andy Whitcroft; +Cc: Wolfram Sang, linux-kernel

On Wed, Nov 12, 2008 at 2:55 PM, Andy Whitcroft <apw@shadowen.org> wrote:
> On Fri, Oct 31, 2008 at 04:36:10PM +0100, Wolfram Sang wrote:
>> 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.
>>
>> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
>> ---
>>  scripts/checkpatch.pl |    8 +++++++-
>>  1 files changed, 7 insertions(+), 1 deletions(-)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index f88bb3e..dae5854 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -1051,6 +1051,7 @@ sub process {
>>       my $in_comment = 0;
>>       my $comment_edge = 0;
>>       my $first_line = 0;
>> +     my $p1_prefix = '';
>>
>>       my $prev_values = 'E';
>>
>> @@ -1196,7 +1197,12 @@ sub process {
>>               # extract the filename as it passes
>>               if ($line=~/^\+\+\+\s+(\S+)/) {
>>                       $realfile = $1;
>> -                     $realfile =~ s@^[^/]*/@@;
>> +                     $realfile =~ s@^([^/]*)/@@;
>> +
>> +                     $p1_prefix = $1;
>> +                     if ($tree && -e "$root/$p1_prefix") {
>> +                             WARN("Patch prefix '$p1_prefix' exists. Is it maybe a p0-patch?\n");
>> +                     }
>>
>>                       if ($realfile =~ m@^include/asm/@) {
>>                               ERROR("do not modify files in include/asm, change architecture specific files in include/asm-<architecture>\n" . "$here$rawline\n");
>
> Looks reasonable.  Committed this with a few mods to my tree.  Will be
> in the next batch of updates.

Hi,

I had sent you a very similar patch (see http://lkml.org/lkml/2007/12/17/19) and
and you turned it down then since it would trigger when the patch creates new
files. Well, this one suffers from the exact opposite problem - it won't trigger
even if it is a -p0 patch on new files, AFAICT.

-- 
Regards/Gruss,
Boris

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH][RESEND] checkpatch: Add warning for p0-patches
  2008-11-12 14:23   ` Borislav Petkov
@ 2008-11-13 22:55     ` Wolfram Sang
  2008-11-14  6:49       ` Borislav Petkov
  0 siblings, 1 reply; 7+ messages in thread
From: Wolfram Sang @ 2008-11-13 22:55 UTC (permalink / raw)
  To: petkovbb; +Cc: Andy Whitcroft, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 371 bytes --]

Hello Boris,

> files. Well, this one suffers from the exact opposite problem - it won't trigger
> even if it is a -p0 patch on new files, AFAICT.

Can you give an example, please? I fail to see this at the moment.

All the best,

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH][RESEND] checkpatch: Add warning for p0-patches
  2008-11-13 22:55     ` Wolfram Sang
@ 2008-11-14  6:49       ` Borislav Petkov
  2008-11-14  9:38         ` Wolfram Sang
  0 siblings, 1 reply; 7+ messages in thread
From: Borislav Petkov @ 2008-11-14  6:49 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: Andy Whitcroft, linux-kernel

Hi Wolfram,

On Thu, Nov 13, 2008 at 11:55:40PM +0100, Wolfram Sang wrote:
> Hello Boris,
> 
> > files. Well, this one suffers from the exact opposite problem - it won't trigger
> > even if it is a -p0 patch on new files, AFAICT.
> 
> Can you give an example, please? I fail to see this at the moment.

Watch this:

Here's an arbitrary piece of a patch one could create:

--- /dev/null   2008-11-09 02:46:02.525014459 +0100
+++ arch/x86/kernel/tsc_resync.c        2008-11-14 07:22:34.000000000 +0100
@@ -0,0 +1 @@
+This is a new file

and, as you can see, it is a -p0 patch. Now, in the code you do:

if ($tree && -e "$root/$p1_prefix") {
	WARN("Patch prefix '$p1_prefix' exists. Is it maybe a p0-patch?\n");

and the "$root/$p1_prefix" won't exist - as a matter of fact - would
lose its "arch" part due to the regex before and the if-condition won't
trigger.

Nevertheless, this can be made to work if a special condition is added
which looks for "/dev/null" or similar strings which are a unique for a
patch adding a new file, or something like that. But I admit, this is a
contrived case and as such it is really rare in reality and you never
gonna have a problem like that if you use git or quilt :).

-- 
Regards/Gruss,
    Boris.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH][RESEND] checkpatch: Add warning for p0-patches
  2008-11-14  6:49       ` Borislav Petkov
@ 2008-11-14  9:38         ` Wolfram Sang
  2008-11-14 10:43           ` Borislav Petkov
  0 siblings, 1 reply; 7+ messages in thread
From: Wolfram Sang @ 2008-11-14  9:38 UTC (permalink / raw)
  To: petkovbb, Andy Whitcroft, linux-kernel

[-- Attachment #1: Type: text/plain, Size: 1601 bytes --]

Hi Boris,

> --- /dev/null   2008-11-09 02:46:02.525014459 +0100
> +++ arch/x86/kernel/tsc_resync.c        2008-11-14 07:22:34.000000000 +0100
> @@ -0,0 +1 @@
> +This is a new file
> 
> and, as you can see, it is a -p0 patch. Now, in the code you do:
> 
> if ($tree && -e "$root/$p1_prefix") {
> 	WARN("Patch prefix '$p1_prefix' exists. Is it maybe a p0-patch?\n");
> 
> and the "$root/$p1_prefix" won't exist - as a matter of fact - would
> lose its "arch" part due to the regex before and the if-condition won't
> trigger.

Careful. My approach is a bit different (inverse so to say) from yours
which I missed back then. $p1_prefix is the part which _was_ cut off and
it is wrong if it _does_ exist. See:

-                       $realfile =~ s@^[^/]*/@@;
+                       $realfile =~ s@^([^/]*)/@@;
+
+                       $p1_prefix = $1;

(So, a way to fool this algorithm is to give your kernel root dir the
same name as a directory inside the root dir, like:

--- drivers.orig/drivers/...
+++ drivers/drivers/...

This will generate a false positive. Oh, well...)

I decided to go this way intentionally to handle the new file problem.
So, in your case (I tried) it will cut off "arch", find the "arch"
directory and will complain. (Did you actually apply this patch? ;))

I thought the variable name 'p1_prefix' would speak for itself, but as
you misinterpreted it, maybe it should be renamed?

All the best,

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH][RESEND] checkpatch: Add warning for p0-patches
  2008-11-14  9:38         ` Wolfram Sang
@ 2008-11-14 10:43           ` Borislav Petkov
  0 siblings, 0 replies; 7+ messages in thread
From: Borislav Petkov @ 2008-11-14 10:43 UTC (permalink / raw)
  To: Wolfram Sang; +Cc: Andy Whitcroft, linux-kernel

Hi Wolfram,

On Fri, Nov 14, 2008 at 10:38 AM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> Hi Boris,
>
>> --- /dev/null   2008-11-09 02:46:02.525014459 +0100
>> +++ arch/x86/kernel/tsc_resync.c        2008-11-14 07:22:34.000000000 +0100
>> @@ -0,0 +1 @@
>> +This is a new file
>>
>> and, as you can see, it is a -p0 patch. Now, in the code you do:
>>
>> if ($tree && -e "$root/$p1_prefix") {
>>       WARN("Patch prefix '$p1_prefix' exists. Is it maybe a p0-patch?\n");
>>
>> and the "$root/$p1_prefix" won't exist - as a matter of fact - would
>> lose its "arch" part due to the regex before and the if-condition won't
>> trigger.
>
> Careful. My approach is a bit different (inverse so to say) from yours
> which I missed back then. $p1_prefix is the part which _was_ cut off and
> it is wrong if it _does_ exist. See:
>
> -                       $realfile =~ s@^[^/]*/@@;
> +                       $realfile =~ s@^([^/]*)/@@;
> +
> +                       $p1_prefix = $1;

Doh, of course. I _did_ misinterpret the $p1_prefix, sorry. I was too
concentrated on the $realfile mangling.

> (So, a way to fool this algorithm is to give your kernel root dir the
> same name as a directory inside the root dir, like:
>
> --- drivers.orig/drivers/...
> +++ drivers/drivers/...
>
> This will generate a false positive. Oh, well...)
>
> I decided to go this way intentionally to handle the new file problem.
> So, in your case (I tried) it will cut off "arch", find the "arch"
> directory and will complain. (Did you actually apply this patch? ;))
>
> I thought the variable name 'p1_prefix' would speak for itself, but as
> you misinterpreted it, maybe it should be renamed?

No, keep it that way, for others who _can_ read, unlike me :).

-- 
Regards/Gruss,
Boris

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2008-11-14 10:43 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-10-31 15:36 [PATCH][RESEND] checkpatch: Add warning for p0-patches Wolfram Sang
2008-11-12 13:55 ` Andy Whitcroft
2008-11-12 14:23   ` Borislav Petkov
2008-11-13 22:55     ` Wolfram Sang
2008-11-14  6:49       ` Borislav Petkov
2008-11-14  9:38         ` Wolfram Sang
2008-11-14 10:43           ` Borislav Petkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).