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

[Josh Poimboeuf <jpoimboe@redhat.com>]

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.

   Or maybe they're just focused on the review, and forget or don't care
   to ask for credit for it at the time by adding a tag.

   Or maybe they're a maintainer who used Signed-off-by instead (but
   Signed-off-by doesn't necessarily imply a review).

   Also, as others have mentioned, it creates the ability to game the
   system by saying you've reviewed a patch when you really haven't.

2. It's too broad of a statement.  It requires the reviewer to make an
   official certification that they fully understand and approve of the
   *entire* patch.  That's certainly a useful statement, but:

   A reviewer might review only part of the patch, and provide some
   useful comments for just the part of the patch they reviewed.  But
   they don't want to take responsibility for saying, to quote Steven,
   "I took enough effort to understand every part of the patch as if I
   wrote it myself."

   Or maybe they didn't carefully review the patch, but instead added
   something valuable to the discussion which improved the patch in a
   future version.

   So it denies credit to all the other people who provided useful
   comments along the way.

Review comments such as "here's a problem with your code" or "have you
thought about doing this instead?" can often be more useful a comment
than "your code looks good".  Maybe it would be a good idea to
acknowledge those people who give valuable feedback, in all its forms.
Maybe a new tag would be useful?  Something which:

a) is reported by the patch author and/or maintainer, since they're in
   the best position to keep track of useful comments across versions;

   and

b) means something like "I'm grateful to this person for giving some
   valuable feedback".


-- 
Josh