This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4750

filelog: stop proxying rawsize() (API)
ClosedPublic

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

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGd909c44d29e1: filelog: stop proxying rawsize() (API)
Summary

This method is no longer used by external consumers. The API is
quite low-level and is effectively len(revision(raw=True)). I don't
see a compelling reason to keep it around.

Let's drop the API and make the file storage interface simpler.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

indygreg created this revision.Sep 26 2018, 2:28 PM
Closed by commit rHGd909c44d29e1: filelog: stop proxying rawsize() (API) (authored by indygreg). · Explain WhySep 26 2018, 5:05 PM
This revision was automatically updated to reflect the committed changes.
mharbison72 added a subscriber: mharbison72.Oct 5 2018, 12:37 AM

Apparently thg uses this to avoid loading the data into memory just to figure out the length like fctx.rawsize(). Any suggestions?

https://groups.google.com/d/msg/thg-dev/6ekYID7ho_o/fbdd6rVDAwAJ

indygreg added a subscriber: durin42.Oct 31 2018, 3:19 PM
In D4750#73744, @mharbison72 wrote:

Apparently thg uses this to avoid loading the data into memory just to figure out the length like fctx.rawsize(). Any suggestions?
https://groups.google.com/d/msg/thg-dev/6ekYID7ho_o/fbdd6rVDAwAJ

Just found this comment when going through mail backlog.

Do we want to do anything about this before the 4.8 release? We could restore the method/proxy easily enough. But I would prefer not to. I'd really like to know why thg wants to access the size of a revision without accessing the revision text...

CC @durin42

yuja added a subscriber: yuja.Nov 1 2018, 7:01 AM
> Apparently thg uses this to avoid loading the data into memory just to figure out the length like fctx.rawsize().  Any suggestions?
>
> https://groups.google.com/d/msg/thg-dev/6ekYID7ho_o/fbdd6rVDAwAJ
Just found this comment when going through mail backlog.
Do we want to do anything about this before the 4.8 release?

No. I've updated the caller to directly access to _revlog.

I'd really like to know why thg wants to access the size of a revision
without accessing the revision text...

In order to decide whether diff should be generated. There's a "max size
to show diff" config knob, and we don't want to load large blob into memory.

What we need is a light-weight method to get approximate data size.

Revision Contents
Changeset List

Diff 11405

mercurial/filelog.py

return self._revlog.children(node) return self._revlog.children(node)
def deltaparent(self, rev): def deltaparent(self, rev):
return self._revlog.deltaparent(rev) return self._revlog.deltaparent(rev)
def iscensored(self, rev): def iscensored(self, rev):
return self._revlog.iscensored(rev) return self._revlog.iscensored(rev)
# Unused.
def rawsize(self, rev):
return self._revlog.rawsize(rev)
# Might be unused. # Might be unused.
def checkhash(self, text, node, p1=None, p2=None, rev=None): def checkhash(self, text, node, p1=None, p2=None, rev=None):
return self._revlog.checkhash(text, node, p1=p1, p2=p2, rev=rev) return self._revlog.checkhash(text, node, p1=p1, p2=p2, rev=rev)
def revision(self, node, _df=None, raw=False): def revision(self, node, _df=None, raw=False):
return self._revlog.revision(node, _df=_df, raw=raw) return self._revlog.revision(node, _df=_df, raw=raw)
def revdiff(self, rev1, rev2): def revdiff(self, rev1, rev2):

mercurial/repository.py

""""Return the revision that is a suitable parent to delta against.""" """"Return the revision that is a suitable parent to delta against."""
class ifiledata(interfaceutil.Interface): class ifiledata(interfaceutil.Interface):
"""Storage interface for data storage of a specific file. """Storage interface for data storage of a specific file.
This complements ``ifileindex`` and provides an interface for accessing This complements ``ifileindex`` and provides an interface for accessing
data for a tracked file. data for a tracked file.
""" """
def rawsize(rev):
"""The size of the fulltext data for a revision as stored."""
def size(rev): def size(rev):
"""Obtain the fulltext size of file data. """Obtain the fulltext size of file data.
Any metadata is excluded from size measurements. Use ``rawsize()`` if Any metadata is excluded from size measurements.
metadata size is important.
""" """
def checkhash(fulltext, node, p1=None, p2=None, rev=None): def checkhash(fulltext, node, p1=None, p2=None, rev=None):
"""Validate the stored hash of a given fulltext and node. """Validate the stored hash of a given fulltext and node.
Raises ``error.StorageError`` is hash validation fails. Raises ``error.StorageError`` is hash validation fails.
""" """

mercurial/testing/storage.py

""" """
def testempty(self): def testempty(self):
f = self._makefilefn() f = self._makefilefn()
self.assertEqual(f.storageinfo(), {}) self.assertEqual(f.storageinfo(), {})
self.assertEqual(f.storageinfo(revisionscount=True, trackedsize=True), self.assertEqual(f.storageinfo(revisionscount=True, trackedsize=True),
{'revisionscount': 0, 'trackedsize': 0}) {'revisionscount': 0, 'trackedsize': 0})
self.assertEqual(f.rawsize(nullrev), 0)
for i in range(-5, 5):
if i == nullrev:
continue
with self.assertRaises(IndexError):
f.rawsize(i)
self.assertEqual(f.size(nullrev), 0) self.assertEqual(f.size(nullrev), 0)
for i in range(-5, 5): for i in range(-5, 5):
if i == nullrev: if i == nullrev:
continue continue
with self.assertRaises(IndexError): with self.assertRaises(IndexError):
f.size(i) f.size(i)
f = self._makefilefn() f = self._makefilefn()
with self._maketransactionfn() as tr: with self._maketransactionfn() as tr:
node = f.add(fulltext, None, tr, 0, nullid, nullid) node = f.add(fulltext, None, tr, 0, nullid, nullid)
self.assertEqual(f.storageinfo(), {}) self.assertEqual(f.storageinfo(), {})
self.assertEqual(f.storageinfo(revisionscount=True, trackedsize=True), self.assertEqual(f.storageinfo(revisionscount=True, trackedsize=True),
{'revisionscount': 1, 'trackedsize': len(fulltext)}) {'revisionscount': 1, 'trackedsize': len(fulltext)})
self.assertEqual(f.rawsize(0), len(fulltext))
with self.assertRaises(IndexError):
f.rawsize(1)
self.assertEqual(f.size(0), len(fulltext)) self.assertEqual(f.size(0), len(fulltext))
with self.assertRaises(IndexError): with self.assertRaises(IndexError):
f.size(1) f.size(1)
f.checkhash(fulltext, node) f.checkhash(fulltext, node)
f.checkhash(fulltext, node, nullid, nullid) f.checkhash(fulltext, node, nullid, nullid)
self.assertEqual(f.storageinfo(), {}) self.assertEqual(f.storageinfo(), {})
self.assertEqual( self.assertEqual(
f.storageinfo(revisionscount=True, trackedsize=True), f.storageinfo(revisionscount=True, trackedsize=True),
{ {
'revisionscount': 3, 'revisionscount': 3,
'trackedsize': len(fulltext0) + len(fulltext1) + len(fulltext2), 'trackedsize': len(fulltext0) + len(fulltext1) + len(fulltext2),
}) })
self.assertEqual(f.rawsize(0), len(fulltext0))
self.assertEqual(f.rawsize(1), len(fulltext1))
self.assertEqual(f.rawsize(2), len(fulltext2))
with self.assertRaises(IndexError):
f.rawsize(3)
self.assertEqual(f.size(0), len(fulltext0)) self.assertEqual(f.size(0), len(fulltext0))
self.assertEqual(f.size(1), len(fulltext1)) self.assertEqual(f.size(1), len(fulltext1))
self.assertEqual(f.size(2), len(fulltext2)) self.assertEqual(f.size(2), len(fulltext2))
with self.assertRaises(IndexError): with self.assertRaises(IndexError):
f.size(3) f.size(3)
f.checkhash(fulltext0, node0) f.checkhash(fulltext0, node0)
]) ])
f = self._makefilefn() f = self._makefilefn()
with self._maketransactionfn() as tr: with self._maketransactionfn() as tr:
node0 = f.add(fulltext0, None, tr, 0, nullid, nullid) node0 = f.add(fulltext0, None, tr, 0, nullid, nullid)
node1 = f.add(fulltext1, meta1, tr, 1, node0, nullid) node1 = f.add(fulltext1, meta1, tr, 1, node0, nullid)
node2 = f.add(fulltext2, meta2, tr, 2, nullid, nullid) node2 = f.add(fulltext2, meta2, tr, 2, nullid, nullid)
self.assertEqual(f.rawsize(1), len(stored1))
self.assertEqual(f.rawsize(2), len(stored2))
# Metadata header isn't recognized when parent isn't nullid. # Metadata header isn't recognized when parent isn't nullid.
self.assertEqual(f.size(1), len(stored1)) self.assertEqual(f.size(1), len(stored1))
self.assertEqual(f.size(2), len(fulltext2)) self.assertEqual(f.size(2), len(fulltext2))
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)
self.assertEqual(f.revision(node2), stored2) self.assertEqual(f.revision(node2), stored2)
self.assertEqual(f.revision(node2, raw=True), stored2) self.assertEqual(f.revision(node2, raw=True), stored2)
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)
self.assertEqual(f.rawsize(0), len(stored0))
self.assertEqual(f.rawsize(1), len(stored1))
# TODO this is buggy. # TODO this is buggy.
self.assertEqual(f.size(0), len(fulltext0) + 4) 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)

tests/simplestorerepo.py

validaterev(rev) validaterev(rev)
if ((self._flags(baserev) & revlog.REVIDX_RAWTEXT_CHANGING_FLAGS) if ((self._flags(baserev) & revlog.REVIDX_RAWTEXT_CHANGING_FLAGS)
or (self._flags(rev) & revlog.REVIDX_RAWTEXT_CHANGING_FLAGS)): or (self._flags(rev) & revlog.REVIDX_RAWTEXT_CHANGING_FLAGS)):
return False return False
return True return True
def rawsize(self, rev):
validaterev(rev)
node = self.node(rev)
return len(self.revision(node, raw=True))
def _processflags(self, text, flags, operation, raw=False): def _processflags(self, text, flags, operation, raw=False):
if flags == 0: if flags == 0:
return text, True return text, True
if flags & ~revlog.REVIDX_KNOWN_FLAGS: if flags & ~revlog.REVIDX_KNOWN_FLAGS:
raise simplestoreerror(_("incompatible revision flag '%#x'") % raise simplestoreerror(_("incompatible revision flag '%#x'") %
(flags & ~revlog.REVIDX_KNOWN_FLAGS)) (flags & ~revlog.REVIDX_KNOWN_FLAGS))

tests/test-lfs.t

> h.update(rawtext) > h.update(rawtext)
> return h.hexdigest()[:4] > return h.hexdigest()[:4]
> def reposetup(ui, repo): > def reposetup(ui, repo):
> # these 2 files are interesting > # these 2 files are interesting
> for name in ['l', 's']: > for name in ['l', 's']:
> fl = repo.file(name) > fl = repo.file(name)
> if len(fl) == 0: > if len(fl) == 0:
> continue > continue
> sizes = [fl.rawsize(i) for i in fl] > sizes = [fl._revlog.rawsize(i) for i in fl]
> texts = [fl.revision(i, raw=True) for i in fl] > texts = [fl.revision(i, raw=True) for i in fl]
> flags = [int(fl._revlog.flags(i)) for i in fl] > flags = [int(fl._revlog.flags(i)) for i in fl]
> hashes = [hash(t) for t in texts] > hashes = [hash(t) for t in texts]
> print(' %s: rawsizes=%r flags=%r hashes=%r' > print(' %s: rawsizes=%r flags=%r hashes=%r'
> % (name, sizes, flags, hashes)) > % (name, sizes, flags, hashes))
> EOF > EOF
$ for i in client client2 server repo3 repo4 repo5 repo6 repo7 repo8 repo9 \ $ for i in client client2 server repo3 repo4 repo5 repo6 repo7 repo8 repo9 \