Merge lp:~parthm/bzr/300062-bad-pattern-error-part-2 into lp:bzr

Proposed by Parth Malwankar on 2010-07-08
Status: Rejected
Rejected by: Parth Malwankar on 2010-07-15
Proposed branch: lp:~parthm/bzr/300062-bad-pattern-error-part-2
Merge into: lp:bzr
Diff against target: 272 lines (+128/-38)
6 files modified
NEWS (+1/-0)
bzrlib/builtins.py (+8/-0)
bzrlib/globbing.py (+73/-23)
bzrlib/tests/blackbox/test_ignore.py (+16/-0)
bzrlib/tests/per_workingtree/test_is_ignored.py (+25/-13)
bzrlib/tests/test_globbing.py (+5/-2)
To merge this branch: bzr merge lp:~parthm/bzr/300062-bad-pattern-error-part-2
Reviewer Review Type Date Requested Status
bzr-core 2010-07-08 Pending
Review via email: mp+29467@code.launchpad.net

Description of the change

=== Fix towards bug #300062 ===
This is the second patch towards bug #300062. The first patch can be found at [1].
This does the following:
 1. `bzr ignore` now fails in case an invalid pattern is supplied. The bad pattern is displayed to the user.
 2. In case an invalid pattern is encountered in .bzrignore or user ignore file, the error message shown now displays the specific pattern(s) that caused the failure.
 3. Some code cleanup is done Globster.

One thing pending is that the error messages do not show what file the bad pattern came from. Ideally, I would like to show a message like "file:line_number:pattern". This requires updating the ignore file parsing logic to store line number and file name information. I plan to look at that in a separate patch. Suggestions welcome.

Sample output:
[a1234]% ~/src/bzr.dev/300062-bad-pattern-error-part-2/bzr ignore "RE:[" foo "RE:*.cpp"
bzr: error: Invalid ignore pattern(s) found.
  RE:[
  RE:*.cpp
bzr: ERROR: Invalid pattern(s) found.
[a1234]% ~/src/bzr.dev/300062-bad-pattern-error-part-2/bzr st
bzr: ERROR: Invalid pattern(s) found. File ~/.bazaar/ignore or .bzrignore contains error(s).
  RE:[
[a1234]%

[1] https://code.launchpad.net/~parthm/bzr/300062-bad-pattern-error-part-1/+merge/28889

To post a comment you must log in.
Parth Malwankar (parthm) wrote :

Is 2.2b4 being considered a feature freeze or can this patch be considered for 2.2?
IMO it would be nice to have this in if possible as the user gets to see the failing pattern in the error message. The specific failing ignore file is not shown but that can be a 2.3 feature.

Martin Pool (mbp) wrote :

On 10 July 2010 19:37, Parth Malwankar <email address hidden> wrote:
> Is 2.2b4 being considered a feature freeze or can this patch be considered for 2.2?
> IMO it would be nice to have this in if possible as the user gets to see the failing pattern in the error message. The specific failing ignore file is not shown but that can be a 2.3 feature.

Based on the cover letter I think this would be fine for 2.2.

--
Martin

Parth Malwankar (parthm) wrote :

I am rejecting this since https://code.launchpad.net/~parthm/bzr/300062-2.2-bad-pattern-error-part-2/+merge/29949 proposes the same changes for 2.2

Unmerged revisions

5348. By Parth Malwankar on 2010-07-19

order of patterns added using _add_pattern is predictable

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2010-07-08 10:34:12 +0000
3+++ NEWS 2010-07-08 13:41:54 +0000
4@@ -94,6 +94,7 @@
5
6 * Invalid patterns supplied to ``Globster`` or ``lazy_regex`` now raise
7 ``InvalidPattern`` exception showing clear error message to the user.
8+ ``bzr ignore PATTERNS`` exits with error if a bad pattern is supplied.
9 (Parth Malwankar #300062)
10
11 * Progress output is cleaned up when exiting. (Aaron Bentley)
12
13=== modified file 'bzrlib/builtins.py'
14--- bzrlib/builtins.py 2010-06-30 12:13:14 +0000
15+++ bzrlib/builtins.py 2010-07-08 13:41:54 +0000
16@@ -2712,6 +2712,14 @@
17 "NAME_PATTERN or --default-rules.")
18 name_pattern_list = [globbing.normalize_pattern(p)
19 for p in name_pattern_list]
20+ bad_patterns = ''
21+ for p in name_pattern_list:
22+ if not globbing.Globster.is_pattern_valid(p):
23+ bad_patterns += ('\n %s' % p)
24+ if bad_patterns:
25+ msg = ('Invalid ignore pattern(s) found. %s' % bad_patterns)
26+ ui.ui_factory.show_error(msg)
27+ raise errors.InvalidPattern('')
28 for name_pattern in name_pattern_list:
29 if (name_pattern[0] == '/' or
30 (len(name_pattern) > 1 and name_pattern[1] == ':')):
31
32=== modified file 'bzrlib/globbing.py'
33--- bzrlib/globbing.py 2010-07-08 05:11:16 +0000
34+++ bzrlib/globbing.py 2010-07-08 13:41:54 +0000
35@@ -179,24 +179,38 @@
36 so are matched first, then the basename patterns, then the fullpath
37 patterns.
38 """
39+ TYPE_FULLPATH = 1
40+ TYPE_BASENAME = 2
41+ TYPE_EXTENSION = 3
42+
43+ translators = {
44+ TYPE_FULLPATH : _sub_fullpath,
45+ TYPE_BASENAME : _sub_basename,
46+ TYPE_EXTENSION : _sub_extension,
47+ }
48+
49+ # Prefixes used to combine various patterns.
50+ # See: Globster._add_patterns
51+ prefixes = {
52+ TYPE_FULLPATH : r'',
53+ TYPE_BASENAME : r'(?:.*/)?(?!.*/)',
54+ TYPE_EXTENSION : r'(?:.*/)?(?!.*/)(?:.*\.)',
55+ }
56+
57 def __init__(self, patterns):
58 self._regex_patterns = []
59- path_patterns = []
60- base_patterns = []
61- ext_patterns = []
62+ pattern_lists = {
63+ Globster.TYPE_FULLPATH : [],
64+ Globster.TYPE_EXTENSION : [],
65+ Globster.TYPE_BASENAME : [],
66+ }
67 for pat in patterns:
68 pat = normalize_pattern(pat)
69- if pat.startswith(u'RE:') or u'/' in pat:
70- path_patterns.append(pat)
71- elif pat.startswith(u'*.'):
72- ext_patterns.append(pat)
73- else:
74- base_patterns.append(pat)
75- self._add_patterns(ext_patterns,_sub_extension,
76- prefix=r'(?:.*/)?(?!.*/)(?:.*\.)')
77- self._add_patterns(base_patterns,_sub_basename,
78- prefix=r'(?:.*/)?(?!.*/)')
79- self._add_patterns(path_patterns,_sub_fullpath)
80+ pattern_lists[Globster.identify(pat)].append(pat)
81+ for pattern_type, patterns in pattern_lists.iteritems():
82+ self._add_patterns(patterns,
83+ Globster.translators[pattern_type],
84+ Globster.prefixes[pattern_type])
85
86 def _add_patterns(self, patterns, translator, prefix=''):
87 while patterns:
88@@ -221,10 +235,51 @@
89 # the combined pattern we sent to regex. Instead we indicate to
90 # the user that an ignore file needs fixing.
91 mutter('Invalid pattern found in regex: %s.', e.message)
92- e.message = "File ~/.bazaar/ignore or .bzrignore contains errors."
93+ e.message = "File ~/.bazaar/ignore or .bzrignore contains error(s)."
94+ bad_patterns = ''
95+ for _, patterns in self._regex_patterns:
96+ for p in patterns:
97+ if not Globster.is_pattern_valid(p):
98+ bad_patterns += ('\n %s' % p)
99+ e.message += bad_patterns
100 raise e
101 return None
102
103+ @staticmethod
104+ def identify(pattern):
105+ """Returns pattern category.
106+
107+ :param pattern: normalized pattern.
108+ Identify if a pattern is fullpath, basename or extension
109+ and returns the appropriate type.
110+ """
111+ if pattern.startswith(u'RE:') or u'/' in pattern:
112+ return Globster.TYPE_FULLPATH
113+ elif pattern.startswith(u'*.'):
114+ return Globster.TYPE_EXTENSION
115+ else:
116+ return Globster.TYPE_BASENAME
117+
118+ @staticmethod
119+ def is_pattern_valid(pattern):
120+ """Returns True if pattern is valid.
121+
122+ :param pattern: Normalized pattern.
123+ is_pattern_valid() assumes pattern to be normalized.
124+ see: globbing.normalize_pattern
125+ """
126+ result = True
127+ translator = Globster.translators[Globster.identify(pattern)]
128+ tpattern = '(%s)' % translator(pattern)
129+ try:
130+ re_obj = re.compile(tpattern, re.UNICODE)
131+ re_obj.search("") # force compile
132+ except errors.InvalidPattern, e:
133+ result = False
134+ return result
135+
136+
137+
138 class ExceptionGlobster(object):
139 """A Globster that supports exception patterns.
140
141@@ -272,14 +327,9 @@
142 self._regex_patterns = []
143 for pat in patterns:
144 pat = normalize_pattern(pat)
145- if pat.startswith(u'RE:') or u'/' in pat:
146- self._add_patterns([pat], _sub_fullpath)
147- elif pat.startswith(u'*.'):
148- self._add_patterns([pat], _sub_extension,
149- prefix=r'(?:.*/)?(?!.*/)(?:.*\.)')
150- else:
151- self._add_patterns([pat], _sub_basename,
152- prefix=r'(?:.*/)?(?!.*/)')
153+ pat_type = Globster.identify(pat)
154+ self._add_patterns([pat], Globster.translators[pat_type],
155+ Globster.prefixes[pat_type])
156
157
158 _slashes = re.compile(r'[\\/]+')
159
160=== modified file 'bzrlib/tests/blackbox/test_ignore.py'
161--- bzrlib/tests/blackbox/test_ignore.py 2010-06-11 07:32:12 +0000
162+++ bzrlib/tests/blackbox/test_ignore.py 2010-07-08 13:41:54 +0000
163@@ -162,3 +162,19 @@
164 tree = self.make_branch_and_tree('a')
165 self.run_bzr(['ignore', '--directory=a', 'README'])
166 self.check_file_contents('a/.bzrignore', 'README\n')
167+
168+ def test_ignored_invalid_pattern(self):
169+ """Ensure graceful handling for invalid ignore pattern.
170+
171+ Test case for #300062.
172+ Invalid pattern should show clear error message.
173+ Invalid pattern should not be added to .bzrignore file.
174+ """
175+ tree = self.make_branch_and_tree('.')
176+ out, err = self.run_bzr(['ignore', 'RE:*.cpp', 'foo', 'RE:['], 3)
177+ self.assertEqual(out, '')
178+ self.assertContainsRe(err,
179+ 'Invalid ignore pattern.*RE:\*\.cpp.*RE:\[', re.DOTALL)
180+ self.assertNotContainsRe(err, 'foo', re.DOTALL)
181+ self.assertFalse(os.path.isfile('.bzrignore'))
182+
183
184=== modified file 'bzrlib/tests/per_workingtree/test_is_ignored.py'
185--- bzrlib/tests/per_workingtree/test_is_ignored.py 2010-02-17 17:11:16 +0000
186+++ bzrlib/tests/per_workingtree/test_is_ignored.py 2010-07-08 13:41:54 +0000
187@@ -22,6 +22,16 @@
188
189 class TestIsIgnored(TestCaseWithWorkingTree):
190
191+ def _set_user_ignore_content(self, ignores):
192+ """Create user ignore file and set its content to ignores."""
193+ config.ensure_config_dir_exists()
194+ user_ignore_file = config.user_ignore_config_filename()
195+ f = open(user_ignore_file, 'wb')
196+ try:
197+ f.write(ignores)
198+ finally:
199+ f.close()
200+
201 def test_is_ignored(self):
202 tree = self.make_branch_and_tree('.')
203 # this will break if a tree changes the ignored format. That is fine
204@@ -46,6 +56,11 @@
205 '#comment\n'
206 ' xx \n' # whitespace
207 )])
208+ # We set user ignore file to contain '' to avoid patterns from
209+ # user ignore being used instead of bzrignore. For .e.g. If we
210+ # don't do this 'foo.~1~' will match '*~' default user ignore
211+ # pattern instead of '*.~*' from bzr ignore as we expect below.
212+ self._set_user_ignore_content('')
213 # is_ignored returns the matching ignore regex when a path is ignored.
214 # we check some expected matches for each rule, and one or more
215 # relevant not-matches that look plausible as cases for bugs.
216@@ -119,19 +134,16 @@
217
218 config.ensure_config_dir_exists()
219 user_ignore_file = config.user_ignore_config_filename()
220- f = open(user_ignore_file, 'wb')
221- try:
222- f.write('*.py[co]\n'
223- './.shelf\n'
224- '# comment line\n'
225- '\n' #Blank line
226- '\r\n' #Blank dos line
227- ' * \n' #Trailing and suffix spaces
228- 'crlf\r\n' # dos style line
229- '*\xc3\xa5*\n' # u'\xe5'.encode('utf8')
230- )
231- finally:
232- f.close()
233+ self._set_user_ignore_content(
234+ '*.py[co]\n'
235+ './.shelf\n'
236+ '# comment line\n'
237+ '\n' #Blank line
238+ '\r\n' #Blank dos line
239+ ' * \n' #Trailing and suffix spaces
240+ 'crlf\r\n' # dos style line
241+ '*\xc3\xa5*\n' # u'\xe5'.encode('utf8')
242+ )
243
244 # Rooted
245 self.assertEqual('./.shelf', tree.is_ignored('.shelf'))
246
247=== modified file 'bzrlib/tests/test_globbing.py'
248--- bzrlib/tests/test_globbing.py 2010-06-30 13:49:02 +0000
249+++ bzrlib/tests/test_globbing.py 2010-07-08 13:41:54 +0000
250@@ -15,6 +15,8 @@
251 # along with this program; if not, write to the Free Software
252 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
253
254+import re
255+
256 from bzrlib import errors
257 from bzrlib.globbing import (
258 Globster,
259@@ -311,10 +313,11 @@
260
261 def test_bad_pattern(self):
262 """Ensure that globster handles bad patterns cleanly."""
263- patterns = [u'RE:[']
264+ patterns = [u'RE:[', u'foo', u'RE:*.cpp']
265 g = Globster(patterns)
266 e = self.assertRaises(errors.InvalidPattern, g.match, 'foo')
267- self.assertContainsRe(e.message, "File.*ignore.*contains errors")
268+ self.assertContainsRe(e.message,
269+ "File.*ignore.*contains error.*RE:\[.*RE:\*\.cpp", flags=re.DOTALL)
270
271
272 class TestExceptionGlobster(TestCase):