[PATCH] checkpatch: use $root/.git when seeding camel case

[PATCH] checkpatch: use $root/.git when seeding camel case

[Jacob Keller]

When seeding the camel case file, checkpatch.pl uses the $root directory
in order to find the acceptable list of allowed camel case words.

However, if the current directory is a git repository, checkpatch.pl
attempts to seed using the local git directory.

This is problematic if checkpatch.pl is passed a --root and is being run
from within another git repository. Rather than seeding from the
provided root tree, checkpatch.pl will seed using the local files. If
the current git repository isn't a kernel tree, this can lead to
unexpected warnings about camel case issues.

Always honor the $root parameter when seeding camelcase files by using
"$root/.git" and changing directory to the $root before invoking git.

Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
Cc: Joe Perches <joe@perches.com>
---
This is a resend of an old patch that appears to have never been picked up.
It was originally reviewed at the following locations a few years ago:

https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20160505231108.1934-1-jacob.e.keller@intel.com/
https://lore.kernel.org/netdev/20190315010334.5707-1-jacob.e.keller@intel.com/

The motivation is that we use checkpatch.pl on code in a separate git
repository that will eventually be submitted upstream. As part of this, we
provide the --root argument to specify the target kernel tree. The camelcase
then gets seeded incorrectly and --strict begins warning about a lot of
cases that aren't errors.

The current workaround is that we cd to the kernel tree and run checkpatch
from there.. but this breaks if we want to use a .checkpatch.conf file as it
won't get picked up from the original directory.

This patch simply fixes the camelcase seeding to honor the git tree at the
$root if one is provided.

I opted to stick with "cd $root &&" instead of "git -C", since there are no other usages
of "git -C" in the checkpatch.pl currently, despite "git -C" being from git
1.8.5,  and being ~7 years old.

 scripts/checkpatch.pl | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

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

Re: [PATCH] checkpatch: use $root/.git when seeding camel case

[Joe Perches]

On Mon, 2020-06-22 at 14:58 -0700, Jacob Keller wrote:
> When seeding the camel case file, checkpatch.pl uses the $root directory
> in order to find the acceptable list of allowed camel case words.
> 
> However, if the current directory is a git repository, checkpatch.pl
> attempts to seed using the local git directory.
> 
> This is problematic if checkpatch.pl is passed a --root and is being run
> from within another git repository. Rather than seeding from the
> provided root tree, checkpatch.pl will seed using the local files. If
> the current git repository isn't a kernel tree, this can lead to
> unexpected warnings about camel case issues.
> 
> Always honor the $root parameter when seeding camelcase files by using
> "$root/.git" and changing directory to the $root before invoking git.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -919,8 +919,8 @@ sub seed_camelcase_includes {
>  
>  	$camelcase_seeded = 1;
>  
> -	if (-e ".git") {
> -		my $git_last_include_commit = `${git_command} log --no-merges --pretty=format:"%h%n" -1 -- include`;
> +	if (-e "$root/.git") {
> +		my $git_last_include_commit = `cd $root && ${git_command} log --no-merges --pretty=format:"%h%n" -1 -- include`;
>  		chomp $git_last_include_commit;
>  		$camelcase_cache = ".checkpatch-camelcase.git.$git_last_include_commit";
>  	} else {
> @@ -947,9 +947,10 @@ sub seed_camelcase_includes {
>  		return;
>  	}
>  
> -	if (-e ".git") {
> -		$files = `${git_command} ls-files "include/*.h"`;
> +	if (-e "$root/.git") {
> +		$files = `cd $root && ${git_command} ls-files "include/*.h"`;
>  		@include_files = split('\n', $files);
> +		@include_files = map("$root/$_", @include_files);
>  	}
>  
>  	foreach my $file (@include_files) {

checkpatch has 4 uses of ${git_command}

Maybe git_command should be changed instead.

I wonder how this interacts with the GIT_DIR environment variable.

Re: [PATCH] checkpatch: use $root/.git when seeding camel case

[Jacob Keller]

On 6/22/2020 3:11 PM, Joe Perches wrote:
> On Mon, 2020-06-22 at 14:58 -0700, Jacob Keller wrote:
>> When seeding the camel case file, checkpatch.pl uses the $root directory
>> in order to find the acceptable list of allowed camel case words.
>>
>> However, if the current directory is a git repository, checkpatch.pl
>> attempts to seed using the local git directory.
>>
>> This is problematic if checkpatch.pl is passed a --root and is being run
>> from within another git repository. Rather than seeding from the
>> provided root tree, checkpatch.pl will seed using the local files. If
>> the current git repository isn't a kernel tree, this can lead to
>> unexpected warnings about camel case issues.
>>
>> Always honor the $root parameter when seeding camelcase files by using
>> "$root/.git" and changing directory to the $root before invoking git.
> []
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -919,8 +919,8 @@ sub seed_camelcase_includes {
>>  
>>  	$camelcase_seeded = 1;
>>  
>> -	if (-e ".git") {
>> -		my $git_last_include_commit = `${git_command} log --no-merges --pretty=format:"%h%n" -1 -- include`;
>> +	if (-e "$root/.git") {
>> +		my $git_last_include_commit = `cd $root && ${git_command} log --no-merges --pretty=format:"%h%n" -1 -- include`;
>>  		chomp $git_last_include_commit;
>>  		$camelcase_cache = ".checkpatch-camelcase.git.$git_last_include_commit";
>>  	} else {
>> @@ -947,9 +947,10 @@ sub seed_camelcase_includes {
>>  		return;
>>  	}
>>  
>> -	if (-e ".git") {
>> -		$files = `${git_command} ls-files "include/*.h"`;
>> +	if (-e "$root/.git") {
>> +		$files = `cd $root && ${git_command} ls-files "include/*.h"`;
>>  		@include_files = split('\n', $files);
>> +		@include_files = map("$root/$_", @include_files);
>>  	}
>>  
>>  	foreach my $file (@include_files) {
> 
> checkpatch has 4 uses of ${git_command}
> 
> Maybe git_command should be changed instead.
>
So, in part, I'm not 100% sure. In our use case, it depends on what git
is being used for.

Something like "checkpatch.pl -g HEAD", we would like to target the
local repository. Where as camelcase, we'd like to target the --root
repository.

What if, instead of this, we opt to use the local .git for camelcase
only when $root is non-empty? That way, if --root is applied, camcel
case seeding always uses --root, rather than the local tree.

Something like:

if (-e ".git" && $root eq "") for this block, so then we fall through to
the non-git style instead?

> I wonder how this interacts with the GIT_DIR environment variable.
> 

Probably poorly. I suspect it would end up looking at GIT_DIR instead of
$root/.git, despite the cd.

Thanks,
Jake