launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18433
Re: [Merge] lp:~cjwatson/launchpad/git-mp-diffs into lp:launchpad
Review: Approve code
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)])
Can you fix these two names? Nothing should depend on them.
> 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
Do we know why it was like that? Possibly for performance, but that seems dubious.
> # 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):
strip_slashes is weird. strip_prefix_segments?
> """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)
This could use an XXX about the lack of prereq support in the underlying API.
> + 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.)
What if we don't have SHA-1s for the involved refs yet?
> + 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
