This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6814

revlog: add a `sidedata` parameters to addrevision
ClosedPublic

Authored by marmoute on Sep 7 2019, 5:28 AM.

Details

Reviewers
yuja
durin42
indygreg
Group Reviewers
hg-reviewers
Commits
rHG33532939c667: revlog: add a `sidedata` parameters to addrevision
Summary

If we want to eventually store sidedata we need to be able to pass them along.

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 created this revision.Sep 7 2019, 5:28 AM
indygreg added inline comments.Sep 7 2019, 1:12 PM
hgext/remotefilelog/remotefilelog.py
132–133

Why an empty tuple here? Isn't None more Pythonic?

marmoute added inline comments.Sep 7 2019, 1:15 PM
hgext/remotefilelog/remotefilelog.py
132–133

because it can be iterated over as a noop.

durin42 requested changes to this revision.Sep 9 2019, 1:52 PM
durin42 added inline comments.
hgext/remotefilelog/remotefilelog.py
132–133

I agree with Greg: if your intent is "optional iterable of data", the tuple is misguided and will cause problems for typecheckers. I know it adds two lines, but None is the correct way to spell "optional value that was empty" in this case.

This revision now requires changes to proceed.Sep 9 2019, 1:52 PM
marmoute added a comment.Sep 9 2019, 1:56 PM

Okay, I'll update it.

marmoute updated this revision to Diff 16480.Sep 9 2019, 5:23 PM
marmoute updated this revision to Diff 16494.Sep 9 2019, 7:08 PM
durin42 requested changes to this revision.Sep 17 2019, 2:14 PM
durin42 added inline comments.
hgext/remotefilelog/remotefilelog.py
132–133

You fixed the default in revlog.py but not in remotefilelog.py.

This revision now requires changes to proceed.Sep 17 2019, 2:14 PM
marmoute updated this revision to Diff 16600.Sep 24 2019, 12:44 PM
marmoute marked an inline comment as done.Sep 24 2019, 12:47 PM
marmoute updated this revision to Diff 16601.
marmoute added a commit: rHG33532939c667: revlog: add a `sidedata` parameters to addrevision.Sep 27 2019, 11:05 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG33532939c667: revlog: add a `sidedata` parameters to addrevision (authored by marmoute). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 16634

hgext/remotefilelog/remotefilelog.py

queue.append(pa2) queue.append(pa2)
visited.add(pa2) visited.add(pa2)
data += ancestortext data += ancestortext
return data return data
def addrevision(self, text, transaction, linknode, p1, p2, cachedelta=None, def addrevision(self, text, transaction, linknode, p1, p2, cachedelta=None,
node=None, flags=revlog.REVIDX_DEFAULT_FLAGS): node=None, flags=revlog.REVIDX_DEFAULT_FLAGS,
sidedata=None):
indygregUnsubmitted
Not Done

Why an empty tuple here? Isn't None more Pythonic?

indygreg: Why an empty tuple here? Isn't `None` more Pythonic?
marmouteAuthorUnsubmitted
Done

because it can be iterated over as a noop.

marmoute: because it can be iterated over as a noop.
durin42Unsubmitted
Not Done

I agree with Greg: if your intent is "optional iterable of data", the tuple is misguided and will cause problems for typecheckers. I know it adds two lines, but None is the correct way to spell "optional value that was empty" in this case.

durin42: I agree with Greg: if your intent is "optional iterable of data", the tuple is misguided and…
durin42Unsubmitted
Done

You fixed the default in revlog.py but not in remotefilelog.py.

durin42: You fixed the default in revlog.py but not in remotefilelog.py.
# text passed to "addrevision" includes hg filelog metadata header # text passed to "addrevision" includes hg filelog metadata header
if node is None: if node is None:
node = storageutil.hashrevisionsha1(text, p1, p2) node = storageutil.hashrevisionsha1(text, p1, p2)
if sidedata is None:
sidedata = {}
meta, metaoffset = storageutil.parsemeta(text) meta, metaoffset = storageutil.parsemeta(text)
rawtext, validatehash = self._processflagswrite(text, flags) rawtext, validatehash = self._processflagswrite(text, flags)
return self.addrawrevision(rawtext, transaction, linknode, p1, p2, return self.addrawrevision(rawtext, transaction, linknode, p1, p2,
node, flags, cachedelta, node, flags, cachedelta,
_metatuple=(meta, metaoffset)) _metatuple=(meta, metaoffset))
def addrawrevision(self, rawtext, transaction, linknode, p1, p2, node, def addrawrevision(self, rawtext, transaction, linknode, p1, p2, node,

mercurial/revlog.py

tr.replace(self.indexfile, trindex * self._io.size) tr.replace(self.indexfile, trindex * self._io.size)
self._chunkclear() self._chunkclear()
def _nodeduplicatecallback(self, transaction, node): def _nodeduplicatecallback(self, transaction, node):
"""called when trying to add a node already stored. """called when trying to add a node already stored.
""" """
def addrevision(self, text, transaction, link, p1, p2, cachedelta=None, def addrevision(self, text, transaction, link, p1, p2, cachedelta=None,
node=None, flags=REVIDX_DEFAULT_FLAGS, deltacomputer=None): node=None, flags=REVIDX_DEFAULT_FLAGS, deltacomputer=None,
sidedata=None):
"""add a revision to the log """add a revision to the log
text - the revision data to add text - the revision data to add
transaction - the transaction object used for rollback transaction - the transaction object used for rollback
link - the linkrev data to add link - the linkrev data to add
p1, p2 - the parent nodeids of the revision p1, p2 - the parent nodeids of the revision
cachedelta - an optional precomputed delta cachedelta - an optional precomputed delta
node - nodeid of revision; typically node is not specified, and it is node - nodeid of revision; typically node is not specified, and it is
computed by default as hash(text, p1, p2), however subclasses might computed by default as hash(text, p1, p2), however subclasses might
use different hashing method (and override checkhash() in such case) use different hashing method (and override checkhash() in such case)
flags - the known flags to set on the revision flags - the known flags to set on the revision
deltacomputer - an optional deltacomputer instance shared between deltacomputer - an optional deltacomputer instance shared between
multiple calls multiple calls
""" """
if link == nullrev: if link == nullrev:
raise error.RevlogError(_("attempted to add linkrev -1 to %s") raise error.RevlogError(_("attempted to add linkrev -1 to %s")
% self.indexfile) % self.indexfile)
if sidedata is None:
sidedata = {}
if flags: if flags:
node = node or self.hash(text, p1, p2) node = node or self.hash(text, p1, p2)
rawtext, validatehash = self._processflagswrite(text, flags) rawtext, validatehash = self._processflagswrite(text, flags)
# If the flag processor modifies the revision data, ignore any provided # If the flag processor modifies the revision data, ignore any provided
# cachedelta. # cachedelta.
if rawtext != text: if rawtext != text: