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

[Pranith Kumar Karampuri]

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.


>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.gluster.org/pipermail/maintainers/attachments/20161003/43eb9e3e/attachment.html>