[PATCH 05 of 10] repair: obtain and validate requirements for upgraded repo

[Pierre-Yves David]

On 11/21/2016 09:50 PM, Augie Fackler wrote:
> On Sat, Nov 05, 2016 at 09:40:21PM -0700, Gregory Szorc wrote:
>> # HG changeset patch
>> # User Gregory Szorc <gregory.szorc at gmail.com>
>> # Date 1478394961 25200
>> #      Sat Nov 05 18:16:01 2016 -0700
>> # Node ID b768004ef2db9c2e6dd267997e9e0c011f1b732a
>> # Parent  7518e68e2f8276e85fb68174b3055a9dd16c665d
>> repair: obtain and validate requirements for upgraded repo
>>
>> Not all existing repositories can be upgraded. Not all requirements
>> are supported in new repositories. Some transitions between
>> repository requirements (notably removing a requirement) are not
>> supported.
>>
>> This patch adds code for validating that the requirements transition
>> for repository upgrades is acceptable and aborts if it isn't.
>> Functionality is split into various functions to give extensions an
>> opportunity to monkeypatch.
>>
>> diff --git a/mercurial/repair.py b/mercurial/repair.py
>> --- a/mercurial/repair.py
>> +++ b/mercurial/repair.py
>> @@ -401,6 +401,85 @@ def upgradefinddeficiencies(repo):
>>
>>      return l, actions
>>
>> +def upgradesupporteddestrequirements(repo):
>> +    """Obtain requirements that upgrade supports in the destination.
>> +
>> +    Extensions should monkeypatch this to add their custom requirements.
>> +    """
>> +    return set([
>> +        'dotencode',
>> +        'fncache',
>> +        'generaldelta',
>> +        'manifestv2',
>> +        'revlogv1',
>> +        'store',
>> +        'treemanifest',
>> +    ])
>> +
>> +def upgraderequiredsourcerequirements(repo):
>> +    """Obtain requirements that must be present in the source repository."""
>> +    return set([
>> +        'revlogv1',
>> +        'store',
>> +    ])
>> +
>> +def upgradeallowednewrequirements(repo):
>> +    """Obtain requirements that can be added to a repository.
>> +
>> +    This is used to disallow proposed requirements from being added when
>> +    they weren't present before.
>> +
>> +    We use a whitelist of allowed requirement additions instead of a
>> +    blacklist of known bad additions because the whitelist approach is
>> +    safer and will prevent future, unknown requirements from accidentally
>> +    being added.
>> +    """
>> +    return set([
>> +        'dotencode',
>> +        'fncache',
>> +        'generaldelta',
>> +    ])
>> +
>> +def upgradereporequirements(repo):
>> +    """Obtain and validate requirements for repository after upgrade.
>> +
>> +    Should raise ``Abort`` if existing or new requirements aren't sufficient.
>> +    """
>> +    # Avoid cycle: cmdutil -> repair -> localrepo -> cmdutil
>> +    from . import localrepo
>
> It looks like there should be relatively minimal work to break the dep
> from localrepo to cmdutil. That dependency definitely looks suspect to
> me (dirstateguard seems like it should be in dirstate.py just from the
> name, and checkunresolved probably belongs in mergemod? or dirstate?)
>
> Not a blocker for this series, but wanted to bring this up. I dream of
> a future without any imports hidden inside a function to avoid cycles.
>
>> +
>> +    existing = repo.requirements
>> +
>> +    missingreqs = upgraderequiredsourcerequirements(repo) - existing
>> +    if missingreqs:
>> +        raise error.Abort(_('cannot upgrade repository; requirement '
>> +                            'missing: %s') % ', '.join(sorted(missingreqs)))
>> +
>> +    createreqs = localrepo.newreporequirements(repo)
>> +
>> +    # This might be overly restrictive. It is definitely safer to enforce this
>> +    # as it prevents unwanted deletion of requirements.
>
> I can't think of a world where we'd want to allow discarding
> requirements on upgrade.

If we come to a point where we can upgrade only part of the requirement, 
(cf my comment on 4.0) this would be useful.

>
>> +    removedreqs = existing - createreqs
>> +    if removedreqs:
>> +        raise error.Abort(_('cannot upgrade repository; requirement would '
>> +                            'be removed: %s') % ', '.join(sorted(removedreqs)))
>> +
>> +    unsupportedreqs = createreqs - upgradesupporteddestrequirements(repo)
>> +    if unsupportedreqs:
>> +        raise error.Abort(_('cannot upgrade repository; new requirement not '
>> +                           'supported: %s') %
>> +                          ', '.join(sorted(unsupportedreqs)))
>> +
>> +    # Alternatively, we could silently remove the disallowed requirement
>> +    # instead of aborting.
>> +    noaddreqs = createreqs - existing - upgradeallowednewrequirements(repo)
>> +    if noaddreqs:
>> +        raise error.Abort(_('cannot upgrade repository; proposed new '
>> +                            'requirement cannot be added: %s') %
>> +                          ', '.join(sorted(noaddreqs)))
>> +
>> +    return createreqs
>> +
>>  def upgraderepo(ui, repo, dryrun=False):
>>      """Upgrade a repository in place."""
>>      repo = repo.unfiltered()
>> @@ -414,3 +493,17 @@ def upgraderepo(ui, repo, dryrun=False):
>>              ui.write('* %s\n' % d)
>>      else:
>>          ui.write(_('no obvious deficiencies found in existing repository\n'))
>> +
>> +    ui.write(_('\n'))
>> +    ui.write(_('checking repository requirements...\n'))
>> +
>> +    newreqs = upgradereporequirements(repo)
>> +    # We don't drop requirements.
>> +    assert not len(repo.requirements - newreqs)
>> +
>> +    ui.write(_('preserving repository requirements: %s\n') %
>> +             ', '.join(sorted(repo.requirements & newreqs)))
>> +    if newreqs - repo.requirements:
>> +        ui.write(_('adding repository requirements: %s\n') %
>> +                 ', '.join(sorted(newreqs - repo.requirements)))
>> +
>
> [...]
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

-- 
Pierre-Yves David