[webkit-dev] questions re: patch length
mjs at apple.com
Mon Dec 14 19:34:28 PST 2009
On Dec 14, 2009, at 7:22 PM, Chris Jerdonek wrote:
> I have a few questions related to patch length:
> (1) Do reviewers take patch length into account when considering
> whether to review a patch? This is useful to know for those who would
> rather have a short patch reviewed more quickly than wait longer for a
> longer patch to be reviewed.
> (2) If reviewers do take patch length into account, what's the best
> way to handle trivial changes that might inflate patch length (for
> example moving a large chunk of code or adding an image) -- should
> such changes be submitted separately?
If they are meaningful to do separately (i.e. tree won't be in a
ridiculous or broken state) then sure. In particular copying or
renaming files or doing large code cleanup is best done separate from
It can also help to mention if parts of the patch are mechanical and
identify just the most meaningful parts.
Another thing that makes it easier for me review is test cases. If I
can see exactly what behavior change is intended in the form of a test
case, it's easier to evaluate the code changes. This is true even if
adding numerous test cases makes the code change overall bigger.
> (3) Finally, in people's experience, what is the "sweet spot" for
> patch length? (There is an optimization problem here somewhere.)
I make some effort to break a patch down into the smallest meaningful
pieces I can if it seems large, even if I have to do this after the
fact. I particularly like to separate preparatory refactorings from
actual behavior changes. On the other hand, if some changes really are
tied to each other and aren't meaningful separately, I usually bite
More information about the webkit-dev