Merge lp:~cjwatson/launchpad/git-update-related-bugs into lp:launchpad

[Colin Watson]

Automatically update related bugs for Git-based merge proposals based on their commit logs.

Automatically update related bugs for Git-based merge proposals based on their commit logs.

I've intentionally used the same regexes that Soyuz uses to parse changelogs, for compatibility with package branches and because it's reasonably compact in commit messages.

There's a corner case here with prerequisite branches where we may end up with too many related bugs, noted in an XXX comment. I think it's tolerable for the moment.

[Celso Providelo (cprov)]

Thanks, Colin.

While I agree that linking to bugs referred from prerequisite branches is tolerable, I wonder if we can chain the change in turnip to fix this problem soon-ish.

review: Approve

=== modified file 'lib/lp/answers/model/question.py'
--- lib/lp/answers/model/question.py	2015-10-08 08:59:00 +0000
+++ lib/lp/answers/model/question.py	2016-09-02 14:40:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Question models."""
@@ -669,11 +669,14 @@
             bulk.load(Bug, bug_ids), key=operator.attrgetter('id')))
 
     # IBugLinkTarget implementation
-    def createBugLink(self, bug):
+    def createBugLink(self, bug, props=None):
         """See BugLinkTargetMixin."""
+        if props is None:
+            props = {}
         # XXX: Should set creator.
         getUtility(IXRefSet).create(
-            {(u'question', unicode(self.id)): {(u'bug', unicode(bug.id)): {}}})
+            {(u'question', unicode(self.id)):
+                {(u'bug', unicode(bug.id)): props}})
 
     def deleteBugLink(self, bug):
         """See BugLinkTargetMixin."""

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2015-10-08 08:59:00 +0000
+++ lib/lp/blueprints/model/specification.py	2016-09-02 14:40:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -800,12 +800,14 @@
         return list(sorted(
             bulk.load(Bug, bug_ids), key=operator.attrgetter('id')))
 
-    def createBugLink(self, bug):
+    def createBugLink(self, bug, props=None):
         """See BugLinkTargetMixin."""
+        if props is None:
+            props = {}
         # XXX: Should set creator.
         getUtility(IXRefSet).create(
             {(u'specification', unicode(self.id)):
-                {(u'bug', unicode(bug.id)): {}}})
+                {(u'bug', unicode(bug.id)): props}})
 
     def deleteBugLink(self, bug):
         """See BugLinkTargetMixin."""

=== modified file 'lib/lp/bugs/interfaces/buglink.py'
--- lib/lp/bugs/interfaces/buglink.py	2015-09-30 01:51:52 +0000
+++ lib/lp/bugs/interfaces/buglink.py	2016-09-02 14:40:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Interfaces for objects that can be linked to bugs."""
@@ -69,7 +69,7 @@
                         value_type=Reference(schema=IBug), readonly=True),
         as_of="devel")
 
-    def linkBug(bug, user=None, check_permissions=True):
+    def linkBug(bug, user=None, check_permissions=True, props=None):
         """Link the object with this bug.
 
         If a new link is created by this method, an ObjectLinkedEvent is

=== modified file 'lib/lp/bugs/model/buglinktarget.py'
--- lib/lp/bugs/model/buglinktarget.py	2016-02-06 01:41:00 +0000
+++ lib/lp/bugs/model/buglinktarget.py	2016-09-02 14:40:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -41,7 +41,7 @@
 class BugLinkTargetMixin:
     """Mixin class for IBugLinkTarget implementation."""
 
-    def createBugLink(self, bug):
+    def createBugLink(self, bug, props=None):
         """Subclass should override that method to create a BugLink instance.
         """
         raise NotImplementedError("missing createBugLink() implementation")
@@ -52,14 +52,14 @@
         raise NotImplementedError("missing deleteBugLink() implementation")
 
     # IBugLinkTarget implementation
-    def linkBug(self, bug, user=None, check_permissions=True):
+    def linkBug(self, bug, user=None, check_permissions=True, props=None):
         """See IBugLinkTarget."""
         if check_permissions and not bug.userCanView(user):
             raise Unauthorized(
                 "Cannot link a private bug you don't have access to")
         if bug in self.bugs:
             return False
-        self.createBugLink(bug)
+        self.createBugLink(bug, props=props)
         notify(ObjectLinkedEvent(bug, self, user=user))
         notify(ObjectLinkedEvent(self, bug, user=user))
         return True

=== modified file 'lib/lp/bugs/model/cve.py'
--- lib/lp/bugs/model/cve.py	2015-09-28 12:25:52 +0000
+++ lib/lp/bugs/model/cve.py	2016-09-02 14:40:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -91,11 +91,13 @@
         assert ref.cve == self
         CveReference.delete(ref.id)
 
-    def createBugLink(self, bug):
+    def createBugLink(self, bug, props=None):
         """See BugLinkTargetMixin."""
+        if props is None:
+            props = {}
         # XXX: Should set creator.
         getUtility(IXRefSet).create(
-            {(u'cve', self.sequence): {(u'bug', unicode(bug.id)): {}}})
+            {(u'cve', self.sequence): {(u'bug', unicode(bug.id)): props}})
 
     def deleteBugLink(self, bug):
         """See BugLinkTargetMixin."""

=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py	2016-06-25 08:05:06 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py	2016-09-02 14:40:37 +0000
@@ -386,6 +386,12 @@
     def getRelatedBugTasks(user):
         """Return the Bug tasks related to this merge proposal."""
 
+    def updateRelatedBugsFromSource():
+        """Update related bug links based on commits in the source branch.
+
+        This is currently only meaningful for Git-based merge proposals.
+        """
+
     def getRevisionsSinceReviewStart():
         """Return all the revisions added since the review began.
 

=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py	2016-06-20 20:32:36 +0000
+++ lib/lp/code/interfaces/gitrepository.py	2016-09-02 14:40:37 +0000
@@ -508,8 +508,11 @@
         kept up to date.
         """
 
-    def scheduleDiffUpdates(paths):
-        """Create UpdatePreviewDiffJobs for landing targets.
+    def updateLandingTargets(paths):
+        """Update landing targets (MPs where this repository is the source).
+
+        For each merge proposal, update linked bugs and create
+        `UpdatePreviewDiffJob`s.
 
         :param paths: A list of reference paths.  Any merge proposals whose
             source is this repository and one of these paths will have their

=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py	2016-07-28 10:35:21 +0000
+++ lib/lp/code/model/branchmergeproposal.py	2016-09-02 14:40:37 +0000
@@ -12,6 +12,7 @@
 
 from email.utils import make_msgid
 from operator import attrgetter
+import sys
 
 from lazr.lifecycle.event import (
     ObjectCreatedEvent,
@@ -39,10 +40,12 @@
     )
 from storm.store import Store
 from zope.component import getUtility
+from zope.error.interfaces import IErrorReportingUtility
 from zope.event import notify
 from zope.interface import implementer
 
 from lp.app.enums import PRIVATE_INFORMATION_TYPES
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.bugs.interfaces.bugtask import IBugTaskSet
 from lp.bugs.interfaces.bugtaskfilter import filter_bugtasks_by_context
 from lp.bugs.interfaces.bugtasksearch import BugTaskSearchParams
@@ -122,7 +125,12 @@
     cachedproperty,
     get_property_cache,
     )
+from lp.services.webapp.errorlog import ScriptRequest
 from lp.services.xref.interfaces import IXRefSet
+from lp.soyuz.enums import (
+    re_bug_numbers,
+    re_lp_closes,
+    )
 
 
 def is_valid_transition(proposal, from_state, next_state, user=None):
@@ -178,6 +186,10 @@
         return True
 
 
+class TooManyRelatedBugs(Exception):
+    """A source branch has too many related bugs linked from commits."""
+
+
 @implementer(IBranchMergeProposal, IHasBranchTarget)
 class BranchMergeProposal(SQLBase, BugLinkTargetMixin):
     """A relationship between a person and a branch."""
@@ -391,12 +403,14 @@
             return filter_bugtasks_by_context(
                 self.source_git_repository.target, tasks)
 
-    def createBugLink(self, bug):
+    def createBugLink(self, bug, props=None):
         """See `BugLinkTargetMixin`."""
+        if props is None:
+            props = {}
         # XXX cjwatson 2016-06-11: Should set creator.
         getUtility(IXRefSet).create(
             {(u'merge_proposal', unicode(self.id)):
-                {(u'bug', unicode(bug.id)): {}}})
+                {(u'bug', unicode(bug.id)): props}})
 
     def deleteBugLink(self, bug):
         """See `BugLinkTargetMixin`."""
@@ -404,7 +418,7 @@
             {(u'merge_proposal', unicode(self.id)):
                 [(u'bug', unicode(bug.id))]})
 
-    def linkBug(self, bug, user=None, check_permissions=True):
+    def linkBug(self, bug, user=None, check_permissions=True, props=None):
         """See `BugLinkTargetMixin`."""
         if self.source_branch is not None:
             # For Bazaar, we currently only store bug/branch links.
@@ -412,7 +426,8 @@
         else:
             # Otherwise, link the bug to the merge proposal directly.
             return super(BranchMergeProposal, self).linkBug(
-                bug, user=user, check_permissions=check_permissions)
+                bug, user=user, check_permissions=check_permissions,
+                props=props)
 
     def unlinkBug(self, bug, user=None, check_permissions=True):
         """See `BugLinkTargetMixin`."""
@@ -428,6 +443,76 @@
             return super(BranchMergeProposal, self).unlinkBug(
                 bug, user=user, check_permissions=check_permissions)
 
+    def _reportTooManyRelatedBugs(self):
+        properties = [
+            ("error-explanation", (
+                "Number of related bugs exceeds limit %d." %
+                config.codehosting.related_bugs_from_source_limit)),
+            ("source", self.merge_source.identity),
+            ("target", self.merge_target.identity),
+            ]
+        if self.merge_prerequisite is not None:
+            properties.append(
+                ("prerequisite", self.merge_prerequisite.identity))
+        request = ScriptRequest(properties)
+        getUtility(IErrorReportingUtility).raising(sys.exc_info(), request)
+
+    def _fetchRelatedBugIDsFromSource(self):
+        """Fetch related bug IDs from the source branch."""
+        from lp.bugs.model.bug import Bug
+        # Only currently used for Git.
+        assert self.source_git_ref is not None
+        # XXX cjwatson 2016-06-11: This may return too many bugs in the case
+        # where a prerequisite branch fixes a bug which is not fixed by
+        # further commits on the source branch.  To fix this, we need
+        # turnip's log API to be able to take multiple stop parameters.
+        commits = self.getUnlandedSourceBranchRevisions()
+        bug_ids = set()
+        limit = config.codehosting.related_bugs_from_source_limit
+        try:
+            for commit in commits:
+                if "commit_message" in commit:
+                    for match in re_lp_closes.finditer(
+                            commit["commit_message"]):
+                        for bug_num in re_bug_numbers.findall(match.group(0)):
+                            bug_id = int(bug_num)
+                            if bug_id not in bug_ids and len(bug_ids) == limit:
+                                raise TooManyRelatedBugs()
+                            bug_ids.add(bug_id)
+        except TooManyRelatedBugs:
+            self._reportTooManyRelatedBugs()
+        # Only return bug IDs that exist.
+        return set(IStore(Bug).find(Bug.id, Bug.id.is_in(bug_ids)))
+
+    def updateRelatedBugsFromSource(self):
+        """See `IBranchMergeProposal`."""
+        from lp.bugs.model.bug import Bug
+        # Only currently used for Git.
+        assert self.source_git_ref is not None
+        current_bug_ids_from_source = {
+            int(id): (props['metadata'] or {}).get('from_source', False)
+            for (_, id), props in getUtility(IXRefSet).findFrom(
+                (u'merge_proposal', unicode(self.id)), types=[u'bug']).items()}
+        current_bug_ids = set(current_bug_ids_from_source)
+        new_bug_ids = self._fetchRelatedBugIDsFromSource()
+        # Only remove links marked as originating in the source branch.
+        remove_bugs = load(Bug, set(
+            bug_id for bug_id in current_bug_ids - new_bug_ids
+            if current_bug_ids_from_source[bug_id]))
+        for bug in remove_bugs:
+            self.unlinkBug(bug, check_permissions=False)
+        add_bugs = load(Bug, new_bug_ids - current_bug_ids)
+        # XXX cjwatson 2016-06-11: We could perhaps set creator and
+        # date_created based on commit information, but then we'd have to
+        # work out what to do in the case of multiple commits referring to
+        # the same bug, updating properties if more such commits arrive
+        # later, etc.  This is simple and does the job for now.
+        for bug in add_bugs:
+            self.linkBug(
+                bug, user=getUtility(ILaunchpadCelebrities).janitor,
+                check_permissions=False,
+                props={'metadata': {'from_source': True}})
+
     @property
     def address(self):
         return 'mp+%d@%s' % (self.id, config.launchpad.code_domain)

=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py	2016-06-20 20:32:36 +0000
+++ lib/lp/code/model/gitrepository.py	2016-09-02 14:40:37 +0000
@@ -967,11 +967,12 @@
             store.invalidate()
         return updated
 
-    def scheduleDiffUpdates(self, paths):
+    def updateLandingTargets(self, paths):
         """See `IGitRepository`."""
         from lp.code.model.branchmergeproposaljob import UpdatePreviewDiffJob
         jobs = []
         for merge_proposal in self.getActiveLandingTargets(paths):
+            merge_proposal.updateRelatedBugsFromSource()
             jobs.append(UpdatePreviewDiffJob.create(merge_proposal))
         return jobs
 

=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py	2016-06-24 23:35:24 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py	2016-09-02 14:40:37 +0000
@@ -10,6 +10,7 @@
     timedelta,
     )
 from difflib import unified_diff
+import hashlib
 from unittest import TestCase
 
 from lazr.lifecycle.event import (
@@ -21,7 +22,9 @@
 from sqlobject import SQLObjectNotFound
 from storm.locals import Store
 from testtools.matchers import (
+    ContainsDict,
     Equals,
+    Is,
     MatchesDict,
     MatchesStructure,
     )
@@ -57,6 +60,7 @@
     IBranchMergeProposalGetter,
     notify_modified,
     )
+from lp.code.interfaces.githosting import IGitHostingClient
 from lp.code.model.branchmergeproposal import (
     BranchMergeProposalGetter,
     is_valid_transition,
@@ -77,6 +81,7 @@
 from lp.services.database.constants import UTC_NOW
 from lp.services.features.testing import FeatureFixture
 from lp.services.webapp import canonical_url
+from lp.services.xref.interfaces import IXRefSet
 from lp.testing import (
     ExpectedException,
     launchpadlib_for,
@@ -90,6 +95,8 @@
     )
 from lp.testing.dbuser import dbuser
 from lp.testing.factory import LaunchpadObjectFactory
+from lp.testing.fakemethod import FakeMethod
+from lp.testing.fixture import ZopeUtilityFixture
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
@@ -1463,6 +1470,142 @@
     def _makeBranchMergeProposal(self):
         return self.factory.makeBranchMergeProposalForGit()
 
+    def test__fetchRelatedBugIDsFromSource(self):
+        # _fetchRelatedBugIDsFromSource makes a reasonable backend call and
+        # parses commit messages.
+        bugs = [self.factory.makeBug() for _ in range(3)]
+        bmp = self._makeBranchMergeProposal()
+        hosting_client = FakeMethod()
+        hosting_client.getLog = FakeMethod(result=[
+            {
+                u"sha1": bmp.source_git_commit_sha1,
+                u"message": u"Commit 1\n\nLP: #%d" % bugs[0].id,
+                },
+            {
+                u"sha1": unicode(hashlib.sha1("1").hexdigest()),
+                # Will not be matched.
+                u"message": u"Commit 2; see LP #%d" % bugs[1].id,
+                },
+            {
+                u"sha1": unicode(hashlib.sha1("2").hexdigest()),
+                u"message": u"Commit 3; LP: #%d" % bugs[2].id,
+                },
+            {
+                u"sha1": unicode(hashlib.sha1("3").hexdigest()),
+                # Non-existent bug ID will not be returned.
+                u"message": u"Non-existent bug; LP: #%d" % (bugs[2].id + 100),
+                },
+            ])
+        self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
+        related_bugs = bmp._fetchRelatedBugIDsFromSource()
+        path = "%s:%s" % (
+            bmp.target_git_repository.getInternalPath(),
+            bmp.source_git_repository.getInternalPath())
+        self.assertEqual(
+            [((path, bmp.source_git_commit_sha1),
+              {"limit": 10, "stop": bmp.target_git_commit_sha1,
+               "logger": None})],
+            hosting_client.getLog.calls)
+        self.assertContentEqual([bugs[0].id, bugs[2].id], related_bugs)
+
+    def _setUpLog(self, bugs):
+        """Set up a fake log response referring to the given bugs."""
+        if getattr(self, "hosting_client", None) is None:
+            self.hosting_client = FakeMethod()
+            self.hosting_client.getLog = FakeMethod()
+            self.useFixture(
+                ZopeUtilityFixture(self.hosting_client, IGitHostingClient))
+        self.hosting_client.getLog = FakeMethod(result=[
+            {
+                u"sha1": unicode(hashlib.sha1(str(i)).hexdigest()),
+                u"message": u"LP: #%d" % bug.id,
+                }
+            for i, bug in enumerate(bugs)])
+
+    def test_updateRelatedBugsFromSource_no_links(self):
+        # updateRelatedBugsFromSource does nothing if there are no related
+        # bugs in either the database or the source branch.
+        bmp = self._makeBranchMergeProposal()
+        self._setUpLog([])
+        bmp.updateRelatedBugsFromSource()
+        self.assertEqual([], bmp.bugs)
+
+    def test_updateRelatedBugsFromSource_new_links(self):
+        # If the source branch has related bugs not yet reflected in the
+        # database, updateRelatedBugsFromSource creates the links.
+        bugs = [self.factory.makeBug() for _ in range(3)]
+        bmp = self._makeBranchMergeProposal()
+        self._setUpLog([bugs[0]])
+        bmp.updateRelatedBugsFromSource()
+        self.assertEqual([bugs[0]], bmp.bugs)
+        self._setUpLog(bugs)
+        bmp.updateRelatedBugsFromSource()
+        self.assertContentEqual(bugs, bmp.bugs)
+
+    def test_updateRelatedBugsFromSource_same_links(self):
+        # If the database and the source branch list the same related bugs,
+        # updateRelatedBugsFromSource does nothing.
+        bug = self.factory.makeBug()
+        bmp = self._makeBranchMergeProposal()
+        self._setUpLog([bug])
+        bmp.updateRelatedBugsFromSource()
+        self.assertEqual([bug], bmp.bugs)
+        # The second run is a no-op.
+        bmp.updateRelatedBugsFromSource()
+        self.assertEqual([bug], bmp.bugs)
+
+    def test_updateRelatedBugsFromSource_removes_old_links(self):
+        # If the database records a source-branch-originating related bug
+        # that is no longer listed by the source branch,
+        # updateRelatedBugsFromSource removes the link.
+        bug = self.factory.makeBug()
+        bmp = self._makeBranchMergeProposal()
+        self._setUpLog([bug])
+        bmp.updateRelatedBugsFromSource()
+        self.assertEqual([bug], bmp.bugs)
+        self._setUpLog([])
+        bmp.updateRelatedBugsFromSource()
+        self.assertEqual([], bmp.bugs)
+
+    def test_updateRelatedBugsFromSource_leaves_manual_links(self):
+        # If a bug was linked to the merge proposal manually,
+        # updateRelatedBugsFromSource leaves the link alone regardless of
+        # whether it is listed by the source branch.
+        bug = self.factory.makeBug()
+        bmp = self._makeBranchMergeProposal()
+        bmp.linkBug(bug)
+        self._setUpLog([])
+        bmp.updateRelatedBugsFromSource()
+        self.assertEqual([bug], bmp.bugs)
+        matches_expected_xref = MatchesDict(
+            {(u"bug", unicode(bug.id)): ContainsDict({"metadata": Is(None)})})
+        self.assertThat(
+            getUtility(IXRefSet).findFrom(
+                (u"merge_proposal", unicode(bmp.id)), types=[u"bug"]),
+            matches_expected_xref)
+        self._setUpLog([bug])
+        self.assertThat(
+            getUtility(IXRefSet).findFrom(
+                (u"merge_proposal", unicode(bmp.id)), types=[u"bug"]),
+            matches_expected_xref)
+
+    def test_updateRelatedBugsFromSource_honours_limit(self):
+        # If the number of bugs to be linked exceeds the configured limit,
+        # updateRelatedBugsFromSource only links that many bugs and logs an
+        # OOPS.
+        self.pushConfig("codehosting", related_bugs_from_source_limit=3)
+        bugs = [self.factory.makeBug() for _ in range(5)]
+        bmp = self._makeBranchMergeProposal()
+        self._setUpLog([bugs[0]])
+        bmp.updateRelatedBugsFromSource()
+        self.assertEqual([bugs[0]], bmp.bugs)
+        self.assertEqual([], self.oopses)
+        self._setUpLog(bugs)
+        bmp.updateRelatedBugsFromSource()
+        self.assertContentEqual(bugs[:3], bmp.bugs)
+        self.assertEqual(1, len(self.oopses))
+        self.assertEqual("TooManyRelatedBugs", self.oopses[0]["type"])
+
 
 class TestNotifyModified(TestCaseWithFactory):
 

=== modified file 'lib/lp/code/model/tests/test_gitjob.py'
--- lib/lp/code/model/tests/test_gitjob.py	2016-05-13 10:58:59 +0000
+++ lib/lp/code/model/tests/test_gitjob.py	2016-09-02 14:40:37 +0000
@@ -245,6 +245,7 @@
                 }},
             }
         hosting_client = FakeGitHostingClient(new_refs, [])
+        hosting_client.getLog = FakeMethod(result=[])
         hosting_client.detectMerges = FakeMethod(
             result={source.commit_sha1: u'0' * 40})
         self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))

=== modified file 'lib/lp/code/model/tests/test_gitref.py'
--- lib/lp/code/model/tests/test_gitref.py	2016-05-28 00:21:40 +0000
+++ lib/lp/code/model/tests/test_gitref.py	2016-09-02 14:40:37 +0000
@@ -26,6 +26,7 @@
 from lp.app.enums import InformationType
 from lp.app.interfaces.informationtype import IInformationType
 from lp.app.interfaces.launchpad import IPrivacy
+from lp.code.errors import InvalidBranchMergeProposal
 from lp.code.interfaces.githosting import IGitHostingClient
 from lp.services.features.testing import FeatureFixture
 from lp.services.memcache.interfaces import IMemcacheClient
@@ -270,6 +271,180 @@
             ]))
 
 
+class TestGitRefCreateMergeProposal(TestCaseWithFactory):
+    """Exercise all the code paths for creating a merge proposal."""
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        super(TestGitRefCreateMergeProposal, self).setUp()
+        with admin_logged_in():
+            self.project = self.factory.makeProduct()
+            self.user = self.factory.makePerson()
+            self.reviewer = self.factory.makePerson(name=u"reviewer")
+            [self.source] = self.factory.makeGitRefs(
+                name=u"source", owner=self.user, target=self.project)
+            [self.target] = self.factory.makeGitRefs(
+                name=u"target", owner=self.user, target=self.project)
+            [self.prerequisite] = self.factory.makeGitRefs(
+                name=u"prerequisite", owner=self.user, target=self.project)
+        self.log = []
+        self.hosting_client = FakeMethod()
+        self.hosting_client.getLog = FakeMethod(result=self.log)
+        self.useFixture(
+            ZopeUtilityFixture(self.hosting_client, IGitHostingClient))
+
+    def assertOnePendingReview(self, proposal, reviewer, review_type=None):
+        # There should be one pending vote for the reviewer with the specified
+        # review type.
+        [vote] = proposal.votes
+        self.assertEqual(reviewer, vote.reviewer)
+        self.assertEqual(self.user, vote.registrant)
+        self.assertIsNone(vote.comment)
+        if review_type is None:
+            self.assertIsNone(vote.review_type)
+        else:
+            self.assertEqual(review_type, vote.review_type)
+
+    def test_personal_source(self):
+        """Personal repositories cannot be used as a source for MPs."""
+        self.source.repository.setTarget(
+            target=self.source.owner, user=self.source.owner)
+        self.assertRaises(
+            InvalidBranchMergeProposal, self.source.addLandingTarget,
+            self.user, self.target)
+
+    def test_target_repository_same_target(self):
+        """The target repository's target must match that of the source."""
+        self.target.repository.setTarget(
+            target=self.target.owner, user=self.target.owner)
+        self.assertRaises(
+            InvalidBranchMergeProposal, self.source.addLandingTarget,
+            self.user, self.target)
+
+        project = self.factory.makeProduct()
+        self.target.repository.setTarget(
+            target=project, user=self.target.owner)
+        self.assertRaises(
+            InvalidBranchMergeProposal, self.source.addLandingTarget,
+            self.user, self.target)
+
+    def test_target_must_not_be_the_source(self):
+        """The target and source references cannot be the same."""
+        self.assertRaises(
+            InvalidBranchMergeProposal, self.source.addLandingTarget,
+            self.user, self.source)
+
+    def test_prerequisite_repository_same_target(self):
+        """The prerequisite repository, if any, must be for the same target."""
+        self.prerequisite.repository.setTarget(
+            target=self.prerequisite.owner, user=self.prerequisite.owner)
+        self.assertRaises(
+            InvalidBranchMergeProposal, self.source.addLandingTarget,
+            self.user, self.target, self.prerequisite)
+
+        project = self.factory.makeProduct()
+        self.prerequisite.repository.setTarget(
+            target=project, user=self.prerequisite.owner)
+        self.assertRaises(
+            InvalidBranchMergeProposal, self.source.addLandingTarget,
+            self.user, self.target, self.prerequisite)
+
+    def test_prerequisite_must_not_be_the_source(self):
+        """The target and source references cannot be the same."""
+        self.assertRaises(
+            InvalidBranchMergeProposal, self.source.addLandingTarget,
+            self.user, self.target, self.source)
+
+    def test_prerequisite_must_not_be_the_target(self):
+        """The target and source references cannot be the same."""
+        self.assertRaises(
+            InvalidBranchMergeProposal, self.source.addLandingTarget,
+            self.user, self.target, self.target)
+
+    def test_existingMergeProposal(self):
+        """If there is an existing merge proposal for the source and target
+        reference pair, then another landing target specifying the same pair
+        raises.
+        """
+        self.source.addLandingTarget(self.user, self.target, self.prerequisite)
+        self.assertRaises(
+            InvalidBranchMergeProposal, self.source.addLandingTarget,
+            self.user, self.target, self.prerequisite)
+
+    def test_existingRejectedMergeProposal(self):
+        """If there is an existing rejected merge proposal for the source
+        and target reference pair, then another landing target specifying
+        the same pair is fine.
+        """
+        proposal = self.source.addLandingTarget(
+            self.user, self.target, self.prerequisite)
+        proposal.rejectBranch(self.user, "some_revision")
+        self.source.addLandingTarget(self.user, self.target, self.prerequisite)
+
+    def test_default_reviewer(self):
+        """If the target repository has a default reviewer set, this
+        reviewer should be assigned to the merge proposal.
+        """
+        [target_with_default_reviewer] = self.factory.makeGitRefs(
+            name=u"target-branch-with-reviewer", owner=self.user,
+            target=self.project, reviewer=self.reviewer)
+        proposal = self.source.addLandingTarget(
+            self.user, target_with_default_reviewer)
+        self.assertOnePendingReview(proposal, self.reviewer)
+
+    def test_default_reviewer_when_owner(self):
+        """If the target repository has a no default reviewer set, the
+        repository owner should be assigned as the reviewer for the merge
+        proposal.
+        """
+        proposal = self.source.addLandingTarget(self.user, self.target)
+        self.assertOnePendingReview(proposal, self.source.owner)
+
+    def test_attribute_assignment(self):
+        """Smoke test to make sure the assignments are there."""
+        commit_message = u"Some commit message"
+        proposal = self.source.addLandingTarget(
+            self.user, self.target, self.prerequisite,
+            commit_message=commit_message)
+        self.assertEqual(self.user, proposal.registrant)
+        self.assertEqual(self.source, proposal.merge_source)
+        self.assertEqual(self.target, proposal.merge_target)
+        self.assertEqual(self.prerequisite, proposal.merge_prerequisite)
+        self.assertEqual(commit_message, proposal.commit_message)
+
+    def test_createMergeProposal_with_reviewers(self):
+        person1 = self.factory.makePerson()
+        person2 = self.factory.makePerson()
+        e = self.assertRaises(
+            ValueError, self.source.createMergeProposal,
+            self.user, self.target, reviewers=[person1, person2])
+        self.assertEqual(
+            "reviewers and review_types must be equal length.", str(e))
+        e = self.assertRaises(
+            ValueError, self.source.createMergeProposal,
+            self.user, self.target, reviewers=[person1, person2],
+            review_types=["review1"])
+        self.assertEqual(
+            "reviewers and review_types must be equal length.", str(e))
+        bmp = self.source.createMergeProposal(
+            self.user, self.target, reviewers=[person1, person2],
+            review_types=["review1", "review2"])
+        votes = {(vote.reviewer, vote.review_type) for vote in bmp.votes}
+        self.assertEqual({(person1, "review1"), (person2, "review2")}, votes)
+
+    def test_updates_related_bugs(self):
+        """The merge proposal has its related bugs updated."""
+        bug = self.factory.makeBug()
+        self.log.append({
+            u"sha1": unicode(hashlib.sha1("tip").hexdigest()),
+            u"message": u"Fix upside-down messages\n\nLP: #%d" % bug.id,
+            })
+        proposal = self.source.addLandingTarget(self.user, self.target)
+        self.assertEqual([bug], proposal.bugs)
+        self.assertEqual([proposal], bug.linked_merge_proposals)
+
+
 class TestGitRefWebservice(TestCaseWithFactory):
     """Tests for the webservice."""
 

=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py	2016-07-01 20:33:06 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py	2016-09-02 14:40:37 +0000
@@ -986,7 +986,7 @@
 class TestGitRepositoryRefs(TestCaseWithFactory):
     """Tests for ref handling."""
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def test__convertRefInfo(self):
         # _convertRefInfo converts a valid info dictionary.
@@ -1107,6 +1107,9 @@
         return [UpdatePreviewDiffJob(job) for job in jobs]
 
     def test_update_schedules_diff_update(self):
+        hosting_client = FakeMethod()
+        hosting_client.getLog = FakeMethod(result=[])
+        self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
         repository = self.factory.makeGitRepository()
         [ref] = self.factory.makeGitRefs(repository=repository)
         self.assertRefsMatch(repository.refs, repository, [ref.path])
@@ -1816,23 +1819,44 @@
         self.assertNotEqual(ref2_2, bmp2.source_git_ref)
 
 
-class TestGitRepositoryScheduleDiffUpdates(TestCaseWithFactory):
-
-    layer = DatabaseFunctionalLayer
-
-    def test_scheduleDiffUpdates(self):
+class TestGitRepositoryUpdateLandingTargets(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_updates_related_bugs(self):
+        """All non-final merge proposals have their related bugs updated."""
+        bug = self.factory.makeBug()
+        bmp = self.factory.makeBranchMergeProposalForGit()
+        self.assertEqual([], bmp.bugs)
+        self.assertEqual([], bug.linked_merge_proposals)
+        hosting_client = FakeMethod()
+        hosting_client.getLog = FakeMethod(result=[
+            {
+                u"sha1": bmp.source_git_commit_sha1,
+                u"message": u"Fix upside-down messages\n\nLP: #%d" % bug.id,
+                },
+            ])
+        self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
+        bmp.source_git_repository.updateLandingTargets([bmp.source_git_path])
+        self.assertEqual([bug], bmp.bugs)
+        self.assertEqual([bmp], bug.linked_merge_proposals)
+
+    def test_schedules_diff_updates(self):
         """Create jobs for all merge proposals."""
+        hosting_client = FakeMethod()
+        hosting_client.getLog = FakeMethod(result=[])
+        self.useFixture(ZopeUtilityFixture(hosting_client, IGitHostingClient))
         bmp1 = self.factory.makeBranchMergeProposalForGit()
         bmp2 = self.factory.makeBranchMergeProposalForGit(
             source_ref=bmp1.source_git_ref)
-        jobs = bmp1.source_git_repository.scheduleDiffUpdates(
+        jobs = bmp1.source_git_repository.updateLandingTargets(
             [bmp1.source_git_path])
         self.assertEqual(2, len(jobs))
         bmps_to_update = [
             removeSecurityProxy(job).branch_merge_proposal for job in jobs]
         self.assertContentEqual([bmp1, bmp2], bmps_to_update)
 
-    def test_scheduleDiffUpdates_ignores_final(self):
+    def test_ignores_final(self):
         """Diffs for proposals in final states aren't updated."""
         [source_ref] = self.factory.makeGitRefs()
         for state in FINAL_STATES:
@@ -1843,7 +1867,7 @@
         for bmp in source_ref.landing_targets:
             if bmp.queue_status not in FINAL_STATES:
                 removeSecurityProxy(bmp).deleteProposal()
-        jobs = source_ref.repository.scheduleDiffUpdates([source_ref.path])
+        jobs = source_ref.repository.updateLandingTargets([source_ref.path])
         self.assertEqual(0, len(jobs))
 
 

=== modified file 'lib/lp/code/subscribers/branchmergeproposal.py'
--- lib/lp/code/subscribers/branchmergeproposal.py	2015-10-19 10:21:11 +0000
+++ lib/lp/code/subscribers/branchmergeproposal.py	2016-09-02 14:40:37 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Event subscribers for branch merge proposals."""
@@ -63,9 +63,10 @@
 def merge_proposal_created(merge_proposal, event):
     """A new merge proposal has been created.
 
-    Create a job to update the diff for the merge proposal.
-    Also create a job to email the subscribers about the new proposal.
+    Update related bug links based on commits in the source branch; create a
+    job to update the diff for the merge proposal; trigger webhooks.
     """
+    merge_proposal.updateRelatedBugsFromSource()
     getUtility(IUpdatePreviewDiffJobSource).create(merge_proposal)
     if getFeatureFlag(BRANCH_MERGE_PROPOSAL_WEBHOOKS_FEATURE_FLAG):
         payload = {

=== modified file 'lib/lp/code/subscribers/git.py'
--- lib/lp/code/subscribers/git.py	2016-06-20 20:32:36 +0000
+++ lib/lp/code/subscribers/git.py	2016-09-02 14:40:37 +0000
@@ -9,7 +9,7 @@
 def refs_updated(repository, event):
     """Some references in a Git repository have been updated."""
     repository.updateMergeCommitIDs(event.paths)
-    repository.scheduleDiffUpdates(event.paths)
+    repository.updateLandingTargets(event.paths)
     repository.markRecipesStale(event.paths)
     repository.markSnapsStale(event.paths)
     repository.detectMerges(event.paths, logger=event.logger)

=== modified file 'lib/lp/services/config/schema-lazr.conf'
--- lib/lp/services/config/schema-lazr.conf	2016-07-18 17:28:42 +0000
+++ lib/lp/services/config/schema-lazr.conf	2016-09-02 14:40:37 +0000
@@ -368,6 +368,10 @@
 # datatype: urlbase
 git_ssh_root: none
 
+# The upper limit on the number of bugs to link to a merge proposal based on
+# Git commit metadata.
+related_bugs_from_source_limit: 1000
+
 
 [codeimport]
 # Where the Bazaar imports are stored.