repo: Raise RefFormatException instead of KeyError on bad refs.

Previously, bad refnames were being interpreted by _follow as broken
symlinks and being written through regardless. This caused unexpected
errors on reading refs; now we will fail faster and with a clearer error
message.

Fixes bug #653527.

Change-Id: I74fedb37572413ebdcf360b1512fa7e74bb6d4a7
Signed-off-by: Jelmer Vernooij <jelmer@samba.org>

dulwich/errors.py

@@ -166,3 +166,7 @@ class NoIndexPresent(Exception):
 
 class CommitError(Exception):
     """An error occurred while performing a commit."""
+
+
+class RefFormatError(Exception):
+    """Indicates an invalid ref name."""

dulwich/repo.py

@@ -35,6 +35,7 @@ from dulwich.errors import (
     NotTagError,
     PackedRefsException,
     CommitError,
+    RefFormatError,
     )
 from dulwich.file import (
     ensure_dir_exists,
@@ -213,7 +214,7 @@ class RefsContainer(object):
         if name == 'HEAD':
             return
         if not name.startswith('refs/') or not check_ref_format(name[5:]):
-            raise KeyError(name)
+            raise RefFormatError(name)
 
     def read_ref(self, refname):
         """Read a reference without following any references.
@@ -990,7 +991,10 @@ class BaseRepo(object):
                 return self.object_store[name]
             except KeyError:
                 pass
-        return self.object_store[self.refs[name]]
+        try:
+            return self.object_store[self.refs[name]]
+        except RefFormatError:
+            raise KeyError(name)
 
     def __contains__(self, name):
         if len(name) in (20, 40):

dulwich/tests/test_repository.py

@@ -617,18 +617,13 @@ class RefsContainerTests(object):
                          self._refs['refs/heads/symbolic'])
 
     def test_check_refname(self):
-        try:
-            self._refs._check_refname('HEAD')
-        except KeyError:
-            self.fail()
-
-        try:
-            self._refs._check_refname('refs/heads/foo')
-        except KeyError:
-            self.fail()
+        self._refs._check_refname('HEAD')
+        self._refs._check_refname('refs/heads/foo')
 
-        self.assertRaises(KeyError, self._refs._check_refname, 'refs')
-        self.assertRaises(KeyError, self._refs._check_refname, 'notrefs/foo')
+        self.assertRaises(errors.RefFormatError, self._refs._check_refname,
+                          'refs')
+        self.assertRaises(errors.RefFormatError, self._refs._check_refname,
+                          'notrefs/foo')
 
     def test_contains(self):
         self.assertTrue('refs/heads/master' in self._refs)
@@ -753,7 +748,8 @@ class DiskRefsContainerTests(RefsContainerTests, TestCase):
         self.assertEquals(
           ('refs/heads/master', '42d06bd4b77fed026b154d16493e5deab78f02ec'),
           self._refs._follow('refs/heads/master'))
-        self.assertRaises(KeyError, self._refs._follow, 'notrefs/foo')
+        self.assertRaises(errors.RefFormatError, self._refs._follow,
+                          'notrefs/foo')
         self.assertRaises(KeyError, self._refs._follow, 'refs/heads/loop')
 
     def test_delitem(self):