[PATCH 6 of 6 V2] histedit: migrate to scmutil.cleanupnodes (BC)

[Martin von Zweigbergk]

On Fri, Jul 7, 2017 at 7:11 PM, Jun Wu <quark at fb.com> wrote:
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -1542,44 +1549,19 @@ def processreplacement(state):
>      return final, tmpnodes, new, newtopmost
>
> -def movebookmarks(ui, repo, mapping, oldtopmost, newtopmost):
> -    """Move bookmark from old to newly created node"""
> -    if not mapping:
> -        # if nothing got rewritten there is not purpose for this function
> +def movebookmarks(repo, oldtopmost, newtopmost):
> +    """Move bookmark from oldtopmost to newly created topmost.
> +
> +    Most of the bookmark movement logic is handled by scmutil.cleanupnodes,
> +    only handle the special case here: topmost bookmark movement.
> +    """
> +    if not oldtopmost or not newtopmost:
>          return
> -    moves = []
> -    for bk, old in sorted(repo._bookmarks.iteritems()):
> -        if old == oldtopmost:
> -            # special case ensure bookmark stay on tip.
> -            #
> -            # This is arguably a feature and we may only want that for the
> -            # active bookmark. But the behavior is kept compatible with the old
> -            # version for now.
> -            moves.append((bk, newtopmost))
> -            continue
> -        base = old
> -        new = mapping.get(base, None)
> -        if new is None:
> -            continue
> -        while not new:
> -            # base is killed, trying with parent
> -            base = repo[base].p1().node()
> -            new = mapping.get(base, (base,))
> -            # nothing to move
> -        moves.append((bk, new[-1]))
> -    if moves:
> -        lock = tr = None
> -        try:
> -            lock = repo.lock()
> -            tr = repo.transaction('histedit')
> +    oldbmarks = repo.nodebookmarks(oldtopmost)
> +    if oldbmarks:
> +        with repo.transaction('histedit') as tr:
>              marks = repo._bookmarks
> -            for mark, new in moves:
> -                old = marks[mark]
> -                ui.note(_('histedit: moving bookmarks %s from %s to %s\n')
> -                        % (mark, node.short(old), node.short(new)))
> -                marks[mark] = new
> +            for name in oldbmarks:
> +                marks[name] = newtopmost
>              marks.recordchange(tr)
> -            tr.close()
> -        finally:
> -            release(tr, lock)

The locking was lost here. AFAICT, that has been unnecessary ever
since the transaction was introduced in 1168499e5266 (histedit: make
use of bookmarks.recordchange instead of bookmarks.write, 2015-11-20).
Removing the unnecessary locking and switching to the context manager
syntax seems unrelated to switching to cleanupnodes, so could you move
that out in the next version as well?

Btw, even with histedit.singletransaction=True, _finishhistedit()
(i.e. obsmarkers and bookmarks) are done outside of the transaction.
Maybe not intended? (I'm obviously not blaming that on your patch.)