launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #01270
[Merge] lp:~abentley/launchpad/incremental-diffs into lp:launchpad
Aaron Bentley has proposed merging lp:~abentley/launchpad/incremental-diffs into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
= Summary =
Implement incremental diff generation
== Proposed fix ==
See above
== Pre-implementation notes ==
Mid-implementation with thumper
== Implementation details ==
The generation of incremental diffs is handled by the Difftacular bzr plugin,
and the full testing of its behaviour is provided by its test suite.
I implemented the diff_changes test method to get a clearer idea what was added
and removed by a given diff.
== Tests ==
bin/test -t TestIncrementalDiff -t TestBranchMergeProposalGetIncrementalDiffs
== Demo and Q/A ==
None
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/configure.zcml
lib/lp/code/model/branchmergeproposal.py
lib/lp/code/model/tests/test_branchmergeproposal.py
lib/lp/code/model/diff.py
lib/lp/testing/factory.py
lib/lp/code/model/tests/test_diff.py
utilities/sourcedeps.conf
lib/lp/code/interfaces/diff.py
./lib/lp/code/model/tests/test_branchmergeproposal.py
192: E231 missing whitespace after ','
194: E231 missing whitespace after ','
./lib/lp/code/model/diff.py
166: E301 expected 1 blank line, found 0
--
https://code.launchpad.net/~abentley/launchpad/incremental-diffs/+merge/36875
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/incremental-diffs into lp:launchpad.
=== added symlink 'bzrplugins/difftacular'
=== target is u'../sourcecode/difftacular/'
=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml 2010-09-21 14:54:43 +0000
+++ lib/lp/code/configure.zcml 2010-09-28 14:47:44 +0000
@@ -233,6 +233,7 @@
review_diff
next_preview_diff_job
preview_diff
+ getIncrementalDiffs
votes
all_comments
related_bugs
@@ -245,7 +246,8 @@
isValidTransition
getUnlandedSourceBranchRevisions
root_message_id
- getUsersVoteReference"/>
+ getUsersVoteReference
+ generateIncrementalDiff"/>
<allow interface="lp.code.interfaces.branchtarget.IHasBranchTarget"/>
<require
permission="launchpad.Edit"
@@ -897,6 +899,10 @@
<class class="lp.code.model.diff.Diff">
<allow interface="lp.code.interfaces.diff.IDiff" />
</class>
+ <class class="lp.code.model.diff.IncrementalDiff">
+ <allow interface="lp.code.interfaces.diff.IDiff" />
+ <allow interface="lp.code.interfaces.diff.IIncrementalDiff" />
+ </class>
<class class="lp.code.model.diff.StaticDiff">
<allow interface="lp.code.interfaces.diff.IStaticDiff" />
</class>
=== modified file 'lib/lp/code/interfaces/diff.py'
--- lib/lp/code/interfaces/diff.py 2010-08-20 20:31:18 +0000
+++ lib/lp/code/interfaces/diff.py 2010-09-28 14:47:44 +0000
@@ -9,6 +9,7 @@
__all__ = [
'IDiff',
+ 'IIncrementalDiff',
'IPreviewDiff',
'IStaticDiff',
'IStaticDiffSource',
@@ -29,6 +30,7 @@
)
from canonical.launchpad import _
+from lp.code.interfaces.revision import IRevision
class IDiff(Interface):
@@ -62,6 +64,29 @@
readonly=True))
+class IIncrementalDiff(Interface):
+ """An incremental diff for a merge proposal."""
+
+ diff = exported(
+ Reference(IDiff, title=_('The Diff object.'), readonly=True))
+
+ # The schema for the Reference gets patched in _schema_circular_imports.
+ branch_merge_proposal = exported(
+ Reference(
+ Interface, readonly=True,
+ title=_('The branch merge proposal that diff relates to.')))
+
+ old_revision = exported(
+ Reference(
+ IRevision, readonly=True,
+ title=_('The old revision of the diff.')))
+
+ new_revision = exported(
+ Reference(
+ IRevision, readonly=True,
+ title=_('The new revision of the diff.')))
+
+
class IStaticDiff(Interface):
"""A diff with a fixed value, i.e. between two revisions."""
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2010-09-24 22:30:48 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2010-09-28 14:47:44 +0000
@@ -50,6 +50,7 @@
SQLBase,
sqlvalues,
)
+from canonical.launchpad.interfaces.lpstorm import IMasterStore
from lp.code.enums import (
BranchMergeProposalStatus,
CodeReviewVote,
@@ -77,7 +78,7 @@
from lp.code.model.branchrevision import BranchRevision
from lp.code.model.codereviewcomment import CodeReviewComment
from lp.code.model.codereviewvote import CodeReviewVoteReference
-from lp.code.model.diff import PreviewDiff
+from lp.code.model.diff import Diff, IncrementalDiff, PreviewDiff
from lp.registry.interfaces.person import (
IPerson,
validate_public_person,
@@ -777,6 +778,45 @@
Store.of(self).flush()
return self.preview_diff
+ def generateIncrementalDiff(self, old_revision, new_revision, diff=None):
+ """Generate an incremental diff for the merge proposal.
+
+ :param old_revision: The `Revision` to generate the diff from.
+ :param new_revision: The `Revision` to generate the diff to.
+ :param diff: If supplied, a pregenerated `Diff`.
+ """
+ if diff is None:
+ source_branch = self.source_branch.getBzrBranch()
+ ignore_branches = [self.target_branch.getBzrBranch()]
+ if self.prerequisite_branch is not None:
+ ignore_branches.append(
+ self.prerequisite_branch.getBzrBranch())
+ diff = Diff.generateIncrementalDiff(
+ old_revision, new_revision, source_branch, ignore_branches)
+ incremental_diff = IncrementalDiff()
+ incremental_diff.diff = diff
+ incremental_diff.branch_merge_proposal = self
+ incremental_diff.old_revision = old_revision
+ incremental_diff.new_revision = new_revision
+ IMasterStore(IncrementalDiff).add(incremental_diff)
+ return incremental_diff
+
+ def getIncrementalDiffs(self, revision_list):
+ """Return a list of diffs for the specified revisions.
+
+ :param revision_list: A list of tuples of (`Revision`, `Revision`).
+ The first revision in the tuple is the old revision. The second
+ is the new revision.
+ :return: A list of IncrementalDiffs in the same order as the supplied
+ Revisions.
+ """
+ diffs = Store.of(self).find(IncrementalDiff,
+ IncrementalDiff.branch_merge_proposal_id == self.id)
+ diff_dict = dict(
+ ((diff.old_revision, diff.new_revision), diff)
+ for diff in diffs)
+ return [diff_dict.get(revisions) for revisions in revision_list]
+
@property
def revision_end_date(self):
"""The cutoff date for showing revisions.
=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py 2010-08-20 20:31:18 +0000
+++ lib/lp/code/model/diff.py 2010-09-28 14:47:44 +0000
@@ -1,23 +1,26 @@
# Copyright 2009 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
+from __future__ import with_statement
+
"""Implementation classes for IDiff, etc."""
__metaclass__ = type
-__all__ = ['Diff', 'PreviewDiff', 'StaticDiff']
+__all__ = ['Diff', 'IncrementalDiff', 'PreviewDiff', 'StaticDiff']
from cStringIO import StringIO
+from contextlib import nested
import sys
from uuid import uuid1
from bzrlib import trace
-from bzrlib.branch import Branch
from bzrlib.diff import show_diff_trees
from bzrlib.merge import Merge3Merger
from bzrlib.patches import (
parse_patches,
Patch,
)
+from bzrlib.plugins.difftacular.generate_diff import diff_ignore_branches
from lazr.delegates import delegates
import simplejson
from sqlobject import (
@@ -44,10 +47,12 @@
from lp.app.errors import NotFoundError
from lp.code.interfaces.diff import (
IDiff,
+ IIncrementalDiff,
IPreviewDiff,
IStaticDiff,
IStaticDiffSource,
)
+from lp.codehosting.bzrutils import read_locked
class Diff(SQLBase):
@@ -192,9 +197,14 @@
diff_content = StringIO()
show_diff_trees(from_tree, to_tree, diff_content, old_label='',
new_label='')
+ return klass.fromFileAtEnd(diff_content, filename)
+
+ @classmethod
+ def fromFileAtEnd(cls, diff_content, filename=None):
+ """Make a Diff from a file object that is currently at its end."""
size = diff_content.tell()
diff_content.seek(0)
- return klass.fromFile(diff_content, size, filename)
+ return cls.fromFile(diff_content, size, filename)
@classmethod
def fromFile(cls, diff_content, size, filename=None):
@@ -255,6 +265,30 @@
file_stats[path] = tuple(patch.stats_values()[:2])
return file_stats
+ @classmethod
+ def generateIncrementalDiff(cls, old_revision, new_revision,
+ source_branch, ignore_branches):
+ """Return a Diff whose contents are an incremental diff.
+
+ The Diff's contents will show the changes made between old_revision
+ and new_revision, except those changes introduced by the
+ ignore_branches.
+
+ :param old_revision: The `Revision` to show changes from.
+ :param new_revision: The `Revision` to show changes to.
+ :param source_branch: The bzr branch containing these revisions.
+ :param ignore_brances: A collection of branches to ignore merges from.
+ :return: a `Diff`.
+ """
+ diff_content = StringIO()
+ read_locks = [read_locked(branch) for branch in [source_branch] +
+ ignore_branches]
+ with nested(*read_locks):
+ diff_ignore_branches(
+ source_branch, ignore_branches, old_revision.revision_id,
+ new_revision.revision_id, diff_content)
+ return cls.fromFileAtEnd(diff_content)
+
class StaticDiff(SQLBase):
"""A diff from one revision to another."""
@@ -303,6 +337,36 @@
diff.destroySelf()
+class IncrementalDiff(Storm):
+ """See `IIncrementalDiff."""
+
+ implements(IIncrementalDiff)
+
+ delegates(IDiff, context='diff')
+
+ __storm_table__ = 'IncrementalDiff'
+
+ id = Int(primary=True, allow_none=False)
+
+ diff_id = Int(name='diff', allow_none=False)
+
+ diff = Reference(diff_id, 'Diff.id')
+
+ branch_merge_proposal_id = Int(
+ name='branch_merge_proposal', allow_none=False)
+
+ branch_merge_proposal = Reference(
+ branch_merge_proposal_id, "BranchMergeProposal.id")
+
+ old_revision_id = Int(name='old_revision', allow_none=False)
+
+ old_revision = Reference(old_revision_id, 'Revision.id')
+
+ new_revision_id = Int(name='new_revision', allow_none=False)
+
+ new_revision = Reference(new_revision_id, 'Revision.id')
+
+
class PreviewDiff(Storm):
"""See `IPreviewDiff`."""
implements(IPreviewDiff)
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py 2010-09-15 20:41:46 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py 2010-09-28 14:47:44 +0000
@@ -1862,5 +1862,45 @@
self.assertRevisionGroups(bmp, expected_groups)
+class TestBranchMergeProposalGetIncrementalDiffs(TestCaseWithFactory):
+
+ layer = LaunchpadFunctionalLayer
+
+ def test_getIncrementalDiffs(self):
+ """getIncrementalDiffs returns the requested values or None.
+
+ None is returned if there is no IncrementalDiff for the requested
+ revision pair and branch_merge_proposal."""
+ bmp = self.factory.makeBranchMergeProposal()
+ diff1 = self.factory.makeIncrementalDiff(merge_proposal=bmp)
+ diff2 = self.factory.makeIncrementalDiff(merge_proposal=bmp)
+ diff3 = self.factory.makeIncrementalDiff()
+ result = bmp.getIncrementalDiffs([
+ (diff1.old_revision, diff1.new_revision),
+ (diff2.old_revision, diff2.new_revision),
+ # Wrong merge proposal
+ (diff3.old_revision, diff3.new_revision),
+ # Mismatched revisions
+ (diff1.old_revision, diff2.new_revision),
+ ])
+ self.assertEqual([diff1, diff2, None, None], result)
+
+ def test_getIncrementalDiffs_respects_input_order(self):
+ """The order of the output follows the input order."""
+ bmp = self.factory.makeBranchMergeProposal()
+ diff1 = self.factory.makeIncrementalDiff(merge_proposal=bmp)
+ diff2 = self.factory.makeIncrementalDiff(merge_proposal=bmp)
+ result = bmp.getIncrementalDiffs([
+ (diff1.old_revision, diff1.new_revision),
+ (diff2.old_revision, diff2.new_revision),
+ ])
+ self.assertEqual([diff1, diff2], result)
+ result = bmp.getIncrementalDiffs([
+ (diff2.old_revision, diff2.new_revision),
+ (diff1.old_revision, diff1.new_revision),
+ ])
+ self.assertEqual([diff2, diff1], result)
+
+
def test_suite():
return TestLoader().loadTestsFromName(__name__)
=== modified file 'lib/lp/code/model/tests/test_diff.py'
--- lib/lp/code/model/tests/test_diff.py 2010-09-15 20:41:46 +0000
+++ lib/lp/code/model/tests/test_diff.py 2010-09-28 14:47:44 +0000
@@ -14,6 +14,7 @@
from unittest import TestLoader
from bzrlib import trace
+from bzrlib.patches import InsertLine, parse_patches, RemoveLine
import transaction
from zope.security.proxy import removeSecurityProxy
@@ -26,6 +27,7 @@
from lp.app.errors import NotFoundError
from lp.code.interfaces.diff import (
IDiff,
+ IIncrementalDiff,
IPreviewDiff,
IStaticDiff,
IStaticDiffSource,
@@ -523,5 +525,69 @@
NotFoundError, diff.getFileByName, 'preview.diff')
+class TestIncrementalDiff(DiffTestCase):
+ """Test that IncrementalDiff objects work."""
+
+ layer = LaunchpadFunctionalLayer
+
+ def test_providesInterface(self):
+ incremental_diff = self.factory.makeIncrementalDiff()
+ verifyObject(IIncrementalDiff, incremental_diff)
+
+ @staticmethod
+ def diff_changes(diff_bytes):
+ inserted = []
+ removed = []
+ for patch in parse_patches(diff_bytes.splitlines(True)):
+ for hunk in patch.hunks:
+ for line in hunk.lines:
+ if isinstance(line, InsertLine):
+ inserted.append(line.contents)
+ if isinstance(line, RemoveLine):
+ removed.append(line.contents)
+ return inserted, removed
+
+ def test_generateIncrementalDiff(self):
+ """generateIncrementalDiff works.
+
+ Changes merged from the prerequisite and target are ignored in the
+ diff.
+ """
+ self.useBzrBranches(direct_database=True)
+ prerequisite_branch = self.factory.makeAnyBranch()
+ bmp = self.factory.makeBranchMergeProposal(
+ prerequisite_branch=prerequisite_branch)
+ target_branch = self.createBzrBranch(bmp.target_branch)
+ old_revision_id = self.commitFile(
+ bmp.target_branch, 'foo', 'a\nb\ne\n')
+ old_revision = self.factory.makeRevision(rev_id=old_revision_id)
+ source_branch = self.createBzrBranch(
+ bmp.source_branch, target_branch)
+ self.commitFile(
+ bmp.source_branch, 'foo', 'a\nc\ne\n')
+ prerequisite = self.createBzrBranch(
+ bmp.prerequisite_branch, target_branch)
+ prerequisite_revision = self.commitFile(bmp.prerequisite_branch,
+ 'foo', 'd\na\nb\ne\n')
+ merge_parent = self.commitFile(bmp.target_branch, 'foo',
+ 'a\nb\ne\nf\n')
+ source_branch.repository.fetch(target_branch.repository,
+ revision_id=merge_parent)
+ self.commitFile(
+ bmp.source_branch, 'foo', 'a\nc\ne\nf\n', [merge_parent])
+ source_branch.repository.fetch(prerequisite.repository,
+ revision_id=prerequisite_revision)
+ new_revision_id = self.commitFile(
+ bmp.source_branch, 'foo', 'd\na\nc\ne\nf\n',
+ [prerequisite_revision])
+ new_revision = self.factory.makeRevision(rev_id=new_revision_id)
+ incremental_diff = bmp.generateIncrementalDiff(
+ old_revision, new_revision)
+ transaction.commit()
+ inserted, removed = self.diff_changes(incremental_diff.text)
+ self.assertEqual(['c\n'], inserted)
+ self.assertEqual(['b\n'], removed)
+
+
def test_suite():
return TestLoader().loadTestsFromName(__name__)
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2010-09-23 19:52:50 +0000
+++ lib/lp/testing/factory.py 2010-09-28 14:47:44 +0000
@@ -1238,6 +1238,19 @@
preview_diff.target_revision_id = self.getUniqueUnicode()
return preview_diff
+ def makeIncrementalDiff(self, merge_proposal=None, old_revision=None,
+ new_revision=None):
+ diff = self.makeDiff()
+ if merge_proposal is None:
+ merge_proposal = self.makeBranchMergeProposal()
+ if old_revision is None:
+ old_revision = self.makeRevision()
+ if new_revision is None:
+ new_revision = self.makeRevision()
+ diff = self.makeDiff()
+ return merge_proposal.generateIncrementalDiff(
+ old_revision, new_revision, diff)
+
def makeStaticDiff(self):
return StaticDiff.acquireFromText(
self.getUniqueUnicode(), self.getUniqueUnicode(),
=== modified file 'utilities/sourcedeps.conf'
--- utilities/sourcedeps.conf 2010-09-27 13:59:38 +0000
+++ utilities/sourcedeps.conf 2010-09-28 14:47:44 +0000
@@ -5,6 +5,7 @@
bzr-svn lp:~launchpad-pqm/bzr-svn/devel;revno=2710
cscvs lp:~launchpad-pqm/launchpad-cscvs/devel;revno=432
dulwich lp:~launchpad-pqm/dulwich/devel;revno=424
+difftacular lp:difftacular;revno=6
loggerhead lp:~launchpad-pqm/loggerhead/devel;revno=176
lpreview lp:~launchpad-pqm/bzr-lpreview/devel;revno=23
mailman lp:~launchpad-pqm/mailman/2.1;revno=976
