Patchwork D342: rebase: change "result would have 3 parent" error message (BC)

login
register
mail settings
Submitter phabricator
Date Aug. 11, 2017, 6:13 a.m.
Message ID <differential-rev-PHID-DREV-ku3vgj74o5t3tzrig2ls-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/22856/
State Superseded
Headers show

Comments

phabricator - Aug. 11, 2017, 6:13 a.m.
quark created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The old error message "cannot use revision REV as base, result would have 3
  parents" is confusing - why use REV as base? why add a new parent?.
  
  This patch changes it to "cannot find new parents", which seems better.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/rebase.py
  tests/test-rebase-brute-force.t
  tests/test-rebase-obsolete.t
  tests/test-rebase-scenario-global.t

CHANGE DETAILS




To: quark, #hg-reviewers
Cc: mercurial-devel
phabricator - Aug. 12, 2017, 4:48 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> rebase.py:1066
>      if set(newps) == set(oldps) and dest not in newps:
> -        # The error message is for compatibility. It's a bit misleading since
> -        # rebase is not supposed to add new parents.
> -        raise error.Abort(_('cannot use revision %d as base, '
> -                            'result would have 3 parents') % rev)
> +        raise error.Abort(_('cannot find new parents for revision %d') % rev)
>  

I feel like "Cannot rebase merge commit %d without rebasing at least one its parents" would be more specific and still be accurate.

It would be nice if we could detect the case upfront, but if we determine the destination dynamically including looking at obsmarkers, that may be difficult

REPOSITORY
  rHG Mercurial

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

To: quark, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Aug. 12, 2017, 6:12 p.m.
quark planned changes to this revision.
quark added inline comments.

INLINE COMMENTS

> martinvonz wrote in rebase.py:1066
> I feel like "Cannot rebase merge commit %d without rebasing at least one its parents" would be more specific and still be accurate.
> 
> It would be nice if we could detect the case upfront, but if we determine the destination dynamically including looking at obsmarkers, that may be difficult

I like the new error message. Will change.

A dry-run of rebase is not feasible until we have in-memory DAG operations, which is one of my future interests.

REPOSITORY
  rHG Mercurial

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

To: quark, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Aug. 13, 2017, 12:35 a.m.
quark added a comment.


  I guess the old error message was intended for the below case:
  
    $ hg debugdrawdag <<'EOF'
    >   H
    >  /|
    > D F
    > | |\
    > C E G
    >  \|/
    >   B Z
    >   |/
    >   A
    > EOF
    $ hg rebase -d Z -r 'C+E+G+H'
  
  Removing D, having C connected to H directly is another interesting case. I think `adjustdest` could be changed to detect that situation (when adjusting H's destination considering its parent F, there are multiple candidates - E and G that may worth a warning or an error. Things could be more fun if E and G has obsolete or ancestor relationship).

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/tests/test-rebase-scenario-global.t b/tests/test-rebase-scenario-global.t
--- a/tests/test-rebase-scenario-global.t
+++ b/tests/test-rebase-scenario-global.t
@@ -271,7 +271,7 @@ 
 
   $ hg rebase -s 6 -d 1
   rebasing 6:eea13746799a "G"
-  abort: cannot use revision 6 as base, result would have 3 parents
+  abort: cannot find new parents for revision 6
   [255]
   $ hg rebase --abort
   rebase aborted
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
@@ -494,7 +494,7 @@ 
   not rebasing ignored 4:26805aba1e60 "C" (C)
   not rebasing ignored 5:4b61ff5c62e2 "E" (E)
   rebasing 6:f15c3adaf214 "F" (F tip)
-  abort: cannot use revision 6 as base, result would have 3 parents
+  abort: cannot find new parents for revision 6
   [255]
 
   $ cd ..
diff --git a/tests/test-rebase-brute-force.t b/tests/test-rebase-brute-force.t
--- a/tests/test-rebase-brute-force.t
+++ b/tests/test-rebase-brute-force.t
@@ -23,15 +23,15 @@ 
      A: A':Z
      B: B':Z
     AB: A':Z B':Z
-     C: ABORT: cannot use revision 3 as base, result would have 3 parents
+     C: ABORT: cannot find new parents for revision 3
     AC: A':Z C':A'B
     BC: B':Z C':B'A
    ABC: A':Z B':Z C':A'B'
      D: D':Z
     AD: A':Z D':Z
     BD: B':Z D':B'
    ABD: A':Z B':Z D':B'
-    CD: ABORT: cannot use revision 3 as base, result would have 3 parents
+    CD: ABORT: cannot find new parents for revision 3
    ACD: A':Z C':A'B D':Z
    BCD: B':Z C':B'A D':B'
   ABCD: A':Z B':Z C':A'B' D':B'
@@ -49,7 +49,7 @@ 
     B: B':Z
     A: 
    BA: B':Z
-    C: ABORT: cannot use revision 3 as base, result would have 3 parents
+    C: ABORT: cannot find new parents for revision 3
    BC: B':Z C':B'A
    AC: 
   BAC: B':Z C':B'A
diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -1063,10 +1063,7 @@ 
     #   |\         None of A and B will be changed to D and rebase fails.
     #   A B   D
     if set(newps) == set(oldps) and dest not in newps:
-        # The error message is for compatibility. It's a bit misleading since
-        # rebase is not supposed to add new parents.
-        raise error.Abort(_('cannot use revision %d as base, '
-                            'result would have 3 parents') % rev)
+        raise error.Abort(_('cannot find new parents for revision %d') % rev)
 
     repo.ui.debug(" future parents are %d and %d\n" % tuple(newps))