This is an archive of the discontinued Mercurial Phabricator instance.

Differential D11203

revlog: recommit 49fd21f32695 with a fix for issue6528
ClosedPublic

Authored by joerg.sonnenberger on Jul 20 2021, 9:57 AM.

Details

Reviewers
durin42
Alphare
Group Reviewers
hg-reviewers
Commits
rHG5b65721a75eb: revlog: recommit 49fd21f32695 with a fix for issue6528
rHG1b95e967e771: revlog: recommit 49fd21f32695 with a fix for issue6528
Summary

filelog.size currently special cases two forms of metadata encoding:

  • copy data via the parent order as flag bit
  • censor data by peaking into the raw delta

All other forms of metadata encoding including the empty metadata block
are mishandled. In basefilectx.cmp the empty metadata block is
explicitly checked to compensate for this.

Restore 49fd21f32695, but disable it for filelog, so that the original
flag bit use contines to work. Document all this mess for now in
preparation of a proper rework.

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

marmoute added a subscriber: marmoute.Jul 20 2021, 2:15 PM

This looks fine, but we should probably take it for 6.0 instead of 5.9 to stay on the safe side. Is there anything actively broken by not having this?

(I am also tempted to suggest we default canonical_parent_order to False too)

Regardless on this, we also need a script to fix parent order in repository where it got corrupt.

joerg.sonnenberger added a comment.Jul 20 2021, 3:36 PM

I would have to reaudit the parents use, but it wasn't just new code that misbehaves on misordered parents. E.g. memctx.parents for a trivial example. So yes, just backing it out introduces regressions as well.

Alphare accepted this revision.Aug 24 2021, 12:03 PM
Alphare added a subscriber: Alphare.

This looks fine (which is what I said last time... but really this time!).

This revision is now accepted and ready to land.Aug 24 2021, 12:03 PM
durin42 added a comment.Oct 18 2021, 3:16 PM

Seems like a good fix, needs rebased

Alphare added a comment.Tue, Mar 29, 8:52 AM

Should this really be for stable? Does this fix any known bug or can it be queued for default instead? Sorry if I forgot the answer to this question, but I see no indication of a bugfix in the commit itself.

mercurial/revlog.py
380

s/obuses/abuses/

joerg.sonnenberger marked an inline comment as done.Tue, Mar 29, 8:56 AM
joerg.sonnenberger updated this revision to Diff 32728.
joerg.sonnenberger added a comment.Tue, Mar 29, 8:57 AM

default should be fine. There are a number of places that don't necessarily work correctly for miss-ordered parents, but that's been the default behavior for years too, so nothing new.

Alphare added inline comments.Wed, Apr 6, 5:59 AM
mercurial/revlog.py
933

This should be removed, and nullid should be self.nullid. I'll amend on-the-fly, this has been too long in the works.

Revision Contents
Changeset List

Diff 32788

mercurial/context.py

raise error.ProgrammingError( raise error.ProgrammingError(
b'filectx.cmp() must be reimplemented if not backed by revlog' b'filectx.cmp() must be reimplemented if not backed by revlog'
) )
if fctx._filenode is None: if fctx._filenode is None:
if self._repo._encodefilterpats: if self._repo._encodefilterpats:
# can't rely on size() because wdir content may be decoded # can't rely on size() because wdir content may be decoded
return self._filelog.cmp(self._filenode, fctx.data()) return self._filelog.cmp(self._filenode, fctx.data())
# filelog.size() has two special cases:
# - censored metadata
# - copy/rename tracking
# The first is detected by peaking into the delta,
# the second is detected by abusing parent order
# in the revlog index as flag bit. This leaves files using
# the dummy encoding and non-standard meta attributes.
# The following check is a special case for the empty
# metadata block used if the raw file content starts with '\1\n'.
# Cases of arbitrary metadata flags are currently mishandled.
if self.size() - 4 == fctx.size(): if self.size() - 4 == fctx.size():
# size() can match: # size() can match:
# if file data starts with '\1\n', empty metadata block is # if file data starts with '\1\n', empty metadata block is
# prepended, which adds 4 bytes to filelog.size(). # prepended, which adds 4 bytes to filelog.size().
return self._filelog.cmp(self._filenode, fctx.data()) return self._filelog.cmp(self._filenode, fctx.data())
if self.size() == fctx.size() or self.flags() == b'l': if self.size() == fctx.size() or self.flags() == b'l':
# size() matches: need to compare content # size() matches: need to compare content
# issue6456: Always compare symlinks because size can represent # issue6456: Always compare symlinks because size can represent

mercurial/filelog.py

class filelog: class filelog:
def __init__(self, opener, path): def __init__(self, opener, path):
self._revlog = revlog.revlog( self._revlog = revlog.revlog(
opener, opener,
# XXX should use the unencoded path # XXX should use the unencoded path
target=(revlog_constants.KIND_FILELOG, path), target=(revlog_constants.KIND_FILELOG, path),
radix=b'/'.join((b'data', path)), radix=b'/'.join((b'data', path)),
censorable=True, censorable=True,
canonical_parent_order=False, # see comment in revlog.py
) )
# Full name of the user visible file, relative to the repository root. # Full name of the user visible file, relative to the repository root.
# Used by LFS. # Used by LFS.
self._revlog.filename = path self._revlog.filename = path
self.nullid = self._revlog.nullid self.nullid = self._revlog.nullid
opts = opener.options opts = opener.options
self._fix_issue6528 = opts.get(b'issue6528.fix-incoming', True) self._fix_issue6528 = opts.get(b'issue6528.fix-incoming', True)
# for revisions with renames, we have to go the slow way # for revisions with renames, we have to go the slow way
node = self.node(rev) node = self.node(rev)
if self.renamed(node): if self.renamed(node):
return len(self.read(node)) return len(self.read(node))
if self.iscensored(rev): if self.iscensored(rev):
return 0 return 0
# XXX if self.read(node).startswith("\1\n"), this returns (size+4) # XXX if self.read(node).startswith("\1\n"), this returns (size+4)
# XXX See also basefilectx.cmp.
return self._revlog.size(rev) return self._revlog.size(rev)
def cmp(self, node, text): def cmp(self, node, text):
"""compare text with a given file revision """compare text with a given file revision
returns True if text is different than what is stored. returns True if text is different than what is stored.
""" """
return not storageutil.filedataequivalent(self, node, text) return not storageutil.filedataequivalent(self, node, text)

mercurial/revlog.py

postfix=None, # only exist for `tmpcensored` now postfix=None, # only exist for `tmpcensored` now
checkambig=False, checkambig=False,
mmaplargeindex=False, mmaplargeindex=False,
censorable=False, censorable=False,
upperboundcomp=None, upperboundcomp=None,
persistentnodemap=False, persistentnodemap=False,
concurrencychecker=None, concurrencychecker=None,
trypending=False, trypending=False,
canonical_parent_order=True,
): ):
""" """
create a revlog object create a revlog object
opener is a function that abstracts the file opening operation opener is a function that abstracts the file opening operation
and can be used to implement COW semantics or the like. and can be used to implement COW semantics or the like.
`target`: a (KIND, ID) tuple that identify the content stored in `target`: a (KIND, ID) tuple that identify the content stored in
self._writinghandles = None self._writinghandles = None
# prevent nesting of addgroup # prevent nesting of addgroup
self._adding_group = None self._adding_group = None
self._loadindex() self._loadindex()
self._concurrencychecker = concurrencychecker self._concurrencychecker = concurrencychecker
# parent order is supposed to be semantically irrelevant, so we
# normally resort parents to ensure that the first parent is non-null,
# if there is a non-null parent at all.
# filelog abuses the parent order as flag to mark some instances of
AlphareUnsubmitted
Done

s/obuses/abuses/

Alphare: s/obuses/abuses/
# meta-encoded files, so allow it to disable this behavior.
self.canonical_parent_order = canonical_parent_order
def _init_opts(self): def _init_opts(self):
"""process options (from above/config) to setup associated default revlog mode """process options (from above/config) to setup associated default revlog mode
These values might be affected when actually reading on disk information. These values might be affected when actually reading on disk information.
The relevant values are returned for use in _loadindex(). The relevant values are returned for use in _loadindex().
* newversionflags: * newversionflags:
def parentrevs(self, rev): def parentrevs(self, rev):
try: try:
entry = self.index[rev] entry = self.index[rev]
except IndexError: except IndexError:
if rev == wdirrev: if rev == wdirrev:
raise error.WdirUnsupported raise error.WdirUnsupported
raise raise
if self.canonical_parent_order and entry[5] == nullrev:
return entry[6], entry[5]
else:
return entry[5], entry[6] return entry[5], entry[6]
# fast parentrevs(rev) where rev isn't filtered # fast parentrevs(rev) where rev isn't filtered
_uncheckedparentrevs = parentrevs _uncheckedparentrevs = parentrevs
def node(self, rev): def node(self, rev):
try: try:
return self.index[rev][7] return self.index[rev][7]
except IndexError: except IndexError:
if rev == wdirrev: if rev == wdirrev:
raise error.WdirUnsupported raise error.WdirUnsupported
raise raise
# Derived from index values. # Derived from index values.
def end(self, rev): def end(self, rev):
return self.start(rev) + self.length(rev) return self.start(rev) + self.length(rev)
def parents(self, node): def parents(self, node):
i = self.index i = self.index
d = i[self.rev(node)] d = i[self.rev(node)]
return i[d[5]][7], i[d[6]][7] # map revisions to nodes inline return i[d[5]][7], i[d[6]][7] # map revisions to nodes inline
AlphareUnsubmitted
Not Done

This should be removed, and nullid should be self.nullid. I'll amend on-the-fly, this has been too long in the works.

Alphare: This should be removed, and `nullid` should be `self.nullid`. I'll amend on-the-fly, this has…
# inline node() to avoid function call overhead
if self.canonical_parent_order and d[5] == nullid:
return i[d[6]][7], i[d[5]][7]
else:
return i[d[5]][7], i[d[6]][7]
def chainlen(self, rev): def chainlen(self, rev):
return self._chaininfo(rev)[0] return self._chaininfo(rev)[0]
def _chaininfo(self, rev): def _chaininfo(self, rev):
chaininfocache = self._chaininfocache chaininfocache = self._chaininfocache
if rev in chaininfocache: if rev in chaininfocache:
return chaininfocache[rev] return chaininfocache[rev]

tests/test-narrow-shallow-merges.t

| o 2 d1c61993ec83... outside 2d | o 2 d1c61993ec83... outside 2d
| |
o 1 bb96a08b062a inside 3 o 1 bb96a08b062a inside 3
| |
o 0 b844052e7b3b... outside 2c o 0 b844052e7b3b... outside 2c
$ hg log -T '{if(ellipsis,"...")}{node|short} {p1node|short} {p2node|short} {desc}\n' | sort $ hg log -T '{if(ellipsis,"...")}{node|short} {p1node|short} {p2node|short} {desc}\n' | sort
...2a20009de83e 000000000000 3ac1f5779de3 outside 10 ...2a20009de83e 3ac1f5779de3 000000000000 outside 10
...3ac1f5779de3 bb96a08b062a 465567bdfb2d merge a/b/c/d 9 ...3ac1f5779de3 bb96a08b062a 465567bdfb2d merge a/b/c/d 9
...8d874d57adea 7ef88b4dd4fa 000000000000 outside 12 ...8d874d57adea 7ef88b4dd4fa 000000000000 outside 12
...b844052e7b3b 000000000000 000000000000 outside 2c ...b844052e7b3b 000000000000 000000000000 outside 2c
...d1c61993ec83 000000000000 000000000000 outside 2d ...d1c61993ec83 000000000000 000000000000 outside 2d
465567bdfb2d d1c61993ec83 000000000000 inside 3 465567bdfb2d d1c61993ec83 000000000000 inside 3
7ef88b4dd4fa 2a20009de83e 000000000000 inside 11 7ef88b4dd4fa 2a20009de83e 000000000000 inside 11
bb96a08b062a b844052e7b3b 000000000000 inside 3 bb96a08b062a b844052e7b3b 000000000000 inside 3