Merge lp:~parthm/bzr/300062-2.2-bad-pattern-error-part-2 into lp:bzr/2.2
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Vincent Ladeuil on 2010-07-21 | ||||
Approved revision: | 5061 | ||||
Merged at revision: | 5065 | ||||
Proposed branch: | lp:~parthm/bzr/300062-2.2-bad-pattern-error-part-2 | ||||
Merge into: | lp:bzr/2.2 | ||||
Diff against target: |
279 lines (+135/-39) 6 files modified
NEWS (+5/-0) bzrlib/builtins.py (+8/-0) bzrlib/globbing.py (+75/-23) bzrlib/tests/blackbox/test_ignore.py (+16/-0) bzrlib/tests/per_workingtree/test_is_ignored.py (+25/-13) bzrlib/tests/test_globbing.py (+6/-3) |
||||
To merge this branch: | bzr merge lp:~parthm/bzr/300062-2.2-bad-pattern-error-part-2 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Vincent Ladeuil | Approve on 2010-07-21 | ||
John A Meinel | 2010-07-15 | Needs Fixing on 2010-07-19 | |
Review via email:
|
Commit message
'bzr ignore' fails on bad pattern. bad pattern error messages now shows the faulting pattern.
Description of the change
This fix is towards bug #300062
It does the following:
1. `bzr ignore PATTERNS` now fails with an error if an invalid pattern is provided.
2. For pattern errors, the faulting pattern is displayed to the user.
E.g.
[a1234]% ~/src/bzr.
bzr: ERROR: Invalid pattern(s) found. File ~/.bazaar/ignore or .bzrignore contains error(s).
RE:[
[a1234]% ~/src/bzr.
bzr: error: Invalid ignore pattern(s) found.
RE:*.cpp
bzr: ERROR: Invalid pattern(s) found.
[a1234]%
This patch is the same as the https:/
If also contains update for e.message -> e.msg fix that came up since the previous patch was created.
Parth Malwankar (parthm) wrote : | # |
> ^- I think factoring the patterns into a dict indirected code is interesting.
> I'm a bit concerned about:
> for pattern_type, patterns in pattern_
>
> I'm pretty sure we explicitly intended the order of 'check extensions' then
> 'check basename' then 'check fullpath'. So that we can do checks on just the
> little bit of the content before we have to check against the whole content. I
> don't have any explicit proof of this, but turning the current deterministic
> ordering with a random one doesn't seem ideal.
>
Makes sense. I have updated the patch to make _add_patterns order deterministic.
>
> otherwise the change seems fine to me. I may be over-concerned, but I do
> believe that 'is_ignored()' is a fair amount of overhead in the 'initial add
> of 30k files', and I'd like us to at least be aware of it. I do wonder about
> changing the internals so that instead of using '?:.*/)?(?!.*/)) to filter out
> everything but the basename for some patterns, if we wouldn't be better off
> knowing that this is a basename-only pattern and just supply that to the
> match.
>
Filed bug #607258 to track this.
Thanks for the review.
>
> I'm certainly willing to discuss it, but the easiest way to move forward is to
> just make the order of _add_patterns deterministic by iterating over a fixed
> ordering.
Vincent Ladeuil (vila) wrote : | # |
43 + TYPE_FULLPATH = 1
44 + TYPE_BASENAME = 2
45 + TYPE_EXTENSION = 3
This looks a bit C-ish, why don't you just use regular keys ('fullpath', 'basename' and 'extension') ?
From a design point of view it looks like you're defining several set of objects associated with the same key which sounds like you want a dict or even a registry...
Also, while this is certainly out of scope for this proposal, keep in mind that some plugins want to define their own '.xxx-ignore' files and our current API is already a pain to use, it would be nice if we can help them by providing the right set of function/classes to process a set of paths against a given set of patterns coming from a file (or several in bzr case...).
Parth Malwankar (parthm) wrote : | # |
> 43 + TYPE_FULLPATH = 1
> 44 + TYPE_BASENAME = 2
> 45 + TYPE_EXTENSION = 3
>
> This looks a bit C-ish, why don't you just use regular keys ('fullpath',
> 'basename' and 'extension') ?
>
> From a design point of view it looks like you're defining several set of
> objects associated with the same key which sounds like you want a dict or even
> a registry...
>
I have updated the patch to use a dict of dict with strings. Its much cleaner now.
> Also, while this is certainly out of scope for this proposal, keep in mind
> that some plugins want to define their own '.xxx-ignore' files and our current
> API is already a pain to use, it would be nice if we can help them by
> providing the right set of function/classes to process a set of paths against
> a given set of patterns coming from a file (or several in bzr case...).
Makes sense. I think it may be possible to build an API on top of the
current implementation to make it easy. I will look into bzr-upload as
suggested by you on IRC.
Thanks for the review.
Vincent Ladeuil (vila) wrote : | # |
Yup, far cleaner.
I think you've also addressed jam's concerns so good to land for me.
Parth Malwankar (parthm) wrote : | # |
sent to pqm by email
79 - self._add_ patterns( ext_patterns, _sub_extension, r'(?:.* /)?(?!. */)(?:. *\.)') patterns( base_patterns, _sub_basename, r'(?:.* /)?(?!. */)') patterns( path_patterns, _sub_fullpath) lists[Globster. identify( pat)].append( pat) lists.iteritems (): patterns( patterns, translators[ pattern_ type], prefixes[ pattern_ type])
80 - prefix=
81 - self._add_
82 - prefix=
83 - self._add_
84 + pattern_
85 + for pattern_type, patterns in pattern_
86 + self._add_
87 + Globster.
88 + Globster.
^- I think factoring the patterns into a dict indirected code is interesting. I'm a bit concerned about: lists.iteritems ():
for pattern_type, patterns in pattern_
I'm pretty sure we explicitly intended the order of 'check extensions' then 'check basename' then 'check fullpath'. So that we can do checks on just the little bit of the content before we have to check against the whole content. I don't have any explicit proof of this, but turning the current deterministic ordering with a random one doesn't seem ideal.
otherwise the change seems fine to me. I may be over-concerned, but I do believe that 'is_ignored()' is a fair amount of overhead in the 'initial add of 30k files', and I'd like us to at least be aware of it. I do wonder about changing the internals so that instead of using '?:.*/)?(?!.*/)) to filter out everything but the basename for some patterns, if we wouldn't be better off knowing that this is a basename-only pattern and just supply that to the match.
I'm certainly willing to discuss it, but the easiest way to move forward is to just make the order of _add_patterns deterministic by iterating over a fixed ordering.