Patchwork [2,of,2,stable,v2] graft: fix graft across merges of duplicates of grafted changes

login
register
mail settings
Submitter Mads Kiilerich
Date May 11, 2017, 3:20 p.m.
Message ID <cf6482170698ea9cd590.1494516035@madski>
Download mbox | patch
Permalink /patch/20571/
State Accepted
Headers show

Comments

Mads Kiilerich - May 11, 2017, 3:20 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1494515920 -7200
#      Thu May 11 17:18:40 2017 +0200
# Branch stable
# Node ID cf6482170698ea9cd590f6e72745348bef5703b4
# Parent  6710017995b4e8b361d6ad5b897ff7d0cc658285
graft: fix graft across merges of duplicates of grafted changes

Graft used findmissingrevs to find the candidates for graft duplicates in the
destination. That function operates with the constraint:

  1. N is an ancestor of some node in 'heads'
  2. N is not an ancestor of any node in 'common'

For our purpose, we do however have to work correctly in cases where the graft
set has multiple roots or where merges between graft ranges are skipped. The
only changesets we can be sure doesn't have ancestors that are grafts of any
changeset in the graftset, are the ones that are common ancestors of *all*
changesets in the graftset. We thus need:

  2. N is not an ancestor of all nodes in 'common'

This change will graft more correctly, but it will also in some cases make
graft slower by making it search through a bigger and unnecessary large sets of
changes to find duplicates. In the general case of grafting individual or
linear sets, we do the same amount of work as before.
Yuya Nishihara - May 12, 2017, 2:20 p.m.
On Thu, 11 May 2017 17:20:35 +0200, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1494515920 -7200
> #      Thu May 11 17:18:40 2017 +0200
> # Branch stable
> # Node ID cf6482170698ea9cd590f6e72745348bef5703b4
> # Parent  6710017995b4e8b361d6ad5b897ff7d0cc658285
> graft: fix graft across merges of duplicates of grafted changes

> -        for rev in repo.changelog.findmissingrevs(revs, [crev]):
> +        # The only changesets we can be sure doesn't contain grafts of any
> +        # revs, are the ones that are common ancestors of *all* revs:
> +        for rev in repo.revs('only(%d,ancestor(%ld))', crev, revs):

Makes sense per your investigation in the previous version.
Queued these, thanks.

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -2295,7 +2295,9 @@  def _dograft(ui, repo, *revs, **opts):
         # check ancestors for earlier grafts
         ui.debug('scanning for duplicate grafts\n')
 
-        for rev in repo.changelog.findmissingrevs(revs, [crev]):
+        # The only changesets we can be sure doesn't contain grafts of any
+        # revs, are the ones that are common ancestors of *all* revs:
+        for rev in repo.revs('only(%d,ancestor(%ld))', crev, revs):
             ctx = repo[rev]
             n = ctx.extra().get('source')
             if n in ids:
diff --git a/tests/test-graft.t b/tests/test-graft.t
--- a/tests/test-graft.t
+++ b/tests/test-graft.t
@@ -1341,16 +1341,15 @@  Grafting of plain changes correctly dete
   skipping already grafted revision 5:43e9eb70dab0 (was grafted from 4:6c9a1289e5f1)
   grafting 2:42127f193bcd "b"
 
-Extending the graft range to include a merge will unfortunately make us miss
-that 3 and 5 should be skipped:
+Extending the graft range to include a (skipped) merge of 3 will not prevent us from
+also detecting that both 3 and 5 should be skipped:
 
   $ hg up -qCr 4
   $ hg graft --tool :local -r 2::7
   skipping ungraftable merge revision 6
+  skipping already grafted revision 3:ca093ca2f1d9 (was grafted from 1:13ec5badbf2a)
   skipping already grafted revision 5:43e9eb70dab0 (was grafted from 4:6c9a1289e5f1)
   grafting 2:42127f193bcd "b"
-  grafting 3:ca093ca2f1d9 "x"
-  note: graft of 3:ca093ca2f1d9 created no changes to commit
   grafting 7:d3c3f2b38ecc "xx"
   note: graft of 7:d3c3f2b38ecc created no changes to commit