Merge lp:~jelmer/brz/bad-bug-url into lp:brz

Proposed by Jelmer Vernooij on 2018-11-17
Status: Merged
Approved by: Jelmer Vernooij on 2018-11-17
Approved revision: 7188
Merge reported by: The Breezy Bot
Merged at revision: not available
Proposed branch: lp:~jelmer/brz/bad-bug-url
Merge into: lp:brz
Diff against target: 266 lines (+135/-33)
5 files modified
breezy/bugtracker.py (+30/-1)
breezy/log.py (+20/-15)
breezy/revision.py (+2/-9)
breezy/tests/test_bugtracker.py (+33/-0)
breezy/tests/test_log.py (+50/-8)
To merge this branch: bzr merge lp:~jelmer/brz/bad-bug-url
Reviewer Review Type Date Requested Status
Martin Packman 2018-11-17 Approve on 2018-11-17
Review via email: mp+358942@code.launchpad.net

Description of the change

Improve handling of bad bug urls:

* Raise an exception when invalid bug URLs are specified to 'brz commit'
* Print exceptions when customer log handlers raise an exception
  rather than aborting altogether.

To post a comment you must log in.
Martin Packman (gz) wrote :

Looks reasonable, see two inline comments. Also, decoded bug urls should probably be defined as either url-escaped ascii or cleanly decoded to unicode, but that can change later.

review: Approve
lp:~jelmer/brz/bad-bug-url updated on 2018-11-17
7188. By Jelmer Vernooij on 2018-11-17

More beees.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'breezy/bugtracker.py'
2--- breezy/bugtracker.py 2018-11-16 11:37:47 +0000
3+++ breezy/bugtracker.py 2018-11-17 17:17:17 +0000
4@@ -186,6 +186,14 @@
5 self.line = line
6
7
8+class InvalidBugUrl(errors.BzrError):
9+
10+ _fmt = "Invalid bug URL: %(url)s"
11+
12+ def __init__(self, url):
13+ self.url = url
14+
15+
16 class InvalidBugStatus(errors.BzrError):
17
18 _fmt = ("Invalid bug status: '%(status)s'")
19@@ -420,4 +428,25 @@
20 :return: A string that will be set as the 'bugs' property of a revision
21 as part of a commit.
22 """
23- return '\n'.join(('%s %s' % (url, tag)) for (url, tag) in bug_urls)
24+ lines = []
25+ for (url, tag) in bug_urls:
26+ if ' ' in url:
27+ raise InvalidBugUrl(url)
28+ lines.append('%s %s' % (url, tag))
29+ return '\n'.join(lines)
30+
31+
32+def decode_bug_urls(bug_text):
33+ """Decode a bug property text.
34+
35+ :param bug_text: Contents of a bugs property
36+ :return: iterator over (url, status) tuples
37+ """
38+ for line in bug_text.splitlines():
39+ try:
40+ url, status = line.split(None, 2)
41+ except ValueError:
42+ raise InvalidLineInBugsProperty(line)
43+ if status not in ALLOWED_BUG_STATUSES:
44+ raise InvalidBugStatus(status)
45+ yield url, status
46
47=== modified file 'breezy/log.py'
48--- breezy/log.py 2018-11-16 19:09:31 +0000
49+++ breezy/log.py 2018-11-17 17:17:17 +0000
50@@ -75,6 +75,7 @@
51 errors,
52 registry,
53 revisionspec,
54+ trace,
55 )
56 from .osutils import (
57 format_date,
58@@ -1522,7 +1523,11 @@
59 """
60 lines = self._foreign_info_properties(revision)
61 for key, handler in properties_handler_registry.iteritems():
62- lines.extend(self._format_properties(handler(revision)))
63+ try:
64+ lines.extend(self._format_properties(handler(revision)))
65+ except Exception:
66+ trace.log_exception_quietly()
67+ trace.print_exception(sys.exc_info(), self.to_file)
68 return lines
69
70 def _foreign_info_properties(self, rev):
71@@ -2156,21 +2161,21 @@
72
73
74 def _bugs_properties_handler(revision):
75+ fixed_bug_urls = []
76+ related_bug_urls = []
77+ for bug_url, status in revision.iter_bugs():
78+ if status == 'fixed':
79+ fixed_bug_urls.append(bug_url)
80+ elif status == 'related':
81+ related_bug_urls.append(bug_url)
82 ret = {}
83- if 'bugs' in revision.properties:
84- bug_lines = revision.properties['bugs'].split('\n')
85- bug_rows = [line.split(' ', 1) for line in bug_lines]
86- fixed_bug_urls = [row[0] for row in bug_rows if
87- len(row) > 1 and row[1] == 'fixed']
88- related_bug_urls = [row[0] for row in bug_rows if
89- len(row) > 1 and row[1] == 'related']
90- if fixed_bug_urls:
91- text = ngettext('fixes bug', 'fixes bugs', len(fixed_bug_urls))
92- ret[text] = ' '.join(fixed_bug_urls)
93- if related_bug_urls:
94- text = ngettext('related bug', 'related bugs',
95- len(related_bug_urls))
96- ret[text] = ' '.join(related_bug_urls)
97+ if fixed_bug_urls:
98+ text = ngettext('fixes bug', 'fixes bugs', len(fixed_bug_urls))
99+ ret[text] = ' '.join(fixed_bug_urls)
100+ if related_bug_urls:
101+ text = ngettext('related bug', 'related bugs',
102+ len(related_bug_urls))
103+ ret[text] = ' '.join(related_bug_urls)
104 return ret
105
106
107
108=== modified file 'breezy/revision.py'
109--- breezy/revision.py 2018-11-16 12:32:43 +0000
110+++ breezy/revision.py 2018-11-17 17:17:17 +0000
111@@ -147,15 +147,8 @@
112 """Iterate over the bugs associated with this revision."""
113 bug_property = self.properties.get('bugs', None)
114 if bug_property is None:
115- return
116- for line in bug_property.splitlines():
117- try:
118- url, status = line.split(None, 2)
119- except ValueError:
120- raise bugtracker.InvalidLineInBugsProperty(line)
121- if status not in bugtracker.ALLOWED_BUG_STATUSES:
122- raise bugtracker.InvalidBugStatus(status)
123- yield url, status
124+ return iter([])
125+ return bugtracker.decode_bug_urls(bug_property)
126
127
128 def iter_ancestors(revision_id, revision_source, only_present=False):
129
130=== modified file 'breezy/tests/test_bugtracker.py'
131--- breezy/tests/test_bugtracker.py 2018-11-16 11:37:47 +0000
132+++ breezy/tests/test_bugtracker.py 2018-11-17 17:17:17 +0000
133@@ -342,3 +342,36 @@
134 bugtracker.encode_fixes_bug_urls(
135 [('http://example.com/bugs/1', 'fixed'),
136 ('http://example.com/bugs/2', 'related')]))
137+
138+ def test_encoding_with_space(self):
139+ self.assertRaises(
140+ bugtracker.InvalidBugUrl,
141+ bugtracker.encode_fixes_bug_urls,
142+ [('http://example.com/bugs/ 1', 'fixed')])
143+
144+
145+class TestPropertyDecoding(TestCase):
146+ """Tests for parsing bug revision properties."""
147+
148+ def test_decoding_one(self):
149+ self.assertEqual(
150+ [('http://example.com/bugs/1', 'fixed')],
151+ list(bugtracker.decode_bug_urls(
152+ 'http://example.com/bugs/1 fixed')))
153+
154+ def test_decoding_zero(self):
155+ self.assertEqual([], list(bugtracker.decode_bug_urls('')))
156+
157+ def test_decoding_two(self):
158+ self.assertEqual(
159+ [('http://example.com/bugs/1', 'fixed'),
160+ ('http://example.com/bugs/2', 'related')],
161+ list(bugtracker.decode_bug_urls(
162+ 'http://example.com/bugs/1 fixed\n'
163+ 'http://example.com/bugs/2 related')))
164+
165+ def test_decoding_invalid(self):
166+ self.assertRaises(
167+ bugtracker.InvalidLineInBugsProperty,
168+ list,
169+ bugtracker.decode_bug_urls('http://example.com/bugs/ 1 fixed\n'))
170
171=== modified file 'breezy/tests/test_log.py'
172--- breezy/tests/test_log.py 2018-11-11 04:08:32 +0000
173+++ breezy/tests/test_log.py 2018-11-17 17:17:17 +0000
174@@ -15,6 +15,7 @@
175 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
176
177 import os
178+import re
179
180 from .. import (
181 branchbuilder,
182@@ -728,7 +729,9 @@
183 log.properties_handler_registry.register(
184 'trivial_custom_prop_handler',
185 trivial_custom_prop_handler)
186- self.assertRaises(Exception, log.show_log, wt.branch, formatter,)
187+ log.show_log(wt.branch, formatter)
188+ self.assertContainsRe(
189+ sio.getvalue(), b'brz: ERROR: Exception: a test error')
190
191 def test_properties_handler_bad_argument(self):
192 wt = self.make_standard_commit('bad_argument',
193@@ -748,7 +751,7 @@
194
195 revision = wt.branch.repository.get_revision(wt.branch.last_revision())
196 formatter.show_properties(revision, '')
197- self.assertEqualDiff(b'''custom_prop_name: test_value\n''',
198+ self.assertEqualDiff(b'custom_prop_name: test_value\n',
199 sio.getvalue())
200
201 def test_show_ids(self):
202@@ -1395,6 +1398,35 @@
203 'test://bug/2 fixed'})
204 return tree
205
206+ def test_bug_broken(self):
207+ tree = self.make_branch_and_tree(u'.')
208+ self.build_tree(['a', 'b'])
209+ tree.add('a')
210+ self.wt_commit(tree, 'simple log message', rev_id=b'a1',
211+ revprops={u'bugs': 'test://bua g/id fixed'})
212+
213+ logfile = self.make_utf8_encoded_stringio()
214+ formatter = log.LongLogFormatter(to_file=logfile)
215+ log.show_log(tree.branch, formatter)
216+
217+ self.assertContainsRe(
218+ logfile.getvalue(),
219+ b'brz: ERROR: breezy.bugtracker.InvalidLineInBugsProperty: '
220+ b'Invalid line in bugs property: \'test://bua g/id fixed\'')
221+
222+ text = logfile.getvalue()
223+ self.assertEqualDiff(
224+ text[text.index(b'-' * 60):],
225+ b"""\
226+------------------------------------------------------------
227+revno: 1
228+committer: Joe Foo <joe@foo.com>
229+branch nick: work
230+timestamp: Tue 2005-11-22 00:00:00 +0000
231+message:
232+ simple log message
233+""")
234+
235 def test_long_bugs(self):
236 tree = self.make_commits_with_bugs()
237 self.assertFormatterResult(b"""\
238@@ -1441,12 +1473,22 @@
239 self.build_tree(['foo'])
240 self.wt_commit(tree, 'simple log message', rev_id=b'a1',
241 revprops={u'bugs': 'test://bug/id invalid_value'})
242- self.assertFormatterResult(b"""\
243- 1 Joe Foo\t2005-11-22
244- simple log message
245-
246-""",
247- tree.branch, log.ShortLogFormatter)
248+
249+ logfile = self.make_utf8_encoded_stringio()
250+ formatter = log.ShortLogFormatter(to_file=logfile)
251+ log.show_log(tree.branch, formatter)
252+
253+ lines = logfile.getvalue().splitlines()
254+
255+ self.assertEqual(
256+ lines[0], b' 1 Joe Foo\t2005-11-22')
257+
258+ self.assertEqual(
259+ lines[1],
260+ b'brz: ERROR: breezy.bugtracker.InvalidBugStatus: Invalid '
261+ b'bug status: \'invalid_value\'')
262+
263+ self.assertEqual(lines[-2], b" simple log message")
264
265 def test_bugs_handler_present(self):
266 self.properties_handler_registry.get('bugs_properties_handler')

Subscribers

People subscribed via source and target branches