On 12/24/2016 05:36 PM, Remi Chaintron wrote:
# HG changeset patch
# User Remi Chaintron <r...@fb.com>
# Date 1482539184 18000
# Fri Dec 23 19:26:24 2016 -0500
# Node ID d0476160913323da1dada49ae46e72d6a7c17d78
# Parent c1bd9b7750cdfe1f2a1437e4ba0ed8f6e48b2dd1
revlog: add 'raw' argument to revision and _addrevision
This patch introduces a new 'raw' argument (defaults to False) to revlog's
revision() and _addrevision() methods.
When the 'raw' argument is set to True, it indicates the revision data should be
handled as raw data by the flagprocessor.
That part seems fine. (that should probably mention that the argument do
not have effect yet, but that's fine).
This patch adds a new 'rawrevision()' method setting the 'raw' argument to True
in the revlog.revision() call that is used to differentiate changegroup
generation and debugdata related calls to revision() from regular read accesses.
I don't get that part. it seems like 'rawrevision(…)' is just
'revision(…, raw=True)' so I do not understant why we need a new method.
Can't we just call 'revision(…, raw=True)' directly. Am I missing
something? Otherwise, we should just drop that method.
Note: Given revlog.addgroup() calls are restricted to changegroup generation, we
can always set raw to True when calling revlog._addrevision() from
revlog.addgroup().
There is a couple of extra small comment that are not blocker, but worth
considering as if we do that extra round-trip.
Can I get you to enable the 'showfunc' feature so that your patch
includes a bit more context?
[diff]
showfunc=1
diff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py
--- a/mercurial/bundlerepo.py
+++ b/mercurial/bundlerepo.py
@@ -117,7 +117,7 @@
return mdiff.textdiff(self.revision(self.node(rev1)),
self.revision(self.node(rev2)))
- def revision(self, nodeorrev):
+ def revision(self, nodeorrev, raw=False):
"""return an uncompressed revision of a given node or revision
number.
"""
diff --git a/mercurial/changegroup.py b/mercurial/changegroup.py
--- a/mercurial/changegroup.py
+++ b/mercurial/changegroup.py
@@ -783,7 +783,7 @@
prefix = ''
if revlog.iscensored(base) or revlog.iscensored(rev):
try:
- delta = revlog.revision(node)
+ delta = revlog.rawrevision(node)
except error.CensoredNodeError as e:
delta = e.tombstone
if base == nullrev:
@@ -792,7 +792,7 @@
baselen = revlog.rawsize(base)
prefix = mdiff.replacediffheader(baselen, len(delta))
elif base == nullrev:
- delta = revlog.revision(node)
+ delta = revlog.rawrevision(node)
prefix = mdiff.trivialdiffheader(len(delta))
else:
delta = revlog.revdiff(base, rev)
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -1110,6 +1110,9 @@
return filectx(self._repo, self._path, fileid=fileid,
filelog=self._filelog, changeid=changeid)
+ def rawdata(self):
+ return self._filelog.rawrevision(self._filenode)
+
That new method is not mentioned in the description and not used
anywhere. What is it about?
def data(self):
try:
return self._filelog.read(self._filenode)
diff --git a/mercurial/debugcommands.py b/mercurial/debugcommands.py
--- a/mercurial/debugcommands.py
+++ b/mercurial/debugcommands.py
@@ -445,7 +445,7 @@
raise error.CommandError('debugdata', _('invalid arguments'))
r = cmdutil.openrevlog(repo, 'debugdata', file_, opts)
try:
- ui.write(r.revision(r.lookup(rev)))
+ ui.write(r.rawrevision(r.lookup(rev)))
except KeyError:
raise error.Abort(_('invalid revision identifier %s') % rev)
diff --git a/mercurial/revlog.py b/mercurial/revlog.py
--- a/mercurial/revlog.py
+++ b/mercurial/revlog.py
@@ -1202,12 +1202,18 @@
return mdiff.textdiff(self.revision(rev1),
self.revision(rev2))
- def revision(self, nodeorrev, _df=None):
+ def rawrevision(self, nodeorrev):
+ """cf. revision() for argument description."""
+ return self.revision(nodeorrev, raw=True)
+
+ def revision(self, nodeorrev, _df=None, raw=False):
"""return an uncompressed revision of a given node or revision
number.
- _df is an existing file handle to read from. It is meant to only be
- used internally.
+ _df - an existing file handle to read from. (internal-only)
+ raw - an optional argument specifying if the revision data is to be
+ treated as raw data when applying flag transforms. 'raw' should be set
+ to True when generating changegroups or in debug commands.
"""
if isinstance(nodeorrev, int):
rev = nodeorrev
@@ -1357,7 +1363,8 @@
ifh = self.opener(self.indexfile, "a+", checkambig=self._checkambig)
try:
return self._addrevision(node, text, transaction, link, p1, p2,
- REVIDX_DEFAULT_FLAGS, cachedelta, ifh,
dfh)
+ REVIDX_DEFAULT_FLAGS, cachedelta, ifh,
dfh,
+ raw=False)
As the default value is 'False' already, we should probably leave this
call untouched.
finally:
if dfh:
dfh.close()
@@ -1412,13 +1419,16 @@
return True
def _addrevision(self, node, text, transaction, link, p1, p2, flags,
- cachedelta, ifh, dfh, alwayscache=False):
+ cachedelta, ifh, dfh, alwayscache=False, raw=False):
"""internal function to add revisions to the log
see addrevision for argument descriptions.
invariants:
- text is optional (can be None); if not set, cachedelta must be set.
if both are set, they must correspond to each other.
+ - raw is optional; if set to True, it indicates the revision data is to
+ be treated by processflags() as raw. It is usually set by changegroup
+ generation and debug commands.
"""
btext = [text]
def buildtext():
@@ -1438,8 +1448,9 @@
fh = ifh
else:
fh = dfh
- basetext = self.revision(self.node(baserev), _df=fh)
+ basetext = self.revision(self.node(baserev), _df=fh, raw=raw)
btext[0] = mdiff.patch(basetext, delta)
+
Gratuitous unrelated new line ;-)
try:
self.checkhash(btext[0], node, p1=p1, p2=p2)
if flags & REVIDX_ISCENSORED:
@@ -1668,10 +1679,14 @@
# the added revision, which will require a call to
# revision(). revision() will fast path if there is a cache
# hit. So, we tell _addrevision() to always cache in this case.
+ # We're only using addgroup() in the context of changegroup
+ # generation so the revision data can always be handled as raw
+ # by the flagprocessor.
chain = self._addrevision(node, None, transaction, link,
p1, p2, flags, (baserev, delta),
ifh, dfh,
- alwayscache=bool(addrevisioncb))
+ alwayscache=bool(addrevisioncb),
+ raw=True)
if addrevisioncb:
addrevisioncb(self, chain)
diff --git a/mercurial/unionrepo.py b/mercurial/unionrepo.py
--- a/mercurial/unionrepo.py
+++ b/mercurial/unionrepo.py
@@ -93,7 +93,7 @@
return mdiff.textdiff(self.revision(self.node(rev1)),
self.revision(self.node(rev2)))
- def revision(self, nodeorrev):
+ def revision(self, nodeorrev, raw=False):
"""return an uncompressed revision of a given node or revision
number.
"""
Cheers,
--
Pierre-Yves David
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
