Re: [PATCH] checkpatch: do not test/warn of leading whitespacebefore signature tags

[Jeff Kirsher]

On Thu, 2011-08-18 at 01:16 -0700, Joe Perches wrote:
> On Thu, 2011-08-18 at 01:07 -0700, Jeff Kirsher wrote:
> > On Thu, 2011-08-18 at 00:59 -0700, Joe Perches wrote:
> > > On Thu, 2011-08-18 at 00:47 -0700, Jeff Kirsher wrote:
> > > > On Thu, 2011-08-18 at 00:26 -0700, Joe Perches wrote:
> > > > > On Wed, 2011-08-17 at 23:48 -0700, Jeff Kirsher wrote:
> > > > > > From: Bruce Allan <bruce.w.allan@xxxxxxxxx>
> > > > > > 
> > > > > > Commit 2011247 introduced additional style checks for signature tags in
> > > > > > patches which is good.  Unfortunately, now whenever patches are checked
> > > > > > by piping the output of 'git show' or 'stg show' through checkpatch it
> > > > > > warns not to use whitespace before all signature tags since these (and the
> > > > > > rest of the patch description) are indented.  Remove this test/warning.
> > > > > 
> > > > > I think this is not a good idea.
> > > > > 
> > > > > checkpatch is meant for patches not git log output.
> > > > > indenting signatures can cause other problems later.
> > > > > 
> > > > > I think you can avoid this easily by using checkpatch
> > > > > option --ignore=BAD_SIGN_OFF when using git log output
> > > > > as input.
> > > > 
> > > > The problem I have with this is that the sign-off's are not bad, they
> > > > are by default indented by 'git show' or 'stg show' so checkpatch.pl
> > > > should handle the "default" formatting of git/stg and if there is
> > > > additional indenting not expected, then the sign-off's should be
> > > > considered bad.
> > > 
> > > I disagree.
> > > 
> > > checkpatch should handle the default input of patches
> > > as best it can.
> > > 
> > > I suppose checkpatch could have a different "--input=git"
> > > or some such to avoid certain things that git might produce
> > > that a patch would not.
> > 
> > That does sound an alternative which would be acceptable.
> > 
> > > 
> > > Deleting useful checks for patches isn't a good idea.
> > > 
> > > > If this option is added, then if there were "real"
> > > > problems with the sign-off, it would not be displayed.
> > > 
> > > So what?
> > > 
> > > It would also be too late to do anything about
> > > it anyway as it would already be committed.
> > 
> > If you are running it on patches already committed to maintainers tree,
> > but if you are running checkpatch.pl on a patch on your local tree
> > before you send it out, you can correct any changes necessary. 
> > 
> > > 
> > > > > You could also use: 
> > > > > git log --format="commit %H%nAuthor: %an <%ae>%nDate:   %aD%n%n%s%n%n%b"
> > > > > so that you get the current default --format=medium
> > > > > output without indenting the commit log body.
> > > > Even doing this does not resolve the "false" warnings" that
> > > > checkpatch.pl produces regarding the sign-off's.
> > > 
> > > I tried it.  It works for me.
> > > What about it doesn't work for you?
> > > 
> > 
> > I did the following using David Miller's net-next tree...
> > 
> > git log -1 > ../test.patch
> > ./scripts/checkpatch.pl ../test.patch
> > 
> > and I get the following:
> > WARNING: Do not use whitespace before Signed-off-by:
> > #9: 
> >     Signed-off-by: Robin Holt <holt@xxxxxxx>
> 
> Dull.
> 
> You didn't do what I suggested, and you aren't
> checking the actual patch for the commit.
> 
> Try:
> 
> $ git log --format="commit %H%nAuthor: %an <%ae>%nDate:   %aD%n%n%s%n%n%b" \
> 	-p -1 master | ./scripts/checkpatch.pl -
> total: 0 errors, 0 warnings, 826 lines checked
> 
> Your patch has no obvious style problems and is ready for submission.
> 
> (btw: checkpatch accepts input from standard input if you use a trailing "-")
> 
> 

I understand what you saying and asking, my point here is the that the
"default" style should be understood by checkpatch.pl and not produce
false warnings.

Sorry my example showed the long way of doing it, I get the same errors
if I were to simply do:

git log -1 | ./scripts/checkpatch.pl -

Attachment: signature.asc
Description: This is a digitally signed message part