[PATCH 1 of 4 evolve-ext] evolve: make --all use the same code path than --rev without filtering

[Pierre-Yves David]

On 05/05/2015 06:22 PM, Laurent Charignon wrote:
> # HG changeset patch
> # User Laurent Charignon <lcharignon at fb.com>
> # Date 1430506124 25200
> #      Fri May 01 11:48:44 2015 -0700
> # Node ID 9a6285b42bd3e81c107c3ba3a4e7a3bbe7c975db
> # Parent  1fe3da0b4601c690815f7e247a18067ef002cd6f
> evolve: make --all use the same code path than --rev without filtering
>
> This simplifies the code of the evolve function to make it easier to understand
> and more reusable in other context.

This options is nice, but it is going to regress/break in two cases:

1) If a revision is affect by multiple trouble, we will only solve one.
2) If revision are evolved in the wrong order, some will end up unstable 
after evolve --all.

There is multiple ways to fix (1) (some being more creative than other) 
I can think you can get with a test case and some simple fix by yourself.

(2) is a bit more tricky It happen when your stack of unstable changeset 
is not linear itself. Given three changesets:


    1:A -- 2:B -- 3:C

You amend B and get C unstable:

    1:A -- 2xB -- 3:C
        \- 4:B'

Then you amend A and get B' unstable


    1:A -- 2xB -- 3:C
        \- 4:B'
    5:A'

Now, both B' and C are unstable and needs to get stabilised on A'. 
However, evolving all troubled changeset in ascending order (behavior of 
old implementation and "maybe yours") is going to evolve 3:C before 
4:B'. So 3:C is going to be rebased above 4:B' and will become obsolete 
once 4:B' itself is rebased. The current implementation eventually reach 
a stable point because it's approach is: "Let's beat this poney hard 
until we have them all in the dead horses pile" leading to C being 
stabilized twice. Your (better) implementation will fail there because 
it touch everything only once. I think we needs either some sort of 
smart reordering of the list of revs to process or a delay mechanism 
that push back the resolution a troubled changeset if it going to evolve 
it about another troubled one.

>
> diff --git a/hgext/evolve.py b/hgext/evolve.py
> --- a/hgext/evolve.py
> +++ b/hgext/evolve.py
> @@ -1287,11 +1287,14 @@
>               raise util.Abort('cannot specify both "--all" and "--continue"')
>           graftcmd = commands.table['graft'][0]
>           return graftcmd(ui, repo, old_obsolete=True, **{'continue': True})
> -    # Rev specified on the commands line
> -    if revopt:
> -        revs = set(repo.revs(revopt))
> +    # Rev specified on the commands line or solving all the troubles
> +    if revopt or allopt:
>           troubled = set(repo.revs('troubled()'))


> -        _revs = revs & troubled
> +        if allopt:
> +            _revs = troubled
> +        else:
> +            revs = set(repo.revs(revopt))

I overlooked this use of `repo.revs` with user input. You should use 
revrange instead that will properly handle users input specific thing 
(revset alias, bookmark with -).

Also, having both _revs and revs is "meh" expecially is the 
super-short-lived-and-only-in-one-branch one is getting the clean (rev) one.

Usually, _variable are used for return value are are not going to be 
used (or for very local variable). Using it for "the list of revs we'll 
fix" seem inappropriate.

> +            _revs = revs & troubled

You should not need to turn everything to set since two smartsets should 
combine together smartly.

But his could simply be

   revs = repo.revs('%ld and troubled()', scmutil.revrange(revopt))



-- 
Pierre-Yves David