Open Stack

On 01/12/15 12:45, Maciej Kwiek wrote:
> Hi,
>
> I recently noticed the influx of big patches hitting Gerrit
> (especially in fuel-web, but I also heard that there was a couple of
> big ones in library). I think that patches that have 1000 LOC are
> simply too big to review thoroughly and reliably.
>
> I would argue that there should be a limit to patch size, either a
> soft one (i.e. written down in contributor guidelines) or a hard one
> (e.g. enforced by gate job).
>
> I think that we need a discussion whether we really need this limit,
> what should it be, and how to enforce it.
>
> I personally think that most patches that are over 400 LOC could be
> easily split into at least two smaller changes.
>
> Regarding the limit enforcement - I think we should go with the soft
> limit, with X LOC written as a guideline and giving the reviewers a
> possibility to give -1s to patches that are too big, but also giving
> the possibility to merge bigger changes if it's absolutely necessary
> (in really rare cases the changes simply cannot be split). We may mix
> in the hard limit for ridiculously large patches (twice the "soft
> limit" would be good in my opinion), so the gate would automatically
> reject such patches, forcing contributor to split his patch.
>
> Please share your thoughts on this.

I think most of your principle is correct.  However I can imagine a file
renaming / moving patch that would appear in Gerrit to be >=1000 LOC,
but would actually just be file moves, with perhaps some trivial changes
to Python module paths; and I don't think it would be helpful to force a
patch like that to be split up.  So it may not be correct to enforce a
hard limit all the time.

    Neil