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

Mon Oct 3 09:19:35 UTC 2016

On Mon, Oct 3, 2016 at 12:17 PM, Joe Julian <joe at julianfamily.org> wrote:

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

Yes when I said review it could be either +1/-1/+2

>
> 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>
>> wrote:
>>
>>> 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 wrote:
>>>>>
>>>>> hi,
>>>>>      At the moment 'Reviewed-by' tag comes only if a +1 is given on the
>>>>> final version of the patch. But for most of the patches, different people
>>>>> 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 the
>>>>> 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 primary
>>>> motivation for doing reviews.
>>>>
>>>
>>> I guess it is probably because the effort needs to be recognized? I
>>> think there is an option to recognize it so it is probably not a good idea
>>> to remove the tag I guess.
>>>
>>>
>>> Yes, numbers provide good motivation for me:
>>> Motivation for looking at patches and finding bugs for known components
>>> even though I am not its maintainer.
>>> Motivation to learning new components because a bug and a fix is usually
>>> 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
>> sure how it can be measured thus improved. I guess at this point getting
>> more eyes on the patches is good enough.
>>
>>
>>>
>>> -Ravi
>>>
>>>
>>>
>>>>
>>>> I would not feel comfortable automatically adding Reviewed-by tags for
>>>>> 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 correct.
>>>>> See the description of Reviewed-by in the Linux kernel sources [0].
>>>>>
>>>>> While the Linux kernel model is the poster child for projects to draw
>>>>> 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
>>>>> sender 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 up
>>>>> by the maintainer who takes patches in etc.
>>>>>
>>>>> Maybe we can add an additional tag that mentions all the people that
>>>>> did do reviews of older versions of the patch. Not sure what the tag
>>>>> 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 revision
>>>>> but that
>>>>> % might be miniscule (zero, really) compared to the normal case where
>>>>> 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 a
>>>>> reasonable
>>>>> time for reviewers to give +1 for the final revision before he/she
>>>>> goes ahead
>>>>> with a +2 and merges it. While we cannot wait indefinitely for all
>>>>> acks, a comment
>>>>> like 'LGTM, will wait for a day for other acks before I go ahead and
>>>>> merge' would be
>>>>> appreciated.
>>>>>
>>>>> Enough of bike-shedding from my end I suppose.:-)
>>>>> Ravi
>>>>>
>>>>> [1] https://lwn.net/Articles/503829/
>>>>>
>>>>> Niels
>>>>>
>>>>> 0. http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n552
>>>>>
>>>>> _______________________________________________
>>>>> Gluster-devel mailing listGluster-devel at gluster.orghttp://www.gluster.org/mailman/listinfo/gluster-devel
>>>>>
>>>>> --
>>>> Pranith
>>>>
>>> --
>>> Pranith
>>>
>>>
>>
>>
> --
> Sent from my Android device with K-9 Mail. Please excuse my brevity.
>

-- 
Pranith
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.gluster.org/pipermail/maintainers/attachments/20161003/81bc4877/attachment-0001.html>