From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp1.linuxfoundation.org (smtp1.linux-foundation.org [172.17.192.35]) by mail.linuxfoundation.org (Postfix) with ESMTPS id D6921AAE for ; Sun, 12 Jul 2015 03:48:27 +0000 (UTC) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by smtp1.linuxfoundation.org (Postfix) with ESMTPS id 57C2463 for ; Sun, 12 Jul 2015 03:48:27 +0000 (UTC) Date: Sat, 11 Jul 2015 22:48:24 -0500 From: Josh Poimboeuf To: Davidlohr Bueso Message-ID: <20150712034824.GA4236@treble.hsd1.ky.comcast.net> References: <201507080121.41463.PeterHuewe@gmx.de> <1481488.5WJFbB0Dlm@vostro.rjw.lan> <1436341028.2136.14.camel@HansenPartnership.com> <20150708080032.CE89E4306F@saturn.retrosnub.co.uk> <20150708145315.29030a75@gandalf.local.home> <20150710181409.GA30145@treble.redhat.com> <1436576450.27924.59.camel@stgolabs.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1436576450.27924.59.camel@stgolabs.net> Cc: James Bottomley , jic23@jic23.retrosnub.co.uk, ksummit-discuss@lists.linuxfoundation.org, Jason Cooper Subject: Re: [Ksummit-discuss] [CORE TOPIC] Recruitment (Reviewers, Testers, Maintainers, Hobbyists) List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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. > > 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). > > At least at some level any patches picked up by a maintainer should at > least be compile tested and looked at any obvious issues -- when the > maintainer trusts other devs/submaintainers that did the more thorough > review process. Right, as I said, Signed-off-by doesn't necessarily imply Reviewed-by. So any review done by a maintainer who only adds Signed-off-by goes uncredited. > > 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. > > The system can always be gamed. > > > > > 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. > > Mentioning/thanking others that helped in any meaningful way is always a > good idea. Sure, but if you only thank them informally then their contribution can't be quantified, which was one of the proposed goals of this discussion. > > 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". > > A tag for this? Nah. > > Thanks, > Davidlohr > -- Josh