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

[Pierre-Yves David]

On 01/15/2017 06:57 PM, Gregory Szorc wrote:
> On Sun, Jan 15, 2017 at 2:06 AM, Pierre-Yves David
> <pierre-yves.david at ens-lyon.org <mailto: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
>             <mailto: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.

This seems like a fine option to avoid the trap we fell into this time.
I would be happy to take such patch.

Cheers,

-- 
Pierre-Yves David