Merge lp:~dpb/tarmac/better-prereq-branches into lp:tarmac

[David Britton]

Better prerequisite handling.

Tarmac understands the launchpad concept of prerequisite branches. The
following conditions are supported when evaluating a candidate merge
proposal (MP).

 * Multiple prerequisite non-superseded MPs => error
 * Prerequiste branch but no associated MP => error
 * Exactly one prerequisite non-superseded unmerged MP => skip
 * Exactly one prerequisite non-superseded *Merged* MP => proceed

Additionally, the tarmac queue is processed taking into account prerequisite
branches. That is, dependent branches are processed *after* their prerequisites.

[dobey (dobey)]

32 + * Multiple prerequisite non-superseded MPs => error

How does one declare multiple prerequisite branches in Launchpad?

161 +++ tarmac/tests/test_commands.py 2013-12-05 22:45:14 +0000

Many of the changes in here seem unrelated to the prerequisite work, and seems to be just you changing some of the names for the third branch in several of the tests. How are these related?

The bug you filed is a duplicate of https://bugs.launchpad.net/tarmac/+bug/900731 and I've marked it as such already. Changing the --fixes here too would be great, if possible.

Also, you need to commit using an e-mail address that is associated with your Launchpad account. Your shortened canonical.com address does not appear to be associated to your account, and so Launchpad cannot link to your account, nor will tarmac be able to find you via the Launchpad API from the e-mail address in the commit.

review: Needs Fixing

[lp:~dpb/tarmac/better-prereq-branches]

426. By David Britton on 2014-01-09

remove renaming of 'branch3'

[David Britton (dpb)]

> 32 + * Multiple prerequisite non-superseded MPs => error
>
> How does one declare multiple prerequisite branches in Launchpad?
>

I don't know, but I didn't change or add this code path. You can see it at line 231 of tarmac/bin/commands.py. I just documented what would happen if this condition was hit.

What would you like me to do?

> 161 +++ tarmac/tests/test_commands.py 2013-12-05 22:45:14 +0000
>
> Many of the changes in here seem unrelated to the prerequisite work, and seems
> to be just you changing some of the names for the third branch in several of
> the tests. How are these related?
>

I reverted since you asked. They were to help me track down which test was failing before adding the 'unique_name' property. The error message coming out of bzrlib was not helpful and just contained 'branch3' which made it hard to see which test failed.

> The bug you filed is a duplicate of
> https://bugs.launchpad.net/tarmac/+bug/900731 and I've marked it as such
> already. Changing the --fixes here too would be great, if possible.

OK, I'll hopefully add '--fixes' to this commit

> Also, you need to commit using an e-mail address that is associated with your
> Launchpad account. Your shortened canonical.com address does not appear to be
> associated to your account, and so Launchpad cannot link to your account, nor
> will tarmac be able to find you via the Launchpad API from the e-mail address
> in the commit.

I added it.

[lp:~dpb/tarmac/better-prereq-branches]

427. By David Britton on 2014-01-09

Adding lp:900731 to the list of fixes

[dobey (dobey)]

> > 32 + * Multiple prerequisite non-superseded MPs => error
> >
> > How does one declare multiple prerequisite branches in Launchpad?
> >
>
> I don't know, but I didn't change or add this code path. You can see it at
> line 231 of tarmac/bin/commands.py. I just documented what would happen if
> this condition was hit.
>
> What would you like me to do?

I think you've misunderstood the code. That code is to deal with the case when a prerequisite branch has multiple proposals (ie, if someone re-proposed the branch and there is a superseded proposal, and such). These are not multiple prerequisites. The comments and documentation need to be clear about what the code is actually doing.

[David Britton (dpb)]

I'll copy it here again:

"Multiple prerequisite non-superseded MPs"

What is wrong with that statement? I didn't say multiple prerequisite
branches. I said multiple prerequisite MPs (merge proposals). What would
you like me to say to clarify it?

Also, there is no comment about this. It's just documentation in a Readme,
unless I'm missing something.

On Sun, Feb 9, 2014 at 12:27 PM, Rodney Dawes <email address hidden>wrote:

> > > 32 + * Multiple prerequisite non-superseded MPs => error
> > >
> > > How does one declare multiple prerequisite branches in Launchpad?
> > >
> >
> > I don't know, but I didn't change or add this code path. You can see it
> at
> > line 231 of tarmac/bin/commands.py. I just documented what would happen
> if
> > this condition was hit.
> >
> > What would you like me to do?
>
> I think you've misunderstood the code. That code is to deal with the case
> when a prerequisite branch has multiple proposals (ie, if someone
> re-proposed the branch and there is a superseded proposal, and such). These
> are not multiple prerequisites. The comments and documentation need to be
> clear about what the code is actually doing.
> --
>
> https://code.launchpad.net/~davidpbritton/tarmac/better-prereq-branches/+merge/197969
> You are the owner of lp:~davidpbritton/tarmac/better-prereq-branches.
>

--
David Britton <email address hidden>

[lp:~dpb/tarmac/better-prereq-branches]

428. By David Britton on 2014-02-09

Clean up wording around prerequisite branches

[David Britton (dpb)]

I committed a rewording change, I hope it's more clear. I think calling an MP "prerequisite" was what was leading to the confusion. (hopefully)

[dobey (dobey)]

review: Approve

=== modified file '.bzrignore'
--- .bzrignore	2010-07-24 13:04:41 +0000
+++ .bzrignore	2014-02-09 23:07:13 +0000
@@ -4,3 +4,4 @@
 dist
 TODO
 debug*
+tarmac.egg-info
 
=== modified file 'docs/introduction.txt'
--- docs/introduction.txt	2013-11-06 19:35:08 +0000
+++ docs/introduction.txt	2014-02-09 23:07:13 +0000
@@ -6,7 +6,6 @@
 merge, it should really just magically be merged.  That's what Tarmac does, and
 it does it well.
 
-
 How it works
 ============
 
@@ -20,6 +19,21 @@
 branch has landed.  This opens up all sorts of branch landing options that can
 happen behind the scenes, while you can safely do more exciting work.
 
+Prerequisite Branches
+=====================
+
+Tarmac understands the launchpad concept of prerequisite branches.  The
+following conditions are supported when evaluating a candidate merge
+proposal (MP).
+
+ * Prerequisite branch without an associated MP => error
+ * Prerequisite branch with multiple non-superseded MPs => error
+ * Prerequisite branch with exactly one non-superseded unmerged MP => skip
+ * Prerequisite branch with exactly one non-superseded *Merged* MP => proceed
+
+Additionally, the tarmac queue is processed taking into account prerequisite
+branches.  That is, dependent branches are processed *after* their
+prerequisites.
 
 Configuration
 =============
 
=== modified file 'tarmac/bin/commands.py'
--- tarmac/bin/commands.py	2013-11-07 17:28:53 +0000
+++ tarmac/bin/commands.py	2014-02-09 23:07:13 +0000
@@ -25,6 +25,17 @@
 from tarmac.utility import get_review_url
 
 
+def _compare_proposals(a, b):
+    """Helper to sort proposals based on a prerequisite branch"""
+    if a.prerequisite_branch is not None:
+        if a.prerequisite_branch.unique_name == b.source_branch.unique_name:
+            return 1
+    if b.prerequisite_branch is not None:
+        if b.prerequisite_branch.unique_name == a.source_branch.unique_name:
+            return -1
+    return 0
+
+
 class TarmacCommand(Command):
     '''A command class.'''
 
@@ -210,9 +221,7 @@
                 try:
                     prerequisite = proposal.prerequisite_branch
                     if prerequisite:
-                        merges = [x for x in  prerequisite.landing_targets
-                                  if x.target_branch == target.lp_branch and
-                                  x.queue_status != u'Superseded']
+                        merges = self._get_prerequisite_proposals(proposal)
                         if len(merges) == 0:
                             raise TarmacMergeError(
                                 u'No proposals of prerequisite branch.',
@@ -227,14 +236,6 @@
                                 u'of %s into %s, which is not Superseded.' % (
                                     prerequisite.web_link,
                                     target.lp_branch.web_link))
-                        elif len(merges) == 1:
-                            if merges[0].queue_status != u'Merged':
-                                raise TarmacMergeError(
-                                    u'Prerequisite not yet merged.',
-                                    u'The prerequisite %s has not yet been '
-                                    u'merged into %s.' % (
-                                        prerequisite.web_link,
-                                        target.lp_branch.web_link))
 
                     if not proposal.reviewed_revid:
                         raise TarmacMergeError(
@@ -325,10 +326,16 @@
             target.cleanup()
 
     def _get_mergable_proposals_for_branch(self, lp_branch):
-        """Return a list of the mergable proposals for the given branch."""
+        """
+        Return a list of the mergable proposals for the given branch.  The
+        list returned will be in the order that they should be processed.
+        """
         proposals = []
-        for entry in lp_branch.landing_candidates:
+        sorted_proposals = sorted(
+            lp_branch.landing_candidates, cmp=_compare_proposals)
+        for entry in sorted_proposals:
             self.logger.debug("Considering merge proposal: {0}".format(entry.web_link))
+            prereqs = self._get_prerequisite_proposals(entry)
 
             if entry.queue_status != u'Approved':
                 self.logger.debug(
@@ -342,9 +349,31 @@
                     "  Skipping proposal: proposal has no commit message")
                 continue
 
+            if len(prereqs) == 1 and prereqs[0].queue_status != u'Merged':
+                # N.B.: The case of a MP with more than one prereq MP open
+                #       will be caught as a merge error.
+                self.logger.debug(
+                    "  Skipping proposal: prerequisite not yet merged")
+                continue
+
             proposals.append(entry)
         return proposals
 
+    def _get_prerequisite_proposals(self, proposal):
+        """
+        Given a proposal, return all prerequisite
+        proposals that are not in the superseded state.  There ideally
+        should be one and only one here (or zero), but sometimes there
+        are not, depending on developer habits
+        """
+        prerequisite = proposal.prerequisite_branch
+        target_branch = proposal.target_branch
+        if not prerequisite or not prerequisite.landing_targets:
+            return []
+        return [x for x in prerequisite.landing_targets
+            if x.target_branch.unique_name == target_branch.unique_name and
+            x.queue_status != u'Superseded']
+
     def _get_reviews(self, proposal):
         """Get the set of reviews from the proposal."""
         reviews = []
@@ -405,3 +434,4 @@
                         'An error occurred trying to merge %s: %s',
                         branch, error)
                     raise
+
 
=== modified file 'tarmac/tests/__init__.py'
--- tarmac/tests/__init__.py	2013-10-31 19:09:08 +0000
+++ tarmac/tests/__init__.py	2014-02-09 23:07:13 +0000
@@ -56,6 +56,7 @@
             self.revision_count = 0
         self.bzr_identity = 'lp:%s' % os.path.basename(self.tree_dir)
         self.web_link = self.bzr_identity
+        self.unique_name = self.bzr_identity
         self.project = MockLPProject()
 
 
 
=== modified file 'tarmac/tests/test_commands.py'
--- tarmac/tests/test_commands.py	2013-11-07 17:28:53 +0000
+++ tarmac/tests/test_commands.py	2014-02-09 23:07:13 +0000
@@ -128,13 +128,16 @@
                 display_name=self.branch2.lp_branch.bzr_identity,
                 web_link=self.branch2.lp_branch.bzr_identity,
                 name='source',
+                unique_name='source',
                 revision_count=self.branch2.lp_branch.revision_count,
-                landing_candidates=[]),
+                landing_candidates=[],
+                landing_targets=[]),
                          Thing(
                 bzr_identity=self.branch1.lp_branch.bzr_identity,
                 display_name=self.branch1.lp_branch.bzr_identity,
                 web_link=self.branch1.lp_branch.bzr_identity,
                 name='target',
+                unique_name='target',
                 revision_count=self.branch1.lp_branch.revision_count,
                 landing_candidates=None)]
         self.proposals = [Thing(
@@ -171,6 +174,8 @@
                         comment=Thing(vote=u'Abstain'),
                         reviewer=Thing(display_name=u'Reviewer2'))])]
         self.branches[1].landing_candidates = self.proposals
+        self.branches[0].landing_targets = [
+                self.proposals[0], self.proposals[1]]
 
         self.launchpad = Thing(branches=Thing(getByUrl=self.getBranchByUrl),
                                me=Thing(display_name='Tarmac'))
@@ -180,6 +185,42 @@
 
         self.command = registry._get_command(commands.cmd_merge, 'merge')
 
+    def addProposal(self, name, prerequisite_branch=None):
+        """Create a 3rd branch with a proposal"""
+        # Create a 3rd branch we'll use to test with
+        branch3_dir = os.path.join(self.TEST_ROOT, name)
+        mock3 = MockLPBranch(branch3_dir, source_branch=self.branch1.lp_branch)
+        branch3 = Branch.create(mock3, self.config, create_tree=True,
+                                target=self.branch1)
+        branch3.commit('Prerequisite commit.')
+        branch3.lp_branch.revision_count += 1
+
+        # Set up an approved proposal for the branch (prereq on branches[0])
+        branch3.lp_branch.display_name = branch3.lp_branch.bzr_identity
+        branch3.lp_branch.name = name
+        branch3.lp_branch.unique_name = '~user/branch/' + name
+        branch3.lp_branch.landing_candidates = []
+        b3_proposal = Thing(
+            self_link=u'http://api.edge.launchpad.net/devel/proposal3',
+            web_link=u'http://edge.launchpad.net/proposal3',
+            queue_status=u'Approved',
+            commit_message=u'Commitable.',
+            source_branch=branch3.lp_branch,
+            target_branch=self.branches[1],
+            prerequisite_branch=prerequisite_branch,
+            createComment=self.createComment,
+            setStatus=self.lp_save,
+            lp_save=self.lp_save,
+            reviewed_revid=None,
+            votes=[Thing(
+                    comment=Thing(vote=u'Approve'),
+                    reviewer=Thing(display_name=u'Reviewer'))])
+
+        branch3.lp_branch.landing_targets = [b3_proposal]
+        self.proposals.append(b3_proposal)
+        self.branches.append(branch3.lp_branch)
+
+
     def lp_save(self, *args, **kwargs):
         """Do nothing here."""
         pass
@@ -248,8 +289,8 @@
         self.assertEqual(last_rev.properties.get('merge_url', None),
                          u'http://code.launchpad.net/proposal0')
 
-    def test_run_merge_with_unmerged_prerequisite_fails(self):
-        """Test that mereging a branch with an unmerged prerequisite fails."""
+    def test_run_merge_with_unmerged_prerequisite_skips(self):
+        """Test that mereging a branch with an unmerged prerequisite skips."""
         # Create a 3rd prerequisite branch we'll use to test with
         branch3_dir = os.path.join(self.TEST_ROOT, 'branch3')
         mock3 = MockLPBranch(branch3_dir, source_branch=self.branch1.lp_branch)
@@ -267,6 +308,7 @@
         # Set up an unapproved proposal for the prerequisite
         branch3.lp_branch.display_name = branch3.lp_branch.bzr_identity
         branch3.lp_branch.name = 'branch3'
+        branch3.unique_name = 'branch3'
         branch3.lp_branch.landing_candidates = []
         b3_proposal = Thing(
             self_link=u'http://api.edge.launchpad.net/devel/proposal3',
@@ -290,11 +332,8 @@
         self.proposals[1].prerequisite_branch = branch3.lp_branch
         self.proposals[1].reviewed_revid = \
             self.branch2.bzr_branch.last_revision()
-        self.command.run(launchpad=self.launchpad)
+        self.assertEqual(self.command.run(launchpad=self.launchpad), None)
         shutil.rmtree(branch3_dir)
-        self.assertEqual(self.error.comment,
-                         u'The prerequisite lp:branch3 has not yet been '
-                         u'merged into lp:branch1.')
 
     def test_run_merge_with_unproposed_prerequisite_fails(self):
         """Test that mereging a branch with an unmerged prerequisite fails."""
@@ -315,6 +354,7 @@
         # Set up an unapproved proposal for the prerequisite
         branch3.lp_branch.display_name = branch3.lp_branch.bzr_identity
         branch3.lp_branch.name = 'branch3'
+        branch3.lp_branch.unique_name = 'branch3'
         branch3.lp_branch.landing_candidates = []
         b3_proposal = Thing(
             self_link=u'http://api.edge.launchpad.net/devel/proposal3',
@@ -363,6 +403,7 @@
         # Set up an unapproved proposal for the prerequisite
         branch3.lp_branch.display_name = branch3.lp_branch.bzr_identity
         branch3.lp_branch.name = 'branch3'
+        branch3.lp_branch.unique_name = 'branch3'
         branch3.lp_branch.landing_candidates = []
         b3_proposal = Thing(
             self_link=u'http://api.edge.launchpad.net/devel/proposal3',
@@ -394,8 +435,8 @@
         shutil.rmtree(branch3_dir)
         self.assertEqual(self.error.comment,
                          u'More than one proposal found for merge of '
-                         u'lp:branch3 into lp:branch1, which is not '
-                         u'Superseded.')
+                         u'lp:branch3 into lp:branch1, '
+                         u'which is not Superseded.')
 
     @patch('bzrlib.workingtree.WorkingTree.open')
     def test_run_merge_with_invalid_workingtree(self, mocked):
@@ -407,3 +448,55 @@
         self.command.run(launchpad=self.launchpad)
         self.assertEqual(self.error.comment,
                          invalid_tree_comment)
+
+    def test__compare_proposals(self):
+        """
+        _compare_proposals is meant to be a sort routine comparison function
+        """
+        self.addProposal("compare_proposals", self.branches[0])
+        self.assertEqual(
+            commands._compare_proposals(self.proposals[0], self.proposals[1]),
+            0)
+        self.assertEqual(
+            commands._compare_proposals(self.proposals[0], self.proposals[2]),
+            -1)
+        self.assertEqual(
+            commands._compare_proposals(self.proposals[2], self.proposals[0]),
+            1)
+
+    def test__get_mergable_proposals_for_branch_are_sorted(self):
+        """
+        Mergable proposals should be in sorted order (prereqs should come
+        first in the list)
+        """
+        self.addProposal("sorted_test", self.branches[0])
+        proposals = self.command._get_mergable_proposals_for_branch(
+                self.branches[1])
+        self.assertEqual(len(proposals), 2)
+        self.assertTrue(proposals[1].source_branch.name == "sorted_test")
+
+    def test__get_mergable_proposals_for_branch_prereq_unmerged(self):
+        """
+        skip mergable proposals that have specified a prereq,
+        but that prereq is not merged yet.  This is edge-casey, since
+        we now process the queue in an order that makes sense for
+        prereqs
+        """
+        self.addProposal("unmerged")
+        self.proposals[0].prerequisite_branch = self.branches[2]
+        self.proposals[1].prerequisite_branch = self.branches[2]
+        proposals = self.command._get_mergable_proposals_for_branch(
+                self.branches[1])
+        self.assertEqual(len(proposals), 1)
+        self.assertEqual(proposals[0].source_branch.name, "unmerged")
+
+    def test__get_prerequisite_proposals_no_prerequisites(self):
+        """proposals[0] does not have a prerequisite branch listed"""
+        proposals = self.command._get_prerequisite_proposals(self.proposals[0])
+        self.assertEqual(len(proposals), 0)
+
+    def test__get_prerequisite_proposals_one_prerequisite(self):
+        """Branches[0] (source) has two open MPs against it"""
+        self.addProposal("one_prerequisite", self.branches[0])
+        proposals = self.command._get_prerequisite_proposals(self.proposals[2])
+        self.assertEqual(len(proposals), 2)