launchpad-dev team mailing list archive

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

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.

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

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

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).

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.

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

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.

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.

 * 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.

 * 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).

 * We should email out inter-diffs when new revisions are pushed.

I've run out of steam now...

Tim