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
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
