launchpad-dev team mailing list archive

[Martin Pool <mbp@xxxxxxxxxxxxx>]

On 30 June 2010 18:42, Tim Penhey <tim.penhey@xxxxxxxxxxxxx> wrote:
> OK...
>
> Here are my thoughts on the whole thing.
>
> We have several different use-cases that we want to either allow or restrict
> depending on whose point of view you are looking at.

I think use cases are a nice way to tackle this, thanks for posting
them.  I wonder if we can get some mentorship on our practice of them
from a Canonical design nerd?

>
> 1) "oops made a mistake in proposing"
>  - this occurs when the branch is proposed to merge to the wrong target, or
> forgetting to add a dependent branch

I think this is a really important case because when people are first
trying out the system, they probably are not going to make an mp that
does exactly what they want.  If they can tweak it a bit and gradually
get it right that's perhaps more encouraging than needing to throw it
out and start over.

Further, people creating an mp for the first time may also be
proposing a patch to a project for the first time and may feel
especially concerned about making a mistake in public.

If a new contributor does make a mistake, perhaps one of the reviewers
wants to fix it for them, rather than making them resubmit.

Also, even the best of us sometimes mistype/misclick and would like to
quickly correct it.

This 'oops' case is especially salient when the mp is brand new and
not touched by anyone else yet.

>
> 2) "I've broken this work up into multiple bits"
>  - during review, it may be suggested that a change be broken into several
> parts
>
> 3) "finishing off someone else's work"
>  - there are times when a different person may polish a piece of work for
> landing.  More common for community contributed work than a core developer

Interesting case, and not perfectly handled at present because you
don't inherit the comments or state.

> 4) "only approved changes should be landed"
>  - this is a little fluffy as we don't (yet) have merge queue integration with
> either PQM or Tarmac (fully).

I think this is behind Aaron's concern about people voting for X and
getting Y.  (As if that never happens in the real world. ;-)

To me this is an important but somewhat special case: normally a
comment is "I agree with the general thrust of this mp" but at some
times you want to say "I sign off on this very specific change."

Perhaps this means that putting an mp into state Approved locks it.
If someone then wants to make further changes they'd need to put it
back into wip/needs review.

>
> 5) "the review has rambled on and we want to start again"
>  - this situation can occur when there has been a significant conversation
> around a change with possible many changes of direction, and a clean slate is
> wanted to start with the latest agreed changes.

Yes please.  At the moment this can be accomplished by starting a
totally new review and just putting a hyperlink to the old one.  That
works decently but I don't think it works if you want a brand new
review on a branch that already has a review?

>
> 6) "changes have been made post code-review"
>  - sometimes this is allowed in the situations of core-developers making a
> trivial change, and sometimes a re-review is wanted in the situation of a
> bigger change, or a untrusted developer making changes
>
> 7) "wrong approach taken - massive rework needed"
>  - this occurs occasionally, and is being included as a use-case for
> completeness even though it doesn't really match the above

It's a bit like #5 too, and it is reasonably common.  Ideally there
will be bidirectional links.

>
> 8) "accidental implied approval approval of unreviewed code"
>  - this is probably the primary use-case of things we want to avoid.
> This is the use-case we want nice big flashing red lights around.

Related to #4 and not really a use case as such, but more of: I've
approved mp_1, but somebody now changed it's depended-upon mp_2.  (By
changing the contents of its branch or  pointing to a different branch
or whatever.)  What does this mean for my approval?

One solution if we like the idea of locking mps is to say that you can
only approve mp_1 when mp_2 is approved and thereby locked.

>
> 9) "malicious behaviour - attempting to land unreviewed code"
>  - this is something that needs to be considered, but is probably not really
> going to happen very often.
>
> Now... I don't propose to have the best solutions for all use-cases.  My
> general thought is that it is better to allow a user to change an existing
> merge proposal rather than making them delete the current one and propose a
> new one where it makes sense.  I think the big key here is "where it makes
> sense".  One of the things that does get sent out when a new merge proposal is
> created is a new email with a primary diff to review.  Where significant changes
> are made to the branches related to a merge proposal, the entire diff should be
> sent out again.
>
>  * We should make it obvious on the page when extra revisions have been
> pushed, or if the reviewed revision is no longer in the branch.

I really like the timeline you have now, and I think you should build
on it.  If the timeline is rigorous that gives some safety.

>  * Changing the source branch (taking over someone else's work) should result
> in a new merge proposal superseding the older branch, and the conversation
> from the old proposal should be shown in the conversation.

+1

>
>  * Wrong approach should result in a rejected proposal and a new one created
> for the new work.
>
>  * Starting a new conversation should result in a new proposal with the old
> one rejected (not superseded), but perhaps keeping a copy of the description
> (and commit message), and the requested reviews, but setting existing reviews
> to pending (like resubmit does).

So it seems like the user may be aiming for "start a new conversation"
and "change the code under review" - either or both.

>
>  * We should email out inter-diffs when new revisions are pushed.
>
> I've run out of steam now...

-- 
Martin