Message-ID: <20140603011125.GW14410@dastard>
Date:	Tue, 3 Jun 2014 11:11:25 +1000
From:	Dave Chinner <david@...morbit.com>
To:	josh@...htriplett.org
Cc:	Joe Perches <joe@...ches.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	paulmck@...ux.vnet.ibm.com, linux-kernel@...r.kernel.org,
	mingo@...nel.org, laijs@...fujitsu.com, dipankar@...ibm.com,
	akpm@...ux-foundation.org, mathieu.desnoyers@...icios.com,
	niv@...ibm.com, tglx@...utronix.de, peterz@...radead.org,
	dhowells@...hat.com, edumazet@...gle.com, dvhart@...ux.intel.com,
	fweisbec@...il.com, oleg@...hat.com, sbw@....edu
Subject: Re: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag

On Mon, Jun 02, 2014 at 04:59:15PM -0700, josh@...htriplett.org wrote:
> On Tue, Jun 03, 2014 at 09:19:49AM +1000, Dave Chinner wrote:
> > On Mon, Jun 02, 2014 at 12:17:46PM -0700, Joe Perches wrote:
> > > What it needs is testing, not reviewing.
> > > 
> > > I tested it for all of 10 seconds.
> > 
> > From Documentation/SubmittingPatches:
> > 
> > "         (c) While there may be things that could be improved with this
> >              submission, I believe that it is, at this time, (1) a
> >              worthwhile modification to the kernel, and (2) free of known
> >              issues which would argue against its inclusion.
> > .....
> > 
> > A Reviewed-by tag is a statement of opinion that the patch is an
> > appropriate modification of the kernel without any remaining serious
> > technical issues."
> > 
> > So, for someone to say they have reviewed the code and are able to
> > say it is free of known issues and has no remaining technical
> > issues, they would have had to apply, compile and test the patch,
> > yes?
> > 
> > i.e. Reviewed-by implies both Acked-by, Tested-by and that the code
> > is technically sound.
> 
> No, not at all.  It implies Acked-by, and that the code is technically
> sound (both at the micro-level and in overall architecture/approach),
> but does not imply Tested-by; that's a separate tag for a reason.

You've ignored the (c).(2) "free of known issues" criteria there.
You cannot say a patch is free of issues if you haven't applied,
compiled and tested it.

> We should not, for instance, prevent someone from providing a
> Reviewed-by (as opposed to an Acked-by) for a driver whose hardware few
> people actually have.  There's significant value in code review even
> without the ability to test.

I don't disagree with you that there's value in code review, but
that's not the only part of what "reviewed-by" means.

You can test that the code is free of known issues without reviewing
it (i.e. tested-by). You can read the code and note that you can't
see any technical issues without testing it (Acked-by).

But you can't say that is it both free of techical and known
issues without both reading the code and testing it (Reviewed-by).

> > Anyone using Reviewed-by without having actually applied and tested
> > the patch is mis-using the tag - they should be using Acked-by: if
> > all they have done is read the code in their mail program....
> 
> Acked-by and Reviewed-by mean two different things (Reviewed-by being a
> superset of Acked-by), and the difference is not "I've applied and
> tested this"; that's Tested-by.

Right, the difference is more than that - Reviewed-by is a
superset of both Acked-by and Tested-by.

And, yes, this is the definition we've been using for "reviewed-by"
for XFS code since, well, years before the "reviewed-by" tag even
existed...

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists