This is an archive of the discontinued Mercurial Phabricator instance.

Differential D3158

histedit: look up partial nodeid as partial nodeid
ClosedPublic

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

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

Event Timeline

martinvonz created this revision.Apr 6 2018, 12:54 PM
indygreg accepted this revision.Apr 6 2018, 2:57 PM
indygreg added a subscriber: indygreg.

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

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

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

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

BTW, it's probably wrong to rely only on _partialmatch() of unfiltered repo.
In changectx.init, we verify the result with repo.changelog.rev().

indygreg added a comment.Apr 7 2018, 3:37 PM

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

martinvonz added a comment.Apr 7 2018, 5:36 PM
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

PathPackages
Mhgext/histedit.py (8 lines)

Diff 7783

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 = repo.unfiltered().changelog._partialmatch(ha)
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.
self.node = repo[ha].node() if self.node is None:
except error.RepoError: raise error.ParseError(_('unknown changeset %s listed') % ha[:12])
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'))