Launchpad itself

Merge lp://staging/~lifeless/launchpad/merge into lp://staging/launchpad

merge
Merge into devel

Proposed by Robert Collins on 2010-04-16

Status:

Merged

Approved by:

Tim Penhey on 2010-05-03

Approved revision:

no longer in the source branch.

Merged at revision:

10831

Proposed branch:

lp://staging/~lifeless/launchpad/merge

Merge into:

lp://staging/launchpad

Diff against target:

292 lines (+95/-36)

6 files modified

lib/lp/code/configure.zcml (+0/-1)
lib/lp/code/doc/branch-merge-proposals.txt (+1/-1)
lib/lp/code/interfaces/branchmergeproposal.py (+3/-3)
lib/lp/code/model/branchmergeproposal.py (+28/-25)
lib/lp/code/model/tests/test_branchmergeproposals.py (+62/-5)
lib/lp/testing/factory.py (+1/-1)

To merge this branch:

bzr merge lp://staging/~lifeless/launchpad/merge

Related bugs:

Bug #561160: API: 'Code failed to merge' setting doesn't work	Low	Fix Released
Bug #561170: MP.queued_revid not set when setting queue status in API	Low	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Tim Penhey (community)		2010-04-16	Approve on 2010-05-03
Review via email: mp+23523@code.staging.launchpad.net

Commit message

Merge proposal tidy up: always dequeue when leaving the queued state; permit merge-failed; store the reviewed revid when queueing by default.

Description of the change

Fix a few bugs around merge proposals:
- when transitioning out of 'queued', always dequeue.
- permit transitioning to merge-failed
- use the reviewed revid by default when queueing

Revision history for this message

Tim Penhey (thumper) wrote on 2010-04-16:

lib/lp/code/interfaces/branchmergeproposal.py
- change needs indenting a space
actually your indentation is all over the place in the tests too (so says the diff)

setStatus should call mergeFailed, and pass in the revision id

should also have a test where you can call setStatus for queued and pass in a revision id

review: Needs Fixing

Revision history for this message

Robert Collins (lifeless) wrote on 2010-04-16:

Ugh, there are tabs in teh diff. Fail-vim-rc-in-vm. will fix.

There are already tests for pasing in a revid to setStatus(QUEUED, revision_id=).

mergeFailed seems damaged, it uses self.merger, which isn't defined or used anywhere.

If I can change its definition, I can call it - so I'll do that change.

...

Have looked into it, mergeFailed isn't used anywhere, isn't exposed in API's and is buggy today (it doesn't dequeue completely). Its behaviour isn't tested either. To fix it and make it safe to call any point will either make setStatus significantly more complex (have to call dequeue when moving from QUEUED in every case except when moving to MERGE_FAILED), or it will make mergeFailed complex (have to accept being called when not in state QUEUED).

I looked for and could not find docs saying about whether setStatus was being deleted, or the other functions were, or something else, and couldn't find anything, so I took the approach that leaves the least code, with no reduction in capabilities - deleting mergeFailed.

Revision history for this message

Robert Collins (lifeless) wrote on 2010-04-16:

Thanks for the review.

Revision history for this message

Robert Collins (lifeless) wrote on 2010-04-29:

Tim, I've checked for the transition you were concerned about (non reviewer -> merge failed) and tightened that up appropriately, with a test.

Revision history for this message

Tim Penhey (thumper) wrote on 2010-05-03:

This is good enough for now, but I really want to refactor the guts out of this whole area.

review: Approve