← Back to team overview

launchpad-reviewers team mailing list archive

Re: [Merge] lp:~cjwatson/launchpad/git-detect-merges into lp:launchpad

 

Review: Approve code



Diff comments:

> === modified file 'lib/lp/code/event/git.py'
> --- lib/lp/code/event/git.py	2015-06-06 08:49:54 +0000
> +++ lib/lp/code/event/git.py	2015-06-12 17:59:30 +0000
> @@ -19,6 +19,7 @@
>  
>      implements(IGitRefsUpdatedEvent)
>  
> -    def __init__(self, repository, paths):
> +    def __init__(self, repository, paths, logger):
>          super(GitRefsUpdatedEvent, self).__init__(repository)
>          self.paths = paths
> +        self.logger = logger
> 
> === modified file 'lib/lp/code/interfaces/gitrepository.py'
> --- lib/lp/code/interfaces/gitrepository.py	2015-06-12 17:59:30 +0000
> +++ lib/lp/code/interfaces/gitrepository.py	2015-06-12 17:59:30 +0000
> @@ -249,12 +249,13 @@
>          :return: An `IGitRef`, or None.
>          """
>  
> -    def createOrUpdateRefs(refs_info, get_objects=False):
> +    def createOrUpdateRefs(refs_info, get_objects=False, logger=None):
>          """Create or update a set of references in this repository.
>  
>          :param refs_info: A dict mapping ref paths to
>              {"sha1": sha1, "type": `GitObjectType`}.
>          :param get_objects: Return the created/updated references.
> +        :param logger: An optional logger.
>  
>          :return: A list of the created/updated references if get_objects,
>              otherwise None.
> @@ -287,13 +288,14 @@
>          :param logger: An optional logger.
>          """
>  
> -    def synchroniseRefs(refs_to_upsert, refs_to_remove):
> +    def synchroniseRefs(refs_to_upsert, refs_to_remove, logger=None):
>          """Synchronise references with those from the hosting service.
>  
>          :param refs_to_upsert: A dictionary mapping ref paths to
>              dictionaries of their fields; these refs will be created or
>              updated as appropriate.
>          :param refs_to_remove: A set of ref paths to remove.
> +        :param logger: An optional logger.
>          """
>  
>      def setOwnerDefault(value):
> @@ -482,6 +484,16 @@
>          "Whether there are recent changes in this repository that have not "
>          "yet been scanned.")
>  
> +    def updateMergeCommitIDs(paths):
> +        """Update commit SHA1s of merge proposals for this repository.
> +
> +        The *_git_commit_sha1 columns of merge proposals are stored
> +        explicitly in order that merge proposals are still meaningful after
> +        associated refs have been deleted.  However, active merge proposals
> +        where the refs in question still exist should have these columns
> +        kept up to date.
> +        """
> +
>      def scheduleDiffUpdates(paths):
>          """Create UpdatePreviewDiffJobs for landing targets.
>  
> @@ -490,6 +502,15 @@
>              diffs updated.
>          """
>  
> +    def detectMerges(paths, logger=None):
> +        """Detect merges of landing candidates.
> +
> +        :param paths: A list of reference paths.  Any merge proposals whose
> +            target is this repository and one of these paths will be
> +            checked.
> +        :param logger: An optional logger.
> +        """
> +
>  
>  class IGitRepositoryModerateAttributes(Interface):
>      """IGitRepository attributes that can be edited by more than one community.
> 
> === modified file 'lib/lp/code/model/githosting.py'
> --- lib/lp/code/model/githosting.py	2015-06-12 17:59:30 +0000
> +++ lib/lp/code/model/githosting.py	2015-06-12 17:59:30 +0000
> @@ -43,70 +43,59 @@
>          # over, but is there some more robust way to do this?
>          return 5.0
>  
> +    def _request(self, method, path, json_data=None, **kwargs):
> +        session = self._makeSession()
> +        if json_data is not None:
> +            # XXX cjwatson 2015-03-01: Once we're on requests >= 2.4.2, we
> +            # should just pass json through directly and drop the explicit
> +            # Content-Type header.
> +            kwargs.setdefault("headers", {})["Content-Type"] = (
> +                "application/json")
> +            kwargs["data"] = json.dumps(json_data)
> +        response = getattr(session, method)(
> +            urljoin(self.endpoint, path), timeout=self.timeout, **kwargs)
> +        if response.status_code != 200:
> +            raise Exception(response.text)

Can you make this exception class slightly more specific?

> +        return response.json()
> +
> +    def _get(self, path, **kwargs):
> +        return self._request("get", path, **kwargs)
> +
> +    def _post(self, path, **kwargs):
> +        return self._request("post", path, **kwargs)
> +
> +    def _delete(self, path, **kwargs):
> +        return self._request("delete", path, **kwargs)
> +
>      def create(self, path, clone_from=None):
>          try:
> -            # XXX cjwatson 2015-03-01: Once we're on requests >= 2.4.2, we
> -            # should just use post(json=) and drop the explicit Content-Type
> -            # header.
>              if clone_from:
>                  request = {"repo_path": path, "clone_from": clone_from}
>              else:
>                  request = {"repo_path": path}
> -            response = self._makeSession().post(
> -                urljoin(self.endpoint, "/repo"),
> -                headers={"Content-Type": "application/json"},
> -                data=json.dumps(request),
> -                timeout=self.timeout)
> +            self._post("/repo", json_data=request)
>          except Exception as e:
>              raise GitRepositoryCreationFault(
>                  "Failed to create Git repository: %s" % unicode(e))
> -        if response.status_code != 200:
> -            raise GitRepositoryCreationFault(
> -                "Failed to create Git repository: %s" % response.text)
>  
>      def getRefs(self, path):
>          try:
> -            response = self._makeSession().get(
> -                urljoin(self.endpoint, "/repo/%s/refs" % path),
> -                timeout=self.timeout)
> +            return self._get("/repo/%s/refs" % path)
>          except Exception as e:
>              raise GitRepositoryScanFault(
>                  "Failed to get refs from Git repository: %s" % unicode(e))
> -        if response.status_code != 200:
> -            raise GitRepositoryScanFault(
> -                "Failed to get refs from Git repository: %s" % response.text)
> -        try:
> -            return response.json()
> -        except ValueError as e:
> -            raise GitRepositoryScanFault(
> -                "Failed to decode ref-scan response: %s" % unicode(e))
>  
>      def getCommits(self, path, commit_oids, logger=None):
>          commit_oids = list(commit_oids)
>          try:
> -            # XXX cjwatson 2015-03-01: Once we're on requests >= 2.4.2, we
> -            # should just use post(json=) and drop the explicit Content-Type
> -            # header.
>              if logger is not None:
>                  logger.info("Requesting commit details for %s" % commit_oids)
> -            response = self._makeSession().post(
> -                urljoin(self.endpoint, "/repo/%s/commits" % path),
> -                headers={"Content-Type": "application/json"},
> -                data=json.dumps({"commits": commit_oids}),
> -                timeout=self.timeout)
> +            return self._post(
> +                "/repo/%s/commits" % path, json_data={"commits": commit_oids})
>          except Exception as e:
>              raise GitRepositoryScanFault(
>                  "Failed to get commit details from Git repository: %s" %
>                  unicode(e))
> -        if response.status_code != 200:
> -            raise GitRepositoryScanFault(
> -                "Failed to get commit details from Git repository: %s" %
> -                response.text)
> -        try:
> -            return response.json()
> -        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.
> @@ -121,35 +110,40 @@
>                  logger.info(
>                      "Requesting merge diff for %s from %s to %s" % (
>                          path, base, head))
> -            response = self._makeSession().get(
> -                urljoin(
> -                    self.endpoint,
> -                    "/repo/%s/compare-merge/%s:%s" % (path, base, head)),
> -                timeout=self.timeout)
> +            return self._get(
> +                "/repo/%s/compare-merge/%s:%s" % (path, base, head))
>          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" %
> -                response.text)
> +
> +    def detectMerges(self, path, target, sources, logger=None):
> +        """Detect merges of any of 'sources' into 'target'.

I'd document that sources and target both refer to commit SHA-1s.

> +
> +        :return: A dict mapping merged commit OIDs from 'sources' to the
> +            first commit OID in the left-hand (first parent only) history of
> +            'target' that is a descendant of the corresponding source
> +            commit.  Unmerged commits are omitted.
> +        """
> +        sources = list(sources)
>          try:
> -            return response.json()
> -        except ValueError as e:
> +            if logger is not None:
> +                logger.info(
> +                    "Detecting merges for %s from %s to %s" % (
> +                        path, sources, target))
> +            return self._post(
> +                "/repo/%s/detect-merges/%s" % (path, target),
> +                json_data={"sources": sources})
> +        except Exception as e:
>              raise GitRepositoryScanFault(
> -                "Failed to decode merge-diff response: %s" % unicode(e))
> +                "Failed to detect merges in Git repository: %s" % unicode(e))
>  
>      def delete(self, path, logger=None):
>          """Delete a repository."""
>          try:
>              if logger is not None:
>                  logger.info("Deleting repository %s" % path)
> -            response = self._makeSession().delete(
> -                urljoin(self.endpoint, "/repo/%s" % path))
> +            return self._delete("/repo/%s" % path)
>          except Exception as e:
>              raise GitRepositoryDeletionFault(
>                  "Failed to delete Git repository: %s" % unicode(e))
> -        if response.status_code != 200:
> -            raise GitRepositoryDeletionFault(
> -                "Failed to delete Git repository: %s" % response.text)
> 
> === modified file 'lib/lp/code/model/gitjob.py'
> --- lib/lp/code/model/gitjob.py	2015-06-12 17:59:30 +0000
> +++ lib/lp/code/model/gitjob.py	2015-06-12 17:59:30 +0000
> @@ -210,7 +210,8 @@
>                      self.repository.planRefChanges(hosting_path, logger=log))
>                  self.repository.fetchRefCommits(
>                      hosting_path, refs_to_upsert, logger=log)
> -                self.repository.synchroniseRefs(refs_to_upsert, refs_to_remove)
> +                self.repository.synchroniseRefs(
> +                    refs_to_upsert, refs_to_remove, logger=log)
>          except LostObjectError:
>              log.info(
>                  "Skipping repository %s because it has been deleted." %
> 
> === modified file 'lib/lp/code/model/gitrepository.py'
> --- lib/lp/code/model/gitrepository.py	2015-06-12 17:59:30 +0000
> +++ lib/lp/code/model/gitrepository.py	2015-06-12 17:59:30 +0000
> @@ -10,7 +10,12 @@
>  
>  from datetime import datetime
>  import email
> -from itertools import chain
> +from functools import partial
> +from itertools import (
> +    chain,
> +    groupby,
> +    )
> +from operator import attrgetter
>  
>  from bzrlib import urlutils
>  import pytz
> @@ -71,6 +76,7 @@
>  from lp.code.event.git import GitRefsUpdatedEvent
>  from lp.code.interfaces.branchmergeproposal import (
>      BRANCH_MERGE_PROPOSAL_FINAL_STATES,
> +    notify_modified,
>      )
>  from lp.code.interfaces.gitcollection import (
>      IAllGitRepositories,
> @@ -440,7 +446,7 @@
>              sha1 = sha1.decode("US-ASCII")
>          return {"sha1": sha1, "type": object_type_map[obj["type"]]}
>  
> -    def createOrUpdateRefs(self, refs_info, get_objects=False):
> +    def createOrUpdateRefs(self, refs_info, get_objects=False, logger=None):
>          """See `IGitRepository`."""
>          def dbify_values(values):
>              return [
> @@ -509,7 +515,8 @@
>  
>          self.date_last_modified = UTC_NOW
>          if updated:
> -            notify(GitRefsUpdatedEvent(self, [value[1] for value in updated]))
> +            notify(GitRefsUpdatedEvent(
> +                self, [value[1] for value in updated], logger))
>          if get_objects:
>              return bulk.load(GitRef, updated + created)
>  
> @@ -597,10 +604,10 @@
>              if committer is not None:
>                  info["committer"] = committer.id
>  
> -    def synchroniseRefs(self, refs_to_upsert, refs_to_remove):
> +    def synchroniseRefs(self, refs_to_upsert, refs_to_remove, logger=None):
>          """See `IGitRepository`."""
>          if refs_to_upsert:
> -            self.createOrUpdateRefs(refs_to_upsert)
> +            self.createOrUpdateRefs(refs_to_upsert, logger=logger)
>          if refs_to_remove:
>              self.removeRefs(refs_to_remove)
>  
> @@ -812,6 +819,15 @@
>              Not(BranchMergeProposal.queue_status.is_in(
>                  BRANCH_MERGE_PROPOSAL_FINAL_STATES)))
>  
> +    def getActiveLandingCandidates(self, paths):
> +        """Merge proposals not in final states where these refs are target."""
> +        return Store.of(self).find(
> +            BranchMergeProposal,
> +            BranchMergeProposal.target_git_repository == self,
> +            BranchMergeProposal.target_git_path.is_in(paths),
> +            Not(BranchMergeProposal.queue_status.is_in(
> +                BRANCH_MERGE_PROPOSAL_FINAL_STATES)))
> +
>      @property
>      def dependent_landings(self):
>          """See `IGitRepository`."""
> @@ -844,6 +860,52 @@
>              Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
>          return not jobs.is_empty()
>  
> +    def updateMergeCommitIDs(self, paths):
> +        """See `IGitRepository`."""
> +        store = Store.of(self)
> +        refs = {
> +            path: commit_sha1 for path, commit_sha1 in store.find(
> +                (GitRef.path, GitRef.commit_sha1),
> +                GitRef.repository_id == self.id,
> +                GitRef.path.is_in(paths))}
> +        updated = set()
> +        for kind in ("source", "target", "prerequisite"):
> +            repository_name = "%s_git_repositoryID" % kind
> +            path_name = "%s_git_path" % kind
> +            commit_sha1_name = "%s_git_commit_sha1" % kind
> +            old_column = partial(getattr, BranchMergeProposal)
> +            db_kind = "dependent" if kind == "prerequisite" else kind
> +            column_types = [
> +                ("%s_git_path" % db_kind, "text"),
> +                ("%s_git_commit_sha1" % db_kind, "character(40)"),
> +                ]
> +            db_values = [(
> +                bulk.dbify_value(old_column(path_name), path),
> +                bulk.dbify_value(old_column(commit_sha1_name), commit_sha1)
> +                ) for path, commit_sha1 in refs.items()]
> +            new_proposals_expr = Values(
> +                "new_proposals", column_types, db_values)
> +            new_proposals = ClassAlias(BranchMergeProposal, "new_proposals")
> +            new_column = partial(getattr, new_proposals)
> +            updated_columns = {
> +                old_column(commit_sha1_name): new_column(commit_sha1_name)}
> +            update_filter = And(
> +                old_column(repository_name) == self.id,
> +                old_column(path_name) == new_column(path_name),
> +                Not(BranchMergeProposal.queue_status.is_in(
> +                    BRANCH_MERGE_PROPOSAL_FINAL_STATES)))
> +            result = store.execute(Returning(BulkUpdate(
> +                updated_columns, table=BranchMergeProposal,
> +                values=new_proposals_expr, where=update_filter,
> +                primary_columns=BranchMergeProposal.id)))
> +            updated.update(item[0] for item in result)
> +        if updated:
> +            # Some existing BranchMergeProposal objects may no longer be
> +            # valid.  Without knowing which ones we already have, it's
> +            # safest to just invalidate everything.
> +            store.invalidate()

Storm really should provide a public API for this, but it doesn't. We're not calling this frequently, so invalidating everything is fine.

If you want a good laugh, search for _iter_alive in ResultSet.set. ResultSet.cached is also some of the best comedy I've seen in years.

> +        return updated
> +
>      def scheduleDiffUpdates(self, paths):
>          """See `IGitRepository`."""
>          from lp.code.model.branchmergeproposaljob import UpdatePreviewDiffJob
> @@ -852,6 +914,33 @@
>              jobs.append(UpdatePreviewDiffJob.create(merge_proposal))
>          return jobs
>  
> +    def _markProposalMerged(self, proposal, merged_revision_id, logger=None):
> +        if logger is not None:
> +            logger.info(
> +                "Merge detected: %s => %s",
> +                proposal.source_git_ref.identity,
> +                proposal.target_git_ref.identity)
> +        notify_modified(
> +            proposal, proposal.markAsMerged,
> +            merged_revision_id=merged_revision_id)
> +
> +    def detectMerges(self, paths, logger=None):
> +        """See `IGitRepository`."""
> +        hosting_client = getUtility(IGitHostingClient)
> +        all_proposals = self.getActiveLandingCandidates(paths).order_by(
> +            BranchMergeProposal.target_git_path)
> +        for _, group in groupby(all_proposals, attrgetter("target_git_path")):
> +            proposals = list(group)
> +            merges = hosting_client.detectMerges(
> +                self.getInternalPath(), proposals[0].target_git_commit_sha1,
> +                set(proposal.source_git_commit_sha1 for proposal in proposals))
> +            for proposal in proposals:
> +                merged_revision_id = merges.get(
> +                    proposal.source_git_commit_sha1)
> +                if merged_revision_id is not None:
> +                    self._markProposalMerged(
> +                        proposal, merged_revision_id, logger=logger)
> +
>      def canBeDeleted(self):
>          """See `IGitRepository`."""
>          # Can't delete if the repository is associated with anything.
> 
> === modified file 'lib/lp/code/model/tests/test_gitrepository.py'
> --- lib/lp/code/model/tests/test_gitrepository.py	2015-06-12 17:59:30 +0000
> +++ lib/lp/code/model/tests/test_gitrepository.py	2015-06-12 17:59:30 +0000
> @@ -37,6 +37,7 @@
>      )
>  from lp.app.interfaces.launchpad import ILaunchpadCelebrities
>  from lp.code.enums import (
> +    BranchMergeProposalStatus,
>      BranchSubscriptionDiffSize,
>      BranchSubscriptionNotificationLevel,
>      CodeReviewNotificationLevel,
> @@ -101,6 +102,7 @@
>  from lp.registry.tests.test_accesspolicy import get_policies_for_artifact
>  from lp.services.config import config
>  from lp.services.database.constants import UTC_NOW
> +from lp.services.database.interfaces import IStore
>  from lp.services.job.interfaces.job import JobStatus
>  from lp.services.job.model.job import Job
>  from lp.services.job.runner import JobRunner
> @@ -125,6 +127,7 @@
>      LaunchpadFunctionalLayer,
>      LaunchpadZopelessLayer,
>      )
> +from lp.testing.mail_helpers import pop_notifications
>  from lp.testing.pages import webservice_for_person
>  
>  
> @@ -1016,6 +1019,9 @@
>          jobs = self._getWaitingUpdatePreviewDiffJobs(repository)
>          self.assertEqual(
>              [bmp, bmp], [job.branch_merge_proposal for job in jobs])
> +        self.assertEqual(
> +            u"0000000000000000000000000000000000000000",
> +            bmp.source_git_commit_sha1)
>  
>      def test_getRefByPath_without_leading_refs_heads(self):
>          [ref] = self.factory.makeGitRefs(paths=[u"refs/heads/master"])
> @@ -1618,6 +1624,66 @@
>                  target=commercial_project, user=owner)
>  
>  
> +class TestGitRepositoryUpdateMergeCommitIDs(TestCaseWithFactory):
> +
> +    layer = DatabaseFunctionalLayer
> +
> +    def test_updates_proposals(self):
> +        # `updateMergeCommitIDs` updates proposals for specified refs.
> +        repository = self.factory.makeGitRepository()
> +        paths = [u"refs/heads/1", u"refs/heads/2", u"refs/heads/3"]
> +        [ref1, ref2, ref3] = self.factory.makeGitRefs(
> +            repository=repository, paths=paths)
> +        bmp1 = self.factory.makeBranchMergeProposalForGit(
> +            target_ref=ref1, source_ref=ref2)
> +        bmp2 = self.factory.makeBranchMergeProposalForGit(
> +            target_ref=ref1, source_ref=ref3, prerequisite_ref=ref2)
> +        removeSecurityProxy(ref1).commit_sha1 = u"0" * 40
> +        removeSecurityProxy(ref2).commit_sha1 = u"1" * 40
> +        removeSecurityProxy(ref3).commit_sha1 = u"2" * 40
> +        self.assertNotEqual(ref1, bmp1.target_git_ref)
> +        self.assertNotEqual(ref2, bmp1.source_git_ref)
> +        self.assertNotEqual(ref1, bmp2.target_git_ref)
> +        self.assertNotEqual(ref3, bmp2.source_git_ref)
> +        self.assertNotEqual(ref2, bmp2.prerequisite_git_ref)
> +        self.assertContentEqual(
> +            [bmp1.id, bmp2.id], repository.updateMergeCommitIDs(paths))
> +        self.assertEqual(ref1, bmp1.target_git_ref)
> +        self.assertEqual(ref2, bmp1.source_git_ref)
> +        self.assertEqual(ref1, bmp2.target_git_ref)
> +        self.assertIsNone(bmp1.prerequisite_git_ref)
> +        self.assertEqual(ref3, bmp2.source_git_ref)
> +        self.assertEqual(ref2, bmp2.prerequisite_git_ref)
> +
> +    def test_skips_unspecified_refs(self):
> +        # `updateMergeCommitIDs` skips unspecified refs.
> +        repository1 = self.factory.makeGitRepository()
> +        paths = [u"refs/heads/1", u"refs/heads/2"]
> +        [ref1_1, ref1_2] = self.factory.makeGitRefs(
> +            repository=repository1, paths=paths)
> +        bmp1 = self.factory.makeBranchMergeProposalForGit(
> +            target_ref=ref1_1, source_ref=ref1_2)
> +        repository2 = self.factory.makeGitRepository(target=repository1.target)
> +        [ref2_1, ref2_2] = self.factory.makeGitRefs(
> +            repository=repository2, paths=paths)
> +        bmp2 = self.factory.makeBranchMergeProposalForGit(
> +            target_ref=ref2_1, source_ref=ref2_2)
> +        removeSecurityProxy(ref1_1).commit_sha1 = u"0" * 40
> +        removeSecurityProxy(ref1_2).commit_sha1 = u"1" * 40
> +        removeSecurityProxy(ref2_1).commit_sha1 = u"2" * 40
> +        removeSecurityProxy(ref2_2).commit_sha1 = u"3" * 40
> +        self.assertNotEqual(ref1_1, bmp1.target_git_ref)
> +        self.assertNotEqual(ref1_2, bmp1.source_git_ref)
> +        self.assertNotEqual(ref2_1, bmp2.target_git_ref)
> +        self.assertNotEqual(ref2_2, bmp2.source_git_ref)
> +        self.assertContentEqual(
> +            [bmp1.id], repository1.updateMergeCommitIDs([paths[0]]))
> +        self.assertEqual(ref1_1, bmp1.target_git_ref)
> +        self.assertNotEqual(ref1_2, bmp1.source_git_ref)
> +        self.assertNotEqual(ref2_1, bmp2.target_git_ref)
> +        self.assertNotEqual(ref2_2, bmp2.source_git_ref)
> +
> +
>  class TestGitRepositoryScheduleDiffUpdates(TestCaseWithFactory):
>  
>      layer = DatabaseFunctionalLayer
> @@ -1649,6 +1715,85 @@
>          self.assertEqual(0, len(jobs))
>  
>  
> +class TestGitRepositoryDetectMerges(TestCaseWithFactory):
> +
> +    layer = LaunchpadZopelessLayer
> +
> +    def test_markProposalMerged(self):
> +        # A merge proposal that is merged is marked as such.
> +        proposal = self.factory.makeBranchMergeProposalForGit()
> +        self.assertNotEqual(
> +            BranchMergeProposalStatus.MERGED, proposal.queue_status)
> +        proposal.target_git_repository._markProposalMerged(
> +            proposal, proposal.target_git_commit_sha1)
> +        self.assertEqual(
> +            BranchMergeProposalStatus.MERGED, proposal.queue_status)
> +        job = IStore(proposal).find(
> +            BranchMergeProposalJob,
> +            BranchMergeProposalJob.branch_merge_proposal == proposal,
> +            BranchMergeProposalJob.job_type ==
> +            BranchMergeProposalJobType.MERGE_PROPOSAL_UPDATED).one()
> +        derived_job = job.makeDerived()
> +        derived_job.run()
> +        notifications = pop_notifications()
> +        self.assertIn(
> +            "Work in progress => Merged",
> +            notifications[0].get_payload(decode=True))
> +        self.assertEqual(
> +            config.canonical.noreply_from_address, notifications[0]["From"])
> +        recipients = set(msg["x-envelope-to"] for msg in notifications)
> +        expected = set(
> +            [proposal.source_git_repository.registrant.preferredemail.email,
> +             proposal.target_git_repository.registrant.preferredemail.email])
> +        self.assertEqual(expected, recipients)
> +
> +    def test_update_detects_merges(self):
> +        # Pushing changes to a branch causes a check whether any active
> +        # merge proposals with that branch as the target have been merged.
> +        repository = self.factory.makeGitRepository()
> +        [target_1, target_2, source_1, source_2] = self.factory.makeGitRefs(
> +            repository, paths=[
> +                u"refs/heads/target-1",
> +                u"refs/heads/target-2",
> +                u"refs/heads/source-1",
> +                u"refs/heads/source-2",
> +                ])
> +        bmp1 = self.factory.makeBranchMergeProposalForGit(
> +            target_ref=target_1, source_ref=source_1)
> +        bmp2 = self.factory.makeBranchMergeProposalForGit(
> +            target_ref=target_1, source_ref=source_2)
> +        bmp3 = self.factory.makeBranchMergeProposalForGit(
> +            target_ref=target_2, source_ref=source_1)
> +        hosting_client = FakeMethod()
> +        hosting_client.detectMerges = FakeMethod(
> +            result={source_1.commit_sha1: u"0" * 40})
> +        self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
> +        repository.createOrUpdateRefs({
> +            u"refs/heads/target-1": {
> +                u"sha1": u"0" * 40,
> +                u"type": GitObjectType.COMMIT,
> +                },
> +            u"refs/heads/target-2": {
> +                u"sha1": u"1" * 40,
> +                u"type": GitObjectType.COMMIT,
> +                }
> +            })
> +        expected_args = [
> +            (repository.getInternalPath(), target_1.commit_sha1,
> +             set([source_1.commit_sha1, source_2.commit_sha1])),
> +            (repository.getInternalPath(), target_2.commit_sha1,
> +             set([source_1.commit_sha1])),
> +            ]
> +        self.assertContentEqual(
> +            expected_args, hosting_client.detectMerges.extract_args())
> +        self.assertEqual(BranchMergeProposalStatus.MERGED, bmp1.queue_status)
> +        self.assertEqual(u"0" * 40, bmp1.merged_revision_id)
> +        self.assertEqual(
> +            BranchMergeProposalStatus.WORK_IN_PROGRESS, bmp2.queue_status)
> +        self.assertEqual(BranchMergeProposalStatus.MERGED, bmp3.queue_status)
> +        self.assertEqual(u"0" * 40, bmp3.merged_revision_id)
> +
> +
>  class TestGitRepositorySet(TestCaseWithFactory):
>  
>      layer = DatabaseFunctionalLayer
> 
> === modified file 'lib/lp/code/subscribers/git.py'
> --- lib/lp/code/subscribers/git.py	2015-06-06 08:49:54 +0000
> +++ lib/lp/code/subscribers/git.py	2015-06-12 17:59:30 +0000
> @@ -8,4 +8,6 @@
>  
>  def refs_updated(repository, event):
>      """Some references in a Git repository have been updated."""
> +    repository.updateMergeCommitIDs(event.paths)
>      repository.scheduleDiffUpdates(event.paths)
> +    repository.detectMerges(event.paths, logger=event.logger)
> 
> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py	2015-06-11 08:06:28 +0000
> +++ lib/lp/testing/factory.py	2015-06-12 17:59:30 +0000
> @@ -1766,8 +1766,11 @@
>                  u"type": GitObjectType.COMMIT,
>                  }
>              for path in paths}
> -        return removeSecurityProxy(repository).createOrUpdateRefs(
> -            refs_info, get_objects=True)
> +        refs_by_path = {
> +            ref.path: ref
> +            for ref in removeSecurityProxy(repository).createOrUpdateRefs(
> +                refs_info, get_objects=True)}
> +        return [refs_by_path[path] for path in paths]
>  
>      def makeBug(self, target=None, owner=None, bug_watch_url=None,
>                  information_type=None, date_closed=None, title=None,
> 


-- 
https://code.launchpad.net/~cjwatson/launchpad/git-detect-merges/+merge/261877
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.


References