( )⚙ D3311 revset: use resolvehexnodeidprefix() in id() predicate (BC)

This is an archive of the discontinued Mercurial Phabricator instance.

revset: use resolvehexnodeidprefix() in id() predicate (BC)
ClosedPublic

Authored by martinvonz on Apr 13 2018, 1:59 PM.

Details

Summary

We now have a public method for this purpose, so we don't need to
access the private revlog._partialmatch(). Also, I'll probably make
some changes to resolvehexnodeidprefix() later, and I want those to be
reflected by the id() predicate.

Note that this breaks a test case, because we now resolve the prefix
in the unfiltered repo and get an ambiguous lookup, which results in
no revision being added to the revset. The test case was already
documented to be broken even though it wasn't. It's important to note
that {shortest(node)} already uses the unfiltered repo, so we're not
going to break people who get the prefix from there.

I think we may not want to ever use shortest() in the filtered
repo. It seems unlikely to be enough of a win to matter much. For
example, in my hg repo, it would save me only 0.2 hex digits. In
another repo that only I modify, it saves a little more, but it's
still only 0.29 hex digits. It seems unlikely that people will prune
enough commits that only 1/16 of the commits are visible (which is
what it would take a to save a single hex digit). Instead, I'm working
on another approach: allow ambiguous matches to be disambiguated
within a user-specified revset. Whether or not that pans out, I hope
we're okay with this little change in behavior for now and we can
decide what to do about it later.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Apr 13 2018, 1:59 PM
yuja requested changes to this revision.Apr 14 2018, 4:08 AM
yuja added a subscriber: yuja.

I have vague memory that it was intentional. Since rev() and id() never
error out on unknown identifier, it doesn't make sense to reject only
ambiguous nodeid.

This revision now requires changes to proceed.Apr 14 2018, 4:08 AM
In D3311#53712, @yuja wrote:

I have vague memory that it was intentional. Since rev() and id() never
error out on unknown identifier, it doesn't make sense to reject only
ambiguous nodeid.

I'm fine with dropping this patch. The rest of the stack doesn't depend on it anyway.

martinvonz abandoned this revision.Apr 14 2018, 12:14 PM
martinvonz reclaimed this revision.May 7 2018, 6:16 PM

Reviving this patch, but changing it so it doesn't result in an exception

This revision now requires changes to proceed.May 7 2018, 6:16 PM
yuja added a comment.May 8 2018, 8:53 AM

I've duplicated "BROKEN" lines as we get two "broken" results by this change.