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()
finally:
  cleanup()

is equivalent to:

try:
  thing()
  another_thing()
finally:
  cleanup()

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

I'm not hugely enamoured of 'except AttributeError', but I've learned
recently that this is a tarmac idiom.

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

This could probably be broken up too, but is still a pretty sensible
grouping as-is.

Wonderful.  Will make some changes, then land.

Thanks,
jml