Patchwork D3092: simplestore: shore up lookup errors

login
register
mail settings
Submitter phabricator
Date April 5, 2018, 1:24 a.m.
Message ID <differential-rev-PHID-DREV-c7hl6hx74fme4xjab6iy-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/30317/
State Superseded
Headers show

Comments

phabricator - April 5, 2018, 1:24 a.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  When revisions or nodes can't be resolved, we're expected to raise
  an error.LookupError. When I ported code from revlog.py, I failed
  to realize that "LookupError" in that module is aliased to
  error.LookupError. I thought we were using the builtin LookupError
  instead.
  
  This commit switches us to error.LookupError. It also fixes
  rev() to raise error.LookupError instead of KeyError.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3092

AFFECTED FILES
  tests/simplestorerepo.py

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel
phabricator - April 5, 2018, 5:31 a.m.
martinvonz added a comment.


  > When revisions or nodes can't be resolved, we're expected to raise
  >  an error.LookupError. When I ported code from revlog.py, I failed
  >  to realize that "LookupError" in that module is aliased to
  >  error.LookupError. I thought we were using the builtin LookupError
  >  instead.
  
  I've also recently been confused by that. Perhaps we should even rename error.LookupError to something else and avoid shadowing the builtin? StoreLookupError, perhaps?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3092

To: indygreg, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - April 6, 2018, 7:38 p.m.
durin42 added a comment.


  In https://phab.mercurial-scm.org/D3092#50126, @martinvonz wrote:
  
  > > When revisions or nodes can't be resolved, we're expected to raise
  > >  an error.LookupError. When I ported code from revlog.py, I failed
  > >  to realize that "LookupError" in that module is aliased to
  > >  error.LookupError. I thought we were using the builtin LookupError
  > >  instead.
  >
  > I've also recently been confused by that. Perhaps we should even rename error.LookupError to something else and avoid shadowing the builtin? StoreLookupError, perhaps?
  
  
  Probably a good idea.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D3092

To: indygreg, #hg-reviewers
Cc: durin42, martinvonz, mercurial-devel

Patch

diff --git a/tests/simplestorerepo.py b/tests/simplestorerepo.py
--- a/tests/simplestorerepo.py
+++ b/tests/simplestorerepo.py
@@ -152,8 +152,10 @@ 
     def rev(self, node):
         validatenode(node)
 
-        # Will raise KeyError.
-        self._indexbynode[node]
+        try:
+            self._indexbynode[node]
+        except KeyError:
+            raise error.LookupError(node, self._indexpath, _('no node'))
 
         for rev, entry in self._indexbyrev.items():
             if entry[b'node'] == node:
@@ -171,11 +173,8 @@ 
             return self.node(node)
 
         if len(node) == 20:
-            try:
-                self.rev(node)
-                return node
-            except LookupError:
-                pass
+            self.rev(node)
+            return node
 
         try:
             rev = int(node)
@@ -196,10 +195,10 @@ 
                 rawnode = bin(node)
                 self.rev(rawnode)
                 return rawnode
-            except (TypeError, LookupError):
+            except TypeError:
                 pass
 
-        raise LookupError(node, self._path, _('invalid lookup input'))
+        raise error.LookupError(node, self._path, _('invalid lookup input'))
 
     def linkrev(self, rev):
         validaterev(rev)
@@ -280,8 +279,6 @@ 
         if node == nullid:
             return b''
 
-        self._indexbynode[node]
-
         rev = self.rev(node)
         flags = self.flags(rev)