Messages in this thread | | | Date | Mon, 2 Jun 2014 16:59:15 -0700 | From | josh@joshtrip ... | Subject | Re: [PATCH RFC 1/2] MAINTAINERS: Add "R:" designated-reviewers tag |
| |
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: > > On Mon, 2014-06-02 at 12:09 -0700, josh@joshtriplett.org wrote: > > > On Mon, Jun 02, 2014 at 12:05:17PM -0700, Joe Perches wrote: > > > > On Mon, 2014-06-02 at 11:55 -0700, josh@joshtriplett.org wrote: > > > > > this should go along with a change to > > > > > get_maintainer.pl to add those folks to the CC list. > > > > > > > > Something like this: > > > > > > Yes, exactly. Given an appropriate commit message, > > > Reviewed-by: Josh Triplett <josh@joshtriplett.org> > > > > That's the sort of patch where reviewing is > > pretty useless. > > > > 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.
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.
> 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.
- Josh Triplett
| |