This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7712

lfs: don't skip locally available blobs when verifying
ClosedPublic

Authored by mharbison72 on Dec 23 2019, 2:47 AM.

Details

Reviewers
durin42
pulkit
Group Reviewers
hg-reviewers
Commits
rHG1a6dd50cd0db: lfs: don't skip locally available blobs when verifying
rHG09fe0f5c3223: lfs: don't skip locally available blobs when verifying
rHG1dff147554b1: lfs: don't skip locally available blobs when verifying
Summary

The skipflags config was introduced in a2ab9ebcd85b, which specifically calls
out downloading and storing all blobs as potentially too expensive. But I don't
see any reason to skip blobs that are already available locally. Hashing the
blob is the only way to indirectly verify the rawdata content stored in the
revlog.

(The note in that commit about skipping renamed is still correct, but the reason
given about needing fulltext isn't.)

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

mharbison72 created this revision.Dec 23 2019, 2:47 AM
indygreg added a subscriber: indygreg.Dec 23 2019, 12:39 PM

This seems good. But planned changes in D7708 will create a merge conflict. So holding off on accepting for now.

durin42 requested changes to this revision.Jan 8 2020, 3:00 PM
durin42 added a subscriber: durin42.

Needs rebased.

This revision now requires changes to proceed.Jan 8 2020, 3:00 PM
pulkit accepted this revision.Jan 14 2020, 10:01 AM
mharbison72 added a commit: rHG1dff147554b1: lfs: don't skip locally available blobs when verifying.Jan 14 2020, 11:13 AM
This revision was not accepted when it landed; it landed in state Needs Revision.
Closed by commit rHG1dff147554b1: lfs: don't skip locally available blobs when verifying (authored by mharbison72). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/lfs/__init__.py (3 lines)
Mhgext/lfs/wrapper.py (15 lines)
Mtests/test-lfs.t (2 lines)

Diff 19213

hgext/lfs/__init__.py

def debuglfsupload(ui, repo, **opts): def debuglfsupload(ui, repo, **opts):
"""upload lfs blobs added by the working copy parent or given revisions""" """upload lfs blobs added by the working copy parent or given revisions"""
revs = opts.get('rev', []) revs = opts.get('rev', [])
pointers = wrapper.extractpointers(repo, scmutil.revrange(repo, revs)) pointers = wrapper.extractpointers(repo, scmutil.revrange(repo, revs))
wrapper.uploadblobs(repo, pointers) wrapper.uploadblobs(repo, pointers)
@eh.wrapcommand( @eh.wrapcommand(
b'verify', opts=[(b'', b'no-lfs', None, _(b'skip all lfs blob content'))] b'verify',
opts=[(b'', b'no-lfs', None, _(b'skip missing lfs blob content'))],
) )
def verify(orig, ui, repo, **opts): def verify(orig, ui, repo, **opts):
skipflags = repo.ui.configint(b'verify', b'skipflags') skipflags = repo.ui.configint(b'verify', b'skipflags')
no_lfs = opts.pop('no_lfs') no_lfs = opts.pop('no_lfs')
if skipflags: if skipflags:
# --lfs overrides the config bit, if set. # --lfs overrides the config bit, if set.
if no_lfs is False: if no_lfs is False:

hgext/lfs/wrapper.py

if _islfs(self._revlog, rev=rev): if _islfs(self._revlog, rev=rev):
# fast path: use lfs metadata to answer size # fast path: use lfs metadata to answer size
rawtext = self._revlog.rawdata(rev) rawtext = self._revlog.rawdata(rev)
metadata = pointer.deserialize(rawtext) metadata = pointer.deserialize(rawtext)
return int(metadata[b'size']) return int(metadata[b'size'])
return orig(self, rev) return orig(self, rev)
@eh.wrapfunction(revlog, b'_verify_revision')
def _verify_revision(orig, rl, skipflags, state, node):
if _islfs(rl, node=node):
rawtext = rl.rawdata(node)
metadata = pointer.deserialize(rawtext)
# Don't skip blobs that are stored locally, as local verification is
# relatively cheap and there's no other way to verify the raw data in
# the revlog.
if rl.opener.lfslocalblobstore.has(metadata.oid()):
skipflags &= ~revlog.REVIDX_EXTSTORED
orig(rl, skipflags, state, node)
@eh.wrapfunction(context.basefilectx, b'cmp') @eh.wrapfunction(context.basefilectx, b'cmp')
def filectxcmp(orig, self, fctx): def filectxcmp(orig, self, fctx):
"""returns True if text is different than fctx""" """returns True if text is different than fctx"""
# some fctx (ex. hg-git) is not based on basefilectx and do not have islfs # some fctx (ex. hg-git) is not based on basefilectx and do not have islfs
if self.islfs() and getattr(fctx, 'islfs', lambda: False)(): if self.islfs() and getattr(fctx, 'islfs', lambda: False)():
# fast path: check LFS oid # fast path: check LFS oid
p1 = pointer.deserialize(self.rawdata()) p1 = pointer.deserialize(self.rawdata())
p2 = pointer.deserialize(fctx.rawdata()) p2 = pointer.deserialize(fctx.rawdata())

tests/test-lfs.t

Verify will not try to download lfs blobs, if told not to process lfs content Verify will not try to download lfs blobs, if told not to process lfs content
$ hg -R fromcorrupt --config lfs.usercache=emptycache verify -v --no-lfs $ hg -R fromcorrupt --config lfs.usercache=emptycache verify -v --no-lfs
repository uses revlog format 1 repository uses revlog format 1
checking changesets checking changesets
checking manifests checking manifests
crosschecking files in changesets and manifests crosschecking files in changesets and manifests
checking files checking files
lfs: found 22f66a3fc0b9bf3f012c814303995ec07099b3a9ce02a7af84b5970811074a3b in the local lfs store
checked 5 changesets with 10 changes to 4 files checked 5 changesets with 10 changes to 4 files
Verify will not try to download lfs blobs, if told not to by the config option Verify will not try to download lfs blobs, if told not to by the config option
$ hg -R fromcorrupt --config lfs.usercache=emptycache verify -v \ $ hg -R fromcorrupt --config lfs.usercache=emptycache verify -v \
> --config verify.skipflags=8192 > --config verify.skipflags=8192
repository uses revlog format 1 repository uses revlog format 1
checking changesets checking changesets
checking manifests checking manifests
crosschecking files in changesets and manifests crosschecking files in changesets and manifests
checking files checking files
lfs: found 22f66a3fc0b9bf3f012c814303995ec07099b3a9ce02a7af84b5970811074a3b in the local lfs store
checked 5 changesets with 10 changes to 4 files checked 5 changesets with 10 changes to 4 files
Verify will copy/link all lfs objects into the local store that aren't already Verify will copy/link all lfs objects into the local store that aren't already
present. Bypass the corrupted usercache to show that verify works when fed by present. Bypass the corrupted usercache to show that verify works when fed by
the (uncorrupted) remote store. the (uncorrupted) remote store.
$ hg -R fromcorrupt --config lfs.usercache=emptycache verify -v $ hg -R fromcorrupt --config lfs.usercache=emptycache verify -v
repository uses revlog format 1 repository uses revlog format 1