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