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

[Christian Brauner <christian.brauner@ubuntu.com>]

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...