This is an archive of the discontinued Mercurial Phabricator instance.

Differential D748

cleanupnodes: separate out bookmark destination calculation from actual update
ClosedPublic

Authored by martinvonz on Sep 20 2017, 12:55 PM.

Details

Reviewers
quark
durin42
Group Reviewers
hg-reviewers
Commits
rHG033a5befbaf7: cleanupnodes: separate out bookmark destination calculation from actual update
Summary

We will soon want to pass in overrides for bookmark movements and this
will make that patch simpler. I also think this makes the code easier
to follow regardless of the later patch.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Sep 20 2017, 12:55 PM
quark accepted this revision.Sep 21 2017, 6:06 PM
quark added a subscriber: quark.

I noticed the change from repo to unfi. That actually looks better. Since we are not querying "descendants", it won't cause correctness issues.

mercurial/scmutil.py
646

This could be removed then.

martinvonz marked an inline comment as done.Sep 21 2017, 6:11 PM
In D748#12944, @quark wrote:

I noticed the change from repo to unfi. That actually looks better. Since we are not querying "descendants", it won't cause correctness issues.

Yes, that was necessary because oldnode could be obsolete and that was previously protected by "if not oldbmarks", which I didn't think would belong in the new loop.

martinvonz updated this revision to Diff 1985.Sep 21 2017, 6:14 PM
quark requested changes to this revision.Sep 21 2017, 6:19 PM
In D748#12948, @martinvonz wrote:

Yes, that was necessary because oldnode could be obsolete and that was previously protected by "if not oldbmarks", which I didn't think would belong in the new loop.

Ah, I didn't realize that minor difference. So the new code could have unnecessary computation - it calculates "moves" for every node even if there is no move needed for it.

I think we still want an extra moves argument, but use it inside the for loop (so the if ...: continue shortcut works).

This revision now requires changes to proceed.Sep 21 2017, 6:19 PM
quark added a comment.Sep 21 2017, 6:25 PM

I think either you or me can update the code. If you're busy with other things, I can update (and split) the original fix directly.

martinvonz added a comment.Sep 21 2017, 6:28 PM
In D748#12953, @quark wrote:
In D748#12948, @martinvonz wrote:

Yes, that was necessary because oldnode could be obsolete and that was previously protected by "if not oldbmarks", which I didn't think would belong in the new loop.

Ah, I didn't realize that minor difference. So the new code could have unnecessary computation - it calculates "moves" for every node even if there is no move needed for it.

Yes. I simply didn't think it would be a noticeable extra computation. For working copy movement that's coming in your later patch, I also assumed that would just do "newwc = moves[oldwc]" (simplified), but I now see that it looks more complex.

I think we still want an extra moves argument, but use it inside the for loop (so the if ...: continue shortcut works).

I'd rather move the " if not oldbmarks:" to the new loop if you think that's necessary.

quark accepted this revision.Sep 21 2017, 6:53 PM
quark added inline comments.
mercurial/scmutil.py
604

I thought this could be O(changelog), but it's O(1) given generatorset implicitly requires the generator to be sorted.

durin42 accepted this revision.Sep 30 2017, 9:09 AM
This revision is now accepted and ready to land.Sep 30 2017, 9:09 AM
martinvonz updated this revision to Diff 2196.Sep 30 2017, 10:16 AM
Closed by commit rHG033a5befbaf7: cleanupnodes: separate out bookmark destination calculation from actual update (authored by martinvonz). · Explain WhySep 30 2017, 10:27 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/scmutil.py (34 lines)

Diff 1936

mercurial/scmutil.py

(we might also want to move working directory parent in the future) (we might also want to move working directory parent in the future)
mapping is {oldnode: [newnode]} or a iterable of nodes if they do not have mapping is {oldnode: [newnode]} or a iterable of nodes if they do not have
replacements. operation is a string, like "rebase". replacements. operation is a string, like "rebase".
""" """
if not util.safehasattr(mapping, 'items'): if not util.safehasattr(mapping, 'items'):
mapping = {n: () for n in mapping} mapping = {n: () for n in mapping}
with repo.transaction('cleanup') as tr: # Calculate bookmark movements
# Move bookmarks moves = {}
bmarks = repo._bookmarks unfi = repo.unfiltered()
bmarkchanges = []
allnewnodes = [n for ns in mapping.values() for n in ns]
for oldnode, newnodes in mapping.items(): for oldnode, newnodes in mapping.items():
oldbmarks = repo.nodebookmarks(oldnode)
if not oldbmarks:
continue
from . import bookmarks # avoid import cycle
if len(newnodes) > 1: if len(newnodes) > 1:
# usually a split, take the one with biggest rev number # usually a split, take the one with biggest rev number
newnode = next(repo.set('max(%ln)', newnodes)).node() newnode = next(unfi.set('max(%ln)', newnodes)).node()
elif len(newnodes) == 0: elif len(newnodes) == 0:
# move bookmark backwards # move bookmark backwards
roots = list(repo.set('max((::%n) - %ln)', oldnode, roots = list(unfi.set('max((::%n) - %ln)', oldnode,
list(mapping))) list(mapping)))
quarkUnsubmitted
Not Done

I thought this could be O(changelog), but it's O(1) given generatorset implicitly requires the generator to be sorted.

quark: I thought this could be `O(changelog)`, but it's `O(1)` given generatorset implicitly requires…
if roots: if roots:
newnode = roots[0].node() newnode = roots[0].node()
else: else:
newnode = nullid newnode = nullid
else: else:
newnode = newnodes[0] newnode = newnodes[0]
moves[oldnode] = newnode
with repo.transaction('cleanup') as tr:
# Move bookmarks
bmarks = repo._bookmarks
bmarkchanges = []
allnewnodes = [n for ns in mapping.values() for n in ns]
for oldnode, newnode in moves.items():
oldbmarks = repo.nodebookmarks(oldnode)
if not oldbmarks:
continue
from . import bookmarks # avoid import cycle
repo.ui.debug('moving bookmarks %r from %s to %s\n' % repo.ui.debug('moving bookmarks %r from %s to %s\n' %
(oldbmarks, hex(oldnode), hex(newnode))) (oldbmarks, hex(oldnode), hex(newnode)))
# Delete divergent bookmarks being parents of related newnodes # Delete divergent bookmarks being parents of related newnodes
deleterevs = repo.revs('parents(roots(%ln & (::%n))) - parents(%n)', deleterevs = repo.revs('parents(roots(%ln & (::%n))) - parents(%n)',
allnewnodes, newnode, oldnode) allnewnodes, newnode, oldnode)
deletenodes = _containsnode(repo, deleterevs) deletenodes = _containsnode(repo, deleterevs)
for name in oldbmarks: for name in oldbmarks:
bmarkchanges.append((name, newnode)) bmarkchanges.append((name, newnode))
for b in bookmarks.divergent2delete(repo, deletenodes, name): for b in bookmarks.divergent2delete(repo, deletenodes, name):
bmarkchanges.append((b, None)) bmarkchanges.append((b, None))
if bmarkchanges: if bmarkchanges:
bmarks.applychanges(repo, tr, bmarkchanges) bmarks.applychanges(repo, tr, bmarkchanges)
# Obsolete or strip nodes # Obsolete or strip nodes
if obsolete.isenabled(repo, obsolete.createmarkersopt): if obsolete.isenabled(repo, obsolete.createmarkersopt):
# If a node is already obsoleted, and we want to obsolete it # If a node is already obsoleted, and we want to obsolete it
# without a successor, skip that obssolete request since it's # without a successor, skip that obssolete request since it's
# unnecessary. That's the "if s or not isobs(n)" check below. # unnecessary. That's the "if s or not isobs(n)" check below.
# Also sort the node in topology order, that might be useful for # Also sort the node in topology order, that might be useful for
# some obsstore logic. # some obsstore logic.
# NOTE: the filtering and sorting might belong to createmarkers. # NOTE: the filtering and sorting might belong to createmarkers.
# Unfiltered repo is needed since nodes in mapping might be hidden. # Unfiltered repo is needed since nodes in mapping might be hidden.
unfi = repo.unfiltered() unfi = repo.unfiltered()
quarkUnsubmitted
Done

This could be removed then.

quark: This could be removed then.
isobs = unfi.obsstore.successors.__contains__ isobs = unfi.obsstore.successors.__contains__
torev = unfi.changelog.rev torev = unfi.changelog.rev
sortfunc = lambda ns: torev(ns[0]) sortfunc = lambda ns: torev(ns[0])
rels = [(unfi[n], tuple(unfi[m] for m in s)) rels = [(unfi[n], tuple(unfi[m] for m in s))
for n, s in sorted(mapping.items(), key=sortfunc) for n, s in sorted(mapping.items(), key=sortfunc)
if s or not isobs(n)] if s or not isobs(n)]
obsolete.createmarkers(repo, rels, operation=operation) obsolete.createmarkers(repo, rels, operation=operation)
else: else: