Merge lp:~james-w/tarmac/break-up-do-merges into lp:tarmac

[James Westby]

Break up the _do_merges method.

This branch breaks up the _do_merges method a little, as it was huge.

I tried to keep changes to a minimum while moving the code around, but it
used "continue" for flow control, so I had to change it a little to
use return values.

(from the u1 fork)

[Paul Hummer (rockstar)]

The prerequisite lp:~james-w/tarmac/utf8 has not yet been merged into lp:tarmac.

[Paul Hummer (rockstar)]

review: Approve

[Paul Hummer (rockstar)]

The prerequisite lp:~james-w/tarmac/utf8 has not yet been merged into lp:tarmac.

[Paul Hummer (rockstar)]

The prerequisite lp:~james-w/tarmac/utf8 has not yet been merged into lp:tarmac.

[dobey (dobey)]

Please create bugs for the issues, include tests, and split the changes into a branch or two which do not have unnecessary dependencies on other unrelated changes.

review: Disapprove

Unmerged revisions

425. By James Westby on 2013-01-31

Merged commit-message into break-up-do-merges.

424. By James Westby on 2013-01-28

Merged commit-message into break-up-do-merges.

423. By James Westby on 2013-01-24

Merge commit-message.

422. By James Westby on 2013-01-24

Merged commit-message into break-up-do-merges.

421. By James Westby on 2013-01-24

Merge commit-message.

420. By James Westby on 2013-01-23

Merge commit message.

419. By James Westby on 2012-12-18

Properly check the statuses of the merges when merging just one branch.

418. By James Westby on 2012-12-18

Remove the option that isn't defined yet.

417. By James Westby on 2012-12-18

Break up the very large do_merges method somewhat.

416. By James Westby on 2012-12-18

Add the commit message plugin that sets [r=<reviewer>] and bug info.

=== modified file 'tarmac/bin/commands.py'
--- tarmac/bin/commands.py	2013-02-01 17:17:21 +0000
+++ tarmac/bin/commands.py	2013-02-01 17:17:21 +0000
@@ -134,6 +134,194 @@
         help_commands(self.outf)
 
 
+def _get_mergable_proposals_for_branch(lp_branch, logger, imply_commit_message=False):
+    """Return a list of the mergable proposals for the given branch."""
+    proposals = []
+    for entry in lp_branch.landing_candidates:
+        logger.debug("Considering merge proposal: {0}".format(entry.web_link))
+
+        if entry.queue_status != u'Approved':
+            logger.debug(
+                "  Skipping proposal: status is {0}, not "
+                "'Approved'".format(entry.queue_status))
+            continue
+
+        if (not imply_commit_message and not entry.commit_message):
+            logger.debug(
+                "  Skipping proposal: proposal has no commit message")
+            continue
+
+        proposals.append(entry)
+    return proposals
+
+
+def _get_reviews(proposal):
+    """Get the set of reviews from the proposal."""
+    reviews = []
+    for vote in proposal.votes:
+        if not vote.comment:
+            continue
+        else:
+            reviews.append('%s;%s' % (vote.reviewer.display_name,
+                                      vote.comment.vote))
+
+    if len(reviews) == 0:
+        return None
+
+    return reviews
+
+
+def setup(logger, debug=False, http_debug=False):
+    if debug:
+        set_up_debug_logging()
+        logger.debug('Debug logging enabled')
+    if http_debug:
+        httplib2.debuglevel = 1
+        logger.debug('HTTP debugging enabled.')
+    logger.debug('Loading plugins')
+    load_plugins()
+    logger.debug('Plugins loaded')
+
+
+def merge_proposals(target, proposals, logger, config, command):
+    logger.debug('Firing tarmac_pre_merge hook')
+    tarmac_hooks.fire('tarmac_pre_merge',
+                      command, target)
+
+    statuses = []
+    try:
+        for proposal in proposals:
+            status = merge_proposal(target, proposal, logger,
+                    config, command)
+            statuses.append(status)
+    # This except is here because we need the else and can't have it
+    # without an except as well.
+    except:
+        raise
+    else:
+        logger.debug('Firing tarmac_post_merge hook')
+        tarmac_hooks.fire('tarmac_post_merge',
+                          command, target)
+    finally:
+        target.cleanup()
+    return statuses
+
+
+def merge_proposal(target, proposal, logger, config, command):
+    target.cleanup()
+    logger.debug(
+        u'Preparing to merge %(source_branch)s' % {
+            'source_branch': proposal.source_branch.web_link})
+    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']
+            if len(merges) == 0:
+                raise TarmacMergeError(
+                    u'No proposals of prerequisite branch.',
+                    u'No proposals found for merge of %s '
+                    u'into %s.' % (
+                        prerequisite.web_link,
+                        target.lp_branch.web_link))
+            elif len(merges) > 1:
+                raise TarmacMergeError(
+                    u'Too many proposals of prerequisite.',
+                    u'More than one proposal found for merge '
+                    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(
+                u'No approved revision specified.')
+
+
+        source = Branch.create(
+            proposal.source_branch, config, target=target)
+
+        source_bzr_branch = source.get_bzr_branch()
+        approved = source_bzr_branch.revision_id_to_revno(
+            str(proposal.reviewed_revid))
+        tip = source_bzr_branch.revno()
+
+        if tip > approved:
+            message = u'Unapproved changes made after approval'
+            lp_comment = (
+                u'There are additional revisions which have not '
+                u'been approved in review. Please seek review and '
+                u'approval of these new revisions.')
+            raise UnapprovedChanges(message, lp_comment)
+
+        logger.debug(
+            'Merging %(source)s at revision %(revision)s' % {
+                'source': proposal.source_branch.web_link,
+                'revision': proposal.reviewed_revid})
+
+        target.merge(source, str(proposal.reviewed_revid))
+
+        logger.debug('Firing tarmac_pre_commit hook')
+        tarmac_hooks.fire('tarmac_pre_commit',
+                          command, target, source, proposal)
+
+    except TarmacMergeError, failure:
+        logger.warn(
+            u'Merging %(source)s into %(target)s failed: %(msg)s' %
+            {'source': proposal.source_branch.web_link,
+             'target': proposal.target_branch.web_link,
+             'msg': str(failure)})
+
+        subject = u'Re: [Merge] %(source)s into %(target)s' % {
+            "source": proposal.source_branch.display_name,
+            "target": proposal.target_branch.display_name}
+
+        if failure.comment:
+            comment = failure.comment
+        else:
+            comment = str(failure)
+
+        proposal.createComment(subject=subject, content=comment)
+        try:
+            proposal.setStatus(
+                status=config.rejected_branch_status)
+        except AttributeError:
+            proposal.setStatus(status=u'Needs review')
+        proposal.lp_save()
+        return False
+
+    except PointlessMerge:
+        logger.warn(
+            'Merging %(source)s into %(target)s would be '
+            'pointless.' % {
+                'source': proposal.source_branch.web_link,
+                'target': proposal.target_branch.web_link})
+        return False
+
+    merge_url = get_review_url(proposal)
+    revprops = {'merge_url': merge_url}
+
+    commit_message = proposal.commit_message
+    if commit_message is None and config.imply_commit_message:
+        commit_message = proposal.description
+
+    target.commit(commit_message,
+                  reviews=_get_reviews(proposal))
+
+    logger.debug('Firing tarmac_post_commit hook')
+    tarmac_hooks.fire('tarmac_post_commit',
+                      command, target, source, proposal)
+    return True
+
+
 class cmd_merge(TarmacCommand):
     '''Automatically merge approved merge proposal branches.'''
 
@@ -143,7 +331,8 @@
         options.http_debug_option,
         options.debug_option,
         options.imply_commit_message_option,
-        options.one_option]
+        options.one_option,
+        ]
 
     def _do_merges(self, branch_url):
 
@@ -152,205 +341,29 @@
             self.logger.info('Not a valid branch: {0}'.format(branch_url))
             return
 
-        proposals = self._get_mergable_proposals_for_branch(lp_branch)
+        proposals = _get_mergable_proposals_for_branch(lp_branch, self.logger,
+                imply_commit_message=self.config.imply_commit_message)
 
         if not proposals:
             self.logger.info(
                 'No approved proposals found for %(branch_url)s' % {
                     'branch_url': branch_url})
-            return
+            return []
+
+        if self.config.one:
+            proposals = proposals[0]
 
         target = Branch.create(lp_branch, self.config, create_tree=True)
-
-        self.logger.debug('Firing tarmac_pre_merge hook')
-        tarmac_hooks.fire('tarmac_pre_merge',
-                          self, target)
-
-        success_count = 0
-        try:
-            for proposal in proposals:
-                target.cleanup()
-                self.logger.debug(
-                    u'Preparing to merge %(source_branch)s' % {
-                        'source_branch': proposal.source_branch.web_link})
-                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']
-                        if len(merges) == 0:
-                            raise TarmacMergeError(
-                                u'No proposals of prerequisite branch.',
-                                u'No proposals found for merge of %s '
-                                u'into %s.' % (
-                                    prerequisite.web_link,
-                                    target.lp_branch.web_link))
-                        elif len(merges) > 1:
-                            raise TarmacMergeError(
-                                u'Too many proposals of prerequisite.',
-                                u'More than one proposal found for merge '
-                                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(
-                            u'No approved revision specified.')
-
-
-                    source = Branch.create(
-                        proposal.source_branch, self.config, target=target)
-
-                    source_bzr_branch = source.get_bzr_branch()
-                    approved = source_bzr_branch.revision_id_to_revno(
-                        str(proposal.reviewed_revid))
-                    tip = source_bzr_branch.revno()
-
-                    if tip > approved:
-                        message = u'Unapproved changes made after approval'
-                        lp_comment = (
-                            u'There are additional revisions which have not '
-                            u'been approved in review. Please seek review and '
-                            u'approval of these new revisions.')
-                        raise UnapprovedChanges(message, lp_comment)
-
-                    self.logger.debug(
-                        'Merging %(source)s at revision %(revision)s' % {
-                            'source': proposal.source_branch.web_link,
-                            'revision': proposal.reviewed_revid})
-
-                    target.merge(source, str(proposal.reviewed_revid))
-
-                    self.logger.debug('Firing tarmac_pre_commit hook')
-                    tarmac_hooks.fire('tarmac_pre_commit',
-                                      self, target, source, proposal)
-
-                except TarmacMergeError, failure:
-                    self.logger.warn(
-                        u'Merging %(source)s into %(target)s failed: %(msg)s' %
-                        {'source': proposal.source_branch.web_link,
-                         'target': proposal.target_branch.web_link,
-                         'msg': str(failure)})
-
-                    subject = u'Re: [Merge] %(source)s into %(target)s' % {
-                        "source": proposal.source_branch.display_name,
-                        "target": proposal.target_branch.display_name}
-
-                    if failure.comment:
-                        comment = failure.comment
-                    else:
-                        comment = str(failure)
-
-                    proposal.createComment(subject=subject, content=comment)
-                    try:
-                        proposal.setStatus(
-                            status=self.config.rejected_branch_status)
-                    except AttributeError:
-                        proposal.setStatus(status=u'Needs review')
-                    proposal.lp_save()
-
-                    # If we've been asked to only merge one branch, then exit.
-                    if self.config.one:
-                        return True
-
-                    continue
-                except PointlessMerge:
-                    self.logger.warn(
-                        'Merging %(source)s into %(target)s would be '
-                        'pointless.' % {
-                            'source': proposal.source_branch.web_link,
-                            'target': proposal.target_branch.web_link})
-                    continue
-
-                merge_url = get_review_url(proposal)
-                revprops = {'merge_url': merge_url}
-
-                commit_message = proposal.commit_message
-                if commit_message is None and self.config.imply_commit_message:
-                    commit_message = proposal.description
-
-                target.commit(commit_message,
-                              revprops=revprops,
-                              authors=source.authors,
-                              reviews=self._get_reviews(proposal))
-
-                self.logger.debug('Firing tarmac_post_commit hook')
-                tarmac_hooks.fire('tarmac_post_commit',
-                                  self, target, source, proposal)
-                success_count += 1
-                target.cleanup()
-                if self.config.one:
-                    return True
-
-        # This except is here because we need the else and can't have it
-        # without an except as well.
-        except:
-            raise
-        else:
-            self.logger.debug('Firing tarmac_post_merge hook')
-            tarmac_hooks.fire('tarmac_post_merge',
-                              self, target, success_count=success_count)
-        finally:
-            target.cleanup()
-
-    def _get_mergable_proposals_for_branch(self, lp_branch):
-        """Return a list of the mergable proposals for the given branch."""
-        proposals = []
-        for entry in lp_branch.landing_candidates:
-            self.logger.debug("Considering merge proposal: {0}".format(entry.web_link))
-
-            if entry.queue_status != u'Approved':
-                self.logger.debug(
-                    "  Skipping proposal: status is {0}, not "
-                    "'Approved'".format(entry.queue_status))
-                continue
-
-            if (not self.config.imply_commit_message and
-                not entry.commit_message):
-                self.logger.debug(
-                    "  Skipping proposal: proposal has no commit message")
-                continue
-
-            proposals.append(entry)
-        return proposals
-
-    def _get_reviews(self, proposal):
-        """Get the set of reviews from the proposal."""
-        reviews = []
-        for vote in proposal.votes:
-            if not vote.comment:
-                continue
-            else:
-                reviews.append('%s;%s' % (vote.reviewer.display_name,
-                                          vote.comment.vote))
-
-        if len(reviews) == 0:
-            return None
-
-        return reviews
+        statuses = merge_proposals(target, proposals, self.logger, self.config, self)
+        return statuses
+
 
     def run(self, branch_url=None, launchpad=None, **kwargs):
         for key, value in kwargs.iteritems():
             self.config.set('Tarmac', key, value)
 
-        if self.config.debug:
-            set_up_debug_logging()
-            self.logger.debug('Debug logging enabled')
-        if self.config.http_debug:
-            httplib2.debuglevel = 1
-            self.logger.debug('HTTP debugging enabled.')
-        self.logger.debug('Loading plugins')
-        load_plugins()
-        self.logger.debug('Plugins loaded')
+        setup(self.logger, debug=self.config.debug,
+                http_debug=self.config.http_debug)
 
         self.launchpad = launchpad
         if self.launchpad is None:
@@ -358,23 +371,24 @@
             self.launchpad = self.get_launchpad_object()
             self.logger.debug('launchpad object loaded')
 
+        statuses = []
+
         if branch_url:
             self.logger.debug('%(branch_url)s specified as branch_url' % {
                 'branch_url': branch_url})
             if not branch_url.startswith('lp:'):
                 raise TarmacCommandError('Branch urls must start with lp:')
-            self._do_merges(branch_url)
-
+            statuses.extend(self._do_merges(branch_url))
         else:
             for branch in self.config.branches:
                 self.logger.debug(
                     'Merging approved branches against %(branch)s' % {
                         'branch': branch})
                 try:
-                    merged = self._do_merges(branch)
+                    statuses.extend(self._do_merges(branch))
 
                     # If we've been asked to only merge one branch, then exit.
-                    if merged and self.config.one:
+                    if statuses and self.config.one:
                         break
                 except LockContention:
                     continue