[Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Christian Borntraeger]

A very late proposal, maybe related to Joe Perches thread about checkpatch and
previous discussions about getting new people.

There are a decent amount of patches just dealing with checkpatch warnings or
Codingtyle things and sometimes the quality of these patches is not the best.

1. What is the general opinion about this patch class? It seems to be trade of
between attracting new developers vs. letting people create a huge amount of
patches for no good reason. The acceptance of these patches seems to differ from
maintainer to maintainer. Are there ideas how to improve things for newbies
without inviting patch bombers?

 2. Some patches are created due to the CodingStyle document, e.g. just
changing the name of labels for gotos. Does it make sense to make CodingStyle
less specific again to avoid these change? Or maybe add some rules in
SubmittingPatches to always think twice before sending patches to existing code.
Any better ideas?

3. CodingStyle seems to get changes which have no ACK or Reviewed-by that seem
to be controversial.  e.g.
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51
suggested to indent labels with a space, and was then immediately followed by
patches. Is there a process in place to verify and challenge such changes?

4.  Does it make sense to make checkpatch less agressive on files than on
patches to avoid a big chunk of changes on existing code?

Christian

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Joe Perches]

On Tue, 2016-09-13 at 20:58 +0200, Christian Borntraeger wrote:
> A very late proposal, maybe related to Joe Perches thread about checkpatch and
> previous discussions about getting new people.
>
> There are a decent amount of patches just dealing with checkpatch warnings or
> Codingtyle things and sometimes the quality of these patches is not the best.
>
> 1. What is the general opinion about this patch class? It seems to be trade of
> between attracting new developers vs. letting people create a huge amount of
> patches for no good reason. The acceptance of these patches seems to differ from
> maintainer to maintainer. Are there ideas how to improve things for newbies
> without inviting patch bombers?

checkpatchg is just fine for drivers/staging, but for almost
everything else, it can lead to a lot of whitespace churn and
patches with niggling to some maintainers style-only changes.

I would like to avoid almost all of these by restricting the
checkpatch --file|-f option.

I like the undocumented "--force" option I proposed a couple
years ago:

Reference threads:
https://lkml.org/lkml/2014/7/17/427
https://patchwork.kernel.org/patch/5814071/

>  2. Some patches are created due to the CodingStyle document, e.g. just
> changing the name of labels for gotos. Does it make sense to make CodingStyle
> less specific again to avoid these change? Or maybe add some rules in
> SubmittingPatches to always think twice before sending patches to existing code.
> Any better ideas?

Get Markus Elfing to send fewer style only changes?

> 3. CodingStyle seems to get changes which have no ACK or Reviewed-by that seem
> to be controversial.  e.g.
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51
> suggested to indent labels with a space, and was then immediately followed by
> patches. Is there a process in place to verify and challenge such changes?

Perhaps more review of these proposed style changes?

In a lot of cases, the CodingStyle document, in either the
.txt form or the new .rst style, is overly detailed.

Style consistency is good, but there's nothing fundamentally
wrong with developers using different styles as long as the
code produced is intelligible and maintainable.

> 4.  Does it make sense to make checkpatch less agressive on files than on
> patches to avoid a big chunk of changes on existing code?

Mostly that's a checkpatch message type classification problem.

> 

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Josh Triplett]

On Tue, Sep 13, 2016 at 08:58:49PM +0200, Christian Borntraeger wrote:
> 3. CodingStyle seems to get changes which have no ACK or Reviewed-by that seem
> to be controversial.  e.g.
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51
> suggested to indent labels with a space, and was then immediately followed by
> patches. Is there a process in place to verify and challenge such changes?

Ideally, that should come up during review of the CodingStyle patch.
Changes shouldn't go into CodingStyle except to document existing
process and unwritten rules, or to document the results of a discussion
and consensus.  That particular change to CodingStyle should have been
rejected, and should be reverted.

- Josh Triplett

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Jonathan Corbet]

On Tue, 13 Sep 2016 12:45:20 -0700
Josh Triplett <josh@joshtriplett.org> wrote:

> On Tue, Sep 13, 2016 at 08:58:49PM +0200, Christian Borntraeger wrote:
> > 3. CodingStyle seems to get changes which have no ACK or Reviewed-by that seem
> > to be controversial.  e.g.
> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51
> > suggested to indent labels with a space, and was then immediately followed by
> > patches. Is there a process in place to verify and challenge such changes?  
> 
> Ideally, that should come up during review of the CodingStyle patch.
> Changes shouldn't go into CodingStyle except to document existing
> process and unwritten rules, or to document the results of a discussion
> and consensus.  That particular change to CodingStyle should have been
> rejected, and should be reverted.

So I'm quite reluctant to take CodingStyle patches for just this reason;
*I* certainly don't want to be the one dictating style for the kernel, but
I'm not really sure who does.  In my time as the docs maintainer I've only
applied two patches there that constitute any sort of rule change - this
one and a78a136fa9337fdc25fdbaa2d253f9b4dc90ad44.  

In general, I would welcome advice on how any future rule-change patches
should be reviewed.

(FWIW, I'm not really sure how I came to take the one mentioned above, I
guess I was having a bad day.  The space-before-label rule strikes me as
strange at best...)

jon

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Joe Perches]

On Tue, 2016-09-13 at 14:03 -0600, Jonathan Corbet wrote:
> On Tue, 13 Sep 2016 12:45:20 -0700 Josh Triplett <josh@joshtriplett.org> wrote:
> > On Tue, Sep 13, 2016 at 08:58:49PM +0200, Christian Borntraeger wrote:
> > > 3. CodingStyle seems to get changes which have no ACK or Reviewed-by that seem
> > > to be controversial.  e.g.
> > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51
> > > suggested to indent labels with a space, and was then immediately followed by
> > > patches. Is there a process in place to verify and challenge such changes?  
> > Ideally, that should come up during review of the CodingStyle patch.
> > Changes shouldn't go into CodingStyle except to document existing
> > process and unwritten rules, or to document the results of a discussion
> > and consensus.  That particular change to CodingStyle should have been
> > rejected, and should be reverted.
> 
> 
> 
> 
> So I'm quite reluctant to take CodingStyle patches for just this reason;
> *I* certainly don't want to be the one dictating style for the kernel, but
> I'm not really sure who does.
[]
> (FWIW, I'm not really sure how I came to take the one mentioned above, I
> guess I was having a bad day.  The space-before-label rule strikes me as
> strange at best...)

The space-before-label rule should be removed.

It's a silly work-around rule for those that don't like
to see labels in a diff instead of a function name.

A new .gitattributes patch from Jean Delvare fixes that.

References:

https://lkml.org/lkml/2011/8/29/410
https://lkml.org/lkml/2016/9/6/445
https://lkml.org/lkml/2016/9/7/316

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Rafael J. Wysocki]

On Tuesday, September 13, 2016 02:03:22 PM Jonathan Corbet wrote:
> On Tue, 13 Sep 2016 12:45:20 -0700
> Josh Triplett <josh@joshtriplett.org> wrote:
> 
> > On Tue, Sep 13, 2016 at 08:58:49PM +0200, Christian Borntraeger wrote:
> > > 3. CodingStyle seems to get changes which have no ACK or Reviewed-by that seem
> > > to be controversial.  e.g.
> > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51
> > > suggested to indent labels with a space, and was then immediately followed by
> > > patches. Is there a process in place to verify and challenge such changes?  
> > 
> > Ideally, that should come up during review of the CodingStyle patch.
> > Changes shouldn't go into CodingStyle except to document existing
> > process and unwritten rules, or to document the results of a discussion
> > and consensus.  That particular change to CodingStyle should have been
> > rejected, and should be reverted.
> 
> So I'm quite reluctant to take CodingStyle patches for just this reason;
> *I* certainly don't want to be the one dictating style for the kernel, but
> I'm not really sure who does.  In my time as the docs maintainer I've only
> applied two patches there that constitute any sort of rule change - this
> one and a78a136fa9337fdc25fdbaa2d253f9b4dc90ad44.  
> 
> In general, I would welcome advice on how any future rule-change patches
> should be reviewed.

I agree with Josh that CodingStyle should reflect the existing practice
(present in the majority of the kernel source) or a broad consensus.

While the "existing practice" case is relatively simple (it boils down to
demonstrating that the given rule is in fact followed in practice in the
majority of the kernel source), the "broad consensus" one is not as
straightforward.  It looks like a Kernel Summit discussion or equivalent
would be required each time to be honest ...

In any case, it might be good to state somewhere that CodingStyle is a
guidance for new code and not a prescription for how all of the kernel code
must look like.

Thanks,
Rafael

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Josh Triplett]

On Wed, Sep 14, 2016 at 01:49:13AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, September 13, 2016 02:03:22 PM Jonathan Corbet wrote:
> > On Tue, 13 Sep 2016 12:45:20 -0700
> > Josh Triplett <josh@joshtriplett.org> wrote:
> > 
> > > On Tue, Sep 13, 2016 at 08:58:49PM +0200, Christian Borntraeger wrote:
> > > > 3. CodingStyle seems to get changes which have no ACK or Reviewed-by that seem
> > > > to be controversial.  e.g.
> > > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51
> > > > suggested to indent labels with a space, and was then immediately followed by
> > > > patches. Is there a process in place to verify and challenge such changes?  
> > > 
> > > Ideally, that should come up during review of the CodingStyle patch.
> > > Changes shouldn't go into CodingStyle except to document existing
> > > process and unwritten rules, or to document the results of a discussion
> > > and consensus.  That particular change to CodingStyle should have been
> > > rejected, and should be reverted.
> > 
> > So I'm quite reluctant to take CodingStyle patches for just this reason;
> > *I* certainly don't want to be the one dictating style for the kernel, but
> > I'm not really sure who does.  In my time as the docs maintainer I've only
> > applied two patches there that constitute any sort of rule change - this
> > one and a78a136fa9337fdc25fdbaa2d253f9b4dc90ad44.  
> > 
> > In general, I would welcome advice on how any future rule-change patches
> > should be reviewed.
> 
> I agree with Josh that CodingStyle should reflect the existing practice
> (present in the majority of the kernel source) or a broad consensus.
> 
> While the "existing practice" case is relatively simple (it boils down to
> demonstrating that the given rule is in fact followed in practice in the
> majority of the kernel source), the "broad consensus" one is not as
> straightforward.  It looks like a Kernel Summit discussion or equivalent
> would be required each time to be honest ...

I suspect a mailing list discussion might suffice, if enough people
weigh in with feedback.

> In any case, it might be good to state somewhere that CodingStyle is a
> guidance for new code and not a prescription for how all of the kernel code
> must look like.

Yes, the preface of the document should explicitly mention this.  "Do
not mass-reformat existing code, even if it doesn't follow these
guidelines; doing so creates noise in version control history and makes
patches fail to apply."

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Joe Perches]

On Tue, 2016-09-13 at 19:03 -0700, Josh Triplett wrote:
> "Do
> not mass-reformat existing code, even if it doesn't follow these
> guidelines; doing so creates noise in version control history and makes
> patches fail to apply."

It also could be useful to somehow separate newer, more actively
developed code from the old stuff that should remain more static.

Old code should really only get code improvements like API changes,
size reductions, constifications, performance improvements and bug
fixes.

Maybe change the MAINTAINERS sections S: entries to have more
comprehensive descriptions of what types of changes are acceptable
for each section.

Right now it's

S:	Supported
or
S:	Maintained

Or maybe add something like a new entry for what types of changes
are acceptable with a default of "none"

C:	Whitespace and Style	

checkpatch could warn when changes are proposed for code that should
remain static.

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Julia Lawall]

On Tue, 13 Sep 2016, Joe Perches wrote:

> On Tue, 2016-09-13 at 14:03 -0600, Jonathan Corbet wrote:
> > On Tue, 13 Sep 2016 12:45:20 -0700 Josh Triplett <josh@joshtriplett.org> wrote:
> > > On Tue, Sep 13, 2016 at 08:58:49PM +0200, Christian Borntraeger wrote:
> > > > 3. CodingStyle seems to get changes which have no ACK or Reviewed-by that seem
> > > > to be controversial.  e.g.
> > > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51
> > > > suggested to indent labels with a space, and was then immediately followed by
> > > > patches. Is there a process in place to verify and challenge such changes?
> > > Ideally, that should come up during review of the CodingStyle patch.
> > > Changes shouldn't go into CodingStyle except to document existing
> > > process and unwritten rules, or to document the results of a discussion
> > > and consensus.  That particular change to CodingStyle should have been
> > > rejected, and should be reverted.
> >
> >
> >
> >
> > So I'm quite reluctant to take CodingStyle patches for just this reason;
> > *I* certainly don't want to be the one dictating style for the kernel, but
> > I'm not really sure who does.
> []
> > (FWIW, I'm not really sure how I came to take the one mentioned above, I
> > guess I was having a bad day.  The space-before-label rule strikes me as
> > strange at best...)
>
> The space-before-label rule should be removed.
>
> It's a silly work-around rule for those that don't like
> to see labels in a diff instead of a function name.
>
> A new .gitattributes patch from Jean Delvare fixes that.
>
> References:
>
> https://lkml.org/lkml/2011/8/29/410
> https://lkml.org/lkml/2016/9/6/445
> https://lkml.org/lkml/2016/9/7/316

Having git log etc do the right thing is really useful if one wants to
analyze patches.  I hope that there will be the right behavior with no
effort on the part of the user as soon as possible.

julia

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Julia Lawall]

On Tue, 13 Sep 2016, Joe Perches wrote:

> On Tue, 2016-09-13 at 19:03 -0700, Josh Triplett wrote:
> > "Do
> > not mass-reformat existing code, even if it doesn't follow these
> > guidelines; doing so creates noise in version control history and makes
> > patches fail to apply."
>
> It also could be useful to somehow separate newer, more actively
> developed code from the old stuff that should remain more static.
>
> Old code should really only get code improvements like API changes,
> size reductions, constifications, performance improvements and bug
> fixes.
>
> Maybe change the MAINTAINERS sections S: entries to have more
> comprehensive descriptions of what types of changes are acceptable
> for each section.
>
> Right now it's
>
> S:	Supported
> or
> S:	Maintained
>
> Or maybe add something like a new entry for what types of changes
> are acceptable with a default of "none"

What types of changes are unacceptable?  What types of changes are
acceptable could be difficult to summarize in advance.

julia

> C:	Whitespace and Style
>
> checkpatch could warn when changes are proposed for code that should
> remain static.
> _______________________________________________
> Ksummit-discuss mailing list
> Ksummit-discuss@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
>

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Joe Perches]

On Wed, 2016-09-14 at 07:57 +0200, Julia Lawall wrote:
> What types of changes are unacceptable?

It's a mixed bag.

Some maintainers reject all "style/whitespace changes".
Some maintainers reject global consistency patches like
int -> bool conversions.
Some maintainers reject literal -> #define changes like
1 -> true and 0 -> false for booleans.

Some of those maintainers are IMO misguided.

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Julia Lawall]

On Tue, 13 Sep 2016, Joe Perches wrote:

> On Wed, 2016-09-14 at 07:57 +0200, Julia Lawall wrote:
> > What types of changes are unacceptable?
>
> It's a mixed bag.
>
> Some maintainers reject all "style/whitespace changes".
> Some maintainers reject global consistency patches like
> int -> bool conversions.
> Some maintainers reject literal -> #define changes like
> 1 -> true and 0 -> false for booleans.
>
> Some of those maintainers are IMO misguided.

Sorry, I just meant that you said that the section should list the kinds
of changes that are acceptable.  But it seems more feasible to say what
kinds of changes are not acceptable.  Then one would need a standardized
language for describing what those unacceptable changes are.

julia

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Joe Perches]

On Wed, 2016-09-14 at 08:35 +0200, Julia Lawall wrote:
> On Tue, 13 Sep 2016, Joe Perches wrote:
> > On Wed, 2016-09-14 at 07:57 +0200, Julia Lawall wrote:
> > > What types of changes are unacceptable?
> >
> > It's a mixed bag.
> >
> > Some maintainers reject all "style/whitespace changes".
> > Some maintainers reject global consistency patches like
> > int -> bool conversions.
> > Some maintainers reject literal -> #define changes like
> > 1 -> true and 0 -> false for booleans.
> >
> > Some of those maintainers are IMO misguided.
> Sorry, I just meant that you said that the section should list the kinds
> of changes that are acceptable.  But it seems more feasible to say what
> kinds of changes are not acceptable.  Then one would need a standardized
> language for describing what those unacceptable changes are.
> julia
> 

<shrug>

Is there really a difference in whatever grammar would be
necessary between what's acceptable and what's not acceptable?

It's still a classification problem.

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Greg KH]

On Tue, Sep 13, 2016 at 07:24:22PM -0700, Joe Perches wrote:
> On Tue, 2016-09-13 at 19:03 -0700, Josh Triplett wrote:
> > "Do
> > not mass-reformat existing code, even if it doesn't follow these
> > guidelines; doing so creates noise in version control history and makes
> > patches fail to apply."
> 
> It also could be useful to somehow separate newer, more actively
> developed code from the old stuff that should remain more static.
> 
> Old code should really only get code improvements like API changes,
> size reductions, constifications, performance improvements and bug
> fixes.
> 
> Maybe change the MAINTAINERS sections S: entries to have more
> comprehensive descriptions of what types of changes are acceptable
> for each section.
> 
> Right now it's
> 
> S:	Supported
> or
> S:	Maintained
> 
> Or maybe add something like a new entry for what types of changes
> are acceptable with a default of "none"
> 
> C:	Whitespace and Style	

Ick, no, we have way too many things in the MAINTAINERS file as it is...

thanks,

greg k-h

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Joe Perches]

On Wed, 2016-09-14 at 13:54 +0200, Greg KH wrote:
> On Tue, Sep 13, 2016 at 07:24:22PM -0700, Joe Perches wrote:
> > On Tue, 2016-09-13 at 19:03 -0700, Josh Triplett wrote:
> > > "Do
> > > not mass-reformat existing code, even if it doesn't follow these
> > > guidelines; doing so creates noise in version control history and makes
> > > patches fail to apply."
> > Or maybe add something like a new entry for what types of changes
> > are acceptable with a default of "none"
> > C:	Whitespace and Style
> Ick, no, we have way too many things in the MAINTAINERS file as it is...

So what would use propose instead?

I think the primary issue is people using "scripts/checkpatch.pl -f"

I think that shouldn't be done without an understanding of when
it is useful and when it is not useful to use that -f option.

I have proposed adding an undocumented --force option to checkpatch
which would disallow -f unless --force is also used.

https://lkml.org/lkml/2015/2/11/433

Does anyone object to this?

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Greg KH]

On Wed, Sep 14, 2016 at 07:23:48AM -0700, Joe Perches wrote:
> On Wed, 2016-09-14 at 13:54 +0200, Greg KH wrote:
> > On Tue, Sep 13, 2016 at 07:24:22PM -0700, Joe Perches wrote:
> > > On Tue, 2016-09-13 at 19:03 -0700, Josh Triplett wrote:
> > > > "Do
> > > > not mass-reformat existing code, even if it doesn't follow these
> > > > guidelines; doing so creates noise in version control history and makes
> > > > patches fail to apply."
> > > Or maybe add something like a new entry for what types of changes
> > > are acceptable with a default of "none"
> > > C:	Whitespace and Style
> > Ick, no, we have way too many things in the MAINTAINERS file as it is...
> 
> So what would use propose instead?
> 
> I think the primary issue is people using "scripts/checkpatch.pl -f"
> 
> I think that shouldn't be done without an understanding of when
> it is useful and when it is not useful to use that -f option.

I agree, people get annoyed by this.  I personally think that anyone who
does get annoyed by it should just ignore them, or fix up the code to
not get triggered by the reports.

But who am I to complain :)

> I have proposed adding an undocumented --force option to checkpatch
> which would disallow -f unless --force is also used.
> 
> https://lkml.org/lkml/2015/2/11/433
> 
> Does anyone object to this?

None from me.

thanks,

greg k-h

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Julia Lawall]

On Wed, 14 Sep 2016, Greg KH wrote:

> On Wed, Sep 14, 2016 at 07:23:48AM -0700, Joe Perches wrote:
> > On Wed, 2016-09-14 at 13:54 +0200, Greg KH wrote:
> > > On Tue, Sep 13, 2016 at 07:24:22PM -0700, Joe Perches wrote:
> > > > On Tue, 2016-09-13 at 19:03 -0700, Josh Triplett wrote:
> > > > > "Do
> > > > > not mass-reformat existing code, even if it doesn't follow these
> > > > > guidelines; doing so creates noise in version control history and makes
> > > > > patches fail to apply."
> > > > Or maybe add something like a new entry for what types of changes
> > > > are acceptable with a default of "none"
> > > > C:	Whitespace and Style
> > > Ick, no, we have way too many things in the MAINTAINERS file as it is...
> >
> > So what would use propose instead?
> >
> > I think the primary issue is people using "scripts/checkpatch.pl -f"
> >
> > I think that shouldn't be done without an understanding of when
> > it is useful and when it is not useful to use that -f option.
>
> I agree, people get annoyed by this.  I personally think that anyone who
> does get annoyed by it should just ignore them, or fix up the code to
> not get triggered by the reports.
>
> But who am I to complain :)
>
> > I have proposed adding an undocumented --force option to checkpatch
> > which would disallow -f unless --force is also used.
> >
> > https://lkml.org/lkml/2015/2/11/433
> >
> > Does anyone object to this?

The --force is only required for non-staging code?  If it is required for
staging code, then we will have to document it in the outreachy tutorial,
and then all the (non-outreachy) newbies who look at the tutorial will
know about it...

julia

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Theodore Ts'o]

On Wed, Sep 14, 2016 at 04:35:06PM +0200, Julia Lawall wrote:
> 
> The --force is only required for non-staging code?  If it is required for
> staging code, then we will have to document it in the outreachy tutorial,
> and then all the (non-outreachy) newbies who look at the tutorial will
> know about it...

The whole point of the option is to try to discourage people from
sending white-space (or only style-only) patches.  So I'm not so sure
that it would be such a tragedy if people like (for example) Markus
Elfing or Nick Krause don't find out about the --force option right
away.  Given that outreachy folks are encouraged to work on staging
code anyway, would it make a difference to them?

     	     	      	     		- Ted

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Guenter Roeck]

On 09/14/2016 07:23 AM, Joe Perches wrote:
> On Wed, 2016-09-14 at 13:54 +0200, Greg KH wrote:
>> On Tue, Sep 13, 2016 at 07:24:22PM -0700, Joe Perches wrote:
>>> On Tue, 2016-09-13 at 19:03 -0700, Josh Triplett wrote:
>>>> "Do
>>>> not mass-reformat existing code, even if it doesn't follow these
>>>> guidelines; doing so creates noise in version control history and makes
>>>> patches fail to apply."
>>> Or maybe add something like a new entry for what types of changes
>>> are acceptable with a default of "none"
>>> C:	Whitespace and Style
>> Ick, no, we have way too many things in the MAINTAINERS file as it is...
>
> So what would use propose instead?
>

Introduce .checkpatch ?

Guenter

> I think the primary issue is people using "scripts/checkpatch.pl -f"
>
> I think that shouldn't be done without an understanding of when
> it is useful and when it is not useful to use that -f option.
>
> I have proposed adding an undocumented --force option to checkpatch
> which would disallow -f unless --force is also used.
>
> https://lkml.org/lkml/2015/2/11/433
>
> Does anyone object to this?
> _______________________________________________
> Ksummit-discuss mailing list
> Ksummit-discuss@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
>

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Joe Perches]

On Wed, 2016-09-14 at 16:32 +0200, Greg KH wrote:
> On Wed, Sep 14, 2016 at 07:23:48AM -0700, Joe Perches wrote:
> > I think the primary issue is people using "scripts/checkpatch.pl -f"
> > I think that shouldn't be done without an understanding of when
> > it is useful and when it is not useful to use that -f option.
> I agree, people get annoyed by this.  I personally think that anyone who
> does get annoyed by it should just ignore them, or fix up the code to
> not get triggered by the reports.
> 
> But who am I to complain :)

Sure, but there really are old and crufty drivers that
shouldn't ever be touched as the hardware is obsolete
and churning that stuff really is almost pointless,
prone to defect insertion, and the result is untested.

Marking those drivers as obsolete or completed in
MAINTAINERS might help.

And there are maintainers that shall remain nameless
that think their code is especially good 'as-is' and
don't want it dusted off as code with cobwebs isn't
worth the bother cleaning to them.

> > I have proposed adding an undocumented --force option to checkpatch
> > which would disallow -f unless --force is also used.
> > https://lkml.org/lkml/2015/2/11/433
> > Does anyone object to this?
> None from me.

Going once...

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Joe Perches]

On Wed, 2016-09-14 at 16:35 +0200, Julia Lawall wrote:

> The --force is only required for non-staging code?  If it is required for
> staging code, then we will have to document it in the outreachy tutorial,
> and then all the (non-outreachy) newbies who look at the tutorial will
> know about it.

Correct.

drivers/staging gets a special exemption to avoid using --force

+	if (!$force && $file && $filename !~ m@^drivers/staging/@) {
+		warn "$P: checking '$filename' is not supported\n";
+		next;
+	}

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Joe Perches]

On Wed, 2016-09-14 at 07:45 -0700, Guenter Roeck wrote:
> On 09/14/2016 07:23 AM, Joe Perches wrote:
> > So what would use propose instead?
> Introduce .checkpatch ?

A generic .checkpatch.conf option already exists which is
looked for in . then $HOME

Do you mean a directory based hierarchy of .checkpatch.conf
files in the git tree so a maintainer could add something like:

$ cat include/trace/events/.checkpatch.conf
--ignore=spacing

and that applies only to that directory?

That might get confusing and have conflicting options
on the command line.

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Alexandre Belloni]

On 13/09/2016 at 23:27:12 -0700, Joe Perches wrote :
> On Wed, 2016-09-14 at 07:57 +0200, Julia Lawall wrote:
> > What types of changes are unacceptable?
> 
> It's a mixed bag.
> 
> Some maintainers reject all "style/whitespace changes".
> Some maintainers reject global consistency patches like
> int -> bool conversions.
> Some maintainers reject literal -> #define changes like
> 1 -> true and 0 -> false for booleans.
> 
> Some of those maintainers are IMO misguided.

On my side, I usually take that kind of changes only when they come with
other substantial changes, especially when the code hasn't been touched
for a while.

The other thing that I find annoying are people using Coccinnelle or any
other static code analysis tool and sending patches without saying how
they found the alleged bug. Sometimes, this results in pointless
cleanups that haven't been tested by the patch author.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Eric W. Biederman]

"Rafael J. Wysocki" <rjw@rjwysocki.net> writes:

> On Tuesday, September 13, 2016 02:03:22 PM Jonathan Corbet wrote:
>> On Tue, 13 Sep 2016 12:45:20 -0700
>> Josh Triplett <josh@joshtriplett.org> wrote:
>> 
>> > On Tue, Sep 13, 2016 at 08:58:49PM +0200, Christian Borntraeger wrote:
>> > > 3. CodingStyle seems to get changes which have no ACK or Reviewed-by that seem
>> > > to be controversial.  e.g.
>> > > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/Documentation/CodingStyle?id=865a1caa4b6b886babdd9d67e7c3608be4567a51
>> > > suggested to indent labels with a space, and was then immediately followed by
>> > > patches. Is there a process in place to verify and challenge such changes?  
>> > 
>> > Ideally, that should come up during review of the CodingStyle patch.
>> > Changes shouldn't go into CodingStyle except to document existing
>> > process and unwritten rules, or to document the results of a discussion
>> > and consensus.  That particular change to CodingStyle should have been
>> > rejected, and should be reverted.
>> 
>> So I'm quite reluctant to take CodingStyle patches for just this reason;
>> *I* certainly don't want to be the one dictating style for the kernel, but
>> I'm not really sure who does.  In my time as the docs maintainer I've only
>> applied two patches there that constitute any sort of rule change - this
>> one and a78a136fa9337fdc25fdbaa2d253f9b4dc90ad44.  
>> 
>> In general, I would welcome advice on how any future rule-change patches
>> should be reviewed.
>
> I agree with Josh that CodingStyle should reflect the existing practice
> (present in the majority of the kernel source) or a broad consensus.
>
> While the "existing practice" case is relatively simple (it boils down to
> demonstrating that the given rule is in fact followed in practice in the
> majority of the kernel source), the "broad consensus" one is not as
> straightforward.  It looks like a Kernel Summit discussion or equivalent
> would be required each time to be honest ...
>
> In any case, it might be good to state somewhere that CodingStyle is a
> guidance for new code and not a prescription for how all of the kernel code
> must look like.

As I recall CodingStyle started life as a minimal codification of how
Linus wants the code, and initially he deliberately kept CodingStyle
quite minimal.

CodingStyle as a minimal least common denominator thing seems
reasonable.  CodingStyle that an experienced developer can't infer from
reading the kernel code seems like a bad idea.  Some people like to
read, some people like to cut & paste code.  Different people learn in
different ways.  Not to mention that different subsystems have their
idiosyncrasies.

For code that evolves slowly whitespace or coding ``style'' cleanups
that come up their own can be painful.  Occasionally there are good
reasons why a piece of code does not strictly conform to some rule.
Tables etc.  I have seen style patches mangle the readability of code.

Furthermore whitespace and coding sytle clean ups can be very painful if
you are doing a backports and a @#$%^&*! unnecessary cleanup breaks the
backport.

Certainly mandating things like having a one space indent on labels are
the kind of thing that are likely to cause people to see that CodeStyle
makes no sense and ignore it completely, rather than point new kernel
developers at it.

Eric

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Julia Lawall]

On Wed, 14 Sep 2016, Theodore Ts'o wrote:

> On Wed, Sep 14, 2016 at 04:35:06PM +0200, Julia Lawall wrote:
> >
> > The --force is only required for non-staging code?  If it is required for
> > staging code, then we will have to document it in the outreachy tutorial,
> > and then all the (non-outreachy) newbies who look at the tutorial will
> > know about it...
>
> The whole point of the option is to try to discourage people from
> sending white-space (or only style-only) patches.  So I'm not so sure
> that it would be such a tragedy if people like (for example) Markus
> Elfing or Nick Krause don't find out about the --force option right
> away.  Given that outreachy folks are encouraged to work on staging
> code anyway, would it make a difference to them?

People other than outreachy applicants may look at the outreachy tutorial.
It is even written that way in some cases.  I guess that checkpatch could
detect whether -f is used on a staging driver and not require --force if
that is the case.  This would have the other desirable benefit of reducing
the chance that outreachy applicants will send patches on non-staging
code.

julia

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 617 bytes --]



On Wed, 14 Sep 2016, Joe Perches wrote:

> On Wed, 2016-09-14 at 16:35 +0200, Julia Lawall wrote:
>
> > The --force is only required for non-staging code?  If it is required for
> > staging code, then we will have to document it in the outreachy tutorial,
> > and then all the (non-outreachy) newbies who look at the tutorial will
> > know about it.
>
> Correct.
>
> drivers/staging gets a special exemption to avoid using --force

OK, seems quite perfect to me.

julia

>
> +	if (!$force && $file && $filename !~ m@^drivers/staging/@) {
> +		warn "$P: checking '$filename' is not supported\n";
> +		next;
> +	}
>
>

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Guenter Roeck]

On Wed, Sep 14, 2016 at 08:13:17AM -0700, Joe Perches wrote:
> On Wed, 2016-09-14 at 07:45 -0700, Guenter Roeck wrote:
> > On 09/14/2016 07:23 AM, Joe Perches wrote:
> > > So what would use propose instead?
> > Introduce .checkpatch ?
> 
> A generic .checkpatch.conf option already exists which is
> looked for in . then $HOME
> 
That isn't kept in the repository as far as I can see.

> Do you mean a directory based hierarchy of .checkpatch.conf
> files in the git tree so a maintainer could add something like:
> 
> $ cat include/trace/events/.checkpatch.conf
> --ignore=spacing
> 
> and that applies only to that directory?
> 
That might solve the problem. However,

> That might get confusing and have conflicting options
> on the command line.

... it currently has a different scope: It is expected to be used in a
local setting, not delivered as part of the repository. So it would not
help to declare that per-directory settings override global settings
since people expect the global settings to work.

But, yes, something like that might help.

Guenter

Re: [Ksummit-discuss] [CORE TOPIC] checkpatch/Codingstyle and trivial patch spam

[Jonathan Cameron]

On 14 September 2016 18:11:07 BST, Alexandre Belloni <alexandre.belloni@free-electrons.com> wrote:
>On 13/09/2016 at 23:27:12 -0700, Joe Perches wrote :
>> On Wed, 2016-09-14 at 07:57 +0200, Julia Lawall wrote:
>> > What types of changes are unacceptable?
>> 
>> It's a mixed bag.
>> 
>> Some maintainers reject all "style/whitespace changes".
>> Some maintainers reject global consistency patches like
>> int -> bool conversions.
>> Some maintainers reject literal -> #define changes like
>> 1 -> true and 0 -> false for booleans.
>> 
>> Some of those maintainers are IMO misguided.
>
>On my side, I usually take that kind of changes only when they come
>with
>other substantial changes, especially when the code hasn't been touched
>for a while.
>
>The other thing that I find annoying are people using Coccinnelle or
>any
>other static code analysis tool and sending patches without saying how
>they found the alleged bug. Sometimes, this results in pointless
>cleanups that haven't been tested by the patch author.

As a maintainer who gets a lot of patches from newbies (partly as I maintain a fair
set of staging patches but also as we have a steady stream of new drivers in
IIO) I am quite happy to take 'good' whitespace cleanups.

Most of the time I can find something deeper to point out in the driver and get
them moving up the food chain.

Personally I think it is worth the pain in recent driver code at least as the
 advantage of pulling someone new in outweighs the costs.

Probably helps that IIO has a fair sized group of reviewers many of whom help
out with advice to newbies.  Hence I have it easy :)

Just thought I'd add a positive viewpoint.

Jonathan
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.