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

diff --git a/dulwich/errors.py b/dulwich/errors.py
index dec7c2e0aa7a04447a7926ef17dcfe22ed6f0783..5ff6d5b9105a550339e6de792d83962a10c081c5 100644 (file)
--- a/dulwich/errors.py
+++ b/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,

diff --git a/dulwich/pack.py b/dulwich/pack.py
index ae75d9db6b95aace215e5ac727e9575491294182..b43eba9360be513642826654cf30d10d98104200 100644 (file)
--- a/dulwich/pack.py
+++ b/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()

diff --git a/dulwich/tests/compat/test_pack.py b/dulwich/tests/compat/test_pack.py
index 52c66c5c863e9f540b47f778d509b9f865c2e85c..3b8a27d03d40915ddddeba8604814c86673c97d4 100644 (file)
--- a/dulwich/tests/compat/test_pack.py
+++ b/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))

diff --git a/dulwich/tests/test_pack.py b/dulwich/tests/test_pack.py
index dfdf23954c09733133ca1f0fe51b04d904d4666f..08421a9c4e0a491ad24ace33b3742a5c12bd3300 100644 (file)
--- a/dulwich/tests/test_pack.py
+++ b/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())