<div dir="ltr"><div>TL/DR: I think you're trying to solve the problem the wrong way. If you're trying to reduce the burden of large patches, I feel the project in question should distribute the burden of reviewing the big ones so one person's not stuck doing them all. That also means you can keep that patch in mental context.</div><div><br></div><div>Also, random concerns I have with using "Line of code" as a metric:</div><div><br></div><div>* <span style="line-height:1.5">What counts as a Line of Code? Does whitespace count? Comments? Docs? Tests? Should comments/docs/tests be included in this heuristic?</span></div><div><div>* <span style="line-height:1.5">Taking a big patch, and splitting it into several, can easily lead to dead code as the first patch may remain completely inactive on master until the entire patch chain merges.</span></div><div><span style="line-height:1.5">* git annotate becomes far less useful, as you cannot simply look at all the applicable changes for a given patch - you have to dig for all related patches.</span></div><div><span style="line-height:1.5">* Reverting things becomes more difficult for the same reason. Would you revert all-in-one? One revert per patch?</span></div></div><div><span style="line-height:1.5"><br></span></div><div><span style="line-height:1.5">I've seen, and written, 1000+ line patches. Some of them were 200 lines of logic, 1000+ lines of tests. Others included extensive documentation, comments, etc, or perhaps-too-verbose parameter and method names that clearly explain what they do. Others use method-parameter-per-line style formatting in their code to assist in legibility.</span><br></div><div><br></div><div>While I totally understand how frustrating it is to have to review a large patch, I'm not in favor of a hard limit for something which is governed mostly by whitespace and formatting preferences.</div><div><br></div><div>Michael</div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Dec 1, 2015 at 6:36 AM Sylwester Brzeczkowski <<a href="mailto:sbrzeczkowski@mirantis.com">sbrzeczkowski@mirantis.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div>Neil, just to clarify: moved/renamed files are marked as "R" so I think there may be some way to ignore such files when counting LOC.<br><br></div>Maciej, I completely agree with you. It's pretty hard to review such big change,and takes a lot of time which could be saved by submitting smaller patches.<br><br></div>+1<br></div><div class="gmail_extra"></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Dec 1, 2015 at 1:50 PM, Neil Jerram <span dir="ltr"><<a href="mailto:Neil.Jerram@metaswitch.com" target="_blank">Neil.Jerram@metaswitch.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span>On 01/12/15 12:45, Maciej Kwiek wrote:<br>
> Hi,<br>
><br>
> I recently noticed the influx of big patches hitting Gerrit<br>
> (especially in fuel-web, but I also heard that there was a couple of<br>
> big ones in library). I think that patches that have 1000 LOC are<br>
> simply too big to review thoroughly and reliably.<br>
><br>
> I would argue that there should be a limit to patch size, either a<br>
> soft one (i.e. written down in contributor guidelines) or a hard one<br>
> (e.g. enforced by gate job).<br>
><br>
> I think that we need a discussion whether we really need this limit,<br>
> what should it be, and how to enforce it.<br>
><br>
> I personally think that most patches that are over 400 LOC could be<br>
> easily split into at least two smaller changes.<br>
><br>
> Regarding the limit enforcement - I think we should go with the soft<br>
> limit, with X LOC written as a guideline and giving the reviewers a<br>
> possibility to give -1s to patches that are too big, but also giving<br>
> the possibility to merge bigger changes if it's absolutely necessary<br>
> (in really rare cases the changes simply cannot be split). We may mix<br>
> in the hard limit for ridiculously large patches (twice the "soft<br>
> limit" would be good in my opinion), so the gate would automatically<br>
> reject such patches, forcing contributor to split his patch.<br>
><br>
> Please share your thoughts on this.<br>
<br>
</span>I think most of your principle is correct.  However I can imagine a file<br>
renaming / moving patch that would appear in Gerrit to be >=1000 LOC,<br>
but would actually just be file moves, with perhaps some trivial changes<br>
to Python module paths; and I don't think it would be helpful to force a<br>
patch like that to be split up.  So it may not be correct to enforce a<br>
hard limit all the time.<br>
<br>
    Neil<br>
<br>
<br>
__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</blockquote></div><br><br clear="all"><br></div><div class="gmail_extra">-- <br><div><div dir="ltr"><div><b>Sylwester Brzeczkowski</b><br></div>Junior Python Software <span>Engineer</span><br>Product Development-Core : Product Engineering<br></div></div>
</div>
__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</blockquote></div>