This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4701

verify: start to abstract file verification
ClosedPublic

Authored by indygreg on Sep 24 2018, 12:12 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG97986c9c69d3: verify: start to abstract file verification
Summary

Currently, the file storage interface has a handful of attributes
that are exclusively or near-exclusively used by repo verification
code. In order to support verification on non-revlog/alternate
storage backends, we'll need to abstract verification so it can
be performed in a storage-agnostic way.

This commit starts that process.

We establish a new verifyintegrity() method on revlogs and expose
it to the file storage interface. Most of verify.verifier.checklog()
has been ported to this new method.

We need a way to represent verification problems. So we invent an
interface to represent a verification problem, invent a revlog type
to implement that interface, and use it.

The arguments to verifyintegrity() will almost certainly change in
the future, once more functionality is ported from the verify code.
And the "revlogv1" version check is very hacky. (The code in verify
is actually buggy because it is comparing the full 32-bit header
integer instead of just the revlog version short. I'll likely fix
this in a subsequent commit.)

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Sep 24 2018, 12:12 PM
mharbison72 added a subscriber: mharbison72.Sep 25 2018, 1:03 AM

Not related to this patch specifically, but if you're redesigning verify APIs...

One of the unresolved issues in LFS is how to handle verification. Right now, verify wants the actual blob data, so it downloads it one at a time, as needed, as part of reading the filelog. I submitted a patch earlier this year to batch prefetch them[1], and pondered if we can just ask the server (assuming it's our hg code) to verify the missing blobs remotely without actually transferring them. Yuya rightly objected to trying to operate on the raw data and JSON en-mass before it's been verified.

I guess we could do a two pass process, where files marked for external storage simply have their JSON parsed in a try/catch (without the rawtext verification), and then once the filelogs are verified, try to process the blob data in the next phase. So I wonder if more of this needs to be moved to a wrappable function, so that the extension can customize the filelog verification.

[1] https://www.mercurial-scm.org/pipermail/mercurial-devel/2018-April/116091.html

indygreg added a comment.Sep 25 2018, 12:09 PM
In D4701#71766, @mharbison72 wrote:

Not related to this patch specifically, but if you're redesigning verify APIs...

That's good context - and context I hadn't considered!

My immediate goal with reworking verify is to move low-level code out of verify and into storage. Getting the interface "correct" will be a later step.

From what you said, I could easily imagine us wanting to add different verify "modes." e.g. controls to specify whether fulltext data should be verified. And maybe something so LFS can tell revlog's verify method to ignore revisions with the extstored flag so LFS can do something reasonable. Tons of possibilities here. If you want to have a go at refactoring things after the patches in this stack land, I'll happily review those changes!

Closed by commit rHG97986c9c69d3: verify: start to abstract file verification (authored by indygreg). · Explain WhySep 26 2018, 1:22 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 11309

mercurial/filelog.py

# renaming a file produces a different hash, even if the data # renaming a file produces a different hash, even if the data
# remains unchanged. Check if it's the case (slow): # remains unchanged. Check if it's the case (slow):
if self.renamed(node): if self.renamed(node):
t2 = self.read(node) t2 = self.read(node)
return t2 != text return t2 != text
return True return True
def verifyintegrity(self, state):
return self._revlog.verifyintegrity(state)
# TODO these aren't part of the interface and aren't internal methods. # TODO these aren't part of the interface and aren't internal methods.
# Callers should be fixed to not use them. # Callers should be fixed to not use them.
# Used by LFS. # Used by LFS.
@property @property
def filename(self): def filename(self):
return self._revlog.filename return self._revlog.filename

mercurial/repository.py

def requirecap(self, name, purpose): def requirecap(self, name, purpose):
if self.capable(name): if self.capable(name):
return return
raise error.CapabilityError( raise error.CapabilityError(
_('cannot %s; remote repository does not support the %r ' _('cannot %s; remote repository does not support the %r '
'capability') % (purpose, name)) 'capability') % (purpose, name))
class iverifyproblem(interfaceutil.Interface):
"""Represents a problem with the integrity of the repository.
Instances of this interface are emitted to describe an integrity issue
with a repository (e.g. corrupt storage, missing data, etc).
Instances are essentially messages associated with severity.
"""
warning = interfaceutil.Attribute(
"""Message indicating a non-fatal problem.""")
error = interfaceutil.Attribute(
"""Message indicating a fatal problem.""")
class irevisiondelta(interfaceutil.Interface): class irevisiondelta(interfaceutil.Interface):
"""Represents a delta between one revision and another. """Represents a delta between one revision and another.
Instances convey enough information to allow a revision to be exchanged Instances convey enough information to allow a revision to be exchanged
with another repository. with another repository.
Instances represent the fulltext revision data or a delta against Instances represent the fulltext revision data or a delta against
another revision. Therefore the ``revision`` and ``delta`` attributes another revision. Therefore the ``revision`` and ``delta`` attributes
""" """
def checksize(): def checksize():
"""Obtain the expected sizes of backing files. """Obtain the expected sizes of backing files.
TODO this is used by verify and it should not be part of the interface. TODO this is used by verify and it should not be part of the interface.
""" """
def verifyintegrity(state):
"""Verifies the integrity of file storage.
``state`` is a dict holding state of the verifier process. It can be
used to communicate data between invocations of multiple storage
primitives.
The method yields objects conforming to the ``iverifyproblem``
interface.
"""
class idirs(interfaceutil.Interface): class idirs(interfaceutil.Interface):
"""Interface representing a collection of directories from paths. """Interface representing a collection of directories from paths.
This interface is essentially a derived data structure representing This interface is essentially a derived data structure representing
directories from a collection of paths. directories from a collection of paths.
""" """
def addpath(path): def addpath(path):

mercurial/revlog.py

p2node = attr.ib() p2node = attr.ib()
basenode = attr.ib() basenode = attr.ib()
linknode = attr.ib() linknode = attr.ib()
flags = attr.ib() flags = attr.ib()
baserevisionsize = attr.ib() baserevisionsize = attr.ib()
revision = attr.ib() revision = attr.ib()
delta = attr.ib() delta = attr.ib()
@interfaceutil.implementer(repository.iverifyproblem)
@attr.s(frozen=True)
class revlogproblem(object):
warning = attr.ib(default=None)
error = attr.ib(default=None)
# index v0: # index v0:
# 4 bytes: offset # 4 bytes: offset
# 4 bytes: compressed length # 4 bytes: compressed length
# 4 bytes: base rev # 4 bytes: base rev
# 4 bytes: link rev # 4 bytes: link rev
# 20 bytes: parent 1 nodeid # 20 bytes: parent 1 nodeid
# 20 bytes: parent 2 nodeid # 20 bytes: parent 2 nodeid
# 20 bytes: nodeid # 20 bytes: nodeid
datawrite.write(chunk) datawrite.write(chunk)
offset += dlen offset += dlen
idxread.close() idxread.close()
idxwrite.close() idxwrite.close()
if dataread is not idxread: if dataread is not idxread:
dataread.close() dataread.close()
datawrite.close() datawrite.close()
def verifyintegrity(self, state):
"""Verifies the integrity of the revlog.
Yields ``revlogproblem`` instances describing problems that are
found.
"""
dd, di = self.checksize()
if dd:
yield revlogproblem(error=_('data length off by %d bytes') % dd)
if di:
yield revlogproblem(error=_('index contains %d extra bytes') % di)
if self.version != REVLOGV0:
if not state['revlogv1']:
yield revlogproblem(warning=_("warning: `%s' uses revlog "
"format 1") % self.indexfile)
elif state['revlogv1']:
yield revlogproblem(warning=_("warning: `%s' uses revlog "
"format 0") % self.indexfile)

mercurial/verify.py

storefiles = set() storefiles = set()
for f, f2, size in repo.store.datafiles(): for f, f2, size in repo.store.datafiles():
if not f: if not f:
self.err(None, _("cannot decode filename '%s'") % f2) self.err(None, _("cannot decode filename '%s'") % f2)
elif (size > 0 or not revlogv1) and f.startswith('data/'): elif (size > 0 or not revlogv1) and f.startswith('data/'):
storefiles.add(_normpath(f)) storefiles.add(_normpath(f))
state = {
'revlogv1': self.revlogv1,
}
files = sorted(set(filenodes) | set(filelinkrevs)) files = sorted(set(filenodes) | set(filelinkrevs))
revisions = 0 revisions = 0
progress = ui.makeprogress(_('checking'), unit=_('files'), progress = ui.makeprogress(_('checking'), unit=_('files'),
total=len(files)) total=len(files))
for i, f in enumerate(files): for i, f in enumerate(files):
progress.update(i, item=f) progress.update(i, item=f)
try: try:
linkrevs = filelinkrevs[f] linkrevs = filelinkrevs[f]
try: try:
storefiles.remove(ff) storefiles.remove(ff)
except KeyError: except KeyError:
if self.warnorphanstorefiles: if self.warnorphanstorefiles:
self.warn(_(" warning: revlog '%s' not in fncache!") % self.warn(_(" warning: revlog '%s' not in fncache!") %
ff) ff)
self.fncachewarned = True self.fncachewarned = True
self.checklog(fl, f, lr) if not len(fl) and (self.havecl or self.havemf):
self.err(lr, _("empty or missing %s") % f)
else:
for problem in fl.verifyintegrity(state):
if problem.warning:
self.warn(problem.warning)
elif problem.error:
self.err(lr, problem.error, f)
else:
raise error.ProgrammingError(
'problem instance does not set warning or error '
'attribute: %s' % problem.msg)
seen = {} seen = {}
rp = None rp = None
for i in fl: for i in fl:
revisions += 1 revisions += 1
n = fl.node(i) n = fl.node(i)
lr = self.checkentry(fl, i, n, seen, linkrevs, f) lr = self.checkentry(fl, i, n, seen, linkrevs, f)
if f in filenodes: if f in filenodes:
if havemf and n not in filenodes[f]: if havemf and n not in filenodes[f]:

tests/test-check-interfaces.py

rdr = changegroup.revisiondeltarequest( rdr = changegroup.revisiondeltarequest(
node=b'', node=b'',
linknode=b'', linknode=b'',
p1node=b'', p1node=b'',
p2node=b'', p2node=b'',
basenode=b'') basenode=b'')
checkzobject(rdr) checkzobject(rdr)
ziverify.verifyClass(repository.iverifyproblem,
revlog.revlogproblem)
checkzobject(revlog.revlogproblem())
main() main()