Re: [Qemu-devel] [PULL 44/50] scripts: let checkpatch.pl process an entire GIT branch

[Alex Williamson]

On Tue, 19 Sep 2017 14:29:33 +0200
Paolo Bonzini <address@hidden> wrote:

> From: "Daniel P. Berrange" <address@hidden>
> 
> Currently before submitting a series, devs should run checkpatch.pl
> across each patch to be submitted. This can be automated using a
> command such as:
> 
>   git rebase -i master -x 'git show | ./scripts/checkpatch.pl -'
> 
> This is rather long winded to type, so this patch introduces a way
> to tell checkpatch.pl to validate a series of GIT revisions.
> 
> There are now three modes it can operate in 1) check a patch 2) check a source
> file, or 3) check a git branch.
> 
> If no flags are given, the mode is determined by checking the args passed to
> the command. If the args contain a literal ".." it is treated as a GIT 
> revision
> list. If the args end in ".patch" or equal "-" it is treated as a patch file.
> Otherwise it is treated as a source file.
> 
> This automatic guessing can be overridden using --[no-]patch --[no-]file or
> --[no-]branch
> 
> For example to check a GIT revision list:
> 
>     $ ./scripts/checkpatch.pl master..
>     total: 0 errors, 0 warnings, 297 lines checked
> 
>     b886d352a2bf58f0996471fb3991a138373a2957 has no obvious style problems 
> and is ready for submission.
>     total: 0 errors, 0 warnings, 182 lines checked
> 
>     2a731f9a9ce145e0e0df6d42dd2a3ce4dfc543fa has no obvious style problems 
> and is ready for submission.
>     total: 0 errors, 0 warnings, 102 lines checked
> 
>     11844169bcc0c8ed4449eb3744a69877ed329dd7 has no obvious style problems 
> and is ready for submission.
> 
> If a genuine patch filename contains the characters '..' it is
> possible to force interpretation of the arg as a patch
> 
>   $ ./scripts/checkpatch.pl --patch master..
> 
> will force it to load a patch file called "master..", or equivalently
> 
>   $ ./scripts/checkpatch.pl --no-branch master..
> 
> will simply turn off guessing of GIT revision lists.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> Message-Id: <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  scripts/checkpatch.pl | 138 
> ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 111 insertions(+), 27 deletions(-)


This introduces the following regression for me:

$ ./scripts/checkpatch.pl patches-next/vfio-pci-add-virtual
ERROR: trailing whitespace
#44: FILE: vfio-pci-add-virtual:44:
+ $

ERROR: trailing whitespace
#50: FILE: vfio-pci-add-virtual:50:
+ $

ERROR: trailing whitespace
#60: FILE: vfio-pci-add-virtual:60:
+ $

ERROR: trailing whitespace
#62: FILE: vfio-pci-add-virtual:62:
+ $

total: 4 errors, 0 warnings, 62 lines checked

patches-next/vfio-pci-add-virtual has style problems, please review.  If any of 
these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

This is reported for the following patch, which contains no trailing
whitespace errors:

https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg05828.html

$ perl -v

This is perl, v5.10.1 (*) built for x86_64-linux-thread-multi

Thanks,
Alex

 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index fa47807..28d71b3 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -18,11 +18,12 @@ use Getopt::Long qw(:config no_auto_abbrev);
>  my $quiet = 0;
>  my $tree = 1;
>  my $chk_signoff = 1;
> -my $chk_patch = 1;
> +my $chk_patch = undef;
> +my $chk_branch = undef;
>  my $tst_only;
>  my $emacs = 0;
>  my $terse = 0;
> -my $file = 0;
> +my $file = undef;
>  my $no_warnings = 0;
>  my $summary = 1;
>  my $mailback = 0;
> @@ -35,14 +36,19 @@ sub help {
>       my ($exitcode) = @_;
>  
>       print << "EOM";
> -Usage: $P [OPTION]... [FILE]...
> +Usage:
> +
> +    $P [OPTION]... [FILE]...
> +    $P [OPTION]... [GIT-REV-LIST]
> +
>  Version: $V
>  
>  Options:
>    -q, --quiet                quiet
>    --no-tree                  run without a kernel tree
>    --no-signoff               do not check for 'Signed-off-by' line
> -  --patch                    treat FILE as patchfile (default)
> +  --patch                    treat FILE as patchfile
> +  --branch                   treat args as GIT revision list
>    --emacs                    emacs compile window format
>    --terse                    one line per report
>    -f, --file                 treat FILE as regular source file
> @@ -69,6 +75,7 @@ GetOptions(
>       'tree!'         => \$tree,
>       'signoff!'      => \$chk_signoff,
>       'patch!'        => \$chk_patch,
> +     'branch!'       => \$chk_branch,
>       'emacs!'        => \$emacs,
>       'terse!'        => \$terse,
>       'f|file!'       => \$file,
> @@ -93,6 +100,49 @@ if ($#ARGV < 0) {
>       exit(1);
>  }
>  
> +if (!defined $chk_branch && !defined $chk_patch && !defined $file) {
> +     $chk_branch = $ARGV[0] =~ /\.\./ ? 1 : 0;
> +     $chk_patch = $chk_branch ? 0 :
> +             $ARGV[0] =~ /\.patch$/ || $ARGV[0] eq "-" ? 1 : 0;
> +     $file = $chk_branch || $chk_patch ? 0 : 1;
> +} elsif (!defined $chk_branch && !defined $chk_patch) {
> +     if ($file) {
> +             $chk_branch = $chk_patch = 0;
> +     } else {
> +             $chk_branch = $ARGV[0] =~ /\.\./ ? 1 : 0;
> +             $chk_patch = $chk_branch ? 0 : 1;
> +     }
> +} elsif (!defined $chk_branch && !defined $file) {
> +     if ($chk_patch) {
> +             $chk_branch = $file = 0;
> +     } else {
> +             $chk_branch = $ARGV[0] =~ /\.\./ ? 1 : 0;
> +             $file = $chk_branch ? 0 : 1;
> +     }
> +} elsif (!defined $chk_patch && !defined $file) {
> +     if ($chk_branch) {
> +             $chk_patch = $file = 0;
> +     } else {
> +             $chk_patch = $ARGV[0] =~ /\.patch$/ || $ARGV[0] eq "-" ? 1 : 0;
> +             $file = $chk_patch ? 0 : 1;
> +     }
> +} elsif (!defined $chk_branch) {
> +     $chk_branch = $chk_patch || $file ? 0 : 1;
> +} elsif (!defined $chk_patch) {
> +     $chk_patch = $chk_branch || $file ? 0 : 1;
> +} elsif (!defined $file) {
> +     $file = $chk_patch || $chk_branch ? 0 : 1;
> +}
> +
> +if (($chk_patch && $chk_branch) ||
> +    ($chk_patch && $file) ||
> +    ($chk_branch && $file)) {
> +     die "Only one of --file, --branch, --patch is permitted\n";
> +}
> +if (!$chk_patch && !$chk_branch && !$file) {
> +     die "One of --file, --branch, --patch is required\n";
> +}
> +
>  my $dbg_values = 0;
>  my $dbg_possible = 0;
>  my $dbg_type = 0;
> @@ -251,32 +301,66 @@ $chk_signoff = 0 if ($file);
>  my @rawlines = ();
>  my @lines = ();
>  my $vname;
> -for my $filename (@ARGV) {
> -     my $FILE;
> -     if ($file) {
> -             open($FILE, '-|', "diff -u /dev/null $filename") ||
> -                     die "$P: $filename: diff failed - $!\n";
> -     } elsif ($filename eq '-') {
> -             open($FILE, '<&STDIN');
> -     } else {
> -             open($FILE, '<', "$filename") ||
> -                     die "$P: $filename: open failed - $!\n";
> -     }
> -     if ($filename eq '-') {
> -             $vname = 'Your patch';
> -     } else {
> -             $vname = $filename;
> -     }
> -     while (<$FILE>) {
> +if ($chk_branch) {
> +     my @patches;
> +     my $HASH;
> +     open($HASH, "-|", "git", "log", "--format=%H", $ARGV[0]) ||
> +             die "$P: git log --format=%H $ARGV[0] failed - $!\n";
> +
> +     while (<$HASH>) {
>               chomp;
> -             push(@rawlines, $_);
> +             push @patches, $_;
> +     }
> +
> +     close $HASH;
> +
> +     die "$P: no revisions returned for revlist '$chk_branch'\n"
> +         unless @patches;
> +
> +     for my $hash (@patches) {
> +             my $FILE;
> +             open($FILE, '-|', "git", "show", $hash) ||
> +                     die "$P: git show $hash - $!\n";
> +             $vname = $hash;
> +             while (<$FILE>) {
> +                     chomp;
> +                     push(@rawlines, $_);
> +             }
> +             close($FILE);
> +             if (!process($hash)) {
> +                     $exit = 1;
> +             }
> +             @rawlines = ();
> +             @lines = ();
>       }
> -     close($FILE);
> -     if (!process($filename)) {
> -             $exit = 1;
> +} else {
> +     for my $filename (@ARGV) {
> +             my $FILE;
> +             if ($file) {
> +                     open($FILE, '-|', "diff -u /dev/null $filename") ||
> +                             die "$P: $filename: diff failed - $!\n";
> +             } elsif ($filename eq '-') {
> +                     open($FILE, '<&STDIN');
> +             } else {
> +                     open($FILE, '<', "$filename") ||
> +                             die "$P: $filename: open failed - $!\n";
> +             }
> +             if ($filename eq '-') {
> +                     $vname = 'Your patch';
> +             } else {
> +                     $vname = $filename;
> +             }
> +             while (<$FILE>) {
> +                     chomp;
> +                     push(@rawlines, $_);
> +             }
> +             close($FILE);
> +             if (!process($filename)) {
> +                     $exit = 1;
> +             }
> +             @rawlines = ();
> +             @lines = ();
>       }
> -     @rawlines = ();
> -     @lines = ();
>  }
>  
>  exit($exit);