linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] scripts: add checkmaintainers.py
@ 2012-12-14 22:19 Cesar Eduardo Barros
  2012-12-17 10:14 ` Michal Marek
  0 siblings, 1 reply; 17+ messages in thread
From: Cesar Eduardo Barros @ 2012-12-14 22:19 UTC (permalink / raw)
  To: linux-kernel
  Cc: Michal Marek, linux-kbuild, Andrew Morton, Cesar Eduardo Barros,
	David Howells, Joe Perches

This small script checks the file patterns in the MAINTAINERS file.

For every file pattern, it checks if the pattern matches any file or
directory in the kernel tree, printing the patterns which do not have a
match.

It also checks for any file pattern pointing to any of the include
directories which does not have a corresponding UAPI file pattern, but
only if the UAPI file pattern would have a match. It also does the same
in the opposite direction.

The script is written in Python; I found its glob function more
well-behaved than Perl's. It should work on both Python 2 and Python 3
without any changes (not even 2to3 is needed); tested on 2.7, 3.2, and
3.3.

I do not think such a short script should have a copyright. But to avoid
any problems, I arbitrarily used the "GNU All-Permissive License" (found
in FSF's GPL-compatible list). If needed, feel free to relicense it as
GPLv2+, 3-clause BSD, or even WTFPLv2 or CC0.

Cc: David Howells <dhowells@redhat.com>
Cc: Joe Perches <joe@perches.com>
Signed-off-by: Cesar Eduardo Barros <cesarb@cesarb.net>
---
 scripts/checkmaintainers.py | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100755 scripts/checkmaintainers.py

diff --git a/scripts/checkmaintainers.py b/scripts/checkmaintainers.py
new file mode 100755
index 0000000..99740e3
--- /dev/null
+++ b/scripts/checkmaintainers.py
@@ -0,0 +1,35 @@
+#!/usr/bin/python
+# Quick check for missing file patterns in the MAINTAINERS file.
+#
+# Copyright (C) 2012 Cesar Eduardo Barros <cesarb@cesarb.net>
+#
+# Copying and distribution of this file, with or without modification,
+# are permitted in any medium without royalty provided the copyright
+# notice and this notice are preserved.  This file is offered as-is,
+# without any warranty.
+
+from __future__ import print_function, unicode_literals, with_statement
+from glob import glob
+
+seen = set()
+
+with open('MAINTAINERS', 'rb') as f:
+    for line in f:
+        line = line.decode('utf-8')
+        if line.startswith('F:'):
+            pattern = line.partition(':')[2].strip()
+            seen.add(pattern)
+
+            if not glob(pattern):
+                print('No match for F: {0}'.format(pattern))
+
+# Check for missing uapi/ pattern
+for pattern in seen:
+    if 'include/' in pattern:
+        if 'include/uapi/' in pattern:
+            other = pattern.replace('include/uapi/', 'include/')
+        else:
+            other = pattern.replace('include/', 'include/uapi/')
+
+        if other not in seen and glob(other):
+            print('Missing {0} for {1}'.format(other, pattern))
-- 
1.7.11.7


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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-14 22:19 [PATCH] scripts: add checkmaintainers.py Cesar Eduardo Barros
@ 2012-12-17 10:14 ` Michal Marek
  2012-12-17 10:27   ` Borislav Petkov
  0 siblings, 1 reply; 17+ messages in thread
From: Michal Marek @ 2012-12-17 10:14 UTC (permalink / raw)
  To: Cesar Eduardo Barros
  Cc: linux-kernel, linux-kbuild, Andrew Morton, David Howells, Joe Perches

On 14.12.2012 23:19, Cesar Eduardo Barros wrote:
> This small script checks the file patterns in the MAINTAINERS file.
> 
> For every file pattern, it checks if the pattern matches any file or
> directory in the kernel tree, printing the patterns which do not have a
> match.

Can't this be added as a warning to scripts/get_maintainer.pl instead?

Thanks,
Michal

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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-17 10:14 ` Michal Marek
@ 2012-12-17 10:27   ` Borislav Petkov
  2012-12-17 15:35     ` Joe Perches
  0 siblings, 1 reply; 17+ messages in thread
From: Borislav Petkov @ 2012-12-17 10:27 UTC (permalink / raw)
  To: Michal Marek
  Cc: Cesar Eduardo Barros, linux-kernel, linux-kbuild, Andrew Morton,
	David Howells, Joe Perches

On Mon, Dec 17, 2012 at 11:14:24AM +0100, Michal Marek wrote:
> On 14.12.2012 23:19, Cesar Eduardo Barros wrote:
> > This small script checks the file patterns in the MAINTAINERS file.
> > 
> > For every file pattern, it checks if the pattern matches any file or
> > directory in the kernel tree, printing the patterns which do not have a
> > match.
> 
> Can't this be added as a warning to scripts/get_maintainer.pl instead?

Yep, it definitely makes more sense than having a standalone script.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-17 10:27   ` Borislav Petkov
@ 2012-12-17 15:35     ` Joe Perches
  2012-12-17 17:00       ` Borislav Petkov
  0 siblings, 1 reply; 17+ messages in thread
From: Joe Perches @ 2012-12-17 15:35 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Michal Marek, Cesar Eduardo Barros, linux-kernel, linux-kbuild,
	Andrew Morton, David Howells

On Mon, 2012-12-17 at 11:27 +0100, Borislav Petkov wrote:
> On Mon, Dec 17, 2012 at 11:14:24AM +0100, Michal Marek wrote:
> > On 14.12.2012 23:19, Cesar Eduardo Barros wrote:
> > > This small script checks the file patterns in the MAINTAINERS file.
> > > 
> > > For every file pattern, it checks if the pattern matches any file or
> > > directory in the kernel tree, printing the patterns which do not have a
> > > match.
> > 
> > Can't this be added as a warning to scripts/get_maintainer.pl instead?
> 
> Yep, it definitely makes more sense than having a standalone script.

I'm not so sure of that myself.

I think maybe it should go into checkpatch.

I do have a similar script as a get_maintainers variant.

Adding a --verify-patterns option seems off-topic to
the generic tool use of get_maintainers.

There's a fair amount of tedium for people to do more
than determine there is a pattern that doesn't match
files in git.

Doing the work that Cesar did to find the commit that
makes a pattern valid/invalid when renames or deletions
occur is the chore that could be automated.

Perhaps Cesar can use his script as a starting point
to find those pattern invalidating commits or maybe
add the capability (or a --strict check) to checkpatch.



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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-17 15:35     ` Joe Perches
@ 2012-12-17 17:00       ` Borislav Petkov
  2012-12-17 18:56         ` Joe Perches
  2012-12-17 21:59         ` Cesar Eduardo Barros
  0 siblings, 2 replies; 17+ messages in thread
From: Borislav Petkov @ 2012-12-17 17:00 UTC (permalink / raw)
  To: Joe Perches
  Cc: Michal Marek, Cesar Eduardo Barros, linux-kernel, linux-kbuild,
	Andrew Morton, David Howells

On Mon, Dec 17, 2012 at 07:35:44AM -0800, Joe Perches wrote:
> Perhaps Cesar can use his script as a starting point to find those
> pattern invalidating commits or maybe add the capability (or a
> --strict check) to checkpatch.

Or that, I don't have a strict preference.

So, yeah, I can see how checkpatch saying: "you've just renamed a
file and thusly invalidated a pattern in MAINTAINERS. Pls, consider
correcting the pattern" could make sense. And I would even add it to
default functionality since the MAINTAINERS patterns are something we
want to always have up-to-date, IMO.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-17 17:00       ` Borislav Petkov
@ 2012-12-17 18:56         ` Joe Perches
  2012-12-17 19:09           ` Joe Perches
  2012-12-17 21:59         ` Cesar Eduardo Barros
  1 sibling, 1 reply; 17+ messages in thread
From: Joe Perches @ 2012-12-17 18:56 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Michal Marek, Cesar Eduardo Barros, linux-kernel, linux-kbuild,
	Andrew Morton, David Howells

On Mon, 2012-12-17 at 18:00 +0100, Borislav Petkov wrote:
> On Mon, Dec 17, 2012 at 07:35:44AM -0800, Joe Perches wrote:
> > Perhaps Cesar can use his script as a starting point to find those
> > pattern invalidating commits or maybe add the capability (or a
> > --strict check) to checkpatch.
> 
> Or that, I don't have a strict preference.
> 
> So, yeah, I can see how checkpatch saying: "you've just renamed a
> file and thusly invalidated a pattern in MAINTAINERS. Pls, consider
> correcting the pattern" could make sense. And I would even add it to
> default functionality since the MAINTAINERS patterns are something we
> want to always have up-to-date, IMO.

Maybe something like this:

 scripts/checkpatch.pl |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 725c596..a7dddc1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1379,6 +1379,7 @@ sub process {
 	# Trace the real file/line as we go.
 	my $realfile = '';
 	my $realline = 0;
+	my $modifiedfile = '';
 	my $realcnt = 0;
 	my $here = '';
 	my $in_comment = 0;
@@ -1536,8 +1537,10 @@ sub process {
 		$here = "#$realline: " if ($file);
 
 		# extract the filename as it passes
-		if ($line =~ /^diff --git.*?(\S+)$/) {
-			$realfile = $1;
+		if ($line =~ /^diff --git\s+(\S+)\s+(\S+)$/) {
+			$modifiedfile = $1;
+			$realfile = $2;
+			$modifiedfile =~ s@^([^/]*)/@@;
 			$realfile =~ s@^([^/]*)/@@;
 			$in_commit_log = 0;
 		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
@@ -1556,6 +1559,19 @@ sub process {
 				ERROR("MODIFIED_INCLUDE_ASM",
 				      "do not modify files in include/asm, change architecture specific files in include/asm-<architecture>\n" . "$here$rawline\n");
 			}
+
+			my $action = "renames $modifiedfile to $realfile";
+			$action = "creates file $realfile" if ($modifiedfile =~ m@dev/null@);
+			$action = "deletes file $modifiedfile" if ($realfile =~ m@dev/null@);
+
+			CHK("MAINTAINERS",
+			    "Patch $action, update MAINTAINERS?\n");
+
+			next;
+		} elsif ($line =~ /^\-\-\-\s+(\S+)/) {
+			$modifiedfile = $1;
+			$modifiedfile =~ s@^([^/]*)/@@;
+			$in_commit_log = 0;
 			next;
 		}
 



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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-17 18:56         ` Joe Perches
@ 2012-12-17 19:09           ` Joe Perches
  2012-12-18 18:28             ` Borislav Petkov
  0 siblings, 1 reply; 17+ messages in thread
From: Joe Perches @ 2012-12-17 19:09 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Michal Marek, Cesar Eduardo Barros, linux-kernel, linux-kbuild,
	Andrew Morton, David Howells

On Mon, 2012-12-17 at 10:56 -0800, Joe Perches wrote:
> On Mon, 2012-12-17 at 18:00 +0100, Borislav Petkov wrote:
> > On Mon, Dec 17, 2012 at 07:35:44AM -0800, Joe Perches wrote:
> > > Perhaps Cesar can use his script as a starting point to find those
> > > pattern invalidating commits or maybe add the capability (or a
> > > --strict check) to checkpatch.

> > So, yeah, I can see how checkpatch saying: "you've just renamed a
> > file and thusly invalidated a pattern in MAINTAINERS. Pls, consider
> > correcting the pattern" could make sense. And I would even add it to
> > default functionality since the MAINTAINERS patterns are something we
> > want to always have up-to-date, IMO.
> 
> Maybe something like this:

A trivial correction:

>  scripts/checkpatch.pl |   20 ++++++++++++++++++--
>  1 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -1556,6 +1559,19 @@ sub process {
>  				ERROR("MODIFIED_INCLUDE_ASM",
>  				      "do not modify files in include/asm, change architecture specific files in include/asm-<architecture>\n" . "$here$rawline\n");
>  			}

This needs a new test here to avoid chirping
on files that aren't added, deleted or renamed.

		next if ($realfile eq $modifiedfile);

> +
> +			my $action = "renames $modifiedfile to $realfile";
> +			$action = "creates file $realfile" if ($modifiedfile =~ m@dev/null@);
> +			$action = "deletes file $modifiedfile" if ($realfile =~ m@dev/null@);
> +
> +			CHK("MAINTAINERS",
> +			    "Patch $action, update MAINTAINERS?\n");



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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-17 17:00       ` Borislav Petkov
  2012-12-17 18:56         ` Joe Perches
@ 2012-12-17 21:59         ` Cesar Eduardo Barros
  1 sibling, 0 replies; 17+ messages in thread
From: Cesar Eduardo Barros @ 2012-12-17 21:59 UTC (permalink / raw)
  To: Borislav Petkov, Joe Perches, Michal Marek, linux-kernel,
	linux-kbuild, Andrew Morton, David Howells

Em 17-12-2012 15:00, Borislav Petkov escreveu:
> On Mon, Dec 17, 2012 at 07:35:44AM -0800, Joe Perches wrote:
>> Perhaps Cesar can use his script as a starting point to find those
>> pattern invalidating commits or maybe add the capability (or a
>> --strict check) to checkpatch.
>
> Or that, I don't have a strict preference.
>
> So, yeah, I can see how checkpatch saying: "you've just renamed a
> file and thusly invalidated a pattern in MAINTAINERS. Pls, consider
> correcting the pattern" could make sense. And I would even add it to
> default functionality since the MAINTAINERS patterns are something we
> want to always have up-to-date, IMO.

Good idea, but a standalone MAINTAINERS checker is still useful, to 
check for things checkpatch did not catch, either because it was not 
run, or because it did not have enough information.

For instance, consider the case of a patch removing the last file in a 
directory going in via one branch, while a patch adding a file pattern 
for that same directory goes in via a separate branch. There is no way 
checkpatch can catch that.

As for finding the pattern invalidating commits, it is usually not that 
hard, a simple "git log -- <pattern>" or similar usually does the trick, 
except in the cases where the pattern addition itself was wrong. The 
hard part can be interpreting it and the surrounding commits to find out 
the committer's intention, how it should affect the MAINTAINERS entry, 
and who should get a Cc: of the fix.

-- 
Cesar Eduardo Barros
cesarb@cesarb.net
cesar.barros@gmail.com

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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-17 19:09           ` Joe Perches
@ 2012-12-18 18:28             ` Borislav Petkov
  2012-12-18 19:34               ` Joe Perches
  0 siblings, 1 reply; 17+ messages in thread
From: Borislav Petkov @ 2012-12-18 18:28 UTC (permalink / raw)
  To: Joe Perches
  Cc: Michal Marek, Cesar Eduardo Barros, linux-kernel, linux-kbuild,
	Andrew Morton, David Howells

On Mon, Dec 17, 2012 at 11:09:43AM -0800, Joe Perches wrote:
> This needs a new test here to avoid chirping
> on files that aren't added, deleted or renamed.
> 
> 		next if ($realfile eq $modifiedfile);

Hmm, I don't think that catches file renames when using the normal 'git
diff' output:

$ git mv README README.old

And this detection should work in the default case with a pre-commit
hook. For example, I have this in .git/hooks/pre-commit:

if git rev-parse --verify HEAD >/dev/null 2>&1
then
        against=HEAD
else
        # Initial commit: diff against an empty tree object
        against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
fi

git diff --cached $against -- | ./scripts/checkpatch.pl --strict --no-signoff -

and when I do this, I get the changes checked:

$ git commit -a
README.old:5: trailing whitespace.
+kernel, and what to do if something goes wrong. 
README.old:19: trailing whitespace.
+  accompanying COPYING file for more details. 
README.old:47: trailing whitespace.
+   these typically contain kernel-specific installation notes for some 
README.old:287: trailing whitespace.
+ - Keep a backup kernel handy in case something goes wrong.  This is 
README.old:301: trailing whitespace.
+   to the place where your regular bootable kernel is found. 
README.old:314: trailing whitespace.
+   Reinstalling LILO is usually a matter of running /sbin/lilo. 
README.old:317: trailing whitespace.
+   work.  See the LILO docs for more information. 
README.old:325: trailing whitespace.
+   recompile the kernel to change these parameters. 
README.old:327: trailing whitespace.
+ - Reboot with the new kernel and enjoy. 
README.old:394: trailing whitespace.
+   interesting one. 
README.old:412: new blank line at EOF.

but not the MAINTAINERS check.

git diff --cached HEAD gives:

diff --git a/README b/README.old
similarity index 100%
rename from README
rename to README.old

so I'd say we're almost there.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-18 18:28             ` Borislav Petkov
@ 2012-12-18 19:34               ` Joe Perches
  2012-12-18 20:31                 ` Borislav Petkov
  0 siblings, 1 reply; 17+ messages in thread
From: Joe Perches @ 2012-12-18 19:34 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Michal Marek, Cesar Eduardo Barros, linux-kernel, linux-kbuild,
	Andrew Morton, David Howells

On Tue, 2012-12-18 at 19:28 +0100, Borislav Petkov wrote:
> On Mon, Dec 17, 2012 at 11:09:43AM -0800, Joe Perches wrote:
> > This needs a new test here to avoid chirping
> > on files that aren't added, deleted or renamed.
> > 
> > 		next if ($realfile eq $modifiedfile);
> 
> Hmm, I don't think that catches file renames when using the normal 'git
> diff' output:

That depends on whether or not an actual patch follows.

This looks for the +++ and --- lines
in a patch.

If no patch is attached, you should get

ERROR: Does not appear to be a unified-diff format patch



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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-18 19:34               ` Joe Perches
@ 2012-12-18 20:31                 ` Borislav Petkov
  2012-12-18 20:36                   ` Joe Perches
  0 siblings, 1 reply; 17+ messages in thread
From: Borislav Petkov @ 2012-12-18 20:31 UTC (permalink / raw)
  To: Joe Perches
  Cc: Michal Marek, Cesar Eduardo Barros, linux-kernel, linux-kbuild,
	Andrew Morton, David Howells

On Tue, Dec 18, 2012 at 11:34:41AM -0800, Joe Perches wrote:
> If no patch is attached, you should get
>
> ERROR: Does not appear to be a unified-diff format patch

Well, it needs to handle the case where a patch simply and only renames
a file.

Then, even if a patch follows:

diff --git a/README b/README.old
similarity index 99%
rename from README
rename to README.old
index a24ec89ba442..b0a7aa56c3cc 100644
--- a/README
+++ b/README.old
@@ -1,3 +1,6 @@
+TEST
+
+
         Linux kernel release 3.x <http://kernel.org/>
 
 These are the release notes for Linux version 3.  Read them carefully,
--

it says:

$ git diff --cached HEAD -- | ./scripts/checkpatch.pl --strict -
Must be run from the top-level dir. of a kernel tree

If I comment out the following hunk:

        if (!defined $root) { 
                print "Must be run from the top-level dir. of a kernel tree\n";
                exit(2);
        }

then it works, albeit with a bunch of warnings:

Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1558.
Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1558.
Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1304.
Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1304.
Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1304.
Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1304.
Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1304.
Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1304.
CHECK: Patch renames README to README.old, update MAINTAINERS?
...

Looks like this would need a bit more work.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-18 20:31                 ` Borislav Petkov
@ 2012-12-18 20:36                   ` Joe Perches
  2012-12-18 20:47                     ` Borislav Petkov
  0 siblings, 1 reply; 17+ messages in thread
From: Joe Perches @ 2012-12-18 20:36 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Michal Marek, Cesar Eduardo Barros, linux-kernel, linux-kbuild,
	Andrew Morton, David Howells

On Tue, 2012-12-18 at 21:31 +0100, Borislav Petkov wrote:
> On Tue, Dec 18, 2012 at 11:34:41AM -0800, Joe Perches wrote:
> > If no patch is attached, you should get
> >
> > ERROR: Does not appear to be a unified-diff format patch
> 
> Well, it needs to handle the case where a patch simply and only renames
> a file.
> 
> Then, even if a patch follows:
> 
> diff --git a/README b/README.old
> similarity index 99%
> rename from README
> rename to README.old
> index a24ec89ba442..b0a7aa56c3cc 100644
> --- a/README
> +++ b/README.old
> @@ -1,3 +1,6 @@
> +TEST
> +
> +
>          Linux kernel release 3.x <http://kernel.org/>
>  
>  These are the release notes for Linux version 3.  Read them carefully,
> --
> 
> it says:
> 
> $ git diff --cached HEAD -- | ./scripts/checkpatch.pl --strict -
> Must be run from the top-level dir. of a kernel tree
> 
> If I comment out the following hunk:
> 
>         if (!defined $root) { 
>                 print "Must be run from the top-level dir. of a kernel tree\n";
>                 exit(2);
>         }
> 
> then it works, albeit with a bunch of warnings:
> 
> Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1558.
> Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1558.
> Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1304.
> Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1304.
> Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1304.
> Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1304.
> Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1304.
> Use of uninitialized value $root in concatenation (.) or string at ./scripts/checkpatch.pl line 1304.
> CHECK: Patch renames README to README.old, update MAINTAINERS?

You renamed README which is one of the filenames used
when checkpatch verifies the top-level dir kernel tree.

Don't do that, README is a required filename.

Try another file in drivers or somewhere other than root.



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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-18 20:36                   ` Joe Perches
@ 2012-12-18 20:47                     ` Borislav Petkov
  2012-12-18 21:33                       ` Joe Perches
  0 siblings, 1 reply; 17+ messages in thread
From: Borislav Petkov @ 2012-12-18 20:47 UTC (permalink / raw)
  To: Joe Perches
  Cc: Michal Marek, Cesar Eduardo Barros, linux-kernel, linux-kbuild,
	Andrew Morton, David Howells

On Tue, Dec 18, 2012 at 12:36:37PM -0800, Joe Perches wrote:
> You renamed README which is one of the filenames used
> when checkpatch verifies the top-level dir kernel tree.
> 
> Don't do that, README is a required filename.

$ git mv scripts/checkpatch{,-2}.pl
$ git diff --cached HEAD -- | ./scripts/checkpatch-2.pl --strict -
ERROR: Does not appear to be a unified-diff format patch

total: 1 errors, 0 warnings, 0 checks, 0 lines checked

Your patch has style problems, please review.

If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.

 [ adjust .git/hooks/pre-commit to use the new ./scripts/checkpatch-2.pl script
   name]

$ git commit -a
scripts/checkpatch-2.pl:2685: space before tab in indent.
+                                               $ok = 1;
scripts/checkpatch-2.pl:2691: space before tab in indent.
+                                               $ok = 1;

No joy :(

Let's try another file:

$ git mv arch/x86/Kconfig{,.old}
$ git diff --cached HEAD -- | ./scripts/checkpatch.pl --strict -
ERROR: Does not appear to be a unified-diff format patch

total: 1 errors, 0 warnings, 0 checks, 0 lines checked

Your patch has style problems, please review.

If any of these errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.

$ git commit -a
arch/x86/Kconfig.old:451: trailing whitespace.
+         Internet Device(MID) platform.

Oh well, enough games for today.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-18 20:47                     ` Borislav Petkov
@ 2012-12-18 21:33                       ` Joe Perches
  2012-12-19 15:07                         ` Borislav Petkov
  0 siblings, 1 reply; 17+ messages in thread
From: Joe Perches @ 2012-12-18 21:33 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Michal Marek, Cesar Eduardo Barros, linux-kernel, linux-kbuild,
	Andrew Morton, David Howells

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

On Tue, 2012-12-18 at 21:47 +0100, Borislav Petkov wrote:
> Oh well, enough games for today.

Maybe try this tomorrow?

[-- Attachment #2: Type: text/x-patch, Size: 2179 bytes --]

 scripts/checkpatch.pl |   40 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 725c596..adc8de1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1317,6 +1317,19 @@ sub check_absolute_file {
 	}
 }
 
+sub check_filenames_changed {
+    my ($oldfile, $newfile, $herecurr) = @_;
+
+    return if ($oldfile eq $newfile);
+
+    my $action = "renames $oldfile to $newfile";
+    $action = "creates file $newfile" if ($oldfile =~ m@dev/null@);
+    $action = "deletes file $oldfile" if ($newfile =~ m@dev/null@);
+
+    CHK("MAINTAINERS",
+	"Patch $action, update MAINTAINERS?\n" . $herecurr);
+}
+
 sub pos_last_openparen {
 	my ($line) = @_;
 
@@ -1379,6 +1392,9 @@ sub process {
 	# Trace the real file/line as we go.
 	my $realfile = '';
 	my $realline = 0;
+	my $modifiedfile = '';
+	my $gitrealfile = '';
+	my $gitmodifiedfile = '';
 	my $realcnt = 0;
 	my $here = '';
 	my $in_comment = 0;
@@ -1536,10 +1552,17 @@ sub process {
 		$here = "#$realline: " if ($file);
 
 		# extract the filename as it passes
-		if ($line =~ /^diff --git.*?(\S+)$/) {
-			$realfile = $1;
+		if ($line =~ /^diff --git\s+(\S+)\s+(\S+)$/) {
+			$modifiedfile = $1;
+			$realfile = $2;
+			$modifiedfile =~ s@^([^/]*)/@@;
 			$realfile =~ s@^([^/]*)/@@;
+
+			$gitmodifiedfile = $modifiedfile;
+			$gitrealfile = $realfile;
 			$in_commit_log = 0;
+			check_filenames_changed($gitmodifiedfile, $gitrealfile);
+
 		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
 			$realfile = $1;
 			$realfile =~ s@^([^/]*)/@@;
@@ -1556,6 +1579,19 @@ sub process {
 				ERROR("MODIFIED_INCLUDE_ASM",
 				      "do not modify files in include/asm, change architecture specific files in include/asm-<architecture>\n" . "$here$rawline\n");
 			}
+
+			if ($modifiedfile ne $gitmodifiedfile ||
+			    $realfile ne $gitrealfile) {
+				check_filenames_changed($modifiedfile, $realfile);
+				$gitmodifiedfile = '';
+				$gitrealfile = '';
+			}
+
+			next;
+		} elsif ($line =~ /^\-\-\-\s+(\S+)/) {
+			$modifiedfile = $1;
+			$modifiedfile =~ s@^([^/]*)/@@;
+			$in_commit_log = 0;
 			next;
 		}
 

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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-18 21:33                       ` Joe Perches
@ 2012-12-19 15:07                         ` Borislav Petkov
  2012-12-19 17:43                           ` Joe Perches
  0 siblings, 1 reply; 17+ messages in thread
From: Borislav Petkov @ 2012-12-19 15:07 UTC (permalink / raw)
  To: Joe Perches
  Cc: Michal Marek, Cesar Eduardo Barros, linux-kernel, linux-kbuild,
	Andrew Morton, David Howells

On Tue, Dec 18, 2012 at 01:33:19PM -0800, Joe Perches wrote:
> On Tue, 2012-12-18 at 21:47 +0100, Borislav Petkov wrote:
> > Oh well, enough games for today.
> 
> Maybe try this tomorrow?

$ git diff --cached HEAD -- | ./scripts/checkpatch.pl --strict -
Use of uninitialized value $herecurr in concatenation (.) or string at ./scripts/checkpatch.pl line 1333.
CHECK: Patch renames arch/x86/Kconfig to arch/x86/Kconfig.old, update MAINTAINERS?

yes, except the warning.

$ git commit -a
arch/x86/Kconfig.old:451: trailing whitespace.
+         Internet Device(MID) platform.

This doesn't seem to work although I have the same pre-commit hook as
the above line: " git diff --cached HEAD -- | ./scripts/checkpatch.pl --strict -"

$ git diff --cached HEAD
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig.old
similarity index 100%
rename from arch/x86/Kconfig
rename to arch/x86/Kconfig.old

and this is probably the reason - you said checkpatch needs an unified
diff after the initial lines.

So this is getting closer except checkpatch needs to still warn in the
case where there's no unified diff following *but* a single file rename.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-19 15:07                         ` Borislav Petkov
@ 2012-12-19 17:43                           ` Joe Perches
  2012-12-20 19:31                             ` Borislav Petkov
  0 siblings, 1 reply; 17+ messages in thread
From: Joe Perches @ 2012-12-19 17:43 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Michal Marek, Cesar Eduardo Barros, linux-kernel, linux-kbuild,
	Andrew Morton, David Howells

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

On Wed, 2012-12-19 at 16:07 +0100, Borislav Petkov wrote:
> On Tue, Dec 18, 2012 at 01:33:19PM -0800, Joe Perches wrote:
> > On Tue, 2012-12-18 at 21:47 +0100, Borislav Petkov wrote:
> > > Oh well, enough games for today.
> > 
> > Maybe try this tomorrow?
> 
> $ git diff --cached HEAD -- | ./scripts/checkpatch.pl --strict -
> Use of uninitialized value $herecurr in concatenation (.) or string at ./scripts/checkpatch.pl line 1333.
> CHECK: Patch renames arch/x86/Kconfig to arch/x86/Kconfig.old, update MAINTAINERS?
> 
> yes, except the warning.
> 
> $ git commit -a
> arch/x86/Kconfig.old:451: trailing whitespace.
> +         Internet Device(MID) platform.

That's the output from a:
$ git diff-index --check --cached HEAD --

> This doesn't seem to work although I have the same pre-commit hook as
> the above line: " git diff --cached HEAD -- | ./scripts/checkpatch.pl --strict -"

perhaps you need an exec prefix

When I use this file
$ cat .git/hooks/pre-commit
exec  git diff --cached HEAD -- | ./scripts/checkpatch.pl --strict -
$

it works fine.

Here's the latest checkpatch diff without that
concatenation warning for you to try.



[-- Attachment #2: cp_boris.diff --]
[-- Type: text/x-patch, Size: 2156 bytes --]

 scripts/checkpatch.pl |   40 ++++++++++++++++++++++++++++++++++++++--
 1 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1d6e4c5..fe5750e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1321,6 +1321,19 @@ sub check_absolute_file {
 	}
 }
 
+sub check_filenames_changed {
+    my ($oldfile, $newfile) = @_;
+
+    return if ($oldfile eq $newfile);
+
+    my $action = "renames $oldfile to $newfile";
+    $action = "creates file $newfile" if ($oldfile =~ m@dev/null@);
+    $action = "deletes file $oldfile" if ($newfile =~ m@dev/null@);
+
+    CHK("MAINTAINERS",
+	"Patch $action, update MAINTAINERS?\n");
+}
+
 sub pos_last_openparen {
 	my ($line) = @_;
 
@@ -1383,6 +1396,9 @@ sub process {
 	# Trace the real file/line as we go.
 	my $realfile = '';
 	my $realline = 0;
+	my $modifiedfile = '';
+	my $gitrealfile = '';
+	my $gitmodifiedfile = '';
 	my $realcnt = 0;
 	my $here = '';
 	my $in_comment = 0;
@@ -1542,10 +1558,17 @@ sub process {
 		$here = "#$realline: " if ($file);
 
 		# extract the filename as it passes
-		if ($line =~ /^diff --git.*?(\S+)$/) {
-			$realfile = $1;
+		if ($line =~ /^diff --git\s+(\S+)\s+(\S+)$/) {
+			$modifiedfile = $1;
+			$realfile = $2;
+			$modifiedfile =~ s@^([^/]*)/@@;
 			$realfile =~ s@^([^/]*)/@@;
+
+			$gitmodifiedfile = $modifiedfile;
+			$gitrealfile = $realfile;
 			$in_commit_log = 0;
+			check_filenames_changed($gitmodifiedfile, $gitrealfile);
+
 		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
 			$realfile = $1;
 			$realfile =~ s@^([^/]*)/@@;
@@ -1562,6 +1585,19 @@ sub process {
 				ERROR("MODIFIED_INCLUDE_ASM",
 				      "do not modify files in include/asm, change architecture specific files in include/asm-<architecture>\n" . "$here$rawline\n");
 			}
+
+			if ($modifiedfile ne $gitmodifiedfile ||
+			    $realfile ne $gitrealfile) {
+				check_filenames_changed($modifiedfile, $realfile);
+				$gitmodifiedfile = '';
+				$gitrealfile = '';
+			}
+
+			next;
+		} elsif ($line =~ /^\-\-\-\s+(\S+)/) {
+			$modifiedfile = $1;
+			$modifiedfile =~ s@^([^/]*)/@@;
+			$in_commit_log = 0;
 			next;
 		}
 

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

* Re: [PATCH] scripts: add checkmaintainers.py
  2012-12-19 17:43                           ` Joe Perches
@ 2012-12-20 19:31                             ` Borislav Petkov
  0 siblings, 0 replies; 17+ messages in thread
From: Borislav Petkov @ 2012-12-20 19:31 UTC (permalink / raw)
  To: Joe Perches
  Cc: Michal Marek, Cesar Eduardo Barros, linux-kernel, linux-kbuild,
	Andrew Morton, David Howells

On Wed, Dec 19, 2012 at 09:43:42AM -0800, Joe Perches wrote:
> When I use this file
> $ cat .git/hooks/pre-commit
> exec  git diff --cached HEAD -- | ./scripts/checkpatch.pl --strict -
> $
> 
> it works fine.
> ^
> Here's the latest checkpatch diff without that
> concatenation warning for you to try.

Yep, looks good. Add a proper commit message and ship it.

Tested-by: Borislav Petkov <bp@alien8.de>

Btw, you should credit Cesar in the commit message for the idea.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

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

end of thread, other threads:[~2012-12-20 19:31 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-12-14 22:19 [PATCH] scripts: add checkmaintainers.py Cesar Eduardo Barros
2012-12-17 10:14 ` Michal Marek
2012-12-17 10:27   ` Borislav Petkov
2012-12-17 15:35     ` Joe Perches
2012-12-17 17:00       ` Borislav Petkov
2012-12-17 18:56         ` Joe Perches
2012-12-17 19:09           ` Joe Perches
2012-12-18 18:28             ` Borislav Petkov
2012-12-18 19:34               ` Joe Perches
2012-12-18 20:31                 ` Borislav Petkov
2012-12-18 20:36                   ` Joe Perches
2012-12-18 20:47                     ` Borislav Petkov
2012-12-18 21:33                       ` Joe Perches
2012-12-19 15:07                         ` Borislav Petkov
2012-12-19 17:43                           ` Joe Perches
2012-12-20 19:31                             ` Borislav Petkov
2012-12-17 21:59         ` Cesar Eduardo Barros

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).