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

Proposed by James Westby on 2012-12-18
Status: Merged
Merged at revision: 420
Proposed branch: lp:~james-w/tarmac/break-up-do-merges
Merge into: lp:~ubuntuone-hackers/tarmac/trunk
Prerequisite: lp:~james-w/tarmac/commit-message
Diff against target: 453 lines (+206/-191)
1 file modified
tarmac/bin/commands.py (+206/-191)
To merge this branch: bzr merge lp:~james-w/tarmac/break-up-do-merges
Reviewer Review Type Date Requested Status
Jonathan Lange (community) 2012-12-18 Needs Fixing on 2012-12-19
Review via email: mp+140554@code.launchpad.net

This proposal supersedes a proposal from 2012-12-18.

Description of the change

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

To post a comment you must log in.
Jonathan Lange (jml) wrote :
Download full text (8.1 KiB)

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

Read more...

Jonathan Lange (jml) :
review: Needs Fixing

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'tarmac/bin/commands.py'
2--- tarmac/bin/commands.py 2012-09-05 18:08:50 +0000
3+++ tarmac/bin/commands.py 2012-12-18 21:25:42 +0000
4@@ -134,6 +134,194 @@
5 help_commands(self.outf)
6
7
8+def _get_mergable_proposals_for_branch(lp_branch, logger, imply_commit_message=False):
9+ """Return a list of the mergable proposals for the given branch."""
10+ proposals = []
11+ for entry in lp_branch.landing_candidates:
12+ logger.debug("Considering merge proposal: {0}".format(entry.web_link))
13+
14+ if entry.queue_status != u'Approved':
15+ logger.debug(
16+ " Skipping proposal: status is {0}, not "
17+ "'Approved'".format(entry.queue_status))
18+ continue
19+
20+ if (not imply_commit_message and not entry.commit_message):
21+ logger.debug(
22+ " Skipping proposal: proposal has no commit message")
23+ continue
24+
25+ proposals.append(entry)
26+ return proposals
27+
28+
29+def _get_reviews(proposal):
30+ """Get the set of reviews from the proposal."""
31+ reviews = []
32+ for vote in proposal.votes:
33+ if not vote.comment:
34+ continue
35+ else:
36+ reviews.append('%s;%s' % (vote.reviewer.display_name,
37+ vote.comment.vote))
38+
39+ if len(reviews) == 0:
40+ return None
41+
42+ return reviews
43+
44+
45+def setup(logger, debug=False, http_debug=False):
46+ if debug:
47+ set_up_debug_logging()
48+ logger.debug('Debug logging enabled')
49+ if http_debug:
50+ httplib2.debuglevel = 1
51+ logger.debug('HTTP debugging enabled.')
52+ logger.debug('Loading plugins')
53+ load_plugins()
54+ logger.debug('Plugins loaded')
55+
56+
57+def merge_proposals(target, proposals, logger, config, command):
58+ logger.debug('Firing tarmac_pre_merge hook')
59+ tarmac_hooks.fire('tarmac_pre_merge',
60+ command, target)
61+
62+ statuses = []
63+ try:
64+ for proposal in proposals:
65+ status = merge_proposal(target, proposal, logger,
66+ config, command)
67+ statuses.append(status)
68+ # This except is here because we need the else and can't have it
69+ # without an except as well.
70+ except:
71+ raise
72+ else:
73+ logger.debug('Firing tarmac_post_merge hook')
74+ tarmac_hooks.fire('tarmac_post_merge',
75+ command, target)
76+ finally:
77+ target.cleanup()
78+ return statuses
79+
80+
81+def merge_proposal(target, proposal, logger, config, command):
82+ target.cleanup()
83+ logger.debug(
84+ u'Preparing to merge %(source_branch)s' % {
85+ 'source_branch': proposal.source_branch.web_link})
86+ try:
87+ prerequisite = proposal.prerequisite_branch
88+ if prerequisite:
89+ merges = [x for x in prerequisite.landing_targets
90+ if x.target_branch == target.lp_branch and
91+ x.queue_status != u'Superseded']
92+ if len(merges) == 0:
93+ raise TarmacMergeError(
94+ u'No proposals of prerequisite branch.',
95+ u'No proposals found for merge of %s '
96+ u'into %s.' % (
97+ prerequisite.web_link,
98+ target.lp_branch.web_link))
99+ elif len(merges) > 1:
100+ raise TarmacMergeError(
101+ u'Too many proposals of prerequisite.',
102+ u'More than one proposal found for merge '
103+ u'of %s into %s, which is not Superseded.' % (
104+ prerequisite.web_link,
105+ target.lp_branch.web_link))
106+ elif len(merges) == 1:
107+ if merges[0].queue_status != u'Merged':
108+ raise TarmacMergeError(
109+ u'Prerequisite not yet merged.',
110+ u'The prerequisite %s has not yet been '
111+ u'merged into %s.' % (
112+ prerequisite.web_link,
113+ target.lp_branch.web_link))
114+
115+ if not proposal.reviewed_revid:
116+ raise TarmacMergeError(
117+ u'No approved revision specified.')
118+
119+
120+ source = Branch.create(
121+ proposal.source_branch, config, target=target)
122+
123+ source_bzr_branch = source.get_bzr_branch()
124+ approved = source_bzr_branch.revision_id_to_revno(
125+ str(proposal.reviewed_revid))
126+ tip = source_bzr_branch.revno()
127+
128+ if tip > approved:
129+ message = u'Unapproved changes made after approval'
130+ lp_comment = (
131+ u'There are additional revisions which have not '
132+ u'been approved in review. Please seek review and '
133+ u'approval of these new revisions.')
134+ raise UnapprovedChanges(message, lp_comment)
135+
136+ logger.debug(
137+ 'Merging %(source)s at revision %(revision)s' % {
138+ 'source': proposal.source_branch.web_link,
139+ 'revision': proposal.reviewed_revid})
140+
141+ target.merge(source, str(proposal.reviewed_revid))
142+
143+ logger.debug('Firing tarmac_pre_commit hook')
144+ tarmac_hooks.fire('tarmac_pre_commit',
145+ command, target, source, proposal)
146+
147+ except TarmacMergeError, failure:
148+ logger.warn(
149+ u'Merging %(source)s into %(target)s failed: %(msg)s' %
150+ {'source': proposal.source_branch.web_link,
151+ 'target': proposal.target_branch.web_link,
152+ 'msg': str(failure)})
153+
154+ subject = u'Re: [Merge] %(source)s into %(target)s' % {
155+ "source": proposal.source_branch.display_name,
156+ "target": proposal.target_branch.display_name}
157+
158+ if failure.comment:
159+ comment = failure.comment
160+ else:
161+ comment = str(failure)
162+
163+ proposal.createComment(subject=subject, content=comment)
164+ try:
165+ proposal.setStatus(
166+ status=config.rejected_branch_status)
167+ except AttributeError:
168+ proposal.setStatus(status=u'Needs review')
169+ proposal.lp_save()
170+ return False
171+
172+ except PointlessMerge:
173+ logger.warn(
174+ 'Merging %(source)s into %(target)s would be '
175+ 'pointless.' % {
176+ 'source': proposal.source_branch.web_link,
177+ 'target': proposal.target_branch.web_link})
178+ return False
179+
180+ merge_url = get_review_url(proposal)
181+ revprops = {'merge_url': merge_url}
182+
183+ commit_message = proposal.commit_message
184+ if commit_message is None and config.imply_commit_message:
185+ commit_message = proposal.description
186+
187+ target.commit(commit_message,
188+ reviews=_get_reviews(proposal))
189+
190+ logger.debug('Firing tarmac_post_commit hook')
191+ tarmac_hooks.fire('tarmac_post_commit',
192+ command, target, source, proposal)
193+ return True
194+
195+
196 class cmd_merge(TarmacCommand):
197 '''Automatically merge approved merge proposal branches.'''
198
199@@ -143,7 +331,8 @@
200 options.http_debug_option,
201 options.debug_option,
202 options.imply_commit_message_option,
203- options.one_option]
204+ options.one_option,
205+ ]
206
207 def _do_merges(self, branch_url):
208
209@@ -152,204 +341,29 @@
210 self.logger.info('Not a valid branch: {0}'.format(branch_url))
211 return
212
213- proposals = self._get_mergable_proposals_for_branch(lp_branch)
214+ proposals = _get_mergable_proposals_for_branch(lp_branch, self.logger,
215+ imply_commit_message=self.config.imply_commit_message)
216
217 if not proposals:
218 self.logger.info(
219 'No approved proposals found for %(branch_url)s' % {
220 'branch_url': branch_url})
221- return
222+ return []
223+
224+ if self.config.one:
225+ proposals = proposals[0]
226
227 target = Branch.create(lp_branch, self.config, create_tree=True)
228-
229- self.logger.debug('Firing tarmac_pre_merge hook')
230- tarmac_hooks.fire('tarmac_pre_merge',
231- self, target)
232-
233- try:
234- for proposal in proposals:
235- target.cleanup()
236- self.logger.debug(
237- u'Preparing to merge %(source_branch)s' % {
238- 'source_branch': proposal.source_branch.web_link})
239- try:
240- prerequisite = proposal.prerequisite_branch
241- if prerequisite:
242- merges = [x for x in prerequisite.landing_targets
243- if x.target_branch == target.lp_branch and
244- x.queue_status != u'Superseded']
245- if len(merges) == 0:
246- raise TarmacMergeError(
247- u'No proposals of prerequisite branch.',
248- u'No proposals found for merge of %s '
249- u'into %s.' % (
250- prerequisite.web_link,
251- target.lp_branch.web_link))
252- elif len(merges) > 1:
253- raise TarmacMergeError(
254- u'Too many proposals of prerequisite.',
255- u'More than one proposal found for merge '
256- u'of %s into %s, which is not Superseded.' % (
257- prerequisite.web_link,
258- target.lp_branch.web_link))
259- elif len(merges) == 1:
260- if merges[0].queue_status != u'Merged':
261- raise TarmacMergeError(
262- u'Prerequisite not yet merged.',
263- u'The prerequisite %s has not yet been '
264- u'merged into %s.' % (
265- prerequisite.web_link,
266- target.lp_branch.web_link))
267-
268- if not proposal.reviewed_revid:
269- raise TarmacMergeError(
270- u'No approved revision specified.')
271-
272-
273- source = Branch.create(
274- proposal.source_branch, self.config, target=target)
275-
276- source_bzr_branch = source.get_bzr_branch()
277- approved = source_bzr_branch.revision_id_to_revno(
278- str(proposal.reviewed_revid))
279- tip = source_bzr_branch.revno()
280-
281- if tip > approved:
282- message = u'Unapproved changes made after approval'
283- lp_comment = (
284- u'There are additional revisions which have not '
285- u'been approved in review. Please seek review and '
286- u'approval of these new revisions.')
287- raise UnapprovedChanges(message, lp_comment)
288-
289- self.logger.debug(
290- 'Merging %(source)s at revision %(revision)s' % {
291- 'source': proposal.source_branch.web_link,
292- 'revision': proposal.reviewed_revid})
293-
294- target.merge(source, str(proposal.reviewed_revid))
295-
296- self.logger.debug('Firing tarmac_pre_commit hook')
297- tarmac_hooks.fire('tarmac_pre_commit',
298- self, target, source, proposal)
299-
300- except TarmacMergeError, failure:
301- self.logger.warn(
302- u'Merging %(source)s into %(target)s failed: %(msg)s' %
303- {'source': proposal.source_branch.web_link,
304- 'target': proposal.target_branch.web_link,
305- 'msg': str(failure)})
306-
307- subject = u'Re: [Merge] %(source)s into %(target)s' % {
308- "source": proposal.source_branch.display_name,
309- "target": proposal.target_branch.display_name}
310-
311- if failure.comment:
312- comment = failure.comment
313- else:
314- comment = str(failure)
315-
316- proposal.createComment(subject=subject, content=comment)
317- try:
318- proposal.setStatus(
319- status=self.config.rejected_branch_status)
320- except AttributeError:
321- proposal.setStatus(status=u'Needs review')
322- proposal.lp_save()
323-
324- # If we've been asked to only merge one branch, then exit.
325- if self.config.one:
326- return True
327-
328- continue
329- except PointlessMerge:
330- self.logger.warn(
331- 'Merging %(source)s into %(target)s would be '
332- 'pointless.' % {
333- 'source': proposal.source_branch.web_link,
334- 'target': proposal.target_branch.web_link})
335- continue
336-
337- merge_url = get_review_url(proposal)
338- revprops = {'merge_url': merge_url}
339-
340- commit_message = proposal.commit_message
341- if commit_message is None and self.config.imply_commit_message:
342- commit_message = proposal.description
343-
344- target.commit(commit_message,
345- revprops=revprops,
346- authors=source.authors,
347- reviews=self._get_reviews(proposal))
348-
349- self.logger.debug('Firing tarmac_post_commit hook')
350- tarmac_hooks.fire('tarmac_post_commit',
351- self, target, source, proposal)
352-
353- # If we've been asked to only merge one branch, then exit.
354- if self.config.one:
355- return True
356-
357- # This except is here because we need the else and can't have it
358- # without an except as well.
359- except:
360- raise
361- else:
362- self.logger.debug('Firing tarmac_post_merge hook')
363- tarmac_hooks.fire('tarmac_post_merge',
364- self, target)
365- finally:
366- target.cleanup()
367-
368- def _get_mergable_proposals_for_branch(self, lp_branch):
369- """Return a list of the mergable proposals for the given branch."""
370- proposals = []
371- for entry in lp_branch.landing_candidates:
372- self.logger.debug("Considering merge proposal: {0}".format(entry.web_link))
373-
374- if entry.queue_status != u'Approved':
375- self.logger.debug(
376- " Skipping proposal: status is {0}, not "
377- "'Approved'".format(entry.queue_status))
378- continue
379-
380- if (not self.config.imply_commit_message and
381- not entry.commit_message):
382- self.logger.debug(
383- " Skipping proposal: proposal has no commit message")
384- continue
385-
386- proposals.append(entry)
387- return proposals
388-
389- def _get_reviews(self, proposal):
390- """Get the set of reviews from the proposal."""
391- reviews = []
392- for vote in proposal.votes:
393- if not vote.comment:
394- continue
395- else:
396- reviews.append('%s;%s' % (vote.reviewer.display_name,
397- vote.comment.vote))
398-
399- if len(reviews) == 0:
400- return None
401-
402- return reviews
403+ statuses = merge_proposals(target, proposals, self.logger, self.config, self)
404+ return statuses
405+
406
407 def run(self, branch_url=None, launchpad=None, **kwargs):
408 for key, value in kwargs.iteritems():
409 self.config.set('Tarmac', key, value)
410
411- if self.config.debug:
412- set_up_debug_logging()
413- self.logger.debug('Debug logging enabled')
414- if self.config.http_debug:
415- httplib2.debuglevel = 1
416- self.logger.debug('HTTP debugging enabled.')
417- self.logger.debug('Loading plugins')
418- load_plugins()
419- self.logger.debug('Plugins loaded')
420+ setup(self.logger, debug=self.config.debug,
421+ http_debug=self.config.http_debug)
422
423 self.launchpad = launchpad
424 if self.launchpad is None:
425@@ -357,23 +371,24 @@
426 self.launchpad = self.get_launchpad_object()
427 self.logger.debug('launchpad object loaded')
428
429+ statuses = []
430+
431 if branch_url:
432 self.logger.debug('%(branch_url)s specified as branch_url' % {
433 'branch_url': branch_url})
434 if not branch_url.startswith('lp:'):
435 raise TarmacCommandError('Branch urls must start with lp:')
436- self._do_merges(branch_url)
437-
438+ statuses.extend(self._do_merges(branch_url))
439 else:
440 for branch in self.config.branches:
441 self.logger.debug(
442 'Merging approved branches against %(branch)s' % {
443 'branch': branch})
444 try:
445- merged = self._do_merges(branch)
446+ statuses.extend(self._do_merges(branch))
447
448 # If we've been asked to only merge one branch, then exit.
449- if merged and self.config.one:
450+ if statuses and self.config.one:
451 break
452 except LockContention:
453 continue

Subscribers

People subscribed via source and target branches