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


...

>> It means that anyone using Globster for anything that isn't *bzr* (imagine
>> qbzr using it for something) is going to get strange results. (As it will tell
>> them about problems with something that isn't related at all.)
>>
> 
> One reason for doing this was that the invalid pattern could come either from .bazaar/ignore of .bzrignore and I wanted to display a message indicating the bad pattern along with the specific file information. E.g.

Could we just include this information as part of the Globster from the
beginning? I understand that knowing where the pattern is bad is
helpful. I'm just concerned that it is using state after-the-fact to
debug a current failure. And anything that isn't *bzr* is likely to be
wrong here.

> 
> [a1234]% ~/src/bzr.dev/300062-better-handling-for-invalid-ignore-pattern/bzr st
> bzr: error: Invalid pattern(s) found in "/home/parthm/tmp/a1234/.bzrignore":
>   RE:*.x
> bzr: error: Invalid pattern(s) found in "/home/parthm/.bazaar/ignore":
>   RE:[
> bzr: ERROR: Invalid pattern(s) found.
> [a1234]% 
> 
> We could use the Globster state to display the message but the user would need to check both files. I would expect that the probability of an error in .bzrignore is higher but it just seems nicer to indicate the file.
> Thoughts?

Pass in the name of the file associated with each pattern as part of
creating the Globster. Alternatively just give the short 'invalid
pattern found'. Better (IMO) than re-parsing the files.

...

>>
>> ^- You make it so that 'ignores.add_unique_user_ignores()' allows an invalid
>> pattern. It seems more logical to fault that at insert time, rather than at
>> 'bzr add' time. (We probably still need to fail there, but we should first
>> forbid people from adding broken entries in the first place.)
>>
> 
> This is done in cmd_ignore.run. This was the "Skipped invalid pattern(s)" warning. So the bad pattern doesn't reach this point. This test case injects the bad pattern to handle the case of when the user might add the bad pattern using and external editor.
> 
> 

It seems better to me to make our internals robust and hard to subvert
accidentally, rather than assuming that will be done at the cmd_* layer.
Certainly qbzr or explorer is likely to end up with their own UI for
adding ignores, and it would be nice if they could also have invalid
ignores captured early rather than late. (Ideally without having to
re-implement it.)

> 
> 
>> ^- I would probably say that we should fail all insertions if one is invalid,
>> but we would want it to be atomic. eg if the do 'bzr ignore valid valid
>> invalid valid' either all valids get inserted [as you have today] or nothing
>> gets inserted. You don't want to add the first two, and then fail, as it
>> leaves things in an unknown state.
>>
>> I would probably tend towards 'validate early and fail', as long as we are
>> choosing that for other commands. (I would still like to see status try even
>> if it gets a bogus ignore, but I understand why Robert feels so strongly about
>> 'add' even if I probably disagree.)
>>
>> Also, it is a little bit odd that if you *have* an invalid entry it fails with
>> an error, but if you try to insert one it just gives you a warning that it
>> skipped it. (maybe one of those is 'ignored' vs 'ignore'.)
>>
> 
> For uniformity I could make ignore fail if it contains a bad pattern. At the moment, it filters out the bad pattern and adds the valid ones.
> 

Well this is my opinion, and I don't think we've discussed it to reach a
consensus.

'bzr ignore' doesn't feel like a command that needs to succeed in the
face of potential confusion. (Unlike 'bzr status' or 'bzr revert' where
the user is actually trying to accomplish something, and getting tripped
up by an ignore would be annoying.) 'bzr commit' could go either way for
me (ignores don't actually change what gets committed, since that is
only determined during the add step).

>>
>> 5) So overall I think this is going in the right direction, but the layering
>> for 'what patterns are invalid' seems a bit incorrect. And the formatting and
>> raising of the exceptions also doesn't seem correct.
> 
> 

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

iEYEARECAAYFAkwqFtQACgkQJdeBCYSNAAOCsACfZTeokZHBCuT3n7afiU/GjFPV
KsoAoLl4T4wRSABFTFMN9Nx/KAsxm3X6
=afhG
-----END PGP SIGNATURE-----