[PATCH evolve-ext] evolve: evolve --rev should warn of unsolvable case and not crash

[Pierre-Yves David]

On 05/29/2015 03:32 PM, Laurent Charignon wrote:
> # HG changeset patch
> # User Laurent Charignon <lcharignon at fb.com>
> # Date 1432938715 25200
> #      Fri May 29 15:31:55 2015 -0700
> # Node ID 2750ff8ab0779cd4f478522f9d7781900b91a231
> # Parent  69e5de3e6129185469c2cbf98383ac6d58260d0c
> evolve: evolve --rev should warn of unsolvable case and not crash
>
> Before this patch, evolve --rev was crashing when the given set of troubles
> was impossible to solve. This patch fixes the crash by adding a new function
> to validate that the troubles are solvable before processing to resolution.

Actually, this changeset does not "warn" it aborts

1) abort should be done with "raise Abort(...)" cf Jordi comment in your 
fold message for details (especially regarding error message length)

2) We should probably just skip such revision with a warning.

3) detecting such situation may be done at run time, when we try to 
solve the revision trouble and realise we cannot. This will probably be 
cheaper and result in simpler code.

(3) Would also remove the crash that happen when such situation is 
triggered without --rev, and give you proper "right trouble with right 
situation" detection (as explained later in this review)

It is also a good practice to provide an example of the message in the 
commit description. This allow discussing it earlier.

> diff --git a/hgext/evolve.py b/hgext/evolve.py
> --- a/hgext/evolve.py
> +++ b/hgext/evolve.py
> @@ -1232,6 +1232,44 @@
>       if repo['.'] != startnode:
>           ui.status(_('working directory is now at %s\n') % repo['.'])
>
> +def unsolvable(repo, revs, troubles):
> +    """ Returns a string describing an issue in case revs cannot be solved
> +
> +    When a user tries to resolve the troubles in his repository, he can make
> +    a selection that is too narrow.
> +    Consider the following example:
> +
> +    O 5
> +    |
> +    O 4
> +    |
> +    O-----X
> +    .     .
> +    .     .
> +    .     .
> +
> +    hg evolve --rev 5 cannot be solved but hg evolve --rev 4:: can be
> +    This is because 5 is based on the troubled revision 4.

I think this unsolvability should focus on instability. It is probably 
possible to resolve some trouble (like divergence) with your parent 
troubles. It is instability that you cannot solve it you inherit if from 
your parent instead of behind the first old to be affected because your 
parent are obsolete.

> +    """
> +    sortedrevs = sorted(revs)

> +    for r in sortedrevs:
> +        p = repo[r].parents()
> +        p1 = p[0].rev() if p[0] is not None else -1
> +        p2 = p[1].rev() if not len(p) == 1 and p[1] is not None else -1

I know we can use these (if else) now but we should probably refrain to 
use it if the conditional in non trivial (as it is the case for p2)

Also, using changelog.parentrevs (and nullid exclusion) will likely be 
more efficient (This probably do not matter for this code)

> +        if p1 in sortedrevs or p2 in sortedrevs:

Sorted provides you a list, p1 in sortedrevs() will do a 
O(len(sortedrevs)) lookup. You want to stick with a set like object, use 
p1 in _revs.

> +            # At least one of the parent in revs. During the troubles
> +            # resolution, the parent's troubles will be resolved before we
> +            # reach this rev: it is therefore solvable, skip it.
> +            continue
> +        if p1 in troubles or p2 in troubles:
> +            # None of the parents are in revs but at least one of them is
> +            # troubled. During the troubles resolution we won't deal with that
> +            # parent before reaching this rev and we won't be able to solve it.
> +            return "%d cannot be solved because one of its parents is troubled\
> + but is not one of the revisions specified with --rev"%(r)

cf comment about "'troubles' are not the set you are looking for".

> +    return None
> +
>   @command('^evolve|stabilize|solve',
>       [('n', 'dry-run', False,
>           'do not perform actions, just print what would be done'),
> @@ -1305,6 +1343,11 @@
>           if not _revs:
>               ui.write_err("No troubled changes in the specified revisions\n")
>           else:
> +            issue = unsolvable(repo, _revs, troubled)
> +            if issue is not None:
> +                ui.write_err("Cannot solve troubles in the specified list.\n",
> +                             "%s\n" %(issue))
> +                return 1

cf above comment about using Abort

>               # For the progress bar to show
>               count = len(_revs)
>               for rev in _revs:
> diff --git a/tests/test-evolve.t b/tests/test-evolve.t
> --- a/tests/test-evolve.t
> +++ b/tests/test-evolve.t
> @@ -1027,5 +1027,44 @@
>     |
>     o  0	: a0 - test
>
> +Check hg evolve --rev on singled out commit
>
>
> +  $ hg up 19 -C
> +  1 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  $ mkcommit j1
> +  $ mkcommit j2
> +  $ mkcommit j3
> +  $ hg up .^^
> +  0 files updated, 0 files merged, 2 files removed, 0 files unresolved
> +  $ echo "hello" > j4
> +  $ hg add j4
> +  $ hg amend
> +  2 new unstable changesets
> +  $ hg glog
> +  @  25	: add j1 - test
> +  |
> +  | o  23	: add j3 - test
> +  | |
> +  | o  22	: add j2 - test
> +  | |
> +  | x  21	: add j1 - test
> +  |/
> +  | o  20	: add gh - test
> +  | |
> +  o |  19	: add gg - test
> +  |/
> +  o  18	: a3 - test
> +  |
> +  o  13	: bumped update to f37ed7a60f43: - test
> +  |
> +  o  11	: a2 - test
> +  |
> +  o  10	testbookmark: a1__ - test
> +  |
> +  o  0	: a0 - test
> +
> +  $ hg evolve --rev 23
> +  Cannot solve  troubles in the specified list.
> +  23 cannot be solved because one of its parents is troubled but is not one of the revisions specified with --rev

cf Jordi Feedback about gigantic message.

-- 
Pierre-Yves David