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.

Download Raw Diff

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

Lint Skipped

Unit

Unit Tests Skipped

Event Timeline

martinvonz created this revision.Apr 5 2018, 6:07 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptApr 5 2018, 6:07 PM

Herald added a subscriber: mercurial-devel. · View Herald Transcript

martinvonz added a child revision: D3146: context: handle stringified ints in revsymbol().Apr 5 2018, 6:07 PM

martinvonz added a child revision: D3155: hgweb: use revsymbol() to determine if query is a revision.Apr 6 2018, 3:08 AM

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

			Path	Packages
M			mercurial/context.py (4 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	7756		Apr 5 2018, 6:07 PM	★	★
Diff 2	7781		Apr 6 2018, 12:39 PM	★	★
Diff 3	7785	rHG3198d5a2514ef4ff91f42e6cf072293473634ce8	Apr 6 2018, 12:34 PM	★	★

Status	Author	Revision
Closed	martinvonz	D3197 context: add deprecation warnings for deprecated types of changeids
Closed	martinvonz	D3196 revsymbol: stop delegating to repo.__getitem__ for unhandled symbols (API)
Closed	martinvonz	D3195 context: handle partial nodeids in revsymbol()
Closed	martinvonz	D3194 context: handle namespaces in revsymbol()
Closed	martinvonz	D3193 scmutil: handle full hex nodeids in revsymbol()
Closed	martinvonz	D3146 context: handle stringified ints in revsymbol()
Closed	martinvonz	D3156 githelp: use revsymbol() for looking up symbol
Closed	martinvonz	D3155 hgweb: use revsymbol() to determine if query is a revision
Closed	martinvonz	D3145 context: stop catching RepoLookupError from namespace.singlenode()
Closed	martinvonz	D3144 context: make repo[<filtered binary nodeid>] match node
Closed	martinvonz	D3143 context: move handling of filtering error to revsymbol() (API)

Diff 7756

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:			except error.FilteredLookupError:
	raise			raise
	except error.RepoLookupError:			except error.LookupError:
	pass			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