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

Mon Oct 3 01:58:32 UTC 2016

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

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