Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Dan Carpenter]

On Wed, Sep 11, 2019 at 08:48:59AM -0700, Dan Williams wrote:
> +Coding Style Addendum
> +---------------------
> +libnvdimm expects multi-line statements to be double indented. I.e.
> +
> +        if (x...
> +                        && ...y) {

That looks horrible and it causes a checkpatch warning.  :(  Why not
do it the same way that everyone else does it.

	if (blah_blah_x && <-- && has to be on the first line for checkpatch
	    blah_blah_y) { <-- [tab][space][space][space][space]blah

Now all the conditions are aligned visually which makes it readable.
They aren't aligned with the indent block so it's easy to tell the
inside from the if condition.

I kind of hate all this extra documentation because now everyone thinks
they can invent new hoops to jump through.

Speaking of hoops, the BPF documentation says that you have to figure
out which tree a patch applies to instead of just working against
linux-next.  Is there an automated way to do that?  I looked through my
inbox and there are a bunch of patches marked as going through the
bpf-next tree but about half were marked incorrectly so it looks like
everyone who tries to tag their patches is going to do it badly anyway.

regards,
dan carpenter

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Jens Axboe]

On 9/11/19 12:43 PM, Dan Carpenter wrote:
> On Wed, Sep 11, 2019 at 08:48:59AM -0700, Dan Williams wrote:
>> +Coding Style Addendum
>> +---------------------
>> +libnvdimm expects multi-line statements to be double indented. I.e.
>> +
>> +        if (x...
>> +                        && ...y) {
> 
> That looks horrible and it causes a checkpatch warning.  :(  Why not
> do it the same way that everyone else does it.
> 
> 	if (blah_blah_x && <-- && has to be on the first line for checkpatch
> 	    blah_blah_y) { <-- [tab][space][space][space][space]blah
> 
> Now all the conditions are aligned visually which makes it readable.
> They aren't aligned with the indent block so it's easy to tell the
> inside from the if condition.
> 
> I kind of hate all this extra documentation because now everyone thinks
> they can invent new hoops to jump through.

FWIW, I completely agree with Dan (Carpenter) here. I absolutely
dislike having these kinds of files, and with subsystems imposing weird
restrictions on style (like the quoted example, yuck).

Additionally, it would seem saner to standardize rules around when
code is expected to hit the maintainers hands for kernel releases. Both
yours and Martins deals with that, there really shouldn't be the need
to have this specified in detail per sub-system.

-- 
Jens Axboe

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Dan Williams]

On Wed, Sep 11, 2019 at 3:11 PM Jens Axboe <axboe@kernel.dk> wrote:
>
> On 9/11/19 12:43 PM, Dan Carpenter wrote:
> > On Wed, Sep 11, 2019 at 08:48:59AM -0700, Dan Williams wrote:
> >> +Coding Style Addendum
> >> +---------------------
> >> +libnvdimm expects multi-line statements to be double indented. I.e.
> >> +
> >> +        if (x...
> >> +                        && ...y) {
> >
> > That looks horrible and it causes a checkpatch warning.  :(  Why not
> > do it the same way that everyone else does it.
> >
> >       if (blah_blah_x && <-- && has to be on the first line for checkpatch
> >           blah_blah_y) { <-- [tab][space][space][space][space]blah
> >
> > Now all the conditions are aligned visually which makes it readable.
> > They aren't aligned with the indent block so it's easy to tell the
> > inside from the if condition.
> >
> > I kind of hate all this extra documentation because now everyone thinks
> > they can invent new hoops to jump through.
>
> FWIW, I completely agree with Dan (Carpenter) here. I absolutely
> dislike having these kinds of files, and with subsystems imposing weird
> restrictions on style (like the quoted example, yuck).
>
> Additionally, it would seem saner to standardize rules around when
> code is expected to hit the maintainers hands for kernel releases. Both
> yours and Martins deals with that, there really shouldn't be the need
> to have this specified in detail per sub-system.

So this is *the* point of the exercise.

I picked up this indentation habit a long while back in response to
review feedback on a patch to a subsystem that formatted code this
way. At the time CodingStyle did not contradict the maintainer's
preference, so I adopted it across the board.

Now I come to find that CodingStyle has settled on clang-format (in
the last 15 months) as the new standard which is a much better answer
to me than a manually specified style open to interpretation. I'll
take a look at getting libnvdimm converted over.

If we can take that further and standardize all of the places where
contributors see variations across subsystems on the fundamental
questions of style, timing, follow-up, and unit test invocation the
Maintainer Entry Profile can be superseded with common guidelines.

Otherwise, yes I completely agree with you that a "Maintainer Entry
Profile" is a sad comment on the state of what contributors need to
navigate, but that's today's reality that needs to be addressed.

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Miguel Ojeda]

On Thu, Sep 12, 2019 at 9:43 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> Now I come to find that CodingStyle has settled on clang-format (in
> the last 15 months) as the new standard which is a much better answer
> to me than a manually specified style open to interpretation. I'll
> take a look at getting libnvdimm converted over.

Note that clang-format cannot do everything as we want within the
kernel just yet, but it is a close enough approximation -- it is near
the point where we could simply agree to use it and stop worrying
about styling issues. However, that would mean everyone needs to have
a recent clang-format available, which I think is the biggest obstacle
at the moment.

Cheers,
Miguel

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Joe Perches]

On Thu, 2019-09-12 at 10:24 +0200, Miguel Ojeda wrote:
> On Thu, Sep 12, 2019 at 9:43 AM Dan Williams <dan.j.williams@intel.com> wrote:
> > Now I come to find that CodingStyle has settled on clang-format (in
> > the last 15 months) as the new standard which is a much better answer
> > to me than a manually specified style open to interpretation. I'll
> > take a look at getting libnvdimm converted over.
> 
> Note that clang-format cannot do everything as we want within the
> kernel just yet, but it is a close enough approximation -- it is near
> the point where we could simply agree to use it and stop worrying
> about styling issues. However, that would mean everyone needs to have
> a recent clang-format available, which I think is the biggest obstacle
> at the moment.

I don't think that's close to true yet for clang-format.

For instance: clang-format does not do anything with
missing braces, or coalescing multi-part strings,
or any number of other nominal coding style defects
like all the for_each macros, aligning or not aligning
columnar contents appropriately, etc...

clang-format as yet has no taste.

I believe it'll take a lot of work to improve it to a point
where its formatting is acceptable and appropriate.

An AI rather than a table based system like clang-format is
more likely to be a real solution, but training that AI
isn't a thing that I want to do.

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Miguel Ojeda]

On Thu, Sep 12, 2019 at 12:18 PM Joe Perches <joe@perches.com> wrote:
>
> I don't think that's close to true yet for clang-format.

I don't expect clang-format to match perfectly our current code style.

However, if core maintainers agree that it is "close enough now"
(specially with newer LLVMs, like 9), then there is a great benefit on
moving to automatically-styled code. The "con" is having to change a
bit our style wherever clang-format does not support exactly our
current style.

> For instance: clang-format does not do anything with
> missing braces, or coalescing multi-part strings,
> or any number of other nominal coding style defects
> like all the for_each macros, aligning or not aligning
> columnar contents appropriately, etc...

Some of these may or may not be fixable tweaking the options. Note
that there are conflicting styles within the kernel at the moment,
e.g. how to indent arguments to function calls. Therefore, some of the
differences do not apply as soon as we decide on a given style.

Furthermore, with automatic formatting we have also the chance to
review some options that we couldn't easily change before.

> clang-format as yet has no taste.
>
> I believe it'll take a lot of work to improve it to a point
> where its formatting is acceptable and appropriate.
>
> An AI rather than a table based system like clang-format is
> more likely to be a real solution, but training that AI
> isn't a thing that I want to do.

I don't think we need taste (or AI-like solutions), because
consistency has a lot of value too. Not just for our brains, but for
patches as well.

Note that clang-format is a tool used by major projects successfully,
it is not like we are experimenting too much here :-)

Cheers,
Miguel

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Jonathan Corbet]

On Wed, 11 Sep 2019 16:11:29 -0600
Jens Axboe <axboe@kernel.dk> wrote:

> On 9/11/19 12:43 PM, Dan Carpenter wrote:
> > 
> > I kind of hate all this extra documentation because now everyone thinks
> > they can invent new hoops to jump through.  
> 
> FWIW, I completely agree with Dan (Carpenter) here. I absolutely
> dislike having these kinds of files, and with subsystems imposing weird
> restrictions on style (like the quoted example, yuck).
> 
> Additionally, it would seem saner to standardize rules around when
> code is expected to hit the maintainers hands for kernel releases. Both
> yours and Martins deals with that, there really shouldn't be the need
> to have this specified in detail per sub-system.

This sort of objection came up at the maintainers summit yesterday; the
consensus was that, while we might not like subsystem-specific rules, they
do currently exist and we're just documenting reality.  To paraphrase
Phillip K. Dick, reality is that which, when you refuse to document it,
doesn't go away.

So I'm expecting to take this kind of stuff into Documentation/.  My own
personal hope is that it can maybe serve to shame some of these "local
quirks" out of existence.  The evidence from this brief discussion suggests
that this might indeed happen.

Thanks,

jon

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Dan Carpenter]

On Fri, Sep 13, 2019 at 01:09:37AM -0600, Jonathan Corbet wrote:
> On Wed, 11 Sep 2019 16:11:29 -0600
> Jens Axboe <axboe@kernel.dk> wrote:
> 
> > On 9/11/19 12:43 PM, Dan Carpenter wrote:
> > > 
> > > I kind of hate all this extra documentation because now everyone thinks
> > > they can invent new hoops to jump through.
> > 
> > FWIW, I completely agree with Dan (Carpenter) here. I absolutely
> > dislike having these kinds of files, and with subsystems imposing weird
> > restrictions on style (like the quoted example, yuck).
> > 
> > Additionally, it would seem saner to standardize rules around when
> > code is expected to hit the maintainers hands for kernel releases. Both
> > yours and Martins deals with that, there really shouldn't be the need
> > to have this specified in detail per sub-system.
> 
> This sort of objection came up at the maintainers summit yesterday; the
> consensus was that, while we might not like subsystem-specific rules, they
> do currently exist and we're just documenting reality.  To paraphrase
> Phillip K. Dick, reality is that which, when you refuse to document it,
> doesn't go away.

There aren't that many subsystem rules.  The big exception is
networking, with the comment style and reverse Chrismas tree
declarations.  Also you have to label which git tree the patch applies
to like [net] or [net-next].

It used to be that infiniband used "sizeof foo" instead of sizeof(foo)
but now there is a new maintainer.

There is one subsystem which where the maintainer will capitalize your
patch prefix and complain.  There are others where they will silently
change it to lower case.  (Maybe that has changed in recent years).

There is one subsystem where the maintainer is super strict rules that
you can't use "I" or "we" in the commit message.  So you can't say "I
noticed a bug while reviewing", you have to say "The code has a bug".

Some maintainers have rules about what you can put in the declaration
block.  No kmalloc() in the declarations is a common rule.
"struct foo *p = kmalloc();".

Some people (I do) have strict rules for error handling, but most won't
complain unless the error handling has bugs.

The bpf people want you to put [bpf] or [bpf-next] in the subject.
Everyone just guesses, and uneducated guesses are worse than leaving it
blank, but that's just my opinion.

> So I'm expecting to take this kind of stuff into Documentation/.  My own
> personal hope is that it can maybe serve to shame some of these "local
> quirks" out of existence.  The evidence from this brief discussion suggests
> that this might indeed happen.

I don't think it's shaming, I think it's validating.  Everyone just
insists that since it's written in the Book of Rules then it's our fault
for not reading it.  It's like those EULA things where there is more
text than anyone can physically read in a life time.

And the documentation doesn't help.  For example, I knew people's rules
about capitalizing the subject but I'd just forget.  I say that if you
can't be bothered to add it to checkpatch then it means you don't really
care that strongly.

regards,
dan carpenter

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Dan Williams]

On Fri, Sep 13, 2019 at 4:49 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> On Fri, Sep 13, 2019 at 01:09:37AM -0600, Jonathan Corbet wrote:
> > On Wed, 11 Sep 2019 16:11:29 -0600
> > Jens Axboe <axboe@kernel.dk> wrote:
> >
> > > On 9/11/19 12:43 PM, Dan Carpenter wrote:
> > > >
> > > > I kind of hate all this extra documentation because now everyone thinks
> > > > they can invent new hoops to jump through.
> > >
> > > FWIW, I completely agree with Dan (Carpenter) here. I absolutely
> > > dislike having these kinds of files, and with subsystems imposing weird
> > > restrictions on style (like the quoted example, yuck).
> > >
> > > Additionally, it would seem saner to standardize rules around when
> > > code is expected to hit the maintainers hands for kernel releases. Both
> > > yours and Martins deals with that, there really shouldn't be the need
> > > to have this specified in detail per sub-system.
> >
> > This sort of objection came up at the maintainers summit yesterday; the
> > consensus was that, while we might not like subsystem-specific rules, they
> > do currently exist and we're just documenting reality.  To paraphrase
> > Phillip K. Dick, reality is that which, when you refuse to document it,
> > doesn't go away.
>
> There aren't that many subsystem rules.  The big exception is
> networking, with the comment style and reverse Chrismas tree
> declarations.  Also you have to label which git tree the patch applies
> to like [net] or [net-next].
>
> It used to be that infiniband used "sizeof foo" instead of sizeof(foo)
> but now there is a new maintainer.
>
> There is one subsystem which where the maintainer will capitalize your
> patch prefix and complain.  There are others where they will silently
> change it to lower case.  (Maybe that has changed in recent years).
>
> There is one subsystem where the maintainer is super strict rules that
> you can't use "I" or "we" in the commit message.  So you can't say "I
> noticed a bug while reviewing", you have to say "The code has a bug".
>
> Some maintainers have rules about what you can put in the declaration
> block.  No kmalloc() in the declarations is a common rule.
> "struct foo *p = kmalloc();".
>
> Some people (I do) have strict rules for error handling, but most won't
> complain unless the error handling has bugs.
>
> The bpf people want you to put [bpf] or [bpf-next] in the subject.
> Everyone just guesses, and uneducated guesses are worse than leaving it
> blank, but that's just my opinion.
>
> > So I'm expecting to take this kind of stuff into Documentation/.  My own
> > personal hope is that it can maybe serve to shame some of these "local
> > quirks" out of existence.  The evidence from this brief discussion suggests
> > that this might indeed happen.
>
> I don't think it's shaming, I think it's validating.  Everyone just
> insists that since it's written in the Book of Rules then it's our fault
> for not reading it.  It's like those EULA things where there is more
> text than anyone can physically read in a life time.
>
> And the documentation doesn't help.  For example, I knew people's rules
> about capitalizing the subject but I'd just forget.  I say that if you
> can't be bothered to add it to checkpatch then it means you don't really
> care that strongly.

True, can someone with better perl skills than me take a shot at a
rule for checkpatch to catch the capitalization preference based on
the subsystem being touched, or otherwise agree that if a maintainer
has a changelog capitalization preference they just silently fix it up
at application time and not waste time pointing out something so
trivial? For example, I notice Linus likes "-" instead of "*" for
bullet lists in changelogs he just fixes it up silently if I forget.

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Randy Dunlap]

On 9/13/19 4:48 AM, Dan Carpenter wrote:

>> So I'm expecting to take this kind of stuff into Documentation/.  My own
>> personal hope is that it can maybe serve to shame some of these "local
>> quirks" out of existence.  The evidence from this brief discussion suggests
>> that this might indeed happen.
> 
> I don't think it's shaming, I think it's validating.  Everyone just
> insists that since it's written in the Book of Rules then it's our fault
> for not reading it.  It's like those EULA things where there is more
> text than anyone can physically read in a life time.

Yes, agreed.

> And the documentation doesn't help.  For example, I knew people's rules
> about capitalizing the subject but I'd just forget.  I say that if you
> can't be bothered to add it to checkpatch then it means you don't really
> care that strongly.

If a subsystem requires a certain spelling/capitalization in patch email
subjects, it should be added to MAINTAINERS IMO.  E.g.,
E:	NuBus

-- 
~Randy

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Rob Herring]

On Fri, Sep 13, 2019 at 4:00 PM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> On 9/13/19 4:48 AM, Dan Carpenter wrote:
>
> >> So I'm expecting to take this kind of stuff into Documentation/.  My own
> >> personal hope is that it can maybe serve to shame some of these "local
> >> quirks" out of existence.  The evidence from this brief discussion suggests
> >> that this might indeed happen.
> >
> > I don't think it's shaming, I think it's validating.  Everyone just
> > insists that since it's written in the Book of Rules then it's our fault
> > for not reading it.  It's like those EULA things where there is more
> > text than anyone can physically read in a life time.
>
> Yes, agreed.
>
> > And the documentation doesn't help.  For example, I knew people's rules
> > about capitalizing the subject but I'd just forget.  I say that if you
> > can't be bothered to add it to checkpatch then it means you don't really
> > care that strongly.
>
> If a subsystem requires a certain spelling/capitalization in patch email
> subjects, it should be added to MAINTAINERS IMO.  E.g.,
> E:      NuBus

+1

Better make this a regex to deal with (net|net-next).

We could probably script populating MAINTAINERS with this using how it
is done manually: git log --oneline <dir>

Rob

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Joe Perches]

On Fri, 2019-09-13 at 16:46 +0100, Rob Herring wrote:
> On Fri, Sep 13, 2019 at 4:00 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> > On 9/13/19 4:48 AM, Dan Carpenter wrote:
> > 
> > > > So I'm expecting to take this kind of stuff into Documentation/.  My own
> > > > personal hope is that it can maybe serve to shame some of these "local
> > > > quirks" out of existence.  The evidence from this brief discussion suggests
> > > > that this might indeed happen.
> > > 
> > > I don't think it's shaming, I think it's validating.  Everyone just
> > > insists that since it's written in the Book of Rules then it's our fault
> > > for not reading it.  It's like those EULA things where there is more
> > > text than anyone can physically read in a life time.
> > 
> > Yes, agreed.
> > 
> > > And the documentation doesn't help.  For example, I knew people's rules
> > > about capitalizing the subject but I'd just forget.  I say that if you
> > > can't be bothered to add it to checkpatch then it means you don't really
> > > care that strongly.
> > 
> > If a subsystem requires a certain spelling/capitalization in patch email
> > subjects, it should be added to MAINTAINERS IMO.  E.g.,
> > E:      NuBus
> 
> +1
> 
> Better make this a regex to deal with (net|net-next).
> 
> We could probably script populating MAINTAINERS with this using how it
> is done manually: git log --oneline <dir>

I made a similar proposal nearly a decade ago to add a grammar
to MAINTAINERS sections for patch subject prefixes.

https://lore.kernel.org/lkml/1289919077.28741.50.camel@Joe-Laptop/

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Rob Herring]

On Fri, Sep 13, 2019 at 11:42 AM Joe Perches <joe@perches.com> wrote:
>
> On Fri, 2019-09-13 at 16:46 +0100, Rob Herring wrote:
> > On Fri, Sep 13, 2019 at 4:00 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> > > On 9/13/19 4:48 AM, Dan Carpenter wrote:
> > >
> > > > > So I'm expecting to take this kind of stuff into Documentation/.  My own
> > > > > personal hope is that it can maybe serve to shame some of these "local
> > > > > quirks" out of existence.  The evidence from this brief discussion suggests
> > > > > that this might indeed happen.
> > > >
> > > > I don't think it's shaming, I think it's validating.  Everyone just
> > > > insists that since it's written in the Book of Rules then it's our fault
> > > > for not reading it.  It's like those EULA things where there is more
> > > > text than anyone can physically read in a life time.
> > >
> > > Yes, agreed.
> > >
> > > > And the documentation doesn't help.  For example, I knew people's rules
> > > > about capitalizing the subject but I'd just forget.  I say that if you
> > > > can't be bothered to add it to checkpatch then it means you don't really
> > > > care that strongly.
> > >
> > > If a subsystem requires a certain spelling/capitalization in patch email
> > > subjects, it should be added to MAINTAINERS IMO.  E.g.,
> > > E:      NuBus
> >
> > +1
> >
> > Better make this a regex to deal with (net|net-next).
> >
> > We could probably script populating MAINTAINERS with this using how it
> > is done manually: git log --oneline <dir>
>
> I made a similar proposal nearly a decade ago to add a grammar
> to MAINTAINERS sections for patch subject prefixes.
>
> https://lore.kernel.org/lkml/1289919077.28741.50.camel@Joe-Laptop/

Perhaps there's more support for it now. I didn't get through all the
thread, but the positions seemed to range from "who cares, subjects
are easy to edit" to "seems like a good idea and doesn't hurt". I
probably would have implemented something, but perl (tacking on to
checkpatch and having you tell me everything wrong is about all I can
do :)).

Rob

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Geert Uytterhoeven]

Hi Randy,

On Fri, Sep 13, 2019 at 5:00 PM Randy Dunlap <rdunlap@infradead.org> wrote:
> On 9/13/19 4:48 AM, Dan Carpenter wrote:
> >> So I'm expecting to take this kind of stuff into Documentation/.  My own
> >> personal hope is that it can maybe serve to shame some of these "local
> >> quirks" out of existence.  The evidence from this brief discussion suggests
> >> that this might indeed happen.
> >
> > I don't think it's shaming, I think it's validating.  Everyone just
> > insists that since it's written in the Book of Rules then it's our fault
> > for not reading it.  It's like those EULA things where there is more
> > text than anyone can physically read in a life time.
>
> Yes, agreed.
>
> > And the documentation doesn't help.  For example, I knew people's rules
> > about capitalizing the subject but I'd just forget.  I say that if you
> > can't be bothered to add it to checkpatch then it means you don't really
> > care that strongly.
>
> If a subsystem requires a certain spelling/capitalization in patch email
> subjects, it should be added to MAINTAINERS IMO.  E.g.,
> E:      NuBus

Oh, I understood the question differently.  I thought it was about
"sub: system: Fix foo" vs. "sub: system: fix foo".

For simple and trivial things, I tend to make changes while applying, as that's
usually less work than complaining, and verifying that it's been fixed in the
next (if any) version n days/weeks/months later.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Jani Nikula]

On Fri, 13 Sep 2019, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> And the documentation doesn't help.  For example, I knew people's rules
> about capitalizing the subject but I'd just forget.  I say that if you
> can't be bothered to add it to checkpatch then it means you don't really
> care that strongly.

It would be entirely possible to add the subsystem/driver specific
checkpatch rules to MAINTAINERS entries or directory specific config
files. As checkpatch options directly. For example

--strict --max-line-length=100 --ignore=BIT_MACRO,SPLIT_STRING,LONG_LINE_STRING,BOOL_MEMBER

or whatever.

SMOP. ;)

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Jason Gunthorpe]

On Fri, Sep 13, 2019 at 02:48:50PM +0300, Dan Carpenter wrote:

> It used to be that infiniband used "sizeof foo" instead of sizeof(foo)
> but now there is a new maintainer.

These days I run everything through checkpatch and generally don't
want to see much deviation from the 'normal' style, a few minor
clang-format quibbles and other check patch positives excluded.

This means when people touch lines they have to adjust minor things
like the odd 'sizeof foo' to make it conforming.

Like others there is a big historical mismatch and the best I hope for
is that new stuff follow the cannonical style. Trying to guess what
some appropriate mongral style is for each patch is just a waste of my
time.

I also hold drivers/infiniband as an example of why the column
alignment style is harmful. That has not aged well and is the cause of
a lot of ugly things.

> There is one subsystem where the maintainer is super strict rules that
> you can't use "I" or "we" in the commit message.  So you can't say "I
> noticed a bug while reviewing", you have to say "The code has a bug".

Ah, the imperative mood nitpick. This one is very exciting to explain
to non-native speakers. With many regular submitters I'm still at the
"I wish you would use proper grammer and sentence structure" phase..

These days I just end up copy editing most of the commit messages :(

> I don't think it's shaming, I think it's validating.  Everyone just
> insists that since it's written in the Book of Rules then it's our fault
> for not reading it.  It's like those EULA things where there is more
> text than anyone can physically read in a life time.

Yeah, I tend to agree.

The big special cases with high patch volumes (net being the classic
example) should remain special.

But everyone else is not special, and shouldn't act the same.

The work people like DanC do with static analysis is valuable, and we
should not be insisting that those contributors have to jump through a
thousand special hoops.

I have simply viewed it as the job of the maintainer to run the
process and deal with minor nit picks on the fly.

Maybe that is what we should be documenting? 

Jason

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Dan Williams]

On Tue, Sep 17, 2019 at 9:16 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Fri, Sep 13, 2019 at 02:48:50PM +0300, Dan Carpenter wrote:
>
> > It used to be that infiniband used "sizeof foo" instead of sizeof(foo)
> > but now there is a new maintainer.
>
> These days I run everything through checkpatch and generally don't
> want to see much deviation from the 'normal' style, a few minor
> clang-format quibbles and other check patch positives excluded.
>
> This means when people touch lines they have to adjust minor things
> like the odd 'sizeof foo' to make it conforming.
>
> Like others there is a big historical mismatch and the best I hope for
> is that new stuff follow the cannonical style. Trying to guess what
> some appropriate mongral style is for each patch is just a waste of my
> time.
>
> I also hold drivers/infiniband as an example of why the column
> alignment style is harmful. That has not aged well and is the cause of
> a lot of ugly things.
>
> > There is one subsystem where the maintainer is super strict rules that
> > you can't use "I" or "we" in the commit message.  So you can't say "I
> > noticed a bug while reviewing", you have to say "The code has a bug".
>
> Ah, the imperative mood nitpick. This one is very exciting to explain
> to non-native speakers. With many regular submitters I'm still at the
> "I wish you would use proper grammer and sentence structure" phase..
>
> These days I just end up copy editing most of the commit messages :(
>
> > I don't think it's shaming, I think it's validating.  Everyone just
> > insists that since it's written in the Book of Rules then it's our fault
> > for not reading it.  It's like those EULA things where there is more
> > text than anyone can physically read in a life time.
>
> Yeah, I tend to agree.
>
> The big special cases with high patch volumes (net being the classic
> example) should remain special.
>
> But everyone else is not special, and shouldn't act the same.
>
> The work people like DanC do with static analysis is valuable, and we
> should not be insisting that those contributors have to jump through a
> thousand special hoops.
>
> I have simply viewed it as the job of the maintainer to run the
> process and deal with minor nit picks on the fly.
>
> Maybe that is what we should be documenting?

In theory, yes, in practice, as long as there is an exception to the
rule, it comes down to a question of "is this case special like net or
not?". I'd rather not waste time debating that on a per-subsystem
basis vs just getting it all documented for contributors.

I do think it is worth clarifying in the guidelines of writing a
profile to make an effort to not be special, and that odd looking
rules will be questioned (like libnvdimm statement continuation), but
lets not fight the new standards fight until it becomes apparent where
the outliers lie.

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Martin K. Petersen]

Jens,

> Additionally, it would seem saner to standardize rules around when
> code is expected to hit the maintainers hands for kernel
> releases. Both yours and Martins deals with that, there really
> shouldn't be the need to have this specified in detail per sub-system.

Yeah. There is basically nothing specific about SCSI in my write-up
outside of the branch naming.

I deliberately didn't mention coding style preferences. We have so much
20+ year old cruft in SCSI that's impossible to even entertain. But I do
request new code to follow coding-style.rst. BYOXT.

Also note that the original target audience for my document. It was
aimed at onboarding new driver contributors from hardware companies. So
people that don't live and breathe Linux development and who are not
intimately familiar with the kernel development process. It's possible
that we have this information in Documentation/ these days; I'll go
look. But it didn't exist when this doc was written. And in my
experience nobody coming to Linux development from the outside
understands what the "merge window" is. And when the appropriate time is
to submit patches and features. I think this would be a great area to
have a common set of guidelines and documentation. I'd prefer for this
to be global and then let maintainers apply their own wiggle room
instead of documenting particular rules for a given subsystem.

One pet peeve I have is that people are pretty bad at indicating the
intended target tree. I often ask for it in private mail but the
practice doesn't seem to stick. I spend a ton of time guessing whether a
patch is a fix for a new feature in the x+1 queue or a fix for the
current release. It is not always obvious.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Dan Carpenter]

On Fri, Sep 13, 2019 at 05:44:39PM -0400, Martin K. Petersen wrote:
> One pet peeve I have is that people are pretty bad at indicating the
> intended target tree. I often ask for it in private mail but the
> practice doesn't seem to stick. I spend a ton of time guessing whether a
> patch is a fix for a new feature in the x+1 queue or a fix for the
> current release. It is not always obvious.

The Fixes tag doesn't help?

Of course, you've never asked me or anyone on kernel-newbies that
question.  We don't normally know the answer either.  I do always try to
figure it out for networking, but it's kind of a pain in the butt and I
mess up surpisingly often for how much effort I put into getting it
right.

regards,
dan carpenter

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Martin K. Petersen]

Dan,

>> One pet peeve I have is that people are pretty bad at indicating the
>> intended target tree. I often ask for it in private mail but the
>> practice doesn't seem to stick. I spend a ton of time guessing whether a
>> patch is a fix for a new feature in the x+1 queue or a fix for the
>> current release. It is not always obvious.
>
> The Fixes tag doesn't help?

Always.

> Of course, you've never asked me or anyone on kernel-newbies that
> question.  We don't normally know the answer either.  I do always try
> to figure it out for networking, but it's kind of a pain in the butt
> and I mess up surpisingly often for how much effort I put into getting
> it right.

It's not a big issue for your patches. These are inevitably fixes and
I'll pick an appropriate tree depending on where we are in the cycle,
how likely one is to hit the issue, whether the driver is widely used,
etc.

Vendor driver submissions, however, are generally huge. Sometimes 50+
patches per submission window. And during this window I often get on the
order of 10 to 20 patches for the same driver in the fixes tree. It is
not always easy to determine whether a bug fix series is for one tree or
the other.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]

On Mon, Sep 16, 2019 at 01:08:45PM -0400, Martin K. Petersen wrote:

> Vendor driver submissions, however, are generally huge. Sometimes 50+
> patches per submission window. And during this window I often get on the
> order of 10 to 20 patches for the same driver in the fixes tree. It is
> not always easy to determine whether a bug fix series is for one tree or
> the other.

I get the impression that a lot of vendors just don't distinguish and
only really care about getting things upstream, especially in the
embedded world many of them realistically expect to be shipping a pile
of out of tree patches to anyone using anything other than mainline
anyway so it's not super clear to them that it's worthwhile.  This goes
back to the converstations about stable and how vendors interact with
that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Aneesh Kumar K.V]

On 9/12/19 12:13 AM, Dan Carpenter wrote:
> On Wed, Sep 11, 2019 at 08:48:59AM -0700, Dan Williams wrote:
>> +Coding Style Addendum
>> +---------------------
>> +libnvdimm expects multi-line statements to be double indented. I.e.
>> +
>> +        if (x...
>> +                        && ...y) {
> 
> That looks horrible and it causes a checkpatch warning.  :(  Why not
> do it the same way that everyone else does it.
> 
> 	if (blah_blah_x && <-- && has to be on the first line for checkpatch
> 	    blah_blah_y) { <-- [tab][space][space][space][space]blah
> 
> Now all the conditions are aligned visually which makes it readable.
> They aren't aligned with the indent block so it's easy to tell the
> inside from the if condition.


I came across this while sending patches to libnvdimm subsystem. W.r.t 
coding Style can we have consistent styles across the kernel? Otherwise, 
one would have to change the editor settings as they work across 
different subsystems in the kernel. In this specific case both 
clang-format and emacs customization tip in the kernel documentation 
directory suggest the later style.

-aneesh

Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile

[Greg KH]

On Fri, Sep 13, 2019 at 07:41:55AM +0530, Aneesh Kumar K.V wrote:
> On 9/12/19 12:13 AM, Dan Carpenter wrote:
> > On Wed, Sep 11, 2019 at 08:48:59AM -0700, Dan Williams wrote:
> > > +Coding Style Addendum
> > > +---------------------
> > > +libnvdimm expects multi-line statements to be double indented. I.e.
> > > +
> > > +        if (x...
> > > +                        && ...y) {
> > 
> > That looks horrible and it causes a checkpatch warning.  :(  Why not
> > do it the same way that everyone else does it.
> > 
> > 	if (blah_blah_x && <-- && has to be on the first line for checkpatch
> > 	    blah_blah_y) { <-- [tab][space][space][space][space]blah
> > 
> > Now all the conditions are aligned visually which makes it readable.
> > They aren't aligned with the indent block so it's easy to tell the
> > inside from the if condition.
> 
> 
> I came across this while sending patches to libnvdimm subsystem. W.r.t
> coding Style can we have consistent styles across the kernel? Otherwise, one
> would have to change the editor settings as they work across different
> subsystems in the kernel. In this specific case both clang-format and emacs
> customization tip in the kernel documentation directory suggest the later
> style.

We _should_ have a consistent coding style across the whole kernel,
that's the whole reason for having a coding style in the first place!

The problem is, we all have agreed on the "basics" a long time ago, but
are now down in the tiny nits as to what some minor things should, or
should not, look like.

It might be time to just bite the bullet and do something like
"clang-format" to stop arguing about stuff like this for new
submissions, if for no other reason to keep us from wasting mental
energy on trivial things like this.

thanks,

greg k-h