D340: rebase: prefer choosing merge base with successor in destination

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D340: rebase: prefer choosing merge base with successor in destination

dsp (David Soria Parra)
quark created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  As demonstrated by the test case change, and the new comment. Choosing a
  merge base candidate with a successor in destination would allow us to avoid
  re-introducing obsoleted changes and is therefore considered better.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/rebase.py
  tests/test-rebase-obsolete.t

CHANGE DETAILS

diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
--- a/tests/test-rebase-obsolete.t
+++ b/tests/test-rebase-obsolete.t
@@ -1069,9 +1069,8 @@
   rebasing 2:b18e25de2cf5 "D" (D)
   note: not rebasing 3:7fb047a69f22 "E" (E), already in destination as 1:112478962961 "B"
   rebasing 5:66f1a38021c9 "F" (F tip)
+  note: rebase of 5:66f1a38021c9 created no changes to commit
   $ hg log -G
-  o  7:9ed45af61fa0 F
-  |
   o  6:8f47515dda15 D
   |
   | x    5:66f1a38021c9 F
@@ -1105,12 +1104,11 @@
   note: not rebasing 2:b18e25de2cf5 "D" (D), already in destination as 1:112478962961 "B"
   rebasing 3:7fb047a69f22 "E" (E)
   rebasing 5:66f1a38021c9 "F" (F tip)
+  note: rebase of 5:66f1a38021c9 created no changes to commit
 
 Rebased F should have one parent, just like in the test case above
 
   $ hg log -G
-  o  7:502540f44880 F
-  |
   o  6:533690786a86 E
   |
   | x    5:66f1a38021c9 F
@@ -1127,6 +1125,77 @@
   
   $ cd ..
 
+Rebase merge where both parents have successors in destination
+
+  $ hg init p12-succ-in-dest
+  $ cd p12-succ-in-dest
+  $ hg debugdrawdag <<'EOS'
+  >   E   F
+  >  /|  /|  # replace: A -> C
+  > A B C D  # replace: B -> D
+  > EOS
+  $ hg rebase -r ::E -d F
+  note: not rebasing 0:426bada5c675 "A" (A), already in destination as 2:96cc3511f894 "C"
+  note: not rebasing 1:fc2b737bb2e5 "B" (B), already in destination as 3:058c1e1fb10a "D"
+  rebasing 4:d6e82823588a "E" (E)
+  warning: cannot decide a unique merge base for 4:d6e82823588a, merge result may be suboptimal
+  $ hg log -Gp -T '{rev}:{node|short} {desc|firstline}\n'
+  o  6:d0827eab33f0 E
+  |  diff -r e0c929a964ce -r d0827eab33f0 A
+  |  --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  |  +++ b/A Thu Jan 01 00:00:00 1970 +0000
+  |  @@ -0,0 +1,1 @@
+  |  +A
+  |  \ No newline at end of file
+  |
+  o    5:e0c929a964ce F
+  |\   diff -r 058c1e1fb10a -r e0c929a964ce C
+  | |  --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  | |  +++ b/C Thu Jan 01 00:00:00 1970 +0000
+  | |  @@ -0,0 +1,1 @@
+  | |  +C
+  | |  \ No newline at end of file
+  | |
+  | | x    4:d6e82823588a E
+  | | |\   diff -r 426bada5c675 -r d6e82823588a B
+  | | | |  --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  | | | |  +++ b/B Thu Jan 01 00:00:00 1970 +0000
+  | | | |  @@ -0,0 +1,1 @@
+  | | | |  +B
+  | | | |  \ No newline at end of file
+  | | | |
+  | o | |  3:058c1e1fb10a D
+  |  / /   diff -r 000000000000 -r 058c1e1fb10a D
+  | | |    --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  | | |    +++ b/D Thu Jan 01 00:00:00 1970 +0000
+  | | |    @@ -0,0 +1,1 @@
+  | | |    +D
+  | | |    \ No newline at end of file
+  | | |
+  o | |  2:96cc3511f894 C
+   / /   diff -r 000000000000 -r 96cc3511f894 C
+  | |    --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  | |    +++ b/C Thu Jan 01 00:00:00 1970 +0000
+  | |    @@ -0,0 +1,1 @@
+  | |    +C
+  | |    \ No newline at end of file
+  | |
+  | x  1:fc2b737bb2e5 B
+  |    diff -r 000000000000 -r fc2b737bb2e5 B
+  |    --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  |    +++ b/B Thu Jan 01 00:00:00 1970 +0000
+  |    @@ -0,0 +1,1 @@
+  |    +B
+  |    \ No newline at end of file
+  |
+  x  0:426bada5c675 A
+     diff -r 000000000000 -r 426bada5c675 A
+     --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+     +++ b/A Thu Jan 01 00:00:00 1970 +0000
+     @@ -0,0 +1,1 @@
+     +A
+     \ No newline at end of file
+  
 Test that bookmark is moved and working dir is updated when all changesets have
 equivalents in destination
   $ hg init rbsrepo && cd rbsrepo
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -1087,9 +1087,35 @@
     if len(bases) > 1:
         bases.difference_update(ancestor(rev, d) for d in set(dests))
 
-    # Only pick the merge base if we have a unique candidate
-    if len(bases) == 1:
-        base = next(iter(bases))
+    # If there are still multiple candidates, prefer obsoleted ones with
+    # successor in destination. Otherwise we might re-introduce unwanted
+    # obsoleted changes. For example,
+    #
+    #     C      # rebase -r A+B+C -d D
+    #    /|      # Suppose A has content "+A", B has "+B", D has "+D".
+    #   A B   D  # replace: A is replaced by D
+    #
+    # B gets moved on top of D, A gets skipped, C gets moved on top of B':
+    #
+    #         C' # When choosing merge base for C, A and B are candidates.
+    #         |  # If we choose B, the difference between C and B are "+A",
+    #     C   B' # and C' will have the content "+A", which is suboptimal
+    #    /|   |  # because it re-introduces obsoleted content. If we choose
+    #   A B   D  # A as merge base, it works as expected - C' may be empty.
+    if len(bases) > 1:
+        bases = set(r for r in bases if any(ancestor(dest, s) == s
+                                            for s in successorrevs(repo, r)))
+
+    # Pick one candidate. Even if there is no unique candidate, picking one is
+    # better than letting the merge code to decide.
+    if len(bases) >= 1:
+        if len(bases) > 1:
+            # This is worth a warning since the merge could be suboptimal.
+            # See the test case introduced by this changeset for an example.
+            repo.ui.warn(_('warning: cannot decide a unique merge base for '
+                           '%d:%s, merge result may be suboptimal\n')
+                         % (rev, short(cl.node(rev))))
+        base = max(bases)
 
     return newps[0], newps[1], base
 



To: quark, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D340: rebase: prefer choosing merge base with successor in destination

dsp (David Soria Parra)
quark updated this revision to Diff 768.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D340?vs=766&id=768

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

AFFECTED FILES
  hgext/rebase.py
  tests/test-rebase-obsolete.t

CHANGE DETAILS

diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
--- a/tests/test-rebase-obsolete.t
+++ b/tests/test-rebase-obsolete.t
@@ -1069,9 +1069,8 @@
   rebasing 2:b18e25de2cf5 "D" (D)
   note: not rebasing 3:7fb047a69f22 "E" (E), already in destination as 1:112478962961 "B"
   rebasing 5:66f1a38021c9 "F" (F tip)
+  note: rebase of 5:66f1a38021c9 created no changes to commit
   $ hg log -G
-  o  7:9ed45af61fa0 F
-  |
   o  6:8f47515dda15 D
   |
   | x    5:66f1a38021c9 F
@@ -1105,12 +1104,11 @@
   note: not rebasing 2:b18e25de2cf5 "D" (D), already in destination as 1:112478962961 "B"
   rebasing 3:7fb047a69f22 "E" (E)
   rebasing 5:66f1a38021c9 "F" (F tip)
+  note: rebase of 5:66f1a38021c9 created no changes to commit
 
-Rebased F should have one parent, just like in the test case above
+F should disappear, just like in the test case above
 
   $ hg log -G
-  o  7:502540f44880 F
-  |
   o  6:533690786a86 E
   |
   | x    5:66f1a38021c9 F
@@ -1127,6 +1125,77 @@
   
   $ cd ..
 
+Rebase merge where both parents have successors in destination
+
+  $ hg init p12-succ-in-dest
+  $ cd p12-succ-in-dest
+  $ hg debugdrawdag <<'EOS'
+  >   E   F
+  >  /|  /|  # replace: A -> C
+  > A B C D  # replace: B -> D
+  > EOS
+  $ hg rebase -r ::E -d F
+  note: not rebasing 0:426bada5c675 "A" (A), already in destination as 2:96cc3511f894 "C"
+  note: not rebasing 1:fc2b737bb2e5 "B" (B), already in destination as 3:058c1e1fb10a "D"
+  rebasing 4:d6e82823588a "E" (E)
+  warning: cannot decide a unique merge base for 4:d6e82823588a, merge result may be suboptimal
+  $ hg log -Gp -T '{rev}:{node|short} {desc|firstline}\n'
+  o  6:d0827eab33f0 E
+  |  diff -r e0c929a964ce -r d0827eab33f0 A
+  |  --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  |  +++ b/A Thu Jan 01 00:00:00 1970 +0000
+  |  @@ -0,0 +1,1 @@
+  |  +A
+  |  \ No newline at end of file
+  |
+  o    5:e0c929a964ce F
+  |\   diff -r 058c1e1fb10a -r e0c929a964ce C
+  | |  --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  | |  +++ b/C Thu Jan 01 00:00:00 1970 +0000
+  | |  @@ -0,0 +1,1 @@
+  | |  +C
+  | |  \ No newline at end of file
+  | |
+  | | x    4:d6e82823588a E
+  | | |\   diff -r 426bada5c675 -r d6e82823588a B
+  | | | |  --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  | | | |  +++ b/B Thu Jan 01 00:00:00 1970 +0000
+  | | | |  @@ -0,0 +1,1 @@
+  | | | |  +B
+  | | | |  \ No newline at end of file
+  | | | |
+  | o | |  3:058c1e1fb10a D
+  |  / /   diff -r 000000000000 -r 058c1e1fb10a D
+  | | |    --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  | | |    +++ b/D Thu Jan 01 00:00:00 1970 +0000
+  | | |    @@ -0,0 +1,1 @@
+  | | |    +D
+  | | |    \ No newline at end of file
+  | | |
+  o | |  2:96cc3511f894 C
+   / /   diff -r 000000000000 -r 96cc3511f894 C
+  | |    --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  | |    +++ b/C Thu Jan 01 00:00:00 1970 +0000
+  | |    @@ -0,0 +1,1 @@
+  | |    +C
+  | |    \ No newline at end of file
+  | |
+  | x  1:fc2b737bb2e5 B
+  |    diff -r 000000000000 -r fc2b737bb2e5 B
+  |    --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  |    +++ b/B Thu Jan 01 00:00:00 1970 +0000
+  |    @@ -0,0 +1,1 @@
+  |    +B
+  |    \ No newline at end of file
+  |
+  x  0:426bada5c675 A
+     diff -r 000000000000 -r 426bada5c675 A
+     --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+     +++ b/A Thu Jan 01 00:00:00 1970 +0000
+     @@ -0,0 +1,1 @@
+     +A
+     \ No newline at end of file
+  
 Test that bookmark is moved and working dir is updated when all changesets have
 equivalents in destination
   $ hg init rbsrepo && cd rbsrepo
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -1087,9 +1087,35 @@
     if len(bases) > 1:
         bases.difference_update(ancestor(rev, d) for d in set(dests))
 
-    # Only pick the merge base if we have a unique candidate
-    if len(bases) == 1:
-        base = next(iter(bases))
+    # If there are still multiple candidates, prefer obsoleted ones with
+    # successor in destination. Otherwise we might re-introduce unwanted
+    # obsoleted changes. For example,
+    #
+    #     C      # rebase -r A+B+C -d D
+    #    /|      # Suppose A has content "+A", B has "+B", D has "+D".
+    #   A B   D  # replace: A is replaced by D
+    #
+    # B gets moved on top of D, A gets skipped, C gets moved on top of B':
+    #
+    #         C' # When choosing merge base for C, A and B are candidates.
+    #         |  # If we choose B, the difference between C and B are "+A",
+    #     C   B' # and C' will have the content "+A", which is suboptimal
+    #    /|   |  # because it re-introduces obsoleted content. If we choose
+    #   A B   D  # A as merge base, it works as expected - C' may be empty.
+    if len(bases) > 1:
+        bases = set(r for r in bases if any(ancestor(dest, s) == s
+                                            for s in successorrevs(repo, r)))
+
+    # Pick one candidate. Even if there is no unique candidate, picking one is
+    # better than letting the merge code to decide.
+    if len(bases) >= 1:
+        if len(bases) > 1:
+            # This is worth a warning since the merge could be suboptimal.
+            # See the test case introduced by this changeset for an example.
+            repo.ui.warn(_('warning: cannot decide a unique merge base for '
+                           '%d:%s, merge result may be suboptimal\n')
+                         % (rev, short(cl.node(rev))))
+        base = max(bases)
 
     return newps[0], newps[1], base
 



To: quark, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D340: rebase: prefer choosing merge base with successor in destination

dsp (David Soria Parra)
In reply to this post by dsp (David Soria Parra)
quark updated this revision to Diff 770.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D340?vs=768&id=770

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

AFFECTED FILES
  hgext/rebase.py
  tests/test-rebase-obsolete.t

CHANGE DETAILS

diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
--- a/tests/test-rebase-obsolete.t
+++ b/tests/test-rebase-obsolete.t
@@ -1069,9 +1069,8 @@
   rebasing 2:b18e25de2cf5 "D" (D)
   note: not rebasing 3:7fb047a69f22 "E" (E), already in destination as 1:112478962961 "B"
   rebasing 5:66f1a38021c9 "F" (F tip)
+  note: rebase of 5:66f1a38021c9 created no changes to commit
   $ hg log -G
-  o  7:9ed45af61fa0 F
-  |
   o  6:8f47515dda15 D
   |
   | x    5:66f1a38021c9 F
@@ -1105,12 +1104,11 @@
   note: not rebasing 2:b18e25de2cf5 "D" (D), already in destination as 1:112478962961 "B"
   rebasing 3:7fb047a69f22 "E" (E)
   rebasing 5:66f1a38021c9 "F" (F tip)
+  note: rebase of 5:66f1a38021c9 created no changes to commit
 
-Rebased F should have one parent, just like in the test case above
+F should disappear, just like in the test case above
 
   $ hg log -G
-  o  7:502540f44880 F
-  |
   o  6:533690786a86 E
   |
   | x    5:66f1a38021c9 F
@@ -1127,6 +1125,79 @@
   
   $ cd ..
 
+Rebase merge where both parents have successors in destination
+
+  $ hg init p12-succ-in-dest
+  $ cd p12-succ-in-dest
+  $ hg debugdrawdag <<'EOS'
+  >   E   F
+  >  /|  /|  # replace: A -> C
+  > A B C D  # replace: B -> D
+  > EOS
+  $ hg rebase -r ::E -d F
+  note: not rebasing 0:426bada5c675 "A" (A), already in destination as 2:96cc3511f894 "C"
+  note: not rebasing 1:fc2b737bb2e5 "B" (B), already in destination as 3:058c1e1fb10a "D"
+  rebasing 4:d6e82823588a "E" (E)
+  warning: cannot decide a unique merge base for 4:d6e82823588a, merge result may be suboptimal
+  $ hg log -Gp -T '{rev}:{node|short} {desc|firstline}\n'
+  o  6:d0827eab33f0 E
+  |  diff -r e0c929a964ce -r d0827eab33f0 A
+  |  --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  |  +++ b/A Thu Jan 01 00:00:00 1970 +0000
+  |  @@ -0,0 +1,1 @@
+  |  +A
+  |  \ No newline at end of file
+  |
+  o    5:e0c929a964ce F
+  |\   diff -r 058c1e1fb10a -r e0c929a964ce C
+  | |  --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  | |  +++ b/C Thu Jan 01 00:00:00 1970 +0000
+  | |  @@ -0,0 +1,1 @@
+  | |  +C
+  | |  \ No newline at end of file
+  | |
+  | | x    4:d6e82823588a E
+  | | |\   diff -r 426bada5c675 -r d6e82823588a B
+  | | | |  --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  | | | |  +++ b/B Thu Jan 01 00:00:00 1970 +0000
+  | | | |  @@ -0,0 +1,1 @@
+  | | | |  +B
+  | | | |  \ No newline at end of file
+  | | | |
+  | o | |  3:058c1e1fb10a D
+  |  / /   diff -r 000000000000 -r 058c1e1fb10a D
+  | | |    --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  | | |    +++ b/D Thu Jan 01 00:00:00 1970 +0000
+  | | |    @@ -0,0 +1,1 @@
+  | | |    +D
+  | | |    \ No newline at end of file
+  | | |
+  o | |  2:96cc3511f894 C
+   / /   diff -r 000000000000 -r 96cc3511f894 C
+  | |    --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  | |    +++ b/C Thu Jan 01 00:00:00 1970 +0000
+  | |    @@ -0,0 +1,1 @@
+  | |    +C
+  | |    \ No newline at end of file
+  | |
+  | x  1:fc2b737bb2e5 B
+  |    diff -r 000000000000 -r fc2b737bb2e5 B
+  |    --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+  |    +++ b/B Thu Jan 01 00:00:00 1970 +0000
+  |    @@ -0,0 +1,1 @@
+  |    +B
+  |    \ No newline at end of file
+  |
+  x  0:426bada5c675 A
+     diff -r 000000000000 -r 426bada5c675 A
+     --- /dev/null Thu Jan 01 00:00:00 1970 +0000
+     +++ b/A Thu Jan 01 00:00:00 1970 +0000
+     @@ -0,0 +1,1 @@
+     +A
+     \ No newline at end of file
+  
+  $ cd ..
+
 Test that bookmark is moved and working dir is updated when all changesets have
 equivalents in destination
   $ hg init rbsrepo && cd rbsrepo
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -1087,9 +1087,35 @@
     if len(bases) > 1:
         bases.difference_update(ancestor(rev, d) for d in set(dests))
 
-    # Only pick the merge base if we have a unique candidate
-    if len(bases) == 1:
-        base = next(iter(bases))
+    # If there are still multiple candidates, prefer obsoleted ones with
+    # successor in destination. Otherwise we might re-introduce unwanted
+    # obsoleted changes. For example,
+    #
+    #     C      # rebase -r A+B+C -d D
+    #    /|      # Suppose A has content "+A", B has "+B", D has "+D".
+    #   A B   D  # replace: A is replaced by D
+    #
+    # B gets moved on top of D, A gets skipped, C gets moved on top of B':
+    #
+    #         C' # When choosing merge base for C, A and B are candidates.
+    #         |  # If we choose B, the difference between C and B are "+A",
+    #     C   B' # and C' will have the content "+A", which is suboptimal
+    #    /|   |  # because it re-introduces obsoleted content. If we choose
+    #   A B   D  # A as merge base, it works as expected - C' may be empty.
+    if len(bases) > 1:
+        bases = set(r for r in bases if any(ancestor(dest, s) == s
+                                            for s in successorrevs(repo, r)))
+
+    # Pick one candidate. Even if there is no unique candidate, picking one is
+    # better than letting the merge code to decide.
+    if len(bases) >= 1:
+        if len(bases) > 1:
+            # This is worth a warning since the merge could be suboptimal.
+            # See the test case introduced by this changeset for an example.
+            repo.ui.warn(_('warning: cannot decide a unique merge base for '
+                           '%d:%s, merge result may be suboptimal\n')
+                         % (rev, short(cl.node(rev))))
+        base = max(bases)
 
     return newps[0], newps[1], base
 



To: quark, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D340: rebase: prefer choosing merge base with successor in destination

dsp (David Soria Parra)
In reply to this post by dsp (David Soria Parra)
quark updated this revision to Diff 830.
quark edited the summary of this revision.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D340?vs=770&id=830

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

AFFECTED FILES
  hgext/rebase.py
  tests/test-rebase-obsolete.t

CHANGE DETAILS

diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
--- a/tests/test-rebase-obsolete.t
+++ b/tests/test-rebase-obsolete.t
@@ -1104,11 +1104,9 @@
   note: not rebasing 2:b18e25de2cf5 "D" (D), already in destination as 1:112478962961 "B"
   rebasing 3:7fb047a69f22 "E" (E)
   rebasing 5:66f1a38021c9 "F" (F tip)
-  warning: rebasing 5:66f1a38021c9 may inlcude unwanted changes from revision 2
+  note: rebase of 5:66f1a38021c9 created no changes to commit
 
   $ hg log -G
-  o  7:502540f44880 F
-  |
   o  6:533690786a86 E
   |
   | x    5:66f1a38021c9 F
@@ -1125,6 +1123,48 @@
   
   $ cd ..
 
+Rebase merge where both parents have successors in destination
+
+  $ hg init p12-succ-in-dest
+  $ cd p12-succ-in-dest
+  $ hg debugdrawdag <<'EOS'
+  >   E   F
+  >  /|  /|  # replace: A -> C
+  > A B C D  # replace: B -> D
+  > | |
+  > X Y
+  > EOS
+  $ hg rebase -r A+B+E -d F
+  note: not rebasing 4:a3d17304151f "A" (A), already in destination as 0:96cc3511f894 "C"
+  note: not rebasing 5:b23a2cc00842 "B" (B), already in destination as 1:058c1e1fb10a "D"
+  rebasing 7:dac5d11c5a7d "E" (E tip)
+  warning: rebasing 7:dac5d11c5a7d may inlcude unwanted changes from revision 3, 5
+  $ hg manifest -r tip
+  B
+  C
+  D
+  Y
+  $ hg log -G -T '{rev}:{node|short} {desc|firstline} +{file_adds}\n'
+  o  8:3306ff4ff997 E +B Y
+  |
+  | x    7:dac5d11c5a7d E +B Y
+  | |\
+  o \ \    6:e0c929a964ce F +C
+  |\ \ \
+  | | | x  5:b23a2cc00842 B +B
+  | | | |
+  | | x |  4:a3d17304151f A +A
+  | | | |
+  | | | o  3:59c792af609c Y +Y
+  | | |
+  | | o  2:ba2b7fa7166d X +X
+  | |
+  | o  1:058c1e1fb10a D +D
+  |
+  o  0:96cc3511f894 C +C
+  
+  $ cd ..
+
 Test that bookmark is moved and working dir is updated when all changesets have
 equivalents in destination
   $ hg init rbsrepo && cd rbsrepo
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -1095,6 +1095,27 @@
 
     repo.ui.debug(" future parents are %d and %d\n" % tuple(newps))
 
+    # If there are multiple merge base candidates, try to remove one of them.
+    # Prefer obsoleted ones with successor in destination. Otherwise we might
+    # re-introduce unwanted obsoleted changes. For example,
+    #
+    #     C      # rebase -r A+B+C -d D
+    #    /|      # Suppose A has content "+A", B has "+B", D has "+D".
+    #   A B   D  # replace: A is replaced by D
+    #
+    # B gets moved on top of D, A gets skipped, C gets moved on top of B':
+    #
+    #         C' # When choosing merge base for C, A and B are candidates.
+    #         |  # If we choose B, the difference between C and B are "+A",
+    #     C   B' # and C' will have the content "+A", which is suboptimal
+    #    /|   |  # because it re-introduces obsoleted content. If we choose
+    #   A B   D  # A as merge base, it works as expected - C' may be empty.
+    if len(bases) > 1:
+        newbases = [r for r in bases
+                    if any(isancestor(s, dest) for s in successorrevs(repo, r))]
+        if len(newbases) == 1:
+            bases = newbases
+
     # Decide a merge base
     base = None
     if len(bases) == 1:



To: quark, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D340: rebase: prefer choosing merge base with successor in destination

dsp (David Soria Parra)
In reply to this post by dsp (David Soria Parra)
quark updated this revision to Diff 868.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D340?vs=830&id=868

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

AFFECTED FILES
  hgext/rebase.py
  tests/test-rebase-obsolete.t

CHANGE DETAILS

diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
--- a/tests/test-rebase-obsolete.t
+++ b/tests/test-rebase-obsolete.t
@@ -1069,10 +1069,8 @@
   rebasing 2:b18e25de2cf5 "D" (D)
   note: not rebasing 3:7fb047a69f22 "E" (E), already in destination as 1:112478962961 "B"
   rebasing 5:66f1a38021c9 "F" (F tip)
-  warning: rebasing 5:66f1a38021c9 may inlcude unwanted changes from revision 3
+  note: rebase of 5:66f1a38021c9 created no changes to commit
   $ hg log -G
-  o  7:9ed45af61fa0 F
-  |
   o  6:8f47515dda15 D
   |
   | x    5:66f1a38021c9 F
@@ -1106,11 +1104,9 @@
   note: not rebasing 2:b18e25de2cf5 "D" (D), already in destination as 1:112478962961 "B"
   rebasing 3:7fb047a69f22 "E" (E)
   rebasing 5:66f1a38021c9 "F" (F tip)
-  warning: rebasing 5:66f1a38021c9 may inlcude unwanted changes from revision 2
+  note: rebase of 5:66f1a38021c9 created no changes to commit
 
   $ hg log -G
-  o  7:502540f44880 F
-  |
   o  6:533690786a86 E
   |
   | x    5:66f1a38021c9 F
@@ -1127,6 +1123,137 @@
   
   $ cd ..
 
+Rebase merge where both parents have successors in destination
+
+  $ hg init p12-succ-in-dest
+  $ cd p12-succ-in-dest
+  $ hg debugdrawdag <<'EOS'
+  >   E   F
+  >  /|  /|  # replace: A -> C
+  > A B C D  # replace: B -> D
+  > | |
+  > X Y
+  > EOS
+  $ hg rebase -r A+B+E -d F
+  note: not rebasing 4:a3d17304151f "A" (A), already in destination as 0:96cc3511f894 "C"
+  note: not rebasing 5:b23a2cc00842 "B" (B), already in destination as 1:058c1e1fb10a "D"
+  rebasing 7:dac5d11c5a7d "E" (E tip)
+  warning: rebasing 7:dac5d11c5a7d may inlcude unwanted changes from revision 3, 5
+  $ hg manifest -r tip
+  B
+  C
+  D
+  Y
+  $ hg log -G -T '{rev}:{node|short} {desc|firstline} +{file_adds}\n'
+  o  8:3306ff4ff997 E +B Y
+  |
+  | x    7:dac5d11c5a7d E +B Y
+  | |\
+  o \ \    6:e0c929a964ce F +C
+  |\ \ \
+  | | | x  5:b23a2cc00842 B +B
+  | | | |
+  | | x |  4:a3d17304151f A +A
+  | | | |
+  | | | o  3:59c792af609c Y +Y
+  | | |
+  | | o  2:ba2b7fa7166d X +X
+  | |
+  | o  1:058c1e1fb10a D +D
+  |
+  o  0:96cc3511f894 C +C
+  
+  $ cd ..
+
+Rebase one parent with successor in destination, the other parent moves as "-d"
+requests. These two tests are to cover the case when the second merge base
+candidate gets selected, new parents get updated accordingly so new p1 matches
+the merge base.
+
+  $ hg init p1-succ-p2-move
+  $ cd p1-succ-p2-move
+  $ hg debugdrawdag <<'EOS'
+  >   D
+  >  /|
+  > A B Z  # replace: A -> C
+  > | | |  # D/D = D
+  > X Y C
+  > EOS
+  $ hg rebase -r A+B+D -d Z --config experimental.rebaseskipobsolete=0
+  rebasing 3:a3d17304151f "A" (A)
+  rebasing 4:b23a2cc00842 "B" (B)
+  rebasing 6:141c08ee43e3 "D" (D tip)
+  warning: rebasing 6:141c08ee43e3 may inlcude unwanted changes from revision 2
+
+  $ rm .hg/localtags
+  $ hg log -G
+  o    9:791364acf1f7 D
+  |\
+  | o  8:eb6ceb8cd2c1 B
+  | |
+  o |  7:fe98b87cd6e0 A
+  |/
+  o  5:50e41c1f3950 Z
+  |
+  | o  2:59c792af609c Y
+  |
+  | o  1:ba2b7fa7166d X
+  |
+  o  0:96cc3511f894 C
+  
+  $ hg files -r tip
+  A
+  B
+  C
+  D
+  Y
+  Z
+
+  $ cd ..
+
+With p1 and p2 swapped from the above case
+
+  $ hg init p1-move-p2-succ
+  $ cd p1-move-p2-succ
+  $ hg debugdrawdag <<'EOS'
+  >   D
+  >  /|
+  > A B Z  # replace: B -> C
+  > | | |  # D/D = D
+  > X Y C
+  > EOS
+  $ hg rebase -r B+A+D -d Z --config experimental.rebaseskipobsolete=0
+  rebasing 3:a3d17304151f "A" (A)
+  rebasing 4:b23a2cc00842 "B" (B)
+  rebasing 6:141c08ee43e3 "D" (D tip)
+  warning: rebasing 6:141c08ee43e3 may inlcude unwanted changes from revision 1
+
+  $ rm .hg/localtags
+  $ hg log -G
+  o    9:433cf923621e D
+  |\
+  | o  8:eb6ceb8cd2c1 B
+  | |
+  o |  7:fe98b87cd6e0 A
+  |/
+  o  5:50e41c1f3950 Z
+  |
+  | o  2:59c792af609c Y
+  |
+  | o  1:ba2b7fa7166d X
+  |
+  o  0:96cc3511f894 C
+  
+  $ hg files -r tip
+  A
+  B
+  C
+  D
+  X
+  Z
+
+  $ cd ..
+
 Test that bookmark is moved and working dir is updated when all changesets have
 equivalents in destination
   $ hg init rbsrepo && cd rbsrepo
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -1096,6 +1096,30 @@
 
     repo.ui.debug(" future parents are %d and %d\n" % tuple(newps))
 
+    # If there are multiple merge base candidates, try to remove one of them.
+    # Prefer keeping obsoleted node with successor in destination. Otherwise we
+    # might re-introduce unwanted obsoleted changes. For example,
+    #
+    #     C      # rebase -r A+B+C -d D
+    #    /|      # Suppose A has content "+A", B has "+B", D has "+D".
+    #   A B   D  # replace: A is replaced by D
+    #
+    # B gets moved on top of D, A gets skipped, C gets moved on top of B':
+    #
+    #         C' # When choosing merge base for C, A and B are candidates.
+    #         |  # If we choose B, the difference between C and B are "+A",
+    #     C   B' # and C' will have the content "+A", which is suboptimal
+    #    /|   |  # because it re-introduces obsoleted content. If we choose
+    #   A B   D  # A as merge base, it works as expected - C' may be empty.
+    if all(b != nullrev for b in bases):
+        assert len(bases) == 2
+        preferred = [any(isancestor(s, dest) for s in successorrevs(repo, r))
+                     for r in bases]
+        if preferred == [False, True]:
+            bases.reverse()
+            if newps[1] != nullrev:
+                newps.reverse()
+
     # "rebasenode" updates to new p1, use the corresponding merge base.
     if bases[0] != nullrev:
         base = bases[0]



To: quark, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D340: rebase: prefer choosing merge base with successor in destination

dsp (David Soria Parra)
In reply to this post by dsp (David Soria Parra)
quark updated this revision to Diff 870.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D340?vs=868&id=870

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

AFFECTED FILES
  hgext/rebase.py
  tests/test-rebase-obsolete.t

CHANGE DETAILS

diff --git a/tests/test-rebase-obsolete.t b/tests/test-rebase-obsolete.t
--- a/tests/test-rebase-obsolete.t
+++ b/tests/test-rebase-obsolete.t
@@ -1069,10 +1069,8 @@
   rebasing 2:b18e25de2cf5 "D" (D)
   note: not rebasing 3:7fb047a69f22 "E" (E), already in destination as 1:112478962961 "B"
   rebasing 5:66f1a38021c9 "F" (F tip)
-  warning: rebasing 5:66f1a38021c9 may include unwanted changes from 3:7fb047a69f22
+  note: rebase of 5:66f1a38021c9 created no changes to commit
   $ hg log -G
-  o  7:9ed45af61fa0 F
-  |
   o  6:8f47515dda15 D
   |
   | x    5:66f1a38021c9 F
@@ -1106,11 +1104,9 @@
   note: not rebasing 2:b18e25de2cf5 "D" (D), already in destination as 1:112478962961 "B"
   rebasing 3:7fb047a69f22 "E" (E)
   rebasing 5:66f1a38021c9 "F" (F tip)
-  warning: rebasing 5:66f1a38021c9 may include unwanted changes from 2:b18e25de2cf5
+  note: rebase of 5:66f1a38021c9 created no changes to commit
 
   $ hg log -G
-  o  7:502540f44880 F
-  |
   o  6:533690786a86 E
   |
   | x    5:66f1a38021c9 F
@@ -1127,6 +1123,121 @@
   
   $ cd ..
 
+Rebase merge where both parents have successors in destination
+
+  $ hg init p12-succ-in-dest
+  $ cd p12-succ-in-dest
+  $ hg debugdrawdag <<'EOS'
+  >   E   F
+  >  /|  /|  # replace: A -> C
+  > A B C D  # replace: B -> D
+  > | |
+  > X Y
+  > EOS
+  $ hg rebase -r A+B+E -d F
+  note: not rebasing 4:a3d17304151f "A" (A), already in destination as 0:96cc3511f894 "C"
+  note: not rebasing 5:b23a2cc00842 "B" (B), already in destination as 1:058c1e1fb10a "D"
+  rebasing 7:dac5d11c5a7d "E" (E tip)
+  warning: rebasing 7:dac5d11c5a7d may include unwanted changes from 3:59c792af609c, 5:b23a2cc00842
+  $ hg manifest -r tip
+  B
+  C
+  D
+  Y
+  $ hg log -G -T '{rev}:{node|short} {desc|firstline} +{file_adds}\n'
+  o  8:3306ff4ff997 E +B Y
+  |
+  | x    7:dac5d11c5a7d E +B Y
+  | |\
+  o \ \    6:e0c929a964ce F +C
+  |\ \ \
+  | | | x  5:b23a2cc00842 B +B
+  | | | |
+  | | x |  4:a3d17304151f A +A
+  | | | |
+  | | | o  3:59c792af609c Y +Y
+  | | |
+  | | o  2:ba2b7fa7166d X +X
+  | |
+  | o  1:058c1e1fb10a D +D
+  |
+  o  0:96cc3511f894 C +C
+  
+  $ cd ..
+
+Rebase one parent with successor in destination, the other parent moves as "-d"
+requests. These two tests cover the case when the second merge base candidate
+gets selected, new parents get updated accordingly so new p1 matches the merge
+base.
+
+  $ hg init p1-succ-p2-move
+  $ cd p1-succ-p2-move
+  $ hg debugdrawdag <<'EOS'
+  >   D Z
+  >  /| | # replace: A -> C
+  > A B C # D/D = D
+  > EOS
+  $ hg rebase -r A+B+D -d Z --config experimental.rebaseskipobsolete=0
+  rebasing 0:426bada5c675 "A" (A)
+  rebasing 1:fc2b737bb2e5 "B" (B)
+  rebasing 3:b8ed089c80ad "D" (D)
+
+  $ rm .hg/localtags
+  $ hg log -G
+  o    7:7636985f7fde D
+  |\
+  | o  6:76840d832e98 B
+  | |
+  o |  5:a81a74d764a6 A
+  |/
+  o  4:50e41c1f3950 Z
+  |
+  o  2:96cc3511f894 C
+  
+  $ hg files -r tip
+  A
+  B
+  C
+  D
+  Z
+
+  $ cd ..
+
+With p1 and p2 swapped from the above case
+
+  $ hg init p1-move-p2-succ
+  $ cd p1-move-p2-succ
+  $ hg debugdrawdag <<'EOS'
+  >   D Z
+  >  /| |  # replace: B -> C
+  > A B C  # D/D = D
+  > EOS
+  $ hg rebase -r B+A+D -d Z --config experimental.rebaseskipobsolete=0
+  rebasing 0:426bada5c675 "A" (A)
+  rebasing 1:fc2b737bb2e5 "B" (B)
+  rebasing 3:b8ed089c80ad "D" (D)
+
+  $ rm .hg/localtags
+  $ hg log -G
+  o    7:7636985f7fde D
+  |\
+  | o  6:76840d832e98 B
+  | |
+  o |  5:a81a74d764a6 A
+  |/
+  o  4:50e41c1f3950 Z
+  |
+  o  2:96cc3511f894 C
+  
+  $ hg files -r tip
+  A
+  B
+  C
+  D
+  Z
+
+  $ cd ..
+
 Test that bookmark is moved and working dir is updated when all changesets have
 equivalents in destination
   $ hg init rbsrepo && cd rbsrepo
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -1098,6 +1098,30 @@
 
     repo.ui.debug(" future parents are %d and %d\n" % tuple(newps))
 
+    # If there are multiple merge base candidates, try to remove one of them.
+    # Prefer keeping obsoleted node with successor in destination. Otherwise we
+    # might re-introduce unwanted obsoleted changes. For example,
+    #
+    #     C      # rebase -r A+B+C -d D
+    #    /|      # Suppose A has content "+A", B has "+B", D has "+D".
+    #   A B   D  # replace: A is replaced by D
+    #
+    # B gets moved on top of D, A gets skipped, C gets moved on top of B':
+    #
+    #         C' # When choosing merge base for C, A and B are candidates.
+    #         |  # If we choose B, the difference between C and B are "+A",
+    #     C   B' # and C' will have the content "+A", which is suboptimal
+    #    /|   |  # because it re-introduces obsoleted content. If we choose
+    #   A B   D  # A as merge base, it works as expected - C' may be empty.
+    if all(b != nullrev for b in bases):
+        assert len(bases) == 2
+        preferred = [any(isancestor(s, dest) for s in successorrevs(repo, r))
+                     for r in bases]
+        if preferred == [False, True]:
+            bases.reverse()
+            if newps[1] != nullrev:
+                newps.reverse()
+
     # "rebasenode" updates to new p1, use the corresponding merge base.
     if bases[0] != nullrev:
         base = bases[0]



To: quark, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D340: rebase: prefer choosing merge base with successor in destination

dsp (David Soria Parra)
In reply to this post by dsp (David Soria Parra)
martinvonz added inline comments.

INLINE COMMENTS

> rebase.py:1115
> +    #    /|   |  # because it re-introduces obsoleted content. If we choose
> +    #   A B   D  # A as merge base, it works as expected - C' may be empty.
> +    if all(b != nullrev for b in bases):

And if there are more ancestors of B, will the "rebasing %d:%s may include unwanted changes" warning appear?

Regardless, if B adds file B and the merge in C involves removing file B, then the total diff will be empty. Does that mean that C' will be empty? It should be reverting B', right? (The same thing applies if it's not a whole file that gets added/removed, of course.) I think I'd prefer to be conservative here and error out until we have a safe answer to these merges that won't risk resulting in surprising contents. In this particular case, the user can work around it by first rebasing "B+C" only (but that may be tricky to realize).

> test-rebase-obsolete.t:1180
> +  > EOS
> +  $ hg rebase -r A+B+D -d Z --config experimental.rebaseskipobsolete=0
> +  rebasing 0:426bada5c675 "A" (A)

Why do we want/need rebaseskipobsolete? I was expecting to see a test case like then one in the documentation you added i rebase.py.

REPOSITORY
  rHG Mercurial

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

To: quark, #hg-reviewers
Cc: martinvonz, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D340: rebase: prefer choosing merge base with successor in destination

dsp (David Soria Parra)
In reply to this post by dsp (David Soria Parra)
quark planned changes to this revision.
quark added a comment.


  Since the "unwanted" warning "covers" the case this patch handles. The behavior is still "correct" (since the warning is printed) without this patch. I'm leaning towards dropping this patch.
 
  For errors vs warnings, I still think it's better to give power users a chance to proceed. How do you think about this approach?
 
  - If there are multiple merge base candidates, calculate "unwanted" revsets for both, and if only one of them is empty, pick that one automatically and proceed.
  - If both merge base candidates have "unwanted" revsets, raise `error.InterventionRequired` so power users could still have a chance to proceed fixing them manually.

INLINE COMMENTS

> martinvonz wrote in rebase.py:1115
> And if there are more ancestors of B, will the "rebasing %d:%s may include unwanted changes" warning appear?
>
> Regardless, if B adds file B and the merge in C involves removing file B, then the total diff will be empty. Does that mean that C' will be empty? It should be reverting B', right? (The same thing applies if it's not a whole file that gets added/removed, of course.) I think I'd prefer to be conservative here and error out until we have a safe answer to these merges that won't risk resulting in surprising contents. In this particular case, the user can work around it by first rebasing "B+C" only (but that may be tricky to realize).

> And if there are more ancestors of B, will the "rebasing %d:%s may include unwanted changes" warning appear?

Yes. In that case maybe either way is suboptimal. The troublesome cases are:

- A merge base candidate has a successor in destination with *different* content
- A merge base candidate has "unwanted" ancestors that are not covered by rebaseset

I think the problem of this patch is it assumes "different" content blindly. It would be better to actually have a quick check about whether that is the case.

Thinking about it again, most cases the reason a commit is in destination is because of a rebase so its content does not change. Therefore this patch is about a very corner case that may not worth the complexity.

> Regardless, if B adds file B and the merge in C involves removing file B, then the total diff will be empty.

That is a surprise to me. Apparently you know merge better than I am :)

> martinvonz wrote in test-rebase-obsolete.t:1180
> Why do we want/need rebaseskipobsolete? I was expecting to see a test case like then one in the documentation you added i rebase.py.

Not sure why I added that. But it does seem to be unnecessary.

REPOSITORY
  rHG Mercurial

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

To: quark, #hg-reviewers
Cc: martinvonz, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

D340: rebase: prefer choosing merge base with successor in destination

dsp (David Soria Parra)
In reply to this post by dsp (David Soria Parra)
martinvonz added a comment.


  In https://phab.mercurial-scm.org/D340#6330, @quark wrote:
 
  > Since the "unwanted" warning "covers" the case this patch handles. The behavior is still "correct" (since the warning is printed) without this patch. I'm leaning towards dropping this patch.
  >
  > For errors vs warnings, I still think it's better to give power users a chance to proceed. How do you think about this approach?
  >
  > - If there are multiple merge base candidates, calculate "unwanted" revsets for both, and if only one of them is empty, pick that one automatically and proceed.
  > - If both merge base candidates have "unwanted" revsets, raise `error.InterventionRequired` so power users could still have a chance to proceed fixing them manually.
 
 
  Sounds good to me. Thanks!
 
  The test cases still seem useful, so maybe you can keep that part of the patch?

REPOSITORY
  rHG Mercurial

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

To: quark, #hg-reviewers
Cc: martinvonz, mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Loading...