<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 30, 2016 at 8:50 PM, Ravishankar N <span dir="ltr">&lt;<a href="mailto:ravishankar@redhat.com" target="_blank">ravishankar@redhat.com</a>&gt;</span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  
    
  
  <div bgcolor="#FFFFFF" text="#000000"><span class="">
    <div>On 09/30/2016 06:38 PM, Niels de Vos
      wrote:<br>
    </div>
    <blockquote type="cite">
      <pre>On Fri, Sep 30, 2016 at 07:11:51AM +0530, Pranith Kumar Karampuri wrote:
</pre>
      <blockquote type="cite">
        <pre>hi,
     At the moment &#39;Reviewed-by&#39; 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 &#39;Reviewed-by&#39; for all the
people who participated in the review?</pre>
      </blockquote>
    </blockquote>
    </span><tt>+1 to this. For the argument that this *might* encourage me-too
      +1s, it only exposes<br>
      such persons in bad light.<br>
    </tt><span class="">
    <blockquote type="cite">
      <blockquote type="cite">
        <pre>Or removing &#39;Reviewed-by&#39; tag completely would also help to make sure it
doesn&#39;t give skewed counts.
</pre>
      </blockquote>
    </blockquote>
    </span><tt>I&#39;m not going to lie, for me, that takes away the incentive of
      doing any reviews at all.</tt><span class=""><br></span></div></blockquote><div><br></div>Could you elaborate why? May be you should also talk about your primary motivation for doing reviews.<br><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><span class="">
    <blockquote type="cite">
      <pre>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 &quot;approved stamp&quot; on it may not be correct.
See the description of Reviewed-by in the Linux kernel sources [0].
</pre>
    </blockquote>
    </span><tt>While the Linux kernel model is the poster child for projects to
      draw standards</tt><tt><br>
    </tt><tt>
      from, IMO, their email based review system is certainly not one to
      emulate. It</tt><tt><br>
    </tt><tt>
      does not provide a clean way to view patch-set diffs, does not
      present a single</tt><tt><br>
    </tt><tt>
      URL based history that tracks all review comments, relies on the
      sender to</tt><tt><br>
    </tt><tt>
      provide information on what changed between versions, allows a
      variety of</tt><tt><br>
    </tt><tt>
      &#39;Komedians&#39; [1] to add random tags which may or may not be picked
      up</tt><tt><br>
    </tt><tt>
      by the maintainer who takes patches in etc.</tt><span class="">
    <blockquote type="cite">
      <pre>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?</pre>
    </blockquote>
    </span><tt>It depends on what tags would be processed to obtain statistics
      on review contributions.</tt><tt><br>
    </tt><tt>I agree that not all reviewers might be okay with the
      latest revision but that</tt><tt><br>
    </tt><tt>% might be miniscule (zero, really) compared to the normal
      case where the reviewer spent </tt><tt><br>
    </tt><tt>considerable time and effort to provide feedback (and an
      eventual +1) on previous</tt><tt><br>
    </tt><tt>revisions. If converting all +1s into &#39;Reviewed-by&#39;s is not
      feasible in gerrit</tt><tt><br>
    </tt><tt>or is not considered acceptable, then the maintainer could
      wait for a reasonable</tt><tt><br>
    </tt><tt>time for reviewers to give +1 for the final revision before
      he/she goes ahead</tt><tt><br>
    </tt><tt>with a +2 and merges it. While we cannot wait indefinitely
      for all acks, a comment<br>
      like </tt><tt>&#39;LGTM, will wait for a day for other acks before I
      go ahead and merge&#39; would be<br>
      appreciated.<br>
      <br>
      Enough of bike-shedding from my end I suppose.<span><span>:-)</span></span><br>
      Ravi </tt><br>
    <br>
    <tt>[1] <a href="https://lwn.net/Articles/503829/" target="_blank">https://lwn.net/Articles/<wbr>503829/</a></tt><br>
    <br>
    <blockquote type="cite">
      <pre>Niels

0. <a href="http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches#n552" target="_blank">http://git.kernel.org/cgit/<wbr>linux/kernel/git/torvalds/<wbr>linux.git/tree/Documentation/<wbr>SubmittingPatches#n552</a>
</pre>
      <br>
      <fieldset></fieldset>
      <br>
      <pre>______________________________<wbr>_________________
Gluster-devel mailing list
<a href="mailto:Gluster-devel@gluster.org" target="_blank">Gluster-devel@gluster.org</a>
<a href="http://www.gluster.org/mailman/listinfo/gluster-devel" target="_blank">http://www.gluster.org/<wbr>mailman/listinfo/gluster-devel</a></pre>
    </blockquote>
    <p><br>
    </p>
  </div>

</blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr">Pranith<br></div></div>
</div></div>