Merge lp:~abentley/launchpad/incremental-diffs into lp:launchpad/db-devel

[Aaron Bentley]

Implement incremental diff generation.

= 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

[Leonard Richardson (leonardr)]

This branch looks good with some trivial cleanup suggested in IRC, one minor change also suggested in IRC, and one major caveat:

The minor change: test_generateIncrementalDiff is pretty confusing, with lots of branches and lots of nearly-identical commits. It needs at least a comment up-front describing what's in the two revisions being diffed, and how the precursor and prerequisite branches affect this.

It might also be easier to understand if you defined a helper method to create a new branch *and its first commit*. This would make it clear which commits are setup, reflecting the "initial" states of the branches, and which commits are the ones leading up to the endpoint of the incremental diff. But an explanatory comment is good enough to get my approval.

The caveat: this branch introduces a new source dependency, difftacular, which was written by Aaron in July. Since it was written in-house, I think difftacular needs to go through the code review process, just like the lazr packages that are used in Launchpad. Fortunately, the entire difftacular project isn't much longer than 800 lines, so it shouldn't be too difficult for me to do the review.

review: Approve

[Leonard Richardson (leonardr)]

Oh, another change I suggested which Aaron carried through was removing the exported() calls from IIncrementalDiff. Those are for publishing fields through the web service, and IIncrementalDiff isn't being published yet.

[Leonard Richardson (leonardr)]

OK, I've done a review of the difftastic codebase and I see no reason why it can't be used in Launchpad. I do have one possible performance improvement, and a number of changes to make the tests more clear.

1. "foo" and "bar" are fine for filenames within the test branches,
but they really hurt comprehension when used as variable names
for the branches themselves. In the helper methods, can you call
the branches 'initial' and 'merged_in' or something that reflects
their history?

Outside the helper methods, you can call them whatever's
helpful. For instance, in test_mainline_diff, I believe
'foo' (aka 'initial') could be called 'mainline' and 'bar' (aka
'merged_in') could be called 'ignored'.

2. typo: "emtpy commit"

3. test_diff_ignore_branches calls
create_unignored_merge_scenario() to do its setup. This sounds
like a contradiction, but we're actually checking that only
*specified* branches are ignored. A docstring would make it clear
that we're checking that the diff ignores the merge of the "bar"
branch, but not the merge of the "unignored" branch. Similarly
for ignore_old_revisions--what's being ignored and what's being
included? It's hard to figure out.

Making the commits that are being ignored really obviously
destructive would also help.

3. In generate_diff.py#apply_merges(), do you need to call
merger.make_preview_transform and transform.get_preview_tree
within the for loop? Unless those methods modify 'merger'
in-place, I think they can be moved outside and run only once.

[Aaron Bentley (abentley)]

1. "foo" is actually the tree whose branch will be diffed. I don't think "initial" describes that any better than "foo". "merged_in" is fine, but I think "merge_input" is better.

2. Sure

3. Yes, I need it. The output of get_preview_tree is fed back into the subsequent merge. Without that, you would generate a bogus merge and likely violate the TreeTransform API.

[Brad Crittenden (bac)]

Is this approved branch ready to land or are you blocked? If you have abandoned the work please change the MP status to "Rejected."

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 11/04/2010 11:08 PM, Brad Crittenden wrote:
> Is this approved branch ready to land or are you blocked? If you have abandoned the work please change the MP status to "Rejected."

These changes will be landed as part of the follow-on branch
incremental-diff-jobs. Please see my comment at
https://code.launchpad.net/~abentley/launchpad/incremental-diff-job/+merge/36881
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkzUBVsACgkQ0F+nu1YWqI285wCfQd6GoKGnYBtp37QgqMIs2RvA
9n4AnRU1u4d8MDX+jvBBF5vWqoQsJeYE
=mGMI
-----END PGP SIGNATURE-----

=== added symlink 'bzrplugins/difftacular'
=== target is u'../sourcecode/difftacular/'
=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml	2010-10-26 13:52:43 +0000
+++ lib/lp/code/configure.zcml	2010-10-29 15:33:57 +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"
@@ -879,6 +881,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-10-29 15:33:57 +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,23 @@
             readonly=True))
 
 
+class IIncrementalDiff(Interface):
+    """An incremental diff for a merge proposal."""
+
+    diff = Reference(IDiff, title=_('The Diff object.'), readonly=True)
+
+    # The schema for the Reference gets patched in _schema_circular_imports.
+    branch_merge_proposal = Reference(
+        Interface, readonly=True,
+        title=_('The branch merge proposal that diff relates to.'))
+
+    old_revision = Reference(
+        IRevision, readonly=True, title=_('The old revision of the diff.'))
+
+    new_revision = 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-10-19 00:44:24 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2010-10-29 15:33:57 +0000
@@ -50,6 +50,7 @@
     SQLBase,
     sqlvalues,
     )
+from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from lp.code.enums import (
     BranchMergeProposalStatus,
     CodeReviewVote,
@@ -78,7 +79,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,
@@ -783,6 +784,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-10-03 15:30:06 +0000
+++ lib/lp/code/model/diff.py	2010-10-29 15:33:57 +0000
@@ -4,10 +4,11 @@
 """Implementation classes for IDiff, etc."""
 
 __metaclass__ = type
-__all__ = ['Diff', 'PreviewDiff', 'StaticDiff']
+__all__ = ['Diff', 'IncrementalDiff', 'PreviewDiff', 'StaticDiff']
 
+from contextlib import nested
+from cStringIO import StringIO
 import sys
-from cStringIO import StringIO
 
 from uuid import uuid1
 
@@ -18,6 +19,7 @@
     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 +46,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):
@@ -194,9 +198,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):
@@ -257,6 +266,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."""
@@ -305,6 +338,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-10-27 14:20:21 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2010-10-29 15:33:57 +0000
@@ -1829,5 +1829,46 @@
         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-10-26 15:47:24 +0000
+++ lib/lp/code/model/tests/test_diff.py	2010-10-29 15:33:57 +0000
@@ -12,6 +12,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
 
@@ -24,6 +25,7 @@
 from lp.app.errors import NotFoundError
 from lp.code.interfaces.diff import (
     IDiff,
+    IIncrementalDiff,
     IPreviewDiff,
     IStaticDiff,
     IStaticDiffSource,
@@ -521,5 +523,87 @@
             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.
+
+        We generate an incremental diff from old_revision_id to
+        new_revision_id.
+
+        old_revision_id has:
+        a
+        b
+        e
+
+        new_revision_id has:
+        d
+        a
+        c
+        e
+        f
+
+        Because the prerequisite branch adds "d", this change is ignored.
+        Because the target branch adds "f", this change is ignored.
+        So the incremental diff shows that "c" was added and "b" was removed.
+        """
+        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-10-28 09:11:36 +0000
+++ lib/lp/testing/factory.py	2010-10-29 15:33:57 +0000
@@ -1266,6 +1266,18 @@
         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()
+        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-10-25 15:23:38 +0000
+++ utilities/sourcedeps.conf	2010-10-29 15:33:57 +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