launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18435
Re: [Merge] lp:~cjwatson/launchpad/git-mp-diffs into lp:launchpad
Diff comments:
> === modified file 'database/schema/security.cfg'
> --- database/schema/security.cfg 2015-04-21 10:34:24 +0000
> +++ database/schema/security.cfg 2015-04-29 14:40:47 +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-29 14:40:47 +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-29 14:40:47 +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-29 14:40:47 +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-29 14:40:47 +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)])
> + ('source_branch', bmp.merge_source.unique_name),
> + ('target_branch', 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,23 @@
> """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)
> + else:
> + if bmp.source_git_ref.commit_sha1 is None:
> + raise UpdatePreviewDiffNotReady(
> + 'The source branch of %s has not been scanned.' % url)
> + if bmp.target_git_ref.commit_sha1 is None:
> + raise UpdatePreviewDiffNotReady(
> + 'The source branch of %s has not been scanned.' % url)
>
> def acquireLease(self, duration=600):
> return self.job.acquireLease(duration)
> @@ -366,7 +376,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 +671,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
I traced it back to the commit that introduced them and they were never used; I couldn't come up with a rationale. My guess is that the person who wrote this code originally had something that used them, but it didn't make it into their initial commit. Anyway the extra tables seem entirely unhelpful here.
> # the date_created, then job type. This should give us all creation
> # jobs before comment jobs.
> @@ -680,7 +690,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-29 14:40:47 +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,7 @@
> 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_slashes=0):
> """Create a Diff from a textual diff.
>
> :diff_content: The diff text
> @@ -254,7 +255,8 @@
> 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_slashes=strip_slashes)
> except Exception:
> getUtility(IErrorReportingUtility).raising(sys.exc_info())
> # Set the diffstat to be empty.
> @@ -272,10 +274,13 @@
> removed_lines_count=removed_lines_count)
>
> @staticmethod
> - def generateDiffstat(diff_bytes):
> + def generateDiffstat(diff_bytes, strip_slashes=0):
> """Generate statistics about the provided diff.
>
> :param diff_bytes: A unified diff, as bytes.
> + :param strip_slashes: 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 +290,8 @@
> if not isinstance(patch, Patch):
> continue
> path = patch.newname.split('\t')[0]
> + if strip_slashes:
> + path = path.split('/', strip_slashes)[-1]
> file_stats[path] = tuple(patch.stats_values()[:2])
> return file_stats
>
> @@ -378,27 +385,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 +426,60 @@
> :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())
> + 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_slashes=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_slashes=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 +489,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_slashes=strip_slashes)
>
> 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-29 14:40:47 +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-29 14:40:47 +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'))
>
--
https://code.launchpad.net/~cjwatson/launchpad/git-mp-diffs/+merge/257768
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
References
