[PATCH v2] graft: support grafting across move/copy (issue4028)
Pierre-Yves David
pierre-yves.david at ens-lyon.org
Tue Aug 2 07:21:00 EDT 2016
On 08/02/2016 11:27 AM, Gábor Stefanik wrote:
> # HG changeset patch
> # User Gábor Stefanik <gabor.stefanik at nng.com>
> # Date 1470047695 -7200
> # Mon Aug 01 12:34:55 2016 +0200
> # Node ID f5220d4f9655dade72857fe310410fb73062bba9
> # Parent 73ff159923c1f05899c27238409ca398342d9ae0
> graft: support grafting across move/copy (issue4028)
There is a lot of things happening in this one patch. You change fairly
low level functions having impact in many places. Making this a series
of smaller patches each doing a small independent step toward your end
goal would make this easier to review and help refine it.
For example, your changes to the updates APIs very often reuse an
argument we already pass to it. (for 'manifestmerge', 'pta' is often
'pa'; for 'calculateupdates', topo_ancestors is often ancestors[0]). So
we could probably leave the main signature untouched in the common case.
Only adding a named argument to be used by the special case.
Can you send a V3 series with the changes to the low level API in small
independent patches ? Each patch should be functional itself.
> 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
>
> diff --git a/contrib/perf.py b/contrib/perf.py
> --- a/contrib/perf.py
> +++ b/contrib/perf.py
> @@ -357,8 +357,8 @@
> def d():
> # acceptremote is True because we don't want prompts in the middle of
> # our benchmark
> - merge.calculateupdates(repo, wctx, rctx, [ancestor], False, False,
> - acceptremote=True, followcopies=True)
> + merge.calculateupdates(repo, wctx, rctx, [ancestor], ancestor, False,
> + False, acceptremote=True, followcopies=True)
> timer(d)
> fm.end()
>
> diff --git a/hgext/convert/hg.py b/hgext/convert/hg.py
> --- a/hgext/convert/hg.py
> +++ b/hgext/convert/hg.py
> @@ -202,7 +202,7 @@
> anc = [p1ctx.ancestor(p2ctx)]
> # Calculate what files are coming from p2
> actions, diverge, rename = mergemod.calculateupdates(
> - self.repo, p1ctx, p2ctx, anc,
> + self.repo, p1ctx, p2ctx, anc, anc[0],
> True, # branchmerge
> True, # force
> False, # acceptremote
> diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
> --- a/hgext/largefiles/overrides.py
> +++ b/hgext/largefiles/overrides.py
> @@ -465,11 +465,11 @@
> # Finally, the merge.applyupdates function will then take care of
> # writing the files into the working copy and lfcommands.updatelfiles
> # will update the largefiles.
> -def overridecalculateupdates(origfn, repo, p1, p2, pas, branchmerge, force,
> - acceptremote, *args, **kwargs):
> +def overridecalculateupdates(origfn, repo, p1, p2, pas, pta, branchmerge,
Note that your naming of the argument here 'pta' is not consistent with
the name change in calculteupdates 'topo_ancestor'
> + force, acceptremote, *args, **kwargs):
> overwrite = force and not branchmerge
> - actions, diverge, renamedelete = origfn(
> - repo, p1, p2, pas, branchmerge, force, acceptremote, *args, **kwargs)
> + actions, diverge, renamedelete = origfn(repo, p1, p2, pas, pta,
> + branchmerge, force, acceptremote, *args, **kwargs)
>
> if overwrite:
> return actions, diverge, renamedelete
> 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,
> @@ -231,7 +232,7 @@
> return _chain(x, y, _backwardrenames(x, a),
> _forwardcopies(a, y, match=match))
>
> -def _computenonoverlap(repo, c1, c2, addedinm1, addedinm2):
> +def _computenonoverlap(repo, c1, c2, addedinm1, addedinm2, silent=False):
> """Computes, based on addedinm1 and addedinm2, the files exclusive to c1
> and c2. This is its own function so extensions can easily wrap this call
> to see what files mergecopies is about to process.
> @@ -242,10 +243,10 @@
> u1 = sorted(addedinm1 - addedinm2)
> u2 = sorted(addedinm2 - addedinm1)
>
> - if u1:
> + if u1 and not silent:
> repo.ui.debug(" unmatched files in local:\n %s\n"
> % "\n ".join(u1))
> - if u2:
> + if u2 and not silent:
> repo.ui.debug(" unmatched files in other:\n %s\n"
> % "\n ".join(u2))
> return u1, u2
This adds of a 'silent' argument is a good example of something that
should go in an independent patch. What is it for? Why do we need it?
> @@ -285,7 +286,7 @@
> return fctx
> return util.lrucachefunc(makectx)
>
> -def mergecopies(repo, c1, c2, ca):
> +def mergecopies(repo, c1, c2, ca, cta=None):
This change to merge copy is another good example, adding the new
argument in an independent patch will make it easier to review the code
change. And as you do not change the function signature, all code should
keep working as before.
> """
> Find moves and copies between context c1 and c2 that are relevant
> for merging.
> @@ -321,6 +322,10 @@
> if repo.ui.configbool('experimental', 'disablecopytrace'):
> return {}, {}, {}, {}
>
> + # cta will only differ from ca when grafting or during non-linear updates
We do not have non-linear update (yet?), you should avoid mentioning it
the comments.
> + if cta is None:
> + cta = ca
> +
> limit = _findlimit(repo, c1.rev(), c2.rev())
> if limit is None:
> # no common ancestor, no copies
> @@ -330,28 +335,43 @@
> m1 = c1.manifest()
> m2 = c2.manifest()
> ma = ca.manifest()
> + mta = cta.manifest()
>
> copy1, copy2, = {}, {}
> + copyfrom, copyto = {}, {}
> movewithdir1, movewithdir2 = {}, {}
> fullcopy1, fullcopy2 = {}, {}
> diverge = {}
>
> # find interesting file sets from manifests
> - addedinm1 = m1.filesnotin(ma)
> - addedinm2 = m2.filesnotin(ma)
> - u1, u2 = _computenonoverlap(repo, c1, c2, addedinm1, addedinm2)
> + addedinm1 = m1.filesnotin(mta)
> + addedinm2 = m2.filesnotin(mta)
> + u1, u2 = _computenonoverlap(repo, c1, c2, addedinm1, addedinm2, ca != cta)
> + if ca == cta:
> + unmatched = u1 + u2
You seems to be doing multiple comparison of ca and cta along the code
base. Using a variable with an explicit name to carry this information
might help readability.
> + 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))
Using operator here seems a bit overkill. What about just adding them in
an extra line?
> 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]
> +
> renamedelete = {}
> renamedeleteset = set()
> divergeset = set()
> @@ -369,10 +389,12 @@
> if bothnew:
> repo.ui.debug(" unmatched files new in both:\n %s\n"
> % "\n ".join(bothnew))
> - bothdiverge, _copy, _fullcopy = {}, {}, {}
> + bothdiverge, _copy, _fullcopy, _copyfrom, _copyto = {}, {}, {}, {}, {}
At that point we start having enough related variable that it gets hard
to follow, I was about to request documentation, but it actually already
exists in checkcopies(). We should probably add a small comment pointing
to it here.
> for f in bothnew:
> - checkcopies(c1, f, m1, m2, ca, limit, bothdiverge, _copy, _fullcopy)
> - checkcopies(c2, f, m2, m1, ca, limit, bothdiverge, _copy, _fullcopy)
> + checkcopies(c1, f, m1, m2, ca, cta, limit, bothdiverge, _copy,
> + _fullcopy, _copyfrom, _copyto)
> + checkcopies(c2, f, m2, m1, ca, cta, limit, bothdiverge, _copy,
> + _fullcopy, _copyfrom, _copyto)
> for of, fl in bothdiverge.items():
> if len(fl) == 2 and fl[0] == fl[1]:
> copy[fl[0]] = of # not actually divergent, just matching renames
> @@ -438,7 +460,7 @@
> (d, dirmove[d]))
>
> # check unaccounted nonoverlapping files against directory moves
> - for f in u1 + u2:
> + for f in unmatched:
> if f not in fullcopy:
> for d in dirmove:
> if f.startswith(d):
> @@ -452,7 +474,8 @@
>
> return copy, movewithdir, diverge, renamedelete
>
> -def checkcopies(ctx, f, m1, m2, ca, limit, diverge, copy, fullcopy):
> +def checkcopies(ctx, f, m1, m2, ca, cta, limit, diverge, copy, fullcopy,
> + copyfrom, copyto):
> """
> check possible copies of f from m1 to m2
>
> @@ -460,14 +483,19 @@
> f = the filename to check
> m1 = the source manifest
> m2 = the destination manifest
> - ca = the changectx of the common ancestor
> + ca = the changectx of the common ancestor, overridden on graft
> + cta = topological common ancestor for graft-like scenarios
> limit = the rev number to not search beyond
> diverge = record all diverges in this dict
> copy = record all non-divergent copies in this dict
> fullcopy = record all copies in this dict
> + copyfrom = source sides of partially known copy tracks
> + copyto = destination sides of partially known copytracks
> """
>
> ma = ca.manifest()
> + mta = cta.manifest()
> + backwards = f in ma # graft common ancestor already contains the rename
> getfctx = _makegetfctx(ctx)
>
> def _related(f1, f2, limit):
> @@ -513,20 +541,32 @@
> continue
> seen.add(of)
>
> - fullcopy[f] = of # remember for dir rename detection
> + # remember for dir rename detection
> + if backwards:
> + fullcopy[of] = f # grafting backwards through renames
> + else:
> + fullcopy[f] = of
> if of not in m2:
> continue # no match, keep looking
> if m2[of] == ma.get(of):
> break # no merge needed, quit early
> c2 = getfctx(of, m2[of])
> - cr = _related(oc, c2, ca.rev())
> + cr = _related(oc, c2, cta.rev())
> if cr and (of == f or of == c2.path()): # non-divergent
> - copy[f] = of
> + if backwards:
> + copy[of] = f
> + else:
> + copy[f] = of
> of = None
> break
>
> if of in ma:
> diverge.setdefault(of, []).append(f)
> + elif of in mta:
> + if backwards:
> + copyfrom[of] = f
> + else:
> + copyto[of] = f
>
> def duplicatecopies(repo, rev, fromrev, skiprev=None):
> '''reproduce copies from fromrev to rev in the dirstate
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -778,11 +778,12 @@
> This is currently not implemented -- it's an extension point."""
> return True
>
> -def manifestmerge(repo, wctx, p2, pa, branchmerge, force, matcher,
> +def manifestmerge(repo, wctx, p2, pa, pta, branchmerge, force, matcher,
> acceptremote, followcopies):
cf comment at the start of the file about keeping the signature
untouched in the common case.
> """
> Merge p1 and p2 with ancestor pa and generate merge action list
>
> + pta = topological common ancestor for graft, needed for rename detection
> branchmerge and force are as passed in to update
> matcher = matcher to filter file lists
> acceptremote = accept the incoming changes without prompting
> @@ -797,7 +798,7 @@
> sorted(wctx.parents() + [p2, pa], key=lambda x: x.rev())]
>
> if followcopies:
> - ret = copies.mergecopies(repo, wctx, p2, pa)
> + ret = copies.mergecopies(repo, wctx, p2, pa, pta)
> copy, movewithdir, diverge, renamedelete = ret
>
> repo.ui.note(_("resolving manifests\n"))
> @@ -937,14 +938,14 @@
> # remote did change but ended up with same content
> del actions[f] # don't get = keep local deleted
>
> -def calculateupdates(repo, wctx, mctx, ancestors, branchmerge, force,
> - acceptremote, followcopies, matcher=None,
> +def calculateupdates(repo, wctx, mctx, ancestors, topo_ancestor, branchmerge,
As per our style guide, we do not use '_' in variable name. Also see my
comment at the top of this email about avoid to break the signature in
the common case.
> […]
--
Pierre-Yves David
More information about the Mercurial-devel
mailing list