On 30 June 2010 18:42, Tim Penhey <[email protected]> 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 _______________________________________________ Mailing list: https://launchpad.net/~launchpad-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-dev More help : https://help.launchpad.net/ListHelp
