On 12/17/2016 09:09 AM, Pierre-Yves David wrote:
On 12/14/2016 12:58 PM, Remi Chaintron wrote:# HG changeset patch # User Remi Chaintron <r...@fb.com> # Date 1481716603 0 # Wed Dec 14 11:56:43 2016 +0000 # Branch stable # Node ID 7599256f2800bbacc3b478ac8d2c4a5746eb4cd1 # Parent d34c04da52a8f2eb61b51bb0aa9cedf1e4b43474 revlog: add ability to apply transforms on flags Add a mechanism for extensions/subclasses of revlog to register transforms on read/write operations on revlog, based on revision flags. Due to some operations being non-commutative, transforms applied in a specific order when writing the contents of the revlog need to be applied on the reverse order when reading. In order to allow the composition of such operation, the 'processflags' method applies transforms on flags in the order defined by REVIDX_FLAGS_ORDER. Extensions can use the `localrepository.registerflagtransform()` method to register default transforms on flags in `reposetup()` in order to ensure their transform will not override/be overridden by other extensions' transforms.As a general principle, we don't add new methods on localrepo. Its tempting to add stuff to that central object but this lead to horrible horrible class bloat in the past. In addition, your method does not seems to make any use of the repo itself. (if we did not wrote that down somewhere, we should) Instead, you should go for one of these two options: - a small function defined next to the dictionary - our shiny and nice "registrar" mechanism. See https://www.mercurial-scm.org/repo/hg/file/tip/hgext/rebase.py#l109 for an example. I would probably pick registrar because the code exists, but given the low amount of registrees it might be overkill. Couple of feedback inlineddiff --git a/mercurial/bundlerepo.py b/mercurial/bundlerepo.py --- a/mercurial/bundlerepo.py +++ b/mercurial/bundlerepo.py @@ -147,7 +147,9 @@ delta = self._chunk(chain.pop()) text = mdiff.patches(text, [delta]) - self.checkhash(text, node, rev=rev) + text, validatehash = self.processflags(text, self.flags(rev)) + if validatehash is True:Should be: if validatehash:+ self.checkhash(text, node, rev=rev) self._cache = (node, rev, text) return text diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py --- a/mercurial/localrepo.py +++ b/mercurial/localrepo.py @@ -48,6 +48,7 @@ phases, pushkey, repoview, + revlog, revset, scmutil, store, @@ -1959,6 +1960,21 @@ fp.close() return self.pathto(fp.name[len(self.root) + 1:]) + def registerflagtransform(self, flag, transform): + """Register transform on revlog flags. + + Extensions can rely on this method to set default transforms on revlog + flags, and ensure they are not overwritten by/overwriting other + extensions'. + """ + if not flag & revlog.REVIDX_KNOWN_FLAGS: + raise error.Abort(_("Cannot register transform on unknown flag."))note that we neither capitalize nor punctuate our error messages.+ transform = revlog.REVIDX_FLAGS_TRANSFORMS.get(flag, None) + if transform is not None: + raise error.Abort( + _("Cannot register multiple transforms on flags."))ditto+ revlog.REVIDX_FLAGS_TRANSFORMS[flag] = transform + # used to avoid circular references so destructors work def aftertrans(files): renamefiles = [tuple(t) for t in files] diff --git a/mercurial/revlog.py b/mercurial/revlog.py --- a/mercurial/revlog.py +++ b/mercurial/revlog.py @@ -56,6 +56,12 @@ REVIDX_ISCENSORED = (1 << 15) # revision has censor metadata, must be verified REVIDX_DEFAULT_FLAGS = 0 REVIDX_KNOWN_FLAGS = REVIDX_ISCENSORED +# stable order in which flags need to be processed and their transforms applied +REVIDX_FLAGS_ORDER = [ + REVIDX_ISCENSORED, +] +REVIDX_FLAGS_TRANSFORMS = { }
I've just tough of something else, those are not constants, because they are designed for other extension to be able to extend them (especially the transform bits). Can you move them to our standard (lowercase, no '_')
(also, our constant are usually lowercase anyway, but this file is inconsistency for the rest of the code base)
Small nits: as we have no "public" accessors of this, we could add a '_' in front of it to discourage people to access it directly.+ # max size of revlog with inline data _maxinline = 131072 @@ -1203,11 +1209,6 @@ if rev is None: rev = self.rev(node) - # check rev flags - if self.flags(rev) & ~REVIDX_KNOWN_FLAGS: - raise RevlogError(_('incompatible revision flag %x') % - (self.flags(rev) & ~REVIDX_KNOWN_FLAGS)) - chain, stopped = self._deltachain(rev, stoprev=cachedrev) if stopped: text = self._cache[2] @@ -1221,7 +1222,11 @@ bins = bins[1:] text = mdiff.patches(text, bins) - self.checkhash(text, node, rev=rev) + + text, validatehash = self.processflags(text, self.flags(rev)) + if validatehash: + self.checkhash(text, node, rev=rev) + self._cache = (node, rev, text) return text @@ -1233,6 +1238,45 @@ """ return hash(text, p1, p2) + def processflags(self, text, flags, reverse=False): + """Process revision flags and apply registered transforms. + + ``text`` - the revision data to process + ``flags`` - the revision flags + ``reverse`` - an option argument describing whether the flags should be + processed in order or reverse order. + + This method processes the flags in the order (or reverse order if + ``reverse`` is False) defined by REVIDX_FLAGS_TRANSFORMS, applying the + transforms registered for present flags. The order of flags defined in + REVIDX_FLAGS_TRANSFORMS needs to be stable for non-commutative + operations. + + Returns a 2-tuple of ``(text, validatehash)`` where ``text`` is the + processed text and ``validatehash`` is a bool indicating whether the + returned text should be checked for hash integrity. + """ + # Check all flags are known. + if flags & ~REVIDX_KNOWN_FLAGS: + raise RevlogError(_('incompatible revision flag %x') % + (flags & ~REVIDX_KNOWN_FLAGS)) + + validatehash = True + # Depending on the operation (read or write), the order might be + # reversed due to non-commutative transforms. + orderedflags = REVIDX_FLAGS_ORDER + if reverse: + orderedflags = reversed(orderedflags) + + for flag in orderedflags: + # If a transform has been registered for a known flag, apply and + # update result tuple. + if flag & flags: + f = REVIDX_FLAGS_TRANSFORMS.get(flag, None) + if f is not None: + text, validatehash = f(self, text) + return text, validatehash + def checkhash(self, text, node, p1=None, p2=None, rev=None): """Check node hash integrity. @@ -1415,7 +1459,10 @@ basetext = self.revision(self.node(baserev), _df=fh) btext[0] = mdiff.patch(basetext, delta) try: - self.checkhash(btext[0], node, p1=p1, p2=p2) + btext[0], validatehash = self.processflags(btext[0], flags, + reverse=True)Should be: if validatehash:+ if validatehash is True: + self.checkhash(btext[0], node, p1=p1, p2=p2) if flags & REVIDX_ISCENSORED: raise RevlogError(_('node %s is not censored') % node) except CensoredNodeError:Cheers,
-- Pierre-Yves David _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
