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

1) It might be clearer to resubmit this, since it has changed fairly significantly since the beginning. (So I don't think Robert's Needs Fixing still directly applies.)

2)
61 +class InvalidPattern(BzrError):
62 +
63 + _fmt = ('Invalid pattern(s) found.')
64 +
65 + def __init__(self, message):
66 + self.message = message
67 +

^- You set a message, but then never display it (there is nothing in _fmt which shows the attributes.)

I suppose in builtins you do:
29 + if invalid_patterns:
30 + pats = '\n '.join(invalid_patterns)
31 + ui.ui_factory.show_warning(
32 + 'Skipped invalid pattern(s):\n %s' % pats)
^- which explicitly displays the bad patterns, but then you do so in a warning and not an error. Which seems to disagree with your "now raises exceptions" comment.

3) It seems very strange to me that 'show_pattern_errors' is a module-level function that re-reads the ignore files and reports on them.

Rather than, for example, having the state of the Globster used to determine what patterns are invalid.

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

4) so for the construct here:
105 + except errors.InvalidPattern, e:
106 + # lazy_regex throws InvalidPattern in case of bad pattern.
107 + # For Globbing, the bad pattern may have come from an ignore
108 + # file, so we should any bad patterns that the ignore file(s)
109 + # may have.
110 + show_pattern_errors()
111 + raise errors.InvalidPattern(str(e))

It seems better to me to do the formatting and put that on the errors.InvalidPattern that is getting raised, rather than calling show_pattern_errors() and having that display.

I also find it strange to catch InvalidPattern and then raise another InvalidPattern(str(first_e))

Especially given your formatting for InvalidPattern this would become:
    InvalidPattern('Invalid pattern(s) found.')

Which would lose any sort of context that the original error tried to pass on. (So if a low level exception raised InvalidPattern('pattern XXX is broken') then you catch it and re-raise a InvalidPattern('Invalid pattern(s) found.') which is pretty unhelpful.)

5) In this test:
351 + def test_add_invalid_ignore_in_global_ignore(self):
352 + """add should handle invalid global ignore pattern gracefully.
353 + """
354 + tree = self.make_branch_and_tree('.')
355 + self.build_tree(['top.txt'])
356 + self.build_tree_contents([('foobar', 'foo\n')])
357 + ignores.add_unique_user_ignores([u"RE:*.cpp"]) # invalid pattern
358 + out, err = self.run_bzr(['add'], 3)
359 + self.assertEquals(out, '')
360 + self.assertContainsRe(err,
361 + ('error: Invalid pattern.*RE:\*\.cpp.*ERROR: '
362 + 'Invalid pattern'),
363 + re.DOTALL)
364 +

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

388 + def test_invalid_pattern_in_ignore_file(self):
389 + """Existing invalid patterns should be shown.
390 + """
391 + tree = self.make_branch_and_tree('.')
392 + self.build_tree(['file'])
393 + self.build_tree_contents([('.bzrignore', 'RE:[\n')])
394 + out, err = self.run_bzr(['ignore', 'RE:*.cpp', 'abc'], 3)
395 + self.assertEqual(out, '')
396 + self.assertContainsRe(err, 'warning: Skipped invalid pattern'
397 + '.*RE:\*\.cpp.*'
398 + 'error: Invalid pattern.*RE:\[', re.DOTALL)
399 + f = open('.bzrignore', 'r')
400 + try:
401 + lines = f.readlines()
402 + self.assertTrue('abc\n' in lines)
403 + finally:
404 + f.close()
405 +

^- 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'.)

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.