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

[Frank Rowand <frowand.list@gmail.com>]

On 7/8/2015 9:06 PM, Michael Ellerman wrote:
> On Wed, 2015-07-08 at 13:08 -0700, Guenter Roeck wrote:
>> On 07/08/2015 11:53 AM, Steven Rostedt wrote:
>>> On Wed, 08 Jul 2015 09:00:32 +0100
>>> jic23@jic23.retrosnub.co.uk wrote:
>>>
>>>
>>>>> We can alter that somewhat.  We used to run a Maintainers lottery for
>>>>> the kernel summit ... we could instead offer places based on the number
>>>>> of Reviewed-by: tags ... we have all the machinery to calculate that.  I
>>>>> know an invitation to the kernel summit isn't a huge incentive, but it's
>>>>> a useful one.
>>>>
>>>> Sounds like a good idea to me, though it would only effect a tiny
>>>> percentage of our reviewers.  I suppose publishing a short list of the top
>>>> n% of reviewers from which the lottery runs might give some
>>>> recognition.
>>>>
>>>
>>> I personally don't trust a Reviewed-by tag much, as I sometimes see
>>> them appear without any comments.

I don't expect my Reviewed-by tag with no extra comments to carry much weight
if I send it to a maintainer who does not know me.

But if I have a history of good reviews to a specific maintainer, then why
should I have to add a message that says: Yes, I really, really did review
the patch.  I truly mean that the patch "has been reviewed and found acceptable
according to the Reviewer's Statement" as listed in SubmittingPatches.

And I read Steve's qualification of "don't trust ... _much_" as being
consistent with what I am saying, so I'm fine with that.  The point I
want to make is that a Reviewed-by tag without comments should not
always be assumed to be without meaning or value.

>>>
>>
>> Except for the following, they are always reliable and can be trusted.
>>
>> Reviewed-by: Edsel Murphy <edsel@murphy.com>
>>
>> Seriously, it does happen that I send Reviewed-by: or Acked-by: feedback if
>> a patch is just fine as-is. What do you expect the reviewer to do in such
>> a case ?
> 
> There's almost always something you can say.

And there is a time to not nit-pick a patch to death.

If my comment will not result in a significantly better patch, then I am
wasting the patch submitter's time, and the time of everyone else on the
mail list that will have to read my comment, and potentially have to read
and review a new spin of the patch.

Reviewed-by does not mean that I believe the patch is perfect and could not
possibly be improved.  From the "Reviewer's statement of oversight":

  (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.

> 
> Even if it's a trivial patch, eg. a spelling fix, as the reviewer you should be
> confirming that only the spelling fix happened, ie. no other changes slipped
> into the diff. And so you can say that.
> 
> If it's more complex than a spelling fix then there's usually something you can
> comment on.
> 
> There might be times when all you can say is "Yep, logic looks right" which
> might seem redundant, but personally I'd prefer to see that than just a plain
> Reviewed-by.

"Yep, logic looks right" actually seems like a useful comment, because it tells
me that the reviewer probably does not understand the full context, but the code
is self consistent and does not have any obvious problems.  So I will not think
that the review is complete.

-Frank