[PATCH 2 of 2 stable] graft: fix graft across merges of duplicates of grafted changes

[Yuya Nishihara]

On Tue, 09 May 2017 00:45:02 +0200, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski at unity3d.com>
> # 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?