Merge lp:~cjwatson/launchpad/git-mp-commits into lp:launchpad

[Colin Watson]

Show unmerged and conversation-relevant Git commits in merge proposal views.

Show unmerged and conversation-relevant Git commits in merge proposal views.

The only significant oddity here is that we're using Git's idea of commit dates rather than when the commit was pushed. This will make sense from an author's point of view, but it will potentially mean that a reviewer sees commits pop up before their review comment. It arguably makes about as much sense as the approach we take with Bazaar, but it's certainly different. However, we can't emulate the Bazaar approach unless we start doing a full branch scanner and creating Revision rows for Git, which we wanted to avoid unless we had no choice, so let's go with this approach.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py	2016-05-07 00:40:18 +0000
+++ lib/lp/code/browser/branchmergeproposal.py	2016-05-18 22:37:48 +0000
@@ -534,9 +534,15 @@
     particular time.
     """
 
-    def __init__(self, revisions, date, branch, diff):
+    def __init__(self, revisions, date, source, diff):
         self.revisions = revisions
-        self.branch = branch
+        self.source = source
+        if IBranch.providedBy(source):
+            self.branch = source
+            self.git_ref = None
+        else:
+            self.branch = None
+            self.git_ref = source
         self.has_body = False
         self.has_footer = True
         # The date attribute is used to sort the comments in the conversation.
@@ -613,7 +619,9 @@
         if IBranch.providedBy(source):
             source = DecoratedBranch(source)
         comments = []
-        if getFeatureFlag('code.incremental_diffs.enabled'):
+        if (getFeatureFlag('code.incremental_diffs.enabled') and
+                merge_proposal.source_branch is not None):
+            # XXX cjwatson 2016-05-09: Implement for Git.
             ranges = [
                 (revisions[0].revision.getLefthandParent(),
                  revisions[-1].revision)
@@ -622,8 +630,12 @@
         else:
             diffs = [None] * len(groups)
         for revisions, diff in zip(groups, diffs):
+            if merge_proposal.source_branch is not None:
+                last_date_created = revisions[-1].revision.date_created
+            else:
+                last_date_created = revisions[-1]["author_date"]
             newrevs = CodeReviewNewRevisions(
-                revisions, revisions[-1].revision.date_created, source, diff)
+                revisions, last_date_created, source, diff)
             comments.append(newrevs)
         while merge_proposal is not None:
             from_superseded = merge_proposal != self.context
 
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py	2016-05-18 12:15:05 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py	2016-05-18 22:37:48 +0000
@@ -11,6 +11,7 @@
     timedelta,
     )
 from difflib import unified_diff
+import hashlib
 import re
 
 from lazr.lifecycle.event import ObjectModifiedEvent
@@ -1031,9 +1032,12 @@
 
 
 class TestBranchMergeProposalRequestReviewViewGit(
-    TestBranchMergeProposalRequestReviewViewMixin, BrowserTestCase):
+    TestBranchMergeProposalRequestReviewViewMixin, GitHostingClientMixin,
+    BrowserTestCase):
     """Test `BranchMergeProposalRequestReviewView` for Git."""
 
+    layer = LaunchpadFunctionalLayer
+
     def makeBranchMergeProposal(self):
         return self.factory.makeBranchMergeProposalForGit()
 
@@ -1261,10 +1265,10 @@
             self.assertEqual('flibble', bmp.superseded_by.description)
 
 
-class TestResubmitBrowserGit(BrowserTestCase):
+class TestResubmitBrowserGit(GitHostingClientMixin, BrowserTestCase):
     """Browser tests for resubmitting branch merge proposals for Git."""
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def test_resubmit_text(self):
         """The text of the resubmit page is as expected."""
@@ -1441,7 +1445,7 @@
             [diff],
             [comment.diff for comment in comments])
 
-    def test_CodeReviewNewRevisions_implements_ICodeReviewNewRevisions(self):
+    def test_CodeReviewNewRevisions_implements_interface_bzr(self):
         # The browser helper class implements its interface.
         review_date = datetime(2009, 9, 10, tzinfo=pytz.UTC)
         revision_date = review_date + timedelta(days=1)
@@ -1454,6 +1458,36 @@
 
         self.assertTrue(verifyObject(ICodeReviewNewRevisions, new_revisions))
 
+    def test_CodeReviewNewRevisions_implements_interface_git(self):
+        # The browser helper class implements its interface.
+        review_date = datetime(2009, 9, 10, tzinfo=pytz.UTC)
+        author = self.factory.makePerson()
+        with person_logged_in(author):
+            author_email = author.preferredemail.email
+        epoch = datetime.fromtimestamp(0, tz=pytz.UTC)
+        review_date = self.factory.getUniqueDate()
+        commit_date = self.factory.getUniqueDate()
+        bmp = self.factory.makeBranchMergeProposalForGit(
+            date_created=review_date)
+        hosting_client = FakeMethod()
+        hosting_client.getLog = FakeMethod(result=[
+            {
+                u'sha1': unicode(hashlib.sha1(b'0').hexdigest()),
+                u'message': u'0',
+                u'author': {
+                    u'name': author.display_name,
+                    u'email': author_email,
+                    u'time': int((commit_date - epoch).total_seconds()),
+                    },
+                }
+            ])
+        self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
+
+        view = create_initialized_view(bmp, '+index')
+        new_commits = view.conversation.comments[0]
+
+        self.assertTrue(verifyObject(ICodeReviewNewRevisions, new_commits))
+
     def test_include_superseded_comments(self):
         for x, time in zip(range(3), time_counter()):
             if x != 0:
@@ -1527,9 +1561,10 @@
         self.assertThat(browser.contents, HTMLContains(expected_meta))
 
 
-class TestBranchMergeProposalBrowserView(BrowserTestCase):
+class TestBranchMergeProposalBrowserView(
+    GitHostingClientMixin, BrowserTestCase):
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def test_prerequisite_bzr(self):
         # A prerequisite branch is rendered in the Bazaar case.
@@ -1727,7 +1762,7 @@
         self.assertEqual('Eric on 2008-09-10', view.status_title)
 
 
-class TestBranchMergeProposal(BrowserTestCase):
+class TestBranchMergeProposal(GitHostingClientMixin, BrowserTestCase):
 
     layer = LaunchpadFunctionalLayer
 
 
=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py	2015-10-19 10:56:16 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py	2016-05-18 22:37:48 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """The interface for branch merge proposals."""
@@ -429,6 +429,9 @@
         source branch that are not in the revision history of the target
         branch.  These are the revisions that have been committed to the
         source branch since it branched off the target branch.
+
+        For Bazaar, this returns a sequence of `BranchRevision` objects.
+        For Git, this returns a sequence of commit information dicts.
         """
 
     def getUsersVoteReference(user):
 
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2015-10-13 17:24:59 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2016-05-18 22:37:48 +0000
@@ -1,7 +1,7 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-"""Database class for branch merge prosals."""
+"""Database class for branch merge proposals."""
 
 __metaclass__ = type
 __all__ = [
@@ -36,10 +36,7 @@
     Int,
     Reference,
     )
-from storm.store import (
-    EmptyResultSet,
-    Store,
-    )
+from storm.store import Store
 from zope.component import getUtility
 from zope.event import notify
 from zope.interface import implementer
@@ -70,8 +67,8 @@
     IBranchMergeProposal,
     IBranchMergeProposalGetter,
     )
-from lp.code.interfaces.branchrevision import IBranchRevision
 from lp.code.interfaces.branchtarget import IHasBranchTarget
+from lp.code.interfaces.codereviewcomment import ICodeReviewComment
 from lp.code.interfaces.codereviewinlinecomment import (
     ICodeReviewInlineCommentSet,
     )
@@ -785,26 +782,38 @@
 
     def getUnlandedSourceBranchRevisions(self):
         """See `IBranchMergeProposal`."""
-        if self.source_branch is None:
-            # XXX cjwatson 2015-04-16: Implement for Git somehow, perhaps by
-            # calling turnip via memcached.
-            return []
-        store = Store.of(self)
-        source = SQL("""source AS (SELECT BranchRevision.branch,
-            BranchRevision.revision, Branchrevision.sequence FROM
-            BranchRevision WHERE BranchRevision.branch = %s and
-            BranchRevision.sequence IS NOT NULL ORDER BY BranchRevision.branch
-            DESC, BranchRevision.sequence DESC
-            LIMIT 10)""" % self.source_branch.id)
-        where = SQL("""BranchRevision.revision NOT IN (SELECT revision from
-            BranchRevision AS target where target.branch = %s and
-            BranchRevision.revision = target.revision)""" %
-            self.target_branch.id)
-        using = SQL("""source as BranchRevision""")
-        revisions = store.with_(source).using(using).find(
-            BranchRevision, where)
-        return list(revisions.order_by(
-            Desc(BranchRevision.sequence)).config(limit=10))
+        if self.source_branch is not None:
+            store = Store.of(self)
+            source = SQL("""
+                source AS (
+                    SELECT
+                        BranchRevision.branch, BranchRevision.revision,
+                        Branchrevision.sequence
+                    FROM BranchRevision
+                    WHERE
+                        BranchRevision.branch = %s
+                        AND BranchRevision.sequence IS NOT NULL
+                    ORDER BY
+                        BranchRevision.branch DESC,
+                        BranchRevision.sequence DESC
+                    LIMIT 10)""" % self.source_branch.id)
+            where = SQL("""
+                BranchRevision.revision NOT IN (
+                    SELECT revision
+                    FROM BranchRevision AS target
+                    WHERE
+                        target.branch = %s
+                        AND BranchRevision.revision = target.revision)""" %
+                self.target_branch.id)
+            using = SQL("""source AS BranchRevision""")
+            revisions = store.with_(source).using(using).find(
+                BranchRevision, where)
+            return list(revisions.order_by(
+                Desc(BranchRevision.sequence)).config(limit=10))
+        else:
+            return self.source_git_ref.getCommits(
+                self.source_git_commit_sha1, limit=10,
+                stop=self.target_git_commit_sha1)
 
     def createComment(self, owner, subject, content=None, vote=None,
                       review_type=None, parent=None, _date_created=DEFAULT,
@@ -1031,34 +1040,39 @@
         return None
 
     def _getNewerRevisions(self):
-        if self.source_branch is None:
-            # XXX cjwatson 2015-04-16: Implement for Git.
-            return EmptyResultSet()
         start_date = self.date_review_requested
         if start_date is None:
             start_date = self.date_created
-        return self.source_branch.getMainlineBranchRevisions(
-            start_date, self.revision_end_date, oldest_first=True)
+        if self.source_branch is not None:
+            revisions = self.source_branch.getMainlineBranchRevisions(
+                start_date, self.revision_end_date, oldest_first=True)
+            return [
+                ((revision.date_created, branch_revision.sequence),
+                 branch_revision)
+                for branch_revision, revision in revisions]
+        else:
+            commits = reversed(self.source_git_ref.getCommits(
+                self.source_git_commit_sha1, stop=self.target_git_commit_sha1,
+                start_date=start_date, end_date=self.revision_end_date))
+            return [
+                ((commit["author_date"], count), commit)
+                for count, commit in enumerate(commits)]
 
     def getRevisionsSinceReviewStart(self):
         """Get the grouped revisions since the review started."""
         entries = [
             ((comment.date_created, -1), comment) for comment
             in self.all_comments]
-        revisions = self._getNewerRevisions()
-        entries.extend(
-            ((revision.date_created, branch_revision.sequence),
-                branch_revision)
-            for branch_revision, revision in revisions)
+        entries.extend(self._getNewerRevisions())
         entries.sort()
         current_group = []
         for sortkey, entry in entries:
-            if IBranchRevision.providedBy(entry):
-                current_group.append(entry)
-            else:
+            if ICodeReviewComment.providedBy(entry):
                 if current_group != []:
                     yield current_group
                     current_group = []
+            else:
+                current_group.append(entry)
         if current_group != []:
             yield current_group
 
 
=== modified file 'lib/lp/code/stories/branches/xx-code-review-comments.txt'
--- lib/lp/code/stories/branches/xx-code-review-comments.txt	2015-10-06 06:48:01 +0000
+++ lib/lp/code/stories/branches/xx-code-review-comments.txt	2016-05-18 22:37:48 +0000
@@ -188,6 +188,56 @@
     4. By ... on 2009-09-12
     and it works!
 
+The same thing works for Git.  Note that the hosting client returns newest
+log entries first.
+
+    >>> from lp.code.interfaces.githosting import IGitHostingClient
+    >>> from lp.testing.fakemethod import FakeMethod
+    >>> from lp.testing.fixture import ZopeUtilityFixture
+
+    >>> login('[e-mail]')
+    >>> bmp = factory.makeBranchMergeProposalForGit()
+    >>> bmp.requestReview(review_date)
+    >>> epoch = datetime.fromtimestamp(0, tz=pytz.UTC)
+    >>> commit_date = review_date + timedelta(days=1)
+    >>> hosting_client = FakeMethod()
+    >>> hosting_client.getLog = FakeMethod(result=[])
+    >>> for i in range(2):
+    ...     hosting_client.getLog.result.insert(0, {
+    ...         u'sha1': unicode(i * 2) * 40,
+    ...         u'message': u'Testing commits in conversation',
+    ...         u'author': {
+    ...             u'name': bmp.registrant.display_name,
+    ...             u'email': bmp.registrant.preferredemail.email,
+    ...             u'time': int((commit_date - epoch).total_seconds()),
+    ...             },
+    ...         })
+    ...     hosting_client.getLog.result.insert(0, {
+    ...         u'sha1': unicode(i * 2 + 1) * 40,
+    ...         u'message': u'and it works!',
+    ...         u'author': {
+    ...             u'name': bmp.registrant.display_name,
+    ...             u'email': bmp.registrant.preferredemail.email,
+    ...             u'time': int((commit_date - epoch).total_seconds()),
+    ...             },
+    ...         })
+    ...     commit_date += timedelta(days=1)
+    >>> url = canonical_url(bmp)
+    >>> logout()
+
+    >>> with ZopeUtilityFixture(hosting_client, IGitHostingClient):
+    ...     browser.open(url)
+    >>> print_tag_with_id(browser.contents, 'conversation')
+    ~.../+git/...:... updated on 2009-09-12 ...
+    0000000... by ... on 2009-09-11
+    Testing commits in conversation
+    1111111... by ... on 2009-09-11
+    and it works!
+    2222222... by ... on 2009-09-12
+    Testing commits in conversation
+    3333333... by ... on 2009-09-12
+    and it works!
+
 
 Inline Comments
 ---------------
 
=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt	2015-04-22 16:11:40 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt	2016-05-18 22:37:48 +0000
@@ -170,6 +170,14 @@
           <p>Recent revisions are not available due to the source branch being remote.</p>
         </tal:remote-branch>
       </tal:bzr-revisions>
+      <tal:git-revisions condition="context/source_git_ref">
+        <tal:history-available condition="context/source_git_ref/has_commits"
+                               define="ref context/source_git_ref;
+                                       commit_infos view/unlanded_revisions">
+          <h2>Unmerged commits</h2>
+          <metal:commits use-macro="ref/@@+macros/ref-commits"/>
+        </tal:history-available>
+      </tal:git-revisions>
     </div>
   </div>
 
 
=== modified file 'lib/lp/code/templates/branchmergeproposal-resubmit.pt'
--- lib/lp/code/templates/branchmergeproposal-resubmit.pt	2015-04-28 16:39:15 +0000
+++ lib/lp/code/templates/branchmergeproposal-resubmit.pt	2016-05-18 22:37:48 +0000
@@ -24,18 +24,28 @@
     </div>
   </div>
 
-  <div id="source-revisions" tal:condition="context/source_branch">
-    <tal:history-available condition="context/source_branch/revision_count"
-                           define="branch context/source_branch;
-                                   revisions view/unlanded_revisions">
-      <h2>Unmerged revisions</h2>
-      <metal:landing-target use-macro="branch/@@+macros/branch-revisions"/>
-    </tal:history-available>
+  <div id="source-revisions">
+    <tal:bzr-revisions condition="context/source_branch">
+      <tal:history-available condition="context/source_branch/revision_count"
+                             define="branch context/source_branch;
+                                     revisions view/unlanded_revisions">
+        <h2>Unmerged revisions</h2>
+        <metal:landing-target use-macro="branch/@@+macros/branch-revisions"/>
+      </tal:history-available>
 
-    <tal:remote-branch condition="context/source_branch/branch_type/enumvalue:REMOTE">
-      <h2>Unmerged revisions</h2>
-      <p>Recent revisions are not available due to the source branch being remote.</p>
-    </tal:remote-branch>
+      <tal:remote-branch condition="context/source_branch/branch_type/enumvalue:REMOTE">
+        <h2>Unmerged revisions</h2>
+        <p>Recent revisions are not available due to the source branch being remote.</p>
+      </tal:remote-branch>
+    </tal:bzr-revisions>
+    <tal:git-revisions condition="context/source_git_ref">
+      <tal:history-available condition="context/source_git_ref/has_commits"
+                             define="ref context/source_git_ref;
+                                     commit_infos view/unlanded_revisions">
+        <h2>Unmerged commits</h2>
+        <metal:commits use-macro="ref/@@+macros/ref-commits"/>
+      </tal:history-available>
+    </tal:git-revisions>
   </div>
 
 </div>
 
=== modified file 'lib/lp/code/templates/codereviewcomment-reply.pt'
--- lib/lp/code/templates/codereviewcomment-reply.pt	2016-01-21 03:23:01 +0000
+++ lib/lp/code/templates/codereviewcomment-reply.pt	2016-05-18 22:37:48 +0000
@@ -8,8 +8,8 @@
 
  <body>
    <h1 metal:fill-slot="heading"
-      tal:define="branch view/branch_merge_proposal/merge_source">
-    Code review comment for <tal:source content="branch/identity"/>
+      tal:define="source view/branch_merge_proposal/merge_source">
+    Code review comment for <tal:source content="source/identity"/>
   </h1>
 
   <div metal:fill-slot="main">
 
=== modified file 'lib/lp/code/templates/codereviewnewrevisions-footer.pt'
--- lib/lp/code/templates/codereviewnewrevisions-footer.pt	2015-06-23 17:30:39 +0000
+++ lib/lp/code/templates/codereviewnewrevisions-footer.pt	2016-05-18 22:37:48 +0000
@@ -3,11 +3,19 @@
    xmlns:metal="http://xml.zope.org/namespaces/metal"
    omit-tag="">
 
-  <tal:revisions define="branch context/branch;
-                         revisions context/revisions;
-                         show_diff_expander python:True;">
-    <metal:landing-target use-macro="branch/@@+macros/branch-revisions"/>
-  </tal:revisions>
+  <tal:bzr-revisions condition="context/branch">
+    <tal:revisions define="branch context/branch;
+                           revisions context/revisions;
+                           show_diff_expander python:True;">
+      <metal:revisions use-macro="branch/@@+macros/branch-revisions"/>
+    </tal:revisions>
+  </tal:bzr-revisions>
+  <tal:git-revisions condition="context/git_ref">
+    <tal:revisions define="ref context/git_ref;
+                           commit_infos context/revisions;">
+      <metal:commits use-macro="ref/@@+macros/ref-commits"/>
+    </tal:revisions>
+  </tal:git-revisions>
   <tal:has-diff condition="context/diff">
     <tal:diff condition="not: request/ss|nothing"
               replace="structure context/diff/text/fmt:diff" />
 
=== modified file 'lib/lp/code/templates/codereviewnewrevisions-header.pt'
--- lib/lp/code/templates/codereviewnewrevisions-header.pt	2009-12-10 01:33:59 +0000
+++ lib/lp/code/templates/codereviewnewrevisions-header.pt	2016-05-18 22:37:48 +0000
@@ -3,7 +3,7 @@
    xmlns:metal="http://xml.zope.org/namespaces/metal"
    omit-tag="">
 
-  <tal:branch replace="structure context/branch/fmt:link"/>
+  <tal:source replace="structure context/source/fmt:link"/>
   updated
   <tal:date replace="context/date/fmt:displaydate" />
 
 
=== modified file 'lib/lp/code/templates/git-macros.pt'
--- lib/lp/code/templates/git-macros.pt	2016-05-18 11:49:31 +0000
+++ lib/lp/code/templates/git-macros.pt	2016-05-18 22:37:48 +0000
@@ -135,7 +135,7 @@
         sha1 python:commit_info['sha1'];
         author python:commit_info['author'];
         author_date python:commit_info['author_date']">
-    <a tal:attributes="href python: context.getCodebrowseUrlForRevision(sha1)"
+    <a tal:attributes="href python: ref.getCodebrowseUrlForRevision(sha1)"
        tal:content="sha1/fmt:shorten/10" />
     by
     <tal:known-person condition="author/person">
@@ -153,7 +153,7 @@
       replace="structure commit_message/fmt:obfuscate-email/fmt:text-to-html" />
   </dd>
 
-  <div tal:define="merge_proposal python:commit_info['merge_proposal']"
+  <div tal:define="merge_proposal python:commit_info.get('merge_proposal')"
        tal:condition="merge_proposal">
     <dd class="subordinate commit-comment"
         tal:define="committer_date merge_proposal/merge_source/committer_date|nothing">