Change check() methods in pack.py to raise rather than return bools.

This makes Pack, PackIndex, and PackData consistent with the rest of
the check() methods recently added. Raising rather than returning makes
factoring out methods easier and exposes more information about the kind
of error that occurred. Moreover, in many cases, the correct default
behavior is probably to die anyway.

Change-Id: I87ab2b1e165c3c9e55727b25a49b1754c0ac4534

dulwich/errors.py

@@ -19,15 +19,22 @@
 
 """Dulwich-related exception classes and utility functions."""
 
+import binascii
+
+
 class ChecksumMismatch(Exception):
     """A checksum didn't match the expected contents."""
 
     def __init__(self, expected, got, extra=None):
+        if len(expected) == 20:
+            expected = binascii.hexlify(expected)
+        if len(got) == 20:
+            got = binascii.hexlify(got)
         self.expected = expected
         self.got = got
         self.extra = extra
         if self.extra is None:
-            Exception.__init__(self, 
+            Exception.__init__(self,
                 "Checksum mismatch: Expected %s, got %s" % (expected, got))
         else:
             Exception.__init__(self,

dulwich/pack.py

@@ -317,7 +317,10 @@ class PackIndex(object):
     def check(self):
         """Check that the stored checksum matches the actual checksum."""
         # TODO: Check pack contents, too
-        return self.calculate_checksum() == self.get_stored_checksum()
+        actual = self.calculate_checksum()
+        stored = self.get_stored_checksum()
+        if actual != stored:
+            raise ChecksumMismatch(stored, actual)
 
     def calculate_checksum(self):
         """Calculate the SHA1 checksum over this pack index.
@@ -730,7 +733,10 @@ class PackData(object):
 
     def check(self):
         """Check the consistency of this pack."""
-        return (self.calculate_checksum() == self.get_stored_checksum())
+        actual = self.calculate_checksum()
+        stored = self.get_stored_checksum()
+        if actual != stored:
+            raise ChecksumMismatch(stored, actual)
 
     def get_object_at(self, offset):
         """Given an offset in to the packfile return the object that is there.
@@ -1222,12 +1228,12 @@ class Pack(object):
         return iter(self.index)
 
     def check(self):
-        """Check the integrity of this pack."""
-        if not self.index.check():
-            return False
-        if not self.data.check():
-            return False
-        return True
+        """Check the integrity of this pack.
+
+        :raise ChecksumMismatch: if a checksum for the index or data is wrong
+        """
+        self.index.check()
+        self.data.check()
 
     def get_stored_checksum(self):
         return self.data.get_stored_checksum()

dulwich/tests/compat/test_pack.py

@@ -52,7 +52,7 @@ class TestPack(PackTests):
 
     def test_copy(self):
         origpack = self.get_pack(pack1_sha)
-        self.assertEquals(True, origpack.index.check())
+        self.assertSucceeds(origpack.index.check)
         pack_path = os.path.join(self._tempdir, "Elch")
         write_pack(pack_path, [(x, "") for x in origpack.iterobjects()],
                    len(origpack))

dulwich/tests/test_pack.py

@@ -26,6 +26,9 @@ import os
 import unittest
 import zlib
 
+from dulwich.errors import (
+    ChecksumMismatch,
+    )
 from dulwich.objects import (
     Tree,
     )
@@ -65,6 +68,12 @@ class PackTests(unittest.TestCase):
     def get_pack(self, sha):
         return Pack(os.path.join(self.datadir, 'pack-%s' % sha))
 
+    def assertSucceeds(self, func, *args, **kwargs):
+        try:
+            func(*args, **kwargs)
+        except ChecksumMismatch, e:
+            self.fail(e)
+
 
 class PackIndexTests(PackTests):
     """Class that tests the index of packfiles"""
@@ -85,11 +94,11 @@ class PackIndexTests(PackTests):
         p = self.get_pack_index(pack1_sha)
         self.assertEquals("\xf2\x84\x8e*\xd1o2\x9a\xe1\xc9.;\x95\xe9\x18\x88\xda\xa5\xbd\x01", str(p.get_stored_checksum()))
         self.assertEquals( 'r\x19\x80\xe8f\xaf\x9a_\x93\xadgAD\xe1E\x9b\x8b\xa3\xe7\xb7' , str(p.get_pack_checksum()))
-  
+
     def test_index_check(self):
         p = self.get_pack_index(pack1_sha)
-        self.assertEquals(True, p.check())
-  
+        self.assertSucceeds(p.check)
+
     def test_iterentries(self):
         p = self.get_pack_index(pack1_sha)
         self.assertEquals([('og\x0c\x0f\xb5?\x94cv\x0br\x95\xfb\xb8\x14\xe9e\xfb \xc8', 178, None), ('\xb2\xa2vj(y\xc2\t\xab\x11v\xe7\xe7x\xb8\x1a\xe4"\xee\xaa', 138, None), ('\xf1\x8f\xaa\x16S\x1a\xc5p\xa3\xfd\xc8\xc7\xca\x16h%H\xda\xfd\x12', 12, None)], list(p.iterentries()))
@@ -134,11 +143,11 @@ class TestPackData(PackTests):
     def test_pack_len(self):
         p = self.get_pack_data(pack1_sha)
         self.assertEquals(3, len(p))
-  
+
     def test_index_check(self):
         p = self.get_pack_data(pack1_sha)
-        self.assertEquals(True, p.check())
-  
+        self.assertSucceeds(p.check)
+
     def test_iterobjects(self):
         p = self.get_pack_data(pack1_sha)
         self.assertEquals([(12, 1, 'tree b2a2766a2879c209ab1176e7e778b81ae422eeaa\nauthor James Westby <jw+debian@jameswestby.net> 1174945067 +0100\ncommitter James Westby <jw+debian@jameswestby.net> 1174945067 +0100\n\nTest commit\n', 3775879613L), (138, 2, '100644 a\x00og\x0c\x0f\xb5?\x94cv\x0br\x95\xfb\xb8\x14\xe9e\xfb \xc8', 912998690L), (178, 3, 'test 1\n', 1373561701L)], [(len, type, "".join(chunks), offset) for (len, type, chunks, offset) in p.iterobjects()])
@@ -195,16 +204,16 @@ class TestPack(PackTests):
 
     def test_copy(self):
         origpack = self.get_pack(pack1_sha)
-        self.assertEquals(True, origpack.index.check())
+        self.assertSucceeds(origpack.index.check)
         write_pack("Elch", [(x, "") for x in origpack.iterobjects()], 
             len(origpack))
         newpack = Pack("Elch")
         self.assertEquals(origpack, newpack)
-        self.assertEquals(True, newpack.index.check())
+        self.assertSucceeds(newpack.index.check)
         self.assertEquals(origpack.name(), newpack.name())
         self.assertEquals(origpack.index.get_pack_checksum(), 
                           newpack.index.get_pack_checksum())
-        
+
         self.assertTrue(
                 (origpack.index.version != newpack.index.version) or
                 (origpack.index.get_stored_checksum() == newpack.index.get_stored_checksum()))
@@ -232,11 +241,17 @@ class TestHexToSha(unittest.TestCase):
 
 class BaseTestPackIndexWriting(object):
 
+    def assertSucceeds(self, func, *args, **kwargs):
+        try:
+            func(*args, **kwargs)
+        except ChecksumMismatch, e:
+            self.fail(e)
+
     def test_empty(self):
         pack_checksum = 'r\x19\x80\xe8f\xaf\x9a_\x93\xadgAD\xe1E\x9b\x8b\xa3\xe7\xb7'
         self._write_fn("empty.idx", [], pack_checksum)
         idx = load_pack_index("empty.idx")
-        self.assertTrue(idx.check())
+        self.assertSucceeds(idx.check)
         self.assertEquals(idx.get_pack_checksum(), pack_checksum)
         self.assertEquals(0, len(idx))
 
@@ -247,7 +262,7 @@ class BaseTestPackIndexWriting(object):
         self._write_fn("single.idx", my_entries, pack_checksum)
         idx = load_pack_index("single.idx")
         self.assertEquals(idx.version, self._expected_version)
-        self.assertTrue(idx.check())
+        self.assertSucceeds(idx.check)
         self.assertEquals(idx.get_pack_checksum(), pack_checksum)
         self.assertEquals(1, len(idx))
         actual_entries = list(idx.iterentries())