[v2] checkpatch.pl: seed camelcase from the provided kernel tree root

Submitted by Keller, Jacob E on Aug. 29, 2016, 9 p.m.

Details

Message ID 20160829210047.25111-1-jacob.e.keller@intel.com
State Under Review
Delegated to: Jeff Kirsher
Headers show

Commit Message

Keller, Jacob E Aug. 29, 2016, 9 p.m.
When checkpatch.pl is run without a git tree, it seeds the camelcase
includes from the $root parameter. However, it does not use the $root
directory when seeding for a git tree. Fix this by using "cd $root &&"
so that a user may run checkpatch with the --root parameter pointing to
a valid kernel source tree. In addition, when generating the list of
files to check, we must also prefix each file with the $root parameter,
in order to properly locate the file when searching.

We avoid the use of -C parameter of git because it is not supported on
old versions of git, so we want to avoid breaking checkpatch.pl on those
systems.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
---
 scripts/checkpatch.pl | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Brown, Aaron F Aug. 31, 2016, 8:27 p.m.
> From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org] On
> Behalf Of Jacob Keller
> Sent: Monday, August 29, 2016 2:01 PM
> To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> Subject: [Intel-wired-lan] [PATCH v2] checkpatch.pl: seed camelcase from the
> provided kernel tree root
> 
> When checkpatch.pl is run without a git tree, it seeds the camelcase
> includes from the $root parameter. However, it does not use the $root
> directory when seeding for a git tree. Fix this by using "cd $root &&"
> so that a user may run checkpatch with the --root parameter pointing to
> a valid kernel source tree. In addition, when generating the list of
> files to check, we must also prefix each file with the $root parameter,
> in order to properly locate the file when searching.
> 
> We avoid the use of -C parameter of git because it is not supported on
> old versions of git, so we want to avoid breaking checkpatch.pl on those
> systems.
> 
> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> ---
>  scripts/checkpatch.pl | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

Well, it does throw a few checkpatch warnings...  oh the irony.
---------------------------------------------------------------------
WARNING: A patch subject line should describe the change not the tool that found it
#4:
Subject: [PATCH] checkpatch.pl: seed camelcase from the provided kernel tree

WARNING: line over 80 characters
#35: FILE: scripts/checkpatch.pl:718:
+               my $git_last_include_commit = `cd $root && git log --no-merges --pretty=format:"%h%n" -1 -- include`;

total: 0 errors, 2 warnings, 22 lines checked
---------------------------------------------------------------------
The first is clearly a false warning, thinks checkpatch is the tool that found the error rather than the tool being fixed.  ;)   The second is just a long line in the perl code, which I don't really consider a blocking issue so...

Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Keller, Jacob E Aug. 31, 2016, 10:37 p.m.
> -----Original Message-----
> From: Brown, Aaron F
> Sent: Wednesday, August 31, 2016 1:28 PM
> To: Keller, Jacob E <jacob.e.keller@intel.com>; Intel Wired LAN <intel-
> wired-lan@lists.osuosl.org>
> Subject: RE: [Intel-wired-lan] [PATCH v2] checkpatch.pl: seed camelcase
> from the provided kernel tree root
> 
> > From: Intel-wired-lan [mailto:intel-wired-lan-bounces@lists.osuosl.org]
> On
> > Behalf Of Jacob Keller
> > Sent: Monday, August 29, 2016 2:01 PM
> > To: Intel Wired LAN <intel-wired-lan@lists.osuosl.org>
> > Subject: [Intel-wired-lan] [PATCH v2] checkpatch.pl: seed camelcase from
> the
> > provided kernel tree root
> >
> > When checkpatch.pl is run without a git tree, it seeds the camelcase
> > includes from the $root parameter. However, it does not use the $root
> > directory when seeding for a git tree. Fix this by using "cd $root &&"
> > so that a user may run checkpatch with the --root parameter pointing to
> > a valid kernel source tree. In addition, when generating the list of
> > files to check, we must also prefix each file with the $root parameter,
> > in order to properly locate the file when searching.
> >
> > We avoid the use of -C parameter of git because it is not supported on
> > old versions of git, so we want to avoid breaking checkpatch.pl on those
> > systems.
> >
> > Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
> > ---
> >  scripts/checkpatch.pl | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> Well, it does throw a few checkpatch warnings...  oh the irony.
> ---------------------------------------------------------------------
> WARNING: A patch subject line should describe the change not the tool that
> found it
> #4:
> Subject: [PATCH] checkpatch.pl: seed camelcase from the provided kernel
> tree
> 
> WARNING: line over 80 characters
> #35: FILE: scripts/checkpatch.pl:718:
> +               my $git_last_include_commit = `cd $root && git log --no-merges -
> -pretty=format:"%h%n" -1 -- include`;
> 
> total: 0 errors, 2 warnings, 22 lines checked
> ---------------------------------------------------------------------
> The first is clearly a false warning, thinks checkpatch is the tool that found
> the error rather than the tool being fixed.  ;)   The second is just a long line in
> the perl code, which I don't really consider a blocking issue so...
> 

Ya I saw those. I saw many lines over 80 characters in the file so I just assumed we don't bother checking the tool with itself.

Thanks,
Jake

> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
Brown, Aaron F Sept. 1, 2016, 1:04 a.m.
> From: Keller, Jacob E
> Sent: Wednesday, August 31, 2016 3:37 PM
> To: Brown, Aaron F <aaron.f.brown@intel.com>; Intel Wired LAN <intel-
> wired-lan@lists.osuosl.org>
> Subject: RE: [Intel-wired-lan] [PATCH v2] checkpatch.pl: seed camelcase from
> the provided kernel tree root
> 
<snip>
> > total: 0 errors, 2 warnings, 22 lines checked
> > ---------------------------------------------------------------------
> > The first is clearly a false warning, thinks checkpatch is the tool that found
> > the error rather than the tool being fixed.  ;)   The second is just a long line
> in
> > the perl code, which I don't really consider a blocking issue so...
> >
> 
> Ya I saw those. I saw many lines over 80 characters in the file so I just
> assumed we don't bother checking the tool with itself.

Well, it showed up in as a patch so I ran it through...  But yeah, it does not make sense to enforce rules intended for the kernel against something else.  Especially as the precedent for line length is clearly already in the file ;)
Jeff Kirsher Sept. 1, 2016, 2:33 a.m.
On Thu, 2016-09-01 at 01:04 +0000, Brown, Aaron F wrote:
> > From: Keller, Jacob E
> > Sent: Wednesday, August 31, 2016 3:37 PM
> > To: Brown, Aaron F <aaron.f.brown@intel.com>; Intel Wired LAN <intel-
> > wired-lan@lists.osuosl.org>
> > Subject: RE: [Intel-wired-lan] [PATCH v2] checkpatch.pl: seed camelcase
> from
> > the provided kernel tree root
> > 
> <snip>
> > > total: 0 errors, 2 warnings, 22 lines checked
> > > ---------------------------------------------------------------------
> > > The first is clearly a false warning, thinks checkpatch is the tool
> that found
> > > the error rather than the tool being fixed.  ;)   The second is just
> a long line
> > in
> > > the perl code, which I don't really consider a blocking issue so...
> > >
> > 
> > Ya I saw those. I saw many lines over 80 characters in the file so I
> just
> > assumed we don't bother checking the tool with itself.
> 
> Well, it showed up in as a patch so I ran it through...  But yeah, it
> does not make sense to enforce rules intended for the kernel against
> something else.  Especially as the precedent for line length is clearly
> already in the file

Although it technically is a part of the kernel source since it is in
scripts.  So the question is, should kernel scripts follow the coding
standards?  If so, then yes it should follow its own rules for kernel
source.

Personally I do not think it should be an issue for the contents of
/scripts in the kernel source, but that would be an interesting question to
pose to Joe Perches and the other checkpatch.pl warlords.
Joe Perches Sept. 1, 2016, 2:46 a.m.
On Wed, 2016-08-31 at 19:33 -0700, Jeff Kirsher wrote:
> On Thu, 2016-09-01 at 01:04 +0000, Brown, Aaron F wrote:
> > From: Keller, Jacob E

Hi all.

> > > > ---------------------------------------------------------------------
> > > > The first is clearly a false warning, thinks checkpatch is the tool
> > > > that found the error rather than the tool being fixed.  ;)   The second is just
> > > > a long line in the perl code, which I don't really consider a blocking issue so...
> > > Ya I saw those. I saw many lines over 80 characters in the file so I
> > > just assumed we don't bother checking the tool with itself.
> > Well, it showed up in as a patch so I ran it through...  But yeah, it
> > does not make sense to enforce rules intended for the kernel against
> > something else.  Especially as the precedent for line length is clearly
> > already in the file
> Although it technically is a part of the kernel source since it is in
> scripts.  So the question is, should kernel scripts follow the coding
> standards?  If so, then yes it should follow its own rules for kernel
> source.
> 
> Personally I do not think it should be an issue for the contents of
> /scripts in the kernel source, but that would be an interesting question to
> pose to Joe Perches and the other checkpatch.pl warlords.

Warlord?  Damn.  Where are my spoils of war?

Perl is already basically unintelligible.
80 column perl would be a whole lot worse.

Anyway, I don't look at scripts with checkpatch.

I think it's really only useful for .[ch] files.
Jeff Kirsher Sept. 1, 2016, 3:17 a.m.
On Wed, 2016-08-31 at 19:46 -0700, Joe Perches wrote:
> On Wed, 2016-08-31 at 19:33 -0700, Jeff Kirsher wrote:
> > 
> > On Thu, 2016-09-01 at 01:04 +0000, Brown, Aaron F wrote:
> > > 
> > > From: Keller, Jacob E
> 
> Hi all.
> 
> > 
> > > 
> > > > 
> > > > > 
> > > > > ---------------------------------------------------------------
> > > > > ------
> > > > > The first is clearly a false warning, thinks checkpatch is the
> > > > > tool
> > > > > that found the error rather than the tool being fixed.  ;)   The
> > > > > second is just
> > > > > a long line in the perl code, which I don't really consider a
> > > > > blocking issue so...
> > > > Ya I saw those. I saw many lines over 80 characters in the file so
> > > > I
> > > > just assumed we don't bother checking the tool with itself.
> > > Well, it showed up in as a patch so I ran it through...  But yeah, it
> > > does not make sense to enforce rules intended for the kernel against
> > > something else.  Especially as the precedent for line length is
> > > clearly
> > > already in the file
> > Although it technically is a part of the kernel source since it is in
> > scripts.  So the question is, should kernel scripts follow the coding
> > standards?  If so, then yes it should follow its own rules for kernel
> > source.
> > 
> > Personally I do not think it should be an issue for the contents of
> > /scripts in the kernel source, but that would be an interesting
> > question to
> > pose to Joe Perches and the other checkpatch.pl warlords.
> 
> Warlord?  Damn.  Where are my spoils of war?

I thought you got $0.02 for every checkpatch.pl warning and $0.05 for every
error found? :-)

> Perl is already basically unintelligible.
> 80 column perl would be a whole lot worse.

Agreed.

> Anyway, I don't look at scripts with checkpatch.
> 
> I think it's really only useful for .[ch] files.

So should a modification be made to anything in scripts/* for when
checkpatch.pl is run?  I know that there are *special* rules for
drivers/net/*, so it should be easy to apply whatever checks are pertinent
for scripts/*?
Joe Perches Sept. 1, 2016, 3:26 a.m.
On Wed, 2016-08-31 at 20:17 -0700, Jeff Kirsher wrote:

> I thought you got $0.02 for every checkpatch.pl warning and $0.05 for every
> error found? :-)

Well, it's nice to know some credits are accruing in
some virtual bank.  Unfortunate for me that it's like
the Hotel California Bank though.

> > I don't look at scripts with checkpatch.
> > I think it's really only useful for .[ch] files.
> So should a modification be made to anything in scripts/* for when
> checkpatch.pl is run?  I know that there are *special* rules for
> drivers/net/*, so it should be easy to apply whatever checks are pertinent
> for scripts/*?

I think scripts should rely a little on the skill
and intelligence of the user.

I have a hammer so let's go fishing, etc...

cheers,  Joe

Patch hide | download patch | download mbox

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4de3cc42fc50..ad8d3509f2d7 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -714,8 +714,8 @@  sub seed_camelcase_includes {
 
 	$camelcase_seeded = 1;
 
-	if (-e ".git") {
-		my $git_last_include_commit = `git log --no-merges --pretty=format:"%h%n" -1 -- include`;
+	if (-e "$root/.git") {
+		my $git_last_include_commit = `cd $root && git log --no-merges --pretty=format:"%h%n" -1 -- include`;
 		chomp $git_last_include_commit;
 		$camelcase_cache = ".checkpatch-camelcase.git.$git_last_include_commit";
 	} else {
@@ -742,9 +742,10 @@  sub seed_camelcase_includes {
 		return;
 	}
 
-	if (-e ".git") {
-		$files = `git ls-files "include/*.h"`;
+	if (-e "$root/.git") {
+		$files = `cd $root && git ls-files "include/*.h"`;
 		@include_files = split('\n', $files);
+		@include_files = map("$root/$_", @include_files);
 	}
 
 	foreach my $file (@include_files) {