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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
722

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
720

prev should be baserev?

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

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 (17 lines)
Mtests/test-lfs-bundle.t (2 lines)
Mtests/test-lfs.t (2 lines)

Diff 6670

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

REVIDX_DEFAULT_FLAGS = 0 REVIDX_DEFAULT_FLAGS = 0
# stable order in which flags need to be processed and their processors applied # stable order in which flags need to be processed and their processors applied
REVIDX_FLAGS_ORDER = [ REVIDX_FLAGS_ORDER = [
REVIDX_ISCENSORED, REVIDX_ISCENSORED,
REVIDX_ELLIPSIS, REVIDX_ELLIPSIS,
REVIDX_EXTSTORED, REVIDX_EXTSTORED,
] ]
REVIDX_KNOWN_FLAGS = util.bitsfrom(REVIDX_FLAGS_ORDER) REVIDX_KNOWN_FLAGS = util.bitsfrom(REVIDX_FLAGS_ORDER)
# bitmark for flags that could cause rawdata content change
REVIDX_RAWTEXT_CHANGING_FLAGS = REVIDX_ISCENSORED | REVIDX_EXTSTORED
# max size of revlog with inline data # max size of revlog with inline data
_maxinline = 131072 _maxinline = 131072
_chunksize = 1048576 _chunksize = 1048576
RevlogError = error.RevlogError RevlogError = error.RevlogError
LookupError = error.LookupError LookupError = error.LookupError
CensoredNodeError = error.CensoredNodeError CensoredNodeError = error.CensoredNodeError
ProgrammingError = error.ProgrammingError ProgrammingError = error.ProgrammingError
# Store flag processors (cf. 'addflagprocessor()' to register) # Store flag processors (cf. 'addflagprocessor()' to register)
_flagprocessors = { _flagprocessors = {
REVIDX_ISCENSORED: None, REVIDX_ISCENSORED: None,
} }
def addflagprocessor(flag, processor): def addflagprocessor(flag, processor):
"""Register a flag processor on a revision data flag. """Register a flag processor on a revision data flag.
Invariant: Invariant:
- Flags need to be defined in REVIDX_KNOWN_FLAGS and REVIDX_FLAGS_ORDER. - Flags need to be defined in REVIDX_KNOWN_FLAGS and REVIDX_FLAGS_ORDER,
and REVIDX_RAWTEXT_CHANGING_FLAGS if they can alter rawtext.
- Only one flag processor can be registered on a specific flag. - Only one flag processor can be registered on a specific flag.
- flagprocessors must be 3-tuples of functions (read, write, raw) with the - flagprocessors must be 3-tuples of functions (read, write, raw) with the
following signatures: following signatures:
- (read) f(self, rawtext) -> text, bool - (read) f(self, rawtext) -> text, bool
- (write) f(self, text) -> rawtext, bool - (write) f(self, text) -> rawtext, bool
- (raw) f(self, rawtext) -> bool - (raw) f(self, rawtext) -> bool
"text" is presented to the user. "rawtext" is stored in revlog data, not "text" is presented to the user. "rawtext" is stored in revlog data, not
directly visible to the user. directly visible to the user.
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 (baserev, 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 requires a content-changing flag
# processor (ex. LFS). This is because such flag processor can alter
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…
# the rawtext content that the delta will be based on, and two clients
# could have a same revlog node with different flags (i.e. different
# rawtext contents) and the delta could be incompatible.
if ((self.flags(baserev) & REVIDX_RAWTEXT_CHANGING_FLAGS)
or (self.flags(rev) & REVIDX_RAWTEXT_CHANGING_FLAGS)):
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 bcc7d23fa6b7: integrity check failed on data/X.i:2 X@2: unpacking bcc7d23fa6b7: integrity check failed on data/X.i:2
Y@2: unpacking 46017a6640e7: integrity check failed on data/Y.i:2 Y@2: unpacking 46017a6640e7: 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