This is an archive of the discontinued Mercurial Phabricator instance.

Differential D342

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

Authored by quark on Aug 11 2017, 2:13 AM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHG19f495fef0a3: rebase: change "result would have 3 parent" error message (BC)
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 move parent", which seems better.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

quark created this revision.Aug 11 2017, 2:13 AM
martinvonz added a subscriber: martinvonz.Aug 12 2017, 12:48 PM
martinvonz added inline comments.
hgext/rebase.py
1087–1096

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

quark planned changes to this revision.Aug 12 2017, 2:12 PM
quark added inline comments.
hgext/rebase.py
1087–1096

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.

quark added a comment.Aug 12 2017, 8:35 PM

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).

quark edited the summary of this revision. (Show Details)Aug 13 2017, 12:10 AM
quark updated this revision to Diff 831.
quark updated this revision to Diff 892.Aug 14 2017, 8:09 PM
martinvonz accepted this revision.Aug 15 2017, 12:31 AM
This revision is now accepted and ready to land.Aug 15 2017, 12:31 AM
Closed by commit rHG19f495fef0a3: rebase: change "result would have 3 parent" error message (BC) (authored by quark). · Explain WhyAug 15 2017, 12:33 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 905

hgext/rebase.py

# "rebasenode" updates to new p1, and the old p1 will be used as merge # "rebasenode" updates to new p1, and the old p1 will be used as merge
# base. If only p2 changes, merging using unchanged p1 as merge base is # base. If only p2 changes, merging using unchanged p1 as merge base is
# suboptimal. Therefore swap parents to make the merge sane. # suboptimal. Therefore swap parents to make the merge sane.
if newps[1] != nullrev and oldps[0] == newps[0]: if newps[1] != nullrev and oldps[0] == newps[0]:
assert len(newps) == 2 and len(oldps) == 2 assert len(newps) == 2 and len(oldps) == 2
newps.reverse() newps.reverse()
bases.reverse() bases.reverse()
# No parent change might be an error because we fail to make rev a # No parent change might be an error because we fail to make rev a
# descendent of requested dest. This can happen, for example: # descendent of requested dest. This can happen, for example:
# #
# C # rebase -r C -d D # C # rebase -r C -d D
# /| # None of A and B will be changed to D and rebase fails. # /| # None of A and B will be changed to D and rebase fails.
# A B D # A B D
if set(newps) == set(oldps) and dest not in newps: if set(newps) == set(oldps) and dest not in newps:
# The error message is for compatibility. It's a bit misleading raise error.Abort(_('cannot rebase %d:%s without '
# since rebase is not supposed to add new parents. 'moving at least one of its parents')
raise error.Abort(_('cannot use revision %d as base, ' % (rev, repo[rev]))
martinvonzUnsubmitted
Done

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

martinvonz: I feel like "Cannot rebase merge commit %d without rebasing at least one its parents" would be…
quarkAuthorUnsubmitted
Not Done

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.

quark: I like the new error message. Will change. A dry-run of rebase is not feasible until we have…
'result would have 3 parents') % rev)
repo.ui.debug(" future parents are %d and %d\n" % tuple(newps)) repo.ui.debug(" future parents are %d and %d\n" % tuple(newps))
# "rebasenode" updates to new p1, use the corresponding merge base. # "rebasenode" updates to new p1, use the corresponding merge base.
if bases[0] != nullrev: if bases[0] != nullrev:
base = bases[0] base = bases[0]
else: else:
base = None base = None

tests/test-rebase-brute-force.t

> |\| > |\|
> A B Z > A B Z
> EOS > EOS
$ hg debugbruterebase 'all()-Z' Z $ hg debugbruterebase 'all()-Z' Z
A: A':Z A: A':Z
B: B':Z B: B':Z
AB: A':Z B':Z AB: A':Z B':Z
C: ABORT: cannot use revision 3 as base, result would have 3 parents C: ABORT: cannot rebase 3:a35c07e8a2a4 without moving at least one of its parents
AC: A':Z C':A'B AC: A':Z C':A'B
BC: B':Z C':B'A BC: B':Z C':B'A
ABC: A':Z B':Z C':A'B' ABC: A':Z B':Z C':A'B'
D: D':Z D: D':Z
AD: A':Z D':Z AD: A':Z D':Z
BD: B':Z D':B' BD: B':Z D':B'
ABD: A':Z 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 rebase 3:a35c07e8a2a4 without moving at least one of its parents
ACD: A':Z C':A'B D':Z ACD: A':Z C':A'B D':Z
BCD: B':Z C':B'A D':B' BCD: B':Z C':B'A D':B'
ABCD: A':Z B':Z C':A'B' D':B' ABCD: A':Z B':Z C':A'B' D':B'
Moving backwards Moving backwards
$ init <<'EOS' $ init <<'EOS'
> C > C
> |\ > |\
> A B > A B
> | > |
> Z > Z
> EOS > EOS
$ hg debugbruterebase 'all()-Z' Z $ hg debugbruterebase 'all()-Z' Z
B: B':Z B: B':Z
A: A:
BA: B':Z BA: B':Z
C: ABORT: cannot use revision 3 as base, result would have 3 parents C: ABORT: cannot rebase 3:b8d7149b562b without moving at least one of its parents
BC: B':Z C':B'A BC: B':Z C':B'A
AC: AC:
BAC: B':Z C':B'A BAC: B':Z C':B'A

tests/test-rebase-obsolete.t

> EOF > EOF
$ hg rebase -d G -r 'B + D + F' $ hg rebase -d G -r 'B + D + F'
rebasing 1:112478962961 "B" (B) rebasing 1:112478962961 "B" (B)
rebasing 2:b18e25de2cf5 "D" (D) rebasing 2:b18e25de2cf5 "D" (D)
not rebasing ignored 4:26805aba1e60 "C" (C) not rebasing ignored 4:26805aba1e60 "C" (C)
not rebasing ignored 5:4b61ff5c62e2 "E" (E) not rebasing ignored 5:4b61ff5c62e2 "E" (E)
rebasing 6:f15c3adaf214 "F" (F tip) rebasing 6:f15c3adaf214 "F" (F tip)
abort: cannot use revision 6 as base, result would have 3 parents abort: cannot rebase 6:f15c3adaf214 without moving at least one of its parents
[255] [255]
$ cd .. $ cd ..
test on rebase dropping a merge test on rebase dropping a merge
(setup) (setup)

tests/test-rebase-scenario-global.t

$ hg rebase -s 5 -d 6 $ hg rebase -s 5 -d 6
abort: source is ancestor of destination abort: source is ancestor of destination
[255] [255]
G onto B - merge revision with both parents not in ancestors of target: G onto B - merge revision with both parents not in ancestors of target:
$ hg rebase -s 6 -d 1 $ hg rebase -s 6 -d 1
rebasing 6:eea13746799a "G" rebasing 6:eea13746799a "G"
abort: cannot use revision 6 as base, result would have 3 parents abort: cannot rebase 6:eea13746799a without moving at least one of its parents
[255] [255]
$ hg rebase --abort $ hg rebase --abort
rebase aborted rebase aborted
These will abort gracefully (using --base): These will abort gracefully (using --base):
G onto G - rebase onto same changeset: G onto G - rebase onto same changeset: