| Message ID | 20160829210047.25111-1-jacob.e.keller@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Jeff Kirsher |
| Headers | show |
> 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>
> -----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>
> 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 ;)
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.
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.
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/*?
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
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) {
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(-)