Patchwork [04,of,10] repair: identify repository deficiencies

# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1478382332 25200
#      Sat Nov 05 14:45:32 2016 -0700
# Node ID 7518e68e2f8276e85fb68174b3055a9dd16c665d
# Parent  9daec9c7adabe8c84cf2c01fc938e010ee4884d6
repair: identify repository deficiencies

A command that upgrades repositories but doesn't say what it is doing
or why it is doing it is less helpful than a command that does. So,
we start the implementation of repository upgrades by introducing
detection of sub-optimal repository state. Deficiencies with the
existing repository are now printed at the beginning of command
execution.

The added function returns a set of planned upgrade actions. This
variable will be used by subsequent patches.

Gregory Szorc wrote:
> A command that upgrades repositories but doesn't say what it is doing

shouldn't it still mention that it isn't doing the work?

On Sun, Nov 6, 2016 at 2:03 AM, timeless <timeless@gmail.com> wrote:

> Gregory Szorc wrote:
> > A command that upgrades repositories but doesn't say what it is doing
>
> shouldn't it still mention that it isn't doing the work?
>

Good question. I'm very open to UX improvements for this command. It is
debug functionality (for now), so I didn't spend as much energy on the UX
as I could have.

FWIW, I do have aspirations for command flags to control upgrade options. I
also thought it might be useful for something in non-debug land (`hg
summary`?) to expose informative "your repo isn't optimally stored"
messages. That's partially the reason "identify deficiencies" and
"determine final actions" are separate. (The main reason is putting it all
in 1 function will make it difficult for extensions to monkeypatch.)

On Sat, Nov 05, 2016 at 09:40:20PM -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1478382332 25200
> #      Sat Nov 05 14:45:32 2016 -0700
> # Node ID 7518e68e2f8276e85fb68174b3055a9dd16c665d
> # Parent  9daec9c7adabe8c84cf2c01fc938e010ee4884d6
> repair: identify repository deficiencies

Mostly happy so far, some string bikeshedding below. Tried to elide
irrelevant bits of the message so they'll be easy to spot.

>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -3756,7 +3756,7 @@ def debugupgraderepo(ui, repo, **opts):
>
>      At times during the upgrade, the repository may not be readable.
>      """
> -    raise error.Abort(_('not yet implemented'))
> +    return repair.upgraderepo(ui, repo, dryrun=opts.get('dry_run'))
>
>  @command('debugwalk', walkopts, _('[OPTION]... [FILE]...'), inferrepo=True)
>  def debugwalk(ui, repo, *pats, **opts):
> diff --git a/mercurial/repair.py b/mercurial/repair.py
> --- a/mercurial/repair.py
> +++ b/mercurial/repair.py
> @@ -360,3 +360,57 @@ def deleteobsmarkers(obsstore, indices):
>          newobsstorefile.write(bytes)
>      newobsstorefile.close()
>      return n
> +
> +def upgradefinddeficiencies(repo):
> +    """Obtain deficiencies with the existing repo and planned actions to fix.
> +
> +    Returns a list of strings that will be printed to the user and a set

[...]

> +
> +    if 'generaldelta' not in repo.requirements:
> +        l.append(_('not using generaldelta storage; repository is larger '
> +                   'and slower than it could be, pulling from '
> +                   'generaldelta repositories will be slow'))

Perhaps word as "pulling from modern servers may be slow" - I'm not
sure that "generaldelta repositories" is really a helpful string to
print to normal humans.

> +        actions.add('generaldelta')
> +
> +    cl = repo.changelog
> +    for rev in cl:
> +        chainbase = cl.chainbase(rev)
> +        if chainbase != rev:
> +            l.append(_('changelog using delta chains; changelog reading '
> +                       'is slower than it could be'))

"changelog is storing deltas; changelog reading is slower than it could be"

(chains are irrelevant here, what matters is that we're trying to
store deltas at all)

> +            actions.add('removecldeltachain')
> +            break
> +
> +    return l, actions
> +
> +def upgraderepo(ui, repo, dryrun=False):
> +    """Upgrade a repository in place."""
> +    repo = repo.unfiltered()
> +    deficiencies, actions = upgradefinddeficiencies(repo)
> +
> +    if deficiencies:
> +        ui.write(_('the following deficiencies with the existing repository '
> +                   'have been identified:\n\n'))
> +
> +        for d in deficiencies:
> +            ui.write('* %s\n' % d)
> +    else:
> +        ui.write(_('no obvious deficiencies found in existing repository\n'))

[...]

Augie Fackler wrote:
> Perhaps word as "pulling from modern servers may be slow" - I'm not
> sure that "generaldelta repositories" is really a helpful string to
> print to normal humans.

> "changelog is storing deltas; changelog reading is slower than it could be"

I'm +1 to these suggestions / justifications.