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

Proposed by Robert Collins
Status: Merged
Approved by: Tim Penhey
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
Reviewer Review Type Date Requested Status
Tim Penhey (community) Approve
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

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

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 :

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 :

Thanks for the review.

Revision history for this message
Robert Collins (lifeless) wrote :

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 :

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

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.