This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4797

storageutil: implement file identifier resolution method (BC)
ClosedPublic

Authored by indygreg on Sep 28 2018, 8:18 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG0e8836be9541: storageutil: implement file identifier resolution method (BC)
Summary

revlog.lookup() has a number of advanced features, including partial
node matching.

These advanced features aren't needed for file id lookup because file
identifiers are almost always from internal sources. (An exception to
this is hgweb, which appears to have some code paths that attempt
to resolve a user-supplied string to a node.)

This commit implements a new function for resolving file identifiers
to nodes. It behaves like revlog.lookup() but without the
advanced features.

Tests reveal behavior changes:

  • Partial hex nodes are no longer resolved to nodes.
  • "-1" now returns nullid instead of raising LookupError.
  • "0" on an empty store now raises LookupError instead of returning nullid.

I'm not sure why "-1" wasn't recognized before. But it seems reasonable
to accept it as a value since integer -1 resolves to nullid.

These changes all seem reasonable to me. And with the exception of
partial hex node matching, we may want to consider changing
revlog.lookup() as well.

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

indygreg created this revision.Sep 28 2018, 8:18 PM
indygreg added a subscriber: martinvonz.Oct 1 2018, 1:37 PM

@martinvonz: you may have an interest in this and D4798, specifically the existing wonky behavior of revlog.lookup(), which these changes effectively skate around.

Closed by commit rHG0e8836be9541: storageutil: implement file identifier resolution method (BC) (authored by indygreg). · Explain WhyOct 3 2018, 11:05 AM
This revision was automatically updated to reflect the committed changes.
martinvonz added a comment.Oct 3 2018, 1:14 PM

"0" on an empty store now raises LookupError instead of returning nullid.

I haven't checked the code, but I suspect this was just a special case of interpreting "rl.lookup(len(rl))" as nullid. I thought I had fixed all those cases about a month ago when I made the index not behave like that (I made only -1 look up the nullid), but maybe I missed a case here.

with the exception of partial hex node matching, we may want to consider changing revlog.lookup() as well

Maybe even without the exception? I thought we used partial nodeids only for debug commands and maybe we can move the partial matching to a higher level.

indygreg added a comment.Oct 3 2018, 1:35 PM
In D4797#73451, @martinvonz wrote:

with the exception of partial hex node matching, we may want to consider changing revlog.lookup() as well

Maybe even without the exception? I thought we used partial nodeids only for debug commands and maybe we can move the partial matching to a higher level.

I support moving partial matching to a higher level. Although some storage backends may support a more efficient "native" partial matcher. e.g. SQLite could use various string matching functions. But I'm not sure it is worth it though: I think I'd rather have a generic object holding DAG/index data [that can be shared across all storage backends] and that exposes a partial match API. I dunno. I don't plan to do anything in this area at this time. If you want to hack on things, go for it.

Revision Contents
Changeset List

Diff 11611

mercurial/filelog.py

def rev(self, node): def rev(self, node):
return self._revlog.rev(node) return self._revlog.rev(node)
def node(self, rev): def node(self, rev):
return self._revlog.node(rev) return self._revlog.node(rev)
def lookup(self, node): def lookup(self, node):
return self._revlog.lookup(node) return storageutil.fileidlookup(self._revlog, node,
self._revlog.indexfile)
def linkrev(self, rev): def linkrev(self, rev):
return self._revlog.linkrev(rev) return self._revlog.linkrev(rev)
def commonancestorsheads(self, node1, node2): def commonancestorsheads(self, node1, node2):
return self._revlog.commonancestorsheads(node1, node2) return self._revlog.commonancestorsheads(node1, node2)
# Used by dagop.blockdescendants(). # Used by dagop.blockdescendants().

mercurial/repository.py

def lookup(value): def lookup(value):
"""Attempt to resolve a value to a node. """Attempt to resolve a value to a node.
Value can be a binary node, hex node, revision number, or a bytes Value can be a binary node, hex node, revision number, or a bytes
that can be converted to an integer. that can be converted to an integer.
Raises ``error.LookupError`` if a ndoe could not be resolved. Raises ``error.LookupError`` if a ndoe could not be resolved.
TODO this is only used by debug* commands and can probably be deleted
easily.
""" """
def parents(node): def parents(node):
"""Returns a 2-tuple of parent nodes for a node. """Returns a 2-tuple of parent nodes for a node.
Values will be ``nullid`` if the parent is empty. Values will be ``nullid`` if the parent is empty.
""" """

mercurial/testing/storage.py

continue continue
with self.assertRaises(IndexError): with self.assertRaises(IndexError):
f.node(i) f.node(i)
self.assertEqual(f.lookup(nullid), nullid) self.assertEqual(f.lookup(nullid), nullid)
self.assertEqual(f.lookup(nullrev), nullid) self.assertEqual(f.lookup(nullrev), nullid)
self.assertEqual(f.lookup(hex(nullid)), nullid) self.assertEqual(f.lookup(hex(nullid)), nullid)
self.assertEqual(f.lookup(b'%d' % nullrev), nullid)
# String converted to integer doesn't work for nullrev.
with self.assertRaises(error.LookupError):
f.lookup(b'%d' % nullrev)
with self.assertRaises(error.LookupError): with self.assertRaises(error.LookupError):
f.lookup(b'badvalue') f.lookup(b'badvalue')
self.assertEqual(f.lookup(hex(nullid)[0:12]), nullid) with self.assertRaises(error.LookupError):
f.lookup(hex(nullid)[0:12])
with self.assertRaises(error.LookupError): with self.assertRaises(error.LookupError):
f.lookup(b'-2') f.lookup(b'-2')
# TODO this is wonky. with self.assertRaises(error.LookupError):
self.assertEqual(f.lookup(b'0'), nullid) f.lookup(b'0')
with self.assertRaises(error.LookupError): with self.assertRaises(error.LookupError):
f.lookup(b'1') f.lookup(b'1')
with self.assertRaises(error.LookupError): with self.assertRaises(error.LookupError):
f.lookup(b'11111111111111111111111111111111111111') f.lookup(b'11111111111111111111111111111111111111')
for i in range(-5, 5): for i in range(-5, 5):
with self.assertRaises(IndexError): with self.assertRaises(IndexError):
f.node(1) f.node(1)
self.assertEqual(f.lookup(node), node) self.assertEqual(f.lookup(node), node)
self.assertEqual(f.lookup(0), node) self.assertEqual(f.lookup(0), node)
self.assertEqual(f.lookup(-1), nullid) self.assertEqual(f.lookup(-1), nullid)
self.assertEqual(f.lookup(b'0'), node) self.assertEqual(f.lookup(b'0'), node)
self.assertEqual(f.lookup(hex(node)), node) self.assertEqual(f.lookup(hex(node)), node)
self.assertEqual(f.lookup(hex(node)[0:12]), node)
with self.assertRaises(error.LookupError):
f.lookup(hex(node)[0:12])
with self.assertRaises(IndexError): with self.assertRaises(IndexError):
f.lookup(-2) f.lookup(-2)
with self.assertRaises(error.LookupError): with self.assertRaises(error.LookupError):
f.lookup(b'-2') f.lookup(b'-2')
with self.assertRaises(IndexError): with self.assertRaises(IndexError):

mercurial/utils/storageutil.py

# storageutil.py - Storage functionality agnostic of backend implementation. # storageutil.py - Storage functionality agnostic of backend implementation.
# #
# Copyright 2018 Gregory Szorc <gregory.szorc@gmail.com> # Copyright 2018 Gregory Szorc <gregory.szorc@gmail.com>
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
from __future__ import absolute_import from __future__ import absolute_import
import hashlib import hashlib
import re import re
from ..i18n import _
from ..node import ( from ..node import (
bin,
nullid, nullid,
) )
from .. import ( from .. import (
error,
pycompat, pycompat,
) )
_nullhash = hashlib.sha1(nullid) _nullhash = hashlib.sha1(nullid)
def hashrevisionsha1(text, p1, p2): def hashrevisionsha1(text, p1, p2):
"""Compute the SHA-1 for revision data and its parents. """Compute the SHA-1 for revision data and its parents.
step = -1 step = -1
stop += step stop += step
if stop > storelen: if stop > storelen:
stop = storelen stop = storelen
else: else:
stop = storelen stop = storelen
return pycompat.xrange(start, stop, step) return pycompat.xrange(start, stop, step)
def fileidlookup(store, fileid, identifier):
"""Resolve the file node for a value.
``store`` is an object implementing the ``ifileindex`` interface.
``fileid`` can be:
* A 20 byte binary node.
* An integer revision number
* A 40 byte hex node.
* A bytes that can be parsed as an integer representing a revision number.
``identifier`` is used to populate ``error.LookupError`` with an identifier
for the store.
Raises ``error.LookupError`` on failure.
"""
if isinstance(fileid, int):
return store.node(fileid)
if len(fileid) == 20:
try:
store.rev(fileid)
return fileid
except error.LookupError:
pass
if len(fileid) == 40:
try:
rawnode = bin(fileid)
store.rev(rawnode)
return rawnode
except TypeError:
pass
try:
rev = int(fileid)
if b'%d' % rev != fileid:
raise ValueError
try:
return store.node(rev)
except (IndexError, TypeError):
pass
except (ValueError, OverflowError):
pass
raise error.LookupError(fileid, identifier, _('no match found'))