[PATCH 2 of 3 v2] graft: support grafting across move/copy (issue4028)

[Gábor STEFANIK]

>


--------------------------------------------------------------------------
This message, including its attachments, is confidential. For more information please read NNG's email policy here:
http://www.nng.com/emailpolicy/
By responding to this email you accept the email policy.


-----Original Message-----
> From: Pierre-Yves David [mailto:pierre-yves.david at ens-lyon.org]
> Sent: Monday, August 08, 2016 5:13 PM
> To: Gábor STEFANIK <Gabor.STEFANIK at nng.com>; mercurial-
> devel at mercurial-scm.org
> Subject: Re: [PATCH 2 of 3 v2] graft: support grafting across move/copy
> (issue4028)
>
>
>
> On 08/04/2016 09:24 PM, Gábor Stefanik wrote:
> > # HG changeset patch
> > # User Gábor Stefanik <gabor.stefanik at nng.com> # Date 1470324130 -7200
> > #      Thu Aug 04 17:22:10 2016 +0200
> > # Node ID 077ebc5f39395428bdbe8e9aa57f7384f2aa93f7
> > # Parent  463cad7d2c55b25e250738688e32e820625d4271
> > graft: support grafting across move/copy (issue4028)
> >
> > Graft performs a merge with a false common ancestor, which must be
> > taken into account when tracking copies. Explicitly pass the real
> > common ancestor in this case, and track copies between the real and false
> common ancestors in reverse.
> >
> > With this change, when grafting a commit with a change to a file moved
> > earlier on the graft's source branch, the change is merged as expected
> > into the original
> > (unmoved) file, rather than recreating it under its new name.
> > It should also make it possible to eventually enable cross-branch
> > updates with merge.
> >
> > v2: handle the case when target branch also has a rename
> > v3: address review comments
> > v4: move ancestry check to mergecopies, split tests to separate commit
> > v5: split out parameter change, address review comments, re-include
> > tests
> > v6: fix reversed logic
> >
> > diff --git a/mercurial/copies.py b/mercurial/copies.py
> > --- a/mercurial/copies.py
> > +++ b/mercurial/copies.py
> > @@ -8,6 +8,7 @@
> >  from __future__ import absolute_import
> >
> >  import heapq
> > +import operator
> >
> >  from . import (
> >      node,
> > @@ -321,6 +322,24 @@
> >      if repo.ui.configbool('experimental', 'disablecopytrace'):
> >          return {}, {}, {}, {}
> >
> > +    # In certain scenarios (e.g. graft, update or rename), ca can be
> overridden
> > +    # We still need to know a real common ancestor in this case
> > +    # We can't just compute _c1.ancestor(_c2) and compare it to ca,
> because
> > +    # there can be multiple common ancestors, e.g. in case of bidmerge.
> > +    graft = False
> > +    cta = ca
> > +    # ca.descendant(wc) and ca.descendant(ca) are False, work around that
> > +    _c1 = c1 if c1.rev() is not None else c1.p1()
> > +    _c2 = c2 if c2.rev() is not None else c2.p1()
> > +    if ca.rev() is None: # the working copy can never be a common ancestor
> > +        graft = True
> > +    elif not (ca == _c1 or ca.descendant(_c1)):
> > +        graft = True
> > +    elif not (ca == _c2 or ca.descendant(_c2)):
> > +        graft = True
> > +    if graft:
> > +        cta = _c1.ancestor(_c2)
>
> Note, this seems to be "
>
> if (ca.rev is None
>      or (ca != _c1 or ca.descendant(_c1)
>      or (ca != _c2 or ca.descendant(_c2)):
>
> We should probably put if down that way.
>

Will do.

> > +
> >      limit = _findlimit(repo, c1.rev(), c2.rev())
> >      if limit is None:
> >          # no common ancestor, no copies @@ -330,28 +349,44 @@
> >      m1 = c1.manifest()
> >      m2 = c2.manifest()
> >      ma = ca.manifest()
> > +    mta = cta.manifest()
> >
> > +    # see checkcopies documentation below for these dicts
> >      copy1, copy2, = {}, {}
> > +    copyfrom, copyto = {}, {}
>
> Cf Matt comment about how we need data about three segments.
>
> > […]
> > @@ -842,3 +852,24 @@
> >    |/
> >    o  0
> >
> > +Graft from behind a move or rename
>
> It would be nice to have a bit more documentation about what is the
> scenario of this test. In addition using a 'hg log --graph' before the
> graft would help seeing the big picture. It would also be a good idea to
> reference the issue number so that people can go look at the discussion
> if needed.
>
> In addition as Matt pointed, there is multiples segment that might see a
> rename. We should get more than one test case to make sure the various
> combination are covered.

Per the wiki, even one new test is grounds for rejection ("change existing tests instead, the testsuite is slow enough already"). I'm already stretching the boundaries here, adding multiple tests would virtually guarantee that some senior developer would shoot it down for slowing down the testsuite.

>
> > +  $ hg init graftmove
> > +  $ cd graftmove
> > +  $ echo c1 > f1
> > +  $ hg ci -qAm 0
> > +  $ hg mv f1 f2
> > +  $ hg ci -qAm 1
> > +  $ echo c2 > f2
> > +  $ hg ci -qAm 2
> > +  $ hg up -q 0
> > +  $ hg graft -r 2
> > +  grafting 2:8a20493ece2a "2" (tip)
> > +  merging f1 and f2 to f1
> > +  $ hg status --change .
> > +  M f1
> > +  $ hg cat f1
> > +  c2
> > +  $ hg cat f2
> > +  f2: no such file in rev ee1f64f8088b
> > +  [1]
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel at mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >
>
> --
> Pierre-Yves David