Re: [PATCH 2 of 2 stable] graft: fix graft across merges of duplicates of grafted changes
On 05/10/2017 03:42 PM, Yuya Nishihara wrote: On Tue, 09 May 2017 00:45:02 +0200, Mads Kiilerich wrote: # HG changeset patch # User Mads Kiilerich# Date 1494283376 -7200 # Tue May 09 00:42:56 2017 +0200 # Branch stable # Node ID dd256de590dfd363fa5d497d566d456f471b8d52 # Parent 6710017995b4e8b361d6ad5b897ff7d0cc658285 graft: fix graft across merges of duplicates of grafted changes Looks good to me. A couple of nits inline. Graft used findmissingrevs to find the candidates for graft duplicates in the destination. That function operates with the constraint: 2. N is not an ancestor of any node in 'common' For our purpose, we do however need: 2. There are nodes in 'common' that doesn't have N as ancestor The right candiates for graft duplicates could be computed with a revset: only(destination,transitiveroots(graftset)) I guess it actually can be computed as only(destination,roots(graftset+roots(graftset)::heads(graftset))) BUT I realize it also is wrong. There could be criss-cross-ish cases where multiple graftset roots have been merged to different branches that have grafts of other roots as ancestor. My proposed patch using min(graftset) would also fail that. Instead, the only changesets we can be sure doesn't contain grafts of any changeset in the graftset, are the ones that are common ancestors of *all* changesets in the graftset: only(destination,ancestor(graftset)) It will exclude one ancestor. In criss-cross cases where there will be more ancestors, it might be inefficient but still correct. Resending ... /Mads where transitiveroots would be a revset function for 'transitive root' and return changesets in set with no ancestor changeset in set. There doesn't seem to be any such function readily available, and we thus use the approximation of just using the smallest revision in the graft set. Can you copy this message as a code comment? It will help future readers. 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. Suppose revisions to be grafted are linear in general, I think this is acceptable. @@ -2295,7 +2295,8 @@ 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]): +expr = revsetlang.formatspec('only(%d,min(%ld))', crev, revs) +for rev in scmutil.revrange(repo, [expr]): scmutil.revrange() may expand user aliases. Please use repo.revs() instead. Alternatively, maybe we could use findmissingrevs(min(revs), ...) to minimize the change? ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Re: [PATCH 2 of 2 stable] graft: fix graft across merges of duplicates of grafted changes
On Tue, 09 May 2017 00:45:02 +0200, Mads Kiilerich wrote: > # HG changeset patch > # User Mads Kiilerich> # Date 1494283376 -7200 > # Tue May 09 00:42:56 2017 +0200 > # Branch stable > # Node ID dd256de590dfd363fa5d497d566d456f471b8d52 > # Parent 6710017995b4e8b361d6ad5b897ff7d0cc658285 > graft: fix graft across merges of duplicates of grafted changes Looks good to me. A couple of nits inline. > Graft used findmissingrevs to find the candidates for graft duplicates in the > destination. That function operates with the constraint: > > 2. N is not an ancestor of any node in 'common' > > For our purpose, we do however need: > > 2. There are nodes in 'common' that doesn't have N as ancestor > > The right candiates for graft duplicates could be computed with a revset: > > only(destination,transitiveroots(graftset)) > > where transitiveroots would be a revset function for 'transitive root' and > return changesets in set with no ancestor changeset in set. There doesn't seem > to be any such function readily available, and we thus use the approximation > of > just using the smallest revision in the graft set. Can you copy this message as a code comment? It will help future readers. > 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. Suppose revisions to be grafted are linear in general, I think this is acceptable. > @@ -2295,7 +2295,8 @@ 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]): > +expr = revsetlang.formatspec('only(%d,min(%ld))', crev, revs) > +for rev in scmutil.revrange(repo, [expr]): scmutil.revrange() may expand user aliases. Please use repo.revs() instead. Alternatively, maybe we could use findmissingrevs(min(revs), ...) to minimize the change? ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
[PATCH 2 of 2 stable] graft: fix graft across merges of duplicates of grafted changes
# HG changeset patch # User Mads Kiilerich# Date 1494283376 -7200 # Tue May 09 00:42:56 2017 +0200 # Branch stable # Node ID dd256de590dfd363fa5d497d566d456f471b8d52 # 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: 2. N is not an ancestor of any node in 'common' For our purpose, we do however need: 2. There are nodes in 'common' that doesn't have N as ancestor The right candiates for graft duplicates could be computed with a revset: only(destination,transitiveroots(graftset)) where transitiveroots would be a revset function for 'transitive root' and return changesets in set with no ancestor changeset in set. There doesn't seem to be any such function readily available, and we thus use the approximation of just using the smallest revision in the graft set. 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. diff --git a/mercurial/commands.py b/mercurial/commands.py --- a/mercurial/commands.py +++ b/mercurial/commands.py @@ -2295,7 +2295,8 @@ 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]): +expr = revsetlang.formatspec('only(%d,min(%ld))', crev, revs) +for rev in scmutil.revrange(repo, [expr]): 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 ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel