This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7708

lfs: add a switch to `hg verify` to ignore the content of blobs
ClosedPublic

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

Details

Reviewers
indygreg
pulkit
Group Reviewers
hg-reviewers
Commits
rHG34e8305f02bd: lfs: add a switch to `hg verify` to ignore the content of blobs
rHGedf6ea7db7c5: lfs: add a switch to `hg verify` to ignore the content of blobs
rHGe540103ce945: lfs: add a switch to `hg verify` to ignore the content of blobs
Summary

Trying to validate the fulltext of an external revision causes missing blobs to
be downloaded and cached. Since the downloads aren't batch prefetched[1] and
aren't compressed, this can be expensive both in terms of time and space.

I made this a tri-state instead of a simple bool because there's an existing
(undocumented) config to handle this, and it would be weird if hg verify were
to suddenly start ignoring that config but an hg recover initiated verify
honors it. Since this uses the same config setting, it too will skip
rename verification (which requires fulltext, but not for LFS).

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

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
pulkit added a subscriber: pulkit.Dec 23 2019, 12:06 PM
pulkit added inline comments.
hgext/lfs/__init__.py
408

IIUC, we can have a --lfs flag and --no-lfs will work.

Also by default, we may not want to verify large files content?

indygreg requested changes to this revision.Dec 23 2019, 12:32 PM
indygreg added a subscriber: indygreg.

Per Pulkit's review, please change the argument to --lfs, as --no-lfs should automatically work.

Also, I didn't realize verify.skipflags existed and exposes low-level bit flags of revlog storage. That's a massive abstraction leak and I'd be in favor of reinventing that feature so it worked based on string feature names.

This revision now requires changes to proceed.Dec 23 2019, 12:32 PM
mharbison72 added a comment.Dec 23 2019, 1:30 PM
In D7708#113600, @indygreg wrote:

Per Pulkit's review, please change the argument to --lfs, as --no-lfs should automatically work.

Does my response to @pulkit change your mind any? (Not sure how familiar you are with the lfs internals- I apparently forgot a lot myself).

Also, I didn't realize verify.skipflags existed and exposes low-level bit flags of revlog storage. That's a massive abstraction leak and I'd be in favor of reinventing that feature so it worked based on string feature names.

Any thumbnail sketch of what this should look like? Having to know that things are externally stored or that rename checking has special requirements for example, also seems leaky to a lesser degree. I was surprised that revlog bits were being passed around like this too. My plan is/was to add an lfs configbool that's documented, which could toggle this on or off, so users don't need to know bit patterns. (I was also surprised that hex numbers in the config don't work, and it pukes when you fetch the value.) Obviously I wouldn't want the lack of --lfs or --no-lfs to override this. IDK if it's better to have an LFS config for it's bit, and then another for that other bit... or some setting that is a list of skip names. I assume whatever pattern is designed here would influence the internal replacement.

hgext/lfs/__init__.py
408

The way it is now, both --lfs and --no-lfs are accepted. I actually started with --lfs, defaulting to on, but then changed it and forget why. I'll try to figure that out today, but it may have been the code was simpler when defaulting to verifying the blobs and honoring the config setting. It may have also had to do with shifting to verifying the locally accessible blobs later in the series. I didn't want to over complicate things with --none, --local-only, and --all. The options with largefiles are kind of annoying. I guess the question is, "What would an average user who doesn't know hg internals want/understand?"

As far as defaulting to *not* verifying blobs, I'm uncertain. I'd think that verifying local blobs wouldn't add much overhead. But the deciding factor for me was that there's no way to verify the content in the revlog without verifying the blob itself- the stored hash in the revlog is for the blob, not the stored JSON data. That's why I asked in IRC the other day about how strong the guarantees are about repo integrity after running a verify. IIRC, largefiles doesn't verify content by default, but it will at least make sure the revlog for the standin file is OK.

mharbison72 requested review of this revision.Jan 8 2020, 11:37 PM

Setting this back to wanting a review because I suspect my response questions were overlooked with vacations, and it won't show up in anybody's dashboard otherwise.

pulkit accepted this revision.Jan 14 2020, 10:01 AM
mharbison72 added a commit: rHGe540103ce945: lfs: add a switch to `hg verify` to ignore the content of blobs.Jan 14 2020, 11:13 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGe540103ce945: lfs: add a switch to `hg verify` to ignore the content of blobs (authored by mharbison72). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/lfs/__init__.py (21 lines)
Mtests/test-lfs.t (21 lines)

Diff 19212

hgext/lfs/__init__.py

b'debuglfsupload', b'debuglfsupload',
[(b'r', b'rev', [], _(b'upload large files introduced by REV'))], [(b'r', b'rev', [], _(b'upload large files introduced by REV'))],
) )
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(
b'verify', opts=[(b'', b'no-lfs', None, _(b'skip all lfs blob content'))]
pulkitUnsubmitted
Not Done

IIUC, we can have a --lfs flag and --no-lfs will work.

Also by default, we may not want to verify large files content?

pulkit: IIUC, we can have a `--lfs` flag and `--no-lfs` will work. Also by default, we may not want to…
mharbison72AuthorUnsubmitted
Not Done

The way it is now, both --lfs and --no-lfs are accepted. I actually started with --lfs, defaulting to on, but then changed it and forget why. I'll try to figure that out today, but it may have been the code was simpler when defaulting to verifying the blobs and honoring the config setting. It may have also had to do with shifting to verifying the locally accessible blobs later in the series. I didn't want to over complicate things with --none, --local-only, and --all. The options with largefiles are kind of annoying. I guess the question is, "What would an average user who doesn't know hg internals want/understand?"

As far as defaulting to *not* verifying blobs, I'm uncertain. I'd think that verifying local blobs wouldn't add much overhead. But the deciding factor for me was that there's no way to verify the content in the revlog without verifying the blob itself- the stored hash in the revlog is for the blob, not the stored JSON data. That's why I asked in IRC the other day about how strong the guarantees are about repo integrity after running a verify. IIRC, largefiles doesn't verify content by default, but it will at least make sure the revlog for the standin file is OK.

mharbison72: The way it is now, both `--lfs` and `--no-lfs` are accepted. I actually started with `--lfs`…
)
def verify(orig, ui, repo, **opts):
skipflags = repo.ui.configint(b'verify', b'skipflags')
no_lfs = opts.pop('no_lfs')
if skipflags:
# --lfs overrides the config bit, if set.
if no_lfs is False:
skipflags &= ~repository.REVISION_FLAG_EXTSTORED
else:
skipflags = 0
if no_lfs is True:
skipflags |= repository.REVISION_FLAG_EXTSTORED
with ui.configoverride({(b'verify', b'skipflags'): skipflags}):
return orig(ui, repo, **opts)

tests/test-lfs.t

resolving manifests resolving manifests
getting l getting l
lfs: found 22f66a3fc0b9bf3f012c814303995ec07099b3a9ce02a7af84b5970811074a3b in the usercache lfs: found 22f66a3fc0b9bf3f012c814303995ec07099b3a9ce02a7af84b5970811074a3b in the usercache
getting s getting s
2 files updated, 0 files merged, 0 files removed, 0 files unresolved 2 files updated, 0 files merged, 0 files removed, 0 files unresolved
$ test -f fromcorrupt/.hg/store/lfs/objects/66/100b384bf761271b407d79fc30cdd0554f3b2c5d944836e936d584b88ce88e $ test -f fromcorrupt/.hg/store/lfs/objects/66/100b384bf761271b407d79fc30cdd0554f3b2c5d944836e936d584b88ce88e
[1] [1]
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
repository uses revlog format 1
checking changesets
checking manifests
crosschecking files in changesets and manifests
checking 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
$ hg -R fromcorrupt --config lfs.usercache=emptycache verify -v \
> --config verify.skipflags=8192
repository uses revlog format 1
checking changesets
checking manifests
crosschecking files in changesets and manifests
checking 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
checking changesets checking changesets
checking manifests checking manifests