On 12/09/2017 18:12, Daniel P. Berrange wrote: > On Tue, Sep 12, 2017 at 05:52:18PM +0200, Paolo Bonzini wrote: >> On 12/09/2017 12:46, Daniel P. Berrange wrote: >>> 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 new >>> flag '--branch' to checkpatch.pl which instructs it to check every >>> patch on the current GIT branch. >> >> Great idea, though I'm not sure about having a default. And to keep it >> easy to invoke, having a sole argument that ends with ".." might DWIM >> and enable --branch too... > > I think it is beneficial to have a default, as I figure the majority > of contributors are working on a branch that's rebased against master.. > Half as many characters to type in the common case :-)
With the DWIM option "--branch" and "master.." are exactly the same length. :) > Sometimes people might write patches against a particular subsystem > staging branch (eg kevin/block), but I don't think there's downside > in assuming 'master..' by default. What about "origin/master.." instead? Paolo >> >> Paolo >> >>> For example: >>> >>> $ ./scripts/checkpatch.pl --branch >>> 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. >>> >>> By default it checks every patch identified by 'master..', however, >>> an alternative origin can be given if desired, if the current branch >>> is rebased to another non-master branch: >>> >>> $ ./scripts/checkpatch.pl --branch somebranch.. >>> >>> Signed-off-by: Daniel P. Berrange <berra...@redhat.com> >>> --- >>> scripts/checkpatch.pl | 97 >>> +++++++++++++++++++++++++++++++++++++-------------- >>> 1 file changed, 71 insertions(+), 26 deletions(-) >>> >>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >>> index fa478074b8..f8d080441f 100755 >>> --- a/scripts/checkpatch.pl >>> +++ b/scripts/checkpatch.pl >>> @@ -19,6 +19,8 @@ my $quiet = 0; >>> my $tree = 1; >>> my $chk_signoff = 1; >>> my $chk_patch = 1; >>> +my $chk_branch = 0; >>> +my $revlist = "master.."; >>> my $tst_only; >>> my $emacs = 0; >>> my $terse = 0; >>> @@ -43,6 +45,7 @@ Options: >>> --no-tree run without a kernel tree >>> --no-signoff do not check for 'Signed-off-by' line >>> --patch treat FILE as patchfile (default) >>> + --branch check all patches on branch since master >>> --emacs emacs compile window format >>> --terse one line per report >>> -f, --file treat FILE as regular source file >>> @@ -69,6 +72,7 @@ GetOptions( >>> 'tree!' => \$tree, >>> 'signoff!' => \$chk_signoff, >>> 'patch!' => \$chk_patch, >>> + 'branch' => \$chk_branch, >>> 'emacs!' => \$emacs, >>> 'terse!' => \$terse, >>> 'f|file!' => \$file, >>> @@ -88,9 +92,19 @@ help(0) if ($help); >>> >>> my $exit = 0; >>> >>> -if ($#ARGV < 0) { >>> - print "$P: no input files\n"; >>> - exit(1); >>> +if ($chk_branch) { >>> + if ($#ARGV > 0) { >>> + print "$P: expected zero or one origni revisions\n"; >>> + exit(1); >>> + } >>> + if ($#ARGV == 0) { >>> + $revlist = shift @ARGV; >>> + } >>> +} else { >>> + if ($#ARGV < 0) { >>> + print "$P: no input files\n"; >>> + exit(1); >>> + } >>> } >>> >>> my $dbg_values = 0; >>> @@ -251,32 +265,63 @@ $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", $revlist) || >>> + die "$P: git log --format=%H $revlist failed - $!\n"; >>> + >>> + while (<$HASH>) { >>> chomp; >>> - push(@rawlines, $_); >>> + push @patches, $_; >>> } >>> - close($FILE); >>> - if (!process($filename)) { >>> - $exit = 1; >>> + >>> + close $HASH; >>> + >>> + 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 = (); >>> + } >>> +} 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); >>> >> > > Regards, > Daniel >