Patchwork [v2] graft: support grafting across move/copy (issue4028)

login
register
mail settings
Submitter Gábor Stefanik
Date Aug. 2, 2016, 9:27 a.m.
Message ID <f5220d4f9655dade7285.1470130050@waste.org>
Download mbox | patch
Permalink /patch/16031/
State Changes Requested
Headers show

Comments

Gábor Stefanik - Aug. 2, 2016, 9:27 a.m.
# HG changeset patch
# User Gábor Stefanik <gabor.stefanik@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)

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
Pierre-Yves David - Aug. 2, 2016, 11:21 a.m.
On 08/02/2016 11:27 AM, Gábor Stefanik wrote:
> # HG changeset patch
> # User Gábor Stefanik <gabor.stefanik@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.

> […]
Gábor Stefanik - Aug. 2, 2016, 2:02 p.m.
Hi,

>



--------------------------------------------------------------------------
This message, including its attachments, is confidential. For more information please read NNG's email policy here:
http://www.nng.com/emailpolicy/
By responding to this email you accept the email policy.


-----Original Message-----
> From: Pierre-Yves David [mailto:pierre-yves.david@ens-lyon.org]

> Sent: Tuesday, August 02, 2016 1:21 PM

> To: Gábor STEFANIK <Gabor.STEFANIK@nng.com>; mercurial-

> devel@mercurial-scm.org

> Subject: Re: [PATCH v2] graft: support grafting across move/copy (issue4028)

>

>

>

> On 08/02/2016 11:27 AM, Gábor Stefanik wrote:

> > # HG changeset patch

> > # User Gábor Stefanik <gabor.stefanik@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.


I will try to sort out the API breakages.

I'm afraid I will have to send a single patch, because I'm using pushgate which AFAIK doesn't support multipart patch series.
(I'm developing on Windows, so no mutt or pine to send whitespace-undamaged patches in e-mail bodies, and no smtp server for patchbomb.)
Also, I'm not using mq, and with the talk about it being obsolete and last-resort, I would rather not start using it now.

>

>

> > 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'


Calculateupdates also uses "ancestors" instead of "pas", which is why I exceptionally used "topo_ancestor" instead of "pta" or "cta" there.

>

> > +                             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?


We are now calling this function twice when grafting, once for the true common ancestor, and again for the fake one.
It would be confusing to print the unmatched file list on both passes.
Since previously we only called it for the fake CA, leave that pass as the one that prints.

Alternative would be to move the debug printouts out of _computenonoverlap into mergecopies, but that would
make the if/else branching in copies.py much more complicated.

>

> > @@ -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.


While we don't currently have non-linear updates exposed on the UI, it's present in the code (although marred by this bug), and extensions are free to call it - in fact, graft is already doing so.

>

> > +    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?


That would require defining two large memory-wasting temporary arrays.

>

> >      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.


Will do.

Please note that these are dummy variables for the 2nd set of checkcopies calls, the real ones are a few lines above.

>

> >      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.


Will do. However, it will require pa and pta to be non-adjacent, which other reviewers may find unacceptable.

>

> >      """

> >      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.


Will fix.

>

> > […]

>

>

>

> --

> Pierre-Yves David
Sean Farley - Aug. 2, 2016, 8:58 p.m.
Kevin Bullock <kbullock+mercurial@ringworld.org> writes:

>> On Aug 2, 2016, at 09:02, Gábor Stefanik <gabor.stefanik@nng.com> wrote:
>> 
>> I'm afraid I will have to send a single patch, because I'm using pushgate which AFAIK doesn't support multipart patch series.
>
> I'd be awfully surprised if it didn't support multi-patch series.

It definitely supports multi-patch series.
Gábor Stefanik - Aug. 3, 2016, 9:52 a.m.
>



--------------------------------------------------------------------------
This message, including its attachments, is confidential. For more information please read NNG's email policy here:
http://www.nng.com/emailpolicy/
By responding to this email you accept the email policy.


-----Original Message-----
> From: Sean Farley [mailto:sean@farley.io]

> Sent: Tuesday, August 02, 2016 10:59 PM

> To: Kevin Bullock <kbullock+mercurial@ringworld.org>; Gábor STEFANIK

> <Gabor.STEFANIK@nng.com>

> Cc: Pierre-Yves David <pierre-yves.david@ens-lyon.org>; mercurial-

> devel@mercurial-scm.org

> Subject: Re: [PATCH v2] graft: support grafting across move/copy (issue4028)

>

> Kevin Bullock <kbullock+mercurial@ringworld.org> writes:

>

> >> On Aug 2, 2016, at 09:02, Gábor Stefanik <gabor.stefanik@nng.com>

> wrote:

> >>

> >> I'm afraid I will have to send a single patch, because I'm using pushgate

> which AFAIK doesn't support multipart patch series.

> >

> > I'd be awfully surprised if it didn't support multi-patch series.

>

> It definitely supports multi-patch series.


In that case, the wiki needs to be updated, as it currently says only [PATCH v2] and [PATCH stable] are supported, not [PATCH x of y}.

Patch

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,
+                             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
@@ -285,7 +286,7 @@ 
         return fctx
     return util.lrucachefunc(makectx)
 
-def mergecopies(repo, c1, c2, ca):
+def mergecopies(repo, c1, c2, ca, cta=None):
     """
     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
+    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
+    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))
+
     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 = {}, {}, {}, {}, {}
     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):
     """
     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,
+                     force, acceptremote, followcopies, matcher=None,
                      mergeforce=False):
     "Calculate the actions needed to merge mctx into wctx using ancestors"
     if len(ancestors) == 1: # default
         actions, diverge, renamedelete = manifestmerge(
-            repo, wctx, mctx, ancestors[0], branchmerge, force, matcher,
-            acceptremote, followcopies)
+            repo, wctx, mctx, ancestors[0], topo_ancestor,
+            branchmerge, force, matcher, acceptremote, followcopies)
         _checkunknownfiles(repo, wctx, mctx, force, actions, mergeforce)
 
     else: # only when merge.preferancestor=* - the default
@@ -958,8 +959,8 @@ 
         for ancestor in ancestors:
             repo.ui.note(_('\ncalculating bids for ancestor %s\n') % ancestor)
             actions, diverge1, renamedelete1 = manifestmerge(
-                repo, wctx, mctx, ancestor, branchmerge, force, matcher,
-                acceptremote, followcopies)
+                repo, wctx, mctx, ancestor, ancestor, branchmerge, force,
+                matcher, acceptremote, followcopies)
             _checkunknownfiles(repo, wctx, mctx, force, actions, mergeforce)
 
             # Track the shortest set of warning on the theory that bid
@@ -1455,6 +1456,13 @@ 
                 pas = [repo[anc] for anc in (sorted(cahs) or [nullid])]
             else:
                 pas = [p1.ancestor(p2, warn=branchmerge)]
+            pta = pas[0]
+        elif not (pas[0].descendant(p1) and pas[0].descendant(p2)):
+            # this can happen during graft or non-linear update
+            # in this case, we still need a true topological common ancestor
+            pta = p1.ancestor(p2)
+        else:
+            pta = pas[0]
 
         fp1, fp2, xp1, xp2 = p1.node(), p2.node(), str(p1), str(p2)
 
@@ -1526,7 +1534,7 @@ 
 
         ### calculate phase
         actionbyfile, diverge, renamedelete = calculateupdates(
-            repo, wc, p2, pas, branchmerge, force, mergeancestor,
+            repo, wc, p2, pas, pta, branchmerge, force, mergeancestor,
             followcopies, matcher=matcher, mergeforce=mergeforce)
 
         # Prompt and create actions. Most of this is in the resolve phase
diff --git a/tests/test-graft.t b/tests/test-graft.t
--- a/tests/test-graft.t
+++ b/tests/test-graft.t
@@ -179,6 +179,11 @@ 
   committing changelog
   grafting 5:97f8bfe72746 "5"
     searching for copies back to rev 1
+    unmatched files new in both:
+     b
+    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
@@ -193,6 +198,11 @@ 
   scanning for duplicate grafts
   grafting 4:9c233e8e184d "4"
     searching for copies back to rev 1
+    unmatched files new in both:
+     b
+    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
@@ -842,3 +852,24 @@ 
   |/
   o  0
   
+Graft from behind a move or rename
+
+  $ hg init graftmove
+  $ cd graftmove
+  $ echo c1 > f1
+  $ hg ci -qAm 0
+  $ hg mv f1 f2
+  $ hg ci -qAm 1
+  $ echo c2 > f2
+  $ hg ci -qAm 2
+  $ hg up -q 0
+  $ hg graft -r 2
+  grafting 2:8a20493ece2a "2" (tip)
+  merging f1 and f2 to f1
+  $ hg status --change .
+  M f1
+  $ hg cat f1
+  c2
+  $ hg cat f2
+  f2: no such file in rev ee1f64f8088b
+  [1]