Patchwork [2,of,3] upgrade: move descriptions and selection logic in individual classes

login
register
mail settings
Submitter Pierre-Yves David
Date April 12, 2017, 11:12 p.m.
Message ID <346b07413137d7f56196.1492038735@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/20142/
State Superseded
Headers show

Comments

Pierre-Yves David - April 12, 2017, 11:12 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1492007645 -7200
#      Wed Apr 12 16:34:05 2017 +0200
# Node ID 346b07413137d7f56196941212958df03e55b202
# Parent  b42c1f35aedd153c77741d54dac50aa6d3ea43e2
# EXP-Topic upgraderepo
# Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
#              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 346b07413137
upgrade: move descriptions and selection logic in individual classes

Our goal here is to get top level definition for all the format variants. Having
them defined outside of the function enabled other users of that logic.

They are two keys components of a format variant:

1) the name and various descriptions of its effect,

2) the code that checks if the repo is using this variant and if the config
   enables it.

That second items make us pick a class-based approach, since different variants
requires different code (even if in practice, many can reuse the same logic).
Each variants define its own class that is then used like a singleton.  The
class-based approach also clarify the definitions part a bit since each are
simple assignment in an indented block.

The 'fromdefault' and 'fromconfig' are respectively replaced by a class
attribute and a method to be called at the one place where "fromconfig"
matters.

Overall, they are many viable approach for this, but this is the one I picked.
Yuya Nishihara - April 16, 2017, 1:18 p.m.
On Thu, 13 Apr 2017 01:12:15 +0200, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1492007645 -7200
> #      Wed Apr 12 16:34:05 2017 +0200
> # Node ID 346b07413137d7f56196941212958df03e55b202
> # Parent  b42c1f35aedd153c77741d54dac50aa6d3ea43e2
> # EXP-Topic upgraderepo
> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 346b07413137
> upgrade: move descriptions and selection logic in individual classes
> 
> Our goal here is to get top level definition for all the format variants. Having
> them defined outside of the function enabled other users of that logic.
> 
> They are two keys components of a format variant:
> 
> 1) the name and various descriptions of its effect,
> 
> 2) the code that checks if the repo is using this variant and if the config
>    enables it.
> 
> That second items make us pick a class-based approach, since different variants
> requires different code (even if in practice, many can reuse the same logic).
> Each variants define its own class that is then used like a singleton.  The
> class-based approach also clarify the definitions part a bit since each are
> simple assignment in an indented block.

Since I'm not a fan of using class as a syntactic sugar, I prefer a plain
function that creates an improvement object. But I have no strong option
about that.

> -class formatvariant(improvement):
> -    """an improvement subclass dedicated to repository format
> +class formatvariant(object):
> +    """an improvement subclass dedicated to repository format"""

Nit: it's no longer be a subclass of improvement.

> +    type = deficiency
> +    ### The following attributes should be defined for each class:
> +
> +    # machine-readable string uniquely identifying this improvement. it will be
> +    # mapped to an action later in the upgrade process.
> +    name = None

Confirmed that the formatvariant class doesn't need __eq__ because it won't be
instantiated.

> +class fncache(requirementformatvariant):
> +    name = 'fncache'
> +
> +    _requirement = 'fncache'
> +
> +    default = True
> +
> +    description = _('long and reserved filenames may not work correctly; '
> +                    'repository performance is sub-optimal')
> +
> +    upgrademessage=_('repository will be more resilient to storing '
> +                     'certain paths and performance of certain '
> +                     'operations should be improved')

Missing spaces around '='. The same formatting issues are seen in several
places.
Pierre-Yves David - April 16, 2017, 11:27 p.m.
On 04/16/2017 03:18 PM, Yuya Nishihara wrote:
> On Thu, 13 Apr 2017 01:12:15 +0200, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1492007645 -7200
>> #      Wed Apr 12 16:34:05 2017 +0200
>> # Node ID 346b07413137d7f56196941212958df03e55b202
>> # Parent  b42c1f35aedd153c77741d54dac50aa6d3ea43e2
>> # EXP-Topic upgraderepo
>> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 346b07413137
>> upgrade: move descriptions and selection logic in individual classes
>>
>> Our goal here is to get top level definition for all the format variants. Having
>> them defined outside of the function enabled other users of that logic.
>>
>> They are two keys components of a format variant:
>>
>> 1) the name and various descriptions of its effect,
>>
>> 2) the code that checks if the repo is using this variant and if the config
>>    enables it.
>>
>> That second items make us pick a class-based approach, since different variants
>> requires different code (even if in practice, many can reuse the same logic).
>> Each variants define its own class that is then used like a singleton.  The
>> class-based approach also clarify the definitions part a bit since each are
>> simple assignment in an indented block.
>
> Since I'm not a fan of using class as a syntactic sugar, I prefer a plain
> function that creates an improvement object. But I have no strong option
> about that.

The main motivation to use class is the distinct code (and sometimes 
attribute) for each variants, having a "builder" function would make the 
code handling of the code much more hacky.

I am not fully happy about the current state, but I do not see a clean 
alternative yet. Either I'm missing something in your suggestion or I 
find it hackier.


>> -class formatvariant(improvement):
>> -    """an improvement subclass dedicated to repository format
>> +class formatvariant(object):
>> +    """an improvement subclass dedicated to repository format"""
>
> Nit: it's no longer be a subclass of improvement.

Gah, bad merge resolution.

I can send a V2 or a follow up as you prefer. (given how near the freeze 
is, a follow up would seems better)

>
>> +    type = deficiency
>> +    ### The following attributes should be defined for each class:
>> +
>> +    # machine-readable string uniquely identifying this improvement. it will be
>> +    # mapped to an action later in the upgrade process.
>> +    name = None
>
> Confirmed that the formatvariant class doesn't need __eq__ because it won't be
> instantiated.

Yep. It does not needs one.

>> +class fncache(requirementformatvariant):
>> +    name = 'fncache'
>> +
>> +    _requirement = 'fncache'
>> +
>> +    default = True
>> +
>> +    description = _('long and reserved filenames may not work correctly; '
>> +                    'repository performance is sub-optimal')
>> +
>> +    upgrademessage=_('repository will be more resilient to storing '
>> +                     'certain paths and performance of certain '
>> +                     'operations should be improved')
>
> Missing spaces around '='. The same formatting issues are seen in several
> places.

Good cache, I can send a V2 or a follow up as you prefer.

Cheers,
Pierre-Yves David - April 17, 2017, 11:40 a.m.
On 04/17/2017 01:27 AM, Pierre-Yves David wrote:
>
>
> On 04/16/2017 03:18 PM, Yuya Nishihara wrote:
>> On Thu, 13 Apr 2017 01:12:15 +0200, Pierre-Yves David wrote:
 >>> […]
>>> -class formatvariant(improvement):
>>> -    """an improvement subclass dedicated to repository format
>>> +class formatvariant(object):
>>> +    """an improvement subclass dedicated to repository format"""
>>
>> Nit: it's no longer be a subclass of improvement.
>
> Gah, bad merge resolution.
>
> I can send a V2 or a follow up as you prefer. (given how near the freeze
> is, a follow up would seems better)

I've a V2 ready, so I'm just going to send it.

Patch

diff --git a/mercurial/upgrade.py b/mercurial/upgrade.py
--- a/mercurial/upgrade.py
+++ b/mercurial/upgrade.py
@@ -133,94 +133,151 @@  class improvement(object):
             return NotImplemented
         return self.name == other.name
 
-class formatvariant(improvement):
-    """an improvement subclass dedicated to repository format
+class formatvariant(object):
+    """an improvement subclass dedicated to repository format"""
+    type = deficiency
+    ### The following attributes should be defined for each class:
+
+    # machine-readable string uniquely identifying this improvement. it will be
+    # mapped to an action later in the upgrade process.
+    name = None
 
-    extra attributes:
+    # message intended for humans explaining the improvement in more detail,
+    # including the implications of it ``deficiency`` types, should be worded
+    # in the present tense.
+    description = None
+
+    # message intended for humans explaining what an upgrade addressing this
+    # issue will do. should be worded in the future tense.
+    upgrademessage = None
 
-    fromdefault (``deficiency`` types only)
-       Boolean indicating whether the current (deficient) state deviates
-       from Mercurial's default configuration.
+    # value of current Mercurial default for new repository
+    default = None
+
+    @staticmethod
+    def fromrepo(repo):
+        """current value of the variant in the repository"""
+        raise NotImplementedError()
 
-    fromconfig (``deficiency`` types only)
-       Boolean indicating whether the current (deficient) state deviates
-       from the current Mercurial configuration.
+    @staticmethod
+    def fromconfig(repo):
+        """current value of the variant in the configuration"""
+        raise NotImplementedError()
+
+class requirementformatvariant(formatvariant):
+    """formatvariant based on a 'requirement' name.
+
+    Many format variant are controlled by a 'requirement'. We define a small
+    subclass to factor the code.
     """
 
-    def __init__(self, name, description, upgrademessage, fromdefault,
-                 fromconfig):
-        super(formatvariant, self).__init__(name, 'deficiency', description,
-                                            upgrademessage)
-        self.fromdefault = fromdefault
-        self.fromconfig = fromconfig
+    # the requirement that control this format variant
+    _requirement = None
+
+    @staticmethod
+    def _newreporequirements(repo):
+        return localrepo.newreporequirements(repo)
+
+    @classmethod
+    def fromrepo(cls, repo):
+        assert cls._requirement is not None
+        return cls._requirement in repo.requirements
+
+    @classmethod
+    def fromconfig(cls, repo):
+        assert cls._requirement is not None
+        return cls._requirement in cls._newreporequirements(repo)
+
+class fncache(requirementformatvariant):
+    name = 'fncache'
+
+    _requirement = 'fncache'
+
+    default = True
+
+    description = _('long and reserved filenames may not work correctly; '
+                    'repository performance is sub-optimal')
+
+    upgrademessage=_('repository will be more resilient to storing '
+                     'certain paths and performance of certain '
+                     'operations should be improved')
+
+class dotencode(requirementformatvariant):
+    name = 'dotencode'
+
+    _requirement = 'dotencode'
+
+    default = True
+
+    description=_('storage of filenames beginning with a period or '
+                  'space may not work correctly')
+
+    upgrademessage=_('repository will be better able to store files '
+                     'beginning with a space or period')
+
+class generaldelta(requirementformatvariant):
+    name='generaldelta'
+
+    _requirement = 'generaldelta'
+
+    default = True
+
+    description=_('deltas within internal storage are unable to '
+                  'choose optimal revisions; repository is larger and '
+                  'slower than it could be; interaction with other '
+                  'repositories may require extra network and CPU '
+                  'resources, making "hg push" and "hg pull" slower')
+
+    upgrademessage=_('repository storage will be able to create '
+                     'optimal deltas; new repository data will be '
+                     'smaller and read times should decrease; '
+                     'interacting with other repositories using this '
+                     'storage model should require less network and '
+                     'CPU resources, making "hg push" and "hg pull" '
+                     'faster')
+
+class removecldeltachain(formatvariant):
+    name='removecldeltachain'
+
+    default = True
+
+    description=_('changelog storage is using deltas instead of '
+                  'raw entries; changelog reading and any '
+                  'operation relying on changelog data are slower '
+                  'than they could be')
+
+    upgrademessage=_('changelog storage will be reformated to '
+                     'store raw entries; changelog reading will be '
+                     'faster; changelog size may be reduced')
+
+    @staticmethod
+    def fromrepo(repo):
+        # Mercurial 4.0 changed changelogs to not use delta chains. Search for
+        # changelogs with deltas.
+        cl = repo.changelog
+        chainbase = cl.chainbase
+        return all(rev == chainbase(rev) for rev in cl)
+
+    @staticmethod
+    def fromconfig(repo):
+        return True
 
 def finddeficiencies(repo):
     """returns a list of deficiencies that the repo suffer from"""
-    newreporeqs = localrepo.newreporequirements(repo)
-
     deficiencies = []
 
     # We could detect lack of revlogv1 and store here, but they were added
     # in 0.9.2 and we don't support upgrading repos without these
     # requirements, so let's not bother.
 
-    if 'fncache' not in repo.requirements:
-        deficiencies.append(formatvariant(
-            name='fncache',
-            description=_('long and reserved filenames may not work correctly; '
-                          'repository performance is sub-optimal'),
-            upgrademessage=_('repository will be more resilient to storing '
-                             'certain paths and performance of certain '
-                             'operations should be improved'),
-            fromdefault=True,
-            fromconfig='fncache' in newreporeqs))
-
-    if 'dotencode' not in repo.requirements:
-        deficiencies.append(formatvariant(
-            name='dotencode',
-            description=_('storage of filenames beginning with a period or '
-                          'space may not work correctly'),
-            upgrademessage=_('repository will be better able to store files '
-                             'beginning with a space or period'),
-            fromdefault=True,
-            fromconfig='dotencode' in newreporeqs))
-
-    if 'generaldelta' not in repo.requirements:
-        deficiencies.append(formatvariant(
-            name='generaldelta',
-            description=_('deltas within internal storage are unable to '
-                          'choose optimal revisions; repository is larger and '
-                          'slower than it could be; interaction with other '
-                          'repositories may require extra network and CPU '
-                          'resources, making "hg push" and "hg pull" slower'),
-            upgrademessage=_('repository storage will be able to create '
-                             'optimal deltas; new repository data will be '
-                             'smaller and read times should decrease; '
-                             'interacting with other repositories using this '
-                             'storage model should require less network and '
-                             'CPU resources, making "hg push" and "hg pull" '
-                             'faster'),
-            fromdefault=True,
-            fromconfig='generaldelta' in newreporeqs))
-
-    # Mercurial 4.0 changed changelogs to not use delta chains. Search for
-    # changelogs with deltas.
-    cl = repo.changelog
-    for rev in cl:
-        chainbase = cl.chainbase(rev)
-        if chainbase != rev:
-            deficiencies.append(formatvariant(
-                name='removecldeltachain',
-                description=_('changelog storage is using deltas instead of '
-                              'raw entries; changelog reading and any '
-                              'operation relying on changelog data are slower '
-                              'than they could be'),
-                upgrademessage=_('changelog storage will be reformated to '
-                                 'store raw entries; changelog reading will be '
-                                 'faster; changelog size may be reduced'),
-                fromdefault=True,
-                fromconfig=True))
-            break
+    if not fncache.fromrepo(repo):
+        deficiencies.append(fncache)
+    if not dotencode.fromrepo(repo):
+        deficiencies.append(dotencode)
+    if not generaldelta.fromrepo(repo):
+        deficiencies.append(generaldelta)
+    if not removecldeltachain.fromrepo(repo):
+        deficiencies.append(removecldeltachain)
 
     return deficiencies
 
@@ -674,9 +731,9 @@  def upgraderepo(ui, repo, run=False, opt
         onlydefault = []
 
         for d in deficiencies:
-            if d.fromconfig:
+            if d.fromconfig(repo):
                 fromconfig.append(d)
-            elif d.fromdefault:
+            elif d.default:
                 onlydefault.append(d)
 
         if fromconfig or onlydefault: