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