This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7906

rebase: define base in only place in defineparents()
ClosedPublic

Authored by martinvonz on Jan 16 2020, 3:10 AM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG3d2de64c49d2: rebase: define base in only place in defineparents()
rHG0712c5e86b9c: rebase: define base in only place in defineparents()
Summary

Just a little refactoring to prepare for the next patch.

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

martinvonz created this revision.Jan 16 2020, 3:10 AM
pulkit added a subscriber: pulkit.Jan 27 2020, 9:40 AM
pulkit added inline comments.
hgext/rebase.py
1813

I am not sure why we are doing bases[i] = bases[0] here? (TBH I didn't try to understand the whole logic, mostly only the diff)

martinvonz added inline comments.Jan 27 2020, 9:43 AM
hgext/rebase.py
1813

Doing it only for consistency with newps. bases and newps should be kept in sync (we don't do that in one other place, but that's pretty weird, IMO). So I think it's clearer this way even though it's unnecessary.

martinvonz updated this revision to Diff 19698.Jan 29 2020, 7:56 PM
pulkit accepted this revision.Jan 30 2020, 9:36 AM
This revision is now accepted and ready to land.Jan 30 2020, 9:36 AM
martinvonz added a commit: rHG0712c5e86b9c: rebase: define base in only place in defineparents().Jan 30 2020, 10:38 AM
Closed by commit rHG0712c5e86b9c: rebase: define base in only place in defineparents() (authored by martinvonz). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/rebase.py (20 lines)

Diff 19720

hgext/rebase.py

) )
# Source should not be ancestor of dest. The check here guarantees it's # Source should not be ancestor of dest. The check here guarantees it's
# impossible. With multi-dest, the initial check does not cover complex # impossible. With multi-dest, the initial check does not cover complex
# cases since we don't have abstractions to dry-run rebase cheaply. # cases since we don't have abstractions to dry-run rebase cheaply.
if any(p != nullrev and isancestor(rev, p) for p in newps): if any(p != nullrev and isancestor(rev, p) for p in newps):
raise error.Abort(_(b'source is ancestor of destination')) raise error.Abort(_(b'source is ancestor of destination'))
# "rebasenode" updates to new p1, use the corresponding merge base.
if bases[0] != nullrev:
base = bases[0]
else:
base = None
# Check if the merge will contain unwanted changes. That may happen if # Check if the merge will contain unwanted changes. That may happen if
# there are multiple special (non-changelog ancestor) merge bases, which # there are multiple special (non-changelog ancestor) merge bases, which
# cannot be handled well by the 3-way merge algorithm. For example: # cannot be handled well by the 3-way merge algorithm. For example:
# #
# F # F
# /| # /|
# D E # "rebase -r D+E+F -d Z", when rebasing F, if "D" was chosen # D E # "rebase -r D+E+F -d Z", when rebasing F, if "D" was chosen
# | | # as merge base, the difference between D and F will include # | | # as merge base, the difference between D and F will include
if revs is not None if revs is not None
) )
) )
raise error.Abort( raise error.Abort(
_(b'rebasing %d:%s will include unwanted changes from %s') _(b'rebasing %d:%s will include unwanted changes from %s')
% (rev, repo[rev], unwanteddesc) % (rev, repo[rev], unwanteddesc)
) )
base = bases[i]
# newps[0] should match merge base if possible. Currently, if newps[i] # newps[0] should match merge base if possible. Currently, if newps[i]
# is nullrev, the only case is newps[i] and newps[j] (j < i), one is # is nullrev, the only case is newps[i] and newps[j] (j < i), one is
# the other's ancestor. In that case, it's fine to not swap newps here. # the other's ancestor. In that case, it's fine to not swap newps here.
# (see CASE-1 and CASE-2 above) # (see CASE-1 and CASE-2 above)
if i != 0 and newps[i] != nullrev: if i != 0:
if newps[i] != nullrev:
newps[0], newps[i] = newps[i], newps[0] newps[0], newps[i] = newps[i], newps[0]
bases[0], bases[i] = bases[i], bases[0]
pulkitUnsubmitted
Not Done

I am not sure why we are doing bases[i] = bases[0] here? (TBH I didn't try to understand the whole logic, mostly only the diff)

pulkit: I am not sure why we are doing `bases[i] = bases[0]` here? (TBH I didn't try to understand the…
martinvonzAuthorUnsubmitted
Done

Doing it only for consistency with newps. bases and newps should be kept in sync (we don't do that in one other place, but that's pretty weird, IMO). So I think it's clearer this way even though it's unnecessary.

martinvonz: Doing it only for consistency with `newps`. `bases` and `newps` should be kept in sync (we…
# "rebasenode" updates to new p1, use the corresponding merge base.
if bases[0] != nullrev:
base = bases[0]
else:
base = None
repo.ui.debug(b" future parents are %d and %d\n" % tuple(newps)) repo.ui.debug(b" future parents are %d and %d\n" % tuple(newps))
return newps[0], newps[1], base return newps[0], newps[1], base
def isagitpatch(repo, patchname): def isagitpatch(repo, patchname):
"""Return true if the given patch is in git format""" """Return true if the given patch is in git format"""