Code review comment for lp:~parthm/bzr/300062-better-handling-for-invalid-ignore-pattern

I had an interesting discussion with jam regarding warning or error on irc. Unfortunately no conclusion yet :P

<jam> hi parthm
<parthm> jam: hi, i was thinking some more about the `bzr st` bad ignore pattern
i am somewhat divided between warning / error but am leaning towards error. if we have a bad pattern other commands like `add` and `ignored` would fail anyway.
<parthm> i am not sure if there are others. so it may be better to fail quickly and have the user fix it sooner than later.
<parthm> there could also be a case that a user ends up pushing a bad pattern to their server/lp. as .bzrignore is already added. its just a `bzr ci` and `bzr push`.
so failing on `st` may be a good idea.
<jam> parthm: why would add and ignore have to fail?
the point is that, especially right now, people may have a bad config
having it fall over when it used to work is bad
having it fall over *in general* is bad
I don't feel super strongly
but we've done the 'error early' in the past
and generally it has provided worse user experience
as a minor misconfiguration in X
suddenly blocks Y and Z from working at all
<parthm> jam: `ignore` works, it issues a warning and discards the bad pattern.
<jam> even if the pattern was already in the file?
<parthm> `ignored` fails as we compile all ignored patterns (99 at a time) into one pattern. so we cant selectively ignore.
<jam> you certainly *could*
<parthm> jam: yes. if a file has a bad pattern, `ignore` would still work. so it issues a warning about the pattern on the command line. if the tree is processed, then a followup error is shown about the existing bad pattern.
<jam> parthm: the problem is that if we filter out the bad pattern from the disk content we make it harder to get the right pattern
since the old one is gone
it may be a fair trade
but something to be thought about
<parthm> jam: we don't filter out the bad pattern from the file. filtering is only for the command like. we display an error message for the pattern in file.
jam: http://pastebin.com/fMWtEUjc
<jam> so for compiling, we could certainly attempt the 100-way compile and if it fails rollback
<parthm> jam: i am not sure i understand. are you talking about `ignored`?
<jam> yes
but also statu
status
add, etc
<parthm> jam: what do you mean by try the compile and rollback?
<jam> parthm: try to compile all 100, if it fails, compile them individually to find the one that failed
<parthm> jam: thats whats done in the current patch. the current patch allow the operation to happen, if there is an exception, it processes the file to find the bad pattern and displays it along with the error message.
<jam> so why does ignored *have* to fail?
<parthm> jam: so are you suggesting we retry after filtering out the bad pattern at runtime?
<jam> That is what I've been suggesting, yes
<parthm> jam: IMO once the user has a bad pattern in the file he would sooner or later end up with a problem so it may be better to just error out pointing to the pattern.
i don't think existing installation would have a bad pattern as it would make bzr crash.
<jam> parthm: the difference is whether someone can still finish what they were trying to do
*before* they have to fix everything
or if they have to drop their train of thought
and go handle this thing
<parthm> jam: i agree with your point on 'train of thought' but the results of a command e.g. `ignored` may not be what the user expects due to the skipped pattern.
<jam> parthm: that is why we give a big warning
<parthm> jam: i suppose being a ui thing there is no obviously right answer :)
jam: i will paste this chat on the merge proposal and probably others can chime in with their views.