Message-ID: <20160828023807.GC19088@sasha-lappy>
Date:   Sat, 27 Aug 2016 22:47:40 -0400
From:   "Levin, Alexander" <alexander.levin@...izon.com>
To:     Joe Perches <joe@...ches.com>
Cc:     "Levin, Alexander" <alexander.levin@...izon.com>,
        Sasha Levin <levinsasha928@...il.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "ksummit-discuss@...ts.linuxfoundation.org" 
        <ksummit-discuss@...ts.linuxfoundation.org>
Subject: Re: checkkpatch (in)sanity ?

On Sat, Aug 27, 2016 at 09:42:59PM -0400, Joe Perches wrote:
> On Sat, 2016-08-27 at 21:06 -0400, Levin, Alexander wrote:
> > On Sat, Aug 27, 2016 at 04:40:52PM -0400, Joe Perches wrote:
> > > On Fri, Aug 26, 2016 at 01:26:35PM +0200, Greg KH wrote:
> > > > On Fri, Aug 26, 2016 at 12:46:51AM -0400, Levin, Alexander wrote:
> > > > > 
> > > > >     - Making checkpatch check for (some) of the stable kernel rules
> > > > >     (and possibly recommend adding the stable@ tag in certain cases?).
> > > > >       - Depends on: making checkpatch sane again
> > > > > >This sounds interesting.  What do you mean by "sane"?
> > > Sasha, can you expand your thoughts here please?
> > Sure. I have 2.5 concerns about the state of checkpatch:
> []
> > > Most all of the trivial spacing stuff can easily be
> > > ignored either by a human determining what's important
> > > or by using command line options like --ignore=spacing
> > 1.
> > This is the wrong default. By default checkpatch shouldn't be showing trivial
> > issues that encourage folks to try and work around them and as a result
> > produce worse code.
> > 
> > Look at the 80 character limit warning for example, what good does it do?
> 
> That argument's been done several times. It keeps Linus happy.
> I don't care one way or another.

I'm not trying to be specific with the 80 character thing, it's also true for
a few other things that makes people produce less readable code than what it
would have looked like if they'd ignore the warning.
 
> I think the biggest issue is the seriousness that some people
> take checkpatch messages as dicta instead of ignorable bleats.

That makes sense to you, but it doesn't make sense to the newer folks who are
told not to submit any patches with checkpatch errors/warnings. You know to
ignore these 80-character warnings when it makes sense, they see it as "you
must make the warning disappear no matter what".
 
> I still think ERROR->defect, WARNING->unstylish, CHECK->nitpick
> would be a good change.
> 
> https://lkml.org/lkml/2015/7/16/568

Probably. Would you agree that by default we shouldn't show anything that's
not an error/defect?

> >  It
> > encourages people to do even stupider things to work around it and results in
> > a bunch of "fix checkpatch warning" that touch existing code just to make the
> > result harder to read and make 'git blame' harder to work with.
> 
> Almost all of the crud in git-blame can be avoided with -w

That doesn't deal with newlines people add to hide the 80 character stuff, nor it
deals with the "harder to read" part.

> > By default you should only get the most critical warnings we have in the
> > kernel like missing S-O-B or corrupt patch.
> 
> I don't think so, but if you do, add a filter for ERROR only.

I could, but the problem is the people who see the default output as "holy".

> > 2. A "who wrote these rules?": there seems to be a disconnect between the rules
> > checkpatch is trying to enforce and the accepted coding style enforced by
> > maintainers. 
> 
> Name some please.

Well look at the git commit id SHA1 length thingie for example (GIT_COMMIT_ID). checkpatch says 12 chars minimum, but as far as I can tell Linus and Greg didn't get the memo.

> > Do a git-format-patch on all of the commits Linus authored in the past year or
> > two and see how many of them fail checkpatch (or do the same for any of the
> > commits that passed through and were accepted by the top maintainers),
> > according to checkpatch we need to make those guys stop touching the kernel.
> 
> Try it yourself and tell me what's wrong with the messages:
> 
> $ git log --pretty=oneline --author=torvalds --no-merges --since=1-year-ago | \
>   grep -v " Linux [34]" | \
>   while read commit ; do \
>     echo $commit ; \
>     git log --stat -p -1 --format=email $(echo $commit | cut -f1 -d" ")  | \
>       ./scripts/checkpatch.pl - ; \
>   done
> 
> Here's a summary done with an additional
> 
>   grep -P "^(ERROR|WARNING)" | cut -f1,2 -d":" | \
>   sort |uniq -c | sort -rn
> 
>      46 WARNING:LONG_LINE_COMMENT
>      45 WARNING:LEADING_SPACE
>      37 WARNING:LONG_LINE
>      16 ERROR:GIT_COMMIT_ID
>      11 WARNING:COMMIT_LOG_LONG_LINE
>       5 WARNING:BRACES
>       2 WARNING:BAD_SIGN_OFF
>       2 WARNING:AVOID_BUG
>       2 ERROR:SPACING
>       1 WARNING:SPLIT_STRING
>       1 WARNING:FILE_PATH_CHANGES
>       1 WARNING:ENOSYS
>       1 ERROR:MISSING_SIGN_OFF

$ git log --pretty=oneline --author=torvalds --no-merges --since=1-year-ago | grep -v " Linux [34]" | wc -l
64

Linus has more errors/warnings than commits. Why do we let him commit stuff?

> > 3. This one is somewhat subjective: scripts/checkpatch.pl is a massive blob of
> > perl code that a fair amount of people don't know how to deal with. In 4.8 it's
> > 6142 lines, making it the 124th largest source file in the kernel, well within
> > the top 1% of source files in the kernel.
> > 
> > This combination of size/language pushes people away from being involved in
> > what is supposed to be a central tool and gives them a reason to never use
> > it again after they see results they don't agree with (rather than fixing it).
> 
> Meh, I'm not a perl guy either.
> 
> I think almost all of it is regexes and most people
> aren't very good at those.
> 
> So it wouldn't matter if it was perl or python.
> 
> spatch isn't the right tool.
> 
> What would you suggest instead?

This is a good topic to talk about, making checkpatch accessible to us
commoners could be useful, we just need to figure out how.

-- 

Thanks,
Sasha

Powered by blists - more mailing lists