This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3145

context: stop catching RepoLookupError from namespace.singlenode()
ClosedPublic

Authored by martinvonz on Apr 5 2018, 6:07 PM.

Details

Reviewers
yuja
Group Reviewers
hg-reviewers
Commits
rHG3198d5a2514e: context: stop catching RepoLookupError from namespace.singlenode()
Summary

As pointed out by Yuya, the RepoLookupError was there for catching
errors from repo.branchtip(). However, since 885c0290f7d5 (localrepo:
add ignoremissing parameter to branchtip, 2014-10-16), that should no
longer happen. I think it should now be an error if a namespace raises
a RepoLookupError, so we propagate the exception up and and make it
easy to fix, rather than trying to interpret the changeid as nodeid
prefix and raise a general "unknown revision '...'" error.

I also don't think we should catch FilteredLookupError and LookupError
from the changelog.rev() call, for the same reason as above: If a
namespace returns a node that doesn't exist, we should provide a more
helpful exception than "unknown revision '...'".

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

martinvonz created this revision.Apr 5 2018, 6:07 PM
yuja requested changes to this revision.Apr 6 2018, 8:40 AM
yuja added a subscriber: yuja.

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).

This revision now requires changes to proceed.Apr 6 2018, 8:40 AM
martinvonz added a comment.Apr 6 2018, 11:42 AM
In 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.

martinvonz added a comment.Apr 6 2018, 12:37 PM
In D3145#50619, @martinvonz wrote:
In 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.

martinvonz edited the summary of this revision. (Show Details)Apr 6 2018, 12:39 PM
martinvonz retitled this revision from context: catch right exceptions from namespace lookup (API) to context: stop catching RepoLookupError from namespace.singlenode().
martinvonz updated this revision to Diff 7781.
yuja accepted this revision.Apr 6 2018, 12:48 PM
This revision is now accepted and ready to land.Apr 6 2018, 12:48 PM
Closed by commit rHG3198d5a2514e: context: stop catching RepoLookupError from namespace.singlenode() (authored by martinvonz). · Explain WhyApr 6 2018, 1:10 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/context.py (4 lines)

Diff 7785

mercurial/context.py

# lookup bookmarks through the name interface # lookup bookmarks through the name interface
try: try:
self._node = repo.names.singlenode(repo, changeid) self._node = repo.names.singlenode(repo, changeid)
self._rev = repo.changelog.rev(self._node) self._rev = repo.changelog.rev(self._node)
return return
except KeyError: except KeyError:
pass pass
except error.FilteredRepoLookupError:
raise
except error.RepoLookupError:
pass
self._node = repo.unfiltered().changelog._partialmatch(changeid) self._node = repo.unfiltered().changelog._partialmatch(changeid)
if self._node is not None: if self._node is not None:
self._rev = repo.changelog.rev(self._node) self._rev = repo.changelog.rev(self._node)
return return
# lookup failed # lookup failed
# check if it might have come from damaged dirstate # check if it might have come from damaged dirstate