This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4752

filelog: return correct size when content begins with metadata prefix
ClosedPublic

Authored by indygreg on Sep 26 2018, 2:28 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG91d2233d3dec: filelog: return correct size when content begins with metadata prefix
Summary

An inline TODO tracked this. And we were reminded of it when
recently implementing storage integration tests. Let's fix it so
we don't have to port the buggy behavior to future storage backends.

The new call to read() should be fast because the revision
fulltext should be cached as part of calling renamed(). So the
overhead here should be minimal.

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

indygreg created this revision.Sep 26 2018, 2:28 PM
Closed by commit rHG91d2233d3dec: filelog: return correct size when content begins with metadata prefix (authored by indygreg). · Explain WhySep 26 2018, 5:12 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Sep 29 2018, 12:46 AM
The new call to read() should be fast because the revision
fulltext should be cached as part of calling renamed(). So the
overhead here should be minimal.

That isn't true unfortunately. renamed() reads the text only if p1 is null,
so size() was cheap in most cases. This patch makes size() as slow as
len(read()).

indygreg added a comment.Oct 1 2018, 12:29 PM

Jun also had concerns about the perf implications of this changeset. We decided to drop this from hg-committed.

Revision Contents
Changeset List

Diff 11428

mercurial/filelog.py

# 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) size = self._revlog.size(rev)
return self._revlog.size(rev)
if self.read(node).startswith(b'\x01\n'):
size -= 4
return size
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.
""" """
t = text t = text

mercurial/testing/storage.py

b'\x01\n\x01\nbar', b'\x01\n\x01\nbar',
]) ])
f = self._makefilefn() f = self._makefilefn()
with self._maketransactionfn() as tr: with self._maketransactionfn() as tr:
node0 = f.add(fulltext0, {}, tr, 0, nullid, nullid) node0 = f.add(fulltext0, {}, tr, 0, nullid, nullid)
node1 = f.add(fulltext1, meta1, tr, 1, nullid, nullid) node1 = f.add(fulltext1, meta1, tr, 1, nullid, nullid)
# TODO this is buggy. self.assertEqual(f.size(0), len(fulltext0))
self.assertEqual(f.size(0), len(fulltext0) + 4)
self.assertEqual(f.size(1), len(fulltext1)) self.assertEqual(f.size(1), len(fulltext1))
self.assertEqual(f.revision(node0), stored0) self.assertEqual(f.revision(node0), stored0)
self.assertEqual(f.revision(node0, raw=True), stored0) self.assertEqual(f.revision(node0, raw=True), stored0)
self.assertEqual(f.revision(node1), stored1) self.assertEqual(f.revision(node1), stored1)
self.assertEqual(f.revision(node1, raw=True), stored1) self.assertEqual(f.revision(node1, raw=True), stored1)

tests/test-filelog.py.out

ERROR: FIXME: This is a known failure of filelog.size for data starting with \1\n
OK. OK.