[webkit-dev] Patch process - let's make it better

[Maciej Stachowiak]

Hi everyone,

One common topic for discussion has been how to make our process  
around patch submission better. As the project grows, it's becoming  
more important for this process to work really smoothly, and we are  
seeing some breakdowns. I've been doing a lot of thinking about this,  
and discussion with some project old hands. I think the right way to  
tackle this is to identify the process problems, and make sure we  
address each one. I'd also like to start by fixing the problems that  
can be addressed without making major wholesale tools changes first,  
then look at the bigger changes. Here are my thoughts on the steps in  
the lifecycle of a patch:


=== 1) Submitting the patch ===

Steps:
1.1) File a bug if there isn't one already.
1.2) Put the bug number in your ChangeLog entry.
1.3) Create a patch file.
1.4) Attach the patch to bugzilla using the bug form.
1.5) Flag the path for review.

Issues:
This is too much work to get a patch sent out for review. It wastes  
people's time, and makes them defect to alternate methods of review,  
such as lisppaste. There's no reason submitting a patch for proper  
review should be more work than posting it on Lisppaste.


=== 2) Getting a reviewer to look at the patch ===

Steps:
2.1) Hope that someone spots it in the review queue.
2.2) Possibly ask a reviewer directly, by IRC, by email or in person.

Issues:
Clearly this part of the process is not working well. The review queue  
grows out of control, and patches linger for a long time.


=== 3) Giving specific, directed feedback on the patch ===

Steps:
3.1) Use the "review patch" view in bugzilla, and read the patch in  
one frame while editing comments in another.

Issues:
It seems clear that inline comments and a side-by-side view of  
different patch versions (as supported by Rietveld) would make this a  
lot easier. In addition it seems likely this would encourage reviewers  
to give more comments, and to iterate rounds of review more, which  
likely would improve our code quality.


=== 4) Indicate the status of the patch in light of feedback ===

Steps:
4.1) Flag the patch as "review+" or "review-" (or in rare cases clear  
the review flag.

Issues:
review- is too negative - what we really mean most of the time is that  
we'd like to see the next iteration. review+ is insufficiently clear -  
sometimes reviewers say it's ok to commit but with some changes.  
Committers can't tell the difference. Also, review+ remains the state  
for a patch that has been committed. That's problematic because when a  
bug has multiple patches, committers have a hard time knowing which  
still need committing.


=== 5) Optionally repeat steps 1-4 if the patch needs revision ===


=== 6) Commit the patch ===

Steps:
There's two cases here, committing your own patch, or committing the  
patch of a contributor who does not yet have committer status. Either  
way, to do it right there's a number of desirable extra steps such as  
including a trac link for the commit, closing the bug, etc.



So, that's a whole host of things that don't work great about our  
review process. I'd like to hear from the community whether there are  
other issues. In the meantime, here is a tentative action plan:


=== Action Plan ===

* Phase 1 *

A) Make it really easy to submit a patch. Eric's bugzilla-tool is  
close to being there. I'm going to help him refine this tool to the  
point that submitting a patch has only one step - a single command  
will make the patch, file a bug if needed, attach the patch to the  
bug, and flag it for review.

B) Make it really easy to commit a patch. Again, I think bugzilla-tool  
is the right path to achieving this.

C) Improve the way we get attention from reviewers. I think we should  
do three things here:
     C.1) Split review queues, based on our emerging [Bracket]  
convention for patches needing specialized review. If we find more  
areas of specialty besides ports, we can add new [Bracket] tags.
     C.2) Improve the quality of emails sent
     C.3) Highlight in a special way patches that have been waiting  
for review longer that some minimum period (a week?). These could be  
highlighted visually in the review queue, and maybe would send extra  
review request emails automatically.

* Phase 2 *

D) Change review flags. I previously suggested that patch states  
should be something like "Review", "Revise and Resubmit",  
"Rejected" (to be used very sparingly!), "Commit", "Commit with  
Changes" and "Landed".


* Phase 3 *

Make the process of giving and reading review comments more pleasant.  
As I see it, there are three viable options proposed so far: (a) use  
rietveld (with some form of better integration with bugzilla); (b) use  
Review Board; or (c) develop similar functionality as part of our  
bugzilla customizations. I think we should explore all of these  
options but hold off on this change until we have the Phase 1 / Phase  
2 fixes deployed and running smoothly for people.


=== Future Tools Directions in General ===

Once we get the review process straightened out, I think the other big  
things to look at are build system (can we reduce the number of files  
you need to change when modifying the build?) and revision control  
system. Deploying wrapper tools for common tasks in the development  
cycle will make it less painful to make some of these other changes  
down the line. Also, it's clear that we critically need try server  
capability, and we're working on setting this up for Apple's ports at  
least, with an easy way for other ports to plug in.


Regards,
Maciej