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 _______________________________________________ Mailing list: https://launchpad.net/~launchpad-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
