martinvonz added a comment.
In https://phab.mercurial-scm.org/D6255#91015, @marmoute wrote: > I did a first path through it, the new code seems reasonable and easier > to read than the previous one. Some comments and questions below. The rest somehow didn't make it here, so I'll copy from the email (i.e. the below is from Pierre-Yves, not from me): I did a first path through it, the new code seems reasonable and easier to read than the previous one. Some comments and questions below. On 4/16/19 7:19 PM, martinvonz (Martin von Zweigbergk) wrote: > martinvonz created this revision. > Herald added subscribers: mercurial-devel, mjpieters. > Herald added a reviewer: hg-reviewers. > > REVISION SUMMARY > When copies are stored in changesets, we need a changeset-centric > version of mergecopies() just like we have a changeset-centric version > of pathcopies(). I think the natural way of thinking about > mergecopies() is in terms of pathcopies() from the base to each of the > commits. So if we can rewrite mergecopies() based two such > pathcopies() calls, we'll get the changeset-centric version for > free. That's what this patch does. I like the approach, if I understand it correctly, we'll have less duplicated code in the end. > > A nice bonus is that it ends up being a lot simpler. mergecopies() has > accumulated a lot of technical debt over time. One good example is the > code for dealing with grafts (the "partial/incomplete/dirty" > stuff). Since pathcopies() already deals with backwards renames and > ping-pong renames, we get that for free. > > I've run tests with hard-coded debug logging for "fullcopy" and while > I haven't looked at every difference it produces, all the ones I have > looked at seemed reasonable to me. How many difference did you had? can you share some example of them with their explanation? on related topic, you seems to be fixing a case in test-fastannotate-hg.t and test-annotate-hg.t that is probably worth mentioning. > One drawback of the rewritten code is that we may now make > remotefilelog prefetch more files. We used to prefetch files that were > unique to either side of the merge compared to the other. We now > prefetch files that are unique to either sise of the merge compared to sise → side > the base. This means that if you added the same file to each side, we > would not prefetch it before, but we would now. Such cases are > probably quite rare, but one likely scenario where they happen is when > moving from a commit to its successor (or the other way around). The > user will probably already have the files in the cache in such cases, > so it's probably not a big deal. > > Some timings for calculating mergecopies between two revisions (all > using the common ancestor as base): Which revisions did you pick? (for the record, the benchmark suite use 1daa622bbe42 and 76caed42cf7c) > In the hg repo: > 4.8 4.8: 0.21s -> 0.21s > 4.0 4.8: 0.35s -> 0.63s > > In and old copy of the mozilla-unified repo: > FIREFOX_BETA_60_BASE^ FIREFOX_BETA_60_BASE: 0.51s -> 0.60s > FIREFOX_NIGHTLY_59_END FIREFOX_BETA_60_BASE: 2.1s -> 2.3s > FIREFOX_BETA_59_END FIREFOX_BETA_60_BASE: 3.1s -> 3.3s > FIREFOX_AURORA_50_BASE FIREFOX_BETA_60_BASE: 30s -> 35s > > So it's measurably slower in most cases. Note that merge copies are > not calculated when updating with a clean working copy, which is > probably the most common case. I therefore think the much simpler code > is worth the slowdown. Do you know where the slowdown comes from ? > REPOSITORY > https://phab.mercurial-scm.org/diffusion/HG/ Mercurial > > REVISION DETAIL > https://phab.mercurial-scm.org/D6255 > > AFFECTED FILES > mercurial/copies.py > tests/test-annotate.t > tests/test-fastannotate-hg.t > tests/test-graft.t > tests/test-rename-merge2.t > > CHANGE DETAILS > > diff --git a/tests/test-rename-merge2.t b/tests/test-rename-merge2.t > > - a/tests/test-rename-merge2.t +++ b/tests/test-rename-merge2.t @@ -433,6 +433,9 @@ > > -------------- > test L:nc a b R:up b W: - 12 merge b no ancestor > -------------- > + all copies found (* = to merge, ! = divergent, % = renamed and deleted): > + src: 'a' -> dst: 'b' > + checking for directory renames > resolving manifests > branchmerge: True, force: False, partial: False > ancestor: 924404dff337, local: 86a2aa42fc76+, remote: af30c7647fc7 > @@ -469,6 +472,9 @@ > -------------- > test L:up b R:nm a b W: - 13 merge b no ancestor > -------------- > + all copies found (* = to merge, ! = divergent, % = renamed and deleted): > + src: 'a' -> dst: 'b' > + checking for directory renames > resolving manifests > branchmerge: True, force: False, partial: False > ancestor: 924404dff337, local: 59318016310c+, remote: bdb19105162a > @@ -506,6 +512,9 @@ > -------------- > test L:nc a b R:up a b W: - 14 merge b no ancestor > -------------- > + all copies found (* = to merge, ! = divergent, % = renamed and deleted): > + src: 'a' -> dst: 'b' > + checking for directory renames > resolving manifests > branchmerge: True, force: False, partial: False > ancestor: 924404dff337, local: 86a2aa42fc76+, remote: 8dbce441892a > @@ -543,6 +552,9 @@ > -------------- > test L:up b R:nm a b W: - 15 merge b no ancestor, remove a > -------------- > + all copies found (* = to merge, ! = divergent, % = renamed and deleted): > + src: 'a' -> dst: 'b' > + checking for directory renames > resolving manifests > branchmerge: True, force: False, partial: False > ancestor: 924404dff337, local: 59318016310c+, remote: bdb19105162a > @@ -580,6 +592,9 @@ > -------------- > test L:nc a b R:up a b W: - 16 get a, merge b no ancestor > -------------- > + all copies found (* = to merge, ! = divergent, % = renamed and deleted): > + src: 'a' -> dst: 'b' > + checking for directory renames > resolving manifests > branchmerge: True, force: False, partial: False > ancestor: 924404dff337, local: 86a2aa42fc76+, remote: 8dbce441892a > @@ -617,6 +632,9 @@ > -------------- > test L:up a b R:nc a b W: - 17 keep a, merge b no ancestor > -------------- > + all copies found (* = to merge, ! = divergent, % = renamed and deleted): > + src: 'a' -> dst: 'b' > + checking for directory renames > resolving manifests > branchmerge: True, force: False, partial: False > ancestor: 924404dff337, local: 0b76e65c8289+, remote: 4ce40f5aca24 > @@ -653,6 +671,9 @@ > -------------- > test L:nm a b R:up a b W: - 18 merge b no ancestor > -------------- > + all copies found (* = to merge, ! = divergent, % = renamed and deleted): > + src: 'a' -> dst: 'b' > + checking for directory renames > resolving manifests > branchmerge: True, force: False, partial: False > ancestor: 924404dff337, local: 02963e448370+, remote: 8dbce441892a > @@ -695,6 +716,9 @@ > -------------- > test L:up a b R:nm a b W: - 19 merge b no ancestor, prompt remove a > -------------- > + all copies found (* = to merge, ! = divergent, % = renamed and deleted): > + src: 'a' -> dst: 'b' > + checking for directory renames > resolving manifests > branchmerge: True, force: False, partial: False > ancestor: 924404dff337, local: 0b76e65c8289+, remote: bdb19105162a > diff --git a/tests/test-graft.t b/tests/test-graft.t > > - a/tests/test-graft.t +++ b/tests/test-graft.t @@ -75,6 +75,8 @@ > > > $ hg graft -r 2 --base 3 > grafting 2:5c095ad7e90f "2" > + note: possible conflict - c was deleted and renamed to: > + a > note: graft of 2:5c095ad7e90f created no changes to commit > > Can't continue without starting: > @@ -220,6 +222,9 @@ > committing changelog > updating the branch cache > grafting 5:97f8bfe72746 "5" > + all copies found (* = to merge, ! = divergent, % = renamed and deleted): > + src: 'c' -> dst: 'b' > + checking for directory renames > resolving manifests > branchmerge: True, force: True, partial: False > ancestor: 4c60f11aa304, local: 6b9e5368ca4e+, remote: 97f8bfe72746 > @@ -233,6 +238,9 @@ > $ HGEDITOR=cat hg graft 4 3 --log --debug > scanning for duplicate grafts > grafting 4:9c233e8e184d "4" > + all copies found (* = to merge, ! = divergent, % = renamed and deleted): > + src: 'c' -> dst: 'b' > + checking for directory renames > resolving manifests > branchmerge: True, force: True, partial: False > ancestor: 4c60f11aa304, local: 1905859650ec+, remote: 9c233e8e184d > @@ -1129,7 +1137,6 @@ > grafting 2:f58c7e2b28fa "C0" > merging f1e and f1b to f1e > merging f2a and f2c to f2c > - merging f5b and f5a to f5a > > Test the cases A.1 (f4x) and A.7 (f3x). > > diff --git a/tests/test-fastannotate-hg.t b/tests/test-fastannotate-hg.t > > - a/tests/test-fastannotate-hg.t +++ b/tests/test-fastannotate-hg.t @@ -273,37 +273,10 @@ > > > EOF > $ hg ci -mc -d '3 0' > created new head > -BROKEN: 'a' was copied to 'b' on both sides. We should not get a merge conflict here > $ hg merge > merging b > - warning: conflicts while merging b! (edit, then use 'hg resolve --mark') > - 0 files updated, 0 files merged, 0 files removed, 1 files unresolved > - use 'hg resolve' to retry unresolved file merges or 'hg merge --abort' to abandon > - [1] > - $ cat b > - <<<<<<< working copy: b80e3e32f75a - test: c > - a > - z > - a > - ||||||| base > - ======= > - a > - a > - a > - b4 > - c > - b5 > - >>>>>>> merge rev: 64afcdf8e29e - test: mergeb > - $ cat <<EOF > b > - > a > - > z > - > a > - > b4 > - > c > - > b5 > - > EOF > - $ hg resolve --mark -q > - $ rm b.orig > + 0 files updated, 1 files merged, 0 files removed, 0 files unresolved > + (branch merge, don't forget to commit) > $ echo d >> b > $ hg ci -mmerge2 -d '4 0' > > diff --git a/tests/test-annotate.t b/tests/test-annotate.t > > - a/tests/test-annotate.t +++ b/tests/test-annotate.t @@ -273,37 +273,10 @@ > > > EOF > $ hg ci -mc -d '3 0' > created new head > -BROKEN: 'a' was copied to 'b' on both sides. We should not get a merge conflict here > $ hg merge > merging b > - warning: conflicts while merging b! (edit, then use 'hg resolve --mark') > - 0 files updated, 0 files merged, 0 files removed, 1 files unresolved > - use 'hg resolve' to retry unresolved file merges or 'hg merge --abort' to abandon > - [1] > - $ cat b > - <<<<<<< working copy: b80e3e32f75a - test: c > - a > - z > - a > - ||||||| base > - ======= > - a > - a > - a > - b4 > - c > - b5 > - >>>>>>> merge rev: 64afcdf8e29e - test: mergeb > - $ cat <<EOF > b > - > a > - > z > - > a > - > b4 > - > c > - > b5 > - > EOF > - $ hg resolve --mark -q > - $ rm b.orig > + 0 files updated, 1 files merged, 0 files removed, 0 files unresolved > + (branch merge, don't forget to commit) > $ echo d >> b > $ hg ci -mmerge2 -d '4 0' > > diff --git a/mercurial/copies.py b/mercurial/copies.py > > - a/mercurial/copies.py +++ b/mercurial/copies.py @@ -373,57 +373,6 @@ > > > return u1, u2 > > -def _makegetfctx(ctx): > - """return a 'getfctx' function suitable for _checkcopies usage > > - - We have to re-setup the function building 'filectx' for each - '_checkcopies' to ensure the linkrev adjustment is properly setup for - each. Linkrev adjustment is important to avoid bug in rename - detection. Moreover, having a proper '_ancestrycontext' setup ensures - the performance impact of this adjustment is kept limited. Without it, - each file could do a full dag traversal making the time complexity of - the operation explode (see issue4537). - - This function exists here mostly to limit the impact on stable. Feel - free to refactor on default. - """ - rev = ctx.rev() - repo = ctx._repo - ac = getattr(ctx, '_ancestrycontext', None) - if ac is None: - revs = [rev] - if rev is None: - revs = [p.rev() for p in ctx.parents()] - ac = repo.changelog.ancestors(revs, inclusive=True) - ctx._ancestrycontext = ac - def makectx(f, n): - if n in node.wdirfilenodeids: # in a working context? - if ctx.rev() is None: - return ctx.filectx(f) - return repo[None][f] - fctx = repo.filectx(f, fileid=n) - # setup only needed for filectx not create from a changectx - fctx._ancestrycontext = ac - fctx._descendantrev = rev - return fctx - return util.lrucachefunc(makectx) - -def _combinecopies(copyfrom, copyto, finalcopy, diverge, incompletediverge): - """combine partial copy paths""" - remainder = {} - for f in copyfrom: - if f in copyto: - finalcopy[copyto[f]] = copyfrom[f] - del copyto[f] - for f in incompletediverge: - assert f not in diverge - ic = incompletediverge[f] - if ic[0] in copyto: - diverge[f] = [copyto[ic[0]], ic[1]] - else: - remainder[f] = ic - return remainder - > > def mergecopies(repo, c1, c2, base): > """ > Finds moves and copies between context c1 and c2 that are relevant for > @@ -526,168 +475,93 @@ > This is pretty slow when a lot of changesets are involved but will track all > the copies. > """ > - # In certain scenarios (e.g. graft, update or rebase), base 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. Because our > - # caller may not know if the revision passed in lieu of the CA is a genuine > - # common ancestor or not without explicitly checking it, it's better to > - # determine that here. > - # > - # base.isancestorof(wc) is False, work around that > - _c1 = c1.p1() if c1.rev() is None else c1 > - _c2 = c2.p1() if c2.rev() is None else c2 > - # an endpoint is "dirty" if it isn't a descendant of the merge base > - # if we have a dirty endpoint, we need to trigger graft logic, and also > - # keep track of which endpoint is dirty > - dirtyc1 = not base.isancestorof(_c1) > - dirtyc2 = not base.isancestorof(_c2) > - graft = dirtyc1 or dirtyc2 > - tca = base > - if graft: > - tca = _c1.ancestor(_c2) > > - - limit = _findlimit(repo, c1, c2) - > > m1 = c1.manifest() > m2 = c2.manifest() > mb = base.manifest() > > - # gather data from _checkcopies: > - # - diverge = record all diverges in this dict > - # - copy = record all non-divergent copies in this dict > - # - fullcopy = record all copies in this dict > - # - incomplete = record non-divergent partial copies here > - # - incompletediverge = record divergent partial copies here > - diverge = {} # divergence data is shared > - incompletediverge = {} > - data1 = {'copy': {}, > - 'fullcopy': {}, > - 'incomplete': {}, > - 'diverge': diverge, > - 'incompletediverge': incompletediverge, > - } > - data2 = {'copy': {}, > - 'fullcopy': {}, > - 'incomplete': {}, > - 'diverge': diverge, > - 'incompletediverge': incompletediverge, > - } > + copies1 = pathcopies(base, c1) > + copies2 = pathcopies(base, c2) > + > + inversecopies1 = {} > + inversecopies2 = {} > + for dst, src in copies1.items(): > + inversecopies1.setdefault(src, []).append(dst) > + for dst, src in copies2.items(): > + inversecopies2.setdefault(src, []).append(dst) > + > + copy = {} > + diverge = {} > + renamedelete = {} > + allsources = set(inversecopies1) | set(inversecopies2) > + for src in allsources: > + dsts1 = inversecopies1.get(src) > + dsts2 = inversecopies2.get(src) > + if dsts1 and dsts2: > + # copied/renamed on both sides > + if src not in m1 and src not in m2: > + # renamed on both sides > + dsts1 = set(dsts1) > + dsts2 = set(dsts2) > + # If there's some overlap in the rename destinations, we > + # consider it not divergent. For example, if side 1 copies 'a' > + # to 'b' and 'c' and deletes 'a', and side 2 copies 'a' to 'c' > + # and 'd' and deletes 'a'. Formatting could help this comment a bit. something like side 1: a -> b -> c side 2: a -> c -> d > + if dsts1 & dsts2: > + for dst in (dsts1 & dsts2): > + copy[dst] = src > + else: > + diverge[src] = sorted(dsts1 | dsts2) > + elif src in m1 and src in m2: > + # copied on both sides > + dsts1 = set(dsts1) > + dsts2 = set(dsts2) > + for dst in (dsts1 & dsts2): > + copy[dst] = src > + # TODO: Handle cases where it was renamed on one side and copied > + # on the other side Is this TODO a regression or some ported limitation. > + elif dsts1: > + # copied/renamed only on side 1 > + if src not in m2: > + # deleted on side 2 > + if src not in m1: > + # renamed on side 1, deleted on side 2 > + renamedelete[src] = dsts1 > + elif m2[src] != mb[src]: > + # modified on side 2 > + for dst in dsts1: > + if dst not in m2: > + # dst not added on side 2 (handle as regular > + # "both created" case in manifestmerge in that case) Can you elaborate a bit on what this case means (and how we deal with it) especially, what happens if dst is indeed in m2 ? > + copy[dst] = src > + elif dsts2: > + # copied/renamed only on side 2 > + if src not in m1: > + # deleted on side 1 > + if src not in m2: > + # renamed on side 2, deleted on side 1 > + renamedelete[src] = dsts2 > + elif m1[src] != mb[src]: > + # modified on side 1 > + for dst in dsts2: > + if dst not in m1: > + # dst not added on side 1 (handle as regular > + # "both created" case in manifestmerge in that case) > + copy[dst] = src Since this seems the very same code as the previous clause, I wonder if we could factor it out. This would help to prevent subtle bug when we update it. (the answer might be "no because python is slow). > + renamedeleteset = set() > + divergeset = set() > + for src, dsts in diverge.items(): > + divergeset.update(dsts) > + for src, dsts in renamedelete.items(): > + renamedeleteset.update(dsts) small nits: Is there any reason not to use diverge.values and renamedelete.values here ? REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D6255 To: martinvonz, #hg-reviewers Cc: marmoute, mjpieters, mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
