This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10768

upgrade: Use `improvement` subclasses everywhere, not instances
ClosedPublic

Authored by SimonSapin on May 25 2021, 2:11 AM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers
Commits
rHGe985a36c2aa3: upgrade: Use `improvement` subclasses everywhere, not instances
rHGb3e2ae9e6d42: upgrade: Use `improvement` subclasses everywhere, not instances
Summary

This changes the source definition of optimizations to match that of formats:
a subclass with a decorator, instead of an instance passed to a function call.
Not having any instance removes the confusion between class attributes and
instance attributes, which were used interchangeably.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

SimonSapin created this revision.May 25 2021, 2:11 AM
marmoute requested changes to this revision.May 25 2021, 4:35 AM
marmoute added a subscriber: marmoute.
marmoute added inline comments.
mercurial/upgrade_utils/actions.py
49–90

Why are dropping these comment? it seems useful.

This revision now requires changes to proceed.May 25 2021, 4:35 AM
SimonSapin added inline comments.May 26 2021, 5:14 AM
mercurial/upgrade_utils/actions.py
49–90

This docstring used to document instance attributes, but instances attributes aren’t used anymore. Class attributes already have individual comments in the base class definition with mostly the same content.

I had missed that touches_* class attributes were not documented individually, I’ll restore that (even though these comments are not doing much besides paraphrasing what the attribute names are already saying).

marmoute added inline comments.May 26 2021, 5:15 AM
mercurial/upgrade_utils/actions.py
49–90

okay, thanks for the explanation.

SimonSapin updated this revision to Diff 28204.May 26 2021, 5:16 AM
SimonSapin added a commit: rHGb3e2ae9e6d42: upgrade: Use `improvement` subclasses everywhere, not instances.May 27 2021, 2:59 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHGb3e2ae9e6d42: upgrade: Use `improvement` subclasses everywhere, not instances (authored by SimonSapin). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/upgrade_utils/actions.py (277 lines)

Diff 28215

mercurial/upgrade_utils/actions.py

return set() return set()
FORMAT_VARIANT = b'deficiency' FORMAT_VARIANT = b'deficiency'
OPTIMISATION = b'optimization' OPTIMISATION = b'optimization'
class improvement(object): class improvement(object):
"""Represents an improvement that can be made as part of an upgrade. """Represents an improvement that can be made as part of an upgrade."""
The following attributes are defined on each instance: ### The following attributes should be defined for each subclass:
name # Either ``FORMAT_VARIANT`` or ``OPTIMISATION``.
Machine-readable string uniquely identifying this improvement. It # A format variant is where we change the storage format. Not all format
will be mapped to an action later in the upgrade process. # variant changes are an obvious problem.
# An optimization is an action (sometimes optional) that
type # can be taken to further improve the state of the repository.
Either ``FORMAT_VARIANT`` or ``OPTIMISATION``. type = None
A format variant is where we change the storage format. Not all format
variant changes are an obvious problem.
An optimization is an action (sometimes optional) that
can be taken to further improve the state of the repository.
description
Message intended for humans explaining the improvement in more detail,
including the implications of it. For ``FORMAT_VARIANT`` types, should be
worded in the present tense. For ``OPTIMISATION`` types, should be
worded in the future tense.
upgrademessage
Message intended for humans explaining what an upgrade addressing this
issue will do. Should be worded in the future tense.
postupgrademessage
Message intended for humans which will be shown post an upgrade
operation when the improvement will be added
postdowngrademessage
Message intended for humans which will be shown post an upgrade
operation in which this improvement was removed
touches_filelogs (bool)
Whether this improvement touches filelogs
touches_manifests (bool)
Whether this improvement touches manifests
touches_changelog (bool)
Whether this improvement touches changelog
touches_requirements (bool)
Whether this improvement changes repository requirements
marmouteUnsubmitted
Not Done

Why are dropping these comment? it seems useful.

marmoute: Why are dropping these comment? it seems useful.
SimonSapinAuthorUnsubmitted
Done

This docstring used to document instance attributes, but instances attributes aren’t used anymore. Class attributes already have individual comments in the base class definition with mostly the same content.

I had missed that touches_* class attributes were not documented individually, I’ll restore that (even though these comments are not doing much besides paraphrasing what the attribute names are already saying).

SimonSapin: This docstring used to document instance attributes, but instances attributes aren’t used…
marmouteUnsubmitted
Not Done

okay, thanks for the explanation.

marmoute: okay, thanks for the explanation.
"""
def __init__(self, name, type, description, upgrademessage):
self.name = name
self.type = type
self.description = description
self.upgrademessage = upgrademessage
self.postupgrademessage = None
self.postdowngrademessage = None
# By default for now, we assume every improvement touches
# all the things
self.touches_filelogs = True
self.touches_manifests = True
self.touches_changelog = True
self.touches_requirements = True
def __eq__(self, other):
if not isinstance(other, improvement):
# This is what python tell use to do
return NotImplemented
return self.name == other.name
def __ne__(self, other):
return not (self == other)
def __hash__(self):
return hash(self.name)
allformatvariant = [] # type: List[Type['formatvariant']]
def registerformatvariant(cls):
allformatvariant.append(cls)
return cls
class formatvariant(improvement):
"""an improvement subclass dedicated to repository format"""
type = FORMAT_VARIANT
### The following attributes should be defined for each class:
# machine-readable string uniquely identifying this improvement. it will be # machine-readable string uniquely identifying this improvement. it will be
# mapped to an action later in the upgrade process. # mapped to an action later in the upgrade process.
name = None name = None
# message intended for humans explaining the improvement in more detail, # message intended for humans explaining the improvement in more detail,
# including the implications of it ``FORMAT_VARIANT`` types, should be # including the implications of it ``FORMAT_VARIANT`` types, should be
# worded # worded
# Message intended for humans which will be shown post an upgrade # Message intended for humans which will be shown post an upgrade
# operation when the improvement will be added # operation when the improvement will be added
postupgrademessage = None postupgrademessage = None
# Message intended for humans which will be shown post an upgrade # Message intended for humans which will be shown post an upgrade
# operation in which this improvement was removed # operation in which this improvement was removed
postdowngrademessage = None postdowngrademessage = None
# By default for now, we assume every improvement touches all the things # By default for now, we assume every improvement touches all the things
# Whether this improvement touches filelogs
touches_filelogs = True touches_filelogs = True
# Whether this improvement touches manifests
touches_manifests = True touches_manifests = True
# Whether this improvement touches changelog
touches_changelog = True touches_changelog = True
# Whether this improvement changes repository requirements
touches_requirements = True touches_requirements = True
def __init__(self):
raise NotImplementedError() allformatvariant = [] # type: List[Type['formatvariant']]
def registerformatvariant(cls):
allformatvariant.append(cls)
return cls
class formatvariant(improvement):
"""an improvement subclass dedicated to repository format"""
type = FORMAT_VARIANT
@staticmethod @staticmethod
def fromrepo(repo): def fromrepo(repo):
"""current value of the variant in the repository""" """current value of the variant in the repository"""
raise NotImplementedError() raise NotImplementedError()
@staticmethod @staticmethod
def fromconfig(repo): def fromconfig(repo):
ALL_OPTIMISATIONS = [] ALL_OPTIMISATIONS = []
def register_optimization(obj): def register_optimization(obj):
ALL_OPTIMISATIONS.append(obj) ALL_OPTIMISATIONS.append(obj)
return obj return obj
register_optimization( class optimization(improvement):
improvement( """an improvement subclass dedicated to optimizations"""
name=b're-delta-parent',
type=OPTIMISATION, type = OPTIMISATION
@register_optimization
class redeltaparents(optimization):
name = b're-delta-parent'
type = OPTIMISATION
description=_( description = _(
b'deltas within internal storage will be recalculated to ' b'deltas within internal storage will be recalculated to '
b'choose an optimal base revision where this was not ' b'choose an optimal base revision where this was not '
b'already done; the size of the repository may shrink and ' b'already done; the size of the repository may shrink and '
b'various operations may become faster; the first time ' b'various operations may become faster; the first time '
b'this optimization is performed could slow down upgrade ' b'this optimization is performed could slow down upgrade '
b'execution considerably; subsequent invocations should ' b'execution considerably; subsequent invocations should '
b'not run noticeably slower' b'not run noticeably slower'
), )
upgrademessage=_( upgrademessage = _(
b'deltas within internal storage will choose a new ' b'deltas within internal storage will choose a new '
b'base revision if needed' b'base revision if needed'
),
)
) )
register_optimization(
improvement( @register_optimization
name=b're-delta-multibase', class redeltamultibase(optimization):
type=OPTIMISATION, name = b're-delta-multibase'
type = OPTIMISATION
description=_( description = _(
b'deltas within internal storage will be recalculated ' b'deltas within internal storage will be recalculated '
b'against multiple base revision and the smallest ' b'against multiple base revision and the smallest '
b'difference will be used; the size of the repository may ' b'difference will be used; the size of the repository may '
b'shrink significantly when there are many merges; this ' b'shrink significantly when there are many merges; this '
b'optimization will slow down execution in proportion to ' b'optimization will slow down execution in proportion to '
b'the number of merges in the repository and the amount ' b'the number of merges in the repository and the amount '
b'of files in the repository; this slow down should not ' b'of files in the repository; this slow down should not '
b'be significant unless there are tens of thousands of ' b'be significant unless there are tens of thousands of '
b'files and thousands of merges' b'files and thousands of merges'
), )
upgrademessage=_( upgrademessage = _(
b'deltas within internal storage will choose an ' b'deltas within internal storage will choose an '
b'optimal delta by computing deltas against multiple ' b'optimal delta by computing deltas against multiple '
b'parents; may slow down execution time ' b'parents; may slow down execution time '
b'significantly' b'significantly'
),
)
) )
register_optimization(
improvement( @register_optimization
name=b're-delta-all', class redeltaall(optimization):
type=OPTIMISATION, name = b're-delta-all'
type = OPTIMISATION
description=_( description = _(
b'deltas within internal storage will always be ' b'deltas within internal storage will always be '
b'recalculated without reusing prior deltas; this will ' b'recalculated without reusing prior deltas; this will '
b'likely make execution run several times slower; this ' b'likely make execution run several times slower; this '
b'optimization is typically not needed' b'optimization is typically not needed'
), )
upgrademessage=_( upgrademessage = _(
b'deltas within internal storage will be fully ' b'deltas within internal storage will be fully '
b'recomputed; this will likely drastically slow down ' b'recomputed; this will likely drastically slow down '
b'execution time' b'execution time'
),
)
) )
register_optimization(
improvement( @register_optimization
name=b're-delta-fulladd', class redeltafulladd(optimization):
type=OPTIMISATION, name = b're-delta-fulladd'
type = OPTIMISATION
description=_( description = _(
b'every revision will be re-added as if it was new ' b'every revision will be re-added as if it was new '
b'content. It will go through the full storage ' b'content. It will go through the full storage '
b'mechanism giving extensions a chance to process it ' b'mechanism giving extensions a chance to process it '
b'(eg. lfs). This is similar to "re-delta-all" but even ' b'(eg. lfs). This is similar to "re-delta-all" but even '
b'slower since more logic is involved.' b'slower since more logic is involved.'
), )
upgrademessage=_( upgrademessage = _(
b'each revision will be added as new content to the ' b'each revision will be added as new content to the '
b'internal storage; this will likely drastically slow ' b'internal storage; this will likely drastically slow '
b'down execution time, but some extensions might need ' b'down execution time, but some extensions might need '
b'it' b'it'
),
)
) )
def findoptimizations(repo): def findoptimizations(repo):
"""Determine optimisation that could be used during upgrade""" """Determine optimisation that could be used during upgrade"""
# These are unconditionally added. There is logic later that figures out # These are unconditionally added. There is logic later that figures out
# which ones to apply. # which ones to apply.
return list(ALL_OPTIMISATIONS) return list(ALL_OPTIMISATIONS)