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.

Details

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

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

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