Code review comment for lp:~jtv/launchpad/branchrevision

Thanks for a particularly thoughtful and helpful review.

> >=== modified file 'lib/lp/code/interfaces/branchrevision.py'
> >--- lib/lp/code/interfaces/branchrevision.py 2009-06-25 04:06:00 +0000
> >+++ lib/lp/code/interfaces/branchrevision.py 2010-07-13 13:32:27 +0000
> >@@ -21,7 +21,7 @@
> > ancestry of a branch. History revisions have an integer sequence, merged
> > revisions have sequence set to None.
> > """
> >-
> >+ # XXX JeroenVermeulen 2010-07-12: We're dropping this column.
>
>
> I assume you are planning on adding a bug for this XXX.

This was really just a transient development note; I've dropped it so that we don't leave unnecessary cruft lying around.

> >=== modified file 'lib/lp/code/model/branch.py'
> >--- lib/lp/code/model/branch.py 2010-07-09 10:22:32 +0000
> >+++ lib/lp/code/model/branch.py 2010-07-13 13:32:27 +0000

> >+ result = Store.of(self).find(
> >+ BranchRevision,
> >+ BranchRevision.branch_id == self.id,
> >+ BranchRevision.sequence != None)
> >+ return result.order_by(Desc(BranchRevision.sequence))
>
>
> You can prejoin the revision like this. The Revision will be added
> to the cache, and since BranchRevision has the foreign key, it will
> retrieve the Revision from the cache by id, even though the list
> comprehension appears to throw it away.

I used Stuart's prejoin helper from lp.services.database. I cleaned that up a bit as well, and corrected the documentation.

> >@@ -509,7 +510,7 @@
> >
> > def latest_revisions(self, quantity=10):
> > """See `IBranch`."""
> >- return self.revision_history.limit(quantity)
> >+ return self.revision_history.config(limit=quantity)
>
>
> Of course, if you use the prejoin above, you will need access to
> the storm query before stripping out the Revision.

Thankfully the prejoin helper takes care of that!

> >@@ -532,14 +533,13 @@
> >
> > def getRevisionsSince(self, timestamp):
> > """See `IBranch`."""
> >- return BranchRevision.select(
> >- 'Revision.id=BranchRevision.revision AND '
> >- 'BranchRevision.branch = %d AND '
> >- 'BranchRevision.sequence IS NOT NULL AND '
> >- 'Revision.revision_date > %s' %
> >- (self.id, quote(timestamp)),
> >- orderBy='-sequence',
> >- clauseTables=['Revision'])
> >+ result = Store.of(self).using(Revision).find(
> >+ BranchRevision,
> >+ Revision == BranchRevision.revision,
> >+ BranchRevision.branch == self,
> >+ BranchRevision.sequence != None,
> >+ Revision.revision_date > timestamp)
> >+ return result.order_by(Desc(BranchRevision.sequence))
>
> A prejoin would be good here, too.

Done.

> >@@ -860,8 +860,8 @@
> >
> > def getScannerData(self):
> > """See `IBranch`."""
> >- cur = cursor()
> >- cur.execute("""
> >+# XXX JeroenVermeulen 2010-07-12: We're dropping BranchRevision.id.
>
>
> This XXX needs that bug number and some indentation.

Removed as above.

> >+ rows = Store.of(self).execute("""
>
>
> Storm queries can retrieve individual columns also:
> Store.of(self).find(
> (BranchRevision.id, BranchRevision.sequence, Revision.revision_id)

You're right, of course! Fixed.

> >@@ -872,10 +872,12 @@
> > ancestry = set()
> > history = []
> > branch_revision_map = {}
> >- for branch_revision_id, sequence, revision_id in cur.fetchall():
> >+ for branch_revision_id, sequence, revision_id in rows:
> > ancestry.add(revision_id)
> > branch_revision_map[revision_id] = branch_revision_id
> > if sequence is not None:
> >+ assert sequence == len(history) + 1, (
> >+ "Broken revision sequence number.")
>
> I hope a `code` developer is comfortable that this won't cause
> too many oopses.

You're right. It was mostly interesting for an initial extra sanity check during testing. Removed now.

> >=== modified file 'lib/lp/codehosting/scanner/tests/test_bzrsync.py'
> >--- lib/lp/codehosting/scanner/tests/test_bzrsync.py 2010-06-16 19:47:27
> +0000
> >+++ lib/lp/codehosting/scanner/tests/test_bzrsync.py 2010-07-13 13:32:27
> +0000
> >@@ -228,10 +231,11 @@
> > :return: A set of tuples (sequence, revision-id) for all the
> > BranchRevisions rows belonging to self.db_branch.
> > """
> >+ branchrevisions = IStore(BranchRevision).find(
> >+ BranchRevision, BranchRevision.branch == db_branch)
>
> Adding an underscore in `branchrevisions` would be good.
> Instead of pulling out the sequence object and revision_id below, the
> storm query can do it.

Done and done.

> >@@ -459,8 +463,9 @@
> > # retrieveDatabaseAncestry.
> > branch = getUtility(IBranchLookup).getByUniqueName(
> > '~name12/+junk/junk.contrib')
> >- sampledata = list(
> >- BranchRevision.selectBy(branch=branch).orderBy('sequence'))
> >+ branchrevisions = IStore(BranchRevision).find(
> >+ BranchRevision, BranchRevision.branch == branch)
> >+ sampledata = list(branchrevisions.order_by(BranchRevision.sequence))
> > expected_ancestry = set(branch_revision.revision.revision_id
> > for branch_revision in sampledata)
>
> storm can pull out the revision_id instead of using a generator.

In this case that would have required some restructuring, since the test produced several data structures based on the same query result. I could restructure this but I don't think it'll pay off much.

> >=== modified file 'lib/lp/code/model/branch.py'
> >--- lib/lp/code/model/branch.py 2010-07-12 17:15:50 +0000
> >+++ lib/lp/code/model/branch.py 2010-07-13 14:55:59 +0000
> >@@ -533,9 +533,9 @@
> >
> > def getRevisionsSince(self, timestamp):
> > """See `IBranch`."""
> >- result = Store.of(self).using(Revision).find(
> >+ result = Store.of(self).using(BranchRevision, Revision).find(
> > BranchRevision,
>
> This is another candidate for a prejoin, and then you won't need
> the using().

Done.

> >=== modified file 'lib/lp/code/model/branchmergeproposal.py'
> >--- lib/lp/code/model/branchmergeproposal.py 2010-07-12 17:15:50 +0000
> >+++ lib/lp/code/model/branchmergeproposal.py 2010-07-13 14:55:59 +0000

> >- # workflow states that approved leads to).
> >+ # - a key concern is to prevent non reviewers putting things in the
> >+ # queue that haven't been oked (and thus moved to approved or one of
>
> s/oked/okayed/
> or maybe just say "approved".

Perfect. Done.

> >+ target_join = LeftJoin(
> >+ TargetRevision, And(
> >+ TargetRevision.revision_id == SourceRevision.revision_id,
> >+ TargetRevision.branch_id == self.target_branch.id))
> >+ result = store.using(SourceRevision, target_join).find(
>
>
> EDG: This was a little confusing since it's formatted differently than
> we do normally:
> origin = [
> SourceRevision,
> LeftJoin(...)
> ]
> store.using(*origin).find(...)

Done.

> You could also prejoin Revision here.

With respect I skipped this one, so as not to get too aggressive about changes in a performance landscape I'm not too familiar with.

Jeroen