Patch Pilot report

Andrew Bennetts andrew.bennetts at canonical.com
Mon Nov 23 01:11:00 GMT 2009


Hi all,

I just finished the first round of patch piloting (see
<http://bazaar-vcs.org/PatchPilot>) last week.  Robert's doing this week... and
then the roster is empty.  Who wants to volunteer?

Here's some scattered observations from the week:

Many patches are under 100 lines.  That's generally not so hard to review, but
Launchpad doesn't display line counts on
<https://code.launchpad.net/bzr/+activereviews> so it's hard to know which are
small and which are monsters without loading them all.

It's tedious looking up which contributors have signed Canonical's contributors
agreement.  There's a Launchpad team they are supposed to be a member of, but to
check you have to go to the user's page (not that team page!) and look over the
complete list of team memberships... and even then I don't think it's current!
There's a Canonical-internal wiki page which seems to be current.  And there are
Canonical employees from other projects, who typically haven't signed a
contributors agreement but (assuming they made the patch during work hours)
don't need to.  Also, if the internal wiki page is the only current source for
this information, then that will be an impediment to non-Canonical folk acting
as patch pilots.

I took a “just do it” approach to piloting patches.  If the changes required
were fairly small, or even moderate, it usually seemed to me to be easier to
just make the tweak and write the missing tests myself, rather than teach and
cajole the contributors to do themselves.  I did try to paste a diff of my
changes into the review comments so that the contributor could see what I did
and hopefully learn from it, if they are that way inclined.  I think other patch
pilots may take a different approach, and it'll be interesting to see how they
work out.  Also, I deliberately chose patches from new contributors over
regulars.

Managing NEWS entry conflicts is noticeably more aggravating when you are
landing several a day.

Subjectively, it doesn't feel like the queue on
<https://code.launchpad.net/bzr/+activereviews> really shrank that much.  Also,
it's a shame that “Approved reviews ready to land” actually includes “approved,
but blocked on signing contributors agreement”.  Perhaps we should bump the
overall status of these to “Needs Fixing” or something just to keep the queue
reasonably accurate.  Generally speaking, that Launchpad page fails as a “what
should I [the patch pilot] look at next” page; any section of that page can have
patches deserving of attention from a patch pilot.  I touched patches from the
top and bottom of that page.

I have a branch of bzr-pqm, lp:~spiv/bzr-pqm/non-local-submission, that adds a
--ignore-local option.  It's pretty handy for landing already approved patches,
but many approved patches are actually lacking NEWS entries.

Some patches took several rounds of interaction over several days, some just
needed a single small nudge.  I guess that's just like ordinary reviewing :)

Patches piloted:

 * lp:~mnordhoff/bzr/remote-control-format-typo: landed, was already approved.
 * lp:~songofacandy/bzr/doc-ja: landed, was already approved.  It turned out to
   have some build system fallout that poolie fixed up afterwards.
 * lp:~mathepic/bzr/remove-tree-multi: fixed test failures, added missing test.
   Blocked on copyright assignment.
 * lp:~gioele/bzr/forgot-commit-message: help requested on IRC, added review
   with explicit next steps to the linked bug, now having regular review
   conversation.
 * lp:~doxxx/bzr/271790: wrote test and NEWS entry, was rejected by PQM for
   undesirable change of stdout to stderr, contributor fixed, landed.
 * lp:~doxxx/bzr/456036: wrote NEWS entry, landed.
 * lp:~amanica/bzr/325618_log_returns_too_much: asked igc to review
 * lp:~slyguy/bzr/bug-440952-bzrdir: asked for contributor agreement
 * Stephen Turnbull's "5 minute contribution guide": turned into patch, tweaked,
   proposed, landed.
 * Helped "rudds" on IRC propose a doc change via Launchpad merge proposals
 * lp:~asac/bzr/lp459276: added tests and fixed buglets, landed.

Of those, these are still in the queue (either unlanded or unrejected, but not
unreviewed!):

 * lp:~mathepic/bzr/remove-tree-multi
 * lp:~gioele/bzr/forgot-commit-message
 * lp:~amanica/bzr/325618_log_returns_too_much
 * lp:~slyguy/bzr/bug-440952-bzrdir

-Andrew.




More information about the bazaar mailing list