Launchpad itself

Merge lp://staging/~thumper/launchpad/scanner-awesomeness into lp://staging/launchpad

scanner-awesomeness
Merge into devel

Proposed by Tim Penhey on 2010-04-12

Status:

Merged

Approved by:

Michael Hudson-Doyle on 2010-04-12

Approved revision:

no longer in the source branch.

Merged at revision:

not available

Proposed branch:

lp://staging/~thumper/launchpad/scanner-awesomeness

Merge into:

lp://staging/launchpad

Diff against target:

291 lines (+163/-9)

4 files modified

lib/lp/codehosting/scanner/mergedetection.py (+39/-5)
lib/lp/codehosting/scanner/tests/test_mergedetection.py (+44/-3)
lib/lp/services/tests/test_utils.py (+40/-1)
lib/lp/services/utils.py (+40/-0)

To merge this branch:

bzr merge lp://staging/~thumper/launchpad/scanner-awesomeness

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Michael Hudson-Doyle		2010-04-12	Approve on 2010-04-12
Review via email: mp+23211@code.staging.launchpad.net

Commit message

Record the merged_revno in the scanner.

Description of the change

This branch records the merged revno for branches that have been merged. It only looks for the landing candidates as it uses the merge sorted graph, and we don't want to do that for the possible landing targets.

Since there may be many landing candidates for a series branch, and the merge sorted graph is a non-trivial thing to generate, I added a caching iterator. This allows the code to be written in a more natural way, but without multiple generations of the merge sorted graph.

tests:
  TestFindMergedRevno
  TestCachingIterator
  TestAutoMergeDetectionForMergeProposals

Revision history for this message

Michael Hudson-Doyle (mwhudson) wrote on 2010-04-12:

I pointed out a couple of typos on IRC.

I think a test of CachedIterator that iterated two iterators in parallel like so:

ci = CachedIterator([...])
i1 = iter(ci)
assert i1.next() == ...
i2 = iter(ci)
assert i2.next() == ...
assert i1.next() == ...

I think it'll pass fine, but it's something my devious mind wants to see tested.

CachingIterator needs some blank lines between its members.

Otherwise it all looks nice, and will be a nice change!

review: Approve

Revision history for this message

Andrew Bennetts (spiv) wrote on 2010-04-12:

Michael Hudson wrote:
> Review: Approve
> I pointed out a couple of typos on IRC.
>
> I think a test of CachedIterator that iterated two iterators in parallel like so:
>
> ci = CachedIterator([...])
> i1 = iter(ci)
> assert i1.next() == ...
> i2 = iter(ci)
> assert i2.next() == ...
> assert i1.next() == ...
>
> I think it'll pass fine, but it's something my devious mind wants to see tested.

You need to be more devious than that: the assumption CachedIterator is making
is that its __iter__ will never call itself. You'd think that's a reasonable
assumption, but I've seen code that accidentally does it. The more interacting
layers of code you have, with lazy DB iterators and so forth, the more likely it
is...

And boy is it confusing as hell when it does happen and you get “impossible”
results from apparently simple code.

So, try this:

class EvilIterator(object):
    def __init__(self):
        self.elem_source = iter('abcdef')
    def next(self):
        e = self.elem_source.next()
        if e == 'c':
            print 'evil:', list(ci)
        return e

ci = CachingIterator(EvilIterator())
i1 = iter(ci)
i2 = iter(ci)
for elem in i1:
print '1:', elem
for elem in i2:
print '2:', elem

-Andrew.