This is an archive of the discontinued Mercurial Phabricator instance.

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

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

Details

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
Lint Skipped
Unit
Unit Tests Skipped

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

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
This revision was automatically updated to reflect the committed changes.