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

[Alex Williamson]

On Wed, 4 Oct 2017 09:33:29 +0100
"Daniel P. Berrange" <address@hidden> wrote:

> On Tue, Oct 03, 2017 at 04:07:06PM -0600, Alex Williamson wrote:
> > 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:  
> 
> IIUC, it is treating 'patches-next/vfio-pci-add-virtual' as a plain
> source file, rather than a patch file because its filename doesn't
> include .patch at the end.
> 
> I the heuristic based on the idea that there's always be a .patch
> at the end of patch files. git format-patch at least will do this
> by default - is there a particular reason you've not got any file
> extension ?
> 
> BTW, assuming this diagnosis is correct, if you use --patch
> it should work. eg try
> 
> ./scripts/checkpatch.pl --patch patches-next/vfio-pci-add-virtual

Yes, it works with the new --patch option.  I use stgit which does
not add a filename extension to the patch name on export, nor have I
ever known patch files to have any accepted standard file extension.
checkpatch.pl has always assumed it's operating on a patch file, as
implied by the name of the script, with an option to check regular
files.  I'd suggest maintaining behavior consistent with the script name
using more comprehensive heuristics than a non-standard file extension.
Thanks,

Alex