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

[Ravishankar N]

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 <mailto:pkarampu at redhat.com>> wrote:
>
>
>
>     On Fri, Sep 30, 2016 at 8:50 PM, Ravishankar N
>     <ravishankar at redhat.com <mailto: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.

-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/
>         <https://lwn.net/Articles/503829/>
>
>>         Niels
>>
>>         0.http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n552
>>         <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n552>
>>
>>         _______________________________________________
>>         Gluster-devel mailing list
>>         Gluster-devel at gluster.org <mailto:Gluster-devel at gluster.org>
>>         http://www.gluster.org/mailman/listinfo/gluster-devel
>>         <http://www.gluster.org/mailman/listinfo/gluster-devel>
>
>     -- 
>     Pranith
>
> -- 
> Pranith

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