This is an archive of the discontinued Mercurial Phabricator instance.

Differential D347

rebase: rewrite _computeobsoletenotrebased
ClosedPublic

Authored by quark on Aug 11 2017, 5:01 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGba9d5d48bf95: rebase: rewrite _computeobsoletenotrebased
Summary

The old code stores successors of all related nodes together, which works
fine if destination is unique. A future patch would make destination
non-unique so let's change the implementation to test successors for
rebaseset separately.

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, 5:01 AM
quark updated this revision to Diff 835.Aug 13 2017, 12:28 AM
quark updated this revision to Diff 899.Aug 14 2017, 8:11 PM
quark updated this revision to Diff 1146.Aug 22 2017, 1:16 AM
martinvonz added a subscriber: martinvonz.Aug 29 2017, 2:16 PM
martinvonz added inline comments.
hgext/rebase.py
1494

Oh, allsuccessors() includes "srcnode" in the output! It took me a while to figure that out. A comment would help.

How about something like this?

successors = list(obsutil.allsuccessors(repo.obsstore, [srcnode]))
successors = successors[1:] # allsuccessors() includes the start nodes
if not successors:
    # prune
...
quark added inline comments.Aug 29 2017, 3:11 PM
hgext/rebase.py
1494

That copies the list therefore slightly less performant. I'll add a comment.

quark updated this revision to Diff 1416.Aug 29 2017, 9:40 PM
martinvonz added inline comments.Aug 30 2017, 12:18 AM
hgext/rebase.py
1495

"returns node itself if it finds obsmarkers" is technically correct, but but also returns it if there are no obsmarkers :-)

However, "When the list only contains one element, it means there is at least one obsmarker with srcnode as predecessor" is not correct; it does not mean that there is at least one obsmarker, it means that there are no successors.

Or did I misunderstand either the text or what allsuccessors() does?

I'll rephrase it in flight unless you tell me soon that I misunderstood.

Closed by commit rHGba9d5d48bf95: rebase: rewrite _computeobsoletenotrebased (authored by quark). · Explain WhyAug 30 2017, 12:53 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/rebase.py (43 lines)

Diff 1424

hgext/rebase.py

def _computeobsoletenotrebased(repo, rebaseobsrevs, dest): def _computeobsoletenotrebased(repo, rebaseobsrevs, dest):
"""return a mapping obsolete => successor for all obsolete nodes to be """return a mapping obsolete => successor for all obsolete nodes to be
rebased that have a successors in the destination rebased that have a successors in the destination
obsolete => None entries in the mapping indicate nodes with no successor""" obsolete => None entries in the mapping indicate nodes with no successor"""
obsoletenotrebased = {} obsoletenotrebased = {}
# Build a mapping successor => obsolete nodes for the obsolete cl = repo.unfiltered().changelog
# nodes to be rebased nodemap = cl.nodemap
allsuccessors = {} destnode = cl.node(dest)
cl = repo.changelog for srcrev in rebaseobsrevs:
for r in rebaseobsrevs: srcnode = cl.node(srcrev)
node = cl.node(r) # XXX: more advanced APIs are required to handle split correctly
for s in obsutil.allsuccessors(repo.obsstore, [node]): successors = list(obsutil.allsuccessors(repo.obsstore, [srcnode]))
try: if len(successors) == 1:
martinvonzUnsubmitted
Not Done

Oh, allsuccessors() includes "srcnode" in the output! It took me a while to figure that out. A comment would help.

How about something like this?

successors = list(obsutil.allsuccessors(repo.obsstore, [srcnode]))
successors = successors[1:] # allsuccessors() includes the start nodes
if not successors:
    # prune
...
martinvonz: Oh, allsuccessors() includes "srcnode" in the output! It took me a while to figure that out. A…
quarkAuthorUnsubmitted
Not Done

That copies the list therefore slightly less performant. I'll add a comment.

quark: That copies the list therefore slightly less performant. I'll add a comment.
allsuccessors[cl.rev(s)] = cl.rev(node) # obsutil.allsuccessors includes node itself. When the list only
martinvonzUnsubmitted
Not Done

"returns node itself if it finds obsmarkers" is technically correct, but but also returns it if there are no obsmarkers :-)

However, "When the list only contains one element, it means there is at least one obsmarker with srcnode as predecessor" is not correct; it does not mean that there is at least one obsmarker, it means that there are no successors.

Or did I misunderstand either the text or what allsuccessors() does?

I'll rephrase it in flight unless you tell me soon that I misunderstood.

martinvonz: "returns node itself if it finds obsmarkers" is technically correct, but but also returns it if…
except LookupError: # contains one element, it means there are no successors.
pass obsoletenotrebased[srcrev] = None
else:
if allsuccessors: for succnode in successors:
# Look for successors of obsolete nodes to be rebased among if succnode == srcnode or succnode not in nodemap:
# the ancestors of dest continue
ancs = cl.ancestors([dest], if cl.isancestor(succnode, destnode):
stoprev=min(allsuccessors), obsoletenotrebased[srcrev] = nodemap[succnode]
inclusive=True) break
for s in allsuccessors:
if s in ancs:
obsoletenotrebased[allsuccessors[s]] = s
elif (s == allsuccessors[s] and
allsuccessors.values().count(s) == 1):
# plain prune
obsoletenotrebased[s] = None
return obsoletenotrebased return obsoletenotrebased
def summaryhook(ui, repo): def summaryhook(ui, repo):
if not repo.vfs.exists('rebasestate'): if not repo.vfs.exists('rebasestate'):
return return
try: try:
rbsrt = rebaseruntime(repo, ui, {}) rbsrt = rebaseruntime(repo, ui, {})