This is an archive of the discontinued Mercurial Phabricator instance.

histedit: look up partial nodeid as partial nodeid
ClosedPublic

Authored by martinvonz on Apr 6 2018, 12:54 PM.

Download Raw Diff

Details

Reviewers

durin42
indygreg

Group Reviewers

hg-reviewers

Commits

rHGc4131138eadb: histedit: look up partial nodeid as partial nodeid

Summary

I'm about to remove support for repo[<partial hex nodeid>]. In the
verify() method, we know that self.node is always a partial or full
binary nodeid, so the most correct way to look up the revision is by
using changelog._partialmatch(), so let's do that. (It's closer to the
current code to do scmutil.revsymbol(), but that's less correct
because it will match a bookmark or tag that happens to have the same
prefix as the node.)

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 6 2018, 12:54 PM

Herald added a reviewer: durin42. · View Herald TranscriptApr 6 2018, 12:54 PM

Herald added a reviewer: hg-reviewers. · View Herald Transcript

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

martinvonz added a child revision: D3159: eol: look up partial nodeid as partial nodeid.Apr 6 2018, 12:54 PM

This is a layering violation. But I don't care because we don't yet have an interface for changelog. Once we do, we'll likely promote _partialmatch to a public method.

This revision is now accepted and ready to land.Apr 6 2018, 2:57 PM

indygreg requested changes to this revision.Apr 6 2018, 3:01 PM

indygreg added inline comments.

hgext/histedit.py
446–448	Err wait. Why is `repo.unfiltered()` being used here? If the previous code worked on on the filtered repo, shouldn't this code? The side effect of this change is that a histedit rule could reference a hidden changeset. That feels wrong.

This revision now requires changes to proceed.Apr 6 2018, 3:01 PM

martinvonz added inline comments.Apr 6 2018, 3:13 PM

hgext/histedit.py
446–448	If the previous code worked on on the filtered repo, shouldn't this code? The previous code just looked like it worked on the filtered repo :) This is copied from changectx.init(), which is where this would end up getting resolved before. I don't remember what the reason is for that code to use the unfiltered repo (I think it had something to do with making {shortest(node)} length match what's actually unambiguous. Either way, this patch should not be changing any behavior, I think.

yuja added a subscriber: yuja.Apr 7 2018, 3:36 AM

yuja added inline comments.

hgext/histedit.py
446–448	Nah. It's for performance reason on ambiguous case, radix tree lookup vs linear search. I've sent a patch to fix the inconsistency, but it was rejected because of that. Maybe we'll need a scmutil function to document that. We shouldn't spill around the weird implementation detail.

yuja added inline comments.Apr 7 2018, 3:45 AM

hgext/histedit.py
446–448	BTW, it's probably wrong to rely only on _partialmatch() of unfiltered repo. In changectx.init, we verify the result with `repo.changelog.rev()`.

+1 to having a new function to handle cases for things like partial match.

In D3158#51161, @indygreg wrote:

+1 to having a new function to handle cases for things like partial match.

I agree, that seems much better.

martinvonz updated this revision to Diff 7879.Apr 8 2018, 12:03 PM

Closed by commit rHGc4131138eadb: histedit: look up partial nodeid as partial nodeid (authored by martinvonz). · Explain WhyApr 10 2018, 12:03 PM

This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

			Path	Packages
M			hgext/histedit.py (8 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	7783		Apr 6 2018, 12:54 PM	★	★
Diff 2	7879		Apr 8 2018, 12:03 PM	★	★
Diff 3	7936	rHGc4131138eadb232ca5888a0b36d49c7aee18c1df	Apr 6 2018, 12:43 PM	★	★

Status	Author	Revision
Closed	martinvonz	D3190 merge: avoid unnecessary conversion from binary nodeid to binary nodeid
Closed	martinvonz	D3159 eol: look up partial nodeid as partial nodeid
Closed	martinvonz	D3158 histedit: look up partial nodeid as partial nodeid
Closed	martinvonz	D3157 histedit: drop unnecessary check for "self.node is not None"

Diff 7936

hgext/histedit.py

	except error.RepoLookupError:			except error.RepoLookupError:
	raise error.ParseError(_("invalid changeset %s") % ruleid)			raise error.ParseError(_("invalid changeset %s") % ruleid)
	return cls(state, rev)			return cls(state, rev)

	def verify(self, prev, expected, seen):			def verify(self, prev, expected, seen):
	""" Verifies semantic correctness of the rule"""			""" Verifies semantic correctness of the rule"""
	repo = self.repo			repo = self.repo
	ha = node.hex(self.node)			ha = node.hex(self.node)
	try:			self.node = scmutil.resolvepartialhexnodeid(repo, ha)
	self.node = repo[ha].node()			if self.node is None:
	except error.RepoError:			raise error.ParseError(_('unknown changeset %s listed') % ha[:12])
				indygregUnsubmitted Not Done Err wait. Why is `repo.unfiltered()` being used here? If the previous code worked on on the filtered repo, shouldn't this code? The side effect of this change is that a histedit rule could reference a hidden changeset. That feels wrong. indygreg: Err wait. Why is `repo.unfiltered()` being used here? If the previous code worked on on the…
				martinvonzAuthorUnsubmitted Not Done If the previous code worked on on the filtered repo, shouldn't this code? The previous code just looked like it worked on the filtered repo :) This is copied from changectx.init(), which is where this would end up getting resolved before. I don't remember what the reason is for that code to use the unfiltered repo (I think it had something to do with making {shortest(node)} length match what's actually unambiguous. Either way, this patch should not be changing any behavior, I think. martinvonz: > If the previous code worked on on the filtered repo, shouldn't this code? The previous code…
				yujaUnsubmitted Not Done Nah. It's for performance reason on ambiguous case, radix tree lookup vs linear search. I've sent a patch to fix the inconsistency, but it was rejected because of that. Maybe we'll need a scmutil function to document that. We shouldn't spill around the weird implementation detail. yuja: Nah. It's for performance reason on ambiguous case, radix tree lookup vs linear search. I've…
				yujaUnsubmitted Not Done BTW, it's probably wrong to rely only on _partialmatch() of unfiltered repo. In changectx.init, we verify the result with `repo.changelog.rev()`. yuja: BTW, it's probably wrong to rely only on _partialmatch() of unfiltered repo. In changectx.
	raise error.ParseError(_('unknown changeset %s listed')
	% ha[:12])
	self._verifynodeconstraints(prev, expected, seen)			self._verifynodeconstraints(prev, expected, seen)

	def _verifynodeconstraints(self, prev, expected, seen):			def _verifynodeconstraints(self, prev, expected, seen):
	# by default command need a node in the edited list			# by default command need a node in the edited list
	if self.node not in expected:			if self.node not in expected:
	raise error.ParseError(_('%s "%s" changeset was not a candidate')			raise error.ParseError(_('%s "%s" changeset was not a candidate')
	% (self.verb, node.short(self.node)),			% (self.verb, node.short(self.node)),
	hint=_('only use listed changesets'))			hint=_('only use listed changesets'))