Merge lp:~parthm/bzr/300062-invalid-pattern-warnings into lp:bzr

[Parth Malwankar]

Better handling for invalid patterns in .bzrignore and user ignore. ignore command rejects bad patterns passed to it.

=== Fixed Bug #300062 ===

Based on the earlier discussion
https://code.edge.launchpad.net/~parthm/bzr/300062-better-handling-for-invalid-ignore-pattern/+merge/26400
this branch make bzr issue a warning if there are invalid patterns in any ignore file. 'ls', 'add', 'ignore', 'ignored' and 'status' work as expected. Below is a sample interaction of these commands.

[aaa]% cat .bzrignore
RE:*.cpp
foo
[aaa]% ~/src/bzr.dev/300062-invalid-pattern-warnings/bzr ignore xyz "RE:[[[[[" pqr
bzr: warning: Skipped invalid pattern argument(s):
  RE:[[[[[
bzr: warning: Skipped invalid pattern(s):
  RE:*.cpp
Please fix above patterns in ".bzrignore" or "/home/parthm/.bazaar/ignore".
Bazaar commands will NOT ignore files meant to match above pattern(s).
[aaa]% cat .bzrignore
RE:*.cpp
foo
xyz
pqr
[aaa]%

=================================================================================
[aaa]% ~/src/bzr.dev/300062-invalid-pattern-warnings/bzr st -S
 M .bzrignore
bzr: warning: Skipped invalid pattern(s):
  RE:*.cpp
Please fix above patterns in ".bzrignore" or "/home/parthm/.bazaar/ignore".
Bazaar commands will NOT ignore files meant to match above pattern(s).
? one.txt
? two.txt
? x
? y
[aaa]% ~/src/bzr.dev/300062-invalid-pattern-warnings/bzr st
bzr: warning: Skipped invalid pattern(s):
  RE:*.cpp
Please fix above patterns in ".bzrignore" or "/home/parthm/.bazaar/ignore".
Bazaar commands will NOT ignore files meant to match above pattern(s).
modified:
  .bzrignore
unknown:
  one.txt
  two.txt
  x
  y

=================================================================================
[aaa]% ~/src/bzr.dev/300062-invalid-pattern-warnings/bzr ls -i
.bzrignore.~1~
bzr: warning: Skipped invalid pattern(s):
  RE:*.cpp
Please fix above patterns in ".bzrignore" or "/home/parthm/.bazaar/ignore".
Bazaar commands will NOT ignore files meant to match above pattern(s).
[aaa]% ~/src/bzr.dev/300062-invalid-pattern-warnings/bzr ls -u
bzr: warning: Skipped invalid pattern(s):
  RE:*.cpp
Please fix above patterns in ".bzrignore" or "/home/parthm/.bazaar/ignore".
Bazaar commands will NOT ignore files meant to match above pattern(s).
one.txt
two.txt
x
y

=================================================================================
[aaa]% ~/src/bzr.dev/300062-invalid-pattern-warnings/bzr ignored
.bzrignore.~1~ *~
bzr: warning: Skipped invalid pattern(s):
  RE:*.cpp
Please fix above patterns in ".bzrignore" or "/home/parthm/.bazaar/ignore".
Bazaar commands will NOT ignore files meant to match above pattern(s).

=================================================================================
[aaa]% ~/src/bzr.dev/300062-invalid-pattern-warnings/bzr add
bzr: warning: Skipped invalid pattern(s):
  RE:*.cpp
Please fix above patterns in ".bzrignore" or "/home/parthm/.bazaar/ignore".
Bazaar commands will NOT ignore files meant to match above pattern(s).
adding one.txt
adding two.txt
adding x
adding y

[John A Meinel (jameinel)]

110 + if validp:
111 + g = Globster(validp)
112 + new_regex_patterns.append(g.regex_patterns[0])

^- This seems odd. A comment here saying that we are collapsing the valid requests into a single compiled regex would be useful.

158 +def is_pattern_valid(pattern):

^- It seems like it would be nice if this could share the code that determines what translation unit to use. Otherwise it seems likely to get skew if one of them is updated. For example, the proposed 'FixedString' prefix. The fallout is that it would normally work fine, but if someone had a *different* invalid regex, then suddenly it would start causing this one to fail as well.

I don't understand why you changed variables from being private. In lazy_regex you renamed _real_re_compile to real_re_compile, and in globbing you changed _regex_patterns.

It is perfectly fine for *tests* to check private variables, it is good to keep them private so that plugins get a feeling that they may not be stable names.

...

342 + def test_invalid_pattern_in_ignore_file(self):
343 + """Existing invalid patterns should be shown.
344 + """
345 + tree = self.make_branch_and_tree('.')
346 + self.build_tree(['file', 'foo.c'])
347 + f = open('.bzrignore', 'w')
348 + try:
349 + f.write(u"*.c\nRE:[\n") # invalid pattern
350 + finally:
351 + f.close()

^- I think you can simplify this to:
self.build_tree_contents([('.bzrignore', '*.c\nRE:[\n')])

It is pretty bad form to write *unicode* strings to f.write() given that in python2 it is an 8-bit request. (It happens to be automatically casting your unicode string down to an ascii string.)

This test might be easier to read if we turned it into a "run_script" test, but that may not be easily available here.

The same comments apply to test_add_with_invalid_ignore_in_global_ignore and
test_invalid_pattern_in_ignore_file in the 2 other places.

review: Needs Fixing

[Parth Malwankar (parthm)]

Thanks for the review John.

> 110 + if validp:
> 111 + g = Globster(validp)
> 112 + new_regex_patterns.append(g.regex_patterns[0])
>
> ^- This seems odd. A comment here saying that we are collapsing the valid
> requests into a single compiled regex would be useful.
>

Done.

> 158 +def is_pattern_valid(pattern):
>
> ^- It seems like it would be nice if this could share the code that determines
> what translation unit to use. Otherwise it seems likely to get skew if one of
> them is updated. For example, the proposed 'FixedString' prefix. The fallout
> is that it would normally work fine, but if someone had a *different* invalid
> regex, then suddenly it would start causing this one to fail as well.
>

I have pulled out the common code into a class _Pattern. _Pattern has an
identify method that categorizes the pattern as fullpath, extension or
basename. This also provides pattern->translator and pattern->prefix
mapping as there are required in multiple places. Unit test is also added
for this.

>
> I don't understand why you changed variables from being private. In lazy_regex
> you renamed _real_re_compile to real_re_compile, and in globbing you changed
> _regex_patterns.
>

real_re_compile is imported by bzrlib.globbing as is_pattern_valid uses
real_re_compile to check if a pattern is valid. Hence I made that public.

For regex_patterns, in case of a pre-existing bad pattern Globbing.match
creates an instance of Globbing and accesses regex_patterns directly as
instance.regex_patterns. Considering that this uses is within the same
file we could allow it to be private but I thought making it public
may be cleaner.

Let me know if the above seems reasonable to you. I haven't done any
changes for this yet.

> It is perfectly fine for *tests* to check private variables, it is good to
> keep them private so that plugins get a feeling that they may not be stable
> names.
>
>
> ...
>
> 342 + def test_invalid_pattern_in_ignore_file(self):
> 343 + """Existing invalid patterns should be shown.
> 344 + """
> 345 + tree = self.make_branch_and_tree('.')
> 346 + self.build_tree(['file', 'foo.c'])
> 347 + f = open('.bzrignore', 'w')
> 348 + try:
> 349 + f.write(u"*.c\nRE:[\n") # invalid pattern
> 350 + finally:
> 351 + f.close()
>
> ^- I think you can simplify this to:
> self.build_tree_contents([('.bzrignore', '*.c\nRE:[\n')])
>

Thanks. That very handy. I wasn't aware of this method.
The tests have been updated to use this.

> It is pretty bad form to write *unicode* strings to f.write() given that in
> python2 it is an 8-bit request. (It happens to be automatically casting your
> unicode string down to an ascii string.)
>
> This test might be easier to read if we turned it into a "run_script" test, hha
> but that may not be easily available here.
>
> The same comments apply to test_add_with_invalid_ignore_in_global_ignore and
> test_invalid_pattern_in_ignore_file in the 2 other places.

[Robert Collins (lifeless)]

I have to say, I still think that this would be better solved by just
making the compile error clearer and having it throw an external
BZRError - no expansion of interfaces needed, the improvements can be
used by other things using regexps etc.

-Rob

[Parth Malwankar (parthm)]

Well, we now have both solutions in place. The warnings based (this) and the error based[1] though the latter may need some cleanup and review.

My personal preference is towards an error based approach. I feel warnings are may be ok for operations like 'status' that don't change anything, but specifically for something like 'add' an error would probably be better so that the user doesn't miss the warning message and go ahead and do the commit. We saw The user could 'revert' but it may be a bit of a pain to do that, or worse, user may miss the message and end up with multiple commits after that containing large binary files :) The 'add' operation with warnings specifically reminds me of bug #549310 where whoami was guessing and I ended up with multiple commits with a bad username[2]. I would prefer a uniform error based approach as Robert suggested.
I don't expect the rollout of an error based approach to be a problem as bzr currently crashes on bad patterns so the existing ignore files will not have bad patterns in them.

That said, I think both warning and error based approach are an improvement over the current situation where bzr shows a stack trace which can scare users. It would be great if we can align on one of these after some more discussion and go ahead.

[1] https://code.edge.launchpad.net/~parthm/bzr/300062-better-handling-for-invalid-ignore-pattern/+merge/26400
[2] https://bugs.launchpad.net/bzr/+bug/549310/comments/3

[Robert Collins (lifeless)]

So, lets go with an error based approach for now, for the following reasons:
 - its a very small change compared to the current code (wrap the
regex compiler to know its taking a list in and print the errorneous
pattern out)
 - it seems genuinely better for some use cases like add
 - we can add a warning based one later if this feels insufficient.

In general I do think its better to stop early and cleanly in commands
that can output lots of stuff - if we show a lot then users will often
miss the fine print.

I'll review the error based one again to move things forward.

[Robert Collins (lifeless)]

Marking this one as rejected for now.

[Martin Pool (mbp)]

On 21 June 2010 16:46, Robert Collins <email address hidden> wrote:
> So, lets go with an error based approach for now, for the following reasons:
>  - its a very small change compared to the current code (wrap the
> regex compiler to know its taking a list in and print the errorneous
> pattern out)
>  - it seems genuinely better for some use cases like add
>  - we can add a warning based one later if this feels insufficient.
>
> In general I do think its better to stop early and cleanly in commands
> that can output lots of stuff - if we show a lot then users will often
> miss the fine print.
>
> I'll review the error based one again to move things forward.

I said previously that I slightly preferred a warning if it was
equally easy to implement. But for the sake of simplicity and of
getting an error in cases where we do need it, an error is fine with
me.

--
Martin

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin Pool wrote:
> On 21 June 2010 16:46, Robert Collins <email address hidden> wrote:
>> So, lets go with an error based approach for now, for the following reasons:
>> - its a very small change compared to the current code (wrap the
>> regex compiler to know its taking a list in and print the errorneous
>> pattern out)
>> - it seems genuinely better for some use cases like add
>> - we can add a warning based one later if this feels insufficient.
>>
>> In general I do think its better to stop early and cleanly in commands
>> that can output lots of stuff - if we show a lot then users will often
>> miss the fine print.
>>
>> I'll review the error based one again to move things forward.
>
> I said previously that I slightly preferred a warning if it was
> equally easy to implement. But for the sake of simplicity and of
> getting an error in cases where we do need it, an error is fine with
> me.
>

We've had a bad habit in the past of falling over on trivial details and
preventing users from getting stuff done. I'm not saying that is the
case here, and as always there is a balance to be struck.

As mentioned, this can't be considered a regression, since we used to
raise an unhelpful exception, so raising a helpful one is strictly
better than what we had.

So certainly, land the update (I have not reviewed the code). I'm not as
convinced that failing early is always the best answer, it is probably
ok here.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwg10gACgkQJdeBCYSNAAPBLwCgokPBGQCdKuxiom9uOmK2DKmN
Q0UAoKYen2O25wLOuQRdvqyUVRV42iLn
=Qoef
-----END PGP SIGNATURE-----

Unmerged revisions

5292. By Parth Malwankar on 2010-06-19

added test for _Pattern

5291. By Parth Malwankar on 2010-06-19

cleanup tests.

5290. By Parth Malwankar on 2010-06-19

cleanup of pattern categorization code

5289. By Parth Malwankar on 2010-06-19

merged in changes from trunk

5288. By Parth Malwankar on 2010-06-19

added _PatternType class to categorize patterns.

5287. By Parth Malwankar on 2010-06-04

updated NEWS

5286. By Parth Malwankar on 2010-06-04

merged in changes from trunk.

5285. By Parth Malwankar on 2010-06-04

added invalid patterns tests for 'ignore'

5284. By Parth Malwankar on 2010-06-04

added invalid pattern test for ignored.

5283. By Parth Malwankar on 2010-06-04

added invalid pattern tests for 'add' and 'ls'

=== modified file 'NEWS'
--- NEWS	2010-06-18 11:57:13 +0000
+++ NEWS	2010-06-19 13:29:28 +0000
@@ -56,6 +56,11 @@
   test that all commands available to the test suite have help.
   (Robert Collins, #177500)
 
+* Invalid patterns in ignore files are now displayed as a warning to the
+  user. Bazaar does not throw a stack trace when an invalid pattern is
+  encountered by commands.
+  (Parth Malwankar, #300062)
+
 * Raise ValueError instead of a string exception.
   (John Arbash Meinel, #586926)
 
 
=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-06-17 08:53:15 +0000
+++ bzrlib/builtins.py	2010-06-19 13:29:28 +0000
@@ -2717,6 +2717,16 @@
                 (len(name_pattern) > 1 and name_pattern[1] == ':')):
                 raise errors.BzrCommandError(
                     "NAME_PATTERN should not be an absolute path")
+        name_pattern_list, invalid_patterns = \
+            globbing.filter_invalid_patterns(name_pattern_list)
+        if invalid_patterns:
+            pats = '\n  '.join(invalid_patterns)
+            ui.ui_factory.show_warning(
+                'Skipped invalid pattern argument(s):\n  %s' % pats)
+        if not name_pattern_list:
+            # After removing invalid patterns, the list may be
+            # empty. There is nothing to do in this case.
+            return
         tree, relpath = WorkingTree.open_containing(directory)
         ignores.tree_ignores_add_patterns(tree, name_pattern_list)
         ignored = globbing.Globster(name_pattern_list)
 
=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2010-06-17 11:52:13 +0000
+++ bzrlib/errors.py	2010-06-19 13:29:28 +0000
@@ -3149,10 +3149,10 @@
     def __init__(self, bzrdir):
         self.bzrdir = bzrdir
 
+
 class NoWhoami(BzrError):
 
     _fmt = ('Unable to determine your name.\n'
         "Please, set your name with the 'whoami' command.\n"
         'E.g. bzr whoami "Your Name <[e-mail]>"')
 
-
 
=== modified file 'bzrlib/globbing.py'
--- bzrlib/globbing.py	2010-02-17 17:11:16 +0000
+++ bzrlib/globbing.py	2010-06-19 13:29:28 +0000
@@ -22,6 +22,12 @@
 
 import re
 
+import bzrlib
+from bzrlib import (
+    config,
+    ui,
+    )
+from bzrlib.lazy_regex import real_re_compile
 from bzrlib.trace import (
     warning
     )
@@ -152,6 +158,47 @@
     return _sub_basename(pattern[2:])
 
 
+class _Pattern(object):
+    """Wrapper for managing pattern categories.
+    """
+    TYPE_FULLPATH = 1
+    TYPE_BASENAME = 2
+    TYPE_EXTENSION = 3
+
+    translators = {
+        TYPE_FULLPATH : _sub_fullpath,
+        TYPE_BASENAME : _sub_basename,
+        TYPE_EXTENSION : _sub_extension,
+    }
+
+    # Prefixes used to combine various patterns.
+    # See: Globster._add_patterns
+    PREFIX_EXTENSION = r'(?:.*/)?(?!.*/)(?:.*\.)'
+    PREFIX_BASENAME = r'(?:.*/)?(?!.*/)'
+    PREFIX_FULLPATH = r''
+
+    prefixes = {
+        TYPE_FULLPATH : PREFIX_FULLPATH,
+        TYPE_BASENAME : PREFIX_BASENAME,
+        TYPE_EXTENSION : PREFIX_EXTENSION,
+    }
+
+    @staticmethod
+    def identify(pattern):
+        """Returns pattern category.
+
+        :param pattern: normalized pattern.
+        Identify if a pattern is fullpath, basename or extension
+        and returns the appropriate type.
+        """
+        if pattern.startswith(u'RE:') or u'/' in pattern:
+            return _Pattern.TYPE_FULLPATH
+        elif pattern.startswith(u'*.'):
+            return _Pattern.TYPE_EXTENSION
+        else:
+            return _Pattern.TYPE_BASENAME
+
+
 class Globster(object):
     """A simple wrapper for a set of glob patterns.
 
@@ -164,7 +211,7 @@
     a super-regex containing groups of up to 99 patterns.
     The 99 limitation is due to the grouping limit of the Python re module.
     The resulting super-regex and associated patterns are stored as a list of
-    (regex,[patterns]) in _regex_patterns.
+    (regex,[patterns]) in regex_patterns.
 
     For performance reasons the patterns are categorised as extension patterns
     (those that match against a file extension), basename patterns
@@ -178,46 +225,83 @@
     patterns.
     """
     def __init__(self, patterns):
-        self._regex_patterns = []
+        self.regex_patterns = []
         path_patterns = []
         base_patterns = []
         ext_patterns = []
+        pattern_lists = {
+            _Pattern.TYPE_FULLPATH : path_patterns,
+            _Pattern.TYPE_EXTENSION : ext_patterns,
+            _Pattern.TYPE_BASENAME : base_patterns,
+        }
         for pat in patterns:
             pat = normalize_pattern(pat)
-            if pat.startswith(u'RE:') or u'/' in pat:
-                path_patterns.append(pat)
-            elif pat.startswith(u'*.'):
-                ext_patterns.append(pat)
-            else:
-                base_patterns.append(pat)
-        self._add_patterns(ext_patterns,_sub_extension,
-            prefix=r'(?:.*/)?(?!.*/)(?:.*\.)')
-        self._add_patterns(base_patterns,_sub_basename,
-            prefix=r'(?:.*/)?(?!.*/)')
-        self._add_patterns(path_patterns,_sub_fullpath)
+            pattern_lists[_Pattern.identify(pat)].append(pat)
+        for k, v in pattern_lists.iteritems():
+            self._add_patterns(v, _Pattern.translators[k],
+                _Pattern.prefixes[k])
 
-    def _add_patterns(self, patterns, translator, prefix=''):
+    def _add_patterns(self, patterns, translator, prefix):
         while patterns:
             grouped_rules = ['(%s)' % translator(pat) for pat in patterns[:99]]
             joined_rule = '%s(?:%s)$' % (prefix, '|'.join(grouped_rules))
-            self._regex_patterns.append((re.compile(joined_rule, re.UNICODE),
+            self.regex_patterns.append((re.compile(joined_rule, re.UNICODE),
                 patterns[:99]))
             patterns = patterns[99:]
 
     def match(self, filename):
+        """Search for a pattern that matches the given filename.
+
+        :return A matching pattern or None if there is no matching pattern.
+        In case of a re.error exception,
+          Scan self._regex_patterns.
+          Issue a warning to the user about the bad pattern.
+          Remove bad pattern.
+          Retry match.
+        """
+        try:
+            return self._match(filename)
+        except re.error, e:
+            new_regex_patterns = []
+            for regex, patterns in self.regex_patterns:
+                try:
+                    c = regex._compile_and_collapse()
+                    new_regex_patterns.append((regex, patterns))
+                except re.error, e:
+                    validp, invalidp = filter_invalid_patterns(patterns)
+                    user_ignore = config.user_ignore_config_filename()
+                    bzrignore = bzrlib.IGNORE_FILENAME
+                    msg = ('Skipped invalid pattern(s):\n  %s\n'
+                        'Please fix above patterns in '
+                        '"%s" or "%s".\n'
+                        'Bazaar commands will NOT ignore files meant to '
+                        'match above pattern(s).' %
+                        ('\n  '.join(invalidp), bzrignore, user_ignore))
+                    ui.ui_factory.show_warning(msg)
+                    if validp:
+                        # Skip invalid pattern and accumulate valid patterns
+                        # into new_regex_patterns so that they can be compiled
+                        # into a single regex for better performance.
+                        g = Globster(validp)
+                        new_regex_patterns.append(g.regex_patterns[0])
+            self.regex_patterns = new_regex_patterns
+            return self._match(filename)
+
+    def _match(self, filename):
         """Searches for a pattern that matches the given filename.
 
         :return A matching pattern or None if there is no matching pattern.
         """
-        for regex, patterns in self._regex_patterns:
+        for regex, patterns in self.regex_patterns:
             match = regex.match(filename)
             if match:
                 return patterns[match.lastindex -1]
         return None
 
+
 class ExceptionGlobster(object):
     """A Globster that supports exception patterns.
-    
+
     Exceptions are ignore patterns prefixed with '!'.  Exception
     patterns take precedence over regular patterns and cause a 
     matching filename to return None from the match() function.  
@@ -225,7 +309,7 @@
     as regular ignores. '!!' patterns are useful to establish ignores
     that apply under paths specified by '!' exception patterns.
     """
-    
+
     def __init__(self,patterns):
         ignores = [[], [], []]
         for p in patterns:
@@ -236,7 +320,7 @@
             else:
                 ignores[0].append(p)
         self._ignores = [Globster(i) for i in ignores]
-        
+
     def match(self, filename):
         """Searches for a pattern that matches the given filename.
 
@@ -250,6 +334,7 @@
         else:
             return self._ignores[0].match(filename)
 
+
 class _OrderedGlobster(Globster):
     """A Globster that keeps pattern order."""
 
@@ -259,17 +344,12 @@
         :param patterns: sequence of glob patterns
         """
         # Note: This could be smarter by running like sequences together
-        self._regex_patterns = []
+        self.regex_patterns = []
         for pat in patterns:
             pat = normalize_pattern(pat)
-            if pat.startswith(u'RE:') or u'/' in pat:
-                self._add_patterns([pat], _sub_fullpath)
-            elif pat.startswith(u'*.'):
-                self._add_patterns([pat], _sub_extension,
-                    prefix=r'(?:.*/)?(?!.*/)(?:.*\.)')
-            else:
-                self._add_patterns([pat], _sub_basename,
-                    prefix=r'(?:.*/)?(?!.*/)')
+            pat_type = _Pattern.identify(pat)
+            self._add_patterns([pat], _Pattern.translators[pat_type],
+                _Pattern.prefixes[pat_type])
 
 
 _slashes = re.compile(r'[\\/]+')
@@ -283,3 +363,38 @@
     if len(pattern) > 1:
         pattern = pattern.rstrip('/')
     return pattern
+
+
+def is_pattern_valid(pattern):
+    """Returns True if pattern is valid.
+
+    :param pattern: Normalized pattern.
+    is_pattern_valid() assumes pattern to be normalized.
+    see: globbing.normalize_pattern
+    """
+    result = True
+    translator = _Pattern.translators[_Pattern.identify(pattern)]
+    tpattern = '(%s)' % translator(pattern)
+    try:
+        cpattern = real_re_compile(tpattern, re.UNICODE)
+    except re.error, e:
+        result = False
+    return result
+
+
+def filter_invalid_patterns(patterns):
+    """Returns valid and invalid patterns.
+
+    :param patterns: List of normalized patterns.
+    filter_invalid_patterns() assumes pattern to be normalized.
+    see: globbing.normalize_pattern
+    """
+    valid_patterns = []
+    invalid_patterns = []
+    for pat in patterns:
+        if is_pattern_valid(pat):
+            valid_patterns.append(pat)
+        else:
+            invalid_patterns.append(pat)
+    return valid_patterns, invalid_patterns
+
 
=== modified file 'bzrlib/lazy_regex.py'
--- bzrlib/lazy_regex.py	2009-03-23 14:59:43 +0000
+++ bzrlib/lazy_regex.py	2010-06-19 13:29:28 +0000
@@ -58,7 +58,7 @@
 
     def _real_re_compile(self, *args, **kwargs):
         """Thunk over to the original re.compile"""
-        return _real_re_compile(*args, **kwargs)
+        return real_re_compile(*args, **kwargs)
 
     def __getattr__(self, attr):
         """Return a member from the proxied regex object.
@@ -97,11 +97,11 @@
     Though the first call will reset back to the original (it doesn't
     track nesting level)
     """
-    re.compile = _real_re_compile
-
-
-_real_re_compile = re.compile
-if _real_re_compile is lazy_compile:
+    re.compile = real_re_compile
+
+
+real_re_compile = re.compile
+if real_re_compile is lazy_compile:
     raise AssertionError(
         "re.compile has already been overridden as lazy_compile, but this would" \
         " cause infinite recursion")
 
=== modified file 'bzrlib/tests/blackbox/test_add.py'
--- bzrlib/tests/blackbox/test_add.py	2010-02-23 07:43:11 +0000
+++ bzrlib/tests/blackbox/test_add.py	2010-06-19 13:29:28 +0000
@@ -18,8 +18,10 @@
 """Tests of the 'bzr add' command."""
 
 import os
+import re
 
 from bzrlib import (
+    ignores,
     osutils,
     tests,
     )
@@ -217,3 +219,29 @@
         os.symlink(osutils.abspath('target'), 'tree/link')
         out = self.run_bzr(['add', 'tree/link'])[0]
         self.assertEquals(out, 'adding link\n')
+
+    def test_add_with_invalid_ignore(self):
+        """add should handle invalid ignore pattern gracefully.
+        """
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['top.txt'])
+        self.build_tree_contents([('.bzrignore', 'RE:*.cpp\n')])
+        out, err = self.run_bzr(['add'])
+        self.assertEquals(out, 'adding .bzrignore\nadding top.txt\n')
+        self.assertContainsRe(err,
+            'warning: Skipped invalid pattern.*RE:\*\.cpp',
+            re.DOTALL)
+
+    def test_add_with_invalid_ignore_in_global_ignore(self):
+        """add should handle invalid global ignore pattern gracefully.
+        """
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['top.txt'])
+        self.build_tree_contents([('foobar', 'foo\n')])
+        ignores.add_unique_user_ignores([u"RE:*.cpp"]) # invalid pattern
+        out, err = self.run_bzr(['add'])
+        self.assertEquals(out, 'adding foobar\nadding top.txt\n')
+        self.assertContainsRe(err,
+            'warning: Skipped invalid pattern.*RE:\*\.cpp',
+            re.DOTALL)
+
 
=== modified file 'bzrlib/tests/blackbox/test_ignore.py'
--- bzrlib/tests/blackbox/test_ignore.py	2010-05-03 09:19:15 +0000
+++ bzrlib/tests/blackbox/test_ignore.py	2010-06-19 13:29:28 +0000
@@ -162,3 +162,35 @@
         tree = self.make_branch_and_tree('a')
         self.run_bzr(['ignore', '--directory=a', 'README'])
         self.check_file_contents('a/.bzrignore', 'README\n')
+
+    def test_ignore_invalid_pattern_argument(self):
+        """Ensure graceful handling for invalid ignore pattern.
+
+        Invalid pattern should show clear error message.
+        Invalid pattern should not be added to .bzrignore file.
+        """
+        tree = self.make_branch_and_tree('.')
+        out, err = self.run_bzr(['ignore', 'RE:*.cpp']) # invalid pattern
+        self.assertEqual(out, '')
+        self.assertContainsRe(err,
+            'bzr: warning: Skipped invalid.*RE:\*\.cpp', re.DOTALL)
+        self.assertFalse(os.path.isfile('.bzrignore'))
+
+    def test_invalid_pattern_in_ignore_file(self):
+        """Existing invalid patterns should be shown.
+        """
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['file'])
+        self.build_tree_contents([('.bzrignore', '*.c\nRE:[\n')])
+        out, err = self.run_bzr(['ignore', 'RE:*.cpp', 'abc'])
+        self.assertEqual(out, '')
+        self.assertContainsRe(err, 'warning: Skipped invalid pattern'
+            '.*RE:\*\.cpp.*'
+            'warning: Skipped invalid pattern.*RE:\[', re.DOTALL)
+        f = open('.bzrignore', 'r')
+        try:
+            lines = f.readlines()
+            self.assertTrue('abc\n' in lines)
+        finally:
+            f.close()
+
 
=== modified file 'bzrlib/tests/blackbox/test_ignored.py'
--- bzrlib/tests/blackbox/test_ignored.py	2010-05-02 20:10:25 +0000
+++ bzrlib/tests/blackbox/test_ignored.py	2010-06-19 13:29:28 +0000
@@ -17,6 +17,8 @@
 
 """Tests of the 'bzr ignored' command."""
 
+import re
+
 from bzrlib.tests.blackbox import ExternalBase
 
 
@@ -46,3 +48,16 @@
                                   ('a/.bzrignore', 'README')])
         out, err = self.run_bzr(['ignored', '--directory=a'])
         self.assertStartsWith(out, 'README')
+
+    def test_invalid_pattern_in_ignore_file(self):
+        """Existing invalid patterns should be shown.
+        """
+        tree = self.make_branch_and_tree('.')
+        self.build_tree(['file', 'foo.c'])
+        self.build_tree_contents([('.bzrignore', '*.c\nRE:[\n')])
+        out, err = self.run_bzr(['ignored'])
+        self.assertContainsRe(out, 'foo\.c[ ]+\*\.c')
+        self.assertContainsRe(err,
+            'warning: Skipped invalid pattern.*RE:\[',
+            re.DOTALL)
+
 
=== modified file 'bzrlib/tests/blackbox/test_ls.py'
--- bzrlib/tests/blackbox/test_ls.py	2010-05-02 20:10:25 +0000
+++ bzrlib/tests/blackbox/test_ls.py	2010-06-19 13:29:28 +0000
@@ -16,6 +16,7 @@
 
 """External tests of 'bzr ls'"""
 
+import re
 import os
 
 from bzrlib import ignores, osutils
@@ -244,3 +245,25 @@
         self.wt.commit('commit')
         self.ls_equals('sub/\nsub/file\n', '--directory=dir')
         self.ls_equals('sub/file\n', '-d dir sub')
+
+    def test_ls_invalid_ignore_pattern(self):
+        """Tests status with invalid ignore pattern"""
+        f = open('.bzrignore', 'w')
+        try:
+            f.write(u"RE:*.cpp\n") # invalid pattern
+        finally:
+            f.close()
+        self.build_tree(['bye.c'])
+        out, err = self.run_bzr(['ls', '-i'])
+        self.assertEquals(out, '')
+        self.assertContainsRe(err,
+            'warning: Skipped invalid pattern.*RE:\*\.cpp',
+            flags=re.DOTALL)
+        out, err = self.run_bzr(['ls', '-u'])
+        self.assertEquals(out, '.bzrignore\na\nbye.c\n')
+        self.assertContainsRe(err,
+            'warning: Skipped invalid pattern.*RE:\*\.cpp',
+            flags=re.DOTALL)
+
+
+
 
=== modified file 'bzrlib/tests/blackbox/test_status.py'
--- bzrlib/tests/blackbox/test_status.py	2010-04-07 21:34:13 +0000
+++ bzrlib/tests/blackbox/test_status.py	2010-06-19 13:29:28 +0000
@@ -25,6 +25,7 @@
 from cStringIO import StringIO
 import codecs
 from os import mkdir, chdir, rmdir, unlink
+import re
 import sys
 from tempfile import TemporaryFile
 
@@ -32,6 +33,7 @@
     bzrdir,
     conflicts,
     errors,
+    ignores,
     osutils,
     )
 import bzrlib.branch
@@ -701,6 +703,73 @@
         out, err = self.run_bzr('status tree/a')
         self.assertNotContainsRe(out, 'pending merge')
 
+    def test_status_invalid_ignore_pattern(self):
+        """Tests status with invalid ignore pattern"""
+        wt = self.make_branch_and_tree('.')
+        f = open('.bzrignore', 'w')
+        try:
+            f.write(u"RE:*.cpp\n") # invalid pattern
+        finally:
+            f.close()
+        wt.commit('commit .bzrignore')
+        self.build_tree(['bye.c'])
+        out, err = self.run_bzr(['status'])
+        self.assertEquals(out, 'unknown:\n  .bzrignore\n  bye.c\n')
+        self.assertContainsRe(err,
+            'warning: Skipped invalid pattern.*RE:\*\.cpp',
+            flags=re.DOTALL)
+
+    def test_status_invalid_ignore_pattern_in_global_ignore(self):
+        """Tests status with invalid ignore pattern in .bazaar/ignore"""
+        wt = self.make_branch_and_tree('.')
+        self.build_tree(['file'])
+        wt.add('file')
+        wt.commit('one')
+        # add unknown file
+        f = open('foobar', 'w')
+        try:
+            f.write(u"foo\n") # invalid pattern
+        finally:
+            f.close()
+        ignores.add_unique_user_ignores([u"RE:*.cpp"]) # invalid pattern
+        out, err = self.run_bzr(['status'])
+        self.assertEquals(out, 'unknown:\n  foobar\n')
+        self.assertContainsRe(err,
+            'warning: Skipped invalid.*RE:\*\.cpp',
+            flags=re.DOTALL)
+
+    def test_short_status_invalid_ignore_pattern(self):
+        """Tests short status with invalid ignore pattern"""
+        wt = self.make_branch_and_tree('.')
+        f = open('.bzrignore', 'w')
+        try:
+            f.write(u"RE:*.cpp\n") # invalid pattern
+        finally:
+            f.close()
+        wt.commit('commit .bzrignore')
+        out, err = self.run_bzr(['status', '-S'])
+        self.assertEquals(out, '?   .bzrignore\n')
+        self.assertContainsRe(err,
+            'warning: Skipped invalid.*RE:\*\.cpp',
+            flags=re.DOTALL)
+
+    def test_specific_status_invalid_ignore_pattern(self):
+        """Tests specific status with invalid ignore pattern"""
+        wt = self.make_branch_and_tree('.')
+        self.build_tree(['file'])
+        wt.add('file')
+        wt.commit('added file')
+        f = open('.bzrignore', 'w')
+        try:
+            f.write(u"RE:*.cpp\n") # invalid pattern
+        finally:
+            f.close()
+        wt.commit('commit .bzrignore')
+        out, err = self.run_bzr(['status', 'file'])
+        self.assertEquals(out, '')
+        self.assertContainsRe(err,
+            'warning: Skipped invalid.*RE:\*\.cpp',
+            flags=re.DOTALL)
 
 class TestStatusEncodings(TestCaseWithTransport):
 
 
=== modified file 'bzrlib/tests/test_globbing.py'
--- bzrlib/tests/test_globbing.py	2010-02-17 17:11:16 +0000
+++ bzrlib/tests/test_globbing.py	2010-06-19 13:29:28 +0000
@@ -16,10 +16,13 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
 from bzrlib.globbing import (
+    ExceptionGlobster,
+    filter_invalid_patterns,
     Globster,
-    ExceptionGlobster,
+    is_pattern_valid,
+    normalize_pattern,
     _OrderedGlobster,
-    normalize_pattern
+    _Pattern,
     )
 from bzrlib.tests import (
     TestCase,
@@ -37,11 +40,11 @@
             for name in positive:
                 self.failUnless(globster.match(name), repr(
                     u'name "%s" does not match glob "%s" (re=%s)' %
-                    (name, glob, globster._regex_patterns[0][0].pattern)))
+                    (name, glob, globster.regex_patterns[0][0].pattern)))
             for name in negative:
                 self.failIf(globster.match(name), repr(
                     u'name "%s" does match glob "%s" (re=%s)' %
-                    (name, glob, globster._regex_patterns[0][0].pattern)))
+                    (name, glob, globster.regex_patterns[0][0].pattern)))
 
     def assertMatchBasenameAndFullpath(self, matchset):
         # test basename matcher
@@ -371,3 +374,37 @@
         """tests that multiple mixed slashes are collapsed to single forward
         slashes and trailing mixed slashes are removed"""
         self.assertEqual(u'/foo/bar', normalize_pattern(u'\\/\\foo//\\///bar/\\\\/'))
+
+class TestInvalidPattern(TestCase):
+
+    def test_is_pattern_valid(self):
+        """Ensure is_pattern_valid() detects invalid patterns.
+        """
+        self.assertTrue(is_pattern_valid(normalize_pattern(u'foo')))
+        self.assertFalse(is_pattern_valid(normalize_pattern(u'RE:*.cpp')))
+
+    def test_filter_invalid_patterns(self):
+        """Ensure filter_invalid_patterns() returns only valid patterns.
+        """
+        patterns = [u'abc', u'RE:*.cpp', u'[', u'def']
+        patterns = [normalize_pattern(p) for p in patterns]
+        ref_valid_patterns = set([u'abc', u'def'])
+        ref_invalid_patterns = set([u'RE:*.cpp', u'['])
+        valid_patterns, invalid_patterns = filter_invalid_patterns(patterns)
+        self.assertEqual(ref_valid_patterns, set(valid_patterns))
+        self.assertEqual(ref_invalid_patterns, set(invalid_patterns))
+
+class TestPatternClass(TestCase):
+
+    def test_pattern_category(self):
+        """Ensure pattern categories are identified correctly.
+        """
+        fullpath0 = normalize_pattern(u'/home/jrandom')
+        fullpath1 = normalize_pattern(u'RE:foobar')
+        extension = normalize_pattern(u'*.py')
+        basename = normalize_pattern(u'random_file')
+        self.assertEqual(_Pattern.identify(fullpath0), _Pattern.TYPE_FULLPATH)
+        self.assertEqual(_Pattern.identify(fullpath1), _Pattern.TYPE_FULLPATH)
+        self.assertEqual(_Pattern.identify(extension), _Pattern.TYPE_EXTENSION)
+        self.assertEqual(_Pattern.identify(basename), _Pattern.TYPE_BASENAME)
+
 
=== modified file 'bzrlib/tests/test_ignores.py'
--- bzrlib/tests/test_ignores.py	2010-05-03 04:51:58 +0000
+++ bzrlib/tests/test_ignores.py	2010-06-19 13:29:28 +0000
@@ -217,3 +217,13 @@
         ignores.tree_ignores_add_patterns(tree, ["myentry2", "foo"])
         self.assertEquals(open('.bzrignore', 'rb').read(), 'myentry1\r\nmyentry2\r\nfoo\r\n')
         self.assertPatternsEquals(["myentry1", "myentry2", "foo"])
+
+    def test_bad_pattern(self):
+        """tree.is_ignored() should handle bad patterns.
+        """
+        tree = self.make_branch_and_tree(".")
+        self.build_tree(['foo.c'])
+        self.build_tree_contents([('.bzrignore', u"RE:*.cpp\nfoo.py\n")])
+        tree.add([".bzrignore"])
+        self.assertFalse(tree.is_ignored('foo.c'))
+