Patchwork D3145: context: catch right exceptions from namespace lookup (API)

login
register
mail settings
Submitter phabricator
Date April 5, 2018, 10:07 p.m.
Message ID <differential-rev-PHID-DREV-4nuo5wea5pip2se27r27-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/30408/
State Superseded
Headers show

Comments

phabricator - April 5, 2018, 10:07 p.m.
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Similar to the previous patch, here we were catching
  FilteredRepoLookupError and RepoLookupError instead of
  FilteredLookupError and LookupError. What makes this trickier is that
  this is calling into namespaces code, and since namespaces are meant
  to be extensible, we could potentially get a RepoLookupError from the
  namespace lookup. If we got a RepoLookupError before, we would swallow
  it and try to interpret the string as a partial nodeid. That means
  that if the user runs `hg co deadbeef` where "deadbeef" is a valid
  nodeid prefix and also something that make some namespace raise a
  RepoLookupError, we would happily check out the requested
  revision. After this patch, we will instead error out.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/context.py

CHANGE DETAILS




To: martinvonz, #hg-reviewers
Cc: mercurial-devel
phabricator - April 6, 2018, 12:40 p.m.
yuja requested changes to this revision.
yuja added a comment.
This revision now requires changes to proceed.


  It seems incorrect to me to catch LookupError of `node` returned by
  a namespace API, not by a user-specified node value.
  
  I think the reason why catching `RepoLookupError` here was that
  we historically used `repo.branchtip(changeid)`.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers, yuja
Cc: yuja, mercurial-devel
phabricator - April 6, 2018, 3:42 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D3145#50598, @yuja wrote:
  
  > It seems incorrect to me to catch LookupError of `node` returned by
  >  a namespace API, not by a user-specified node value.
  
  
  The idea wasn't to catch those from the namespace API, but from changelog.rev() just after. The overly broad catching has bothered me before, so let me fix that and split this patch up.
  
  > I think the reason why catching `RepoLookupError` here was that
  >  we historically used `repo.branchtip(changeid)`.
  
  Ah, thanks for the information.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers, yuja
Cc: yuja, mercurial-devel
phabricator - April 6, 2018, 4:37 p.m.
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D3145#50619, @martinvonz wrote:
  
  > In https://phab.mercurial-scm.org/D3145#50598, @yuja wrote:
  >
  > > It seems incorrect to me to catch LookupError of `node` returned by
  > >  a namespace API, not by a user-specified node value.
  >
  >
  > The idea wasn't to catch those from the namespace API, but from changelog.rev() just after. The overly broad catching has bothered me before, so let me fix that and split this patch up.
  
  
  Actually, thinking more about, I think we should not even catch LookupError, so I ended up just deleting these lines. Thanks for making me check.
  
  > 
  > 
  >> I think the reason why catching `RepoLookupError` here was that
  >>  we historically used `repo.branchtip(changeid)`.
  > 
  > Ah, thanks for the information.

REPOSITORY
  rHG Mercurial

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

To: martinvonz, #hg-reviewers, yuja
Cc: yuja, mercurial-devel

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -449,9 +449,9 @@ 
                 return
             except KeyError:
                 pass
-            except error.FilteredRepoLookupError:
+            except error.FilteredLookupError:
                 raise
-            except error.RepoLookupError:
+            except error.LookupError:
                 pass
 
             self._node = repo.unfiltered().changelog._partialmatch(changeid)