[PATCH 2 of 2 rfc] diff: search beyond ancestor when detecting renames

[Mads Kiilerich]

On 11/08/2013 03:49 AM, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski at unity3d.com>
> # Date 1383878972 -3600
> #      Fri Nov 08 03:49:32 2013 +0100
> # Branch stable
> # Node ID 79b83373ff5e66b61eaccafa3fc2bd7487e06476
> # Parent  d3df4d706b86fa1780f6156a033062a7f617cd4c
> diff: search beyond ancestor when detecting renames
>
> This removes an optimization that was introduced in 91eb4512edd0 but seems to
> be too aggressive - as indicated by how it changed test-mq-merge.t .
>
> Removing the optimization will in some cases give a different and arguably more
> correct result. It will probably also introduce a significant performance
> regression in other cases.
>
> diff --git a/mercurial/copies.py b/mercurial/copies.py
> --- a/mercurial/copies.py
> +++ b/mercurial/copies.py
> @@ -100,14 +100,11 @@
>   
>   def _tracefile(fctx, actx):
>       '''return file context that is the ancestor of fctx present in actx'''
> -    stop = actx.rev()
>       am = actx.manifest()
>   
>       for f in fctx.ancestors():
>           if am.get(f.path(), None) == f.filenode():
>               return f
> -        if f.rev() < stop:
> -            return None
>   
>   def _dirstatecopies(d):
>       ds = d._repo.dirstate

Something like this might be more efficient ... but I can't convince 
myself that it really is correct in all cases:

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -710,13 +710,14 @@

          return None

-    def ancestors(self, followfirst=False):
+    def ancestors(self, followfirst=False, stop=-1):
          visit = {}
          c = self
          cut = followfirst and 1 or None
          while True:
-            for parent in c.parents()[:cut]:
-                visit[(parent.rev(), parent.node())] = parent
+            if c.rev() >= stop:
+                for parent in c.parents()[:cut]:
+                    visit[(parent.rev(), parent.node())] = parent
              if not visit:
                  break
              c = visit.pop(max(visit))
diff --git a/mercurial/copies.py b/mercurial/copies.py
--- a/mercurial/copies.py
+++ b/mercurial/copies.py
@@ -103,11 +103,9 @@
      stop = actx.rev()
      am = actx.manifest()

-    for f in fctx.ancestors():
+    for f in fctx.ancestors(stop=stop):
          if am.get(f.path(), None) == f.filenode():
              return f
-        if f.rev() < stop:
-            return None

  def _dirstatecopies(d):
      ds = d._repo.dirstate

> diff --git a/tests/test-copied-diff.t b/tests/test-copied-diff.t
> --- a/tests/test-copied-diff.t
> +++ b/tests/test-copied-diff.t
> @@ -49,25 +49,16 @@
>     |/
>     o  f=0
>     
> -Diff from "other" to tip do not see the rename:
> +Diff from "other" to tip do see the rename:
>   
>     $ hg diff --git -r 2 -r tip
> -  diff --git a/f b/f
> -  deleted file mode 100644
> -  --- a/f
> -  +++ /dev/null
> -  @@ -1,1 +0,0 @@
> -  -0
> -  diff --git a/g b/g
> -  new file mode 100644
> -  --- /dev/null
> -  +++ b/g
> -  @@ -0,0 +1,1 @@
> -  +0
> +  diff --git a/f b/g
> +  rename from f
> +  rename to g
>   
>     $ cd ..
>   
> -Diff from "other" to diff with another topological ordering do see the rename:
> +Diff from "other" to diff with another topological ordering also see the rename:
>   
>     $ hg clone -q -U repo1 repo2 -r 5
>     $ cd repo2
> diff --git a/tests/test-mq-merge.t b/tests/test-mq-merge.t
> --- a/tests/test-mq-merge.t
> +++ b/tests/test-mq-merge.t
> @@ -147,11 +147,13 @@
>     -b
>     +a
>     +c
> -  diff --git a/aa b/aa
> -  new file mode 100644
> -  --- /dev/null
> +  diff --git a/a b/aa
> +  copy from a
> +  copy to aa
> +  --- a/a
>     +++ b/aa
> -  @@ -0,0 +1,1 @@
> +  @@ -1,1 +1,1 @@
> +  -b
>     +a
>   
>   Check patcha2 is still a regular patch:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20131108/424140ff/attachment-0002.html>