[PATCH 1 of 2 "V3] compression: introduce an official `zstd-revlog` requirement

[Pierre-Yves David]

On 4/16/19 12:28 PM, Gregory Szorc wrote:
> On Mon, Apr 15, 2019 at 8:34 AM Augie Fackler <raf at durin42.com 
> <mailto:raf at durin42.com>> wrote:
> 
>     Greg: ping on these? I'm aware you had some unease on this topic, and
>     I'd like to figure out if we should ship this. Thanks!
> 
> 
> I've been enjoying my holiday away from the computer. I met with 
> Pierre-Yves and Georges yesterday and agreed to review these before the 
> freeze...
> 
> 
>     On Tue, Apr 09, 2019 at 12:06:00PM +0200, Pierre-Yves David wrote:
>      > # HG changeset patch
>      > # User Pierre-Yves David <pierre-yves.david at octobus.net
>     <mailto:pierre-yves.david at octobus.net>>
>      > # Date 1553707623 -3600
>      > #      Wed Mar 27 18:27:03 2019 +0100
>      > # Node ID 8ce788576265eb8b84cb697ea2e09b2246fb7b81
>      > # Parent  456c37433c433ee59d321315da24eb944e495f08
>      > # EXP-Topic zstd-revlog
>      > # Available At https://bitbucket.org/octobus/mercurial-devel/
>      > #              hg pull
>     https://bitbucket.org/octobus/mercurial-devel/ -r 8ce788576265
>      > compression: introduce an official `zstd-revlog` requirement
>      >
>      > This requirement supersedes `exp-compression-zstd`. However, we
>     keep support for
>      > the old requirement.
>      >
>      > Strictly speaking, we do not need to add a new requirement, there
>     are no logic
>      > change making "new" repo incompatible with mercurial client using
>     a version
>      > that support `exp-compression-zstd`.
>      >
>      > The choice to introduce a new requirement is motivated by the
>     following:
>      >
>      > * The previous requirement was explicitly "experimental". Using
>     it by default
>      >   could confuse users.
>      >
>      > * adding support for a hypothetical third compression engine will
>     requires new
>      >   code, and will comes with its own requirement tool.
>      >
>      > * We won't use it as the default for a while since I do not think
>     we support
>      >   zstd on all platform. I can imagine we'll gain another
>     (unrelated but on my
>      >   default) requirement by the time we turn this zstd by default.
> 
> 
> So, the reason why I named things "compression-<format>" was so we could 
> have a per-compressor requirement that could potentially allow 
> *anything* to use that compressor. For example, "compression-zstd" could 
> allow random on-disk files to be compressed with zstd.
> 
> Of course, *any* time you make a BC change to how the repository is read 
> from the filesystem, you need a new requirement, otherwise old clients 
> may attempt to open the repo and get confused or cause corruption. So 
> even if we have an e.g. "compression-zstd" requirement, we would still 
> need additional requirements to denote when zstd support is added to 
> individual components. e.g. we'd need a dedicated requirement for "zstd 
> in revlogs."
> 
> I think I'm OK with the approach in this patch.
> 
>      >
>      > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>      > --- a/mercurial/localrepo.py
>      > +++ b/mercurial/localrepo.py
>      > @@ -645,6 +645,8 @@ def gathersupportedrequirements(ui):
>      >          engine = util.compengines[name]
>      >          if engine.revlogheader():
>      >              supported.add(b'exp-compression-%s' % name)
>      > +            if engine.name <http://engine.name>() == 'zstd':
>      > +                supported.add(b'revlog-compression-zstd')
> 
> 
> I'm still not a fan of special casing zstd: there should be something on 
> the compression engine API doing this. But I'm willing to look the other 
> way for now because doing it as part of the compression engine API means 
> stabilizing the "exp-compression-<format>" bit and I don't want to do 
> that just yet.
> 
> There is also an existing bug where we fail to check engine.available(). 
> This will result in Mercurial thinking it can open zstd repositories and 
> then failing when it tries to use zstd when it isn't available. 
> (Compression engines are always present/registered but they may not be 
> available at run-time due to missing dependencies.)
> 
> Could you please send a patch (either standalone or a v4 of this one) 
> that adds a check for engine.available() and skips engines that aren't 
> usable?

I am on it. It looks like only one line needs to be updated. I am 
sending a V4 with this extra patch + update to the last patch.

-- 
Pierre-Yves David