[MERGE] is_ignored improvements...

[Jan Hudec]

On Fri, May 19, 2006 at 08:54:53 +1000, Robert Collins wrote:
> On Thu, 2006-05-18 at 19:18 +0200, Jan Hudec wrote:
> > On Fri, May 19, 2006 at 00:14:08 +1000, Robert Collins wrote:
> > > Heres my alternative patch, finished up:
> > > 
> > > I currently prefer this over the original patch you prepared because of
> > > the preservation of API. I dont think many users will have more than 100
> > > regexs, and it should perform just fine.
> > > 
> > > What do you think? Does this make the future improvements you are
> > > working on harder? Is it a problem to take this variation?
> > 
> > I certainly isn't a problem. I can combine the patches later when/as needed.
> 
> Ok
> 
> > However I have some comments, below in the patch. The most important is
> > probably that you should use the fact, that fnmatch.translate will never
> > generate capturing (nor non-capturing) groups, and simplify the combiner.
> 
> Thanks for the review. With respect to the complication translation of
> regex's - I did look in fnmatch.translate and see they dont ever use
> capturing groups. However, its not part of the published api that it
> wont *ever* do that - so I wrote the translator to the published
> interface which is simply
> --
> fnmatch.translate = translate(pat)
>     Translate a shell PATTERN to a regular expression.
>     
>     There is no way to quote meta-characters.
> --
> 
> I felt it was risky to assume that the fnmatch translation internals
> would never change. We could change it to do a compile, assert that
> groups is 0, and after that do a slice, which will make the return value
> from _combine_ignore_rules a little simpler (list rather than regex),
> but at the cost of forcing a dpendence on the internals of translate.

Well, I think they will use non-capturing group if they ever change it,
but for now I think and assert is enough and later we can use the
Replacer (part of the new converter) to convert all capturing groups to
non-capturing ones -- I actually do that for the RE:<regexp> patterns in
the new engine.

> > Also I think you should add a blackbox test that actually creates
> > a 1000-lines long .bzrignore and checks some files matched by patterns
> > towards the end are ignored and proper pattern is reported for them. I had
> > such test there simply creating patterns '*.0' through '*.999' and matching
> > a few files like 'foo.333' and 'bar.987' or somesuch. This will test that the
> > regexp engine will actually handle the patterns as split and report correct
> > group matches.
> 
> Well, I have a smoke test for 198 patterns, I can certainly modify that
> to be more in-depth. I dont agree about it being black box though -
> theres nothing in the ui that relates to the # of ignore rules.

Well, in fact my test wasn't blackbox either. It was just with a working
tree. Yes, that's enough. Just to make sure it really returns the right
group when there is not all in one pattern and that the pattern length
does not exceed the python limit.

-- 
						 Jan 'Bulb' Hudec <bulb at ucw.cz>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060519/ffb5c25b/attachment.pgp