[PATCH 2 of 4 evolve-ext] evolve: restructure evolve function into trouble selection and resolution

[Pierre-Yves David]

On 05/05/2015 06:22 PM, Laurent Charignon wrote:
> # HG changeset patch
> # User Laurent Charignon <lcharignon at fb.com>
> # Date 1430507008 25200
> #      Fri May 01 12:03:28 2015 -0700
> # Node ID 5b3c5ad3a7e110b0f64ceeb11830f992d49d3ff2
> # Parent  9a6285b42bd3e81c107c3ba3a4e7a3bbe7c975db
> evolve: restructure evolve function into trouble selection and resolution
>
> This makes the code easier to understand and reduce the branching.
>
> diff --git a/hgext/evolve.py b/hgext/evolve.py
> --- a/hgext/evolve.py
> +++ b/hgext/evolve.py
> @@ -1268,12 +1268,6 @@
>       confirmopt = opts['confirm']
>       revopt = opts['rev']
>       ui.setconfig('ui', 'forcemerge', opts.get('tool', ''), 'evolve')
> -    troubled = set(repo.revs('troubled()'))
> -
> -    # Progress handling
> -    seen = 1
> -    count = allopt and len(troubled) or 1
> -    showprogress = allopt
>
>       def progresscb():
>           if revopt or allopt:
> @@ -1287,39 +1281,34 @@
>               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 or solving all the troubles
> -    if revopt or allopt:
> -        troubled = set(repo.revs('troubled()'))
> -        if allopt:
> -            _revs = troubled
> -        else:
> -            revs = set(repo.revs(revopt))
> -            _revs = revs & troubled
> -        if not _revs:
> -            ui.write_err("No troubled changes in the specified revset")
> -        else:
> -            # For the progress bar to show
> -            count = len(_revs)
> -            for rev in _revs:
> -                progresscb()
> -                _solveone(ui, repo, repo[rev], dryrunopt, confirmopt,
> -                        progresscb)
> -                seen += 1
> -            progresscb()
> -            _cleanup(ui, repo, startnode, showprogress)
> -            return
> -    nexttrouble = _picknexttroubled(ui, repo, anyopt or allopt)
> -    # No trouble to resolve
> -    if not nexttrouble:
> +
> +    # Select revision to fix
> +    if allopt:
> +        _revs = set(repo.revs('troubled()'))
> +    elif revopt:
> +        _revs = set(repo.revs(revopt)) & set(repo.revs('troubled()'))

Why do we keep turning things into set here? It does not seems useful. 
Moreover it you never turn the list of revs into and ordered lists so 
the order we process them seems undermined (recipe to disaster)


> +    else:
> +        _revs = _picknexttroubled(ui, repo, anyopt)

All others tests assign a set to '_revs' but _picknexttroubled return a 
simple integer. You should turn that into a list directly from here.

> +    # No troubles found, warn the user that they might want to use --all
> +    if not _revs:
>           return handlenotrouble(ui, repo, startnode, dryrunopt)
>
> -    if not allopt:
> -        # Resolving a single trouble
> -        _solveone(ui, repo, nexttrouble, dryrunopt, confirmopt, progresscb)
> -
> +    # Solve the troubles
> +    seen = 1
> +    if not isinstance(_revs, set):
> +        _revs = set([_revs])

1) using 'isinstance' in python code is usually a bad idea. Expecially 
in such simple code. Fixing the handling of _picknexttroubled should fix 
that.

2) You should not be iterating on set, this will have unpredictable 
order, It sounds like a suspicious miracle than test have been passing 
so far since you cannot evolve the children before the parent.

> +    count = len(_revs)
> +
> +    for rev in _revs:
> +        progresscb()

(note: I would not have any issue with triggering the progress bar in 
all cases (even single rev to evolve) and drop the progresscb logic). 
(but this is unrelated to this change)

> +        _solveone(ui, repo, repo[rev], dryrunopt, confirmopt,

Three is a trailing white space on this line that you could kill in the 
process.

> +                progresscb)
> +        seen += 1
> +    progresscb()
> +    showprogress = count >= 2
>       _cleanup(ui, repo, startnode, showprogress)
>
> -
>   def _evolveany(ui, repo, tro, dryrunopt, confirmopt, progresscb):
>       repo = repo.unfiltered()
>       tro = repo[tro.rev()]
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>

-- 
Pierre-Yves David