[Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Wolfram Sang]

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


In the parts of the Kernel I work with, reviews are usually given by a plain
tag. I think this is not enough to keep a good code quality, so I'll start with
my theses first:

1) we need a better distinction between Acked-by: and Reviewed-by: and encourage
   stricter use of that

2) Reviewed-by should have a description of the review done (and the review not
   done)

3) trivial patches should rather get Acked-by

4) failing the above should be constructively criticized


Some more words about each item:

1) I am definitely not striving for a clear line, that's impossible. Yet, from
what I experience, the overlap between the two became large. It reduces the
extra value a Reviewed-by should have.

2) A short paragraph will usually do. Of course, trust helps a lot, but it
doesn't solve everything. Trusted people can be in a hurry, too, etc. And for
people I don't know, the plain tag doesn't tell me much. Examples for short
descriptions: "I can't say much about the media part, but the I2C part is
proper" or "I also checked the documentation and I think this is a good
approach to overcome the issue" or "All my concerns in the preceding
discussions have been addressed"

3) Again, no hard line on what is trivial can be made. Still, I think it will
add to the extra value of a review tag if it is only applied to something which
is non-trivial, so we should try to have a better distinction.

4) We are in such a need for people reviewing that it can be challenging for
maintainers to be picky about reviews (you can partly include me here). A
kernel-wide movement aiming for a better distinction between ack (= looks good)
and review helps both maintainers and developers, I think.

These things will hopefully help me as a maintainer to better evaluate trust
for a patch based on the tags given. So, I will try that in the I2C subsystem.
I would prefer, though, not to be an island but to have something which is
accepted kernel-wide.

Disclaimer: I am mainly active in the drivers section of Linux. If reviews are
handled differently in other parts, I am all ears.

Well, I am all ears, anyhow. Opinions?

Kind regards,

  Wolfram


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

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Leon Romanovsky]

On Sat, Jul 06, 2019 at 04:27:38PM +0200, Wolfram Sang wrote:
>
> In the parts of the Kernel I work with, reviews are usually given by a plain
> tag. I think this is not enough to keep a good code quality, so I'll start with
> my theses first:
>
> 1) we need a better distinction between Acked-by: and Reviewed-by: and encourage
>    stricter use of that

Agree

>
> 2) Reviewed-by should have a description of the review done (and the review not
>    done)

IMHO, this path of thinking will lead us to less reviews due to an extra
work and wouldn't bring an extra quality which we want.

Right now, everything is built on trust and it will continue to be after
we will demand to add extra sentence. It means that we don't fully trust
in Reviewed-by of one time contributors now and we won't trust in their
description of their Reviewed-by either.

Thanks

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Wolfram Sang]

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

Hi Leon,

> > 2) Reviewed-by should have a description of the review done (and the review not
> >    done)
> 
> IMHO, this path of thinking will lead us to less reviews due to an extra
> work and wouldn't bring an extra quality which we want.

I'd argue that this extra work is needed in the same way a good patch
description is needed.

> Right now, everything is built on trust and it will continue to be after
> we will demand to add extra sentence. It means that we don't fully trust
> in Reviewed-by of one time contributors now and we won't trust in their
> description of their Reviewed-by either.

Per default, I do trust a new contributor to have done the review. I
don't want this extra sentence as a proof of that.

The "problem" with a new reviewer is that I don't know if all aspects of
a patch have been reviewed or just a subset. Actually, this holds true
for people I do know just the same way. If a get a Rev-by from Linus
Walleij I am extremly sure the GPIO parts have been throughly checked.
But I still don't know if he had time to check e.g. the locking or not.
There is a huge difference if I get three plain Rev-by or three Rev-by
saying "I did check <xy> but not the media parts".

Thanks for your feedback. I think this clarification was important.

Regards,

   Wolfram


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

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Jan Kara]

Hi!

On Sat 06-07-19 19:17:24, Wolfram Sang wrote:
> > Right now, everything is built on trust and it will continue to be after
> > we will demand to add extra sentence. It means that we don't fully trust
> > in Reviewed-by of one time contributors now and we won't trust in their
> > description of their Reviewed-by either.
> 
> Per default, I do trust a new contributor to have done the review. I
> don't want this extra sentence as a proof of that.
> 
> The "problem" with a new reviewer is that I don't know if all aspects of
> a patch have been reviewed or just a subset. Actually, this holds true
> for people I do know just the same way. If a get a Rev-by from Linus
> Walleij I am extremly sure the GPIO parts have been throughly checked.
> But I still don't know if he had time to check e.g. the locking or not.
> There is a huge difference if I get three plain Rev-by or three Rev-by
> saying "I did check <xy> but not the media parts".

There are two things here: If I review a patch and I'm not confident I did
a good job for some parts (because I didn't have time or I just don't know
that part of the kernel), then I should write that to the reply with
Reviewed-by tag. That's IMHO a good rule but I don't think you can enforce
it in any way. You can just ask people that do reviews for your subsystem
if you think they're omitting this.

The second thing is that if human doesn't know something, then he/she has
a tendency to underestimate how much he/she doesn't know (this even has a
psychological term "cognitive bias"). So the self-evaluation of "how good is
my review" is always going to be subjective and it is upto maintainer to
judge what is the value of the review.

To give an exaple, Ted Tso (ext4 maintainer) tends to just ignore "empty
Reviewed-by" replies from people that haven't built enough credit in the
kernel community by actually finding bugs with their reviews...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Geert Uytterhoeven]

Hi Wolfram,

On Sat, Jul 6, 2019 at 4:32 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> In the parts of the Kernel I work with, reviews are usually given by a plain
> tag. I think this is not enough to keep a good code quality, so I'll start with
> my theses first:
>
> 1) we need a better distinction between Acked-by: and Reviewed-by: and encourage
>    stricter use of that

Before we had "Reviewed-by", "Acked-by" meant "looks OK to me".
Then we got "Reviewed-by" for more thorough reviews.

> 2) Reviewed-by should have a description of the review done (and the review not
>    done)
>
> 3) trivial patches should rather get Acked-by

These days when given by a maintainer, "Acked-by" means that the
maintainer is happy for the patch going in through another subsystem.

> Some more words about each item:

> 2) A short paragraph will usually do. Of course, trust helps a lot, but it
> doesn't solve everything. Trusted people can be in a hurry, too, etc. And for
> people I don't know, the plain tag doesn't tell me much. Examples for short
> descriptions: "I can't say much about the media part, but the I2C part is
> proper" or "I also checked the documentation and I think this is a good
> approach to overcome the issue" or "All my concerns in the preceding
> discussions have been addressed"

Definitely good to have, but hard to enforce, without making the process
heavier.

I have a fifth thesis: many people (incl. guilty me) browse quickly
through many patches flying by on mailing lists, but don't always go to
the effort of replying if they don't see something wrong immediately.
This means we don't catch a share of the reviews happening.

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] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Wolfram Sang]

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

Hi Jan,

> There are two things here: If I review a patch and I'm not confident I did
> a good job for some parts (because I didn't have time or I just don't know
> that part of the kernel), then I should write that to the reply with
> Reviewed-by tag. That's IMHO a good rule but I don't think you can enforce
> it in any way. You can just ask people that do reviews for your subsystem
> if you think they're omitting this.

I agree to this. This is why I intentionally wrote my theses with words
like "should" and "encourage" because I don't believe in "enforcing"
such a thing.

Nonetheless, having a clear statement worked well for commit messages, I
think. We have spread the word how important good commit messages are
and from what I observe they have become better. I wish for a similar
process with reviews. And from my side, it could be as simple as
"checked everything, all good".

> The second thing is that if human doesn't know something, then he/she has
> a tendency to underestimate how much he/she doesn't know (this even has a
> psychological term "cognitive bias"). So the self-evaluation of "how good is
> my review" is always going to be subjective and it is upto maintainer to
> judge what is the value of the review.

I still consider the mere description of what was reviewed in detail and
what not already helpful. I agree that the maintainer still has to
evaluate the review.

> To give an exaple, Ted Tso (ext4 maintainer) tends to just ignore "empty
> Reviewed-by" replies from people that haven't built enough credit in the
> kernel community by actually finding bugs with their reviews...

This is good to know. I will apply some rules for I2C. Yet, it feels
easier if I2C is not some obscure island but part of something.

Thanks,

   Wolfram


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

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Wolfram Sang]

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

Hi Geert,

> > 1) we need a better distinction between Acked-by: and Reviewed-by: and encourage
> >    stricter use of that
> 
> Before we had "Reviewed-by", "Acked-by" meant "looks OK to me".
> Then we got "Reviewed-by" for more thorough reviews.

This is what still makes most sense to me. You can express e.g. that you
like a patch series and approve the general approach taken but haven't
gone for the gory details -> Acked-by (a short explaining paragraph
would make sense, then, too)

Is that old fashioned?

Acked-by only for maintainers doesn't make sense to me. Neiher does when
Acked-by has a different meaning for maintainers and non-maintainers.

> > 3) trivial patches should rather get Acked-by
> 
> These days when given by a maintainer, "Acked-by" means that the
> maintainer is happy for the patch going in through another subsystem.

I still see this as a "looks OK to me" variant. A patch is good enough
to enter my subsystem. Sometimes, I also use "Reviewed-by" for this,
namely when I thoroughly looked at (=reviewed) a patch.

> > 2) A short paragraph will usually do. Of course, trust helps a lot, but it
> > doesn't solve everything. Trusted people can be in a hurry, too, etc. And for
> > people I don't know, the plain tag doesn't tell me much. Examples for short
> > descriptions: "I can't say much about the media part, but the I2C part is
> > proper" or "I also checked the documentation and I think this is a good
> > approach to overcome the issue" or "All my concerns in the preceding
> > discussions have been addressed"
> 
> Definitely good to have, but hard to enforce, without making the process
> heavier.

As I wrote before, I don't want to enforce that. But spread the word
that it is good to have and should be done and common sense should apply.

> I have a fifth thesis: many people (incl. guilty me) browse quickly
> through many patches flying by on mailing lists, but don't always go to
> the effort of replying if they don't see something wrong immediately.
> This means we don't catch a share of the reviews happening.

For me, Acked-by would do here.

Thanks,

   Wolfram


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

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Mark Brown]

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

On Mon, Jul 08, 2019 at 01:21:49PM +0200, Geert Uytterhoeven wrote:

> I have a fifth thesis: many people (incl. guilty me) browse quickly
> through many patches flying by on mailing lists, but don't always go to
> the effort of replying if they don't see something wrong immediately.
> This means we don't catch a share of the reviews happening.

Right, and it's also a lot easier to review in the negative ("I
found a problem!") than it is to review in the positive and say
you're confident that something is OK which means that even with
more thorough reviews there's a bias against reporting.

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

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Jonathan Cameron]

On Sat, 6 Jul 2019 16:27:38 +0200
Wolfram Sang <wsa@the-dreams.de> wrote:

> In the parts of the Kernel I work with, reviews are usually given by a plain
> tag. I think this is not enough to keep a good code quality, so I'll start with
> my theses first:
> 
> 1) we need a better distinction between Acked-by: and Reviewed-by: and encourage
>    stricter use of that
> 
> 2) Reviewed-by should have a description of the review done (and the review not
>    done)
> 
> 3) trivial patches should rather get Acked-by
> 
> 4) failing the above should be constructively criticized
> 
> 
> Some more words about each item:
> 
> 1) I am definitely not striving for a clear line, that's impossible. Yet, from
> what I experience, the overlap between the two became large. It reduces the
> extra value a Reviewed-by should have.
> 
> 2) A short paragraph will usually do. Of course, trust helps a lot, but it
> doesn't solve everything. Trusted people can be in a hurry, too, etc. And for
> people I don't know, the plain tag doesn't tell me much. Examples for short
> descriptions: "I can't say much about the media part, but the I2C part is
> proper" or "I also checked the documentation and I think this is a good
> approach to overcome the issue" or "All my concerns in the preceding
> discussions have been addressed"
> 
> 3) Again, no hard line on what is trivial can be made. Still, I think it will
> add to the extra value of a review tag if it is only applied to something which
> is non-trivial, so we should try to have a better distinction.
> 
> 4) We are in such a need for people reviewing that it can be challenging for
> maintainers to be picky about reviews (you can partly include me here). A
> kernel-wide movement aiming for a better distinction between ack (= looks good)
> and review helps both maintainers and developers, I think.
> 
> These things will hopefully help me as a maintainer to better evaluate trust
> for a patch based on the tags given. So, I will try that in the I2C subsystem.
> I would prefer, though, not to be an island but to have something which is
> accepted kernel-wide.
> 
> Disclaimer: I am mainly active in the drivers section of Linux. If reviews are
> handled differently in other parts, I am all ears.
> 
> Well, I am all ears, anyhow. Opinions?
> 

To throw another element in here, as a maintainer, the level of review
done by myself varies a lot depending on

1. Trust of the submitter.  I won't check register definitions against
   data sheets from someone whom history has suggested is careful.
   When the submitter is someone new to me, I'm much more likely to
   go through these with a fine toothed comb.

2. Reviews from others.  This is the scalability question.  I have a
   number of very good reviewers for my corner of the kernel.  I'll take
   a much more superficial look at new code if one of them as given a
   reviewed-by.

3. Chances of side effects.  Core code patches always need (ideally
   multiple) thorough reviews and even then I'll take a careful look
   at them and sometimes spin up some additional tests.

I'm guessing many others follow a similar 'risk' assessment based
approach.  Should we be reflecting that in the tags that maintainers
add?  Normally it's all just hidden in a signed-off-by.

Of course, we can take the view that this info is mostly useful
to maintainers anyway so there would be little advantage in
recording this information. I'm curious as to what others think
on this...

Thanks,

Jonathan

> Kind regards,
> 
>   Wolfram
> 

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Thomas Gleixner]

Jonathan,

On Sun, 14 Jul 2019, Jonathan Cameron wrote:
> To throw another element in here, as a maintainer, the level of review
> done by myself varies a lot depending on
> 
> 1. Trust of the submitter.  I won't check register definitions against
>    data sheets from someone whom history has suggested is careful.
>    When the submitter is someone new to me, I'm much more likely to
>    go through these with a fine toothed comb.
> 
> 2. Reviews from others.  This is the scalability question.  I have a
>    number of very good reviewers for my corner of the kernel.  I'll take
>    a much more superficial look at new code if one of them as given a
>    reviewed-by.
> 
> 3. Chances of side effects.  Core code patches always need (ideally
>    multiple) thorough reviews and even then I'll take a careful look
>    at them and sometimes spin up some additional tests.
> 
> I'm guessing many others follow a similar 'risk' assessment based
> approach.

I certainly do and from my observation this seems to be a pretty common
modus operandi.

> Should we be reflecting that in the tags that maintainers
> add?  Normally it's all just hidden in a signed-off-by.

So we'd need to come up with another set of complicated rules which merily
create the illusion of an objective and quantifyable meaning of these tags.

Even if we agree on a set of new tags the usage will still be based on
individual interpretation, which brings us back to square one.

So no, let's just accept that these things are subjective and apply common
sense to make the best use of them.

Thanks,

	tglx

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Rafael J. Wysocki]

On Sunday, July 14, 2019 12:13:53 PM CEST Thomas Gleixner wrote:
> Jonathan,
> 
> On Sun, 14 Jul 2019, Jonathan Cameron wrote:
> > To throw another element in here, as a maintainer, the level of review
> > done by myself varies a lot depending on
> > 
> > 1. Trust of the submitter.  I won't check register definitions against
> >    data sheets from someone whom history has suggested is careful.
> >    When the submitter is someone new to me, I'm much more likely to
> >    go through these with a fine toothed comb.
> > 
> > 2. Reviews from others.  This is the scalability question.  I have a
> >    number of very good reviewers for my corner of the kernel.  I'll take
> >    a much more superficial look at new code if one of them as given a
> >    reviewed-by.
> > 
> > 3. Chances of side effects.  Core code patches always need (ideally
> >    multiple) thorough reviews and even then I'll take a careful look
> >    at them and sometimes spin up some additional tests.
> > 
> > I'm guessing many others follow a similar 'risk' assessment based
> > approach.
> 
> I certainly do and from my observation this seems to be a pretty common
> modus operandi.
> 
> > Should we be reflecting that in the tags that maintainers
> > add?  Normally it's all just hidden in a signed-off-by.
> 
> So we'd need to come up with another set of complicated rules which merily
> create the illusion of an objective and quantifyable meaning of these tags.
> 
> Even if we agree on a set of new tags the usage will still be based on
> individual interpretation, which brings us back to square one.
> 
> So no, let's just accept that these things are subjective and apply common
> sense to make the best use of them.

I totally agree.

Cheers,
Rafael

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Mauro Carvalho Chehab]

Em Mon, 8 Jul 2019 13:59:50 +0200
Wolfram Sang <wsa@the-dreams.de> escreveu:

> Hi Geert,
> 
> > > 1) we need a better distinction between Acked-by: and Reviewed-by: and encourage
> > >    stricter use of that  
> > 
> > Before we had "Reviewed-by", "Acked-by" meant "looks OK to me".
> > Then we got "Reviewed-by" for more thorough reviews.  
> 
> This is what still makes most sense to me. You can express e.g. that you
> like a patch series and approve the general approach taken but haven't
> gone for the gory details -> Acked-by (a short explaining paragraph
> would make sense, then, too)
> 
> Is that old fashioned?
> 
> Acked-by only for maintainers doesn't make sense to me. Neiher does when
> Acked-by has a different meaning for maintainers and non-maintainers.

On my case, when I receive an Acked-by, I assume that this came
from a maintainer (either a subsystem maintainer or a driver
maintainer) - as I expect that non-maintainers (and reviewers)
will either send me a reviewed-by or a tested-by.

When a review a code that will be merged via someone's else tree,
I usually either give:

- Acked-by - if it is something that touches the subsystem
  I maintain and will be merged via some other tree, but I 
  didn't make a comprehensive review;

- Reviewed-by - if I did a comprehensive review. I can either
  be the maintainer or not of the files touched by the patch.

So, I usually expect the others do about the same.

Looking at Documentation/process/5.Posting.rst:

	 - Acked-by: indicates an agreement by another developer (often a
	   maintainer of the relevant code) that the patch is appropriate for
	   inclusion into the kernel.

	 - Reviewed-by: the named developer has reviewed the patch for correctness;
	   see the reviewer's statement in :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
	   for more detail.

I guess the descriptions are already enough to describe those
tags.

Thanks,
Mauro

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Mauro Carvalho Chehab]

Em Sat, 6 Jul 2019 19:17:24 +0200
Wolfram Sang <wsa@the-dreams.de> escreveu:

> The "problem" with a new reviewer is that I don't know if all aspects of
> a patch have been reviewed or just a subset. 

That's a good point.

While receiving feedback from some -doc patches I wrote, I guess we
currently have something that we can improve for acked-by/reviewed-by
tag description: how to indicate a partial review/ack?

I received several such acks with things like:

	For driver_foo:

	Acked-by : me <my@email>

I also received such acks as:

	Acked-by : me <my@email> # driver_foo
or:
	Acked-by : me <my@email> # for driver_foo

I guess we could agree on a "syntax" for that, with could be easily
be parsed by scripts, documenting it at
Documentation/process/5.Posting.rst.

I'm in favor of:

	Acked-by : me <my@email> # driver_foo
and:
	Reviewed-by : me <my@email> # driver_foo

Thanks,
Mauro

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Theodore Y. Ts'o]

On Mon, Jul 15, 2019 at 12:58:00PM -0300, Mauro Carvalho Chehab wrote:
> On my case, when I receive an Acked-by, I assume that this came
> from a maintainer (either a subsystem maintainer or a driver
> maintainer) - as I expect that non-maintainers (and reviewers)
> will either send me a reviewed-by or a tested-by.

The problem is that the documentation doesn't exactly match your
expectations:

> When a review a code that will be merged via someone's else tree,
> I usually either give:
> 
> - Acked-by - if it is something that touches the subsystem
>   I maintain and will be merged via some other tree, but I 
>   didn't make a comprehensive review;
> 
> - Reviewed-by - if I did a comprehensive review. I can either
>   be the maintainer or not of the files touched by the patch.
> 
> So, I usually expect the others do about the same.
> 
> Looking at Documentation/process/5.Posting.rst:
> 
> 	 - Acked-by: indicates an agreement by another developer (often a
> 	   maintainer of the relevant code) that the patch is appropriate for
> 	   inclusion into the kernel.
> 
> 	 - Reviewed-by: the named developer has reviewed the patch for correctness;
> 	   see the reviewer's statement in :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
> 	   for more detail.
> 
> I guess the descriptions are already enough to describe those
> tags.

I'd suggest changing the text to read:

 	 - Acked-by: indicates an agreement by the maintainer or
	   reviewer of the the relevant code that the patch is
	   appropriate for inclusion into the kernel.

My concern is that if we have dozens of "Acked-by" by people who are
not domain experts in any part of the code in the git log, it's just
noise in the system.  Of course, we can't stop people from sending
Acked-by's on the mailing list, but when I recently pointed out that
I'm going to ignore an bare Acked-by by someone who I have no idea
whether or not I can trust the source of that Acked-by, I got yelled
at for not following the documented process.

That complaint isn't going to change how *I* interpret or decide to
include Acked-by's, but if we have general agreement on the
expectations Maintainers should have (and my expectations match
yours), then perhaps we can adjust the documentation to make it more
clear.

					- Ted

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Mauro Carvalho Chehab]

Em Mon, 15 Jul 2019 13:00:45 -0400
"Theodore Y. Ts'o" <tytso@mit.edu> escreveu:

> On Mon, Jul 15, 2019 at 12:58:00PM -0300, Mauro Carvalho Chehab wrote:

> I'd suggest changing the text to read:
> 
>  	 - Acked-by: indicates an agreement by the maintainer or
> 	   reviewer of the the relevant code that the patch is
> 	   appropriate for inclusion into the kernel.

Yeah, that sounds clearer.

> My concern is that if we have dozens of "Acked-by" by people who are
> not domain experts in any part of the code in the git log, it's just
> noise in the system. 

Agreed.

Thanks,
Mauro

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Wolfram Sang]

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


> Even if we agree on a set of new tags the usage will still be based on
> individual interpretation, which brings us back to square one.

I agree. New tags won't help us much.

> So no, let's just accept that these things are subjective and apply common
> sense to make the best use of them.

I meanwhile do think, though, that for "best use" it will be helpful to
add my Rev-by to my SoB when I really did a full review (and not just
trusted other reviewers). To properly document the process of a patch.


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

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Wolfram Sang]

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


> > Looking at Documentation/process/5.Posting.rst:
> > 
> > 	 - Acked-by: indicates an agreement by another developer (often a
> > 	   maintainer of the relevant code) that the patch is appropriate for
> > 	   inclusion into the kernel.
> > 
> > 	 - Reviewed-by: the named developer has reviewed the patch for correctness;
> > 	   see the reviewer's statement in :ref:`Documentation/process/submitting-patches.rst <submittingpatches>`
> > 	   for more detail.
> > 
> > I guess the descriptions are already enough to describe those
> > tags.
> 
> I'd suggest changing the text to read:
> 
>  	 - Acked-by: indicates an agreement by the maintainer or
> 	   reviewer of the the relevant code that the patch is
> 	   appropriate for inclusion into the kernel.

This sounds a tad too strict for me, yet I am not into bike-shedding and
will step aside if more people prefer this phrasing...

> That complaint isn't going to change how *I* interpret or decide to
> include Acked-by's, but if we have general agreement on the
> expectations Maintainers should have (and my expectations match
> yours), then perhaps we can adjust the documentation to make it more
> clear.

... because I fully agree to this. It is one of the reasons I started
this thread. Working on a general agreement. Oh, and increase awareness
that Acks and Revs can be rejected, of course.


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

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Olof Johansson]

On Tue, Jul 16, 2019 at 2:16 PM Wolfram Sang <wsa@the-dreams.de> wrote:
>
>
> > Even if we agree on a set of new tags the usage will still be based on
> > individual interpretation, which brings us back to square one.
>
> I agree. New tags won't help us much.
>
> > So no, let's just accept that these things are subjective and apply common
> > sense to make the best use of them.
>
> I meanwhile do think, though, that for "best use" it will be helpful to
> add my Rev-by to my SoB when I really did a full review (and not just
> trusted other reviewers). To properly document the process of a patch.

I don't see how this is bringing much value. You picked up the patch,
and if you did so without looking closely at it, the end result is the
same: You're likely to be on the line for dealing with issues.

The only thing a "S-o-b" without "R-b" would bring is "It's not my
fault that stuff broke" -- but it still is since your name is on it,
and that's not something that adds value for the project as a whole.


-Olof

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Thomas Gleixner]

On Tue, 16 Jul 2019, Olof Johansson wrote:
> On Tue, Jul 16, 2019 at 2:16 PM Wolfram Sang <wsa@the-dreams.de> wrote:
> >
> >
> > > Even if we agree on a set of new tags the usage will still be based on
> > > individual interpretation, which brings us back to square one.
> >
> > I agree. New tags won't help us much.
> >
> > > So no, let's just accept that these things are subjective and apply common
> > > sense to make the best use of them.
> >
> > I meanwhile do think, though, that for "best use" it will be helpful to
> > add my Rev-by to my SoB when I really did a full review (and not just
> > trusted other reviewers). To properly document the process of a patch.
> 
> I don't see how this is bringing much value. You picked up the patch,
> and if you did so without looking closely at it, the end result is the
> same: You're likely to be on the line for dealing with issues.
> 
> The only thing a "S-o-b" without "R-b" would bring is "It's not my
> fault that stuff broke" -- but it still is since your name is on it,
> and that's not something that adds value for the project as a whole.

Right, if you commit it then you are responsible for it. It does not matter
whether you reviewed it yourself or relied on someone else review.

Thanks,

	tglx

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Randy Dunlap]

On 7/16/19 3:27 PM, Thomas Gleixner wrote:
> On Tue, 16 Jul 2019, Olof Johansson wrote:
>> On Tue, Jul 16, 2019 at 2:16 PM Wolfram Sang <wsa@the-dreams.de> wrote:
>>>
>>>
>>>> Even if we agree on a set of new tags the usage will still be based on
>>>> individual interpretation, which brings us back to square one.
>>>
>>> I agree. New tags won't help us much.
>>>
>>>> So no, let's just accept that these things are subjective and apply common
>>>> sense to make the best use of them.
>>>
>>> I meanwhile do think, though, that for "best use" it will be helpful to
>>> add my Rev-by to my SoB when I really did a full review (and not just
>>> trusted other reviewers). To properly document the process of a patch.
>>
>> I don't see how this is bringing much value. You picked up the patch,
>> and if you did so without looking closely at it, the end result is the
>> same: You're likely to be on the line for dealing with issues.
>>
>> The only thing a "S-o-b" without "R-b" would bring is "It's not my
>> fault that stuff broke" -- but it still is since your name is on it,
>> and that's not something that adds value for the project as a whole.
> 
> Right, if you commit it then you are responsible for it. It does not matter
> whether you reviewed it yourself or relied on someone else review.

It's a bit amazing (and scary) that this is even being discussed (as though
it's unknown) after these many years.

-- 
~Randy

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Wolfram Sang]

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


> > Right, if you commit it then you are responsible for it. It does not matter
> > whether you reviewed it yourself or relied on someone else review.
> 
> It's a bit amazing (and scary) that this is even being discussed (as though
> it's unknown) after these many years.

Guys, I am just trying to better cope with an amount of patches I can't
handle on my own currently.

My strategy to get more people involved is divide and conquer, e.g. look
for people willing to maintain a single driver. That makes the "trust
matrix" kind of complex. So, I still do think I can improve where to put
my always-too-limited efforts by documenting where I put it in the past.

I can keep track of that differently than a R-b tag, that is fine with
me.

I never had the intention of sneaking out of responsibility and I only
sign patches I am sure they are ready to be upstreamed. This discussion
exists because I want to keep this state.


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

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Linus Walleij]

On Wed, Jul 17, 2019 at 9:31 AM Wolfram Sang <wsa@the-dreams.de> wrote:

> Guys, I am just trying to better cope with an amount of patches I can't
> handle on my own currently.

Maintainers don't scale right? :/

What I tend to do is git log on whatever is affected by a patch
and then pick some people who touched the code recently
and just add them to CC and ask them to help. This is especially
nice with drivers as I can quickly see (by intuition) whether
they are doing some general cleanup or actually are running
and testing the hardware, even if they didn't sign up as
maintainers. Sometimes nudge them to maintain the stuff
they seem so intimate with by adding themselves to
MAINTAINERS. If it was a while since they did the changes
I often do git log and grep for their name as they often
just changed email address.

It would be really neat if get_maintainer could do this but I
think it requires real intelligence rather than pattern-matching.

Just my €0.01

Linus Walleij

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Wolfram Sang]

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


> > Guys, I am just trying to better cope with an amount of patches I can't
> > handle on my own currently.
> 
> Maintainers don't scale right? :/

Yup, no news here.

> What I tend to do is git log on whatever is affected by a patch
> and then pick some people who touched the code recently
> and just add them to CC and ask them to help. This is especially

I do this, too. I also ask people from the same company as the patch
author to jump in.

> nice with drivers as I can quickly see (by intuition) whether
> they are doing some general cleanup or actually are running
> and testing the hardware, even if they didn't sign up as
> maintainers. Sometimes nudge them to maintain the stuff
> they seem so intimate with by adding themselves to
> MAINTAINERS.

I do this, too, and it works OK. I got a few driver maintainers listed.
Some are really active which is nice, some disappear again which is sad
but not too surprising. Yet, for every active new maintainer, I have to
check how they review.

This is what I mean with "trust matrix getting complex". Lots of new
maintainers where I need to build some trust relationship. Maybe it is
just a workflow thing, yet I feel I need some kind of tracking to do
this. And other stuff I mentioned in my initial mail, especially a short
summary of the review and a higher awareness that reviews can be
constructively criticized.


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

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Paul Walmsley]

On Mon, 15 Jul 2019, Theodore Y. Ts'o wrote:

> I'd suggest changing the text to read:
> 
>  	 - Acked-by: indicates an agreement by the maintainer or
> 	   reviewer of the the relevant code that the patch is
> 	   appropriate for inclusion into the kernel.

This would be a positive step forward.  I would be in favor of this. 

It would also be good to state here, if it isn't stated already, that 
"reviewer" means "someone who is listed in an R: line in MAINTAINERS".


- Paul

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Jan Kara]

On Sat 17-08-19 21:35:29, Paul Walmsley wrote:
> On Mon, 15 Jul 2019, Theodore Y. Ts'o wrote:
> 
> > I'd suggest changing the text to read:
> > 
> >  	 - Acked-by: indicates an agreement by the maintainer or
> > 	   reviewer of the the relevant code that the patch is
> > 	   appropriate for inclusion into the kernel.
> 
> This would be a positive step forward.  I would be in favor of this. 
> 
> It would also be good to state here, if it isn't stated already, that 
> "reviewer" means "someone who is listed in an R: line in MAINTAINERS".

I don't think that 'R:' entry in MAINTAINERS should be really asked for.
IMO that is unnecessary bureaucracy and discourages review from people
that are not core developers. Sure the quality of the review may be lower
than from core developer but still there's some value in it. So I'd really
leave it at the discretion of the maintainer whether he accepts or just
ignores Reviewed-by tag.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Jiri Kosina]

On Mon, 19 Aug 2019, Jan Kara wrote:

> I don't think that 'R:' entry in MAINTAINERS should be really asked for.
> IMO that is unnecessary bureaucracy and discourages review from people
> that are not core developers. Sure the quality of the review may be lower
> than from core developer but still there's some value in it. So I'd really
> leave it at the discretion of the maintainer whether he accepts or just
> ignores Reviewed-by tag.

I totally agree.

Sure, Acked-by: (and other tags) is sometimes over-used by random people 
just to get their name listed in the kernel changelog ... on the other 
hand, restricting this only to "the MAINTAINERS club" is definitely not 
how you invite more relevant reviewers to join and help.

-- 
Jiri Kosina
SUSE Labs

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Julia Lawall]

On Mon, 19 Aug 2019, Jan Kara wrote:

> On Sat 17-08-19 21:35:29, Paul Walmsley wrote:
> > On Mon, 15 Jul 2019, Theodore Y. Ts'o wrote:
> >
> > > I'd suggest changing the text to read:
> > >
> > >  	 - Acked-by: indicates an agreement by the maintainer or
> > > 	   reviewer of the the relevant code that the patch is
> > > 	   appropriate for inclusion into the kernel.
> >
> > This would be a positive step forward.  I would be in favor of this.
> >
> > It would also be good to state here, if it isn't stated already, that
> > "reviewer" means "someone who is listed in an R: line in MAINTAINERS".
>
> I don't think that 'R:' entry in MAINTAINERS should be really asked for.
> IMO that is unnecessary bureaucracy and discourages review from people
> that are not core developers. Sure the quality of the review may be lower
> than from core developer but still there's some value in it. So I'd really
> leave it at the discretion of the maintainer whether he accepts or just
> ignores Reviewed-by tag.

Is there some other tag for "I'm interested in and reasonably
knowledgeable about this change and it looks good to me"?

Note that there is a double "the" in the above text.

julia

>
> 								Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
> _______________________________________________
> Ksummit-discuss mailing list
> Ksummit-discuss@lists.linuxfoundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/ksummit-discuss
>

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Jan Kara]

On Mon 19-08-19 09:06:26, Julia Lawall wrote:
> On Mon, 19 Aug 2019, Jan Kara wrote:
> 
> > On Sat 17-08-19 21:35:29, Paul Walmsley wrote:
> > > On Mon, 15 Jul 2019, Theodore Y. Ts'o wrote:
> > >
> > > > I'd suggest changing the text to read:
> > > >
> > > >  	 - Acked-by: indicates an agreement by the maintainer or
> > > > 	   reviewer of the the relevant code that the patch is
> > > > 	   appropriate for inclusion into the kernel.
> > >
> > > This would be a positive step forward.  I would be in favor of this.
> > >
> > > It would also be good to state here, if it isn't stated already, that
> > > "reviewer" means "someone who is listed in an R: line in MAINTAINERS".
> >
> > I don't think that 'R:' entry in MAINTAINERS should be really asked for.
> > IMO that is unnecessary bureaucracy and discourages review from people
> > that are not core developers. Sure the quality of the review may be lower
> > than from core developer but still there's some value in it. So I'd really
> > leave it at the discretion of the maintainer whether he accepts or just
> > ignores Reviewed-by tag.
> 
> Is there some other tag for "I'm interested in and reasonably
> knowledgeable about this change and it looks good to me"?
> 
> Note that there is a double "the" in the above text.

No. But is there a need for such tag? I, as a maintainer, would like to see
in the email where someone offers the Reviewed-by tag, how confident the
reviewer feels (otherwise I just make my educated guess). But I don't
really see a point in recording this in the changelog. After all the tag in
the changelog serves only two purposes I know about - to give credit to the
reviewer and to have another person to blame (CC on bug reports ;). So I
don't see any need in recording quality of review in the changelog for
long-term recording of the fact...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Julia Lawall]

On Mon, 19 Aug 2019, Jan Kara wrote:

> On Mon 19-08-19 09:06:26, Julia Lawall wrote:
> > On Mon, 19 Aug 2019, Jan Kara wrote:
> >
> > > On Sat 17-08-19 21:35:29, Paul Walmsley wrote:
> > > > On Mon, 15 Jul 2019, Theodore Y. Ts'o wrote:
> > > >
> > > > > I'd suggest changing the text to read:
> > > > >
> > > > >  	 - Acked-by: indicates an agreement by the maintainer or
> > > > > 	   reviewer of the the relevant code that the patch is
> > > > > 	   appropriate for inclusion into the kernel.
> > > >
> > > > This would be a positive step forward.  I would be in favor of this.
> > > >
> > > > It would also be good to state here, if it isn't stated already, that
> > > > "reviewer" means "someone who is listed in an R: line in MAINTAINERS".
> > >
> > > I don't think that 'R:' entry in MAINTAINERS should be really asked for.
> > > IMO that is unnecessary bureaucracy and discourages review from people
> > > that are not core developers. Sure the quality of the review may be lower
> > > than from core developer but still there's some value in it. So I'd really
> > > leave it at the discretion of the maintainer whether he accepts or just
> > > ignores Reviewed-by tag.
> >
> > Is there some other tag for "I'm interested in and reasonably
> > knowledgeable about this change and it looks good to me"?
> >
> > Note that there is a double "the" in the above text.
>
> No. But is there a need for such tag? I, as a maintainer, would like to see
> in the email where someone offers the Reviewed-by tag, how confident the
> reviewer feels (otherwise I just make my educated guess). But I don't
> really see a point in recording this in the changelog. After all the tag in
> the changelog serves only two purposes I know about - to give credit to the
> reviewer and to have another person to blame (CC on bug reports ;). So I
> don't see any need in recording quality of review in the changelog for
> long-term recording of the fact...

So is there no tag at all for what I describe?  Concretely, Coccinelle
reports bugs via 0-day, sometimes people send me the patch, and sometimes
I would like to say "yes, I looked at it and it seems to be fixing the bug
that was reported", without implying that I have extensively tested the
code.  So is there a concise unambiguous way to do that?

julia


>
> 								Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
>

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Thomas Gleixner]

On Mon, 19 Aug 2019, Jan Kara wrote:
> On Sat 17-08-19 21:35:29, Paul Walmsley wrote:
> > On Mon, 15 Jul 2019, Theodore Y. Ts'o wrote:
> > 
> > > I'd suggest changing the text to read:
> > > 
> > >  	 - Acked-by: indicates an agreement by the maintainer or
> > > 	   reviewer of the the relevant code that the patch is
> > > 	   appropriate for inclusion into the kernel.
> > 
> > This would be a positive step forward.  I would be in favor of this. 
> > 
> > It would also be good to state here, if it isn't stated already, that 
> > "reviewer" means "someone who is listed in an R: line in MAINTAINERS".
> 
> I don't think that 'R:' entry in MAINTAINERS should be really asked for.
> IMO that is unnecessary bureaucracy and discourages review from people
> that are not core developers. Sure the quality of the review may be lower
> than from core developer but still there's some value in it. So I'd really
> leave it at the discretion of the maintainer whether he accepts or just
> ignores Reviewed-by tag.

The R: in MAINTAINERS is there to make sure these people get actually CC'ed
on patches against that particular subsystem. It does not mean that others
are not allowed or encouraged to review patches in that area.

Thanks,

	tglx

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Christian Brauner]

On Mon, Aug 19, 2019 at 10:26:01AM +0200, Thomas Gleixner wrote:
> On Mon, 19 Aug 2019, Jan Kara wrote:
> > On Sat 17-08-19 21:35:29, Paul Walmsley wrote:
> > > On Mon, 15 Jul 2019, Theodore Y. Ts'o wrote:
> > > 
> > > > I'd suggest changing the text to read:
> > > > 
> > > >  	 - Acked-by: indicates an agreement by the maintainer or
> > > > 	   reviewer of the the relevant code that the patch is
> > > > 	   appropriate for inclusion into the kernel.
> > > 
> > > This would be a positive step forward.  I would be in favor of this. 
> > > 
> > > It would also be good to state here, if it isn't stated already, that 
> > > "reviewer" means "someone who is listed in an R: line in MAINTAINERS".
> > 
> > I don't think that 'R:' entry in MAINTAINERS should be really asked for.
> > IMO that is unnecessary bureaucracy and discourages review from people
> > that are not core developers. Sure the quality of the review may be lower
> > than from core developer but still there's some value in it. So I'd really
> > leave it at the discretion of the maintainer whether he accepts or just
> > ignores Reviewed-by tag.
> 
> The R: in MAINTAINERS is there to make sure these people get actually CC'ed
> on patches against that particular subsystem. It does not mean that others
> are not allowed or encouraged to review patches in that area.

If I may, I agree that only accepting acks/reviews by people in R: is
too strict. Imho, it sends the wrong message and probably discourages
participation in kernel development. It's also a high bar currently to
get people even listed as R: In my experience people are reluctant to
suggest they be added as R: in that file because it might be conceived
as being overly assertive of ones abilities. One easy fix could be to
encourage maintainers of a given subsystem to be more open to add people
they trust as R:

Christian

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Thomas Gleixner]

On Mon, 19 Aug 2019, Christian Brauner wrote:
> On Mon, Aug 19, 2019 at 10:26:01AM +0200, Thomas Gleixner wrote:
> > On Mon, 19 Aug 2019, Jan Kara wrote:
> > > On Sat 17-08-19 21:35:29, Paul Walmsley wrote:
> > > > On Mon, 15 Jul 2019, Theodore Y. Ts'o wrote:
> > > > 
> > > > > I'd suggest changing the text to read:
> > > > > 
> > > > >  	 - Acked-by: indicates an agreement by the maintainer or
> > > > > 	   reviewer of the the relevant code that the patch is
> > > > > 	   appropriate for inclusion into the kernel.
> > > > 
> > > > This would be a positive step forward.  I would be in favor of this. 
> > > > 
> > > > It would also be good to state here, if it isn't stated already, that 
> > > > "reviewer" means "someone who is listed in an R: line in MAINTAINERS".
> > > 
> > > I don't think that 'R:' entry in MAINTAINERS should be really asked for.
> > > IMO that is unnecessary bureaucracy and discourages review from people
> > > that are not core developers. Sure the quality of the review may be lower
> > > than from core developer but still there's some value in it. So I'd really
> > > leave it at the discretion of the maintainer whether he accepts or just
> > > ignores Reviewed-by tag.
> > 
> > The R: in MAINTAINERS is there to make sure these people get actually CC'ed
> > on patches against that particular subsystem. It does not mean that others
> > are not allowed or encouraged to review patches in that area.
> 
> If I may, I agree that only accepting acks/reviews by people in R: is
> too strict.

It is.

> Imho, it sends the wrong message and probably discourages
> participation in kernel development. It's also a high bar currently to
> get people even listed as R: In my experience people are reluctant to
> suggest they be added as R: in that file because it might be conceived
> as being overly assertive of ones abilities. One easy fix could be to
> encourage maintainers of a given subsystem to be more open to add people
> they trust as R:

We do, but not over the head of the developer. We ask people before doing
that and quite some decline.

Thanks,

	tglx

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[Christian Brauner]

On Mon, Aug 19, 2019 at 09:04:49PM +0200, Thomas Gleixner wrote:
> On Mon, 19 Aug 2019, Christian Brauner wrote:
> > On Mon, Aug 19, 2019 at 10:26:01AM +0200, Thomas Gleixner wrote:
> > > On Mon, 19 Aug 2019, Jan Kara wrote:
> > > > On Sat 17-08-19 21:35:29, Paul Walmsley wrote:
> > > > > On Mon, 15 Jul 2019, Theodore Y. Ts'o wrote:
> > > > > 
> > > > > > I'd suggest changing the text to read:
> > > > > > 
> > > > > >  	 - Acked-by: indicates an agreement by the maintainer or
> > > > > > 	   reviewer of the the relevant code that the patch is
> > > > > > 	   appropriate for inclusion into the kernel.
> > > > > 
> > > > > This would be a positive step forward.  I would be in favor of this. 
> > > > > 
> > > > > It would also be good to state here, if it isn't stated already, that 
> > > > > "reviewer" means "someone who is listed in an R: line in MAINTAINERS".
> > > > 
> > > > I don't think that 'R:' entry in MAINTAINERS should be really asked for.
> > > > IMO that is unnecessary bureaucracy and discourages review from people
> > > > that are not core developers. Sure the quality of the review may be lower
> > > > than from core developer but still there's some value in it. So I'd really
> > > > leave it at the discretion of the maintainer whether he accepts or just
> > > > ignores Reviewed-by tag.
> > > 
> > > The R: in MAINTAINERS is there to make sure these people get actually CC'ed
> > > on patches against that particular subsystem. It does not mean that others
> > > are not allowed or encouraged to review patches in that area.
> > 
> > If I may, I agree that only accepting acks/reviews by people in R: is
> > too strict.
> 
> It is.
> 
> > Imho, it sends the wrong message and probably discourages
> > participation in kernel development. It's also a high bar currently to
> > get people even listed as R: In my experience people are reluctant to
> > suggest they be added as R: in that file because it might be conceived
> > as being overly assertive of ones abilities. One easy fix could be to
> > encourage maintainers of a given subsystem to be more open to add people
> > they trust as R:
> 
> We do, but not over the head of the developer. We ask people before doing

Sorry, I implicitly assumed the asking-for-developer-consent part.
You're right.

> that and quite some decline.

Right, I had that experience as well. Imho, that's even more of a sign
that restricting "valuable" reviewers to R: is not a good idea.

One idea I had a while back - and this maybe crazy - was to make a
review by a certain person mandatory: A way to mark a given patch as
requiring a review by someone before it can be considered for inclusion,
i.e. something stronger than "Cc e.g. "Requires-Review-by" without
necessarily having that person to be listed in MAINTAINERS. Maybe that's
already expressed in the R: field though...

Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Keeping reviews meaningful

[James Bottomley]

On Mon, 2019-08-19 at 10:13 +0200, Julia Lawall wrote:
> 
> On Mon, 19 Aug 2019, Jan Kara wrote:
> 
> > On Mon 19-08-19 09:06:26, Julia Lawall wrote:
> > > On Mon, 19 Aug 2019, Jan Kara wrote:
> > > 
> > > > On Sat 17-08-19 21:35:29, Paul Walmsley wrote:
> > > > > On Mon, 15 Jul 2019, Theodore Y. Ts'o wrote:
> > > > > 
> > > > > > I'd suggest changing the text to read:
> > > > > > 
> > > > > >  	 - Acked-by: indicates an agreement by the
> > > > > > maintainer or
> > > > > > 	   reviewer of the the relevant code that the patch is
> > > > > > 	   appropriate for inclusion into the kernel.
> > > > > 
> > > > > This would be a positive step forward.  I would be in favor
> > > > > of this.
> > > > > 
> > > > > It would also be good to state here, if it isn't stated
> > > > > already, that "reviewer" means "someone who is listed in an
> > > > > R: line in MAINTAINERS".
> > > > 
> > > > I don't think that 'R:' entry in MAINTAINERS should be really
> > > > asked for. IMO that is unnecessary bureaucracy and discourages
> > > > review from people that are not core developers. Sure the
> > > > quality of the review may be lower than from core developer but
> > > > still there's some value in it. So I'd really leave it at the
> > > > discretion of the maintainer whether he accepts or just ignores
> > > > Reviewed-by tag.
> > > 
> > > Is there some other tag for "I'm interested in and reasonably
> > > knowledgeable about this change and it looks good to me"?
> > > 
> > > Note that there is a double "the" in the above text.
> > 
> > No. But is there a need for such tag? I, as a maintainer, would
> > like to see in the email where someone offers the Reviewed-by tag,
> > how confident the reviewer feels (otherwise I just make my educated
> > guess). But I don't really see a point in recording this in the
> > changelog. After all the tag in the changelog serves only two
> > purposes I know about - to give credit to the reviewer and to have
> > another person to blame (CC on bug reports ;). So I don't see any
> > need in recording quality of review in the changelog for long-term
> > recording of the fact...
> 
> So is there no tag at all for what I describe?  Concretely,
> Coccinelle reports bugs via 0-day, sometimes people send me the
> patch, and sometimes I would like to say "yes, I looked at it and it
> seems to be fixing the bug that was reported", without implying that
> I have extensively tested the code.  So is there a concise
> unambiguous way to do that?

Yes, that's "Reviewed-by:".  If you actually tested it, you'd add a
"Tested-by:" as well.  Sometimes people do

Reviewed-by: me@me.com #the bits I understand or care about

But usually it's up to the maintainer of the file to decide whether the
review is meaningful enough to be accepted.  Whether the Reviewed-by:
is accepted by the maintainer is completely their decision and really
has nothing to do with R: tags in the Maintainers file.

James