launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #20362
[Merge] lp:~cjwatson/launchpad/git-mp-commits into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/git-mp-commits into lp:launchpad with lp:~cjwatson/launchpad/git-commits-link-mps as a prerequisite.
Commit message:
Show unmerged and conversation-relevant Git commits in merge proposal views.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-mp-commits/+merge/294671
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-mp-commits into lp:launchpad.
=== 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-13 17:22:59 +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 2015-11-27 11:32:50 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2016-05-13 17:22:59 +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).
@@ -11,6 +11,7 @@
timedelta,
)
from difflib import unified_diff
+import hashlib
import re
from lazr.lifecycle.event import ObjectModifiedEvent
@@ -59,6 +60,7 @@
IMergeProposalNeedsReviewEmailJobSource,
IMergeProposalUpdatedEmailJobSource,
)
+from lp.code.interfaces.githosting import IGitHostingClient
from lp.code.model.diff import PreviewDiff
from lp.code.tests.helpers import (
add_revision_to_branch,
@@ -87,6 +89,8 @@
time_counter,
verifyObject,
)
+from lp.testing.fakemethod import FakeMethod
+from lp.testing.fixture import ZopeUtilityFixture
from lp.testing.layers import (
DatabaseFunctionalLayer,
LaunchpadFunctionalLayer,
@@ -99,6 +103,16 @@
from lp.testing.views import create_initialized_view
+class GitHostingClientMixin:
+
+ def setUp(self):
+ super(GitHostingClientMixin, self).setUp()
+ self.hosting_client = FakeMethod()
+ self.hosting_client.getLog = FakeMethod(result=[])
+ self.useFixture(
+ ZopeUtilityFixture(self.hosting_client, IGitHostingClient))
+
+
class TestBranchMergeProposalContextMenu(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
@@ -219,7 +233,8 @@
class TestBranchMergeProposalMergedViewGit(
- TestBranchMergeProposalMergedViewMixin, BrowserTestCase):
+ TestBranchMergeProposalMergedViewMixin, GitHostingClientMixin,
+ BrowserTestCase):
"""Tests for `BranchMergeProposalMergedView` for Git."""
arbitrary_revisions = ("0" * 40, "1" * 40, "2" * 40)
@@ -1015,7 +1030,8 @@
class TestBranchMergeProposalRequestReviewViewGit(
- TestBranchMergeProposalRequestReviewViewMixin, BrowserTestCase):
+ TestBranchMergeProposalRequestReviewViewMixin, GitHostingClientMixin,
+ BrowserTestCase):
"""Test `BranchMergeProposalRequestReviewView` for Git."""
def makeBranchMergeProposal(self):
@@ -1245,7 +1261,7 @@
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
@@ -1425,7 +1441,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)
@@ -1438,6 +1454,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:
@@ -1511,7 +1557,8 @@
self.assertThat(browser.contents, HTMLContains(expected_meta))
-class TestBranchMergeProposalBrowserView(BrowserTestCase):
+class TestBranchMergeProposalBrowserView(
+ GitHostingClientMixin, BrowserTestCase):
layer = DatabaseFunctionalLayer
@@ -1711,7 +1758,7 @@
self.assertEqual('Eric on 2008-09-10', view.status_title)
-class TestBranchMergeProposal(BrowserTestCase):
+class TestBranchMergeProposal(GitHostingClientMixin, BrowserTestCase):
layer = LaunchpadFunctionalLayer
@@ -1903,7 +1950,8 @@
class TestBranchMergeProposalDeleteViewGit(
- TestBranchMergeProposalDeleteViewMixin, BrowserTestCase):
+ TestBranchMergeProposalDeleteViewMixin, GitHostingClientMixin,
+ BrowserTestCase):
"""Test the BranchMergeProposal deletion view for Git."""
def _makeBranchMergeProposal(self, **kwargs):
=== 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-13 17:22:59 +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-13 17:22:59 +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-13 17:22:59 +0000
@@ -188,6 +188,58 @@
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('admin@xxxxxxxxxxxxx')
+ >>> 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.extend([
+ ... {
+ ... 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()),
+ ... },
+ ... },
+ ... {
+ ... 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()),
+ ... },
+ ... },
+ ... ])
+ ... 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-13 17:22:59 +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-13 17:22:59 +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-13 17:22:59 +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-13 17:22:59 +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-13 17:22:59 +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-13 17:22:58 +0000
+++ lib/lp/code/templates/git-macros.pt 2016-05-13 17:22:59 +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">
<a tal:attributes="href merge_proposal/fmt:url">Merged</a> branch
Follow ups
