Code review comment for lp:~parthm/bzr/300062-better-handling-for-invalid-ignore-pattern

Revision history for this message
Robert Collins (lifeless) wrote :

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

« Back to merge proposal