[webkit-dev] questions re: patch length

[Maciej Stachowiak]

Personal thoughts:

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.

Yes.

> (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  
substantive changes.

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  
the bullet.

Regards,
Maciej