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

login
register
mail settings
Submitter Andrew Halberstadt
Date Nov. 27, 2015, 2:41 a.m.
Message ID <21b5e4b3df092855c554.1448592080@localhost.localdomain>
Download mbox | patch
Permalink /patch/11681/
State Changes Requested
Delegated to: Pierre-Yves David
Headers show

Comments

Andrew Halberstadt - Nov. 27, 2015, 2:41 a.m.
# HG changeset patch
# User Andrew Halberstadt <ahalberstadt@mozilla.com>
# Date 1448588311 18000
#      Thu Nov 26 20:38:31 2015 -0500
# Node ID 21b5e4b3df092855c554331587fdd7b713b9551e
# Parent  72f50a17780674de8f372803a333d43c77230df5
evolve: handle merge commit with single obsolete parent (issue4389)
Pierre-Yves David - Dec. 2, 2015, 7:58 p.m.
On 11/26/2015 06:41 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 21b5e4b3df092855c554331587fdd7b713b9551e
> # Parent  72f50a17780674de8f372803a333d43c77230df5
> evolve: handle merge commit with single obsolete parent (issue4389)

This is a nice change, but it is done in the wrong layer.

The relocate function is not expected to know anything about the higher 
level evolution process. We should not be adding evolution related logic 
as this patch do.

The code modified here is responsible for finding the proper merge base 
and bail out in case of merge because of the ambiguity. We should add 
some "parents/base" argument and compute what it should be in the higher 
function actually calling relocate.

> diff --git a/hgext/evolve.py b/hgext/evolve.py
> --- a/hgext/evolve.py
> +++ b/hgext/evolve.py
> @@ -899,20 +899,32 @@ def relocate(repo, orig, dest, keepbranc
>       """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")
> +    parents = orig.parents()
> +    if len(parents) == 2:
> +        if all(p.obsolete() for p in parents):
> +            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")
> +
> +        for p in parents:
> +            if p.obsolete():
> +                pctx = p
> +            else:
> +                pother = p.node()
> +    else:
> +        pctx = orig.p1()
> +        pother = nullid
> +
>       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 +954,26 @@ 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'])
> +            r = merge.graft(repo, orig, pctx, ['local', 'graft'])
> +
> +            if pother != nullid:
> +                # Add back the original second parent which got nullified
> +                # by merge.graft().
> +                repo.dirstate.beginparentchange()
> +                repo.setparents(repo['.'].node(), pother)
> +                repo.dirstate.write(repo.currenttransaction())
> +                repo.dirstate.endparentchange()

Could we get hg graft able to grat merge instead (basically the same 
drill as what is needed of relocate)

Many thanks for working on this.
Andrew Halberstadt - Dec. 2, 2015, 8:25 p.m.
On 02/12/15 02:58 PM, Pierre-Yves David wrote:
> On 11/26/2015 06:41 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 21b5e4b3df092855c554331587fdd7b713b9551e
>> # Parent  72f50a17780674de8f372803a333d43c77230df5
>> evolve: handle merge commit with single obsolete parent (issue4389)
>
> This is a nice change, but it is done in the wrong layer.
>
> The relocate function is not expected to know anything about the higher
> level evolution process. We should not be adding evolution related logic
> as this patch do.
>
> The code modified here is responsible for finding the proper merge base
> and bail out in case of merge because of the ambiguity. We should add
> some "parents/base" argument and compute what it should be in the higher
> function actually calling relocate.

Makes sense. I only put it there because that's where the error message
was coming from.


>> -            r = merge.graft(repo, orig, orig.p1(), ['local', 'graft'])
>> +            r = merge.graft(repo, orig, pctx, ['local', 'graft'])
>> +
>> +            if pother != nullid:
>> +                # Add back the original second parent which got
>> nullified
>> +                # by merge.graft().
>> +                repo.dirstate.beginparentchange()
>> +                repo.setparents(repo['.'].node(), pother)
>> +                repo.dirstate.write(repo.currenttransaction())
>> +                repo.dirstate.endparentchange()
>
> Could we get hg graft able to grat merge instead (basically the same
> drill as what is needed of relocate)
>
> Many thanks for working on this.

I was originally going to fix this in merge.graft, but that would make
evolve incompatible with older versions of mercurial. Would I need to
add some sort of version check somewhere? Or does the fact that evolve
is experimental mean we don't care.

-Andrew
Pierre-Yves David - Dec. 2, 2015, 8:27 p.m.
On 12/02/2015 12:25 PM, Andrew Halberstadt wrote:
> On 02/12/15 02:58 PM, Pierre-Yves David wrote:
>> On 11/26/2015 06:41 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 21b5e4b3df092855c554331587fdd7b713b9551e
>>> # Parent  72f50a17780674de8f372803a333d43c77230df5
>>> evolve: handle merge commit with single obsolete parent (issue4389)
>>
>> This is a nice change, but it is done in the wrong layer.
>>
>> The relocate function is not expected to know anything about the higher
>> level evolution process. We should not be adding evolution related logic
>> as this patch do.
>>
>> The code modified here is responsible for finding the proper merge base
>> and bail out in case of merge because of the ambiguity. We should add
>> some "parents/base" argument and compute what it should be in the higher
>> function actually calling relocate.
>
> Makes sense. I only put it there because that's where the error message
> was coming from.

The error message is a small layer violation in itself (pleading guilty)

>>> -            r = merge.graft(repo, orig, orig.p1(), ['local', 'graft'])
>>> +            r = merge.graft(repo, orig, pctx, ['local', 'graft'])
>>> +
>>> +            if pother != nullid:
>>> +                # Add back the original second parent which got
>>> nullified
>>> +                # by merge.graft().
>>> +                repo.dirstate.beginparentchange()
>>> +                repo.setparents(repo['.'].node(), pother)
>>> +                repo.dirstate.write(repo.currenttransaction())
>>> +                repo.dirstate.endparentchange()
>>
>> Could we get hg graft able to grat merge instead (basically the same
>> drill as what is needed of relocate)
>>
>> Many thanks for working on this.
>
> I was originally going to fix this in merge.graft, but that would make
> evolve incompatible with older versions of mercurial. Would I need to
> add some sort of version check somewhere? Or does the fact that evolve
> is experimental mean we don't care.

We do not care to much. We are okay with newer feature being only 
available for newer mercurial. eg: having some try except around the 
callgraph to catch the unknown new parameter would do the trick.

Patch

diff --git a/hgext/evolve.py b/hgext/evolve.py
--- a/hgext/evolve.py
+++ b/hgext/evolve.py
@@ -899,20 +899,32 @@  def relocate(repo, orig, dest, keepbranc
     """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")
+    parents = orig.parents()
+    if len(parents) == 2:
+        if all(p.obsolete() for p in parents):
+            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")
+
+        for p in parents:
+            if p.obsolete():
+                pctx = p
+            else:
+                pother = p.node()
+    else:
+        pctx = orig.p1()
+        pother = nullid
+
     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 +954,26 @@  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'])
+            r = merge.graft(repo, orig, pctx, ['local', 'graft'])
+
+            if pother != nullid:
+                # Add back the original second parent which got nullified
+                # by merge.graft().
+                repo.dirstate.beginparentchange()
+                repo.setparents(repo['.'].node(), pother)
+                repo.dirstate.write(repo.currenttransaction())
+                repo.dirstate.endparentchange()
+
             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']
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
@@ -155,17 +151,17 @@  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:[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