[drupal-devel] [bug] Patch review queue

Thu Aug 4 19:31:04 UTC 2005

Issue status update for 
http://drupal.org/node/28210
Post a follow up: 
http://drupal.org/project/comments/add/28210

 Project:      Drupal
 Version:      cvs
 Component:    base system
-Category:     feature requests
+Category:     bug reports
 Priority:     normal
 Assigned to:  Anonymous
 Reported by:  robertDouglass
 Updated by:   robertDouglass
-Status:       active
+Status:       patch (code needs review)

[dirty trick] I'd like to have better ways of making issues like this
show up on the devel-list.

robertDouglass

Previous comments:
------------------------------------------------------------------------

Thu, 04 Aug 2005 17:48:46 +0000 : robertDouglass

I've had some fun clicking the "Play patch bingo!" link repeatedly and
think it is a very fine addition to the Contributor links block. One
thing that really impressed me was the huge number of patches that seem
like a good idea, whose authors assert that the patch is ready to
commit, and that have several "+1 I reviewed and liked" comments. All
of this made me think a little more about the undead issue of whether
we need more core committers and how patches can get reviewed and
processed in a more organized manner. Then I had an idea. Here it is.

In order to 1) motivate the masses of developers to review patches, 2)
guarantee that patches don't get buried in the queue and never get
read, 3) have a way to *measure* whether Dries and Steven can keep up
on their own, why don't we implement a patch review schedule? I imagine
it working something like this:

* New patches are put into a first-in-first-out queue. This queue is
the order in which committers will review the patch.
* Committers must (must is a very flexible word here... more about
expectations than rules) review the top patch before reviewing the one
right in line behind it, kind of like a judge has to review court cases
in a fixed order.
* The "up next" patches get well advertised so that developers and
those interested in a particular patch know when to do their lobbying
and when to make sure that things get reviewed by a critical mass of
developers so that the committer has enough support to make a decision.
* Every patch review must result in a status change or update comment.
This is the history or review trail of a patch. This occurs now anyway,
so this is no big change but rather an acknowledgment of the expectation
that the committers never "silently" look at a patch and then move it to
the back of the queue.
* Every patch review results in either a committ, having the patch
removed from the queue (won't fix), or being sent to the back of the
queue with concrete criteria and expectations for when it gets reviewed
next. (I'm not suggesting that if Dries writes "remove this space and
send a new patch" and someone does this 5 minutes later that the patch
has to wait two weeks until it gets to the top of the list)

That's the extent of what I propose to add/change. Here's what I hope
that we would gain from it. I expect that the testing efforts of the
developer community would become more focused, meaning we would be more
likely to be testing the same patch at the same time. Why? Let's say
that I have a patch in the queue and it is in place 5. That means very
soon, Dries or Steven will be reviewing it. My big chance! Since I know
that the opportunity is at hand and that the price of not getting it
committed this time around will be high (put back to the end of the
queue), I will make sure the patch still applies and then get on IRC
and say "hey chx, walkah, killes and amazon, please test this patch
today... it is up for review and I need some critical mass". The result
will be that more eyes and intentions will be focused on the patch at
that moment, which hopefully will give Dries the info he needs and has
asked for when it comes to review time.

Please note that I am not suggesting that committers should be
forbidden from following a patch that they're interested in and
committing patches from the middle of the queue. I'm proposing a system
for addressing those days when they wake up in the morning and say
"well, let's see.... maybe I'll review patches today. Let's take a look
at that patch queue." The value of the list is that the whole world will
know that very soon, the patch on the top of it will be reviewed and
when it is, that is the biggest chance for a while that it will get
committed. Furthermore, nobody submitting an issue will have the voice
in their head saying "hope Dries is online today so he sees this patch
before it gets burried in the queue". Instead you'll know that your
patch *is* burried at the end of the queue, but that it will
definitely, 100% guaranteed, get reviewed eventually.

We can also train the masses to participate more in the review process.
If the "up to bat" and "on deck" and "in the hole" issues are visible on
drupal.org, and if we provide nice instructions for reviewing patches in
such a way that people get the idea "oh, this is something I'm expected
to do", I think we'll have a higher review rate.

Another thing that such a queue would open the door for is an issue
freeze that comes before a code freeze in the development cycle (of
course critical bugs will always be handled ASAP and not be put in any
queue). Maybe two weeks before a code freeze we can decide that all new
issues will go into the queue for the next cycle, and the timing of the
code freeze will depend on working through the current queue. As it is,
there are dozens of small bugfixes in the queue which will likely not
see the light of day before the code freeze or release. This method of
organizing the queue might help give us more concrete goals for each
stage in the cycle.

Finally, and this is the part that is hardest to say since I love Dries
and Steven and think they do a bang-up job, a system like this would
make it very clear if the system isn't keeping up with demand. If we
can't even make it through the whole queue once per development cycle,
there is a bottleneck that is restraining our progress. Perhaps we'd
decide, for example, to not increase the number of committers, but
rather that we need a pre-commit team (5-10 people) who are the ones
who review every patch, and only the ones they approve get sent to
Dries and Steven. A solution like that would make two similar queues;
the one I just described with everything in it which will get reviewed
in order by memebers of the pre-committ team, and a queue for Dries and
Steven that functions the same way but only contains patches that have
passed the pre-commit review phase and was promoted by a member of the
pre-commit team (perhaps members of the pre-commit team cannot promote
their own patches).

Ok - there it is. Flame me!

cheers,

Robert

------------------------------------------------------------------------

Thu, 04 Aug 2005 19:11:09 +0000 : robertDouglass

Another benefit that I forgot to mention is that the price of ignoring
crappy issues is higher. Take the case where a really important and
really anticipated patch is in place 12 and the 11 issues before it are
crap, meaning submitted by bad coders, not addressing important issues
and generally unlikely to make core. Anyone interested in the good
issue in place 12 will be saying "hey cats, can't we clean the litter
box here, I mean, what is all this crap keeping us from getting to the
real deal". And *whooosh*, 11 crappy issues are marked to "get a life"
or at least "won't fix" and removed from the queue. The price of
ignoring issues is high already; our good issues get lost in the crap.
The real price we pay for this is delayed and comes in the form of a
bloated queue that a core committer won't touch with a ten foot
clue-bat. By requiring that we clear the crap to get to the meat the
price is moved upfront, which might seem more expensive, but the slim
and trim patch queue that will result will be worth it and the overall
price of crappy issues will be lower.