[PATCH 2 of 2 zstd-revlogs V2] localrepo: experimental support for non-zlib revlog compression

Gregory Szorc gregory.szorc at gmail.com
Sun Jan 15 17:57:59 UTC 2017


On Sun, Jan 15, 2017 at 2:06 AM, Pierre-Yves David <
pierre-yves.david at ens-lyon.org> wrote:

>
>
> On 01/15/2017 10:33 AM, Yuya Nishihara wrote:
>
>> On Fri, 13 Jan 2017 20:45:57 -0800, Gregory Szorc wrote:
>>
>>> # HG changeset patch
>>> # User Gregory Szorc <gregory.szorc at gmail.com>
>>> # Date 1484367416 28800
>>> #      Fri Jan 13 20:16:56 2017 -0800
>>> # Node ID 01dce0ba83c49989c2e75bbda111f2816b1eb61e
>>> # Parent  081a7a0c0d5665056476ed35875d663ea5eaac73
>>> localrepo: experimental support for non-zlib revlog compression
>>>
>>
>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>>> --- a/mercurial/localrepo.py
>>> +++ b/mercurial/localrepo.py
>>> @@ -284,6 +284,12 @@ class localrepository(object):
>>>          else:
>>>              self.supported = self._basesupported
>>>
>>> +        # Add compression engines.
>>> +        for name in util.compengines:
>>> +            engine = util.compengines[name]
>>> +            if engine.revlogheader:
>>>
>>
>> Perhaps this should be "if engine.revlogheader()". Maybe it can be fixed
>> in flight?
>>
>
> Good catch, I've fixed the version on the hg-committed repo.
>
> My python Programmer sense are wondering why this isn't an
> attribute/property. This seems like a "static" property of the object
> (actually even of the class for most of them) and would seems more natural
> to me written "engine.revlogheader" in Python. In addition, given this
> value is boolean, it would help prevent issue as the one yuya just caught.
> What do you think?
>

I agree it feels like an attribute/property and not a method. But there are
a few reasons I chose to use methods:

a) consistency (everything is a method)
b) easier to document the API (see detailed docstrings in compressionengine
class)
c) makes it easier to implement complex logic (I don't like @property
because at the point you are executing a function, you might as well have
the caller specify "()" as a signal that "this isn't a simple lookup").
(See the common performance badness that localrepository.changelog has
caused over the years.)

Given we now have 4 methods defining engine properties and only use them
during engine registration, I'm tempted to refactor the whole mechanism
into a "capabilities()" method that returns a dict. That feels a bit more
extensible and future proof. I could do this today if wanted.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170115/b4985eb0/attachment-0002.html>


More information about the Mercurial-devel mailing list