[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: Matt Mackall [mailto:mpm at selenic.com]
> Sent: Saturday, August 06, 2016 1:25 AM
> 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 Thu, 2016-08-04 at 14:24 -0500, 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.
>
> I'm pretty sure this patch is not sufficient to correctly fix the bug. But it's also
> extremely hard to tell. Please try to separate out all the preparatory
> plumbing changes into their own steps so that you can provide a short patch
> that has the guts of your change.

I don't see anything that can be meaningfully factored out.

>
> There are at least three stretches of copy data that we need to compute to
> correctly calculate a graft/rebase merge, as I detailed with pretty pictures in
> my last email:
>
> 1: graft changeset -> graft parent (effective mergebase)
> 2: graft parent -> real common ancestor (results must be reversed)
> 3: target -> real common ancestor
>
> Then the second (reversed set) need to be -chained- to the first set (like
> pathcopies does) to use for one leg of the merge. So if there's a rename
> from A-
> >B on one side and A->C on the other, we end up with an effect C->A->B =
> >C->B
> rename when doing the merge.
>
> Here's a new diagram with the copy tracing segments labeled:
>
>  3
>  v
> A--B---B' <- B' is the C' change grafted onto B
>  \ <- 2
>   C--C' <- graft this change
>    ^
>    1
>
> And here it's "rotated" so that C is the merge base and 2 is now reversed:
>
>  2r  3
>  v   v
> C--A--B---B' <- find the change in C' and graft it onto B
>  \ <- 1
>   C'
>
> ..and the merge thus uses chained(reversed(2), 3) for one leg of the merge
> and just 1 for the other.

This patch deals with segments 1 and 2 in one checkcopies() pass (reversing renames as necessary).
Then, mergecopies does the chaining between 2 and 3 for renames that span both checkcopies calls.

>
> > +    # In certain scenarios (e.g. graft, update or rename), ca can be
> > overridden
>
> Did you mean rebase?

Yes, nice catch.

>
> > +    if not graft:
> > +        unmatched = u1 + u2
> > +    else: # need to recompute this for directory move handling when
> > +grafting
> > +        unmatched = operator.add(*_computenonoverlap(repo, c1, c2,
> > +                                 m1.filesnotin(ma),
> > +m2.filesnotin(ma),
> > False))
>
> The "three segments" observation applies here as well.

I'm pretty sure there is no need to find renames in 3 passes here if we are just going to combine them anyhow.

I use ma rather than mta here, so we are already getting segment 1 on one side and 2+3 on the other, with 2 reversed.

>
> > +
> >      bothnew = sorted(addedinm1 & addedinm2)
> >
> >      for f in u1:
> > -        checkcopies(c1, f, m1, m2, ca, limit, diverge, copy1,
> > fullcopy1)
> > +        checkcopies(c1, f, m1, m2, ca, cta, limit, diverge, copy1,
> > +                    fullcopy1, copyfrom, copyto)
> >
> >      for f in u2:
> > -        checkcopies(c2, f, m2, m1, ca, limit, diverge, copy2,
> > fullcopy2)
> > +        checkcopies(c2, f, m2, m1, ca, cta, limit, diverge, copy2,
> > +                    fullcopy2, copyfrom, copyto)
> >
> >      copy = dict(copy1.items() + copy2.items())
> >      movewithdir = dict(movewithdir1.items() + movewithdir2.items())
> >      fullcopy = dict(fullcopy1.items() + fullcopy2.items())
> >
> > +    # combine partial copy paths discovered in the previous step
> > +    for f in copyfrom:
> > +        if f in copyto:
> > +            copy[copyto[f]] = copyfrom[f]
> > +
>
> I'm not sure what's happening here.

If there is a rename of the same file on both sides of the topological common ancestor, and one is supposed to be reversed, the other isn't, chain them together.

Checkcopies uses the copyfrom and copyto arrays if it finds divergence w.r.t. the topological common ancestor. If it's a forward rename, it's recorded in copyto, while a rename that needs to be reversed is put into copyfrom. Thus, if there is both a forward and reverse rename of the same file w.r.t. the topological ancestor, they need to be chained, which is done here.

I believe the only thing this may get wrong is if there is a rename on all 3 segments - but that's divergent renames, and I'm not sure graft is able to handle that anyway.

>
> --
> Mathematics is the supreme nostalgia of our time.