Patchwork [4,of,6] histedit: lock repo while editing

# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1366162894 -7200
# Node ID 32a7b55e7d9f4052b8a4cfeccc3c7298e122fa85
# Parent  50b42260018a4739637cde911f42c04ecd402c88
histedit: lock repo while editing

Just adding locking introduces an 'invalid branchheads cache' message. That
seems like a strange error elsewhere.

On 04/17/2013 04:11 AM, Mads Kiilerich wrote:
> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1366162894 -7200
> # Node ID 32a7b55e7d9f4052b8a4cfeccc3c7298e122fa85
> # Parent  50b42260018a4739637cde911f42c04ecd402c88
> histedit: lock repo while editing
>
> Just adding locking introduces an 'invalid branchheads cache' message. That
> seems like a strange error elsewhere.

Understanding this issue is probably more important than adding the 
extra locking.

(We have several places where commands essentially do 
read-check-lock-write-unlock. Commands are thus not fully reliable ... 
but adding more locks makes this and concurrent commands more reliable.)

/Mads

This one will need a resend in any case, since I'm pushing marmoute's very helpful refactoring of the histedit code.

On Apr 16, 2013, at 10:11 PM, Mads Kiilerich <mads@kiilerich.com> wrote:

> # HG changeset patch
> # User Mads Kiilerich <madski@unity3d.com>
> # Date 1366162894 -7200
> # Node ID 32a7b55e7d9f4052b8a4cfeccc3c7298e122fa85
> # Parent  50b42260018a4739637cde911f42c04ecd402c88
> histedit: lock repo while editing
> 
> Just adding locking introduces an 'invalid branchheads cache' message. That
> seems like a strange error elsewhere.
> 
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -158,6 +158,7 @@ from mercurial import util
> from mercurial import obsolete
> from mercurial import merge as mergemod
> from mercurial.i18n import _
> +from mercurial.lock import release
> 
> cmdtable = {}
> command = cmdutil.command(cmdtable)
> @@ -430,147 +431,159 @@ actiontable = {'p': pick,
> def histedit(ui, repo, *parent, **opts):
>     """interactively edit changeset history
>     """
> -    # TODO only abort if we try and histedit mq patches, not just
> -    # blanket if mq patches are applied somewhere
> -    mq = getattr(repo, 'mq', None)
> -    if mq and mq.applied:
> -        raise util.Abort(_('source has mq patches applied'))
> +    wlock = lock = None
> +    try:
> +        wlock = repo.wlock()
> +        lock = repo.lock()
> 
> -    parent = list(parent) + opts.get('rev', [])
> -    if opts.get('outgoing'):
> -        if len(parent) > 1:
> -            raise util.Abort(
> -                _('only one repo argument allowed with --outgoing'))
> -        elif parent:
> -            parent = parent[0]
> +        # TODO only abort if we try and histedit mq patches, not just
> +        # blanket if mq patches are applied somewhere
> +        mq = getattr(repo, 'mq', None)
> +        if mq and mq.applied:
> +            raise util.Abort(_('source has mq patches applied'))
> 
> -        dest = ui.expandpath(parent or 'default-push', parent or 'default')
> -        dest, revs = hg.parseurl(dest, None)[:2]
> -        ui.status(_('comparing with %s\n') % util.hidepassword(dest))
> +        parent = list(parent) + opts.get('rev', [])
> +        if opts.get('outgoing'):
> +            if len(parent) > 1:
> +                raise util.Abort(
> +                    _('only one repo argument allowed with --outgoing'))
> +            elif parent:
> +                parent = parent[0]
> 
> -        revs, checkout = hg.addbranchrevs(repo, repo, revs, None)
> -        other = hg.peer(repo, opts, dest)
> +            dest = ui.expandpath(parent or 'default-push', parent or 'default')
> +            dest, revs = hg.parseurl(dest, None)[:2]
> +            ui.status(_('comparing with %s\n') % util.hidepassword(dest))
> 
> -        if revs:
> -            revs = [repo.lookup(rev) for rev in revs]
> +            revs, checkout = hg.addbranchrevs(repo, repo, revs, None)
> +            other = hg.peer(repo, opts, dest)
> 
> -        # hexlify nodes from outgoing, because we're going to parse
> -        # parent[0] using revsingle below, and if the binary hash
> -        # contains special revset characters like ":" the revset
> -        # parser can choke.
> -        parent = [node.hex(n) for n in discovery.findcommonoutgoing(
> -            repo, other, revs, force=opts.get('force')).missing[0:1]]
> -    else:
> -        if opts.get('force'):
> -            raise util.Abort(_('--force only allowed with --outgoing'))
> +            if revs:
> +                revs = [repo.lookup(rev) for rev in revs]
> 
> -    if opts.get('continue', False):
> -        if len(parent) != 0:
> -            raise util.Abort(_('no arguments allowed with --continue'))
> -        (parentctxnode, rules, keep, topmost, replacements) = readstate(repo)
> -        currentparent, wantnull = repo.dirstate.parents()
> -        parentctx = repo[parentctxnode]
> -        parentctx, repl = bootstrapcontinue(ui, repo, parentctx, rules, opts)
> -        replacements.extend(repl)
> -    elif opts.get('abort', False):
> -        if len(parent) != 0:
> -            raise util.Abort(_('no arguments allowed with --abort'))
> -        (parentctxnode, rules, keep, topmost, replacements) = readstate(repo)
> -        mapping, tmpnodes, leafs, _ntm = processreplacement(repo, replacements)
> -        ui.debug('restore wc to old parent %s\n' % node.short(topmost))
> -        hg.clean(repo, topmost)
> -        cleanupnode(ui, repo, 'created', tmpnodes)
> -        cleanupnode(ui, repo, 'temp', leafs)
> +            # hexlify nodes from outgoing, because we're going to parse
> +            # parent[0] using revsingle below, and if the binary hash
> +            # contains special revset characters like ":" the revset
> +            # parser can choke.
> +            parent = [node.hex(n) for n in discovery.findcommonoutgoing(
> +                repo, other, revs, force=opts.get('force')).missing[0:1]]
> +        else:
> +            if opts.get('force'):
> +                raise util.Abort(_('--force only allowed with --outgoing'))
> +
> +        if opts.get('continue', False):
> +            if len(parent) != 0:
> +                raise util.Abort(_('no arguments allowed with --continue'))
> +            (parentctxnode, rules, keep, topmost, replacements
> +             ) = readstate(repo)
> +            currentparent, wantnull = repo.dirstate.parents()
> +            parentctx = repo[parentctxnode]
> +            parentctx, repl = bootstrapcontinue(ui, repo, parentctx, rules,
> +                                                opts)
> +            replacements.extend(repl)
> +        elif opts.get('abort', False):
> +            if len(parent) != 0:
> +                raise util.Abort(_('no arguments allowed with --abort'))
> +            (parentctxnode, rules, keep, topmost, replacements
> +             ) = readstate(repo)
> +            mapping, tmpnodes, leafs, _ntm = processreplacement(repo,
> +                                                                replacements)
> +            ui.debug('restore wc to old parent %s\n' % node.short(topmost))
> +            hg.clean(repo, topmost)
> +            cleanupnode(ui, repo, 'created', tmpnodes)
> +            cleanupnode(ui, repo, 'temp', leafs)
> +            os.unlink(os.path.join(repo.path, 'histedit-state'))
> +            return
> +        else:
> +            cmdutil.bailifchanged(repo)
> +            if os.path.exists(os.path.join(repo.path, 'histedit-state')):
> +                raise util.Abort(_('history edit already in progress, try '
> +                                   '--continue or --abort'))
> +
> +            topmost, empty = repo.dirstate.parents()
> +
> +            if len(parent) != 1:
> +                raise util.Abort(_('histedit requires exactly one parent '
> +                                   'revision'))
> +            parent = scmutil.revsingle(repo, parent[0]).node()
> +
> +            keep = opts.get('keep', False)
> +            revs = between(repo, parent, topmost, keep)
> +            if not revs:
> +                raise util.Abort(_('%s is not an ancestor of working '
> +                                   'directory') % node.short(parent))
> +
> +            ctxs = [repo[r] for r in revs]
> +            rules = opts.get('commands', '')
> +            if not rules:
> +                rules = '\n'.join([makedesc(c) for c in ctxs])
> +                rules += '\n\n'
> +                rules += editcomment % (node.short(parent), node.short(topmost))
> +                rules = ui.edit(rules, ui.username())
> +                # Save edit rules in .hg/histedit-last-edit.txt in case
> +                # the user needs to ask for help after something
> +                # surprising happens.
> +                f = open(repo.join('histedit-last-edit.txt'), 'w')
> +                f.write(rules)
> +                f.close()
> +            else:
> +                f = open(rules)
> +                rules = f.read()
> +                f.close()
> +            rules = [l for l in (r.strip() for r in rules.splitlines())
> +                     if l and not l[0] == '#']
> +            rules = verifyrules(rules, repo, ctxs)
> +
> +            parentctx = repo[parent].parents()[0]
> +            keep = opts.get('keep', False)
> +            replacements = []
> +
> +        while rules:
> +            writestate(repo, parentctx.node(), rules, keep, topmost,
> +                       replacements)
> +            action, ha = rules.pop(0)
> +            ui.debug('histedit: processing %s %s\n' % (action, ha))
> +            actfunc = actiontable[action]
> +            parentctx, replacement_ = actfunc(ui, repo, parentctx, ha, opts)
> +            replacements.extend(replacement_)
> +
> +        hg.update(repo, parentctx.node())
> +
> +        mapping, tmpnodes, created, ntm = processreplacement(repo, replacements)
> +        if mapping:
> +            for prec, succs in mapping.iteritems():
> +                if not succs:
> +                    ui.debug('histedit: %s is dropped\n' % node.short(prec))
> +                else:
> +                    ui.debug('histedit: %s is replaced by %s\n' % (
> +                        node.short(prec), node.short(succs[0])))
> +                    if len(succs) > 1:
> +                        m = 'histedit:                            %s'
> +                        for n in succs[1:]:
> +                            ui.debug(m % node.short(n))
> +
> +        if not keep:
> +            if mapping:
> +                movebookmarks(ui, repo, mapping, topmost, ntm)
> +                # TODO update mq state
> +            if obsolete._enabled:
> +                markers = []
> +                # sort by revision number because it sound "right"
> +                for prec in sorted(mapping, key=repo.changelog.rev):
> +                    succs = mapping[prec]
> +                    markers.append((repo[prec],
> +                                    tuple(repo[s] for s in succs)))
> +                if markers:
> +                    obsolete.createmarkers(repo, markers)
> +            else:
> +                cleanupnode(ui, repo, 'replaced', mapping)
> +
> +        cleanupnode(ui, repo, 'temp', tmpnodes)
>         os.unlink(os.path.join(repo.path, 'histedit-state'))
> -        return
> -    else:
> -        cmdutil.bailifchanged(repo)
> -        if os.path.exists(os.path.join(repo.path, 'histedit-state')):
> -            raise util.Abort(_('history edit already in progress, try '
> -                               '--continue or --abort'))
> +        if os.path.exists(repo.sjoin('undo')):
> +            os.unlink(repo.sjoin('undo'))
> 
> -        topmost, empty = repo.dirstate.parents()
> -
> -        if len(parent) != 1:
> -            raise util.Abort(_('histedit requires exactly one parent revision'))
> -        parent = scmutil.revsingle(repo, parent[0]).node()
> -
> -        keep = opts.get('keep', False)
> -        revs = between(repo, parent, topmost, keep)
> -        if not revs:
> -            raise util.Abort(_('%s is not an ancestor of working directory') %
> -                             node.short(parent))
> -
> -        ctxs = [repo[r] for r in revs]
> -        rules = opts.get('commands', '')
> -        if not rules:
> -            rules = '\n'.join([makedesc(c) for c in ctxs])
> -            rules += '\n\n'
> -            rules += editcomment % (node.short(parent), node.short(topmost))
> -            rules = ui.edit(rules, ui.username())
> -            # Save edit rules in .hg/histedit-last-edit.txt in case
> -            # the user needs to ask for help after something
> -            # surprising happens.
> -            f = open(repo.join('histedit-last-edit.txt'), 'w')
> -            f.write(rules)
> -            f.close()
> -        else:
> -            f = open(rules)
> -            rules = f.read()
> -            f.close()
> -        rules = [l for l in (r.strip() for r in rules.splitlines())
> -                 if l and not l[0] == '#']
> -        rules = verifyrules(rules, repo, ctxs)
> -
> -        parentctx = repo[parent].parents()[0]
> -        keep = opts.get('keep', False)
> -        replacements = []
> -
> -
> -    while rules:
> -        writestate(repo, parentctx.node(), rules, keep, topmost, replacements)
> -        action, ha = rules.pop(0)
> -        ui.debug('histedit: processing %s %s\n' % (action, ha))
> -        actfunc = actiontable[action]
> -        parentctx, replacement_ = actfunc(ui, repo, parentctx, ha, opts)
> -        replacements.extend(replacement_)
> -
> -    hg.update(repo, parentctx.node())
> -
> -    mapping, tmpnodes, created, ntm = processreplacement(repo, replacements)
> -    if mapping:
> -        for prec, succs in mapping.iteritems():
> -            if not succs:
> -                ui.debug('histedit: %s is dropped\n' % node.short(prec))
> -            else:
> -                ui.debug('histedit: %s is replaced by %s\n' % (
> -                    node.short(prec), node.short(succs[0])))
> -                if len(succs) > 1:
> -                    m = 'histedit:                            %s'
> -                    for n in succs[1:]:
> -                        ui.debug(m % node.short(n))
> -
> -    if not keep:
> -        if mapping:
> -            movebookmarks(ui, repo, mapping, topmost, ntm)
> -            # TODO update mq state
> -        if obsolete._enabled:
> -            markers = []
> -            # sort by revision number because it sound "right"
> -            for prec in sorted(mapping, key=repo.changelog.rev):
> -                succs = mapping[prec]
> -                markers.append((repo[prec],
> -                                tuple(repo[s] for s in succs)))
> -            if markers:
> -                obsolete.createmarkers(repo, markers)
> -        else:
> -            cleanupnode(ui, repo, 'replaced', mapping)
> -
> -    cleanupnode(ui, repo, 'temp', tmpnodes)
> -    os.unlink(os.path.join(repo.path, 'histedit-state'))
> -    if os.path.exists(repo.sjoin('undo')):
> -        os.unlink(repo.sjoin('undo'))
> -
> +    finally:
> +        release(lock, wlock)
> 
> def bootstrapcontinue(ui, repo, parentctx, rules, opts):
>     action, currentnode = rules.pop(0)
> diff --git a/tests/test-histedit-drop.t b/tests/test-histedit-drop.t
> --- a/tests/test-histedit-drop.t
> +++ b/tests/test-histedit-drop.t
> @@ -99,6 +99,7 @@ log after edit
> Check histedit_source
> 
>   $ hg log --debug --rev f518305ce889
> +  invalid branchheads cache (visible): tip differs
>   changeset:   4:f518305ce889c07cb5bd05522176d75590ef3324
>   tag:         tip
>   phase:       draft
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel