Patchwork D8237: copies-tests: update the analysis of the BD/DB cases

login
register
mail settings
Submitter phabricator
Date March 5, 2020, 6:14 p.m.
Message ID <differential-rev-PHID-DREV-yakkzpcwmh36xuf5s45o-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/45520/
State Superseded
Headers show

Comments

phabricator - March 5, 2020, 6:14 p.m.
marmoute created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  After thinking more about it, the current change in output between the two
  merges is not due to an ambiguity, it is introduce by a bug in merge/commit
  that record a wrong file history.
  
  Fixing the bug should fix the output. In the meantime, we have a wrong behavior.
  
  In practice the bug is not in the merge code (who does the right things), but in
  the commit code (who get confused when recording the new file revision).

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D8237

AFFECTED FILES
  tests/test-copies-chain-merge.t

CHANGE DETAILS




To: marmoute, #hg-reviewers
Cc: mercurial-devel
phabricator - March 6, 2020, 8:05 a.m.
This revision now requires changes to proceed.
martinvonz added a comment.
martinvonz requested changes to this revision.


  I'm sending a bunch of comments on D8078 <https://phab.mercurial-scm.org/D8078>. I'm hoping some of them can incorporated in this series.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8237/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8237

To: marmoute, #hg-reviewers, martinvonz
Cc: mercurial-devel
phabricator - March 6, 2020, 4:07 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> test-copies-chain-merge.t:297
>  Note:
> -| In this case, the merge get conflicting information since each side have a
> -| different way to reach 'f'.
> +| In this case, on of the merge record bad information and fails to see that
> +| there is no merge going on.

"*one* of"

Also, shouldn't this exactly duplicate the comment on lines 456-457?

> test-copies-chain-merge.t:521
>    A d
> -    a
> +    a (true !)
>    R a

why change this?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8237/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8237

To: marmoute, #hg-reviewers, martinvonz
Cc: mercurial-devel
phabricator - March 6, 2020, 4:20 p.m.
marmoute added inline comments.

INLINE COMMENTS

> martinvonz wrote in test-copies-chain-merge.t:521
> why change this?

I wanted to highlight the expected change when the fix arrive. We can drop it if this confuse you.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8237/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8237

To: marmoute, #hg-reviewers, martinvonz
Cc: mercurial-devel
phabricator - March 6, 2020, 4:28 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> marmoute wrote in test-copies-chain-merge.t:521
> I wanted to highlight the expected change when the fix arrive. We can drop it if this confuse you.

Yes, I find it confusing (especially with the "This output is correct" comment above), so please remove.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8237/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D8237

To: marmoute, #hg-reviewers, martinvonz
Cc: mercurial-devel

Patch

diff --git a/tests/test-copies-chain-merge.t b/tests/test-copies-chain-merge.t
--- a/tests/test-copies-chain-merge.t
+++ b/tests/test-copies-chain-merge.t
@@ -195,7 +195,7 @@ 
 
 Merge:
 - one with change to an unrelated file
-- one deleting and recreating the change
+- one deleting and recreating the file
 
 Note:
 | In this case, the merge get conflicting information since on one side we have
@@ -294,8 +294,8 @@ 
   
 
 Note:
-| In this case, the merge get conflicting information since each side have a
-| different way to reach 'f'.
+| In this case, on of the merge record bad information and fails to see that
+| there is no merge going on.
 
 final summary
 
@@ -453,11 +453,8 @@ 
 - one deleting and recreating the change
 
 Note:
-| In this case, the merge get conflicting information since on one side we have
-| a "brand new" d. and one the other one we have "d renamed from c (itself
-| renamed from c)".
-|
-| The current code arbitrarily pick one side
+| In this case, one of the merge wrongly record a merge while there is none.
+| This lead to bad copy tracing information to be dug up.
 
   $ hg status --copies --rev 'desc("b-1")' --rev 'desc("mBDm-0")'
   M d
@@ -475,14 +472,18 @@ 
   M b
   M d
 
-The recorded copy is different depending of where we started the merge from since
+The bugs makes recorded copy is different depending of where we started the merge from since
 
   $ hg manifest --debug --rev 'desc("mBDm-0")' | grep '644   d'
   0bb5445dc4d02f4e0d86cf16f9f3a411d0f17744 644   d
   $ hg manifest --debug --rev 'desc("mDBm-0")' | grep '644   d'
   b004912a8510032a0350a74daa2803dadfb00e12 644   d
 
-This second b004912a8510032a0350a74daa2803dadfb00e12 seems wrong. We should record the merge
+The 0bb5445dc4d02f4e0d86cf16f9f3a411d0f17744 entry is wrong, since the file was
+deleted on one side (then recreate) and untouched on the other side, no "merge"
+has happened. The resulting `d` file is the untouched version from branch `D`,
+not a merge.
+
   $ hg manifest --debug --rev 'desc("d-2")' | grep '644   d'
   b004912a8510032a0350a74daa2803dadfb00e12 644   d
   $ hg manifest --debug --rev 'desc("b-1")' | grep '644   d'
@@ -493,6 +494,8 @@ 
        1      10 b004912a8510 000000000000 000000000000
        2      15 0bb5445dc4d0 01c2f5eabdc4 b004912a8510
 
+(That output if wrong, since no merge actually happened).
+
   $ hg log -Gfr 'desc("mBDm-0")' d
   o    15 mBDm-0 simple merge - one way]
   |\
@@ -505,8 +508,7 @@ 
   o  0 i-0 initial commit: a b]
   
 
-(That output seems wrong, if we had opportunity to record the merge, we should
-probably have recorded the merge).
+This output is correct
 
   $ hg log -Gfr 'desc("mDBm-0")' d
   o  14 d-2 re-add d]
@@ -516,7 +518,7 @@ 
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mBDm-0")'
   M b
   A d
-    a
+    a (true !)
   R a
   $ hg status --copies --rev 'desc("i-0")' --rev 'desc("mDBm-0")'
   M b