This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2067

changegroup: do not delta lfs revisions
ClosedPublic

Authored by quark on Feb 6 2018, 8:14 PM.

Details

Reviewers
indygreg
ryanmce
Group Reviewers
hg-reviewers
Commits
rHGd031609b3cb7: changegroup: do not delta lfs revisions
Summary

There is no way to distinguish whether a delta base is LFS or non-LFS.

If the delta is against LFS rawtext, and the client trying to apply it has
the base revision stored as fulltext, the delta (aka. bundle) will fail to
apply.

This patch forbids using delta for LFS revisions in changegroup so bad
deltas won't be transmitted.

Note: this does not solve the problem entirely. It solves LFS delta applying
to non-LFS base. But the other direction: non-LFS delta applying to LFS base
is not solved yet.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Feb 6 2018, 8:14 PM
quark updated this revision to Diff 5269.Feb 6 2018, 9:13 PM
quark updated this revision to Diff 5273.Feb 6 2018, 10:18 PM
indygreg requested changes to this revision.Feb 7 2018, 5:33 PM

This looks mostly good. I would like a change to address a future footgun though.

I would also appreciate someone familiar with censor and narrow to weigh in on the implications of disabling delta generation for revisions that have the censor and ellipsis flags set. I'm pretty sure narrow will cope since it reimplements changegroup generation. Not sure how censor will react. (But I know we already have random code for detecting censored nodes during changegroup generation.)

mercurial/revlog.py
719

This logic assumes that revision flags will only ever be used to influence the presence of content. That is true today because our flags are for REVIDX_ISCENSORED, REVIDX_ELLIPSIS, and REVIDX_EXTSTORED. But if we ever introduced a new revision flag for e.g. compression strategy, then testing for non-empty revision flags would be wrong.

I think we want to capture the bitmask of revision flags influencing delta generation explicitly. Or we want a big comment by the revision flags constants at the top of the file telling people to audit candelta() when adding new revision flags.

This revision now requires changes to proceed.Feb 7 2018, 5:33 PM
quark updated this revision to Diff 5327.Feb 7 2018, 7:35 PM
durin42 added a subscriber: durin42.Feb 13 2018, 1:35 PM
In D2067#34758, @indygreg wrote:

This looks mostly good. I would like a change to address a future footgun though.
I would also appreciate someone familiar with censor and narrow to weigh in on the implications of disabling delta generation for revisions that have the censor and ellipsis flags set. I'm pretty sure narrow will cope since it reimplements changegroup generation. Not sure how censor will react. (But I know we already have random code for detecting censored nodes during changegroup generation.)

Hmm, I could swear we already don't generate deltas for ellipsis nodes, and I'm having trouble defending the idea that maybe we should. Censor I'm 99% sure disables deltas. I'll ask adgar, but if you don't hear from me assume I was right.

martinvonz added a subscriber: martinvonz.Feb 13 2018, 1:57 PM
In D2067#34758, @indygreg wrote:

This looks mostly good. I would like a change to address a future footgun though.
I would also appreciate someone familiar with censor and narrow to weigh in on the implications of disabling delta generation for revisions that have the censor and ellipsis flags set. I'm pretty sure narrow will cope since it reimplements changegroup generation. Not sure how censor will react. (But I know we already have random code for detecting censored nodes during changegroup generation.)

I think narrow would be functionally fine with it disabled, but it would be terrible for storage space. We don't set the ellipsis flag on files, but we do on manifests (and we use tree manifests). Some directories have tens of thousands of entries and tens of thousands of entries and tens of thousands of revisions. Storing them all in full would be bad. We get a compression of close to 2000x (not a typo) on these directories. I just created a pretty large clone for testing this and one directory's revlog there ended up being 9MB instead of 15GB.

In D2067#36959, @durin42 wrote:
In D2067#34758, @indygreg wrote:

This looks mostly good. I would like a change to address a future footgun though.
I would also appreciate someone familiar with censor and narrow to weigh in on the implications of disabling delta generation for revisions that have the censor and ellipsis flags set. I'm pretty sure narrow will cope since it reimplements changegroup generation. Not sure how censor will react. (But I know we already have random code for detecting censored nodes during changegroup generation.)

Hmm, I could swear we already don't generate deltas for ellipsis nodes, and I'm having trouble defending the idea that maybe we should. Censor I'm 99% sure disables deltas. I'll ask adgar, but if you don't hear from me assume I was right.

quark updated this revision to Diff 5650.Feb 13 2018, 2:41 PM
ryanmce accepted this revision.Mar 3 2018, 2:36 PM
indygreg accepted this revision.Mar 3 2018, 2:40 PM
This revision is now accepted and ready to land.Mar 3 2018, 2:40 PM
ryanmce added inline comments.Mar 3 2018, 2:50 PM
mercurial/revlog.py
717

prev should be baserev?

quark added inline comments.Mar 3 2018, 2:53 PM
mercurial/revlog.py
717

Should be baserev.

quark updated this revision to Diff 6469.Mar 3 2018, 3:27 PM
indygreg accepted this revision.Mar 6 2018, 1:01 PM
indygreg added inline comments.
tests/test-lfs.t
352

Where did this change come from?

quark added inline comments.Mar 6 2018, 1:10 PM
tests/test-lfs.t
352

The number grows from 3 digits to 4 digits because of increased size, and one of the space becomes a digit.

Closed by commit rHGd031609b3cb7: changegroup: do not delta lfs revisions (authored by quark). · Explain WhyMar 6 2018, 1:10 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/changegroup.py (11 lines)
Mmercurial/revlog.py (7 lines)
Mtests/test-lfs-bundle.t (2 lines)
Mtests/test-lfs.t (2 lines)

Diff 5269

mercurial/changegroup.py

yield h yield h
for chunk in self.group(filenodes, filerevlog, lookupfilelog): for chunk in self.group(filenodes, filerevlog, lookupfilelog):
size += len(chunk) size += len(chunk)
yield chunk yield chunk
self._verbosenote(_('%8.i %s\n') % (size, fname)) self._verbosenote(_('%8.i %s\n') % (size, fname))
progress(msgbundling, None) progress(msgbundling, None)
def deltaparent(self, revlog, rev, p1, p2, prev): def deltaparent(self, revlog, rev, p1, p2, prev):
if not revlog.candelta(prev, rev):
raise error.ProgrammingError('cg1 should not be used in this case')
return prev return prev
def revchunk(self, revlog, rev, prev, linknode): def revchunk(self, revlog, rev, prev, linknode):
node = revlog.node(rev) node = revlog.node(rev)
p1, p2 = revlog.parentrevs(rev) p1, p2 = revlog.parentrevs(rev)
base = self.deltaparent(revlog, rev, p1, p2, prev) base = self.deltaparent(revlog, rev, p1, p2, prev)
prefix = '' prefix = ''
if dp == nullrev and revlog.storedeltachains: if dp == nullrev and revlog.storedeltachains:
# Avoid sending full revisions when delta parent is null. Pick prev # Avoid sending full revisions when delta parent is null. Pick prev
# in that case. It's tempting to pick p1 in this case, as p1 will # in that case. It's tempting to pick p1 in this case, as p1 will
# be smaller in the common case. However, computing a delta against # be smaller in the common case. However, computing a delta against
# p1 may require resolving the raw text of p1, which could be # p1 may require resolving the raw text of p1, which could be
# expensive. The revlog caches should have prev cached, meaning # expensive. The revlog caches should have prev cached, meaning
# less CPU for changegroup generation. There is likely room to add # less CPU for changegroup generation. There is likely room to add
# a flag and/or config option to control this behavior. # a flag and/or config option to control this behavior.
return prev base = prev
elif dp == nullrev: elif dp == nullrev:
# revlog is configured to use full snapshot for a reason, # revlog is configured to use full snapshot for a reason,
# stick to full snapshot. # stick to full snapshot.
return nullrev base = nullrev
elif dp not in (p1, p2, prev): elif dp not in (p1, p2, prev):
# Pick prev when we can't be sure remote has the base revision. # Pick prev when we can't be sure remote has the base revision.
return prev return prev
else: else:
return dp base = dp
if base != nullrev and not revlog.candelta(base, rev):
base = nullrev
return base
def builddeltaheader(self, node, p1n, p2n, basenode, linknode, flags): def builddeltaheader(self, node, p1n, p2n, basenode, linknode, flags):
# Do nothing with flags, it is implicitly 0 in cg1 and cg2 # Do nothing with flags, it is implicitly 0 in cg1 and cg2
return struct.pack(self.deltaheader, node, p1n, p2n, basenode, linknode) return struct.pack(self.deltaheader, node, p1n, p2n, basenode, linknode)
class cg3packer(cg2packer): class cg3packer(cg2packer):
version = '03' version = '03'
deltaheader = _CHANGEGROUPV3_DELTA_HEADER deltaheader = _CHANGEGROUPV3_DELTA_HEADER

mercurial/revlog.py

def hasnode(self, node): def hasnode(self, node):
try: try:
self.rev(node) self.rev(node)
return True return True
except KeyError: except KeyError:
return False return False
def candelta(self, baserev, rev):
"""whether two revisions (prev, rev) can be delta-ed or not"""
ryanmceUnsubmitted
Not Done

prev should be baserev?

ryanmce: `prev` should be `baserev`?
quarkAuthorUnsubmitted
Not Done

Should be baserev.

quark: Should be `baserev`.
# disable delta if either rev uses non-default flag (ex. LFS)
if self.flags(baserev) or self.flags(rev):
indygregUnsubmitted
Not Done

This logic assumes that revision flags will only ever be used to influence the presence of content. That is true today because our flags are for REVIDX_ISCENSORED, REVIDX_ELLIPSIS, and REVIDX_EXTSTORED. But if we ever introduced a new revision flag for e.g. compression strategy, then testing for non-empty revision flags would be wrong.

I think we want to capture the bitmask of revision flags influencing delta generation explicitly. Or we want a big comment by the revision flags constants at the top of the file telling people to audit candelta() when adding new revision flags.

indygreg: This logic assumes that revision flags will only ever be used to influence the presence of…
return False
return True
def clearcaches(self): def clearcaches(self):
self._cache = None self._cache = None
self._chainbasecache.clear() self._chainbasecache.clear()
self._chunkcache = (0, '') self._chunkcache = (0, '')
self._pcache = {} self._pcache = {}
try: try:
self._nodecache.clearcaches() self._nodecache.clearcaches()

tests/test-lfs-bundle.t

---- Applying src-normal.bundle to dst-normal ---- ---- Applying src-normal.bundle to dst-normal ----
OK OK
---- Applying src-normal.bundle to dst-lfs ---- ---- Applying src-normal.bundle to dst-lfs ----
X@2: unpacking 0609652b7877: integrity check failed on data/X.i:2 X@2: unpacking 0609652b7877: integrity check failed on data/X.i:2
Y@2: unpacking e384812cdeb9: integrity check failed on data/Y.i:2 Y@2: unpacking e384812cdeb9: integrity check failed on data/Y.i:2
2 integrity errors encountered! 2 integrity errors encountered!
(first damaged changeset appears to be 2) (first damaged changeset appears to be 2)
---- Applying src-lfs.bundle to dst-normal ---- ---- Applying src-lfs.bundle to dst-normal ----
CRASHED OK
---- Applying src-lfs.bundle to dst-lfs ---- ---- Applying src-lfs.bundle to dst-lfs ----
OK OK

tests/test-lfs.t

lfs: found 5ab7a3739a5feec94a562d070a14f36dba7cad17e5484a4a89eea8e5f3166888 in the local lfs store lfs: found 5ab7a3739a5feec94a562d070a14f36dba7cad17e5484a4a89eea8e5f3166888 in the local lfs store
lfs: found a9c7d1cd6ce2b9bbdf46ed9a862845228717b921c089d0d42e3bcaed29eb612e in the local lfs store lfs: found a9c7d1cd6ce2b9bbdf46ed9a862845228717b921c089d0d42e3bcaed29eb612e in the local lfs store
lfs: found f693890c49c409ec33673b71e53f297681f76c1166daf33b2ad7ebf8b1d3237e in the local lfs store lfs: found f693890c49c409ec33673b71e53f297681f76c1166daf33b2ad7ebf8b1d3237e in the local lfs store
lfs: found fda198fea753eb66a252e9856915e1f5cddbe41723bd4b695ece2604ad3c9f75 in the local lfs store lfs: found fda198fea753eb66a252e9856915e1f5cddbe41723bd4b695ece2604ad3c9f75 in the local lfs store
4 changesets found 4 changesets found
uncompressed size of bundle content: uncompressed size of bundle content:
* (changelog) (glob) * (changelog) (glob)
* (manifests) (glob) * (manifests) (glob)
* a (glob) * a (glob)
indygregUnsubmitted
Not Done

Where did this change come from?

indygreg: Where did this change come from?
quarkAuthorUnsubmitted
Not Done

The number grows from 3 digits to 4 digits because of increased size, and one of the space becomes a digit.

quark: The number grows from 3 digits to 4 digits because of increased size, and one of the space…
$ hg --config extensions.strip= strip -r 2 --no-backup --force -q $ hg --config extensions.strip= strip -r 2 --no-backup --force -q
$ hg -R bundle.hg log -p -T '{rev} {desc}\n' a $ hg -R bundle.hg log -p -T '{rev} {desc}\n' a
5 branching 5 branching
diff --git a/a b/a diff --git a/a b/a
--- a/a --- a/a
+++ b/a +++ b/a
@@ -1,1 +1,1 @@ @@ -1,1 +1,1 @@
-THIS-IS-LFS-two -THIS-IS-LFS-two