Patchwork tests: test coverage of tricky case where diffs show wrong copy information

login
register
mail settings
Submitter Mads Kiilerich
Date Dec. 10, 2014, 4:15 p.m.
Message ID <62364c6c6e7ed064acc3.1418228126@ssl.google-analytics.com>
Download mbox | patch
Permalink /patch/7042/
State Superseded
Headers show

Comments

Mads Kiilerich - Dec. 10, 2014, 4:15 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1418228036 -3600
#      Wed Dec 10 17:13:56 2014 +0100
# Node ID 62364c6c6e7ed064acc3f2ee2a6e3fcad75201ef
# Parent  3575f42e1b7b174f46f1c69951526d410452abf4
tests: test coverage of tricky case where diffs show wrong copy information

Soundtrack: No, no, no, no, no, no, no, no, no, no, no, no, there's no limit!

This shows pretty much the same issue as addressed in 652ab726ba93, indicating
that the fix there is insufficient. We can blame that on linkrev and how it is
used to calculate the limit.

I don't see an easy fix that also has perfect performance characteristics. I
think the only solution right now is to replace the tweak in 243ea5ffdf31 with
not having a limit at all, searching through the whole filelog all the way to
nullrev. The search is only done for added files and it is usually not that
much more expensive to iterate over a whole filelog instead of just the top of
it.

The clamp added in 652ab726ba93 worked around some real issues. We can take it
to the next level by using a limit that is lower than the one we currently
calculate. It could be the lowest parent of any single sided ancestor
(min(parents(only(a,b)+only(b,a)))) or the highest ancestor of all single sided
ancestors (max(ancestor(roots(only(a,b)+only(b,a))))?). I guess both of them
still would have some value as limits and would make it work correctly in more
cases (including the example given here and the real-world example behind it)
but they would not be perfect and I don't know which of them would be
preferable.

Patch

diff --git a/tests/test-mv-cp-st-diff.t b/tests/test-mv-cp-st-diff.t
--- a/tests/test-mv-cp-st-diff.t
+++ b/tests/test-mv-cp-st-diff.t
@@ -1605,3 +1605,72 @@ 
   -
   +f
   $ cd ..
+
+another test showing that we still don't look sufficiently far back:
+
+  $ hg init diffstoplinkrev
+  $ cd diffstoplinkrev
+
+  $ touch f
+  $ hg ci -Aqm 'empty f'
+
+  $ echo change > f
+  $ hg ci -m 'change f'
+  $ hg tag -l change
+
+  $ hg up -qr 0
+
+  $ hg branch -q dev
+  $ hg ci -Aqm dev
+  $ hg tag -l dev-start
+
+  $ hg mv f renamed
+  $ hg ci -m renamed
+  $ hg tag -l renamed
+
+  $ hg up -qr dev-start
+
+  $ hg graft -q change
+
+  $ hg merge -q renamed
+  $ hg ci -m 'merge'
+
+  $ hg log -G -T '{rev} {desc}'
+  @    5 merge
+  |\
+  | o  4 change f
+  | |
+  o |  3 renamed
+  |/
+  o  2 dev
+  |
+  | o  1 change f
+  |/
+  o  0 empty f
+  
+  $ hg diff --git -r dev-start -r dev
+  diff --git a/f b/f
+  deleted file mode 100644
+  diff --git a/renamed b/renamed
+  new file mode 100644
+  --- /dev/null
+  +++ b/renamed
+  @@ -0,0 +1,1 @@
+  +change
+
+where a diff that doesn't notice the rename is "unfortunate" ...
+
+diff works correctly with different toposort / linkrev aliasing:
+
+  $ hg --config extensions.strip= strip -r change
+  saved backup bundle to $TESTTMP/diffstoplinkrev/.hg/strip-backup/3a7ff4680a1c-backup.hg (glob)
+  $ hg diff --git -r dev-start -r dev
+  diff --git a/f b/renamed
+  rename from f
+  rename to renamed
+  --- a/f
+  +++ b/renamed
+  @@ -0,0 +1,1 @@
+  +change
+
+  $ cd ..