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

[James Westby]

Hi,

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.

Thanks,

James

[Jonathan Lange (jml)]

On 18 December 2012 21:26, James Westby <email address hidden> wrote:
> James Westby has proposed merging lp:~james-w/tarmac/break-up-do-merges into lp:~ubuntuone-hackers/tarmac/trunk with lp:~james-w/tarmac/commit-message as a prerequisite.
>
> Requested reviews:
> Ubuntu One hackers (ubuntuone-hackers)
>
> For more details, see:
> https://code.launchpad.net/~james-w/tarmac/break-up-do-merges/+merge/140554
>
> Hi,
>
> This branch breaks up the _do_merges method a little, as it was huge.
>

It was!

See inline.

> 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.
>

...
> === modified file 'tarmac/bin/commands.py'
> --- tarmac/bin/commands.py 2012-09-05 18:08:50 +0000
> +++ tarmac/bin/commands.py 2012-12-18 21:25:42 +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
> +

This seems a natural candidate for an iterator. Still, no big deal.

> +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
> +

Again, a potential iterator, and again, I don't care enough for this branch.

Implementation could probably be more functional, but even this is a
huge step forwards.

> +
> +def setup(logger, debug=False, http_debug=False):

"setup" is a noun, "set up" a verb phrase. This should be spelled
"set_up". Also, it should probably say what it's going to set up.

> +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.

Wat.

try:
  thing()
except:
  raise
else:
  another_thing()
...

[Jonathan Lange (jml)]

review: Needs Fixing

=== modified file 'tarmac/bin/commands.py'
--- tarmac/bin/commands.py	2012-09-05 18:08:50 +0000
+++ tarmac/bin/commands.py	2012-12-18 21:25:42 +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,204 +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)
-
-        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)
-
-                # If we've been asked to only merge one branch, then exit.
-                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)
-        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:
@@ -357,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