Merge lp:~parthm/bzr/300062-better-handling-for-invalid-ignore-pattern into lp:bzr

[Parth Malwankar]

Improved handling of invalid ignore patterns by add, ignore, ignored and status.

=== Fixes Bug #300062 ===

This started out as a simple bug fix to ensure that 'bzr ignore' handles invalid patterns correctly but as invalid patterns impacted other commands this patch ended up fixing those as well. bzr can encounter invalid patterns in either .bzrignore file or .bazaar/ignore file. These could earlier get in either by using 'bzr ignore' or by editing the files directly. The former is now fixed. This patch fixes the following:
1. 'bzr ignore' when supplied with possibly multiple patterns, filters out invalid patters and only adds valid patterns to .bzrignore

2. 'bzr status' checks ignore patterns before showing status. This was crashing if invalid patterns were present. 'bzr status' now issues a warning if invalid pattern(s) are seen and does not crash. In order to avoid any performance penalty, `status` works as before, but if it sees sees re.error then it parser both ignore files and prints out the bad pattern(s).

3. 'bzr add' checks ignore patterns before adding files. This was crashing if invalid patterns were present. 'bzr add' now exists with an error message indicating invalid patterns. Initially I was trying to decide between a warning or error for this but decided that its better for `add` to not be too smart as it might end up adding files that were meant to be ignored.

Implementations Notes
=====================
As `re` is lazy by default. I have added a function `bzrlib.lazy_regex.eager_re_compile` that is used by `bzrlib.globbing.is_pattern_valid` which in turn is used by to create `bzrlib.globbing.filter_invalid_patterns`. This is used by `ignore` command to ensure that no junk enters .bzrignore.

`bzrlib.ignores` has two new functions, `get_pattern_errors` gives information about bad patterns in .bzrignore and .bazaar/ignore. `show_pattern_errors` is used to display warning about bad patterns to the user. `show_pattern_errors` is used by `status`. `get_pattern_errors` is used by `add` which raises error `InvalidIgnorePattern`.

I noticed that imports in bzrlib.status and bzrlib.ignores were not lazy. So I have made these lazy and I had to add a few imports to these files.

Tests have been added as needed.

[Robert Collins (lifeless)]

So, this is comprehensive, and I like that. I have a general point,
and a couple of specifics. I haven't done a line by line reading [yet,
 I will after the general point is discussed].

Firstly, the general point. I think its fine, if the ignore file is
damaged to just stop. add doesn't need to be able to succeed when the
environment is broken. Stop (let the exception propogate) and have the
user fix the issue. Better that than adding something they wanted
ignored, I think. Similarly for status.

Now, I might be misreading, and thats what you're doing : but if thats
what you're doing, its leaking up into code that it shouldn't. So I'd
like to see this patch with no changes to code that uses ignores - no
changes should be needed to do what I've described. This is because I
don't think the approach of scattering try/excepts around in higher
level code makes sense: push it down into the ignores module.

Secondly, the specific issue - to expose the real re compile function,
just assign it to a variable that can be accessed: wrappers of the
form:
def a_foo(param):
   result = real_foo(param)
   return result

can be rewritten as
def a_foo(param):
    return real_foo(param)

which is precisely equivalent to
a_foo = real_foo

So in this case, just call the _real_re_compile function, as is. No
changes needed.

sidenote: I rather suspect theres a bunch of edits here we may end up
not wanting, and perhaps we'll want to flatten the history to avoid
that overhead.

-Rob

[Robert Collins (lifeless)]

Oh, and one more thing: the tests you're writing are very far away
from the code you're testing; please consider using tests for the
specific code, not blackbox tests. blackbox tests should really only
be needed when you're changing code in builtins.py or the few other
command-class containing files.

-Rob

[Parth Malwankar (parthm)]

> So, this is comprehensive, and I like that. I have a general point,
> and a couple of specifics. I haven't done a line by line reading [yet,
> I will after the general point is discussed].
>
> Firstly, the general point. I think its fine, if the ignore file is
> damaged to just stop. add doesn't need to be able to succeed when the
> environment is broken. Stop (let the exception propogate) and have the
> user fix the issue. Better that than adding something they wanted
> ignored, I think. Similarly for status.
>

I have update `status` to fail as well. `add` was already raising an
exception for invalid pattern. Making it consistent does make the
code cleaner.

> Secondly, the specific issue - to expose the real re compile function,
> just assign it to a variable that can be accessed: wrappers of the
> form:
> def a_foo(param):
> result = real_foo(param)
> return result
>
> can be rewritten as
> def a_foo(param):
> return real_foo(param)
>
> which is precisely equivalent to
> a_foo = real_foo
>
>
> So in this case, just call the _real_re_compile function, as is. No
> changes needed.
>

I was reluctant to use _real_re_compile as it was private. Its now
renamed to real_re_compile and used directly.

> sidenote: I rather suspect theres a bunch of edits here we may end up
> not wanting, and perhaps we'll want to flatten the history to avoid
> that overhead.
>

I am not sure I understand. Do you mean using bzr-rewrite?

> -Rob

Thanks for the review.

[Parth Malwankar (parthm)]

> Oh, and one more thing: the tests you're writing are very far away
> from the code you're testing; please consider using tests for the
> specific code, not blackbox tests. blackbox tests should really only
> be needed when you're changing code in builtins.py or the few other
> command-class containing files.
>

Done. I have added unit tests for the new functions along with the
blackbox tests.

[Parth Malwankar (parthm)]

Some other things are fixed in the latest set of changes:
 - `ignored` was crashing on invalid pattern.
 - lock/unlock for ignore wasn't in try/finally or cleanup.
 - As InvalidPattern exception is thrown from the lowest level, any uncaught regex error in bzr will give a InvalidPattern bzr error rather than stack trace. `add`, `ignore`, `ignored`, `status` catch this to give bad ignore pattern info.

[Robert Collins (lifeless)]

Why do they need to catch it? Why can't the exception __str__ just
show it, avoiding all changes to add/ignore/ignored and status
commands?

-Rob

[John A Meinel (jameinel)]

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

Robert Collins wrote:
> Why do they need to catch it? Why can't the exception __str__ just
> show it, avoiding all changes to add/ignore/ignored and status
> commands?
>
> -Rob

Just mentioning that I don't think falling over and failing on an error
is the best route. When people want to run "bzr status" they want it to
complete. Completely blocking the user because they have a typo in their
ignore file is not going to win "user-friendly" points.

Obviously Robert and I disagree here, though, so we may need someone
else to chime in as a tiebreaker.

John
=:->

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

iEYEARECAAYFAkwFHfQACgkQJdeBCYSNAAM68QCgkT7TycEQ5NeRcs1rXgfRklrl
Bq8AoKfmmTzMfK+IbGrwaQ41kEuJWWvf
=n8/E
-----END PGP SIGNATURE-----

[Parth Malwankar (parthm)]

> Why do they need to catch it? Why can't the exception __str__ just
> show it, avoiding all changes to add/ignore/ignored and status
> commands?
>
> -Rob

regex are used in other places like `log -m`. So, a failure
doesn't necessarily mean its an ignore pattern issue. Hence,
the caller needs to decide whats the best way to deal with
it. Also, in some cases we want to show a warning (ignore)
and in others, an error (add).

[Parth Malwankar (parthm)]

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Robert Collins wrote:
> > Why do they need to catch it? Why can't the exception __str__ just
> > show it, avoiding all changes to add/ignore/ignored and status
> > commands?
> >
> > -Rob
>
> Just mentioning that I don't think falling over and failing on an error
> is the best route. When people want to run "bzr status" they want it to
> complete. Completely blocking the user because they have a typo in their
> ignore file is not going to win "user-friendly" points.
>

Having `status` work seems reasonable to me because in case of failed
ignores the files displayed end up as 'unknown' and the rest of the
status should be the same. The code does get a little ugly as we need
to catch the exception in three places in status.py.

[Robert Collins (lifeless)]

John, lets explore this a little more.

Possibly the most common case of a broken ignore file will be new
users, learning how to use the system, and running 'bzr st' on a newly
inited tree, showing hundreds or thousands of files.

I don't think a little warning message shown partway through the
output will win user friendly points - and they may well be scratching
their head going 'why won't it ignore what I asked it to': people
learning a new system can often get rattled.

I don't think stopping cold is necessarily better than outputting
details about whats wrong, but I am not at all convinced that
completing-with-potentially-very-many-unknowns is better.

I'm concerned about having more try:excepts in the very innermost loop
of what we do, we've had to take them out of code paths before, and
status is one of the most heavily exercised paths. What I saw of the
earlier diff had try: excepts: inside inner loops, rather than once,
at compilation of the regex time. Data will help (test on a 20K file
tree, with 40K ignored files (.o's and .d's)

Also, as we only compile *once* there is a clear bug - if the
categorisation fails on the first file, and the latter ones just fail
open to unknowns, then first file *also* should fail open to an
unknown, and there is no reason to be raising an exception - just log
directly from the compilation failure point.

[Martin Pool (mbp)]

On 2 June 2010 00:52, John A Meinel <email address hidden> wrote:
> Just mentioning that I don't think falling over and failing on an error
> is the best route. When people want to run "bzr status" they want it to
> complete. Completely blocking the user because they have a typo in their
> ignore file is not going to win "user-friendly" points.

I would tend to expect it to give a user warning not an error. If
it's unreasonably messy or slow to implement I wouldn't absolutely
insist on it. A user friendly error is an improvement on an internal
error.

--
Martin <http://launchpad.net/~mbp/>

[Robert Collins (lifeless)]

To be clear, I was suggesting a user user - nicely formatted, and stop
the operation, not a traceback or InternalError.

[Parth Malwankar (parthm)]

This is how the output currently looks (with status showing an error):

[aaa]% ~/src/bzr.dev/300062-better-handling-for-invalid-ignore-pattern/bzr ignore "RE:[" aaa
bzr: warning: Skipped invalid pattern(s):
  RE:[
========================================================================
[aaa]% echo "RE:[" >> .bzrignore
[aaa]% ~/src/bzr.dev/300062-better-handling-for-invalid-ignore-pattern/bzr st
bzr: error: Invalid pattern(s) found in "/home/parthm/tmp/aaa/.bzrignore":
  RE:[
bzr: ERROR: Invalid pattern(s) found.
========================================================================
[aaa]% ~/src/bzr.dev/300062-better-handling-for-invalid-ignore-pattern/bzr add
bzr: error: Invalid pattern(s) found in "/home/parthm/tmp/aaa/.bzrignore":
  RE:[
bzr: ERROR: Invalid pattern(s) found.
========================================================================
[aaa]% echo "RE:*.cpp" >> ~/.bazaar/ignore
[aaa]% ~/src/bzr.dev/300062-better-handling-for-invalid-ignore-pattern/bzr add
bzr: error: Invalid pattern(s) found in "/home/parthm/tmp/aaa/.bzrignore":
  RE:[
bzr: error: Invalid pattern(s) found in "/home/parthm/.bazaar/ignore":
  RE:*.cpp
bzr: ERROR: Invalid pattern(s) found.
[aaa]%

[Parth Malwankar (parthm)]

I had an interesting discussion with jam regarding warning or error on irc. Unfortunately no conclusion yet :P

<jam> hi parthm
<parthm> jam: hi, i was thinking some more about the `bzr st` bad ignore pattern
i am somewhat divided between warning / error but am leaning towards error. if we have a bad pattern other commands like `add` and `ignored` would fail anyway.
<parthm> i am not sure if there are others. so it may be better to fail quickly and have the user fix it sooner than later.
<parthm> there could also be a case that a user ends up pushing a bad pattern to their server/lp. as .bzrignore is already added. its just a `bzr ci` and `bzr push`.
so failing on `st` may be a good idea.
<jam> parthm: why would add and ignore have to fail?
the point is that, especially right now, people may have a bad config
having it fall over when it used to work is bad
having it fall over *in general* is bad
I don't feel super strongly
but we've done the 'error early' in the past
and generally it has provided worse user experience
as a minor misconfiguration in X
suddenly blocks Y and Z from working at all
<parthm> jam: `ignore` works, it issues a warning and discards the bad pattern.
<jam> even if the pattern was already in the file?
<parthm> `ignored` fails as we compile all ignored patterns (99 at a time) into one pattern. so we cant selectively ignore.
<jam> you certainly *could*
<parthm> jam: yes. if a file has a bad pattern, `ignore` would still work. so it issues a warning about the pattern on the command line. if the tree is processed, then a followup error is shown about the existing bad pattern.
<jam> parthm: the problem is that if we filter out the bad pattern from the disk content we make it harder to get the right pattern
since the old one is gone
it may be a fair trade
but something to be thought about
<parthm> jam: we don't filter out the bad pattern from the file. filtering is only for the command like. we display an error message for the pattern in file.
jam: http://pastebin.com/fMWtEUjc
<jam> so for compiling, we could certainly attempt the 100-way compile and if it fails rollback
<parthm> jam: i am not sure i understand. are you talking about `ignored`?
<jam> yes
but also statu
status
add, etc
<parthm> jam: what do you mean by try the compile and rollback?
<jam> parthm: try to compile all 100, if it fails, compile them individually to find the one that failed
<parthm> jam: thats whats done in the current patch. the current patch allow the operation to happen, if there is an exception, it processes the file to find the bad pattern and displays it along with the error message.
<jam> so why does ignored *have* to fail?
<parthm> jam: so are you suggesting we retry after filtering out the bad pattern at runtime?
<jam> That is what I've been suggesting, yes
<parthm> jam: IMO once the user has a bad pattern in the file he would sooner or later end up with a problem so it may be better to just error out pointing to the pattern.
i don't think existing installation would have a bad pattern as it would make bzr crash.
<jam> parthm: the difference is whether someone can still finish what they were trying to do
*before* they have to fix everything
o...

[Vincent Ladeuil (vila)]

+1 for warning, blocking a user is bad, especially if .bzrignore is
versioned and the said user just got one with a bad pattern (some people
will do that)

[Parth Malwankar (parthm)]

It seem to me that the majority prefers bzr to be more forgiving about bad patterns. I can go ahead with a warning based approach if there are no objections to this over the next day or two.

Here is what I understand.

1. `add` - This should fail (error) as we don't wan't to add file that the user may want to ignore
2. `ignore` - Show warning and skip any bad patterns found on the command line.
3. `ignored` - Try to show ignored files. If this errors out with bad regex, filter out the faulting regex(es) at runtime and retry. The disk files are not changes. Issue a warning about faulting regexes.
4. `status` - If we see a regex failure, filter out the faulting pattern at runtime and retry the ignored files. The retry based approach should reduce the number of 'unknown' files listed.

This would also impact `ls -i/-u`. I will look into that some more. I suppose we would want a similar retry based behavior for this also.

Any further thoughts/ideas about the user interaction should be? Any more commands that might be impacted?

[John A Meinel (jameinel)]

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

Parth Malwankar wrote:
> It seem to me that the majority prefers bzr to be more forgiving about bad patterns. I can go ahead with a warning based approach if there are no objections to this over the next day or two.
>
> Here is what I understand.
>
> 1. `add` - This should fail (error) as we don't wan't to add file that the user may want to ignore

I would actually disagree and just warn. The user can always 'bzr
revert' or 'bzr rm'. Especially if they were supplying explicit paths
("bzr add foo" shouldn't care about ignore patterns, because it always
adds foo even if it was ignored.)

> 2. `ignore` - Show warning and skip any bad patterns found on the command line.
> 3. `ignored` - Try to show ignored files. If this errors out with bad regex, filter out the faulting regex(es) at runtime and retry. The disk files are not changes. Issue a warning about faulting regexes.
> 4. `status` - If we see a regex failure, filter out the faulting pattern at runtime and retry the ignored files. The retry based approach should reduce the number of 'unknown' files listed.
>
> This would also impact `ls -i/-u`. I will look into that some more. I suppose we would want a similar retry based behavior for this also.
>
> Any further thoughts/ideas about the user interaction should be? Any more commands that might be impacted?
>

I would look to do it the 'cleanest' way internally. My feeling is that
turning everything into a warning is likely to be fairly straightforward
and the most worthwhile.

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

iEYEARECAAYFAkwH7UQACgkQJdeBCYSNAAN3hACgvtuI/Jb1yJ8pJQPsGxwfG0f0
eF8AoI+7+E1fKzOZZ4kq3phkxCJmTEXL
=fiKw
-----END PGP SIGNATURE-----

[Robert Collins (lifeless)]

I'd like to see a really clean internal behaviour + a good user experience.

I don't agree with John that the user may have a bad ignore file
already: compilation will be breaking, and it will be throwing the
native RE error at the moment, so existing, broken files are fine to
error on. The point is to error:
 - cleanly
 - helpfully
 - in a way they can correct

(and correcting a .bzrignore file is easy)

I have a strong feeling that, particularly for add and status in newly
created trees, erroring is actually more user friendly that warning:
warnings can get lost and ignored, particularly when you're dealing
with hundreds or thousands of lines of output. However, I think any
improvement is better than what we have today, so we should just do
whatever Parth wants to do here, and iterate when we find out its
wrong ;).

In terms of internal code, I'd like it if whatever approach is taken:
 - does not need handling code per-code-path-using-ignores
 - is entirely encapsulated in ignores.py
 - doesn't add a new exception handler to every unversioned file
considered by add

This is totally doable, I think, for either warnings or erroring, and
not something I expect any of us to disagree on:)

-Rob

[Robert Collins (lifeless)]

Ok, I've looked through this again, and I'd like to suggest a deeper implementation strategy.

From the top level, whats going on here is this:
 - working trees have some user data (ignore rules)
 - we compile the user rules into an intermediary language (a list of regexes which we | together)
 - we lazy-compile that intermediary form into a list of re's we can match things against.

So there are the following translation steps that can fail:
 - ignores line -> string regex
 - string regex -> compiled regex

Constraint wise we don't want to compile re's more than once in normal circumstances.

We don't want to expose internal workings unless needed.

We want all commands that use ignores to benefit.

If you look at ExceptionGlobster and Globster, these objects already know about all the patterns, and they are responsible for the glob->re compilation too. As you have made their match raise InvalidPattern error, its actually entirely reasonable for the outer most one (ExceptionGlobster) to when it has a pattern error, check all of its regexs on the spot, and raise an InvalidPatternError which has all the patterns that are wrong.

That won't affect log because log doesn't using globbing (and if it did that would be ok too, because it would show the bad pattern); it will also help 'rules' if you do this to _OrderedGlobster as well.

So in short:
 - unwind all the changes to builtins and ignores, they are the wrong level and API to be fixing this; change just globbing.py and add the new error to errors.py; add some unit tests for this case to *Globster.match, and we're done.

review: Needs Fixing

[lp:~parthm/bzr/300062-better-handling-for-invalid-ignore-pattern]

5288. By Parth Malwankar on 2010-06-21

merged in changes from trunk

5289. By Parth Malwankar on 2010-06-22

better invalid pattern error handling.
Invalid patterns are now handled in a single location,
bzrlib.globbing.Globster.

[Parth Malwankar (parthm)]

> Ok, I've looked through this again, and I'd like to suggest a deeper
> implementation strategy.

Thanks for the review and comment. I have cleanup the error handling now.
Basically, the following is done:
1. General regex compile failure now raises InvalidPattern error.
2. Invalid pattern errors are now raised only by bzrlib.globbing.Globster.match
3. `bzr ignore PATTERN..` skips invalid patterns passed. Existing errors are handled by bzrlib.globbing.Globster.match.

[John A Meinel (jameinel)]

1) It might be clearer to resubmit this, since it has changed fairly significantly since the beginning. (So I don't think Robert's Needs Fixing still directly applies.)

2)
61 +class InvalidPattern(BzrError):
62 +
63 + _fmt = ('Invalid pattern(s) found.')
64 +
65 + def __init__(self, message):
66 + self.message = message
67 +

^- You set a message, but then never display it (there is nothing in _fmt which shows the attributes.)

I suppose in builtins you do:
29 + if invalid_patterns:
30 + pats = '\n '.join(invalid_patterns)
31 + ui.ui_factory.show_warning(
32 + 'Skipped invalid pattern(s):\n %s' % pats)
^- which explicitly displays the bad patterns, but then you do so in a warning and not an error. Which seems to disagree with your "now raises exceptions" comment.

3) It seems very strange to me that 'show_pattern_errors' is a module-level function that re-reads the ignore files and reports on them.

Rather than, for example, having the state of the Globster used to determine what patterns are invalid.

It means that anyone using Globster for anything that isn't *bzr* (imagine qbzr using it for something) is going to get strange results. (As it will tell them about problems with something that isn't related at all.)

4) so for the construct here:
105 + except errors.InvalidPattern, e:
106 + # lazy_regex throws InvalidPattern in case of bad pattern.
107 + # For Globbing, the bad pattern may have come from an ignore
108 + # file, so we should any bad patterns that the ignore file(s)
109 + # may have.
110 + show_pattern_errors()
111 + raise errors.InvalidPattern(str(e))

It seems better to me to do the formatting and put that on the errors.InvalidPattern that is getting raised, rather than calling show_pattern_errors() and having that display.

I also find it strange to catch InvalidPattern and then raise another InvalidPattern(str(first_e))

Especially given your formatting for InvalidPattern this would become:
    InvalidPattern('Invalid pattern(s) found.')

Which would lose any sort of context that the original error tried to pass on. (So if a low level exception raised InvalidPattern('pattern XXX is broken') then you catch it and re-raise a InvalidPattern('Invalid pattern(s) found.') which is pretty unhelpful.)

5) In this test:
351 + def test_add_invalid_ignore_in_global_ignore(self):
352 + """add should handle invalid global ignore pattern gracefully.
353 + """
354 + tree = self.make_branch_and_tree('.')
355 + self.build_tree(['top.txt'])
356 + self.build_tree_contents([('foobar', 'foo\n')])
357 + ignores.add_unique_user_ignores([u"RE:*.cpp"]) # invalid pattern
358 + out, err = self.run_bzr(['add'], 3)
359 + self.assertEquals(out, '')
360 + self.assertContainsRe(err,
361 + ('error: Invalid pattern.*RE:\*\.cpp.*ERROR: '
362 + 'Invalid pattern'),
363 + re.DOTALL)
364 +

^- You make it so that 'ignores.add_unique_user_ignores()' allows an invalid pattern. It seems more logical to fault that at insert time, rather than at 'bzr add' time. (We probably still need to fail there, but we should first forbid people from adding broken entries in the first place.)

388 + def test_invalid_pattern_in_ignore_file(self):
389 + """Existing inval...

review: Needs Fixing

[Parth Malwankar (parthm)]

> 1) It might be clearer to resubmit this, since it has changed fairly
> significantly since the beginning. (So I don't think Robert's Needs Fixing
> still directly applies.)
>
> 2)
> 61 +class InvalidPattern(BzrError):
> 62 +
> 63 + _fmt = ('Invalid pattern(s) found.')
> 64 +
> 65 + def __init__(self, message):
> 66 + self.message = message
> 67 +
>
>
> ^- You set a message, but then never display it (there is nothing in _fmt
> which shows the attributes.)
>

Good catch. Will fix it and resubmit.

> I suppose in builtins you do:
> 29 + if invalid_patterns:
> 30 + pats = '\n '.join(invalid_patterns)
> 31 + ui.ui_factory.show_warning(
> 32 + 'Skipped invalid pattern(s):\n %s' % pats)
> ^- which explicitly displays the bad patterns, but then you do so in a warning
> and not an error. Which seems to disagree with your "now raises exceptions"
> comment.
>

This is for the `bzr ignore` command where if the user gives 'bzr good1 good2 bad1 good3 bad2', good1-3 get added to .bzrignore and a warning is issued for bad1-2.

> 3) It seems very strange to me that 'show_pattern_errors' is a module-level
> function that re-reads the ignore files and reports on them.
>
> Rather than, for example, having the state of the Globster used to determine
> what patterns are invalid.
>
> It means that anyone using Globster for anything that isn't *bzr* (imagine
> qbzr using it for something) is going to get strange results. (As it will tell
> them about problems with something that isn't related at all.)
>

One reason for doing this was that the invalid pattern could come either from .bazaar/ignore of .bzrignore and I wanted to display a message indicating the bad pattern along with the specific file information. E.g.

[a1234]% ~/src/bzr.dev/300062-better-handling-for-invalid-ignore-pattern/bzr st
bzr: error: Invalid pattern(s) found in "/home/parthm/tmp/a1234/.bzrignore":
  RE:*.x
bzr: error: Invalid pattern(s) found in "/home/parthm/.bazaar/ignore":
  RE:[
bzr: ERROR: Invalid pattern(s) found.
[a1234]%

We could use the Globster state to display the message but the user would need to check both files. I would expect that the probability of an error in .bzrignore is higher but it just seems nicer to indicate the file.
Thoughts?

> 4) so for the construct here:
> 105 + except errors.InvalidPattern, e:
> 106 + # lazy_regex throws InvalidPattern in case of bad pattern.
> 107 + # For Globbing, the bad pattern may have come from an ignore
> 108 + # file, so we should any bad patterns that the ignore file(s)
> 109 + # may have.
> 110 + show_pattern_errors()
> 111 + raise errors.InvalidPattern(str(e))
>
> It seems better to me to do the formatting and put that on the
> errors.InvalidPattern that is getting raised, rather than calling
> show_pattern_errors() and having that display.
>
> I also find it strange to catch InvalidPattern and then raise another
> InvalidPattern(str(first_e))
>
> Especially given your formatting for InvalidPattern this would become:
> InvalidPattern('Invalid pattern(s) found.')
>
> Which would lose any sort of context that the original error...

[lp:~parthm/bzr/300062-better-handling-for-invalid-ignore-pattern]

5290. By Parth Malwankar on 2010-06-29

merged changes from trunk

5291. By Parth Malwankar on 2010-06-29

removed re_compile_checked as re.compile shows InvalidPattern error.
Fixed test cases.

[John A Meinel (jameinel)]

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

...

>> It means that anyone using Globster for anything that isn't *bzr* (imagine
>> qbzr using it for something) is going to get strange results. (As it will tell
>> them about problems with something that isn't related at all.)
>>
>
> One reason for doing this was that the invalid pattern could come either from .bazaar/ignore of .bzrignore and I wanted to display a message indicating the bad pattern along with the specific file information. E.g.

Could we just include this information as part of the Globster from the
beginning? I understand that knowing where the pattern is bad is
helpful. I'm just concerned that it is using state after-the-fact to
debug a current failure. And anything that isn't *bzr* is likely to be
wrong here.

>
> [a1234]% ~/src/bzr.dev/300062-better-handling-for-invalid-ignore-pattern/bzr st
> bzr: error: Invalid pattern(s) found in "/home/parthm/tmp/a1234/.bzrignore":
> RE:*.x
> bzr: error: Invalid pattern(s) found in "/home/parthm/.bazaar/ignore":
> RE:[
> bzr: ERROR: Invalid pattern(s) found.
> [a1234]%
>
> We could use the Globster state to display the message but the user would need to check both files. I would expect that the probability of an error in .bzrignore is higher but it just seems nicer to indicate the file.
> Thoughts?

Pass in the name of the file associated with each pattern as part of
creating the Globster. Alternatively just give the short 'invalid
pattern found'. Better (IMO) than re-parsing the files.

...

>>
>> ^- You make it so that 'ignores.add_unique_user_ignores()' allows an invalid
>> pattern. It seems more logical to fault that at insert time, rather than at
>> 'bzr add' time. (We probably still need to fail there, but we should first
>> forbid people from adding broken entries in the first place.)
>>
>
> This is done in cmd_ignore.run. This was the "Skipped invalid pattern(s)" warning. So the bad pattern doesn't reach this point. This test case injects the bad pattern to handle the case of when the user might add the bad pattern using and external editor.
>
>

It seems better to me to make our internals robust and hard to subvert
accidentally, rather than assuming that will be done at the cmd_* layer.
Certainly qbzr or explorer is likely to end up with their own UI for
adding ignores, and it would be nice if they could also have invalid
ignores captured early rather than late. (Ideally without having to
re-implement it.)

>
>
>> ^- I would probably say that we should fail all insertions if one is invalid,
>> but we would want it to be atomic. eg if the do 'bzr ignore valid valid
>> invalid valid' either all valids get inserted [as you have today] or nothing
>> gets inserted. You don't want to add the first two, and then fail, as it
>> leaves things in an unknown state.
>>
>> I would probably tend towards 'validate early and fail', as long as we are
>> choosing that for other commands. (I would still like to see status try even
>> if it gets a bogus ignore, but I understand why Robert feels so strongly about
>> 'add' even if I probably disagree.)
>>
>> Also, it is a little bit odd that if you *have* an invalid entry it fails wi...

Unmerged revisions

5291. By Parth Malwankar on 2010-06-29

removed re_compile_checked as re.compile shows InvalidPattern error.
Fixed test cases.

5290. By Parth Malwankar on 2010-06-29

merged changes from trunk

5289. By Parth Malwankar on 2010-06-22

better invalid pattern error handling.
Invalid patterns are now handled in a single location,
bzrlib.globbing.Globster.

5288. By Parth Malwankar on 2010-06-21

merged in changes from trunk

5287. By Parth Malwankar on 2010-06-01

fixed scope error due to lazy ui import

5286. By Parth Malwankar on 2010-06-01

filter_invalid_patterns now returns invalid and valid patterns.
dispay is left to the caller.

5285. By Parth Malwankar on 2010-06-01

added unit test for is_pattern_valid and filter_invalid_patterns

5284. By Parth Malwankar on 2010-06-01

unit tests for get_pattern_errors

5283. By Parth Malwankar on 2010-06-01

improved comments

5282. By Parth Malwankar on 2010-06-01

updated NEWS.

=== modified file 'NEWS'
--- NEWS	2010-06-28 21:45:10 +0000
+++ NEWS	2010-06-29 04:04:25 +0000
@@ -84,6 +84,14 @@
   test that all commands available to the test suite have help.
   (Robert Collins, #177500)
 
+* Improved handling of invalid ignore patterns. ``ignore`` rejects invalid
+  patterns with a warning and does not add them to branch ignore file.
+  In case of an invalid pattern in ignore files, bzr commands accessing
+  the ignores fail with a clear error message indicating the invalid pattern
+  and file. Invalid regular expression will now display a bzr error
+  ``InvalidPattern`` instead of a stack trace.
+  (Parth Malwankar, #300062)
+
 * Progress output is cleaned up when exiting.  (Aaron Bentley)
 
 * Raise ValueError instead of a string exception.
 
=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-06-17 08:53:15 +0000
+++ bzrlib/builtins.py	2010-06-29 04:04:25 +0000
@@ -2717,18 +2717,27 @@
                 (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(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)
         matches = []
-        tree.lock_read()
+        self.add_cleanup(tree.lock_read().unlock)
         for entry in tree.list_files():
             id = entry[3]
             if id is not None:
                 filename = entry[0]
                 if ignored.match(filename):
                     matches.append(filename)
-        tree.unlock()
         if len(matches) > 0:
             self.outf.write("Warning: the following files are version controlled and"
                   " match your ignore pattern:\n%s"
 
=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2010-06-17 11:52:13 +0000
+++ bzrlib/errors.py	2010-06-29 04:04:25 +0000
@@ -3156,3 +3156,10 @@
         'E.g. bzr whoami "Your Name <[e-mail]>"')
 
 
+class InvalidPattern(BzrError):
+
+    _fmt = ('Invalid pattern(s) found. %(message)s')
+
+    def __init__(self, message):
+        self.message = message
+
 
=== modified file 'bzrlib/globbing.py'
--- bzrlib/globbing.py	2010-02-17 17:11:16 +0000
+++ bzrlib/globbing.py	2010-06-29 04:04:25 +0000
@@ -21,10 +21,19 @@
 """
 
 import re
+import errno
 
+import bzrlib
+from bzrlib.lazy_regex import real_re_compile
 from bzrlib.trace import (
     warning
     )
+from bzrlib import (
+    config,
+    bzrdir,
+    errors,
+    ui,
+    )
 
 
 class Replacer(object):
@@ -209,10 +218,24 @@
 
         :return A matching pattern or None if there is no matching pattern.
         """
-        for regex, patterns in self._regex_patterns:
-            match = regex.match(filename)
-            if match:
-                return patterns[match.lastindex -1]
+        try:
+            for regex, patterns in self._regex_patterns:
+                match = regex.match(filename)
+                if match:
+                    return patterns[match.lastindex -1]
+        except errors.InvalidPattern, e:
+            # lazy_regex throws InvalidPattern in case of bad pattern.
+            # For Globbing, the bad pattern may have come from an ignore
+            # file, so we should display any bad patterns that the ignore
+            # file(s) may have.
+            # The invalid pattern message (e.message) is specific to a
+            # pattern that re.compile failed to compile. As we show all
+            # possible pattern errors in .bazaar/ignore and .bzrignore
+            # we set the message to '' and causing InvalidPattern to show
+            # a generic error message.
+            show_pattern_errors()
+            e.message = ''
+            raise e
         return None
 
 class ExceptionGlobster(object):
@@ -283,3 +306,110 @@
     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: normalize_pattern
+    """
+    result = True
+    translator = None
+    if pattern.startswith(u'RE:') or u'/' in pattern:
+        translator = _sub_fullpath
+    elif pattern.startswith(u'*.'):
+        translator = _sub_extension
+    else:
+        translator = _sub_basename
+    tpattern = '(%s)' % translator(pattern)
+    try:
+        cpattern = real_re_compile(tpattern)
+    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: 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
+
+
+def get_pattern_errors():
+    """Return two tuples with information about invalid patterns.
+
+    :return: This returns two tuples in the format (path, patterns).
+      path is the full path to either the ignores file or .bzrignore.
+      patterns is a list of invalid patterns found in that file.
+
+    see also: show_pattern_errors()
+    """
+    # import ignores locally to avoid circular dependency
+    # with bzrlib.ignores. We need ignores.parse_ignore_file
+    # while bzrlib.ignores already uses bzrlib.globbing.
+    from bzrlib import ignores
+    def _get_errors(path):
+        invalid_patterns = []
+        f = None
+        try:
+            try:
+                f = open(path, 'rb')
+                patterns = ignores.parse_ignore_file(f)
+                for p in patterns:
+                    if not is_pattern_valid(p):
+                        invalid_patterns.append(p)
+            except (IOError, OSError), e:
+                # open() shouldn't return an IOError without errno,
+                # but just in case
+                err = getattr(e, 'errno', None)
+                if err != errno.ENOENT:
+                    # Its not necessary that the user with have
+                    # .bzrignore or .bazaar/ignore. If we are in
+                    # _get_errors she definitely has at least one,
+                    # but not necessarily both. So we ignore
+                    # errno.ENOENT
+                    raise e
+        finally:
+            if f:
+                f.close()
+        return invalid_patterns
+
+    wt, branch, relpath = \
+        bzrdir.BzrDir.open_containing_tree_or_branch('.')
+    bzrignore = wt.abspath(bzrlib.IGNORE_FILENAME)
+    if wt:
+        bzr_ignore_patterns = _get_errors(bzrignore)
+    else:
+        bzrignore = []
+        bzr_ignore_patterns = []
+    user_ignore = config.user_ignore_config_filename()
+    user_ignore_patterns = _get_errors(user_ignore)
+    return (bzrignore, bzr_ignore_patterns), (user_ignore, user_ignore_patterns)
+
+
+def show_pattern_errors():
+    """Displays a warning about invalid patterns found.
+
+    Display warning(s) with the list of invalid patterns
+    found in ignores file or .bzrignore.
+    """
+    invalid_pattern_info = get_pattern_errors()
+    for path, invalid_patterns in invalid_pattern_info:
+        if invalid_patterns:
+            pattern_str = '\n  '.join(invalid_patterns)
+            ui.ui_factory.show_error('Invalid pattern(s) found in "%s":\n  %s'
+                % (path, pattern_str))
+
 
=== modified file 'bzrlib/ignores.py'
--- bzrlib/ignores.py	2010-05-03 04:51:58 +0000
+++ bzrlib/ignores.py	2010-06-29 04:04:25 +0000
@@ -44,10 +44,9 @@
 ]
 
 
-
 def parse_ignore_file(f):
     """Read in all of the lines in the file and turn it into an ignore list
-    
+
     Continue in the case of utf8 decoding errors, and emit a warning when 
     such and error is found. Optimise for the common case -- no decoding 
     errors.
@@ -176,7 +175,6 @@
     """Get the current set of runtime ignores."""
     return _runtime_ignores
 
-
 def tree_ignores_add_patterns(tree, name_pattern_list):
     """Add more ignore patterns to the ignore file in a tree.
     If ignore file does not exist then it will be created.
@@ -227,3 +225,4 @@
 
     if not tree.path2id(bzrlib.IGNORE_FILENAME):
         tree.add([bzrlib.IGNORE_FILENAME])
+
 
=== modified file 'bzrlib/lazy_regex.py'
--- bzrlib/lazy_regex.py	2009-03-23 14:59:43 +0000
+++ bzrlib/lazy_regex.py	2010-06-29 04:04:25 +0000
@@ -22,6 +22,8 @@
 
 import re
 
+from bzrlib import errors
+
 
 class LazyRegex(object):
     """A proxy around a real regex, which won't be compiled until accessed."""
@@ -58,7 +60,12 @@
 
     def _real_re_compile(self, *args, **kwargs):
         """Thunk over to the original re.compile"""
-        return _real_re_compile(*args, **kwargs)
+        try:
+            return real_re_compile(*args, **kwargs)
+        except re.error, e:
+            # raise InvalidPattern instead of re.error as this gives a
+            # cleaner message to the user.
+            raise errors.InvalidPattern('"' + args[0] + '" ' +str(e))
 
     def __getattr__(self, attr):
         """Return a member from the proxied regex object.
@@ -97,11 +104,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/log.py'
--- bzrlib/log.py	2010-06-17 08:53:15 +0000
+++ bzrlib/log.py	2010-06-29 04:04:25 +0000
@@ -86,7 +86,6 @@
     format_date,
     format_date_with_offset_in_original_timezone,
     get_terminal_encoding,
-    re_compile_checked,
     terminal_width,
     )
 from bzrlib.symbol_versioning import (
@@ -811,8 +810,7 @@
     """
     if search is None:
         return log_rev_iterator
-    searchRE = re_compile_checked(search, re.IGNORECASE,
-            'log message filter')
+    searchRE = re.compile(search, re.IGNORECASE)
     return _filter_message_re(searchRE, log_rev_iterator)
 
 
 
=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2010-06-25 20:34:05 +0000
+++ bzrlib/osutils.py	2010-06-29 04:04:25 +0000
@@ -2169,29 +2169,6 @@
             raise
 
 
-def re_compile_checked(re_string, flags=0, where=""):
-    """Return a compiled re, or raise a sensible error.
-
-    This should only be used when compiling user-supplied REs.
-
-    :param re_string: Text form of regular expression.
-    :param flags: eg re.IGNORECASE
-    :param where: Message explaining to the user the context where
-        it occurred, eg 'log search filter'.
-    """
-    # from https://bugs.launchpad.net/bzr/+bug/251352
-    try:
-        re_obj = re.compile(re_string, flags)
-        re_obj.search("")
-        return re_obj
-    except re.error, e:
-        if where:
-            where = ' in ' + where
-        # despite the name 'error' is a type
-        raise errors.BzrCommandError('Invalid regular expression%s: %r: %s'
-            % (where, re_string, e))
-
-
 if sys.platform == "win32":
     import msvcrt
     def getchar():
 
=== modified file 'bzrlib/tests/__init__.py'
--- bzrlib/tests/__init__.py	2010-06-25 06:12:56 +0000
+++ bzrlib/tests/__init__.py	2010-06-29 04:04:25 +0000
@@ -2742,8 +2742,7 @@
     :param pattern: A regular expression string.
     :return: A callable that returns True if the re matches.
     """
-    filter_re = osutils.re_compile_checked(pattern, 0,
-        'test filter')
+    filter_re = re.compile(pattern, 0)
     def condition(test):
         test_id = test.id()
         return filter_re.search(test_id)
 
=== 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-29 04:04:25 +0000
@@ -18,8 +18,10 @@
 """Tests of the 'bzr add' command."""
 
 import os
+import re
 
 from bzrlib import (
+    ignores,
     osutils,
     tests,
     )
@@ -217,3 +219,31 @@
         os.symlink(osutils.abspath('target'), 'tree/link')
         out = self.run_bzr(['add', 'tree/link'])[0]
         self.assertEquals(out, 'adding link\n')
+
+    def test_add_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'], 3)
+        self.assertEquals(out, '')
+        self.assertContainsRe(err,
+            ('error: Invalid pattern.*RE:\*\.cpp.*ERROR: '
+             'Invalid pattern'),
+            re.DOTALL)
+
+    def test_add_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'], 3)
+        self.assertEquals(out, '')
+        self.assertContainsRe(err,
+            ('error: Invalid pattern.*RE:\*\.cpp.*ERROR: '
+             'Invalid pattern'),
+            re.DOTALL)
+
 
=== modified file 'bzrlib/tests/blackbox/test_ignore.py'
--- bzrlib/tests/blackbox/test_ignore.py	2010-06-11 07:32:12 +0000
+++ bzrlib/tests/blackbox/test_ignore.py	2010-06-29 04:04:25 +0000
@@ -162,3 +162,36 @@
         tree = self.make_branch_and_tree('a')
         self.run_bzr(['ignore', '--directory=a', 'README'])
         self.check_file_contents('a/.bzrignore', 'README\n')
+
+    def test_ignored_invalid_pattern(self):
+        """Ensure graceful handling for invalid ignore pattern.
+
+        Test case for #300062.
+        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'])
+        self.assertEqual(out, '')
+        self.assertContainsRe(err,
+            'bzr: warning: Skipped invalid pattern.*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', 'RE:[\n')])
+        out, err = self.run_bzr(['ignore', 'RE:*.cpp', 'abc'], 3)
+        self.assertEqual(out, '')
+        self.assertContainsRe(err, 'warning: Skipped invalid pattern'
+            '.*RE:\*\.cpp.*'
+            'error: 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-06-11 07:32:12 +0000
+++ bzrlib/tests/blackbox/test_ignored.py	2010-06-29 04:04:25 +0000
@@ -17,6 +17,8 @@
 
 """Tests of the 'bzr ignored' command."""
 
+import re
+
 from bzrlib.tests import TestCaseWithTransport
 
 
@@ -46,3 +48,15 @@
                                   ('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'])
+        self.build_tree_contents([('.bzrignore', 'RE:[\n')])
+        out, err = self.run_bzr(['ignored'], 3)
+        self.assertEqual(out, '')
+        self.assertContainsRe(err, 'error: Invalid pattern.*RE:\[.*ERROR:',
+            re.DOTALL)
+
 
=== modified file 'bzrlib/tests/blackbox/test_log.py'
--- bzrlib/tests/blackbox/test_log.py	2010-06-25 08:01:24 +0000
+++ bzrlib/tests/blackbox/test_log.py	2010-06-29 04:04:25 +0000
@@ -386,11 +386,8 @@
         """
         self.make_minimal_branch()
         out, err = self.run_bzr(['log', '-m', '*'], retcode=3)
-        self.assertEqual("bzr: ERROR: Invalid regular expression"
-            " in log message filter"
-            ": '*'"
-            ": nothing to repeat\n", err)
-        self.assertEqual('', out)
+        self.assertContainsRe(err, "ERROR.*Invalid pattern.*nothing to repeat")
+        self.assertEqual(out, '')
 
     def test_log_unsupported_timezone(self):
         self.make_linear_branch()
 
=== 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-29 04:04:25 +0000
@@ -24,12 +24,14 @@
 
 from cStringIO import StringIO
 import codecs
+import re
 from os import mkdir, chdir, rmdir, unlink
 import sys
 from tempfile import TemporaryFile
 
 from bzrlib import (
     bzrdir,
+    ignores,
     conflicts,
     errors,
     osutils,
@@ -701,6 +703,57 @@
         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('.')
+        self.build_tree_contents([('.bzrignore', 'RE:*.cpp\n')])
+        wt.commit('commit .bzrignore')
+        out, err = self.run_bzr(['status'], 3)
+        self.assertEquals(out, '')
+        self.assertContainsRe(err, 'error: Invalid pattern.*RE:\*\.cpp.*ERROR',
+            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'], 3)
+        self.assertEquals(out, '')
+        self.assertContainsRe(err, 'error: Invalid pattern.*RE:\*\.cpp.*ERROR',
+            flags=re.DOTALL)
+
+    def test_short_status_invalid_ignore_pattern(self):
+        """Tests short status with invalid ignore pattern"""
+        wt = self.make_branch_and_tree('.')
+        self.build_tree_contents([('.bzrignore', 'RE:*.cpp\n')])
+        wt.commit('commit .bzrignore')
+        out, err = self.run_bzr(['status', '-S'], 3)
+        self.assertEquals(out, '')
+        self.assertContainsRe(err, 'error: Invalid pattern.*RE:\*\.cpp.*ERROR',
+            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')
+        self.build_tree_contents([('.bzrignore', 'RE:*.cpp\n')])
+        wt.commit('commit .bzrignore')
+        out, err = self.run_bzr(['status', 'file'], 3)
+        self.assertEquals(out, '')
+        self.assertContainsRe(err, 'error: Invalid pattern.*RE:\*\.cpp.*ERROR',
+            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-29 04:04:25 +0000
@@ -15,15 +15,20 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
+from bzrlib import config, ignores
 from bzrlib.globbing import (
     Globster,
     ExceptionGlobster,
+    filter_invalid_patterns,
+    get_pattern_errors,
+    is_pattern_valid,
     _OrderedGlobster,
     normalize_pattern
     )
 from bzrlib.tests import (
     TestCase,
     TestCaseInTempDir,
+    TestCaseWithTransport,
     )
 
 
@@ -371,3 +376,48 @@
         """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(TestCaseWithTransport):
+
+    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))
+
+    def test_invalid_ignores(self):
+        """Ensure that invalid user ignores are detected.
+        """
+        ignore_path = config.user_ignore_config_filename()
+        user_ignores = ignores.get_user_ignores()
+        f = open(ignore_path, 'wb')
+        try:
+            f.write(u'foo\nRE:*.cpp\nbar\nRE:[\nbaz')
+        finally:
+            f.close()
+        (bi, bip), (ui, uip) = get_pattern_errors()
+        self.assertEquals(set(uip), set([u'RE:*.cpp', u'RE:[']))
+        self.assertEquals(bip, [])
+
+    def test_invalid_bzrignore(self):
+        """Ensure that invalid .bzrignore entries are detected.
+        """
+        tree = self.make_branch_and_tree(".")
+        self.build_tree_contents([('.bzrignore',
+            "foo\nRE:*.cpp\nbar\nRE:[\nbaz\n")])
+        tree.add([".bzrignore"])
+        (bi, bip), (ui, uip) = get_pattern_errors()
+        self.assertEquals(set(bip), set([u'RE:*.cpp', u'RE:[']))
+        self.assertEquals(uip, [])
+
 
=== 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-29 04:04:25 +0000
@@ -217,3 +217,4 @@
         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"])
+
 
=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2010-06-08 01:45:09 +0000
+++ bzrlib/tests/test_osutils.py	2010-06-29 04:04:25 +0000
@@ -1703,24 +1703,6 @@
         self.assertRaises(IOError, osutils.resource_string, 'bzrlib', 'yyy.xx')
 
 
-class TestReCompile(tests.TestCase):
-
-    def test_re_compile_checked(self):
-        r = osutils.re_compile_checked(r'A*', re.IGNORECASE)
-        self.assertTrue(r.match('aaaa'))
-        self.assertTrue(r.match('aAaA'))
-
-    def test_re_compile_checked_error(self):
-        # like https://bugs.launchpad.net/bzr/+bug/251352
-        err = self.assertRaises(
-            errors.BzrCommandError,
-            osutils.re_compile_checked, '*', re.IGNORECASE, 'test case')
-        self.assertEqual(
-            "Invalid regular expression in test case: '*': "
-            "nothing to repeat",
-            str(err))
-
-
 class TestDirReader(tests.TestCaseInTempDir):
 
     # Set by load_tests