LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] checkpatch: Allow not using -f with files that are in git
@ 2020-08-25  0:09 Joe Perches
  2020-08-25 12:23 ` Rasmus Villemoes
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Joe Perches @ 2020-08-25  0:09 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Andy Whitcroft, LKML

If a file exists in git and checkpatch is used without the -f
flag for scanning a file, then checkpatch will scan the file
assuming it's a patch and emit:

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

Change the behavior to assume the -f flag if the file exists
in git.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 79fc357b18cd..cdee7cfadc11 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -976,6 +976,16 @@ sub seed_camelcase_includes {
 	}
 }
 
+sub git_is_single_file {
+	my ($filename) = @_;
+
+	return 0 if ((which("git") eq "") || !(-e "$gitroot"));
+
+	my $output = `${git_command} ls-files -- $filename`;
+	my $count = $output =~ tr/\n//;
+	return $count eq 1 && $output =~ m{^${filename}$};
+}
+
 sub git_commit_info {
 	my ($commit, $id, $desc) = @_;
 
@@ -1049,6 +1059,9 @@ my $vname;
 $allow_c99_comments = !defined $ignore_type{"C99_COMMENT_TOLERANCE"};
 for my $filename (@ARGV) {
 	my $FILE;
+	my $is_git_file = git_is_single_file($filename);
+	my $oldfile = $file;
+	$file = 1 if ($is_git_file);
 	if ($git) {
 		open($FILE, '-|', "git format-patch -M --stdout -1 $filename") ||
 			die "$P: $filename: git format-patch failed - $!\n";
@@ -1093,6 +1106,7 @@ for my $filename (@ARGV) {
 	@modifierListFile = ();
 	@typeListFile = ();
 	build_types();
+	$file = $oldfile if ($is_git_file);
 }
 
 if (!$quiet) {



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

* Re: [PATCH] checkpatch: Allow not using -f with files that are in git
  2020-08-25  0:09 [PATCH] checkpatch: Allow not using -f with files that are in git Joe Perches
@ 2020-08-25 12:23 ` Rasmus Villemoes
  2020-08-28  8:03   ` Joe Perches
  2020-08-28 18:17   ` Joe Perches
  2020-09-13  4:02 ` Joe Perches
  2020-10-18 14:03 ` Geert Uytterhoeven
  2 siblings, 2 replies; 9+ messages in thread
From: Rasmus Villemoes @ 2020-08-25 12:23 UTC (permalink / raw)
  To: Joe Perches, Andrew Morton; +Cc: Andy Whitcroft, LKML

On 25/08/2020 02.09, Joe Perches wrote:
> If a file exists in git and checkpatch is used without the -f
> flag for scanning a file, then checkpatch will scan the file
> assuming it's a patch and emit:
> 
> ERROR: Does not appear to be a unified-diff format patch
> 
> Change the behavior to assume the -f flag if the file exists
> in git.

Heh, I read the patch subject to mean you introduced a way for subsystem
maintainers to prevent running checkpatch -f on their files, which I
think some would like ;)

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 79fc357b18cd..cdee7cfadc11 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -976,6 +976,16 @@ sub seed_camelcase_includes {
>  	}
>  }
>  
> +sub git_is_single_file {
> +	my ($filename) = @_;
> +
> +	return 0 if ((which("git") eq "") || !(-e "$gitroot"));
> +
> +	my $output = `${git_command} ls-files -- $filename`;
> +	my $count = $output =~ tr/\n//;
> +	return $count eq 1 && $output =~ m{^${filename}$};
> +}

Isn't that somewhat expensive to do for each file? Why not postpone that
check till we're about to complain that the file is not a diff (haven't
looked at how such a refactoring would look).

Rasmus

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

* Re: [PATCH] checkpatch: Allow not using -f with files that are in git
  2020-08-25 12:23 ` Rasmus Villemoes
@ 2020-08-28  8:03   ` Joe Perches
  2020-08-28 18:17   ` Joe Perches
  1 sibling, 0 replies; 9+ messages in thread
From: Joe Perches @ 2020-08-28  8:03 UTC (permalink / raw)
  To: Rasmus Villemoes, Andrew Morton; +Cc: Andy Whitcroft, LKML

On Tue, 2020-08-25 at 14:23 +0200, Rasmus Villemoes wrote:
> On 25/08/2020 02.09, Joe Perches wrote:
> > If a file exists in git and checkpatch is used without the -f
> > flag for scanning a file, then checkpatch will scan the file
> > assuming it's a patch and emit:
> > 
> > ERROR: Does not appear to be a unified-diff format patch
> > 
> > Change the behavior to assume the -f flag if the file exists
> > in git.
> 
> Heh, I read the patch subject to mean you introduced a way for subsystem
> maintainers to prevent running checkpatch -f on their files, which I
> think some would like ;)
> 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 79fc357b18cd..cdee7cfadc11 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -976,6 +976,16 @@ sub seed_camelcase_includes {
> >  	}
> >  }
> >  
> > +sub git_is_single_file {
> > +	my ($filename) = @_;
> > +
> > +	return 0 if ((which("git") eq "") || !(-e "$gitroot"));
> > +
> > +	my $output = `${git_command} ls-files -- $filename`;
> > +	my $count = $output =~ tr/\n//;
> > +	return $count eq 1 && $output =~ m{^${filename}$};
> > +}
> 
> Isn't that somewhat expensive to do for each file? Why not postpone that
> check till we're about to complain that the file is not a diff (haven't
> looked at how such a refactoring would look).

It's necessary because you need the --file option set _before_
analyzing the file content.

Oddly, I didn't receive this email directly so I couldn't reply
to it earlier.


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

* Re: [PATCH] checkpatch: Allow not using -f with files that are in git
  2020-08-25 12:23 ` Rasmus Villemoes
  2020-08-28  8:03   ` Joe Perches
@ 2020-08-28 18:17   ` Joe Perches
  1 sibling, 0 replies; 9+ messages in thread
From: Joe Perches @ 2020-08-28 18:17 UTC (permalink / raw)
  To: Rasmus Villemoes, Andrew Morton; +Cc: Andy Whitcroft, LKML

On Tue, 2020-08-25 at 14:23 +0200, Rasmus Villemoes wrote:
> On 25/08/2020 02.09, Joe Perches wrote:
> > If a file exists in git and checkpatch is used without the -f
> > flag for scanning a file, then checkpatch will scan the file
> > assuming it's a patch

[]

> > +sub git_is_single_file {
> > +	my ($filename) = @_;
> > +
> > +	return 0 if ((which("git") eq "") || !(-e "$gitroot"));
> > +
> > +	my $output = `${git_command} ls-files -- $filename`;
> > +	my $count = $output =~ tr/\n//;
> > +	return $count eq 1 && $output =~ m{^${filename}$};
> > +}
> 
> Isn't that somewhat expensive to do for each file?

Just FYI:  Not really.

On my 4 year old laptop git ls-files -- <file> takes
about .02 seconds.

$ time git ls-files -- 'net/l2tp/l2tp_ip.c'
net/l2tp/l2tp_ip.c

real	0m0.013s
user	0m0.009s
sys	0m0.004s

Even uncached, it's quite quick.

# sync; echo 3 > /proc/sys/vm/drop_caches
$ time git ls-files -- 'net/l2tp/l2tp_ip.c'
net/l2tp/l2tp_ip.c

real	0m0.079s
user	0m0.004s
sys	0m0.037s

cheers, Joe


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

* Re: [PATCH] checkpatch: Allow not using -f with files that are in git
  2020-08-25  0:09 [PATCH] checkpatch: Allow not using -f with files that are in git Joe Perches
  2020-08-25 12:23 ` Rasmus Villemoes
@ 2020-09-13  4:02 ` Joe Perches
  2020-10-18 14:03 ` Geert Uytterhoeven
  2 siblings, 0 replies; 9+ messages in thread
From: Joe Perches @ 2020-09-13  4:02 UTC (permalink / raw)
  To: Andrew Morton; +Cc: Andy Whitcroft, LKML

On Mon, 2020-08-24 at 17:09 -0700, Joe Perches wrote:
> If a file exists in git and checkpatch is used without the -f
> flag for scanning a file, then checkpatch will scan the file
> assuming it's a patch and emit:
> 
> ERROR: Does not appear to be a unified-diff format patch
> 
> Change the behavior to assume the -f flag if the file exists
> in git.

Andrew?  ping?

> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  scripts/checkpatch.pl | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 79fc357b18cd..cdee7cfadc11 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -976,6 +976,16 @@ sub seed_camelcase_includes {
>  	}
>  }
>  
> +sub git_is_single_file {
> +	my ($filename) = @_;
> +
> +	return 0 if ((which("git") eq "") || !(-e "$gitroot"));
> +
> +	my $output = `${git_command} ls-files -- $filename`;
> +	my $count = $output =~ tr/\n//;
> +	return $count eq 1 && $output =~ m{^${filename}$};
> +}
> +
>  sub git_commit_info {
>  	my ($commit, $id, $desc) = @_;
>  
> @@ -1049,6 +1059,9 @@ my $vname;
>  $allow_c99_comments = !defined $ignore_type{"C99_COMMENT_TOLERANCE"};
>  for my $filename (@ARGV) {
>  	my $FILE;
> +	my $is_git_file = git_is_single_file($filename);
> +	my $oldfile = $file;
> +	$file = 1 if ($is_git_file);
>  	if ($git) {
>  		open($FILE, '-|', "git format-patch -M --stdout -1 $filename") ||
>  			die "$P: $filename: git format-patch failed - $!\n";
> @@ -1093,6 +1106,7 @@ for my $filename (@ARGV) {
>  	@modifierListFile = ();
>  	@typeListFile = ();
>  	build_types();
> +	$file = $oldfile if ($is_git_file);
>  }
>  
>  if (!$quiet) {
> 


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

* Re: [PATCH] checkpatch: Allow not using -f with files that are in git
  2020-08-25  0:09 [PATCH] checkpatch: Allow not using -f with files that are in git Joe Perches
  2020-08-25 12:23 ` Rasmus Villemoes
  2020-09-13  4:02 ` Joe Perches
@ 2020-10-18 14:03 ` Geert Uytterhoeven
  2020-10-18 16:07   ` Joe Perches
  2 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2020-10-18 14:03 UTC (permalink / raw)
  To: Joe Perches; +Cc: Andrew Morton, Andy Whitcroft, LKML

Hi Joe,

On Tue, Aug 25, 2020 at 2:12 AM Joe Perches <joe@perches.com> wrote:
> If a file exists in git and checkpatch is used without the -f
> flag for scanning a file, then checkpatch will scan the file
> assuming it's a patch and emit:
>
> ERROR: Does not appear to be a unified-diff format patch
>
> Change the behavior to assume the -f flag if the file exists
> in git.
>
> Signed-off-by: Joe Perches <joe@perches.com>

Thanks for your patch!

> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -976,6 +976,16 @@ sub seed_camelcase_includes {
>         }
>  }
>
> +sub git_is_single_file {
> +       my ($filename) = @_;
> +
> +       return 0 if ((which("git") eq "") || !(-e "$gitroot"));
> +
> +       my $output = `${git_command} ls-files -- $filename`;
> +       my $count = $output =~ tr/\n//;
> +       return $count eq 1 && $output =~ m{^${filename}$};
> +}
> +
>  sub git_commit_info {
>         my ($commit, $id, $desc) = @_;
>

This is now commit f5f613259f3fea81 ("checkpatch: allow not using -f
with files that are in git"), causing:

    Global symbol "$gitroot" requires explicit package name (did you
forget to declare "my $gitroot"?) at scripts/checkpatch.pl line 980.
    Execution of scripts/checkpatch.pl aborted due to compilation errors.

FWIW, host system is running Ubuntu 18.04.5 LTS (upgrade to 20.04 LTS
planned soon ;-).

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

* Re: [PATCH] checkpatch: Allow not using -f with files that are in git
  2020-10-18 14:03 ` Geert Uytterhoeven
@ 2020-10-18 16:07   ` Joe Perches
  2020-10-18 18:15     ` Geert Uytterhoeven
  0 siblings, 1 reply; 9+ messages in thread
From: Joe Perches @ 2020-10-18 16:07 UTC (permalink / raw)
  To: Geert Uytterhoeven, Linus Torvalds; +Cc: Andrew Morton, Andy Whitcroft, LKML

On Sun, 2020-10-18 at 16:03 +0200, Geert Uytterhoeven wrote:
> Hi Joe,
> 
> On Tue, Aug 25, 2020 at 2:12 AM Joe Perches <joe@perches.com> wrote:
> > If a file exists in git and checkpatch is used without the -f
> > flag for scanning a file, then checkpatch will scan the file
> > assuming it's a patch and emit:
> > 
> > ERROR: Does not appear to be a unified-diff format patch
> > 
> > Change the behavior to assume the -f flag if the file exists
> > in git.
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> 
> Thanks for your patch!
> 
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -976,6 +976,16 @@ sub seed_camelcase_includes {
> >         }
> >  }
> > 
> > +sub git_is_single_file {
> > +       my ($filename) = @_;
> > +
> > +       return 0 if ((which("git") eq "") || !(-e "$gitroot"));
> > +
> > +       my $output = `${git_command} ls-files -- $filename`;
> > +       my $count = $output =~ tr/\n//;
> > +       return $count eq 1 && $output =~ m{^${filename}$};
> > +}
> > +
> >  sub git_commit_info {
> >         my ($commit, $id, $desc) = @_;
> > 
> 
> This is now commit f5f613259f3fea81 ("checkpatch: allow not using -f
> with files that are in git"), causing:
> 
>     Global symbol "$gitroot" requires explicit package name (did you
> forget to declare "my $gitroot"?) at scripts/checkpatch.pl line 980.
>     Execution of scripts/checkpatch.pl aborted due to compilation errors.
> 
> FWIW, host system is running Ubuntu 18.04.5 LTS (upgrade to 20.04 LTS
> planned soon ;-).

I believe there is a dependency on another patch
in -next that wasn't pushed to Linus' tree.

commit 5ec1f7de97b26a3fa364bbb31fdd2e42c8e6fa22
Author: Joe Perches <joe@perches.com>
Date:   Thu Oct 8 11:53:44 2020 +1100

    checkpatch: test $GIT_DIR changes

So it'd be better to revert right now until
this other patch is accepted or pushed.


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

* Re: [PATCH] checkpatch: Allow not using -f with files that are in git
  2020-10-18 16:07   ` Joe Perches
@ 2020-10-18 18:15     ` Geert Uytterhoeven
  2020-10-18 18:26       ` Joe Perches
  0 siblings, 1 reply; 9+ messages in thread
From: Geert Uytterhoeven @ 2020-10-18 18:15 UTC (permalink / raw)
  To: Joe Perches; +Cc: Linus Torvalds, Andrew Morton, Andy Whitcroft, LKML

Hi Joe,

On Sun, Oct 18, 2020 at 6:07 PM Joe Perches <joe@perches.com> wrote:
> On Sun, 2020-10-18 at 16:03 +0200, Geert Uytterhoeven wrote:
> > On Tue, Aug 25, 2020 at 2:12 AM Joe Perches <joe@perches.com> wrote:
> > > If a file exists in git and checkpatch is used without the -f
> > > flag for scanning a file, then checkpatch will scan the file
> > > assuming it's a patch and emit:
> > >
> > > ERROR: Does not appear to be a unified-diff format patch
> > >
> > > Change the behavior to assume the -f flag if the file exists
> > > in git.
> > >
> > > Signed-off-by: Joe Perches <joe@perches.com>
> >
> > Thanks for your patch!
> >
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -976,6 +976,16 @@ sub seed_camelcase_includes {
> > >         }
> > >  }
> > >
> > > +sub git_is_single_file {
> > > +       my ($filename) = @_;
> > > +
> > > +       return 0 if ((which("git") eq "") || !(-e "$gitroot"));
> > > +
> > > +       my $output = `${git_command} ls-files -- $filename`;
> > > +       my $count = $output =~ tr/\n//;
> > > +       return $count eq 1 && $output =~ m{^${filename}$};
> > > +}
> > > +
> > >  sub git_commit_info {
> > >         my ($commit, $id, $desc) = @_;
> > >
> >
> > This is now commit f5f613259f3fea81 ("checkpatch: allow not using -f
> > with files that are in git"), causing:
> >
> >     Global symbol "$gitroot" requires explicit package name (did you
> > forget to declare "my $gitroot"?) at scripts/checkpatch.pl line 980.
> >     Execution of scripts/checkpatch.pl aborted due to compilation errors.
> >
> > FWIW, host system is running Ubuntu 18.04.5 LTS (upgrade to 20.04 LTS
> > planned soon ;-).
>
> I believe there is a dependency on another patch
> in -next that wasn't pushed to Linus' tree.
>
> commit 5ec1f7de97b26a3fa364bbb31fdd2e42c8e6fa22
> Author: Joe Perches <joe@perches.com>
> Date:   Thu Oct 8 11:53:44 2020 +1100
>
>     checkpatch: test $GIT_DIR changes
>
> So it'd be better to revert right now until
> this other patch is accepted or pushed.

Thanks, after cherry-picking that one from next, checkpatch works again.
However, there are some issues with that commit:
  1. ERROR: Missing Signed-off-by: line by nominal patch author 'Joe
Perches <joe@perches.com>',
  2. The Link: is bogus, and gives 404.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

* Re: [PATCH] checkpatch: Allow not using -f with files that are in git
  2020-10-18 18:15     ` Geert Uytterhoeven
@ 2020-10-18 18:26       ` Joe Perches
  0 siblings, 0 replies; 9+ messages in thread
From: Joe Perches @ 2020-10-18 18:26 UTC (permalink / raw)
  To: Geert Uytterhoeven; +Cc: Linus Torvalds, Andrew Morton, Andy Whitcroft, LKML

On Sun, 2020-10-18 at 20:15 +0200, Geert Uytterhoeven wrote:
> Hi Joe,

rehi Geert

> On Sun, Oct 18, 2020 at 6:07 PM Joe Perches <joe@perches.com> wrote:
> > On Sun, 2020-10-18 at 16:03 +0200, Geert Uytterhoeven wrote:
[]
> > > This is now commit f5f613259f3fea81 ("checkpatch: allow not using -f
> > > with files that are in git"), causing:
> > > 
> > >     Global symbol "$gitroot" requires explicit package name (did you
> > > forget to declare "my $gitroot"?) at scripts/checkpatch.pl line 980.
> > >     Execution of scripts/checkpatch.pl aborted due to compilation errors.
[]
> > I believe there is a dependency on another patch
> > in -next that wasn't pushed to Linus' tree.
> > 
> > commit 5ec1f7de97b26a3fa364bbb31fdd2e42c8e6fa22
> > Author: Joe Perches <joe@perches.com>
> > Date:   Thu Oct 8 11:53:44 2020 +1100
> > 
> >     checkpatch: test $GIT_DIR changes
> > 
> > So it'd be better to revert right now until
> > this other patch is accepted or pushed.
> 
> Thanks, after cherry-picking that one from next, checkpatch works again.
> However, there are some issues with that commit:
>   1. ERROR: Missing Signed-off-by: line by nominal patch author 'Joe
> Perches <joe@perches.com>',
>   2. The Link: is bogus, and gives 404.

I generally create patches against -next.

The above commit was a test patch for Andrew who
had some inconvenience because he doesn't generally
use git or has a git repo in some non-standard path.

I believe it works well enough to be OK, but I
didn't test it and don't have the same setup.

I'll post it again as a reply to this email with a
with a sign-off and a better commit description and
Linus/Andrew can decide if it's better to revert
f5f613259f3f or apply it separately.



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

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-25  0:09 [PATCH] checkpatch: Allow not using -f with files that are in git Joe Perches
2020-08-25 12:23 ` Rasmus Villemoes
2020-08-28  8:03   ` Joe Perches
2020-08-28 18:17   ` Joe Perches
2020-09-13  4:02 ` Joe Perches
2020-10-18 14:03 ` Geert Uytterhoeven
2020-10-18 16:07   ` Joe Perches
2020-10-18 18:15     ` Geert Uytterhoeven
2020-10-18 18:26       ` Joe Perches

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git