This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4654

error: introduce StorageError
ClosedPublic

Authored by indygreg on Sep 18 2018, 8:06 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGcb65d4b7e429: error: introduce StorageError
Summary

Errors in revlogs are often represented by RevlogError. It's fine
for revlogs to raise a revlog-specific exception. But in the context
of multiple storage backends, it doesn't make sense to be throwing or
catching an exception with "revlog" in its name when revlogs may not
even be in play.

This commit introduces a new generic StorageError type for representing
errors in the storage layer.

RevlogError is an instance of this type.

Interface documentation and tests referencing RevlogError has been
updated to specify StorageError should be used.

.. api::

``error.StorageError`` has been introduced to represent errors in
storage. It should be used in place of ``error.RevlogError`` unless
the error is known to come from a revlog.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

indygreg created this revision.Sep 18 2018, 8:06 PM
yuja added a subscriber: yuja.Sep 23 2018, 6:11 AM

-class RevlogError(Hint, Exception):
+class StorageError(Hint, Exception):
+ """Raised when an error occurs in a storage layer.
+
+ Usually subclassed by a storage-specific exception.
+ """
+ bytes = _tobytes
+
+class RevlogError(StorageError):

__bytes__ = _tobytes

Perhaps LookupError will have to be either reparented to StorageError
or detached from the storage-type error. Just a note. I think you would have
some plan.

Closed by commit rHGcb65d4b7e429: error: introduce StorageError (authored by indygreg). · Explain WhySep 23 2018, 6:13 AM
This revision was automatically updated to reflect the committed changes.
indygreg added a comment.Sep 24 2018, 11:56 AM
In D4654#71289, @yuja wrote:

-class RevlogError(Hint, Exception):
+class StorageError(Hint, Exception):
+ """Raised when an error occurs in a storage layer.
+
+ Usually subclassed by a storage-specific exception.
+ """
+ bytes = _tobytes
+
+class RevlogError(StorageError):

__bytes__ = _tobytes

Perhaps LookupError will have to be either reparented to StorageError
or detached from the storage-type error. Just a note. I think you would have
some plan.

I, uh, missed that LookupError inherited from RevlogError. I think it should inherit from StorageError directly.

Ideally, I think it shouldn't be part of StorageError. But I suspect a number of except StorageError will need updated to consider that. I'm not willing to scope bloat that work... yet.

I attempted the trivial reparenting to StorageError and this broke a test:

--- /home/gps/src/hg/tests/test-revset.t
+++ /home/gps/src/hg/tests/test-revset.t.err
@@ -1941,7 +1941,7 @@
   2:fffb6093b00943f91034b9bdad069402c834e572 fffb6
   3:fff48a9b9de34a4d64120c29548214c67980ade3 fff4
   4:ffff85cff0ff78504fcdc3c0bc10de0c65379249 ffff8
-  2147483647:ffffffffffffffffffffffffffffffffffffffff fffff
+  2147483647:ffffffffffffffffffffffffffffffffffffffff ffffffffffffffffffffffffffffffffffffffff
   $ hg debugobsolete fffbae3886c8fbb2114296380d276fd37715d571
   obsoleted 1 changesets

Why, I'm not exactly sure. Perhaps an overzealous except RevlogError in revlog.py somewhere? I may track this down and fix it...

yuja added a comment.Sep 25 2018, 9:36 AM
    • /home/gps/src/hg/tests/test-revset.t +++ /home/gps/src/hg/tests/test-revset.t.err @@ -1941,7 +1941,7 @@ 2:fffb6093b00943f91034b9bdad069402c834e572 fffb6 3:fff48a9b9de34a4d64120c29548214c67980ade3 fff4 4:ffff85cff0ff78504fcdc3c0bc10de0c65379249 ffff8
  • 2147483647:ffffffffffffffffffffffffffffffffffffffff fffff + 2147483647:ffffffffffffffffffffffffffffffffffffffff ffffffffffffffffffffffffffffffffffffffff $ hg debugobsolete fffbae3886c8fbb2114296380d276fd37715d571 obsoleted 1 changesets

It should be fixed by D4735.

Revision Contents
Changeset List

PathPackages
Mmercurial/error.py (9 lines)
Mmercurial/repository.py (2 lines)
Mmercurial/testing/storage.py (20 lines)

Diff 11280

mercurial/error.py

This should come first in the inheritance list to consume a hint and This should come first in the inheritance list to consume a hint and
pass remaining arguments to the exception class. pass remaining arguments to the exception class.
""" """
def __init__(self, *args, **kw): def __init__(self, *args, **kw):
self.hint = kw.pop(r'hint', None) self.hint = kw.pop(r'hint', None)
super(Hint, self).__init__(*args, **kw) super(Hint, self).__init__(*args, **kw)
class RevlogError(Hint, Exception): class StorageError(Hint, Exception):
"""Raised when an error occurs in a storage layer.
Usually subclassed by a storage-specific exception.
"""
__bytes__ = _tobytes
class RevlogError(StorageError):
__bytes__ = _tobytes __bytes__ = _tobytes
class FilteredIndexError(IndexError): class FilteredIndexError(IndexError):
__bytes__ = _tobytes __bytes__ = _tobytes
class LookupError(RevlogError, KeyError): class LookupError(RevlogError, KeyError):
def __init__(self, name, index, message): def __init__(self, name, index, message):
self.name = name self.name = name

mercurial/repository.py

Any metadata is excluded from size measurements. Use ``rawsize()`` if Any metadata is excluded from size measurements. Use ``rawsize()`` if
metadata size is important. metadata size is important.
""" """
def checkhash(fulltext, node, p1=None, p2=None, rev=None): def checkhash(fulltext, node, p1=None, p2=None, rev=None):
"""Validate the stored hash of a given fulltext and node. """Validate the stored hash of a given fulltext and node.
Raises ``error.RevlogError`` is hash validation fails. Raises ``error.StorageError`` is hash validation fails.
""" """
def revision(node, raw=False): def revision(node, raw=False):
""""Obtain fulltext data for a node. """"Obtain fulltext data for a node.
By default, any storage transformations are applied before the data By default, any storage transformations are applied before the data
is returned. If ``raw`` is True, non-raw storage transformations is returned. If ``raw`` is True, non-raw storage transformations
are not applied. are not applied.

mercurial/testing/storage.py

for i in range(-5, 5): for i in range(-5, 5):
if i == nullrev: if i == nullrev:
continue continue
with self.assertRaises(IndexError): with self.assertRaises(IndexError):
f.size(i) f.size(i)
with self.assertRaises(error.RevlogError): with self.assertRaises(error.StorageError):
f.checkhash(b'', nullid) f.checkhash(b'', nullid)
with self.assertRaises(error.LookupError): with self.assertRaises(error.LookupError):
f.checkhash(b'', b'\x01' * 20) f.checkhash(b'', b'\x01' * 20)
self.assertEqual(f.revision(nullid), b'') self.assertEqual(f.revision(nullid), b'')
self.assertEqual(f.revision(nullid, raw=True), b'') self.assertEqual(f.revision(nullid, raw=True), b'')
self.assertEqual(f.size(0), len(fulltext)) self.assertEqual(f.size(0), len(fulltext))
with self.assertRaises(IndexError): with self.assertRaises(IndexError):
f.size(1) f.size(1)
f.checkhash(fulltext, node) f.checkhash(fulltext, node)
f.checkhash(fulltext, node, nullid, nullid) f.checkhash(fulltext, node, nullid, nullid)
with self.assertRaises(error.RevlogError): with self.assertRaises(error.StorageError):
f.checkhash(fulltext + b'extra', node) f.checkhash(fulltext + b'extra', node)
with self.assertRaises(error.RevlogError): with self.assertRaises(error.StorageError):
f.checkhash(fulltext, node, b'\x01' * 20, nullid) f.checkhash(fulltext, node, b'\x01' * 20, nullid)
with self.assertRaises(error.RevlogError): with self.assertRaises(error.StorageError):
f.checkhash(fulltext, node, nullid, b'\x01' * 20) f.checkhash(fulltext, node, nullid, b'\x01' * 20)
self.assertEqual(f.revision(node), fulltext) self.assertEqual(f.revision(node), fulltext)
self.assertEqual(f.revision(node, raw=True), fulltext) self.assertEqual(f.revision(node, raw=True), fulltext)
self.assertEqual(f.read(node), fulltext) self.assertEqual(f.read(node), fulltext)
self.assertFalse(f.renamed(node)) self.assertFalse(f.renamed(node))
with self.assertRaises(IndexError): with self.assertRaises(IndexError):
f.size(3) f.size(3)
f.checkhash(fulltext0, node0) f.checkhash(fulltext0, node0)
f.checkhash(fulltext1, node1) f.checkhash(fulltext1, node1)
f.checkhash(fulltext1, node1, node0, nullid) f.checkhash(fulltext1, node1, node0, nullid)
f.checkhash(fulltext2, node2, node1, nullid) f.checkhash(fulltext2, node2, node1, nullid)
with self.assertRaises(error.RevlogError): with self.assertRaises(error.StorageError):
f.checkhash(fulltext1, b'\x01' * 20) f.checkhash(fulltext1, b'\x01' * 20)
with self.assertRaises(error.RevlogError): with self.assertRaises(error.StorageError):
f.checkhash(fulltext1 + b'extra', node1, node0, nullid) f.checkhash(fulltext1 + b'extra', node1, node0, nullid)
with self.assertRaises(error.RevlogError): with self.assertRaises(error.StorageError):
f.checkhash(fulltext1, node1, node0, node0) f.checkhash(fulltext1, node1, node0, node0)
self.assertEqual(f.revision(node0), fulltext0) self.assertEqual(f.revision(node0), fulltext0)
self.assertEqual(f.revision(node0, raw=True), fulltext0) self.assertEqual(f.revision(node0, raw=True), fulltext0)
self.assertEqual(f.revision(node1), fulltext1) self.assertEqual(f.revision(node1), fulltext1)
self.assertEqual(f.revision(node1, raw=True), fulltext1) self.assertEqual(f.revision(node1, raw=True), fulltext1)
self.assertEqual(f.revision(node2), fulltext2) self.assertEqual(f.revision(node2), fulltext2)
self.assertEqual(f.revision(node2, raw=True), fulltext2) self.assertEqual(f.revision(node2, raw=True), fulltext2)
self.assertEqual(node1, node0) self.assertEqual(node1, node0)
self.assertEqual(node2, node0) self.assertEqual(node2, node0)
self.assertEqual(len(f), 1) self.assertEqual(len(f), 1)
def testaddrevisionbadnode(self): def testaddrevisionbadnode(self):
f = self._makefilefn() f = self._makefilefn()
with self._maketransactionfn() as tr: with self._maketransactionfn() as tr:
# Adding a revision with bad node value fails. # Adding a revision with bad node value fails.
with self.assertRaises(error.RevlogError): with self.assertRaises(error.StorageError):
f.addrevision(b'foo', tr, 0, nullid, nullid, node=b'\x01' * 20) f.addrevision(b'foo', tr, 0, nullid, nullid, node=b'\x01' * 20)
def testaddrevisionunknownflag(self): def testaddrevisionunknownflag(self):
f = self._makefilefn() f = self._makefilefn()
with self._maketransactionfn() as tr: with self._maketransactionfn() as tr:
for i in range(15, 0, -1): for i in range(15, 0, -1):
if (1 << i) & ~revlog.REVIDX_KNOWN_FLAGS: if (1 << i) & ~revlog.REVIDX_KNOWN_FLAGS:
flags = 1 << i flags = 1 << i
break break
with self.assertRaises(error.RevlogError): with self.assertRaises(error.StorageError):
f.addrevision(b'foo', tr, 0, nullid, nullid, flags=flags) f.addrevision(b'foo', tr, 0, nullid, nullid, flags=flags)
def testaddgroupsimple(self): def testaddgroupsimple(self):
f = self._makefilefn() f = self._makefilefn()
callbackargs = [] callbackargs = []
def cb(*args, **kwargs): def cb(*args, **kwargs):
callbackargs.append((args, kwargs)) callbackargs.append((args, kwargs))
fulltext0 = b'foo' fulltext0 = b'foo'
delta0 = mdiff.trivialdiffheader(len(fulltext0)) + fulltext0 delta0 = mdiff.trivialdiffheader(len(fulltext0)) + fulltext0
deltas = [ deltas = [
(b'\x01' * 20, nullid, nullid, nullid, nullid, delta0, 0), (b'\x01' * 20, nullid, nullid, nullid, nullid, delta0, 0),
] ]
with self._maketransactionfn() as tr: with self._maketransactionfn() as tr:
with self.assertRaises(error.RevlogError): with self.assertRaises(error.StorageError):
f.addgroup(deltas, linkmapper, tr, addrevisioncb=cb) f.addgroup(deltas, linkmapper, tr, addrevisioncb=cb)
node0 = f.add(fulltext0, None, tr, 0, nullid, nullid) node0 = f.add(fulltext0, None, tr, 0, nullid, nullid)
f = self._makefilefn() f = self._makefilefn()
deltas = [ deltas = [
(node0, nullid, nullid, nullid, nullid, delta0, 0), (node0, nullid, nullid, nullid, nullid, delta0, 0),