From: Dave Borowitz Date: Tue, 9 Mar 2010 01:22:01 +0000 (-0800) Subject: Change check() methods in pack.py to raise rather than return bools. X-Git-Tag: dulwich-0.6.0~19^2~4 X-Git-Url: http://git.samba.org/samba.git/?a=commitdiff_plain;h=dbc9d9629d514c829d8b9b6d02a0015676fac7f1;p=jelmer%2Fdulwich-libgit2.git 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 dec7c2e..5ff6d5b 100644 --- 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 ae75d9d..b43eba9 100644 --- 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 52c66c5..3b8a27d 100644 --- 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 dfdf239..08421a9 100644 --- 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 1174945067 +0100\ncommitter James Westby 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())