[development] What about reviewing patches?

[Nathaniel Catchpole]

On Wed, Aug 13, 2008 at 9:08 PM, Katherine Senzee wrote:
> On Wed, Aug 13, 2008 at 12:52 PM, Nathaniel Catchpole
>  wrote:
>> Some patch reviews take less than 30 seconds, especially when the
>> issue queue is long and has lots of stale patches in it.
>
> I'm not sure I follow. I review patches when I get the time, but I
> figure on at least 20 minutes -- read the issue, check out HEAD, apply
> the patch, create a new database, install the test site,
> download/install devel, generate stuff, see if the patch works as
> advertised, post my review. If I have a recent enough HEAD install I
> figure I can skip the database bit and just use my existing database,
> but it's still a decent chunk of time. What am I doing wrong?

20 minutes is about right to do a proper review, but a lot of patches
don't even apply any more, including RTBC ones.

So if you already have HEAD installed (I pretty much only clean my
database before or after reviewing a patch with an upgrade path or
schema change), you can do this:

// clean your HEAD of old changes.
cvs -q diff | patch -p0 -R

wget http://drupal.org/files/issues/somepatch.patch

patch -p0 < somepatch.patch

1 out of 1 hunks failed etc.

Mark to code needs work with 'needs a re-roll'.

Maybe 45 seconds then, but not too much more than that.

Also simpletest is a much, much easier and more comprehensive way to
avoid regressions that installing devel, generating content and
clicking around. So that stage is reduced to: admin/build/modules
enable simpletest
admin/build/testing run all tests

Although you can go make a cup of tea while the tests run, there's
little effort involved running them. You should obviously test the
actual functionality of the patch on top of this, but that's a bit
less RSI inducing.

Nat