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

[Colin Watson]

Generate preview diffs for Git-based merge proposals.

Generate preview diffs for Git-based merge proposals.

[William Grant (wgrant)]

review: Approve (code)

=== modified file 'database/schema/security.cfg'
--- database/schema/security.cfg	2015-04-21 10:34:24 +0000
+++ database/schema/security.cfg	2015-04-30 13:12:11 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 #
 # Possible permissions: SELECT, INSERT, UPDATE, EXECUTE
@@ -1937,6 +1937,8 @@
 public.distribution                     = SELECT
 public.distroseries                     = SELECT
 public.emailaddress                     = SELECT
+public.gitref                           = SELECT
+public.gitrepository                    = SELECT
 public.incrementaldiff                  = SELECT, INSERT
 public.job                              = SELECT, INSERT, UPDATE
 public.karma                            = SELECT, INSERT

=== modified file 'lib/lp/app/browser/stringformatter.py'
--- lib/lp/app/browser/stringformatter.py	2013-12-12 05:06:11 +0000
+++ lib/lp/app/browser/stringformatter.py	2015-04-30 13:12:11 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """TALES formatter for strings."""
@@ -884,7 +884,9 @@
         for row, line in enumerate(text.splitlines()[:max_format_lines]):
             result.append('<tr id="diff-line-%s">' % (row + 1))
             result.append('<td class="line-no">%s</td>' % (row + 1))
-            if line.startswith('==='):
+            if (line.startswith('===') or
+                    line.startswith('diff') or
+                    line.startswith('index')):
                 css_class = 'diff-file text'
                 header_next = True
             elif (header_next and

=== modified file 'lib/lp/app/browser/tests/test_stringformatter.py'
--- lib/lp/app/browser/tests/test_stringformatter.py	2013-12-13 02:09:14 +0000
+++ lib/lp/app/browser/tests/test_stringformatter.py	2015-04-30 13:12:11 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Unit tests for the string TALES formatter."""
@@ -165,7 +165,7 @@
 
 
 class TestLinkifyingProtocols(TestCaseWithFactory):
-    
+
     layer = DatabaseFunctionalLayer
 
     def test_normal_set(self):
@@ -357,6 +357,40 @@
              'diff-comment text'],
             [str(tag['class']) for tag in text])
 
+    def test_cssClasses_git(self):
+        # Git diffs look slightly different, so check that they also end up
+        # with the correct CSS classes.
+        diff = dedent('''\
+            diff --git a/tales.py b/tales.py
+            index aaaaaaa..bbbbbbb 100644
+            --- a/tales.py
+            +++ b/tales.py
+            @@ -2435,6 +2435,8 @@
+                 def format_diff(self):
+            -        removed this line
+            +        added this line
+            -------- a sql style comment
+            ++++++++ a line of pluses
+            ''')
+        html = FormattersAPI(diff).format_diff()
+        line_numbers = find_tags_by_class(html, 'line-no')
+        self.assertEqual(
+            ['1', '2', '3', '4', '5', '6', '7', '8', '9', '10'],
+            [tag.renderContents() for tag in line_numbers])
+        text = find_tags_by_class(html, 'text')
+        self.assertEqual(
+            ['diff-file text',
+             'diff-file text',
+             'diff-header text',
+             'diff-header text',
+             'diff-chunk text',
+             'text',
+             'diff-removed text',
+             'diff-added text',
+             'diff-removed text',
+             'diff-added text'],
+            [str(tag['class']) for tag in text])
+
     def test_config_value_limits_line_count(self):
         # The config.diff.max_line_format contains the maximum number of lines
         # to format.

=== modified file 'lib/lp/code/githosting.py'
--- lib/lp/code/githosting.py	2015-03-31 00:59:44 +0000
+++ lib/lp/code/githosting.py	2015-04-30 13:12:11 +0000
@@ -101,3 +101,35 @@
         except ValueError as e:
             raise GitRepositoryScanFault(
                 "Failed to decode commit-scan response: %s" % unicode(e))
+
+    def getMergeDiff(self, path, base, head, logger=None):
+        """Get the merge preview diff between two commits.
+
+        :return: A dict mapping 'commits' to a list of commits between
+            'base' and 'head' (formatted as with `getCommits`), 'patch' to
+            the text of the diff between 'base' and 'head', and 'conflicts'
+            to a list of conflicted paths.
+        """
+        try:
+            if logger is not None:
+                logger.info(
+                    "Requesting merge diff for %s from %s to %s" % (
+                        path, base, head))
+            response = self._makeSession().get(
+                urlutils.join(
+                    self.endpoint, "repo", path, "compare-merge",
+                    "%s:%s" % (base, head)),
+                timeout=self.timeout)
+        except Exception as e:
+            raise GitRepositoryScanFault(
+                "Failed to get merge diff from Git repository: %s" %
+                unicode(e))
+        if response.status_code != 200:
+            raise GitRepositoryScanFault(
+                "Failed to get merge diff from Git repository: %s" %
+                unicode(e))
+        try:
+            return response.json()
+        except ValueError as e:
+            raise GitRepositoryScanFault(
+                "Failed to decode merge-diff response: %s" % unicode(e))

=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
--- lib/lp/code/model/branchmergeproposaljob.py	2014-05-28 12:23:10 +0000
+++ lib/lp/code/model/branchmergeproposaljob.py	2015-04-30 13:12:11 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Job classes related to BranchMergeProposals are in here.
@@ -21,6 +21,7 @@
     'UpdatePreviewDiffJob',
     ]
 
+from contextlib import contextmanager
 from datetime import (
     datetime,
     timedelta,
@@ -39,7 +40,6 @@
     Desc,
     Or,
     )
-from storm.info import ClassAlias
 from storm.locals import (
     Int,
     Reference,
@@ -102,6 +102,7 @@
 from lp.services.mail.sendmail import format_address_for_person
 from lp.services.webapp import canonical_url
 
+
 class BranchMergeProposalJobType(DBEnumeratedType):
     """Values that ICodeImportJob.state can take."""
 
@@ -235,7 +236,7 @@
         return '<%(job_type)s job for merge %(merge_id)s on %(branch)s>' % {
             'job_type': self.context.job_type.name,
             'merge_id': bmp.id,
-            'branch': bmp.source_branch.unique_name,
+            'branch': bmp.merge_source.unique_name,
             }
 
     @classmethod
@@ -278,12 +279,13 @@
                 Job.id.is_in(Job.ready_jobs),
                 BranchMergeProposalJob.branch_merge_proposal
                     == BranchMergeProposal.id,
-                BranchMergeProposal.source_branch == Branch.id,
-                # A proposal isn't considered ready if it has no revisions,
-                # or if it is hosted but pending a mirror.
-                Branch.revision_count > 0,
-                Or(Branch.next_mirror_time == None,
-                   Branch.branch_type != BranchType.HOSTED)))
+                Or(And(BranchMergeProposal.source_branch == Branch.id,
+                       # A proposal isn't considered ready if it has no
+                       # revisions, or if it is hosted but pending a mirror.
+                       Branch.revision_count > 0,
+                       Or(Branch.next_mirror_time == None,
+                          Branch.branch_type != BranchType.HOSTED)),
+                   BranchMergeProposal.source_git_repository != None)))
         return (klass(job) for job in jobs)
 
     def getOopsVars(self):
@@ -293,8 +295,8 @@
         vars.extend([
             ('branchmergeproposal_job_id', self.context.id),
             ('branchmergeproposal_job_type', self.context.job_type.title),
-            ('source_branch', bmp.source_branch.unique_name),
-            ('target_branch', bmp.target_branch.unique_name)])
+            ('merge_source', bmp.merge_source.unique_name),
+            ('merge_target', bmp.merge_target.unique_name)])
         return vars
 
 
@@ -320,8 +322,8 @@
 
     def getOperationDescription(self):
         return ('notifying people about the proposal to merge %s into %s' %
-            (self.branch_merge_proposal.source_branch.bzr_identity,
-             self.branch_merge_proposal.target_branch.bzr_identity))
+            (self.branch_merge_proposal.merge_source.identity,
+             self.branch_merge_proposal.merge_target.identity))
 
 
 class UpdatePreviewDiffJob(BranchMergeProposalJobDerived):
@@ -350,15 +352,16 @@
         """Is this job ready to run?"""
         bmp = self.branch_merge_proposal
         url = canonical_url(bmp)
-        if bmp.source_branch.last_scanned_id is None:
-            raise UpdatePreviewDiffNotReady(
-                'The source branch of %s has no revisions.' % url)
-        if bmp.target_branch.last_scanned_id is None:
-            raise UpdatePreviewDiffNotReady(
-                'The target branch of %s has no revisions.' % url)
-        if bmp.source_branch.pending_writes:
-            raise BranchHasPendingWrites(
-                'The source branch of %s has pending writes.' % url)
+        if bmp.source_branch is not None:
+            if bmp.source_branch.last_scanned_id is None:
+                raise UpdatePreviewDiffNotReady(
+                    'The source branch of %s has no revisions.' % url)
+            if bmp.target_branch.last_scanned_id is None:
+                raise UpdatePreviewDiffNotReady(
+                    'The target branch of %s has no revisions.' % url)
+            if bmp.source_branch.pending_writes:
+                raise BranchHasPendingWrites(
+                    'The source branch of %s has pending writes.' % url)
 
     def acquireLease(self, duration=600):
         return self.job.acquireLease(duration)
@@ -366,7 +369,13 @@
     def run(self):
         """See `IRunnableJob`."""
         self.checkReady()
-        with server(get_ro_server(), no_replace=True):
+        if self.branch_merge_proposal.source_branch is not None:
+            server_context = server(get_ro_server(), no_replace=True)
+        else:
+            # A no-op context manager.  (This could be simplified with
+            # contextlib.ExitStack from Python 3.3.)
+            server_context = contextmanager(lambda: (None for _ in [None]))()
+        with server_context:
             with BranchMergeProposalDelta.monitor(self.branch_merge_proposal):
                 PreviewDiff.fromBranchMergeProposal(self.branch_merge_proposal)
 
@@ -655,22 +664,16 @@
 
     @staticmethod
     def iterReady(job_type=None):
-        from lp.code.model.branch import Branch
-        SourceBranch = ClassAlias(Branch)
-        TargetBranch = ClassAlias(Branch)
         clauses = [
             BranchMergeProposalJob.job == Job.id,
             Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]),
             BranchMergeProposalJob.branch_merge_proposal ==
-            BranchMergeProposal.id, BranchMergeProposal.source_branch ==
-            SourceBranch.id, BranchMergeProposal.target_branch ==
-            TargetBranch.id,
+            BranchMergeProposal.id,
             ]
         if job_type is not None:
             clauses.append(BranchMergeProposalJob.job_type == job_type)
-        jobs = IMasterStore(Branch).find(
-            (BranchMergeProposalJob, Job, BranchMergeProposal,
-             SourceBranch, TargetBranch), And(*clauses))
+        jobs = IMasterStore(BranchMergeProposalJob).find(
+            (BranchMergeProposalJob, Job, BranchMergeProposal), And(*clauses))
         # Order by the job status first (to get running before waiting), then
         # the date_created, then job type.  This should give us all creation
         # jobs before comment jobs.
@@ -680,7 +683,7 @@
         # Now only return one job for any given merge proposal.
         ready_jobs = []
         seen_merge_proposals = set()
-        for bmp_job, job, bmp, source, target in jobs:
+        for bmp_job, job, bmp in jobs:
             # If we've seen this merge proposal already, skip this job.
             if bmp.id in seen_merge_proposals:
                 continue

=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py	2014-02-26 13:38:39 +0000
+++ lib/lp/code/model/diff.py	2015-04-30 13:12:11 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Implementation classes for IDiff, etc."""
@@ -41,6 +41,7 @@
 from zope.interface import implements
 
 from lp.app.errors import NotFoundError
+from lp.code.githosting import GitHostingClient
 from lp.code.interfaces.diff import (
     IDiff,
     IIncrementalDiff,
@@ -233,7 +234,8 @@
         return cls.fromFile(diff_content, size, filename)
 
     @classmethod
-    def fromFile(cls, diff_content, size, filename=None):
+    def fromFile(cls, diff_content, size, filename=None,
+                 strip_prefix_segments=0):
         """Create a Diff from a textual diff.
 
         :diff_content: The diff text
@@ -254,7 +256,9 @@
             diff_content_bytes = diff_content.read(size)
             diff_lines_count = len(diff_content_bytes.strip().split('\n'))
         try:
-            diffstat = cls.generateDiffstat(diff_content_bytes)
+            diffstat = cls.generateDiffstat(
+                diff_content_bytes,
+                strip_prefix_segments=strip_prefix_segments)
         except Exception:
             getUtility(IErrorReportingUtility).raising(sys.exc_info())
             # Set the diffstat to be empty.
@@ -272,10 +276,13 @@
                    removed_lines_count=removed_lines_count)
 
     @staticmethod
-    def generateDiffstat(diff_bytes):
+    def generateDiffstat(diff_bytes, strip_prefix_segments=0):
         """Generate statistics about the provided diff.
 
         :param diff_bytes: A unified diff, as bytes.
+        :param strip_prefix_segments: Strip the smallest prefix containing
+            this many leading slashes from each file name found in the patch
+            file, as with "patch -p".
         :return: A map of {filename: (added_line_count, removed_line_count)}
         """
         file_stats = {}
@@ -285,6 +292,8 @@
             if not isinstance(patch, Patch):
                 continue
             path = patch.newname.split('\t')[0]
+            if strip_prefix_segments:
+                path = path.split('/', strip_prefix_segments)[-1]
             file_stats[path] = tuple(patch.stats_values()[:2])
         return file_stats
 
@@ -378,27 +387,39 @@
         # diff was generated for absent branch revisions (e.g. the source
         # or target branch was overwritten). Which means some entries for
         # the same BMP may have much wider titles depending on the
-        # branch history. It is particulary bad for rendering the diff
+        # branch history. It is particularly bad for rendering the diff
         # navigator 'select' widget in the UI.
-        source_rev = bmp.source_branch.getBranchRevision(
-            revision_id=self.source_revision_id)
-        if source_rev and source_rev.sequence:
-            source_revno = u'r{0}'.format(source_rev.sequence)
-        else:
-            source_revno = self.source_revision_id
-        target_rev = bmp.target_branch.getBranchRevision(
-            revision_id=self.target_revision_id)
-        if target_rev and target_rev.sequence:
-            target_revno = u'r{0}'.format(target_rev.sequence)
-        else:
-            target_revno = self.target_revision_id
+        if bmp.source_branch is not None:
+            source_revision = bmp.source_branch.getBranchRevision(
+                revision_id=self.source_revision_id)
+            if source_revision and source_revision.sequence:
+                source_rev = u'r{0}'.format(source_revision.sequence)
+            else:
+                source_rev = self.source_revision_id
+            target_revision = bmp.target_branch.getBranchRevision(
+                revision_id=self.target_revision_id)
+            if target_revision and target_revision.sequence:
+                target_rev = u'r{0}'.format(target_revision.sequence)
+            else:
+                target_rev = self.target_revision_id
+        else:
+            # For Git, we shorten to seven characters since that's usual.
+            # We should perhaps shorten only as far as preserves uniqueness,
+            # but that requires talking to the hosting service and it's
+            # unlikely to be a problem in practice.
+            source_rev = self.source_revision_id[:7]
+            target_rev = self.target_revision_id[:7]
 
-        return u'{0} into {1}'.format(source_revno, target_revno)
+        return u'{0} into {1}'.format(source_rev, target_rev)
 
     @property
     def has_conflicts(self):
         return self.conflicts is not None and self.conflicts != ''
 
+    @staticmethod
+    def getGitHostingClient():
+        return GitHostingClient(config.codehosting.internal_git_api_endpoint)
+
     @classmethod
     def fromBranchMergeProposal(cls, bmp):
         """Create a `PreviewDiff` from a `BranchMergeProposal`.
@@ -407,34 +428,62 @@
         :param bmp: The `BranchMergeProposal` to generate a `PreviewDiff` for.
         :return: A `PreviewDiff`.
         """
-        source_branch = bmp.source_branch.getBzrBranch()
-        source_revision = source_branch.last_revision()
-        target_branch = bmp.target_branch.getBzrBranch()
-        target_revision = target_branch.last_revision()
-        if bmp.prerequisite_branch is not None:
-            prerequisite_branch = bmp.prerequisite_branch.getBzrBranch()
+        if bmp.source_branch is not None:
+            source_branch = bmp.source_branch.getBzrBranch()
+            source_revision = source_branch.last_revision()
+            target_branch = bmp.target_branch.getBzrBranch()
+            target_revision = target_branch.last_revision()
+            if bmp.prerequisite_branch is not None:
+                prerequisite_branch = bmp.prerequisite_branch.getBzrBranch()
+            else:
+                prerequisite_branch = None
+            diff, conflicts = Diff.mergePreviewFromBranches(
+                source_branch, source_revision, target_branch,
+                prerequisite_branch)
+            preview = cls()
+            preview.source_revision_id = source_revision.decode('utf-8')
+            preview.target_revision_id = target_revision.decode('utf-8')
+            preview.branch_merge_proposal = bmp
+            preview.diff = diff
+            preview.conflicts = u''.join(
+                unicode(conflict) + '\n' for conflict in conflicts)
         else:
-            prerequisite_branch = None
-        diff, conflicts = Diff.mergePreviewFromBranches(
-            source_branch, source_revision, target_branch, prerequisite_branch)
-        preview = cls()
-        preview.source_revision_id = source_revision.decode('utf-8')
-        preview.target_revision_id = target_revision.decode('utf-8')
-        preview.branch_merge_proposal = bmp
-        preview.diff = diff
-        preview.conflicts = u''.join(
-            unicode(conflict) + '\n' for conflict in conflicts)
+            hosting_client = cls.getGitHostingClient()
+            source_repository = bmp.source_git_repository
+            target_repository = bmp.target_git_repository
+            if source_repository == target_repository:
+                path = source_repository.getInternalPath()
+            else:
+                path = "%s:%s" % (
+                    target_repository.getInternalPath(),
+                    source_repository.getInternalPath())
+            # XXX cjwatson 2015-04-30: Add prerequisite handling once turnip
+            # supports it.
+            response = hosting_client.getMergeDiff(
+                path, bmp.source_git_ref.commit_sha1,
+                bmp.target_git_ref.commit_sha1)
+            source_revision_id = bmp.source_git_ref.commit_sha1
+            target_revision_id = bmp.target_git_ref.commit_sha1
+            if bmp.prerequisite_git_ref is not None:
+                prerequisite_revision_id = bmp.prerequisite_git_ref.commit_sha1
+            else:
+                prerequisite_revision_id = None
+            conflicts = u"".join(
+                u"Conflict in %s\n" % path for path in response['conflicts'])
+            preview = cls.create(
+                bmp, response['patch'], source_revision_id, target_revision_id,
+                prerequisite_revision_id, conflicts, strip_prefix_segments=1)
         del get_property_cache(bmp).preview_diffs
         del get_property_cache(bmp).preview_diff
         return preview
 
     @classmethod
     def create(cls, bmp, diff_content, source_revision_id, target_revision_id,
-               prerequisite_revision_id, conflicts):
+               prerequisite_revision_id, conflicts, strip_prefix_segments=0):
         """Create a PreviewDiff with specified values.
 
         :param bmp: The `BranchMergeProposal` this diff references.
-        :param diff_content: The text of the dift, as bytes.
+        :param diff_content: The text of the diff, as bytes.
         :param source_revision_id: The revision_id of the source branch.
         :param target_revision_id: The revision_id of the target branch.
         :param prerequisite_revision_id: The revision_id of the prerequisite
@@ -444,7 +493,9 @@
         """
         filename = str(uuid1()) + '.txt'
         size = len(diff_content)
-        diff = Diff.fromFile(StringIO(diff_content), size, filename)
+        diff = Diff.fromFile(
+            StringIO(diff_content), size, filename,
+            strip_prefix_segments=strip_prefix_segments)
 
         preview = cls()
         preview.branch_merge_proposal = bmp

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposaljobs.py'
--- lib/lp/code/model/tests/test_branchmergeproposaljobs.py	2014-05-29 07:08:17 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposaljobs.py	2015-04-30 13:12:11 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for branch merge proposal jobs."""
@@ -38,6 +38,7 @@
     IUpdatePreviewDiffJob,
     IUpdatePreviewDiffJobSource,
     )
+from lp.code.interfaces.gitrepository import GIT_FEATURE_FLAG
 from lp.code.model.branchmergeproposaljob import (
     BranchMergeProposalJob,
     BranchMergeProposalJobDerived,
@@ -219,6 +220,14 @@
             JobRunner([job]).runAll()
         self.checkExampleMerge(bmp.preview_diff.text)
 
+    def test_run_git(self):
+        self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+        bmp, _, _, patch = self.createExampleGitMerge()
+        job = UpdatePreviewDiffJob.create(bmp)
+        with dbuser("merge-proposal-jobs"):
+            JobRunner([job]).runAll()
+        self.assertEqual(patch, bmp.preview_diff.text)
+
     def test_run_object_events(self):
         # While the job runs a single IObjectModifiedEvent is issued when the
         # preview diff has been calculated.
@@ -514,6 +523,16 @@
         self.assertEqual(job.branch_merge_proposal, bmp)
         self.assertIsInstance(job, MergeProposalUpdatedEmailJob)
 
+    def test_iterReady_supports_git(self):
+        # iterReady supports merge proposals based on Git.  (These are
+        # currently considered ready regardless of scanning, since the hard
+        # work is done by the backend.)
+        self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+        bmp = self.factory.makeBranchMergeProposalForGit()
+        [job] = self.job_source.iterReady()
+        self.assertEqual(bmp, job.branch_merge_proposal)
+        self.assertIsInstance(job, UpdatePreviewDiffJob)
+
 
 class TestCodeReviewCommentEmailJob(TestCaseWithFactory):
 

=== modified file 'lib/lp/code/model/tests/test_diff.py'
--- lib/lp/code/model/tests/test_diff.py	2014-02-26 13:38:39 +0000
+++ lib/lp/code/model/tests/test_diff.py	2015-04-30 13:12:11 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for Diff, etc."""
@@ -8,6 +8,7 @@
 from cStringIO import StringIO
 from difflib import unified_diff
 import logging
+from textwrap import dedent
 
 from bzrlib import trace
 from bzrlib.patches import (
@@ -15,6 +16,7 @@
     parse_patches,
     RemoveLine,
     )
+from fixtures import MonkeyPatch
 import transaction
 from zope.security.proxy import removeSecurityProxy
 
@@ -24,11 +26,13 @@
     IIncrementalDiff,
     IPreviewDiff,
     )
+from lp.code.interfaces.gitrepository import GIT_FEATURE_FLAG
 from lp.code.model.diff import (
     Diff,
     PreviewDiff,
     )
 from lp.code.model.directbranchcommit import DirectBranchCommit
+from lp.services.features.testing import FeatureFixture
 from lp.services.librarian.interfaces.client import (
     LIBRARIAN_SERVER_DEFAULT_TIMEOUT,
     )
@@ -40,6 +44,7 @@
     TestCaseWithFactory,
     verifyObject,
     )
+from lp.testing.fakemethod import FakeMethod
 from lp.testing.layers import (
     LaunchpadFunctionalLayer,
     LaunchpadZopelessLayer,
@@ -86,6 +91,10 @@
     return bmp, source_rev_id, target_rev_id
 
 
+class FakeGitHostingClient:
+    pass
+
+
 class DiffTestCase(TestCaseWithFactory):
 
     def createExampleMerge(self):
@@ -128,6 +137,44 @@
         return (source_bzr, source_rev_id, target_bzr, prerequisite_bzr,
                 prerequisite)
 
+    def installFakeGitMergeDiff(self, result=None, failure=None):
+        hosting_client = FakeGitHostingClient()
+        hosting_client.getMergeDiff = FakeMethod(
+            result=result, failure=failure)
+        self.useFixture(MonkeyPatch(
+            "lp.code.model.diff.PreviewDiff.getGitHostingClient",
+            staticmethod(lambda: hosting_client)))
+
+    def createExampleGitMerge(self):
+        """Create an example Git-based merge scenario.
+
+        The actual diff work is done in turnip, and we have no turnip
+        fixture as yet, so for the moment we just install a fake hosting
+        client that will return a plausible-looking diff.
+        """
+        bmp = self.factory.makeBranchMergeProposalForGit()
+        source_sha1 = bmp.source_git_ref.commit_sha1
+        target_sha1 = bmp.target_git_ref.commit_sha1
+        patch = dedent("""\
+            diff --git a/foo b/foo
+            index %s..%s 100644
+            --- a/foo
+            +++ b/foo
+            @@ -1,2 +1,7 @@
+            +<<<<<<< foo
+             c
+            +=======
+            +d
+            +>>>>>>> foo
+             a
+            +b
+            """) % (target_sha1[:7], source_sha1[:7])
+        self.installFakeGitMergeDiff(result={
+            "patch": patch,
+            "conflicts": ["foo"],
+            })
+        return bmp, source_sha1, target_sha1, patch
+
 
 class TestDiff(DiffTestCase):
 
@@ -329,6 +376,10 @@
 
     layer = LaunchpadFunctionalLayer
 
+    def setUp(self):
+        super(TestPreviewDiff, self).setUp()
+        self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+
     def _createProposalWithPreviewDiff(self, prerequisite_branch=None,
                                        content=None):
         # Create and return a preview diff.
@@ -348,6 +399,24 @@
         transaction.commit()
         return mp
 
+    def _createGitProposalWithPreviewDiff(self, merge_prerequisite=None,
+                                          content=None):
+        mp = self.factory.makeBranchMergeProposalForGit(
+            prerequisite_ref=merge_prerequisite)
+        login_person(mp.registrant)
+        if merge_prerequisite is None:
+            prerequisite_revision_id = None
+        else:
+            prerequisite_revision_id = u"c" * 40
+        if content is None:
+            content = "".join(unified_diff("", "content"))
+        mp.updatePreviewDiff(
+            content, u"a" * 40, u"b" * 40,
+            prerequisite_revision_id=prerequisite_revision_id)
+        # Make sure the librarian file is written.
+        transaction.commit()
+        return mp
+
     def test_providesInterface(self):
         # In order to test the interface provision, we need to make sure that
         # the associated diff object that is delegated to is also created.
@@ -365,7 +434,7 @@
 
     def test_title(self):
         # PreviewDiff has title and it copes with absent
-        # BranchRevision{.sequence} when branches get overridden/rebased. 
+        # BranchRevision{.sequence} when branches get overridden/rebased.
         mp = self._createProposalWithPreviewDiff(content='')
         preview = mp.preview_diff
         # Both, source and target, branch revisions are absent,
@@ -386,6 +455,12 @@
         target_rev.sequence = 1
         self.assertEqual('r10 into r1', preview.title)
 
+    def test_title_git(self):
+        # Git commit IDs are shortened to seven characters.
+        mp = self._createGitProposalWithPreviewDiff(content='')
+        preview = mp.preview_diff
+        self.assertEqual('aaaaaaa into bbbbbbb', preview.title)
+
     def test_empty_diff(self):
         # Once the source is merged into the target, the diff between the
         # branches will be empty.
@@ -495,6 +570,24 @@
         finally:
             logger.removeHandler(handler)
 
+    def test_fromBranchMergeProposalForGit(self):
+        # Correctly generates a PreviewDiff from a Git-based
+        # BranchMergeProposal.
+        bmp, source_rev_id, target_rev_id, patch = self.createExampleGitMerge()
+        preview = PreviewDiff.fromBranchMergeProposal(bmp)
+        self.assertEqual(source_rev_id, preview.source_revision_id)
+        self.assertEqual(target_rev_id, preview.target_revision_id)
+        transaction.commit()
+        self.assertEqual(patch, preview.text)
+        self.assertEqual({'foo': (5, 0)}, preview.diffstat)
+
+    def test_fromBranchMergeProposalForGit_sets_conflicts(self):
+        # Conflicts are set on the PreviewDiff.
+        bmp, source_rev_id, target_rev_id, _ = self.createExampleGitMerge()
+        preview = PreviewDiff.fromBranchMergeProposal(bmp)
+        self.assertEqual('Conflict in foo\n', preview.conflicts)
+        self.assertTrue(preview.has_conflicts)
+
     def test_getFileByName(self):
         diff = self._createProposalWithPreviewDiff().preview_diff
         self.assertEqual(diff.diff_text, diff.getFileByName('preview.diff'))