[Gluster-devel] [Gluster-Maintainers] 'Reviewd-by' tag for commits

Joe Julian joe at julianfamily.org
Mon Oct 3 06:47:09 UTC 2016

If you get credit for +1, shouldn't you also get credit for -1? It seems to me that catching a fault is at least as valuable if not more so. 

On October 3, 2016 3:58:32 AM GMT+02:00, Pranith Kumar Karampuri <pkarampu at redhat.com> wrote:
>On Mon, Oct 3, 2016 at 7:23 AM, Ravishankar N <ravishankar at redhat.com>
>> On 10/03/2016 06:58 AM, Pranith Kumar Karampuri wrote:
>> On Mon, Oct 3, 2016 at 6:41 AM, Pranith Kumar Karampuri <
>> pkarampu at redhat.com> wrote:
>>> On Fri, Sep 30, 2016 at 8:50 PM, Ravishankar N
><ravishankar at redhat.com>
>>> wrote:
>>>> On 09/30/2016 06:38 PM, Niels de Vos wrote:
>>>> On Fri, Sep 30, 2016 at 07:11:51AM +0530, Pranith Kumar Karampuri
>>>> hi,
>>>>      At the moment 'Reviewed-by' tag comes only if a +1 is given on
>>>> final version of the patch. But for most of the patches, different
>>>> would spend time on different versions making the patch better,
>they may
>>>> not get time to do the review for every version of the patch. Is it
>>>> possible to change the gerrit script to add 'Reviewed-by' for all
>>>> people who participated in the review?
>>>> +1 to this. For the argument that this *might* encourage me-too
>+1s, it
>>>> only exposes
>>>> such persons in bad light.
>>>> Or removing 'Reviewed-by' tag completely would also help to make
>sure it
>>>> doesn't give skewed counts.
>>>> I'm not going to lie, for me, that takes away the incentive of
>doing any
>>>> reviews at all.
>>> Could you elaborate why? May be you should also talk about your
>>> motivation for doing reviews.
>> I guess it is probably because the effort needs to be recognized? I
>> there is an option to recognize it so it is probably not a good idea
>> remove the tag I guess.
>> Yes, numbers provide good motivation for me:
>> Motivation for looking at patches and finding bugs for known
>> even though I am not its maintainer.
>> Motivation to learning new components because a bug and a fix is
>> when I look at code for unknown components.
>> Motivation to level-up when statistics indicate I'm behind my peers.
>> I think even you said some time back in an ML thread that what can be
>> measured can be improved.
>I am still not sure how to quantify good review from a bad one. So not
>how it can be measured thus improved. I guess at this point getting
>eyes on the patches is good enough.
>> -Ravi
>>> I would not feel comfortable automatically adding Reviewed-by tags
>>>> people that did not review the last version. They may not agree
>with the
>>>> last version, so adding their "approved stamp" on it may not be
>>>> See the description of Reviewed-by in the Linux kernel sources [0].
>>>> While the Linux kernel model is the poster child for projects to
>>>> standards
>>>> from, IMO, their email based review system is certainly not one to
>>>> emulate. It
>>>> does not provide a clean way to view patch-set diffs, does not
>present a
>>>> single
>>>> URL based history that tracks all review comments, relies on the
>>>> to
>>>> provide information on what changed between versions, allows a
>variety of
>>>> 'Komedians' [1] to add random tags which may or may not be picked
>>>> by the maintainer who takes patches in etc.
>>>> Maybe we can add an additional tag that mentions all the people
>>>> did do reviews of older versions of the patch. Not sure what the
>>>> would be, maybe just CC?
>>>> It depends on what tags would be processed to obtain statistics on
>>>> review contributions.
>>>> I agree that not all reviewers might be okay with the latest
>>>> but that
>>>> % might be miniscule (zero, really) compared to the normal case
>>>> the reviewer spent
>>>> considerable time and effort to provide feedback (and an eventual
>+1) on
>>>> previous
>>>> revisions. If converting all +1s into 'Reviewed-by's is not
>feasible in
>>>> gerrit
>>>> or is not considered acceptable, then the maintainer could wait for
>>>> reasonable
>>>> time for reviewers to give +1 for the final revision before he/she
>>>> ahead
>>>> with a +2 and merges it. While we cannot wait indefinitely for all
>>>> a comment
>>>> like 'LGTM, will wait for a day for other acks before I go ahead
>>>> merge' would be
>>>> appreciated.
>>>> Enough of bike-shedding from my end I suppose.:-)
>>>> Ravi
>>>> [1] https://lwn.net/Articles/503829/
>>>> Niels
>>>> 0.
>>>> _______________________________________________
>>>> Gluster-devel mailing
>listGluster-devel at gluster.orghttp://www.gluster.org/mailman/listinfo/gluster-devel
>>>> --
>>> Pranith
>> --
>> Pranith
>Gluster-devel mailing list
>Gluster-devel at gluster.org

Sent from my Android device with K-9 Mail. Please excuse my brevity.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.gluster.org/pipermail/gluster-devel/attachments/20161003/64cd820b/attachment.html>

More information about the Gluster-devel mailing list