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

[Laurent Charignon]

> On Jun 1, 2015, at 1:41 AM, Pierre-Yves David <pierre-yves.david at ens-lyon.org> wrote:
> 
> 
> 
> 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)

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

I think we should abort in that case because it is just not matching what the user wants and he can end up in a weird state not really knowing how to recover.

> 
> 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.


I like to have this check beforehand as we will be able to combine it with the ordering computation (that we have to do eventually).
Another argument: this approach avoids adding too much code to the evolve function and allows to explain what we are doing in a lengthy doctoring.
I am not concerned about performance here: while talking about 'progress display', you said that the cost of going through the troubles before resolving instability is negligible, I agreed with you then.


> 
> (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)

I don't really understand despite reading the rest of the review, can you reformulate?

> 
> 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.

Good point, I will change the function name and docstring

> 
>> +    """
>> +    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)

Ok, agreed

> 
> 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.

Good catch

> 
>> +            # 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

ok

> 
>>              # 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.

ok

> 
> -- 
> Pierre-Yves David