Patchwork [7,of,8,"] compression: introduce an official `zstd-revlog` requirement

login
register
mail settings
Submitter Pierre-Yves David
Date March 31, 2019, 3:36 p.m.
Message ID <2cfe9983fa92313d58f0.1554046583@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/39427/
State Accepted
Headers show

Comments

Pierre-Yves David - March 31, 2019, 3:36 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@octobus.net>
# Date 1553707623 -3600
#      Wed Mar 27 18:27:03 2019 +0100
# Node ID 2cfe9983fa92313d58f0420ec62f2341a810343e
# Parent  108e26fa0a97fe5342a1ce246cc4e4c185803454
# EXP-Topic zstd-revlog
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 2cfe9983fa92
compression: introduce an official `zstd-revlog` requirement

This requirement supersede `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.
Josef 'Jeff' Sipek - April 2, 2019, 7:52 a.m.
On Sun, Mar 31, 2019 at 17:36:23 +0200, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> # Date 1553707623 -3600
> #      Wed Mar 27 18:27:03 2019 +0100
> # Node ID 2cfe9983fa92313d58f0420ec62f2341a810343e
> # Parent  108e26fa0a97fe5342a1ce246cc4e4c185803454
> # EXP-Topic zstd-revlog
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 2cfe9983fa92
> compression: introduce an official `zstd-revlog` requirement

Is the requirement for the compression algo or for the compression algo's
use in revlog?

If the former, something like 'compression-<algo>' makes more sense.

If the later, would it be better to call it 'revlog-compression-<algo>' or
something to that effect?

Either way, while a *human* knows that zstd is a compression algo, could it
make sense to make it easily parsable?  I'm imagining a slightly better
error messages when requirements fail, or just the ability to
programmatically identify the algo.  For example, instead of the current:

  abort: repository requires features unknown to this Mercurial: foobar-revlog!

hg could emit:

  abort: repository requires a compression algo unknown to this Mercurial: foobar!

> 
> This requirement supersede `exp-compression-zstd`. However, we keep support for

s/supersede/supersedes/ :)

Jeff.
Pierre-Yves David - April 2, 2019, 1:57 p.m.
On 4/2/19 9:52 AM, Josef 'Jeff' Sipek wrote:
> On Sun, Mar 31, 2019 at 17:36:23 +0200, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@octobus.net>
>> # Date 1553707623 -3600
>> #      Wed Mar 27 18:27:03 2019 +0100
>> # Node ID 2cfe9983fa92313d58f0420ec62f2341a810343e
>> # Parent  108e26fa0a97fe5342a1ce246cc4e4c185803454
>> # EXP-Topic zstd-revlog
>> # Available At https://bitbucket.org/octobus/mercurial-devel/
>> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 2cfe9983fa92
>> compression: introduce an official `zstd-revlog` requirement
> 
> Is the requirement for the compression algo or for the compression algo's
> use in revlog?

The use of zstd in revlog

> 
> If the former, something like 'compression-<algo>' makes more sense.
> 
> If the later, would it be better to call it 'revlog-compression-<algo>' or
> something to that effect?



> Either way, while a *human* knows that zstd is a compression algo, could it
> make sense to make it easily parsable?  I'm imagining a slightly better
> error messages when requirements fail, or just the ability to
> programmatically identify the algo.  For example, instead of the current:
> 
>    abort: repository requires features unknown to this Mercurial: foobar-revlog!
> 
> hg could emit:
> 
>    abort: repository requires a compression algo unknown to this Mercurial: foobar!

I'm that longer version has much value. Most of our requirement has name 
opaque to normal user. This is why we link to an explanatory pages.

Once this series is in, I am planning to do some UI polish. Especially, 
for version that know this requirement but have been compiled without 
zstd support, we can issue a better message.

> 
>>
>> This requirement supersede `exp-compression-zstd`. However, we keep support for
> 
> s/supersede/supersedes/ :)
> 
> Jeff.
>
Gregory Szorc - April 2, 2019, 6:28 p.m.
On Tue, Apr 2, 2019 at 6:57 AM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 4/2/19 9:52 AM, Josef 'Jeff' Sipek wrote:
> > On Sun, Mar 31, 2019 at 17:36:23 +0200, Pierre-Yves David wrote:
> >> # HG changeset patch
> >> # User Pierre-Yves David <pierre-yves.david@octobus.net>
> >> # Date 1553707623 -3600
> >> #      Wed Mar 27 18:27:03 2019 +0100
> >> # Node ID 2cfe9983fa92313d58f0420ec62f2341a810343e
> >> # Parent  108e26fa0a97fe5342a1ce246cc4e4c185803454
> >> # EXP-Topic zstd-revlog
> >> # Available At https://bitbucket.org/octobus/mercurial-devel/
> >> #              hg pull https://bitbucket.org/octobus/mercurial-devel/
> -r 2cfe9983fa92
> >> compression: introduce an official `zstd-revlog` requirement
> >
> > Is the requirement for the compression algo or for the compression algo's
> > use in revlog?
>
> The use of zstd in revlog
>
> >
> > If the former, something like 'compression-<algo>' makes more sense.
> >
> > If the later, would it be better to call it 'revlog-compression-<algo>'
> or
> > something to that effect?
>

I agree with Jeff that the requirement name should be derived from the
compression engine name. e.g. the zstd compression engine supporting
revlogs will result in the "revlog-compression-zstd" requirement. That, or
the compression engine itself advertises an attribute denoting the
requirement for revlogs with that compression format. I think I like the
latter better, as it means no special casing of specific string values in
the requirements code - just special casing for `None`.

Could you please try something along these lines? For clarity, the thing
that is bothering me is the leaky implementation where the low-level
requirements code has to know about zlib and zstd. All of this should be
generic and derived from the registered compression engines: that's what
the compression engine abstraction is for.


>
> > Either way, while a *human* knows that zstd is a compression algo, could
> it
> > make sense to make it easily parsable?  I'm imagining a slightly better
> > error messages when requirements fail, or just the ability to
> > programmatically identify the algo.  For example, instead of the current:
> >
> >    abort: repository requires features unknown to this Mercurial:
> foobar-revlog!
> >
> > hg could emit:
> >
> >    abort: repository requires a compression algo unknown to this
> Mercurial: foobar!
>
> I'm that longer version has much value. Most of our requirement has name
> opaque to normal user. This is why we link to an explanatory pages.
>
> Once this series is in, I am planning to do some UI polish. Especially,
> for version that know this requirement but have been compiled without
> zstd support, we can issue a better message.
>
> >
> >>
> >> This requirement supersede `exp-compression-zstd`. However, we keep
> support for
> >
> > s/supersede/supersedes/ :)
> >
> > Jeff.
> >
>
> --
> Pierre-Yves David
>
Pierre-Yves David - April 9, 2019, 10:06 a.m.
On 4/2/19 8:28 PM, Gregory Szorc wrote:
> On Tue, Apr 2, 2019 at 6:57 AM Pierre-Yves David 
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>> 
> wrote:
> 
> 
> 
>     On 4/2/19 9:52 AM, Josef 'Jeff' Sipek wrote:
>      > On Sun, Mar 31, 2019 at 17:36:23 +0200, Pierre-Yves David wrote:
>      >> # HG changeset patch
>      >> # User Pierre-Yves David <pierre-yves.david@octobus.net
>     <mailto:pierre-yves.david@octobus.net>>
>      >> # Date 1553707623 -3600
>      >> #      Wed Mar 27 18:27:03 2019 +0100
>      >> # Node ID 2cfe9983fa92313d58f0420ec62f2341a810343e
>      >> # Parent  108e26fa0a97fe5342a1ce246cc4e4c185803454
>      >> # EXP-Topic zstd-revlog
>      >> # Available At https://bitbucket.org/octobus/mercurial-devel/
>      >> #              hg pull
>     https://bitbucket.org/octobus/mercurial-devel/ -r 2cfe9983fa92
>      >> compression: introduce an official `zstd-revlog` requirement
>      >
>      > Is the requirement for the compression algo or for the
>     compression algo's
>      > use in revlog?
> 
>     The use of zstd in revlog
> 
>      >
>      > If the former, something like 'compression-<algo>' makes more sense.
>      >
>      > If the later, would it be better to call it
>     'revlog-compression-<algo>' or
>      > something to that effect?
> 
> I agree with Jeff that the requirement name should be derived from the 
> compression engine name. e.g. the zstd compression engine supporting 
> revlogs will result in the "revlog-compression-zstd" requirement. That, 
> or the compression engine itself advertises an attribute denoting the 
> requirement for revlogs with that compression format. I think I like the 
> latter better, as it means no special casing of specific string values 
> in the requirements code - just special casing for `None`.
> 
> Could you please try something along these lines? For clarity, the thing 
> that is bothering me is the leaky implementation where the low-level 
> requirements code has to know about zlib and zstd. All of this should be 
> generic and derived from the registered compression engines: that's what 
> the compression engine abstraction is for.

My main concerns here is to avoid accidental promotion of some odd 
compression engine. So I don't want to automatically generates 
revlog-compression-XXX for engine that supports revlog compression.

The requirement related attributes for compression engine seems like a 
good idea. However we needs it to be a list since zstd requires two 
value. Are you fine with that ?
An alternative approach would be to keep the automatic generation with 
some extra argument to convey the fact a compression engine as official 
support.

I am sending a V3 with:
* the updated requirements names
* more generic approach to requirement (but still some hard coding)
* other feedback integrated

I am happy to send follow up removing the hard coding once we converge 
on an approach about that.

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() == 'zstd':
+                supported.add(b'zstd-revlog')
 
     return supported
 
@@ -794,7 +796,13 @@  def resolverevlogstorevfsoptions(ui, req
         options[b'maxchainlen'] = maxchainlen
 
     for r in requirements:
-        if r.startswith(b'exp-compression-'):
+        # we allow multiple compression engine requirement to co-exist because
+        # strickly speaking, revlog seems to support mixed compression style.
+        #
+        # The compression used for new entries will be "the last one"
+        if r == 'zstd-revlog':
+            options[b'compengine'] = 'zstd'
+        elif r.startswith(b'exp-compression-'):
             options[b'compengine'] = r[len(b'exp-compression-'):]
 
     options[b'zlib.level'] = ui.configint(b'storage', b'revlog.zlib.level')
@@ -2929,7 +2937,9 @@  def newreporequirements(ui, createopts):
                                  'compression engines'))
 
     # zlib is the historical default and doesn't need an explicit requirement.
-    if compengine != 'zlib':
+    elif compengine == 'zstd':
+        requirements.add('zstd-revlog')
+    elif compengine != 'zlib':
         requirements.add('exp-compression-%s' % compengine)
 
     if scmutil.gdinitconfig(ui):
diff --git a/mercurial/upgrade.py b/mercurial/upgrade.py
--- a/mercurial/upgrade.py
+++ b/mercurial/upgrade.py
@@ -326,6 +326,8 @@  class compressionengine(formatvariant):
     @classmethod
     def fromrepo(cls, repo):
         for req in repo.requirements:
+            if req.startswith('zstd-revlog'):
+                return 'zstd'
             if req.startswith('exp-compression-'):
                 return req.split('-', 2)[2]
         return 'zlib'
diff --git a/tests/test-repo-compengines.t b/tests/test-repo-compengines.t
--- a/tests/test-repo-compengines.t
+++ b/tests/test-repo-compengines.t
@@ -44,12 +44,12 @@  A requirement specifying an unknown comp
   $ cd zstd
   $ cat .hg/requires
   dotencode
-  exp-compression-zstd
   fncache
   generaldelta
   revlogv1
   sparserevlog
   store
+  zstd-revlog
   testonly-simplestore (reposimplestore !)
 
   $ touch foo