← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~cjwatson/launchpad/git-ref-commits into lp:launchpad

 

Colin Watson has proposed merging lp:~cjwatson/launchpad/git-ref-commits into lp:launchpad.

Commit message:
Show recent commits on GitRef:+index.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-ref-commits/+merge/294631

Now that GitHostingClient honours the request timeout, we can implement a long-discussed idea for how to show recent commits on Git branches: rather than the extremely expensive way this is implemented for Bazaar, Git is fast enough that we should be able to just request log information from the turnip API when we need it and stash the answer in memcached.

This will of course put quite a bit more load on memcached than we're used to, since we previously only used it to cache recent blog posts on the home page.  I think it's nevertheless the right thing to do: an LRU cache is the right property for this kind of thing.  This arrangement does mean that potentially GitRef:+index might fail in some cases where it previously worked, for example if turnip is under heavy load or being upgraded, but I think it's worth the new coupling.  Since it honours the request timeout, the worst case should be confined to the pages that use this.

In case you, like me, wondered about transporting non-UTF-8 commit messages through JSON, I tested this.  In an unusual display of thoughtful UX, Git in fact makes it very difficult to create commit messages whose encoding it doesn't know: in almost all cases it tries to add an encoding header to the commit, and libgit2/pygit2 uses that to give us UTF-8.  I had to work quite hard to construct a situation where it couldn't manage to do this: I ended up having to use git hash-object.  But even if you do construct such a situation, then libgit2/pygit2 does the equivalent of .encode(error="replace"), replacing characters it can't encode as UTF-8 with U+FFFD, and these make it all the way through to the web UI without problems.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-ref-commits into lp:launchpad.
=== modified file 'lib/lp/code/browser/gitref.py'
--- lib/lp/code/browser/gitref.py	2016-01-25 12:21:31 +0000
+++ lib/lp/code/browser/gitref.py	2016-05-13 11:37:00 +0000
@@ -12,6 +12,7 @@
     ]
 
 import json
+from urllib import quote_plus
 
 from lazr.restful.interface import copy_field
 from zope.component import getUtility
@@ -65,8 +66,13 @@
     usedfor = IGitRef
     facet = 'branches'
     links = [
-        'create_recipe', 'create_snap', 'register_merge', 'source',
-        'view_recipes']
+        'browse_commits',
+        'create_recipe',
+        'create_snap',
+        'register_merge',
+        'source',
+        'view_recipes',
+        ]
 
     def source(self):
         """Return a link to the branch's browsing interface."""
@@ -74,6 +80,14 @@
         url = self.context.getCodebrowseUrl()
         return Link(url, text, icon="info")
 
+    def browse_commits(self):
+        """Return a link to the branch's commit log."""
+        text = "All commits"
+        url = "%s/log/?h=%s" % (
+            self.context.repository.getCodebrowseUrl(),
+            quote_plus(self.context.name))
+        return Link(url, text)
+
     def register_merge(self):
         text = 'Propose for merging'
         enabled = self.context.namespace.supports_merge_proposals
@@ -104,15 +118,6 @@
         return check_permission("launchpad.Edit", self.context)
 
     @property
-    def tip_commit_info(self):
-        return {
-            "sha1": self.context.commit_sha1,
-            "author": self.context.author,
-            "author_date": self.context.author_date,
-            "commit_message": self.context.commit_message,
-            }
-
-    @property
     def show_merge_links(self):
         """Return whether or not merge proposal links should be shown.
 
@@ -162,6 +167,10 @@
     def dependent_landing_count_text(self):
         return self._getBranchCountText(len(self.dependent_landings))
 
+    @cachedproperty
+    def commit_infos(self):
+        return self.context.getLatestCommits()
+
     @property
     def recipes_link(self):
         """A link to recipes for this reference."""

=== modified file 'lib/lp/code/browser/tests/test_gitref.py'
--- lib/lp/code/browser/tests/test_gitref.py	2015-09-18 15:41:08 +0000
+++ lib/lp/code/browser/tests/test_gitref.py	2016-05-13 11:37:00 +0000
@@ -1,24 +1,47 @@
-# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for GitRefView."""
 
 __metaclass__ = type
 
+from datetime import datetime
+import hashlib
 import re
 
+import pytz
 import soupmatchers
 from zope.component import getUtility
 
+from lp.code.interfaces.githosting import IGitHostingClient
 from lp.code.interfaces.gitrepository import IGitRepositorySet
-from lp.testing import BrowserTestCase
-from lp.testing.layers import DatabaseFunctionalLayer
-from lp.testing.views import create_view
+from lp.testing import (
+    admin_logged_in,
+    BrowserTestCase,
+    )
+from lp.testing.fakemethod import FakeMethod
+from lp.testing.fixture import ZopeUtilityFixture
+from lp.testing.layers import LaunchpadFunctionalLayer
+from lp.testing.pages import (
+    extract_text,
+    find_tags_by_class,
+    )
+from lp.testing.views import (
+    create_initialized_view,
+    create_view,
+    )
 
 
 class TestGitRefView(BrowserTestCase):
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestGitRefView, self).setUp()
+        self.hosting_client = FakeMethod()
+        self.hosting_client.getLog = FakeMethod(result=[])
+        self.useFixture(
+            ZopeUtilityFixture(self.hosting_client, IGitHostingClient))
 
     def test_rendering(self):
         repository = self.factory.makeGitRepository(
@@ -64,3 +87,66 @@
             git clone -b branch https://.*
             git clone -b branch git\+ssh://.*
             """, text)
+
+    def makeCommitLog(self):
+        authors = [self.factory.makePerson() for _ in range(5)]
+        with admin_logged_in():
+            author_emails = [author.preferredemail.email for author in authors]
+        epoch = datetime.fromtimestamp(0, tz=pytz.UTC)
+        dates = [
+            datetime(2015, 1, day + 1, tzinfo=pytz.UTC) for day in range(5)]
+        return [
+            {
+                u"sha1": unicode(hashlib.sha1(str(i)).hexdigest()),
+                u"message": u"Commit %d" % i,
+                u"author": {
+                    u"name": authors[i].display_name,
+                    u"email": author_emails[i],
+                    u"time": int((dates[i] - epoch).total_seconds()),
+                    },
+                u"committer": {
+                    u"name": authors[i].display_name,
+                    u"email": author_emails[i],
+                    u"time": int((dates[i] - epoch).total_seconds()),
+                    },
+                u"parents": [unicode(hashlib.sha1(str(i - 1)).hexdigest())],
+                u"tree": unicode(hashlib.sha1("").hexdigest()),
+                }
+            for i in range(5)]
+
+    def test_recent_commits(self):
+        [ref] = self.factory.makeGitRefs(paths=[u"refs/heads/branch"])
+        log = self.makeCommitLog()
+        self.hosting_client.getLog.result = list(reversed(log))
+        view = create_initialized_view(ref, "+index")
+        expected_texts = list(reversed([
+            "%.7s...\nby\n%s\non 2015-01-%02d" % (
+                log[i]["sha1"], log[i]["author"]["name"], i + 1)
+            for i in range(5)]))
+        details = find_tags_by_class(view(), "commit-details")
+        self.assertEqual(
+            expected_texts, [extract_text(detail) for detail in details])
+        expected_urls = list(reversed([
+            "https://git.launchpad.dev/%s/commit/?id=%s"; % (
+                ref.repository.shortened_path, log[i]["sha1"])
+            for i in range(5)]))
+        self.assertEqual(
+            expected_urls, [detail.a["href"] for detail in details])
+
+    def test_all_commits_link(self):
+        [ref] = self.factory.makeGitRefs(paths=[u"refs/heads/branch"])
+        self.hosting_client.getLog.result = self.makeCommitLog()
+        view = create_initialized_view(ref, "+index")
+        recent_commits_tag = soupmatchers.Tag(
+            'recent commits', 'div', attrs={'id': 'recent-commits'})
+        expected_url = (
+            'https://git.launchpad.dev/%s/log/?h=branch' %
+            ref.repository.shortened_path)
+        self.assertThat(
+            view(),
+            soupmatchers.HTMLContains(
+                soupmatchers.Within(
+                    recent_commits_tag,
+                    soupmatchers.Tag(
+                        'all commits link', 'a', text='All commits',
+                        attrs={'href': expected_url}))))

=== modified file 'lib/lp/code/interfaces/githosting.py'
--- lib/lp/code/interfaces/githosting.py	2016-05-03 15:12:46 +0000
+++ lib/lp/code/interfaces/githosting.py	2016-05-13 11:37:00 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interface for communication with the Git hosting service."""
@@ -55,6 +55,18 @@
             requested commits.  Non-existent commits will be omitted.
         """
 
+    def getLog(path, start, limit=None, stop=None, logger=None):
+        """Get commit log information.
+
+        :param path: Physical path of the repository on the hosting service.
+        :param start: The commit to start listing from.
+        :param limit: If not None, return no more than this many commits.
+        :param stop: If not None, ignore this commit and its ancestors.
+        :param logger: An optional logger.
+        :return: A list of dicts each of which represents a commit from the
+            start commit's history.
+        """
+
     def getMergeDiff(path, base, head, logger=None):
         """Get the merge preview diff between two commits.
 

=== modified file 'lib/lp/code/interfaces/gitref.py'
--- lib/lp/code/interfaces/gitref.py	2016-01-12 03:54:38 +0000
+++ lib/lp/code/interfaces/gitref.py	2016-05-13 11:37:00 +0000
@@ -314,6 +314,24 @@
         "Whether there are recent changes in this repository that have not "
         "yet been scanned.")
 
+    def getCommits(start, limit=None, stop=None,
+                   start_date=None, end_date=None, logger=None):
+        """Get commit information from this reference.
+
+        :param start: The commit to start listing from.
+        :param limit: If not None, return no more than this many commits.
+        :param stop: If not None, ignore this commit and its ancestors.
+        :param start_date: If not None, ignore commits before this date.
+        :param end_date: If not None, ignore commits after this date.
+        :param logger: An optional logger.
+        :return: An iterable of commit information dicts.
+        """
+
+    def getLatestCommits(quantity=10):
+        """Return a specific number of the latest commits in this ref."""
+
+    has_commits = Attribute("Whether this reference has any commits.")
+
 
 class IGitRefBatchNavigator(ITableBatchNavigator):
     pass

=== modified file 'lib/lp/code/model/githosting.py'
--- lib/lp/code/model/githosting.py	2016-05-03 18:42:18 +0000
+++ lib/lp/code/model/githosting.py	2016-05-13 11:37:00 +0000
@@ -108,6 +108,22 @@
                 "Failed to get commit details from Git repository: %s" %
                 unicode(e))
 
+    def getLog(self, path, start, limit=None, stop=None, logger=None):
+        """See `IGitHostingClient`."""
+        try:
+            if logger is not None:
+                logger.info(
+                    "Requesting commit log for %s: "
+                    "start %s, limit %s, stop %s" %
+                    (path, start, limit, stop))
+            return self._get(
+                "/repo/%s/log/%s" % (path, quote(start)),
+                params={"limit": limit, "stop": stop})
+        except Exception as e:
+            raise GitRepositoryScanFault(
+                "Failed to get commit log from Git repository: %s" %
+                unicode(e))
+
     def getMergeDiff(self, path, base, head, prerequisite=None, logger=None):
         """See `IGitHostingClient`."""
         try:

=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py	2016-02-06 02:20:04 +0000
+++ lib/lp/code/model/gitref.py	2016-05-13 11:37:00 +0000
@@ -7,6 +7,7 @@
     'GitRefFrozen',
     ]
 
+import json
 from urllib import quote_plus
 
 from lazr.lifecycle.event import ObjectCreatedEvent
@@ -22,6 +23,7 @@
 from zope.component import getUtility
 from zope.event import notify
 from zope.interface import implementer
+from zope.security.proxy import removeSecurityProxy
 
 from lp.app.errors import NotFoundError
 from lp.code.enums import (
@@ -40,17 +42,20 @@
     BRANCH_MERGE_PROPOSAL_FINAL_STATES,
     )
 from lp.code.interfaces.gitcollection import IAllGitRepositories
+from lp.code.interfaces.githosting import IGitHostingClient
 from lp.code.interfaces.gitref import IGitRef
 from lp.code.model.branchmergeproposal import (
     BranchMergeProposal,
     BranchMergeProposalGetter,
     )
+from lp.services.config import config
 from lp.services.database.bulk import load_related
 from lp.services.database.constants import UTC_NOW
 from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.enumcol import EnumCol
 from lp.services.database.interfaces import IStore
 from lp.services.database.stormbase import StormBase
+from lp.services.memcache.interfaces import IMemcacheClient
 
 
 class GitRefMixin:
@@ -242,6 +247,63 @@
         """See `IGitRef`."""
         return self.repository.pending_writes
 
+    def _getLog(self, start, limit=None, stop=None, logger=None):
+        hosting_client = getUtility(IGitHostingClient)
+        memcache_client = getUtility(IMemcacheClient)
+        path = self.repository.getInternalPath()
+        memcache_key = "%s:git-revisions:%s:%s" % (
+            config.instance_name, path, start)
+        if limit is not None:
+            memcache_key += ":limit=%s" % limit
+        if stop is not None:
+            memcache_key += ":stop=%s" % stop
+        if isinstance(memcache_key, unicode):
+            memcache_key = memcache_key.encode("UTF-8")
+        log = None
+        cached_log = memcache_client.get(memcache_key)
+        if cached_log is not None:
+            try:
+                log = json.loads(cached_log)
+            except Exception as e:
+                logger.info(
+                    "Cannot load cached log information for %s:%s (%s); "
+                    "deleting" % (path, start, str(e)))
+                memcache_client.delete(memcache_key)
+        if log is None:
+            log = removeSecurityProxy(hosting_client.getLog(
+                path, start, limit=limit, stop=stop, logger=logger))
+            memcache_client.set(memcache_key, json.dumps(log))
+        return log
+
+    def getCommits(self, start, limit=None, stop=None,
+                   start_date=None, end_date=None, logger=None):
+        # Circular import.
+        from lp.code.model.gitrepository import parse_git_commits
+
+        log = self._getLog(start, limit=limit, stop=stop, logger=logger)
+        parsed_commits = parse_git_commits(log)
+        revisions = []
+        for commit in log:
+            if "sha1" not in commit:
+                continue
+            parsed_commit = parsed_commits[commit["sha1"]]
+            author_date = parsed_commit.get("author_date")
+            if start_date is not None:
+                if author_date is None or author_date < start_date:
+                    continue
+            if end_date is not None:
+                if author_date is None or author_date > end_date:
+                    continue
+            revisions.append(parsed_commit)
+        return revisions
+
+    def getLatestCommits(self, quantity=10):
+        return self.getCommits(self.commit_sha1, limit=quantity)
+
+    @property
+    def has_commits(self):
+        return len(self.getLatestCommits())
+
     @property
     def recipes(self):
         """See `IHasRecipes`."""

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2016-05-05 07:22:32 +0000
+++ lib/lp/code/model/gitrepository.py	2016-05-13 11:37:00 +0000
@@ -6,6 +6,7 @@
     'get_git_repository_privacy_filter',
     'GitRepository',
     'GitRepositorySet',
+    'parse_git_commits',
     ]
 
 from datetime import datetime
@@ -161,6 +162,57 @@
     }
 
 
+def parse_git_commits(commits):
+    """Parse commit information returned by turnip.
+
+    :param commits: A list of turnip-formatted commit object dicts.
+    :return: A dict mapping sha1 identifiers of commits to parsed commit
+        dicts: keys may include "sha1", "author_date", "author_addr",
+        "author", "committer_date", "committer_addr", "committer", and
+        "commit_message".
+    """
+    parsed = {}
+    authors_to_acquire = []
+    committers_to_acquire = []
+    for commit in commits:
+        if "sha1" not in commit:
+            continue
+        info = {"sha1": commit["sha1"]}
+        author = commit.get("author")
+        if author is not None:
+            if "time" in author:
+                info["author_date"] = datetime.fromtimestamp(
+                    author["time"], tz=pytz.UTC)
+            if "name" in author and "email" in author:
+                author_addr = email.utils.formataddr(
+                    (author["name"], author["email"]))
+                info["author_addr"] = author_addr
+                authors_to_acquire.append(author_addr)
+        committer = commit.get("committer")
+        if committer is not None:
+            if "time" in committer:
+                info["committer_date"] = datetime.fromtimestamp(
+                    committer["time"], tz=pytz.UTC)
+            if "name" in committer and "email" in committer:
+                committer_addr = email.utils.formataddr(
+                    (committer["name"], committer["email"]))
+                info["committer_addr"] = committer_addr
+                committers_to_acquire.append(committer_addr)
+        if "message" in commit:
+            info["commit_message"] = commit["message"]
+        parsed[commit["sha1"]] = info
+    revision_authors = getUtility(IRevisionSet).acquireRevisionAuthors(
+        authors_to_acquire + committers_to_acquire)
+    for info in parsed.values():
+        author = revision_authors.get(info.get("author_addr"))
+        if author is not None:
+            info["author"] = author
+        committer = revision_authors.get(info.get("committer_addr"))
+        if committer is not None:
+            info["committer"] = committer
+    return parsed
+
+
 def git_repository_modified(repository, event):
     """Update the date_last_modified property when a GitRepository is modified.
 
@@ -520,8 +572,10 @@
         columns = [getattr(GitRef, name) for name in column_names]
         values = [
             (self.id, path, info["sha1"], info["type"],
-             info.get("author"), info.get("author_date"),
-             info.get("committer"), info.get("committer_date"),
+             info["author"].id if "author" in info else None,
+             info.get("author_date"),
+             info["committer"].id if "committer" in info else None,
+             info.get("committer_date"),
              info.get("commit_message"))
             for path, info in refs_info.items()]
         db_values = dbify_values(values)
@@ -602,47 +656,13 @@
     def fetchRefCommits(hosting_path, refs, logger=None):
         """See `IGitRepository`."""
         oids = sorted(set(info["sha1"] for info in refs.values()))
-        commits = {
-            commit.get("sha1"): commit
-            for commit in getUtility(IGitHostingClient).getCommits(
-                hosting_path, oids, logger=logger)}
-        authors_to_acquire = []
-        committers_to_acquire = []
+        commits = parse_git_commits(
+            getUtility(IGitHostingClient).getCommits(
+                hosting_path, oids, logger=logger))
         for info in refs.values():
             commit = commits.get(info["sha1"])
-            if commit is None:
-                continue
-            author = commit.get("author")
-            if author is not None:
-                if "time" in author:
-                    info["author_date"] = datetime.fromtimestamp(
-                        author["time"], tz=pytz.UTC)
-                if "name" in author and "email" in author:
-                    author_addr = email.utils.formataddr(
-                        (author["name"], author["email"]))
-                    info["author_addr"] = author_addr
-                    authors_to_acquire.append(author_addr)
-            committer = commit.get("committer")
-            if committer is not None:
-                if "time" in committer:
-                    info["committer_date"] = datetime.fromtimestamp(
-                        committer["time"], tz=pytz.UTC)
-                if "name" in committer and "email" in committer:
-                    committer_addr = email.utils.formataddr(
-                        (committer["name"], committer["email"]))
-                    info["committer_addr"] = committer_addr
-                    committers_to_acquire.append(committer_addr)
-            if "message" in commit:
-                info["commit_message"] = commit["message"]
-        revision_authors = getUtility(IRevisionSet).acquireRevisionAuthors(
-            authors_to_acquire + committers_to_acquire)
-        for info in refs.values():
-            author = revision_authors.get(info.get("author_addr"))
-            if author is not None:
-                info["author"] = author.id
-            committer = revision_authors.get(info.get("committer_addr"))
-            if committer is not None:
-                info["committer"] = committer.id
+            if commit is not None:
+                info.update(commit)
 
     def synchroniseRefs(self, refs_to_upsert, refs_to_remove, logger=None):
         """See `IGitRepository`."""

=== modified file 'lib/lp/code/model/tests/test_githosting.py'
--- lib/lp/code/model/tests/test_githosting.py	2016-05-06 09:28:28 +0000
+++ lib/lp/code/model/tests/test_githosting.py	2016-05-13 11:37:00 +0000
@@ -158,6 +158,28 @@
                 "Bad request",
                 self.client.getCommits, "123", ["0"])
 
+    def test_getLog(self):
+        with self.mockRequests(content=b'[{"sha1": "0"}]'):
+            log = self.client.getLog("123", "refs/heads/master")
+        self.assertEqual([{"sha1": "0"}], log)
+        self.assertRequest("repo/123/log/refs/heads/master", method="GET")
+
+    def test_getLog_limit_stop(self):
+        with self.mockRequests(content=b'[{"sha1": "0"}]'):
+            log = self.client.getLog(
+                "123", "refs/heads/master", limit=10, stop="refs/heads/old")
+        self.assertEqual([{"sha1": "0"}], log)
+        self.assertRequest(
+            "repo/123/log/refs/heads/master?limit=10&stop=refs%2Fheads%2Fold",
+            method="GET")
+
+    def test_getLog_failure(self):
+        with self.mockRequests(status_code=400, content=b"Bad request"):
+            self.assertRaisesWithContent(
+                GitRepositoryScanFault,
+                "Failed to get commit log from Git repository: Bad request",
+                self.client.getLog, "123", "refs/heads/master")
+
     def test_getMergeDiff(self):
         with self.mockRequests(content=b'{"patch": ""}'):
             diff = self.client.getMergeDiff("123", "a", "b")

=== modified file 'lib/lp/code/model/tests/test_gitjob.py'
--- lib/lp/code/model/tests/test_gitjob.py	2016-03-02 21:21:26 +0000
+++ lib/lp/code/model/tests/test_gitjob.py	2016-05-13 11:37:00 +0000
@@ -119,7 +119,7 @@
         epoch = datetime.fromtimestamp(0, tz=pytz.UTC)
         dates = {path: next(author_date_gen) for path in paths}
         return [{
-            "sha1": hashlib.sha1(path).hexdigest(),
+            "sha1": unicode(hashlib.sha1(path).hexdigest()),
             "message": "tip of %s" % path,
             "author": {
                 "name": author.displayname,
@@ -132,7 +132,7 @@
                 "time": int((dates[path] - epoch).total_seconds()),
                 },
             "parents": [],
-            "tree": hashlib.sha1("").hexdigest(),
+            "tree": unicode(hashlib.sha1("").hexdigest()),
             } for path in paths]
 
     def assertRefsMatch(self, refs, repository, paths):

=== modified file 'lib/lp/code/model/tests/test_gitref.py'
--- lib/lp/code/model/tests/test_gitref.py	2015-10-12 12:58:32 +0000
+++ lib/lp/code/model/tests/test_gitref.py	2016-05-13 11:37:00 +0000
@@ -1,26 +1,48 @@
-# Copyright 2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for Git references."""
 
 __metaclass__ = type
 
+from datetime import (
+    datetime,
+    timedelta,
+    )
 import hashlib
+import json
 
-from testtools.matchers import EndsWith
+import pytz
+from testtools.matchers import (
+    ContainsDict,
+    EndsWith,
+    Equals,
+    MatchesListwise,
+    MatchesStructure,
+    )
+from zope.component import getUtility
 
 from lp.app.enums import InformationType
 from lp.app.interfaces.informationtype import IInformationType
 from lp.app.interfaces.launchpad import IPrivacy
+from lp.code.interfaces.githosting import IGitHostingClient
+from lp.services.config import config
+from lp.services.memcache.interfaces import IMemcacheClient
 from lp.services.webapp.interfaces import OAuthPermission
 from lp.testing import (
+    admin_logged_in,
     ANONYMOUS,
     api_url,
     person_logged_in,
     TestCaseWithFactory,
     verifyObject,
     )
-from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.fakemethod import FakeMethod
+from lp.testing.fixture import ZopeUtilityFixture
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
 from lp.testing.pages import webservice_for_person
 
 
@@ -65,6 +87,132 @@
         self.assertEqual(InformationType.USERDATA, ref.information_type)
 
 
+class TestGitRefGetCommits(TestCaseWithFactory):
+    """Tests for retrieving commit information from a Git reference."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestGitRefGetCommits, self).setUp()
+        [self.ref] = self.factory.makeGitRefs()
+        self.authors = [self.factory.makePerson() for _ in range(2)]
+        with admin_logged_in():
+            self.author_emails = [
+                author.preferredemail.email for author in self.authors]
+        epoch = datetime.fromtimestamp(0, tz=pytz.UTC)
+        self.dates = [
+            datetime(2015, 1, 1, 0, 0, 0, tzinfo=pytz.UTC),
+            datetime(2015, 1, 2, 0, 0, 0, tzinfo=pytz.UTC),
+            ]
+        self.sha1_tip = unicode(hashlib.sha1("tip").hexdigest())
+        self.sha1_root = unicode(hashlib.sha1("root").hexdigest())
+        self.log = [
+            {
+                u"sha1": self.sha1_tip,
+                u"message": u"tip",
+                u"author": {
+                    u"name": self.authors[0].display_name,
+                    u"email": self.author_emails[0],
+                    u"time": int((self.dates[1] - epoch).total_seconds()),
+                    },
+                u"committer": {
+                    u"name": self.authors[1].display_name,
+                    u"email": self.author_emails[1],
+                    u"time": int((self.dates[1] - epoch).total_seconds()),
+                    },
+                u"parents": [self.sha1_root],
+                u"tree": unicode(hashlib.sha1("").hexdigest()),
+                },
+            {
+                u"sha1": self.sha1_root,
+                u"message": u"root",
+                u"author": {
+                    u"name": self.authors[1].display_name,
+                    u"email": self.author_emails[1],
+                    u"time": int((self.dates[0] - epoch).total_seconds()),
+                    },
+                u"committer": {
+                    u"name": self.authors[0].display_name,
+                    u"email": self.author_emails[0],
+                    u"time": int((self.dates[0] - epoch).total_seconds()),
+                    },
+                u"parents": [],
+                u"tree": unicode(hashlib.sha1("").hexdigest()),
+                },
+            ]
+        self.hosting_client = FakeMethod()
+        self.hosting_client.getLog = FakeMethod(result=self.log)
+        self.useFixture(
+            ZopeUtilityFixture(self.hosting_client, IGitHostingClient))
+
+    def test_basic(self):
+        revisions = self.ref.getCommits(self.sha1_tip)
+        path = self.ref.repository.getInternalPath()
+        self.assertEqual(
+            [((path, self.sha1_tip),
+              {"limit": None, "stop": None, "logger": None})],
+            self.hosting_client.getLog.calls)
+        self.assertThat(revisions, MatchesListwise([
+            ContainsDict({
+                "sha1": Equals(self.sha1_tip),
+                "author": MatchesStructure.byEquality(person=self.authors[0]),
+                "author_date": Equals(self.dates[1]),
+                "commit_message": Equals(u"tip"),
+                }),
+            ContainsDict({
+                "sha1": Equals(self.sha1_root),
+                "author": MatchesStructure.byEquality(person=self.authors[1]),
+                "author_date": Equals(self.dates[0]),
+                "commit_message": Equals(u"root"),
+                }),
+            ]))
+        key = u"%s:git-revisions:%s:%s" % (
+            config.instance_name, path, self.sha1_tip)
+        self.assertEqual(
+            json.dumps(self.log),
+            getUtility(IMemcacheClient).get(key.encode("UTF-8")))
+
+    def test_cache(self):
+        path = self.ref.repository.getInternalPath()
+        key = u"%s:git-revisions:%s:%s" % (
+            config.instance_name, path, self.sha1_tip)
+        getUtility(IMemcacheClient).set(key.encode("UTF-8"), "[]")
+        self.assertEqual([], self.ref.getCommits(self.sha1_tip))
+
+    def test_limit_stop(self):
+        self.ref.getCommits(self.sha1_tip, limit=10, stop=self.sha1_root)
+        path = self.ref.repository.getInternalPath()
+        self.assertEqual(
+            [((path, self.sha1_tip),
+              {"limit": 10, "stop": self.sha1_root, "logger": None})],
+            self.hosting_client.getLog.calls)
+        key = u"%s:git-revisions:%s:%s:limit=10:stop=%s" % (
+            config.instance_name, path, self.sha1_tip, self.sha1_root)
+        self.assertEqual(
+            json.dumps(self.log),
+            getUtility(IMemcacheClient).get(key.encode("UTF-8")))
+
+    def test_start_date(self):
+        revisions = self.ref.getCommits(
+            self.sha1_tip, start_date=(self.dates[1] - timedelta(seconds=1)))
+        path = self.ref.repository.getInternalPath()
+        self.assertThat(revisions, MatchesListwise([
+            ContainsDict({"sha1": Equals(self.sha1_tip)}),
+            ]))
+        key = u"%s:git-revisions:%s:%s" % (
+            config.instance_name, path, self.sha1_tip)
+        self.assertEqual(
+            json.dumps(self.log),
+            getUtility(IMemcacheClient).get(key.encode("UTF-8")))
+
+    def test_end_date(self):
+        revisions = self.ref.getCommits(
+            self.sha1_tip, end_date=(self.dates[1] - timedelta(seconds=1)))
+        self.assertThat(revisions, MatchesListwise([
+            ContainsDict({"sha1": Equals(self.sha1_root)}),
+            ]))
+
+
 class TestGitRefWebservice(TestCaseWithFactory):
     """Tests for the webservice."""
 

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2016-05-05 07:22:32 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2016-05-13 11:37:00 +0000
@@ -1265,10 +1265,10 @@
             u"refs/heads/master": {
                 u"sha1": master_sha1,
                 u"type": GitObjectType.COMMIT,
-                u"author": expected_author.id,
+                u"author": expected_author,
                 u"author_addr": expected_author_addr,
                 u"author_date": author_date,
-                u"committer": expected_committer.id,
+                u"committer": expected_committer,
                 u"committer_addr": expected_committer_addr,
                 u"committer_date": committer_date,
                 u"commit_message": u"tip of master",

=== modified file 'lib/lp/code/templates/git-macros.pt'
--- lib/lp/code/templates/git-macros.pt	2015-09-18 15:41:08 +0000
+++ lib/lp/code/templates/git-macros.pt	2016-05-13 11:37:00 +0000
@@ -98,6 +98,27 @@
 
 </div>
 
+<metal:ref-commits define-macro="ref-commits">
+
+  <tal:comment condition="nothing">
+    This macro requires the following defined variables:
+      ref - the ref that has the revisions
+      commit_infos - a list of dicts of commit information (sha1, author,
+                     author_date, commit_message)
+  </tal:comment>
+  <style type="text/css">
+    .subordinate {
+      margin-left: 1em;
+    }
+  </style>
+  <dl class="commit">
+    <tal:commit repeat="commit_info commit_infos">
+      <metal:commit-text use-macro="ref/@@+macros/commit-text"/>
+    </tal:commit>
+  </dl>
+
+</metal:ref-commits>
+
 <metal:commit-text define-macro="commit-text">
 
   <tal:comment condition="nothing">
@@ -113,7 +134,7 @@
         sha1 python:commit_info['sha1'];
         author python:commit_info['author'];
         author_date python:commit_info['author_date']">
-    <a tal:attributes="href string:${context/repository/getCodebrowseUrl}/commit/?id=${sha1}"
+    <a tal:attributes="href python: context.getCodebrowseUrlForRevision(sha1)"
        tal:content="sha1/fmt:shorten/10" />
     by
     <tal:known-person condition="author/person">

=== modified file 'lib/lp/code/templates/gitref-commits.pt'
--- lib/lp/code/templates/gitref-commits.pt	2015-03-19 17:04:22 +0000
+++ lib/lp/code/templates/gitref-commits.pt	2016-05-13 11:37:00 +0000
@@ -1,27 +1,25 @@
 <div
   xmlns:tal="http://xml.zope.org/namespaces/tal";
   xmlns:metal="http://xml.zope.org/namespaces/metal";
-  xmlns:i18n="http://xml.zope.org/namespaces/i18n";>
+  xmlns:i18n="http://xml.zope.org/namespaces/i18n";
+  tal:define="context_menu view/context/menu:context">
 
-  <p tal:condition="not: context/committer_date">
+  <p tal:condition="not: context/has_commits">
     <metal:no-commit-message use-macro="context/@@+macros/no-commit-message" />
   </p>
 
-  <tal:has_commit condition="context/committer_date">
-    <style type="text/css">
-      .subordinate {
-        margin-left: 1em;
-      }
-    </style>
-    <dl class="revision">
-      <tal:comment condition="nothing">
-        XXX cjwatson 2015-03-19: This should display more commits once we
-        have them.  For now, we only have the tip commit.
-      </tal:comment>
-      <tal:tip define="commit_info view/tip_commit_info">
-        <metal:commit-text use-macro="context/@@+macros/commit-text" />
-      </tal:tip>
-    </dl>
-  </tal:has_commit>
+  <tal:history-available condition="context/has_commits"
+                         define="ref view/context;
+                                 commit_infos view/commit_infos">
+    <metal:landing-target use-macro="ref/@@+macros/ref-commits"/>
+
+    <div tal:define="link context_menu/browse_commits">
+      &raquo;
+      <span class="actions" tal:content="structure link/render">
+        All commits
+      </span>
+    </div>
+
+  </tal:history-available>
 
 </div>


Follow ups