Merge lp:~parthm/bzr/300062-bad-pattern-error-part-1 into lp:bzr

Proposed by Parth Malwankar on 2010-06-30
Status: Merged
Approved by: John A Meinel on 2010-07-07
Approved revision: 5334
Merged at revision: 5339
Proposed branch: lp:~parthm/bzr/300062-bad-pattern-error-part-1
Merge into: lp:bzr
Diff against target: 0 lines
To merge this branch: bzr merge lp:~parthm/bzr/300062-bad-pattern-error-part-1
Reviewer Review Type Date Requested Status
Martin Pool Approve on 2010-07-07
John A Meinel 2010-06-30 Needs Information on 2010-06-30
Review via email: mp+28889@code.launchpad.net

Commit message

Better regex compile errors

Description of the change

=== Fix towards bug #300062 ===

Based on the discussions on the previous patches[1][2] towards bug #300062 I thought it may be better to land this fix in multiple pieces. This this part does the following:
1. lazy_regex now raises errors.InvalidPattern with a clear error message instead of re.error + stack trace
2. Globster, re-raises errors.InvalidPattern with a message indicating ignore files contain error.
3. As lazy_regex now gives a clear error, I have removed special handling of "bzr log -m PATTERN" pattern errors. This raises InvalidPattern error.

So the effect is that invalid pattern errors now show a clear error instead of a stack trace.

Following is pending and I plan to add this to part-2+.
1. `bzr ignore PATTERNS` does not reject bad pattern.
Two options that came up for this in previous discussion (for say 'bzr ignore good0 bad0 good1') were: (a) add good0 good1 to ignore file and issue warning. (b) fail and not add anything (suggested by John in [1]). I don't really have a major preference between (a) or (b). I plan to go ahead and implement (b) for uniformity if no-one raises an objection.

2. Improve InvalidPattern error in Globster to indicate which file contains what bad pattern(s). As John suggested, I will be looking into having this information associated the patterns on initial read rather than re-reading/parsing the ignore files.

3. There is some duplication of pattern handling (which was cleaned up in the _Pattern class in [2]). I will looking into doing this. I discussed this with lifeless earlier on IRC and he suggested that it may be better to enhance Globster rather than have a separate _Pattern class. I think that makes sense and will look into using that approach.

[1] https://code.edge.launchpad.net/~parthm/bzr/300062-better-handling-for-invalid-ignore-pattern
[2] https://code.edge.launchpad.net/~parthm/bzr/300062-invalid-pattern-warnings

To post a comment you must log in.
John A Meinel (jameinel) wrote :
Download full text (3.8 KiB)

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

Parth Malwankar wrote:
> Parth Malwankar has proposed merging lp:~parthm/bzr/300062-bad-pattern-error-part-1 into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #300062 give a better message for an invalid ignore regexp
> https://bugs.launchpad.net/bugs/300062
>
>
> === Fix towards bug #300062 ===
>
> Based on the discussions on the previous patches[1][2] towards bug #300062 I thought it may be better to land this fix in multiple pieces. This this part does the following:
> 1. lazy_regex now raises errors.InvalidPattern with a clear error message instead of re.error + stack trace
> 2. Globster, re-raises errors.InvalidPattern with a message indicating ignore files contain error.
> 3. As lazy_regex now gives a clear error, I have removed special handling of "bzr log -m PATTERN" pattern errors. This raises InvalidPattern error.
>
> So the effect is that invalid pattern errors now show a clear error instead of a stack trace.
>
> Following is pending and I plan to add this to part-2+.
> 1. `bzr ignore PATTERNS` does not reject bad pattern.
> Two options that came up for this in previous discussion (for say 'bzr ignore good0 bad0 good1') were: (a) add good0 good1 to ignore file and issue warning. (b) fail and not add anything (suggested by John in [1]). I don't really have a major preference between (a) or (b). I plan to go ahead and implement (b) for uniformity if no-one raises an objection.
>
> 2. Improve InvalidPattern error in Globster to indicate which file contains what bad pattern(s). As John suggested, I will be looking into having this information associated the patterns on initial read rather than re-reading/parsing the ignore files.
>
> 3. There is some duplication of pattern handling (which was cleaned up in the _Pattern class in [2]). I will looking into doing this. I discussed this with lifeless earlier on IRC and he suggested that it may be better to enhance Globster rather than have a separate _Pattern class. I think that makes sense and will look into using that approach.
>
> [1] https://code.edge.launchpad.net/~parthm/bzr/300062-better-handling-for-invalid-ignore-pattern
> [2] https://code.edge.launchpad.net/~parthm/bzr/300062-invalid-pattern-warnings
>
>

I think this is looking good, but I believe when globster re-raises the
error, it now omits the pattern. Specifically:
+ except errors.InvalidPattern, e:
+ # We can't show the default e.message to the user as thats for
+ # the combined pattern we sent to regex. Instead we indicate to
+ # the user that an ignore file needs fixing.
+ e.message = "File ~/.bazaar/ignore or .bzrignore contains
errors."
+ raise e
         return None

^- Is there a reason you can't just iterate over the individual patterns
at this point? Are you just intending to do that in a future step (as
part of determining which file the pattern came from)?

This is probably still better than what we have today, but it does mean
details will get omitted. Maybe we could trace.mutter() the original
e.message. So while it would be messy, it could still give the user a...

Read more...

review: Needs Information
5331. By Parth Malwankar on 2010-07-01

merged in changes from trunk

5332. By Parth Malwankar on 2010-07-01

deprecate re_compile_checked rather than remove it.

5333. By Parth Malwankar on 2010-07-01

Globster now mutters regex failure message before changing message

5334. By Parth Malwankar on 2010-07-01

updated NEWS with deprecation info.

Parth Malwankar (parthm) wrote :

Thanks for the review John.

> ^- Is there a reason you can't just iterate over the individual patterns
> at this point? Are you just intending to do that in a future step (as
> part of determining which file the pattern came from)?
>

I will be doing that in a follow up patch. As the patterns need to be
translated into an actual regex before they can be checked for errors, I plan
to do this as a part of removing code duplication (roughly along the lines of
'class _Pattern' in [1] but done in Globster). Suggestions welcome.

Adding file name information will probably be a separate patch.

[1] https://code.edge.launchpad.net/~parthm/bzr/300062-invalid-pattern-warnings/+merge/26809

> This is probably still better than what we have today, but it does mean
> details will get omitted. Maybe we could trace.mutter() the original
> e.message. So while it would be messy, it could still give the user a
> place to look.
>

I have added a mutter for now as suggested. The follow up patch with print more detailed error message.

>
> - -def re_compile_checked(re_string, flags=0, where=""):
>
> ^- Can we actually just remove this? I would guess we should probably
> mark it as deprecated. It exists in bzr 2.1, so it has certainly been
> present in a release.
>

Good point. re_compile_checked is now marked as deprecated and not used in
bzrlib.

Parth Malwankar (parthm) wrote :

Hi John, does this patch look ok to you?
If this looks fine then I just need to trouble another reviewer so I can land this :)
I plan to work on the follow on patches once this lands.

Martin Pool (mbp) wrote :

That looks reasonable to me, if it has addressed all John's concerns.

review: Approve
Martin Pool (mbp) wrote :

sorry for the delay, i commented earlier but it was lost

John A Meinel (jameinel) wrote :

sent to pqm by email

Parth Malwankar (parthm) wrote :

Looks like the merge didn't know through (I don't see it in the PQM and its not merged in).
I will resubmit it and check.

Parth Malwankar (parthm) wrote :

sent to pqm by email

5335. By Parth Malwankar on 2010-07-08

fixed import error in globbing.py

5336. By Parth Malwankar on 2010-07-08

updated re_compile_checked tests to handle deprecation.

Parth Malwankar (parthm) wrote :

sent to pqm by email

5337. By Parth Malwankar on 2010-07-08

re-install lazy re compile for failing test.

Parth Malwankar (parthm) wrote :

sent to pqm by email

John A Meinel (jameinel) wrote :
Download full text (5.6 KiB)

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

Parth Malwankar wrote:
> sent to pqm by email
>

Failing in PQM with:

======================================================================
ERROR: bzrlib.tests.test_globbing.TestGlobster.test_bad_pattern
- ----------------------------------------------------------------------
_StringException: Text attachment: log
- ------------

- ------------
Text attachment: traceback
- ------------
Traceback (most recent call last):
  File "/usr/lib/python2.4/site-packages/testtools/runtest.py", line
128, in _run_user
    return fn(*args)
  File "/usr/lib/python2.4/site-packages/testtools/testcase.py", line
368, in _run_test_method
    testMethod()
  File
"/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/tests/test_globbing.py",
line 316, in test_bad
pattern
    e = self.assertRaises(errors.InvalidPattern, g.match, 'foo')
  File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/tests/__init__.py",
line 1249, in assertRaises
    callableObj(*args, **kwargs)
  File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/globbing.py", line
219, in match
    except errors.InvalidPattern, e:
  File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/lazy_import.py",
line 106, in __getattribute__
    obj = _replace()
  File "/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/lazy_import.py",
line 88, in _replace
    extra=e)
IllegalUseOfScopeReplacer: ScopeReplacer object 'errors' was used
incorrectly: Object already clean
d up, did you assign it to another variable?: _factory
- ------------

======================================================================
ERROR: bzrlib.tests.test_osutils.TestReCompile.test_re_compile_checked
- ----------------------------------------------------------------------
_StringException: Text attachment: log
- ------------
4178.727 Deprecated function called
Called from:
  File "/usr/lib/python2.4/site-packages/testtools/runtest.py", line
128, in _run_user
    return fn(*args)
  File "/usr/lib/python2.4/site-packages/testtools/testcase.py", line
368, in _run_test_method
    testMethod()
  File
"/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/tests/test_osutils.py",
line 1709, in test_re_
ompile_checked
    r = osutils.re_compile_checked(r'A*', re.IGNORECASE)
  File
"/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/symbol_versioning.py",
line 95, in decorated_f
nction
    trace.mutter_callsite(4, "Deprecated function called")
- ------------
Text attachment: traceback
- ------------
Traceback (most recent call last):
  File "/usr/lib/python2.4/site-packages/testtools/runtest.py", line
128, in _run_user
    return fn(*args)
  File "/usr/lib/python2.4/site-packages/testtools/testcase.py", line
368, in _run_test_method
    testMethod()
  File
"/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/tests/test_osutils.py",
line 1709, in test_re_
ompile_checked
    r = osutils.re_compile_checked(r'A*', re.IGNORECASE)
  File
"/home/pqm/bzr-pqm-workdir/home/+trunk/bzrlib/symbol_versioning.py",
line 97, in decorated_f
nction
    DeprecationWarning, stacklevel=2)
  File "/usr/lib/python2.4/warnings.py", line 61, in warn
    warn_explicit(message, category, filename, lineno, module, registry)
  File "/usr/lib/python2.4/warnings.py", li...

Read more...

Preview Diff

Empty