Patchwork D7712: lfs: don't skip locally available blobs when verifying

login
register
mail settings
Submitter phabricator
Date Dec. 23, 2019, 7:48 a.m.
Message ID <differential-rev-PHID-DREV-awjjhwpotn35w6xxx44w-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/44020/
State Superseded
Headers show

Comments

phabricator - Dec. 23, 2019, 7:48 a.m.
mharbison72 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The `skipflags` config was introduced in a2ab9ebcd85b <https://phab.mercurial-scm.org/rHGa2ab9ebcd85b09e33d3036ceccb2dff6f6353e79>, 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.)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D7712

AFFECTED FILES
  hgext/lfs/__init__.py
  hgext/lfs/wrapper.py
  tests/test-lfs.t

CHANGE DETAILS




To: mharbison72, #hg-reviewers
Cc: mercurial-devel
phabricator - Dec. 23, 2019, 5:39 p.m.
indygreg added a comment.


  This seems good. But planned changes in D7708 <https://phab.mercurial-scm.org/D7708> will create a merge conflict. So holding off on accepting for now.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7712/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7712

To: mharbison72, #hg-reviewers
Cc: indygreg, mercurial-devel
phabricator - Jan. 8, 2020, 8 p.m.
This revision now requires changes to proceed.
durin42 added a comment.
durin42 requested changes to this revision.


  Needs rebased.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D7712/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D7712

To: mharbison72, #hg-reviewers, durin42
Cc: durin42, indygreg, mercurial-devel

Patch

diff --git a/tests/test-lfs.t b/tests/test-lfs.t
--- a/tests/test-lfs.t
+++ b/tests/test-lfs.t
@@ -804,6 +804,7 @@ 
   checking manifests
   crosschecking files in changesets and manifests
   checking files
+  lfs: found 22f66a3fc0b9bf3f012c814303995ec07099b3a9ce02a7af84b5970811074a3b in the local lfs store
   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
@@ -815,6 +816,7 @@ 
   checking manifests
   crosschecking files in changesets and manifests
   checking files
+  lfs: found 22f66a3fc0b9bf3f012c814303995ec07099b3a9ce02a7af84b5970811074a3b in the local lfs store
   checked 5 changesets with 10 changes to 4 files
 
 Verify will copy/link all lfs objects into the local store that aren't already
diff --git a/hgext/lfs/wrapper.py b/hgext/lfs/wrapper.py
--- a/hgext/lfs/wrapper.py
+++ b/hgext/lfs/wrapper.py
@@ -225,6 +225,21 @@ 
     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')
 def filectxcmp(orig, self, fctx):
     """returns True if text is different than fctx"""
diff --git a/hgext/lfs/__init__.py b/hgext/lfs/__init__.py
--- a/hgext/lfs/__init__.py
+++ b/hgext/lfs/__init__.py
@@ -405,7 +405,8 @@ 
 
 
 @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):
     skipflags = repo.ui.configint(b'verify', b'skipflags')