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

[James Bottomley <James.Bottomley@HansenPartnership.com>]

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