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

[Ravishankar N]

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.
> 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 list
> Gluster-devel at gluster.org
> http://www.gluster.org/mailman/listinfo/gluster-devel


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.gluster.org/pipermail/maintainers/attachments/20160930/6d5173d4/attachment-0001.html>