On Mon, Oct 3, 2016 at 7:23 AM, Ravishankar N <ravishan...@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 < > pkara...@redhat.com> wrote: > >> >> >> On Fri, Sep 30, 2016 at 8:50 PM, Ravishankar N <ravishan...@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@gluster.orghttp://www.gluster.org/mailman/listinfo/gluster-devel >>> >>> -- >> Pranith >> > -- > Pranith > > -- Pranith
_______________________________________________ Gluster-devel mailing list Gluster-devel@gluster.org http://www.gluster.org/mailman/listinfo/gluster-devel