Patchwork [1,of,2] graft: allow creating sibling grafts

login
register
mail settings
Submitter Durham Goode
Date April 6, 2015, 2:24 a.m.
Message ID <409103499e22860c64bd.1428287066@dev2000.prn2.facebook.com>
Download mbox | patch
Permalink /patch/8505/
State Superseded
Commit a8e6897dffbe147198cc3f09cfb2ae090004666b
Headers show

Comments

Durham Goode - April 6, 2015, 2:24 a.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1428260138 25200
#      Sun Apr 05 11:55:38 2015 -0700
# Node ID 409103499e22860c64bd334e671753dfcf0072a6
# Parent  4a4018831d2ebc3c9cae9c6613e6a2497b4f0993
graft: allow creating sibling grafts

Previously it was impossible to graft a commit onto it's own parent (i.e. create
a copy of the commit). This is useful when wanting to create a backup of the
commit before continuing to amend it. This patch enables that behavior.

The change to the histedit test is because histedit uses graft to apply commits.
The test in question moves a commit backwards onto an ancestor. Since the graft
logic now more explicitly supports this, it knows to simply accept the incoming
changes (since they are more recent), instead of prompting.
Matt Mackall - April 6, 2015, 8:29 p.m.
On Sun, 2015-04-05 at 19:24 -0700, Durham Goode wrote:
> # HG changeset patch
> # User Durham Goode <durham@fb.com>
> # Date 1428260138 25200
> #      Sun Apr 05 11:55:38 2015 -0700
> # Node ID 409103499e22860c64bd334e671753dfcf0072a6
> # Parent  4a4018831d2ebc3c9cae9c6613e6a2497b4f0993
> graft: allow creating sibling grafts
> 
> Previously it was impossible to graft a commit onto it's own parent (i.e. create
> a copy of the commit). This is useful when wanting to create a backup of the
> commit before continuing to amend it. This patch enables that behavior.

I'm about +0 on these two patches from functionality level, but I'm
finding the patch itself confusing.

> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -1184,12 +1184,14 @@ def graft(repo, ctx, pctx, labels):
>      labels - merge labels eg ['local', 'graft']
>  
>      """
> +    wpctx = repo['.']
> +    mergeancestor = repo.changelog.isancestor(wpctx.node(), ctx.node())
> +    stats = update(repo, ctx.node(), True, True, False, pctx.node(),
> +                   mergeancestor=mergeancestor, labels=labels)

So.. we should allow merging with an ancestor whenever the target is an
ancestor? If that's actually the case, then this patch should be a
single line adding mergeancestor=True. But the only case we've described
so far is just when the target is p1(source).

(I think we should in fact be able to graft patches onto ancestors. For
instance, grafting a bug fix back on to the not-yet-diverged release
point, but am less motivated by the particular abuse^Wuse case you have
in mind. Similarly, I think there's value in tracking repeated grafts.)

>      repo.dirstate.beginparentchange()
> -    repo.setparents(repo['.'].node(), nullid)
> +    repo.setparents(wpctx.node(), nullid)

Why is this changing? Is it just a clean-up because we've made wpctx a
variable above? If so, it actually obscures the intent a bit. We just
want to drop the second parent _of the current state_, and it's not
immediately obvious to someone reading the code that wpctx is still
relevant after an update().
Durham Goode - April 6, 2015, 8:43 p.m.
On 4/6/15 1:29 PM, Matt Mackall wrote:
> On Sun, 2015-04-05 at 19:24 -0700, Durham Goode wrote:
>> # HG changeset patch
>> # User Durham Goode <durham@fb.com>
>> # Date 1428260138 25200
>> #      Sun Apr 05 11:55:38 2015 -0700
>> # Node ID 409103499e22860c64bd334e671753dfcf0072a6
>> # Parent  4a4018831d2ebc3c9cae9c6613e6a2497b4f0993
>> graft: allow creating sibling grafts
>>
>> Previously it was impossible to graft a commit onto it's own parent (i.e. create
>> a copy of the commit). This is useful when wanting to create a backup of the
>> commit before continuing to amend it. This patch enables that behavior.
> I'm about +0 on these two patches from functionality level, but I'm
> finding the patch itself confusing.
>
>> diff --git a/mercurial/merge.py b/mercurial/merge.py
>> --- a/mercurial/merge.py
>> +++ b/mercurial/merge.py
>> @@ -1184,12 +1184,14 @@ def graft(repo, ctx, pctx, labels):
>>       labels - merge labels eg ['local', 'graft']
>>   
>>       """
>> +    wpctx = repo['.']
>> +    mergeancestor = repo.changelog.isancestor(wpctx.node(), ctx.node())
>> +    stats = update(repo, ctx.node(), True, True, False, pctx.node(),
>> +                   mergeancestor=mergeancestor, labels=labels)
> So.. we should allow merging with an ancestor whenever the target is an
> ancestor? If that's actually the case, then this patch should be a
> single line adding mergeancestor=True. But the only case we've described
> so far is just when the target is p1(source).
mergeancestor isn't just about merging with the ancestor, it's about 
telling the merge logic to always accept the incoming change (in the 
case of a prompt, like 'remote changed (c) but local deleted (d)') 
because we know they are more recent.  That's not the case for all 
grafts, so I didn't want to set it to True all the time.
>
> (I think we should in fact be able to graft patches onto ancestors. For
> instance, grafting a bug fix back on to the not-yet-diverged release
> point, but am less motivated by the particular abuse^Wuse case you have
> in mind. Similarly, I think there's value in tracking repeated grafts.)
We already can graft on to ancestors (using graft -f), just not the 
immediate parent (which is what this fixes).
>
>>       repo.dirstate.beginparentchange()
>> -    repo.setparents(repo['.'].node(), nullid)
>> +    repo.setparents(wpctx.node(), nullid)
> Why is this changing? Is it just a clean-up because we've made wpctx a
> variable above? If so, it actually obscures the intent a bit. We just
> want to drop the second parent _of the current state_, and it's not
> immediately obvious to someone reading the code that wpctx is still
> relevant after an update().
This was just cleanup since we already had the value, and update in this 
case doesn't change the working copy (i think?).  I see how it could be 
confusing though. I can change it back.
Matt Mackall - April 6, 2015, 8:50 p.m.
On Mon, 2015-04-06 at 13:43 -0700, Durham Goode wrote:
> 
> On 4/6/15 1:29 PM, Matt Mackall wrote:
> > On Sun, 2015-04-05 at 19:24 -0700, Durham Goode wrote:
> >> # HG changeset patch
> >> # User Durham Goode <durham@fb.com>
> >> # Date 1428260138 25200
> >> #      Sun Apr 05 11:55:38 2015 -0700
> >> # Node ID 409103499e22860c64bd334e671753dfcf0072a6
> >> # Parent  4a4018831d2ebc3c9cae9c6613e6a2497b4f0993
> >> graft: allow creating sibling grafts
> >>
> >> Previously it was impossible to graft a commit onto it's own parent (i.e. create
> >> a copy of the commit). This is useful when wanting to create a backup of the
> >> commit before continuing to amend it. This patch enables that behavior.
> > I'm about +0 on these two patches from functionality level, but I'm
> > finding the patch itself confusing.
> >
> >> diff --git a/mercurial/merge.py b/mercurial/merge.py
> >> --- a/mercurial/merge.py
> >> +++ b/mercurial/merge.py
> >> @@ -1184,12 +1184,14 @@ def graft(repo, ctx, pctx, labels):
> >>       labels - merge labels eg ['local', 'graft']
> >>   
> >>       """
> >> +    wpctx = repo['.']
> >> +    mergeancestor = repo.changelog.isancestor(wpctx.node(), ctx.node())
> >> +    stats = update(repo, ctx.node(), True, True, False, pctx.node(),
> >> +                   mergeancestor=mergeancestor, labels=labels)
> > So.. we should allow merging with an ancestor whenever the target is an
> > ancestor? If that's actually the case, then this patch should be a
> > single line adding mergeancestor=True. But the only case we've described
> > so far is just when the target is p1(source).
> mergeancestor isn't just about merging with the ancestor, it's about 
> telling the merge logic to always accept the incoming change (in the 
> case of a prompt, like 'remote changed (c) but local deleted (d)') 
> because we know they are more recent.  That's not the case for all 
> grafts, so I didn't want to set it to True all the time.

Ok, so there's officially enough magic here to require a comment.

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -1184,12 +1184,14 @@  def graft(repo, ctx, pctx, labels):
     labels - merge labels eg ['local', 'graft']
 
     """
+    wpctx = repo['.']
+    mergeancestor = repo.changelog.isancestor(wpctx.node(), ctx.node())
+    stats = update(repo, ctx.node(), True, True, False, pctx.node(),
+                   mergeancestor=mergeancestor, labels=labels)
 
-    stats = update(repo, ctx.node(), True, True, False, pctx.node(),
-                   labels=labels)
     # drop the second merge parent
     repo.dirstate.beginparentchange()
-    repo.setparents(repo['.'].node(), nullid)
+    repo.setparents(wpctx.node(), nullid)
     repo.dirstate.write()
     # fix up dirstate for copies and renames
     copies.duplicatecopies(repo, ctx.rev(), pctx.rev())
diff --git a/tests/test-graft.t b/tests/test-graft.t
--- a/tests/test-graft.t
+++ b/tests/test-graft.t
@@ -730,3 +730,29 @@  Empty graft
   $ hg graft -f 27
   grafting 27:3d35c4c79e5a "28"
   note: graft of 27:3d35c4c79e5a created no changes to commit
+
+  $ cd ..
+
+Graft to duplicate a commit
+
+  $ hg init graftsibling
+  $ cd graftsibling
+  $ touch a
+  $ hg commit -qAm a
+  $ touch b
+  $ hg commit -qAm b
+  $ hg log -G -T '{rev}\n'
+  @  1
+  |
+  o  0
+  
+  $ hg up -q 0
+  $ hg graft -r 1
+  grafting 1:0e067c57feba "b" (tip)
+  $ hg log -G -T '{rev}\n'
+  @  2
+  |
+  | o  1
+  |/
+  o  0
+  
diff --git a/tests/test-histedit-non-commute-abort.t b/tests/test-histedit-non-commute-abort.t
--- a/tests/test-histedit-non-commute-abort.t
+++ b/tests/test-histedit-non-commute-abort.t
@@ -70,8 +70,6 @@  edit the history
   > pick 652413bf663e f
   > EOF
   0 files updated, 0 files merged, 2 files removed, 0 files unresolved
-  remote changed e which local deleted
-  use (c)hanged version or leave (d)eleted? c
   0 files updated, 0 files merged, 0 files removed, 0 files unresolved
   merging e
   warning: conflicts during merge.