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

Proposed by James Westby on 2013-01-28
Status: Rejected
Rejected by: dobey on 2013-10-31
Proposed branch: lp:~james-w/tarmac/break-up-do-merges
Merge into: lp:tarmac
Prerequisite: lp:~james-w/tarmac/utf8
Diff against target: 454 lines (+206/-192)
1 file modified
tarmac/bin/commands.py (+206/-192)
To merge this branch: bzr merge lp:~james-w/tarmac/break-up-do-merges
Reviewer Review Type Date Requested Status
dobey Disapprove on 2013-10-31
Paul Hummer 2013-01-28 Approve on 2013-02-04
Review via email: mp+145269@code.launchpad.net

This proposal supersedes a proposal from 2013-01-24.

Commit message

Break up the _do_merges method.

Description of the change

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)

To post a comment you must log in.
Paul Hummer (rockstar) wrote :

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

Paul Hummer (rockstar) :
review: Approve
Paul Hummer (rockstar) wrote :

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

Paul Hummer (rockstar) wrote :

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

dobey (dobey) wrote :

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.

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 2013-02-01 17:17:21 +0000
3+++ tarmac/bin/commands.py 2013-02-01 17:17:21 +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,205 +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- success_count = 0
234- try:
235- for proposal in proposals:
236- target.cleanup()
237- self.logger.debug(
238- u'Preparing to merge %(source_branch)s' % {
239- 'source_branch': proposal.source_branch.web_link})
240- try:
241- prerequisite = proposal.prerequisite_branch
242- if prerequisite:
243- merges = [x for x in prerequisite.landing_targets
244- if x.target_branch == target.lp_branch and
245- x.queue_status != u'Superseded']
246- if len(merges) == 0:
247- raise TarmacMergeError(
248- u'No proposals of prerequisite branch.',
249- u'No proposals found for merge of %s '
250- u'into %s.' % (
251- prerequisite.web_link,
252- target.lp_branch.web_link))
253- elif len(merges) > 1:
254- raise TarmacMergeError(
255- u'Too many proposals of prerequisite.',
256- u'More than one proposal found for merge '
257- u'of %s into %s, which is not Superseded.' % (
258- prerequisite.web_link,
259- target.lp_branch.web_link))
260- elif len(merges) == 1:
261- if merges[0].queue_status != u'Merged':
262- raise TarmacMergeError(
263- u'Prerequisite not yet merged.',
264- u'The prerequisite %s has not yet been '
265- u'merged into %s.' % (
266- prerequisite.web_link,
267- target.lp_branch.web_link))
268-
269- if not proposal.reviewed_revid:
270- raise TarmacMergeError(
271- u'No approved revision specified.')
272-
273-
274- source = Branch.create(
275- proposal.source_branch, self.config, target=target)
276-
277- source_bzr_branch = source.get_bzr_branch()
278- approved = source_bzr_branch.revision_id_to_revno(
279- str(proposal.reviewed_revid))
280- tip = source_bzr_branch.revno()
281-
282- if tip > approved:
283- message = u'Unapproved changes made after approval'
284- lp_comment = (
285- u'There are additional revisions which have not '
286- u'been approved in review. Please seek review and '
287- u'approval of these new revisions.')
288- raise UnapprovedChanges(message, lp_comment)
289-
290- self.logger.debug(
291- 'Merging %(source)s at revision %(revision)s' % {
292- 'source': proposal.source_branch.web_link,
293- 'revision': proposal.reviewed_revid})
294-
295- target.merge(source, str(proposal.reviewed_revid))
296-
297- self.logger.debug('Firing tarmac_pre_commit hook')
298- tarmac_hooks.fire('tarmac_pre_commit',
299- self, target, source, proposal)
300-
301- except TarmacMergeError, failure:
302- self.logger.warn(
303- u'Merging %(source)s into %(target)s failed: %(msg)s' %
304- {'source': proposal.source_branch.web_link,
305- 'target': proposal.target_branch.web_link,
306- 'msg': str(failure)})
307-
308- subject = u'Re: [Merge] %(source)s into %(target)s' % {
309- "source": proposal.source_branch.display_name,
310- "target": proposal.target_branch.display_name}
311-
312- if failure.comment:
313- comment = failure.comment
314- else:
315- comment = str(failure)
316-
317- proposal.createComment(subject=subject, content=comment)
318- try:
319- proposal.setStatus(
320- status=self.config.rejected_branch_status)
321- except AttributeError:
322- proposal.setStatus(status=u'Needs review')
323- proposal.lp_save()
324-
325- # If we've been asked to only merge one branch, then exit.
326- if self.config.one:
327- return True
328-
329- continue
330- except PointlessMerge:
331- self.logger.warn(
332- 'Merging %(source)s into %(target)s would be '
333- 'pointless.' % {
334- 'source': proposal.source_branch.web_link,
335- 'target': proposal.target_branch.web_link})
336- continue
337-
338- merge_url = get_review_url(proposal)
339- revprops = {'merge_url': merge_url}
340-
341- commit_message = proposal.commit_message
342- if commit_message is None and self.config.imply_commit_message:
343- commit_message = proposal.description
344-
345- target.commit(commit_message,
346- revprops=revprops,
347- authors=source.authors,
348- reviews=self._get_reviews(proposal))
349-
350- self.logger.debug('Firing tarmac_post_commit hook')
351- tarmac_hooks.fire('tarmac_post_commit',
352- self, target, source, proposal)
353- success_count += 1
354- target.cleanup()
355- if self.config.one:
356- return True
357-
358- # This except is here because we need the else and can't have it
359- # without an except as well.
360- except:
361- raise
362- else:
363- self.logger.debug('Firing tarmac_post_merge hook')
364- tarmac_hooks.fire('tarmac_post_merge',
365- self, target, success_count=success_count)
366- finally:
367- target.cleanup()
368-
369- def _get_mergable_proposals_for_branch(self, lp_branch):
370- """Return a list of the mergable proposals for the given branch."""
371- proposals = []
372- for entry in lp_branch.landing_candidates:
373- self.logger.debug("Considering merge proposal: {0}".format(entry.web_link))
374-
375- if entry.queue_status != u'Approved':
376- self.logger.debug(
377- " Skipping proposal: status is {0}, not "
378- "'Approved'".format(entry.queue_status))
379- continue
380-
381- if (not self.config.imply_commit_message and
382- not entry.commit_message):
383- self.logger.debug(
384- " Skipping proposal: proposal has no commit message")
385- continue
386-
387- proposals.append(entry)
388- return proposals
389-
390- def _get_reviews(self, proposal):
391- """Get the set of reviews from the proposal."""
392- reviews = []
393- for vote in proposal.votes:
394- if not vote.comment:
395- continue
396- else:
397- reviews.append('%s;%s' % (vote.reviewer.display_name,
398- vote.comment.vote))
399-
400- if len(reviews) == 0:
401- return None
402-
403- return reviews
404+ statuses = merge_proposals(target, proposals, self.logger, self.config, self)
405+ return statuses
406+
407
408 def run(self, branch_url=None, launchpad=None, **kwargs):
409 for key, value in kwargs.iteritems():
410 self.config.set('Tarmac', key, value)
411
412- if self.config.debug:
413- set_up_debug_logging()
414- self.logger.debug('Debug logging enabled')
415- if self.config.http_debug:
416- httplib2.debuglevel = 1
417- self.logger.debug('HTTP debugging enabled.')
418- self.logger.debug('Loading plugins')
419- load_plugins()
420- self.logger.debug('Plugins loaded')
421+ setup(self.logger, debug=self.config.debug,
422+ http_debug=self.config.http_debug)
423
424 self.launchpad = launchpad
425 if self.launchpad is None:
426@@ -358,23 +371,24 @@
427 self.launchpad = self.get_launchpad_object()
428 self.logger.debug('launchpad object loaded')
429
430+ statuses = []
431+
432 if branch_url:
433 self.logger.debug('%(branch_url)s specified as branch_url' % {
434 'branch_url': branch_url})
435 if not branch_url.startswith('lp:'):
436 raise TarmacCommandError('Branch urls must start with lp:')
437- self._do_merges(branch_url)
438-
439+ statuses.extend(self._do_merges(branch_url))
440 else:
441 for branch in self.config.branches:
442 self.logger.debug(
443 'Merging approved branches against %(branch)s' % {
444 'branch': branch})
445 try:
446- merged = self._do_merges(branch)
447+ statuses.extend(self._do_merges(branch))
448
449 # If we've been asked to only merge one branch, then exit.
450- if merged and self.config.one:
451+ if statuses and self.config.one:
452 break
453 except LockContention:
454 continue

Subscribers

People subscribed via source and target branches