Patch Pilot report

[Martin Pool]

2009/11/23 Jonathan Lange <jml at canonical.com>:
> On Mon, Nov 23, 2009 at 4:23 AM, Martin Pool <mbp at canonical.com> wrote:
>> 2009/11/23 Ian Clatworthy <ian.clatworthy at canonical.com>:
>>> Andrew Bennetts wrote:
>>>> 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.
>
> What *should* the patch pilot look at next?

The highest-order bit is to distinguish 'things they should act on'
from others.  The types of action they're going to take are:

 * read the cover letter and comment
 * read the patch and comment
 * do the extra work (eg adding tests) needed for the patch
 * land it
 * report that it failed to land because of eg failing tests
 * ask someone else to read it instead or as well
 * ask the person to rework it
 * supersede the mp by a different branch

Things I don't need to do anything about are:

 * it's by a person who could land it and I expect they will do it
 * it needs to be reworked by the submitter
 * it needs cleanup and I'm not going to do them
 * i don't know/care enough about this review to do it

(These have fuzzy edges; they're not intended to be formally encoded.)

The biggest problem with lp reviews at the moment is that looking at
<https://code.edge.launchpad.net/bzr/+activereviews> there is no
distinction between things I should do something to, and things I only
need to be aware of in the background.  So (in a very GTD way) this
means that every time I look at the page I have to seek around to find
what to actually do.

In theory it doesn't matter what specific next action you suggest
first as long as it's something actionable.  In practice if I have
limited time I might want to do a quick landing or if pqm is busy I
might want to do something other than a landing, so seeing what is
needed helps.  Or a project or person might just have a bias towards
finishing things which are the closest to being complete.  Also, in
general I might prefer not to think about patches that already have
sufficient attention.

Despite that I want the tool to help me focus on things I should do, I
still want to be aware of other things.  People do comment on reviews
that have already got a reviewer.

Looking through that page as it appears at the moment to me

heading "approved reviews ready to land"
first review <https://code.edge.launchpad.net/~danny.vanheumen/bzr/84659-serve--allow-writes-allows-more-than-you-might-think/+merge/9274>
- long discussion, needs to tweak and land

2 <https://code.edge.launchpad.net/~mathepic/bzr/mkdir-recursive/+merge/13380>
- apparently can just land

3 <https://code.edge.launchpad.net/~mathepic/bzr/remove-tree-multi/+merge/13451>
- this branch breaks things and can't land, but there's another branch
that can; also the person must sign the contributor agreement

4 <https://code.edge.launchpad.net/~jameinel/bzr/2.1-remove-win32-build/+merge/13848>
- really is approved, but it's up to john to land it

5 <https://code.edge.launchpad.net/~mbp/bzr/cleanup/+merge/14941> - my
patch, needs tweaks and then landing

6 <https://code.edge.launchpad.net/~benoit.pierre/bzr/fix-error-debug-flag/+merge/15055>
- genuinely approved, needs landing

now into "requested reviews I can do"

7 <https://code.edge.launchpad.net/~vila/bzr/releasing-clarified/+merge/10854>
apparently actually ok to land?

8 <https://code.edge.launchpad.net/~vila/bzr/353370-notty-no-term-width/+merge/13832>
does need a review from me

9 <https://code.edge.launchpad.net/~amanica/bzr/325618_log_returns_too_much/+merge/14275>
-- seems to have had a final review from igc and to be waiting for a
reply from marius

10 <https://code.edge.launchpad.net/~jameinel/bzr/2.1.0b4-builder-no-keys/+merge/14890>
- still needs review but robert's already looking into it

11 <https://code.edge.launchpad.net/~doxxx/bzr/quiet-serve/+merge/15112>
robert has approved it and promised to shepherd it but wants review
from someone else

12 <https://code.edge.launchpad.net/~mwhudson/bzr/bytestring-environment-variables/+merge/15136>
ditto

I could continue, but from this sample I can draw some conclusions:

 * there's no clear distinction between reviews where my input is
needed and those where it's not
 * there's no clear distinction between mps I could land immediately,
mps I could tweak and land, and mps that need major work
 * it's not clear which mps already have somebody responsible for them or not

Some of these, like #7, could in principle be addressed by people just
using the system better - in this case, making it easy to set the
overall status at the same time as you add your vote (for which there
is a bug.)  Some of them, like indicating "I want more reviewers" vs
"this needs more review but not more people" or "$person is looking
after this this" are more open ended and not representable at the
moment.

>>> The thing I love about LP reviews is that it works so nicely across
>>> every project on LP. The thing I dislike about it is that BundleBuggy
>>> was a better queue manager for bzr core reviews.
>
> I *think* this is because BundleBuggy encoded Bazaar-specific policy
> into its behaviour. I bet that someone could hack up a client-side
> thingy that would get you most of the way there.

No, I disagree, BB had some bzr specific policy but it also had better
workflow in general - and even more could have been done.
.
>> I agree, BB had somewhat better workflow by being quite clear about
>> what things you could/should act on next for each type of work.
>>
>
> What do you mean by "type of work"? Can you please give some examples?
>
>> I believe Tim is working on lp reviews so he might appreciate some
>> more feedback.  Or he might already know it and just not have got
>> around to fixing it yet.
>
> Although I can't speak for Tim, I am personally interested in finding
> out more and moving from these concerns to more concrete bug reports
> and then to actual fixes.

There are already quite a few bug reports about this, but I know that
for UI stuff individual bugs filed over time can become quite diffuse.

-- 
Martin <http://launchpad.net/~mbp/>