[Ksummit-discuss] [CORE TOPIC] Recruitment (Reviewers, Testers, Maintainers, Hobbyists)

Josh Poimboeuf jpoimboe at redhat.com
Sun Jul 12 12:28:23 UTC 2015


On Sat, Jul 11, 2015 at 10:23:17PM -0700, Josh Triplett wrote:
> On Sat, Jul 11, 2015 at 10:48:24PM -0500, Josh Poimboeuf wrote:
> > On Fri, Jul 10, 2015 at 06:00:50PM -0700, Davidlohr Bueso wrote:
> > > On Fri, 2015-07-10 at 13:14 -0500, Josh Poimboeuf wrote:
> > > > On Wed, Jul 08, 2015 at 02:53:15PM -0400, Steven Rostedt wrote:
> > > > > I personally don't trust a Reviewed-by tag much, as I sometimes see
> > > > > them appear without any comments.
> > > > > 
> > > > > I was thinking of writing a perl script that would read my LKML archive
> > > > > and somewhat intelligently looking at people who replied to patches,
> > > > > that also has snippets of the patch in the reply, and counting them. I
> > > > > think that would be a more accurate use of real reviewers than just the
> > > > > Reviewed-by tag.
> > > > 
> > > > I'm relatively new to lkml so my perceptions might be skewed.  But it
> > > > seems to me that, for non-trivial patches, the Reviewed-by tag is pretty
> > > > much useless as a metric.  From what I can tell, the biggest problems
> > > > with relying on Reviewed-by to get meaningful statistics are:
> > > > 
> > > > 1. It's self-reported by the reviewer.
> > > > 
> > > >    Maybe a reviewer gave some hugely valuable feedback in versions 1-4
> > > >    of the patch, but wasn't around to provide the Reviewed-by tag for
> > > >    the final v6.
> > > 
> > > Normally good maintainers pick up ack/review tags along the way when
> > > they pickup the final patch.
> > 
> > A maintainer can't pick up the tag if the reviewer doesn't post it.
> > 
> > In this example the reviewer wouldn't have posted Reviewed-by because,
> > despite the fact that they did a thorough review, the patch still had
> > issues in its earlier versions.  The reviewer did a lot of work but
> > didn't get credit for it.
> 
> And that's not always a bad thing.  Sometimes I review the previous
> version of a patch, and then see that tag applied to a later version
> that is substantially different from the version I reviewed; that makes
> me uncomfortable.  I care less about getting credit for a review and
> more about not having my Reviewed-by tag attached to something I didn't
> review.

If our goal is to quantify the work of good reviewers, so that we can
shine a light on them and hopefully motivate them to do more review,
then I'd argue that having a reviewer doing a lot of work and not
getting credit for it *is* a bad thing.

I agree that the Reviewed-by tag doesn't apply here.  It communicates
patch approval, rather than review helpfulness.  That's why I proposed a
new tag from the patch author and/or maintainer.

-- 
Josh


More information about the Ksummit-discuss mailing list