launchpad-reviewers team mailing list archive

[Jelmer Vernooij <jelmer@xxxxxxxxx>]

Jelmer Vernooij has proposed merging lp:~jelmer/launchpad/branchjob-cache-authors into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jelmer/launchpad/branchjob-cache-authors/+merge/92470

When processing new branches, don't repeatedly look up author display names for authors we've already seen.

This should help a bit towards fixing bug 808930.
-- 
https://code.launchpad.net/~jelmer/launchpad/branchjob-cache-authors/+merge/92470
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jelmer/launchpad/branchjob-cache-authors into lp:launchpad.

=== modified file 'lib/lp/code/model/branchjob.py'
--- lib/lp/code/model/branchjob.py	2011-12-30 06:14:56 +0000
+++ lib/lp/code/model/branchjob.py	2012-02-10 13:46:47 +0000
@@ -464,6 +464,7 @@
         super(RevisionsAddedJob, self).__init__(context)
         self._bzr_branch = None
         self._tree_cache = {}
+        self._author_display_cache = {}
 
     @property
     def bzr_branch(self):
@@ -644,6 +645,27 @@
             [proposal for proposal, date_created in proposals.itervalues()],
             key=operator.attrgetter('date_created'), reverse=True)
 
+    def getAuthorDisplayName(self, author):
+        """Get the display name for an author.
+
+        This caches the result.
+
+        :param author: Author text as found in the bzr revision
+        :return: Prettified author name
+        """
+        try:
+            return self._author_display_cache[author]
+        except KeyError:
+            pass
+        revision_set = RevisionSet()
+        rev_author = revision_set.acquireRevisionAuthor(author)
+        if rev_author.person is None:
+            displayname = rev_author.name
+        else:
+            displayname = rev_author.person.unique_displayname
+        self._author_display_cache[author] = displayname
+        return displayname
+
     def getRevisionMessage(self, revision_id, revno):
         """Return the log message for a revision.
 
@@ -655,18 +677,11 @@
         try:
             graph = self.bzr_branch.repository.get_graph()
             merged_revisions = self.getMergedRevisionIDs(revision_id, graph)
+            outf = StringIO()
             authors = self.getAuthors(merged_revisions, graph)
-            revision_set = RevisionSet()
-            rev_authors = set(revision_set.acquireRevisionAuthor(author) for
-                              author in authors)
-            outf = StringIO()
-            pretty_authors = []
-            for rev_author in rev_authors:
-                if rev_author.person is None:
-                    displayname = rev_author.name
-                else:
-                    displayname = rev_author.person.unique_displayname
-                pretty_authors.append('  %s' % displayname)
+            pretty_authors = list(set([
+                '  %s' % self.getAuthorDisplayName(author) for author in
+                authors]))
 
             if len(pretty_authors) > 0:
                 outf.write('Merge authors:\n')

=== modified file 'lib/lp/code/model/tests/test_branchjob.py'
--- lib/lp/code/model/tests/test_branchjob.py	2012-01-24 12:36:15 +0000
+++ lib/lp/code/model/tests/test_branchjob.py	2012-02-10 13:46:47 +0000
@@ -24,6 +24,7 @@
 import pytz
 from sqlobject import SQLObjectNotFound
 from storm.locals import Store
+from testtools.matchers import Equals
 import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -72,7 +73,10 @@
 from lp.services.job.runner import JobRunner
 from lp.services.osutils import override_environ
 from lp.services.webapp import canonical_url
-from lp.testing import TestCaseWithFactory
+from lp.testing import (
+    StormStatementRecorder,
+    TestCaseWithFactory,
+    )
 from lp.testing.dbuser import (
     dbuser,
     switch_dbuser,
@@ -83,6 +87,7 @@
     )
 from lp.testing.librarianhelpers import get_newest_librarian_file
 from lp.testing.mail_helpers import pop_notifications
+from lp.testing.matchers import HasQueryCount
 from lp.translations.enums import RosettaImportStatus
 from lp.translations.interfaces.translationimportqueue import (
     ITranslationImportQueue,
@@ -426,6 +431,21 @@
             tree.unlock()
         return branch, tree
 
+    def test_getAuthorDisplayName(self):
+        """getAuthorDisplayName lookups the display name for an author."""
+        self.useBzrBranches(direct_database=True)
+        branch, tree = self.create3CommitsBranch()
+        job = RevisionsAddedJob.create(branch, 'rev1', 'rev2', '')
+        job.bzr_branch.lock_read()
+        self.addCleanup(job.bzr_branch.unlock)
+        self.assertEquals("Somebody <foo@xxxxxxxxxxx>",
+            job.getAuthorDisplayName("Somebody <foo@xxxxxxxxxxx>"))
+        # The next call should be cached
+        with StormStatementRecorder() as recorder:
+            self.assertEquals("Somebody <foo@xxxxxxxxxxx>",
+                job.getAuthorDisplayName("Somebody <foo@xxxxxxxxxxx>"))
+        self.assertThat(recorder, HasQueryCount(Equals(0)))
+
     def test_iterAddedMainline(self):
         """iterAddedMainline iterates through mainline revisions."""
         self.useBzrBranches(direct_database=True)