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

[Parth Malwankar]

=== Fix towards bug #300062 ===
This is the second patch towards bug #300062. The first patch can be found at [1].
This does the following:
 1. `bzr ignore` now fails in case an invalid pattern is supplied. The bad pattern is displayed to the user.
 2. In case an invalid pattern is encountered in .bzrignore or user ignore file, the error message shown now displays the specific pattern(s) that caused the failure.
 3. Some code cleanup is done Globster.

One thing pending is that the error messages do not show what file the bad pattern came from. Ideally, I would like to show a message like "file:line_number:pattern". This requires updating the ignore file parsing logic to store line number and file name information. I plan to look at that in a separate patch. Suggestions welcome.

Sample output:
[a1234]% ~/src/bzr.dev/300062-bad-pattern-error-part-2/bzr ignore "RE:[" foo "RE:*.cpp"
bzr: error: Invalid ignore pattern(s) found.
  RE:[
  RE:*.cpp
bzr: ERROR: Invalid pattern(s) found.
[a1234]% ~/src/bzr.dev/300062-bad-pattern-error-part-2/bzr st
bzr: ERROR: Invalid pattern(s) found. File ~/.bazaar/ignore or .bzrignore contains error(s).
  RE:[
[a1234]%

[1] https://code.launchpad.net/~parthm/bzr/300062-bad-pattern-error-part-1/+merge/28889

[Parth Malwankar (parthm)]

Is 2.2b4 being considered a feature freeze or can this patch be considered for 2.2?
IMO it would be nice to have this in if possible as the user gets to see the failing pattern in the error message. The specific failing ignore file is not shown but that can be a 2.3 feature.

[Martin Pool (mbp)]

On 10 July 2010 19:37, Parth Malwankar <email address hidden> wrote:
> Is 2.2b4 being considered a feature freeze or can this patch be considered for 2.2?
> IMO it would be nice to have this in if possible as the user gets to see the failing pattern in the error message. The specific failing ignore file is not shown but that can be a 2.3 feature.

Based on the cover letter I think this would be fine for 2.2.

--
Martin

[Parth Malwankar (parthm)]

I am rejecting this since https://code.launchpad.net/~parthm/bzr/300062-2.2-bad-pattern-error-part-2/+merge/29949 proposes the same changes for 2.2

Unmerged revisions

5348. By Parth Malwankar on 2010-07-19

order of patterns added using _add_pattern is predictable

=== modified file 'NEWS'
--- NEWS	2010-07-08 10:34:12 +0000
+++ NEWS	2010-07-08 13:41:54 +0000
@@ -94,6 +94,7 @@
 
 * Invalid patterns supplied to ``Globster`` or ``lazy_regex`` now raise
   ``InvalidPattern`` exception showing clear error message to the user.
+  ``bzr ignore PATTERNS`` exits with error if a bad pattern is supplied.
   (Parth Malwankar #300062)
 
 * Progress output is cleaned up when exiting.  (Aaron Bentley)
 
=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-06-30 12:13:14 +0000
+++ bzrlib/builtins.py	2010-07-08 13:41:54 +0000
@@ -2712,6 +2712,14 @@
                 "NAME_PATTERN or --default-rules.")
         name_pattern_list = [globbing.normalize_pattern(p)
                              for p in name_pattern_list]
+        bad_patterns = ''
+        for p in name_pattern_list:
+            if not globbing.Globster.is_pattern_valid(p):
+                bad_patterns += ('\n  %s' % p)
+        if bad_patterns:
+            msg = ('Invalid ignore pattern(s) found. %s' % bad_patterns)
+            ui.ui_factory.show_error(msg)
+            raise errors.InvalidPattern('')
         for name_pattern in name_pattern_list:
             if (name_pattern[0] == '/' or
                 (len(name_pattern) > 1 and name_pattern[1] == ':')):
 
=== modified file 'bzrlib/globbing.py'
--- bzrlib/globbing.py	2010-07-08 05:11:16 +0000
+++ bzrlib/globbing.py	2010-07-08 13:41:54 +0000
@@ -179,24 +179,38 @@
     so are matched first, then the basename patterns, then the fullpath
     patterns.
     """
+    TYPE_FULLPATH = 1
+    TYPE_BASENAME = 2
+    TYPE_EXTENSION = 3
+
+    translators = {
+        TYPE_FULLPATH : _sub_fullpath,
+        TYPE_BASENAME : _sub_basename,
+        TYPE_EXTENSION : _sub_extension,
+    }
+
+    # Prefixes used to combine various patterns.
+    # See: Globster._add_patterns
+    prefixes = {
+        TYPE_FULLPATH : r'',
+        TYPE_BASENAME : r'(?:.*/)?(?!.*/)',
+        TYPE_EXTENSION : r'(?:.*/)?(?!.*/)(?:.*\.)',
+    }
+
     def __init__(self, patterns):
         self._regex_patterns = []
-        path_patterns = []
-        base_patterns = []
-        ext_patterns = []
+        pattern_lists = {
+            Globster.TYPE_FULLPATH : [],
+            Globster.TYPE_EXTENSION : [],
+            Globster.TYPE_BASENAME : [],
+        }
         for pat in patterns:
             pat = normalize_pattern(pat)
-            if pat.startswith(u'RE:') or u'/' in pat:
-                path_patterns.append(pat)
-            elif pat.startswith(u'*.'):
-                ext_patterns.append(pat)
-            else:
-                base_patterns.append(pat)
-        self._add_patterns(ext_patterns,_sub_extension,
-            prefix=r'(?:.*/)?(?!.*/)(?:.*\.)')
-        self._add_patterns(base_patterns,_sub_basename,
-            prefix=r'(?:.*/)?(?!.*/)')
-        self._add_patterns(path_patterns,_sub_fullpath)
+            pattern_lists[Globster.identify(pat)].append(pat)
+        for pattern_type, patterns in pattern_lists.iteritems():
+            self._add_patterns(patterns,
+                Globster.translators[pattern_type],
+                Globster.prefixes[pattern_type])
 
     def _add_patterns(self, patterns, translator, prefix=''):
         while patterns:
@@ -221,10 +235,51 @@
             # the combined pattern we sent to regex. Instead we indicate to
             # the user that an ignore file needs fixing.
             mutter('Invalid pattern found in regex: %s.', e.message)
-            e.message = "File ~/.bazaar/ignore or .bzrignore contains errors."
+            e.message = "File ~/.bazaar/ignore or .bzrignore contains error(s)."
+            bad_patterns = ''
+            for _, patterns in self._regex_patterns:
+                for p in patterns:
+                    if not Globster.is_pattern_valid(p):
+                        bad_patterns += ('\n  %s' % p)
+            e.message += bad_patterns
             raise e
         return None
 
+    @staticmethod
+    def identify(pattern):
+        """Returns pattern category.
+
+        :param pattern: normalized pattern.
+        Identify if a pattern is fullpath, basename or extension
+        and returns the appropriate type.
+        """
+        if pattern.startswith(u'RE:') or u'/' in pattern:
+            return Globster.TYPE_FULLPATH
+        elif pattern.startswith(u'*.'):
+            return Globster.TYPE_EXTENSION
+        else:
+            return Globster.TYPE_BASENAME
+
+    @staticmethod
+    def is_pattern_valid(pattern):
+        """Returns True if pattern is valid.
+
+        :param pattern: Normalized pattern.
+        is_pattern_valid() assumes pattern to be normalized.
+        see: globbing.normalize_pattern
+        """
+        result = True
+        translator = Globster.translators[Globster.identify(pattern)]
+        tpattern = '(%s)' % translator(pattern)
+        try:
+            re_obj = re.compile(tpattern, re.UNICODE)
+            re_obj.search("") # force compile
+        except errors.InvalidPattern, e:
+            result = False
+        return result
+
+
+
 class ExceptionGlobster(object):
     """A Globster that supports exception patterns.
     
@@ -272,14 +327,9 @@
         self._regex_patterns = []
         for pat in patterns:
             pat = normalize_pattern(pat)
-            if pat.startswith(u'RE:') or u'/' in pat:
-                self._add_patterns([pat], _sub_fullpath)
-            elif pat.startswith(u'*.'):
-                self._add_patterns([pat], _sub_extension,
-                    prefix=r'(?:.*/)?(?!.*/)(?:.*\.)')
-            else:
-                self._add_patterns([pat], _sub_basename,
-                    prefix=r'(?:.*/)?(?!.*/)')
+            pat_type = Globster.identify(pat)
+            self._add_patterns([pat], Globster.translators[pat_type],
+                Globster.prefixes[pat_type])
 
 
 _slashes = re.compile(r'[\\/]+')
 
=== modified file 'bzrlib/tests/blackbox/test_ignore.py'
--- bzrlib/tests/blackbox/test_ignore.py	2010-06-11 07:32:12 +0000
+++ bzrlib/tests/blackbox/test_ignore.py	2010-07-08 13:41:54 +0000
@@ -162,3 +162,19 @@
         tree = self.make_branch_and_tree('a')
         self.run_bzr(['ignore', '--directory=a', 'README'])
         self.check_file_contents('a/.bzrignore', 'README\n')
+
+    def test_ignored_invalid_pattern(self):
+        """Ensure graceful handling for invalid ignore pattern.
+
+        Test case for #300062.
+        Invalid pattern should show clear error message.
+        Invalid pattern should not be added to .bzrignore file.
+        """
+        tree = self.make_branch_and_tree('.')
+        out, err = self.run_bzr(['ignore', 'RE:*.cpp', 'foo', 'RE:['], 3)
+        self.assertEqual(out, '')
+        self.assertContainsRe(err,
+            'Invalid ignore pattern.*RE:\*\.cpp.*RE:\[', re.DOTALL)
+        self.assertNotContainsRe(err, 'foo', re.DOTALL)
+        self.assertFalse(os.path.isfile('.bzrignore'))
+
 
=== modified file 'bzrlib/tests/per_workingtree/test_is_ignored.py'
--- bzrlib/tests/per_workingtree/test_is_ignored.py	2010-02-17 17:11:16 +0000
+++ bzrlib/tests/per_workingtree/test_is_ignored.py	2010-07-08 13:41:54 +0000
@@ -22,6 +22,16 @@
 
 class TestIsIgnored(TestCaseWithWorkingTree):
 
+    def _set_user_ignore_content(self, ignores):
+        """Create user ignore file and set its content to ignores."""
+        config.ensure_config_dir_exists()
+        user_ignore_file = config.user_ignore_config_filename()
+        f = open(user_ignore_file, 'wb')
+        try:
+            f.write(ignores)
+        finally:
+            f.close()
+
     def test_is_ignored(self):
         tree = self.make_branch_and_tree('.')
         # this will break if a tree changes the ignored format. That is fine
@@ -46,6 +56,11 @@
                            '#comment\n'
                            ' xx \n' # whitespace
             )])
+        # We set user ignore file to contain '' to avoid patterns from
+        # user ignore being used instead of bzrignore. For .e.g. If we
+        # don't do this 'foo.~1~' will match '*~' default user ignore
+        # pattern instead of '*.~*' from bzr ignore as we expect below.
+        self._set_user_ignore_content('')
         # is_ignored returns the matching ignore regex when a path is ignored.
         # we check some expected matches for each rule, and one or more
         # relevant not-matches that look plausible as cases for bugs.
@@ -119,19 +134,16 @@
 
         config.ensure_config_dir_exists()
         user_ignore_file = config.user_ignore_config_filename()
-        f = open(user_ignore_file, 'wb')
-        try:
-            f.write('*.py[co]\n'
-                    './.shelf\n'
-                    '# comment line\n'
-                    '\n' #Blank line
-                    '\r\n' #Blank dos line
-                    ' * \n' #Trailing and suffix spaces
-                    'crlf\r\n' # dos style line
-                    '*\xc3\xa5*\n' # u'\xe5'.encode('utf8')
-                    )
-        finally:
-            f.close()
+        self._set_user_ignore_content(
+            '*.py[co]\n'
+            './.shelf\n'
+            '# comment line\n'
+            '\n' #Blank line
+            '\r\n' #Blank dos line
+            ' * \n' #Trailing and suffix spaces
+            'crlf\r\n' # dos style line
+            '*\xc3\xa5*\n' # u'\xe5'.encode('utf8')
+            )
 
         # Rooted
         self.assertEqual('./.shelf', tree.is_ignored('.shelf'))
 
=== modified file 'bzrlib/tests/test_globbing.py'
--- bzrlib/tests/test_globbing.py	2010-06-30 13:49:02 +0000
+++ bzrlib/tests/test_globbing.py	2010-07-08 13:41:54 +0000
@@ -15,6 +15,8 @@
 # along with this program; if not, write to the Free Software
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
 
+import re
+
 from bzrlib import errors
 from bzrlib.globbing import (
     Globster,
@@ -311,10 +313,11 @@
 
     def test_bad_pattern(self):
         """Ensure that globster handles bad patterns cleanly."""
-        patterns = [u'RE:[']
+        patterns = [u'RE:[', u'foo', u'RE:*.cpp']
         g = Globster(patterns)
         e = self.assertRaises(errors.InvalidPattern, g.match, 'foo')
-        self.assertContainsRe(e.message, "File.*ignore.*contains errors")
+        self.assertContainsRe(e.message,
+            "File.*ignore.*contains error.*RE:\[.*RE:\*\.cpp", flags=re.DOTALL)
 
 
 class TestExceptionGlobster(TestCase):