Patchwork [evolve-ext] evolve: handle merge commit with single obsolete parent (issue4389)

# HG changeset patch
# User Andrew Halberstadt <ahalberstadt@mozilla.com>
# Date 1448588311 18000
#      Thu Nov 26 20:38:31 2015 -0500
# Node ID 51adab56f47c124702d2b7f2df1030a16c49678d
# Parent  333e056b3034aa328bc7a260b0cdcf641fe3d323
evolve: handle merge commit with single obsolete parent (issue4389)

This handles evolving merge commits with a single obsolete parent. Merge
commits with two obsolete parents are still unsupported. Note this depends
on a change to merge.graft in core. Older versions of mercurial will not
have this functionality. Also, test-unstable.t will fail with older
versions.

On 04/12/15 08:33 PM, Andrew Halberstadt wrote:
> # HG changeset patch
> # User Andrew Halberstadt <ahalberstadt@mozilla.com>
> # Date 1448588311 18000
> #      Thu Nov 26 20:38:31 2015 -0500
> # Node ID 51adab56f47c124702d2b7f2df1030a16c49678d
> # Parent  333e056b3034aa328bc7a260b0cdcf641fe3d323
> evolve: handle merge commit with single obsolete parent (issue4389)
>
> This handles evolving merge commits with a single obsolete parent. Merge
> commits with two obsolete parents are still unsupported. Note this depends
> on a change to merge.graft in core. Older versions of mercurial will not
> have this functionality. Also, test-unstable.t will fail with older
> versions.

There are still two "problems" with this patch I could use some advice on.

1. test-unstable.t fails with versions of mercurial that don't have the
merge.graft change I submitted earlier. Is that ok?

2. This seems to change the sha1's in a lot of tests. I'm not sure why,
or what to do about it.

-Andrew


> diff --git a/hgext/evolve.py b/hgext/evolve.py
> --- a/hgext/evolve.py
> +++ b/hgext/evolve.py
> @@ -890,29 +890,26 @@ def rewrite(repo, old, updates, head, ne
>           tr.close()
>           return newid, created
>       finally:
>           lockmod.release(lock, wlock, tr)
>
>   class MergeFailure(util.Abort):
>       pass
>
> -def relocate(repo, orig, dest, keepbranch=False):
> +def relocate(repo, orig, dest, pctx=None, keepbranch=False):
>       """rewrite <rev> on dest"""
>       if orig.rev() == dest.rev():
>           raise util.Abort(_('tried to relocate a node on top of itself'),
>                            hint=_("This shouldn't happen. If you still "
>                                   "need to move changesets, please do so "
>                                   "manually with nothing to rebase - working "
>                                   "directory parent is also destination"))
>
> -    if not orig.p2().rev() == node.nullrev:
> -        raise util.Abort(
> -            'no support for evolving merge changesets yet',
> -            hint="Redo the merge and use `hg prune <old> --succ <new>` to obsolete the old one")
> +    pctx = pctx or orig.p1()
>       destbookmarks = repo.nodebookmarks(dest.node())
>       nodesrc = orig.node()
>       destphase = repo[nodesrc].phase()
>       commitmsg = orig.description()
>
>       cache = {}
>       sha1s = re.findall(sha1re, commitmsg)
>       unfi = repo.unfiltered()
> @@ -942,17 +939,28 @@ def relocate(repo, orig, dest, keepbranc
>           try:
>               if repo['.'].rev() != dest.rev():
>                   merge.update(repo, dest, False, True, False)
>               if bmactive(repo):
>                   repo.ui.status(_("(leaving bookmark %s)\n") % bmactive(repo))
>               bmdeactivate(repo)
>               if keepbranch:
>                   repo.dirstate.setbranch(orig.branch())
> -            r = merge.graft(repo, orig, orig.p1(), ['local', 'graft'])
> +
> +            try:
> +                r = merge.graft(repo, orig, pctx, ['local', 'graft'], True)
> +            except TypeError:
> +                # not using recent enough mercurial
> +                if len(orig.parents()) == 2:
> +                    raise util.Abort(
> +                        'no support for evolving merge changesets yet',
> +                        hint="Redo the merge and use `hg prune <old> --succ <new>` to obsolete the old one")
> +
> +                r = merge.graft(repo, orig, pctx, ['local', 'graft'])
> +
>               if r[-1]:  #some conflict
>                   raise util.Abort(
>                           'unresolved merge conflicts (see hg help resolve)')
>               if commitmsg is None:
>                   commitmsg = orig.description()
>               extra = dict(orig.extra())
>               if 'branch' in extra:
>                   del extra['branch']
> @@ -1743,23 +1751,29 @@ def _aspiringdescendant(repo, revs):
>               if unstable not in result:
>                   tovisit.append(unstable)
>                   result.add(unstable)
>       return sorted(result - target)
>
>   def _solveunstable(ui, repo, orig, dryrun=False, confirm=False,
>                      progresscb=None):
>       """Stabilize a unstable changeset"""
> -    obs = orig.parents()[0]
> -    if not obs.obsolete() and len(orig.parents()) == 2:
> -        obs = orig.parents()[1] # second parent is obsolete ?
> -
> -    if not obs.obsolete():
> +    pctx = orig.p1()
> +    if len(orig.parents()) == 2:
> +        if not pctx.obsolete():
> +            pctx = orig.p2()  # second parent is obsolete ?
> +        elif orig.p2().obsolete():
> +            raise util.Abort(
> +                'no support for evolving merge changesets with two obsolete parents yet',
> +                hint="Redo the merge and use `hg prune <old> --succ <new>` to obsolete the old one")
> +
> +    if not pctx.obsolete():
>           ui.warn("cannot solve instability of %s, skipping\n" % orig)
>           return False
> +    obs = pctx
>       newer = obsolete.successorssets(repo, obs.node())
>       # search of a parent which is not killed
>       while not newer or newer == [()]:
>           ui.debug("stabilize target %s is plain dead,"
>                    " trying to stabilize on its parent\n" %
>                    obs)
>           obs = obs.parents()[0]
>           newer = obsolete.successorssets(repo, obs.node())
> @@ -1794,17 +1808,17 @@ def _solveunstable(ui, repo, orig, dryru
>       todo = 'hg rebase -r %s -d %s\n' % (orig, target)
>       if dryrun:
>           repo.ui.write(todo)
>       else:
>           repo.ui.note(todo)
>           if progresscb: progresscb()
>           keepbranch = orig.p1().branch() != orig.branch()
>           try:
> -            relocate(repo, orig, target, keepbranch)
> +            relocate(repo, orig, target, pctx, keepbranch)
>           except MergeFailure:
>               repo.opener.write('graftstate', orig.hex() + '\n')
>               repo.ui.write_err(_('evolve failed!\n'))
>               repo.ui.write_err(
>                   _('fix conflict and run "hg evolve --continue"'
>                     ' or use "hg update -C" to abort\n'))
>               raise
>
> diff --git a/tests/test-unstable.t b/tests/test-unstable.t
> --- a/tests/test-unstable.t
> +++ b/tests/test-unstable.t
> @@ -98,27 +98,23 @@ Not supported yet
>     | x  1:b3264cec9506@default(draft) add _a
>     |/
>     o  0:b4952fcf48cf@default(draft) add base
>
>
>     $ hg evo --all --any --unstable
>     move:[3] merge
>     atop:[4] aprime
> -  abort: no support for evolving merge changesets yet
> -  (Redo the merge and use `hg prune <old> --succ <new>` to obsolete the old one)
> -  [255]
> +  working directory is now at 0bf3f3a59c8c
>     $ hg log -G
> -  @  4:47127ea62e5f@default(draft) aprime
> -  |
> -  | o    3:6b4280e33286@default(draft) merge
> -  | |\
> -  +---o  2:474da87dd33b@default(draft) add _c
> +  @    5:0bf3f3a59c8c@default(draft) merge
> +  |\
> +  | o  4:47127ea62e5f@default(draft) aprime
>     | |
> -  | x  1:b3264cec9506@default(draft) add _a
> +  o |  2:474da87dd33b@default(draft) add _c
>     |/
>     o  0:b4952fcf48cf@default(draft) add base
>
>
>     $ cd ..
>
>   ===============================================================================
>   Test instability resolution for a merge changeset unstable because both
> @@ -153,19 +149,17 @@ Not supported yet
>     +---x  2:474da87dd33b@default(draft) add _c
>     | |
>     | x  1:b3264cec9506@default(draft) add _a
>     |/
>     o  0:b4952fcf48cf@default(draft) add base
>
>
>     $ hg evo --all --any --unstable
> -  move:[3] merge
> -  atop:[5] cprime
> -  abort: no support for evolving merge changesets yet
> +  abort: no support for evolving merge changesets with two obsolete parents yet
>     (Redo the merge and use `hg prune <old> --succ <new>` to obsolete the old one)
>     [255]
>     $ hg log -G
>     @  5:2db39fda7e2f@default(draft) cprime
>     |
>     | o  4:47127ea62e5f@default(draft) aprime
>     |/
>     | o    3:6b4280e33286@default(draft) merge
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>

On 12/04/2015 05:37 PM, Andrew Halberstadt wrote:
> 2. This seems to change the sha1's in a lot of tests. I'm not sure why,
> or what to do about it.

The sha change is unrelated, I just pushed Laurent fix for it.

On 12/04/2015 05:33 PM, Andrew Halberstadt wrote:
> # HG changeset patch
> # User Andrew Halberstadt <ahalberstadt@mozilla.com>
> # Date 1448588311 18000
> #      Thu Nov 26 20:38:31 2015 -0500
> # Node ID 51adab56f47c124702d2b7f2df1030a16c49678d
> # Parent  333e056b3034aa328bc7a260b0cdcf641fe3d323
> evolve: handle merge commit with single obsolete parent (issue4389)

That patch is close to be ready. I just have a small feedback on the way 
we handle pctx=None

(btw, is it even possible to have pctx to None now? should we forbid it?)

> This handles evolving merge commits with a single obsolete parent. Merge
> commits with two obsolete parents are still unsupported. Note this depends
> on a change to merge.graft in core. Older versions of mercurial will not
> have this functionality. Also, test-unstable.t will fail with older
> versions.
>
> diff --git a/hgext/evolve.py b/hgext/evolve.py
> --- a/hgext/evolve.py
> +++ b/hgext/evolve.py
> @@ -890,29 +890,26 @@ def rewrite(repo, old, updates, head, ne
>           tr.close()
>           return newid, created
>       finally:
>           lockmod.release(lock, wlock, tr)
>
>   class MergeFailure(util.Abort):
>       pass
>
> -def relocate(repo, orig, dest, keepbranch=False):
> +def relocate(repo, orig, dest, pctx=None, keepbranch=False):
>       """rewrite <rev> on dest"""
>       if orig.rev() == dest.rev():
>           raise util.Abort(_('tried to relocate a node on top of itself'),
>                            hint=_("This shouldn't happen. If you still "
>                                   "need to move changesets, please do so "
>                                   "manually with nothing to rebase - working "
>                                   "directory parent is also destination"))
>
> -    if not orig.p2().rev() == node.nullrev:
> -        raise util.Abort(
> -            'no support for evolving merge changesets yet',
> -            hint="Redo the merge and use `hg prune <old> --succ <new>` to obsolete the old one")
> +    pctx = pctx or orig.p1()

We should still detect case where pctx is None and orig has two parents 
and bail out. This would catch bad API usage.

Also, if you are testing, please explicitly test for None

The mercurial blessed patter for this assignement is:

if pctx is None:
     pctx = orig.p1()

>       destbookmarks = repo.nodebookmarks(dest.node())
>       nodesrc = orig.node()
>       destphase = repo[nodesrc].phase()
>       commitmsg = orig.description()
>
>       cache = {}
>       sha1s = re.findall(sha1re, commitmsg)
>       unfi = repo.unfiltered()
> @@ -942,17 +939,28 @@ def relocate(repo, orig, dest, keepbranc
>           try:
>               if repo['.'].rev() != dest.rev():
>                   merge.update(repo, dest, False, True, False)
>               if bmactive(repo):
>                   repo.ui.status(_("(leaving bookmark %s)\n") % bmactive(repo))
>               bmdeactivate(repo)
>               if keepbranch:
>                   repo.dirstate.setbranch(orig.branch())
> -            r = merge.graft(repo, orig, orig.p1(), ['local', 'graft'])
> +
> +            try:
> +                r = merge.graft(repo, orig, pctx, ['local', 'graft'], True)
> +            except TypeError:
> +                # not using recent enough mercurial
> +                if len(orig.parents()) == 2:
> +                    raise util.Abort(
> +                        'no support for evolving merge changesets yet',
> +                        hint="Redo the merge and use `hg prune <old> --succ <new>` to obsolete the old one")
> +
> +                r = merge.graft(repo, orig, pctx, ['local', 'graft'])
> +
>               if r[-1]:  #some conflict
>                   raise util.Abort(
>                           'unresolved merge conflicts (see hg help resolve)')
>               if commitmsg is None:
>                   commitmsg = orig.description()
>               extra = dict(orig.extra())
>               if 'branch' in extra:
>                   del extra['branch']
> @@ -1743,23 +1751,29 @@ def _aspiringdescendant(repo, revs):
>               if unstable not in result:
>                   tovisit.append(unstable)
>                   result.add(unstable)
>       return sorted(result - target)
>
>   def _solveunstable(ui, repo, orig, dryrun=False, confirm=False,
>                      progresscb=None):
>       """Stabilize a unstable changeset"""
> -    obs = orig.parents()[0]
> -    if not obs.obsolete() and len(orig.parents()) == 2:
> -        obs = orig.parents()[1] # second parent is obsolete ?
> -
> -    if not obs.obsolete():
> +    pctx = orig.p1()
> +    if len(orig.parents()) == 2:
> +        if not pctx.obsolete():
> +            pctx = orig.p2()  # second parent is obsolete ?
> +        elif orig.p2().obsolete():
> +            raise util.Abort(
> +                'no support for evolving merge changesets with two obsolete parents yet',
> +                hint="Redo the merge and use `hg prune <old> --succ <new>` to obsolete the old one")
> +

Fun fact: the next step to be able to handle merge is to evolve unstable 
merge twice, one time for each parents. That will be further 
modification in:
1) this very block
2) the logic scheduling resolution to schedule these two steps.

(not that this would be the most simple way to be able to evolve such 
merge, a nicer way probably involves a consensus merge like approach).

On 07/12/15 01:17 PM, Pierre-Yves David wrote:
>
>
> On 12/04/2015 05:33 PM, Andrew Halberstadt wrote:
>> # HG changeset patch
>> # User Andrew Halberstadt <ahalberstadt@mozilla.com>
>> # Date 1448588311 18000
>> #      Thu Nov 26 20:38:31 2015 -0500
>> # Node ID 51adab56f47c124702d2b7f2df1030a16c49678d
>> # Parent  333e056b3034aa328bc7a260b0cdcf641fe3d323
>> evolve: handle merge commit with single obsolete parent (issue4389)
>
> That patch is close to be ready. I just have a small feedback on the way
> we handle pctx=None
>
> (btw, is it even possible to have pctx to None now? should we forbid it?)

There is another call to relocate in _solvebumped that doesn't pass in 
pctx. I'm not 100% sure what a bumped changeset is, but if that's 
something that should be solved as part of issue4389, I can look into it.

Thanks. I'm about to head out to a work week and going on vacation 
after, so may be a couple weeks until I have time for a new patch.

-Andrew

>> This handles evolving merge commits with a single obsolete parent. Merge
>> commits with two obsolete parents are still unsupported. Note this
>> depends
>> on a change to merge.graft in core. Older versions of mercurial will not
>> have this functionality. Also, test-unstable.t will fail with older
>> versions.
>>
>> diff --git a/hgext/evolve.py b/hgext/evolve.py
>> --- a/hgext/evolve.py
>> +++ b/hgext/evolve.py
>> @@ -890,29 +890,26 @@ def rewrite(repo, old, updates, head, ne
>>           tr.close()
>>           return newid, created
>>       finally:
>>           lockmod.release(lock, wlock, tr)
>>
>>   class MergeFailure(util.Abort):
>>       pass
>>
>> -def relocate(repo, orig, dest, keepbranch=False):
>> +def relocate(repo, orig, dest, pctx=None, keepbranch=False):
>>       """rewrite <rev> on dest"""
>>       if orig.rev() == dest.rev():
>>           raise util.Abort(_('tried to relocate a node on top of
>> itself'),
>>                            hint=_("This shouldn't happen. If you still "
>>                                   "need to move changesets, please do
>> so "
>>                                   "manually with nothing to rebase -
>> working "
>>                                   "directory parent is also
>> destination"))
>>
>> -    if not orig.p2().rev() == node.nullrev:
>> -        raise util.Abort(
>> -            'no support for evolving merge changesets yet',
>> -            hint="Redo the merge and use `hg prune <old> --succ
>> <new>` to obsolete the old one")
>> +    pctx = pctx or orig.p1()
>
> We should still detect case where pctx is None and orig has two parents
> and bail out. This would catch bad API usage.
>
> Also, if you are testing, please explicitly test for None
>
> The mercurial blessed patter for this assignement is:
>
> if pctx is None:
>      pctx = orig.p1()
 >
>>       destbookmarks = repo.nodebookmarks(dest.node())
>>       nodesrc = orig.node()
>>       destphase = repo[nodesrc].phase()
>>       commitmsg = orig.description()
>>
>>       cache = {}
>>       sha1s = re.findall(sha1re, commitmsg)
>>       unfi = repo.unfiltered()
>> @@ -942,17 +939,28 @@ def relocate(repo, orig, dest, keepbranc
>>           try:
>>               if repo['.'].rev() != dest.rev():
>>                   merge.update(repo, dest, False, True, False)
>>               if bmactive(repo):
>>                   repo.ui.status(_("(leaving bookmark %s)\n") %
>> bmactive(repo))
>>               bmdeactivate(repo)
>>               if keepbranch:
>>                   repo.dirstate.setbranch(orig.branch())
>> -            r = merge.graft(repo, orig, orig.p1(), ['local', 'graft'])
>> +
>> +            try:
>> +                r = merge.graft(repo, orig, pctx, ['local', 'graft'],
>> True)
>> +            except TypeError:
>> +                # not using recent enough mercurial
>> +                if len(orig.parents()) == 2:
>> +                    raise util.Abort(
>> +                        'no support for evolving merge changesets yet',
>> +                        hint="Redo the merge and use `hg prune <old>
>> --succ <new>` to obsolete the old one")
>> +
>> +                r = merge.graft(repo, orig, pctx, ['local', 'graft'])
>> +
>>               if r[-1]:  #some conflict
>>                   raise util.Abort(
>>                           'unresolved merge conflicts (see hg help
>> resolve)')
>>               if commitmsg is None:
>>                   commitmsg = orig.description()
>>               extra = dict(orig.extra())
>>               if 'branch' in extra:
>>                   del extra['branch']
>> @@ -1743,23 +1751,29 @@ def _aspiringdescendant(repo, revs):
>>               if unstable not in result:
>>                   tovisit.append(unstable)
>>                   result.add(unstable)
>>       return sorted(result - target)
>>
>>   def _solveunstable(ui, repo, orig, dryrun=False, confirm=False,
>>                      progresscb=None):
>>       """Stabilize a unstable changeset"""
>> -    obs = orig.parents()[0]
>> -    if not obs.obsolete() and len(orig.parents()) == 2:
>> -        obs = orig.parents()[1] # second parent is obsolete ?
>> -
>> -    if not obs.obsolete():
>> +    pctx = orig.p1()
>> +    if len(orig.parents()) == 2:
>> +        if not pctx.obsolete():
>> +            pctx = orig.p2()  # second parent is obsolete ?
>> +        elif orig.p2().obsolete():
>> +            raise util.Abort(
>> +                'no support for evolving merge changesets with two
>> obsolete parents yet',
>> +                hint="Redo the merge and use `hg prune <old> --succ
>> <new>` to obsolete the old one")
>> +
>
> Fun fact: the next step to be able to handle merge is to evolve unstable
> merge twice, one time for each parents. That will be further
> modification in:
> 1) this very block
> 2) the logic scheduling resolution to schedule these two steps.
>
> (not that this would be the most simple way to be able to evolve such
> merge, a nicer way probably involves a consensus merge like approach).
>