Code review comment for lp:~parthm/bzr/300062-bad-pattern-error-part-1

Revision history for this message
John A Meinel (jameinel) wrote :

-----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
place to look.

- -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.

I don't *know* that any 3rd party is using it, but it seems nicer to
deprecate when possible, rather than remove.

 review: needsinformation

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

iEYEARECAAYFAkwrsnEACgkQJdeBCYSNAAOOYwCeIMBXdDD/p2OzRxcMvDMug/6l
1A8An2TceYZ5QRYVy0+fdaYy3T6EWU7b
=LZuh
-----END PGP SIGNATURE-----

review: Needs Information

« Back to merge proposal