Trap exceptions in Subversion backend when revision number is negative

[Ryan J Ollos]

Invalid revisions such as a are trapped in the Subversion backend and a NoSuchChangeset exception is raised. However a negative revision number is not trapped and the SubversionException propagates. We should trap the exception and raise a NoSuchChangeset exception.

20:47:11 Trac[formatter] ERROR: Macro Include(source:proj1/workflow@-1) failed:
Traceback (most recent call last):
  File "/Users/rjollos/Documents/Workspace/th-dev/IncludeMacro/pve/lib/python2.7/site-packages/trac/wiki/formatter.py", line 789, in _macro_formatter
    return macro.ensure_inline(macro.process(args))
  File "/Users/rjollos/Documents/Workspace/th-dev/IncludeMacro/pve/lib/python2.7/site-packages/trac/wiki/formatter.py", line 358, in process
    text = self.processor(text)
  File "/Users/rjollos/Documents/Workspace/th-dev/IncludeMacro/pve/lib/python2.7/site-packages/trac/wiki/formatter.py", line 345, in _macro_processor
    text)
  File "/Users/rjollos/Documents/Workspace/th-dev/IncludeMacro/trunk/includemacro/macros.py", line 130, in expand_macro
    dest_format)
  File "/Users/rjollos/Documents/Workspace/th-dev/IncludeMacro/trunk/includemacro/macros.py", line 202, in _get_source
    node = repos.get_node(path, rev)
  File "/Users/rjollos/Documents/Workspace/th-dev/IncludeMacro/pve/lib/python2.7/site-packages/trac/versioncontrol/cache.py", line 298, in get_node
    return self.repos.get_node(path, self.normalize_rev(rev))
  File "/Users/rjollos/Documents/Workspace/th-dev/IncludeMacro/pve/lib/python2.7/site-packages/tracopt/versioncontrol/svn/svn_fs.py", line 511, in get_node
    return SubversionNode(path, rev, self, self.pool)
  File "/Users/rjollos/Documents/Workspace/th-dev/IncludeMacro/pve/lib/python2.7/site-packages/tracopt/versioncontrol/svn/svn_fs.py", line 768, in __init__
    self.root = fs.revision_root(self.fs_ptr, rev, pool)
  File "/Users/rjollos/Documents/Workspace/th-dev/IncludeMacro/pve/lib/python2.7/site-packages/libsvn/fs.py", line 319, in svn_fs_revision_root
    return _fs.svn_fs_revision_root(*args)
SubversionException: 160006 - Invalid revision number '-1'

[Ryan J Ollos]

Fixed on 1.0-stable in [14454] , merged to trunk in [14455].

[Jun Omae]

I think we should raise NoSuchChangeset/NoSuchNode rather than TracError from SubversionRepository.get_node and SubversionNode.__init__.

tracopt/versioncontrol/svn/svn_fs.py

@@ -258 +258 @@
     def rev_db(self, rev):
         return int(rev or 0)
 
+    def normalize_rev(self, rev):
+        return self.repos.normalize_rev(rev)
+
 
 class SubversionConnector(Component):
 
@@ -429 +432 @@
         else:
             try:
                 rev = int(rev)
-                if rev <= self.youngest_rev:
+                if 0 <= rev <= self.youngest_rev:
                     return rev
             except (ValueError, TypeError):
                 pass
@@ -768 +771 @@
             try:
                 self.root = fs.revision_root(self.fs_ptr, rev, pool)
             except core.SubversionException, e:
-                raise TracError(e)
+                raise NoSuchNode(path, rev, exception_to_unicode(e))
         node_type = fs.check_path(self.root, self._scoped_path_utf8, pool)
         if not node_type in _kindmap:
             raise NoSuchNode(path, rev)

tracopt/versioncontrol/svn/tests/svn_fs.py

@@ -130 +130 @@
         self.assertEqual(HEAD, self.repos.normalize_rev(None))
         self.assertEqual(11, self.repos.normalize_rev('11'))
         self.assertEqual(11, self.repos.normalize_rev(11))
+        self.assertRaises(NoSuchChangeset, self.repos.normalize_rev, -1)
+        self.assertRaises(NoSuchChangeset, self.repos.normalize_rev, -42)
 
     def test_rev_navigation(self):
         self.assertEqual(1, self.repos.oldest_rev)
@@ -169 +171 @@
         self.assertEqual(HEAD, node.created_rev)
         self.assertEqual(datetime(2015, 6, 15, 14, 9, 13, 664490, utc),
                          node.last_modified)
-        self.assertRaises(TracError, self.repos.get_node, u'/', -1)
+        self.assertRaises(NoSuchChangeset, self.repos.get_node, u'/', -1)
         node = self.repos.get_node(u'/tête')
         self.assertEqual(u'tête', node.name)
         self.assertEqual(u'/tête', node.path)

[Ryan J Ollos]

Thanks, I should have thought to use the NoSuchChangeset exception. I'll push the changes later today.

[Ryan J Ollos]

comment:2 changes committed in [14469,14471], merged in [14470,14472].