Patchwork use correct locking in histedit and cause 'invalid branchheads cache' warning

login
register
mail settings
Submitter Mads Kiilerich
Date April 17, 2013, 11:50 p.m.
Message ID <3f941a21632b4ff545ea.1366242600@xps>
Download mbox | patch
Permalink /patch/1405/
State Changes Requested, archived
Headers show

Comments

Mads Kiilerich - April 17, 2013, 11:50 p.m.
# HG changeset patch
# User Mads Kiilerich <madski@unity3d.com>
# Date 1366162894 -7200
# Node ID 3f941a21632b4ff545eadb43418744f2edcf0e4a
# Parent  341083b02d1be3fc707f27c360981bb00213dd07
use correct locking in histedit and cause 'invalid branchheads cache' warning

I don't know why it introduces a warning - it shouldn't.

'tip differs' is a very brief and (like other branchmap messages messages)
untranslated user-facing message. It comes from branchmap.read where all kinds
of exceptions are caught and mangled into warnings.
Matt Mackall - April 18, 2013, 3:41 a.m.
On Wed, 2013-04-17 at 22:15 -0500, Kevin Bullock wrote:
> On 17 Apr 2013, at 6:50 PM, Mads Kiilerich wrote:
> 
> > # HG changeset patch
> > # User Mads Kiilerich <madski@unity3d.com>
> > # Date 1366162894 -7200
> > # Node ID 3f941a21632b4ff545eadb43418744f2edcf0e4a
> > # Parent  341083b02d1be3fc707f27c360981bb00213dd07
> > use correct locking in histedit and cause 'invalid branchheads cache' warning
> > 
> > I don't know why it introduces a warning - it shouldn't.
> > 
> > 'tip differs' is a very brief and (like other branchmap messages messages)
> > untranslated user-facing message. It comes from branchmap.read where all kinds
> > of exceptions are caught and mangled into warnings.
> 
> It's a result of the branchcache collaboration changes from just before 2.5 and only shows up with --debug.

I can't tell if this patch is an RFC or what.
Mads Kiilerich - April 18, 2013, 11:20 a.m.
On 04/18/2013 05:41 AM, Matt Mackall wrote:
> On Wed, 2013-04-17 at 22:15 -0500, Kevin Bullock wrote:
>> On 17 Apr 2013, at 6:50 PM, Mads Kiilerich wrote:
>>
>>> # HG changeset patch
>>> # User Mads Kiilerich <madski@unity3d.com>
>>> # Date 1366162894 -7200
>>> # Node ID 3f941a21632b4ff545eadb43418744f2edcf0e4a
>>> # Parent  341083b02d1be3fc707f27c360981bb00213dd07
>>> use correct locking in histedit and cause 'invalid branchheads cache' warning
>>>
>>> I don't know why it introduces a warning - it shouldn't.
>>>
>>> 'tip differs' is a very brief and (like other branchmap messages messages)
>>> untranslated user-facing message. It comes from branchmap.read where all kinds
>>> of exceptions are caught and mangled into warnings.
>> It's a result of the branchcache collaboration changes from just before 2.5 and only shows up with --debug.
> I can't tell if this patch is an RFC or what.

It is a bad patch - primarily because it shows how adding innocent and 
correct locking has a strange side effect.

So I guess it was more of a bug report or 'request for research and fix' 
than a RFC.

/Mads

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -454,154 +454,157 @@  actiontable = {'p': pick,
 def histedit(ui, repo, *freeargs, **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()
 
-    # basic argument incompatibility processing
-    outg = opts.get('outgoing')
-    cont = opts.get('continue')
-    abort = opts.get('abort')
-    force = opts.get('force')
-    rules = opts.get('commands', '')
-    revs = opts.get('rev', [])
-    goal = 'new' # This invocation goal, in new, continue, abort
-    if force and not outg:
-        raise util.Abort(_('--force only allowed with --outgoing'))
-    if cont:
-        if util.any((outg, abort, revs, freeargs, rules)):
-            raise util.Abort(_('no arguments allowed with --continue'))
-        goal = 'continue'
-    elif abort:
-        if util.any((outg, revs, freeargs, rules)):
-            raise util.Abort(_('no arguments allowed with --abort'))
-        goal = 'abort'
-    else:
-        if os.path.exists(os.path.join(repo.path, 'histedit-state')):
-            raise util.Abort(_('history edit already in progress, try '
-                               '--continue or --abort'))
-        if outg:
-            if revs:
-                raise util.Abort(_('no revisions allowed with --outgoing'))
-            if len(freeargs) > 1:
-                raise util.Abort(
-                    _('only one repo argument allowed with --outgoing'))
+        # basic argument incompatibility processing
+        outg = opts.get('outgoing')
+        cont = opts.get('continue')
+        abort = opts.get('abort')
+        force = opts.get('force')
+        rules = opts.get('commands', '')
+        revs = opts.get('rev', [])
+        goal = 'new' # This invocation goal, in new, continue, abort
+        if force and not outg:
+            raise util.Abort(_('--force only allowed with --outgoing'))
+        if cont:
+            if util.any((outg, abort, revs, freeargs, rules)):
+                raise util.Abort(_('no arguments allowed with --continue'))
+            goal = 'continue'
+        elif abort:
+            if util.any((outg, revs, freeargs, rules)):
+                raise util.Abort(_('no arguments allowed with --abort'))
+            goal = 'abort'
         else:
-            revs.extend(freeargs)
-            if len(revs) != 1:
-                raise util.Abort(
-                    _('histedit requires exactly one parent revision'))
+            if os.path.exists(os.path.join(repo.path, 'histedit-state')):
+                raise util.Abort(_('history edit already in progress, try '
+                                   '--continue or --abort'))
+            if outg:
+                if revs:
+                    raise util.Abort(_('no revisions allowed with --outgoing'))
+                if len(freeargs) > 1:
+                    raise util.Abort(
+                        _('only one repo argument allowed with --outgoing'))
+            else:
+                revs.extend(freeargs)
+                if len(revs) != 1:
+                    raise util.Abort(
+                        _('histedit requires exactly one parent revision'))
 
 
-    if goal == '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 goal == '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)
+        if goal == '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 goal == '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)
+
+            topmost, empty = repo.dirstate.parents()
+            if outg:
+                if freeargs:
+                    remote = freeargs[0]
+                else:
+                    remote = None
+                root = findoutgoing(ui, repo, remote, force, opts)
+            else:
+                root = revs[0]
+                root = scmutil.revsingle(repo, root).node()
+
+            keep = opts.get('keep', False)
+            revs = between(repo, root, topmost, keep)
+            if not revs:
+                raise util.Abort(_('%s is not an ancestor of working '
+                                   'directory') % node.short(root))
+
+            ctxs = [repo[r] for r in revs]
+            if not rules:
+                rules = '\n'.join([makedesc(c) for c in ctxs])
+                rules += '\n\n'
+                rules += editcomment % (node.short(root), 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:
+                if rules == '-':
+                    f = sys.stdin
+                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[root].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)
-
-        topmost, empty = repo.dirstate.parents()
-        if outg:
-            if freeargs:
-                remote = freeargs[0]
-            else:
-                remote = None
-            root = findoutgoing(ui, repo, remote, force, opts)
-        else:
-            root = revs[0]
-            root = scmutil.revsingle(repo, root).node()
-
-        keep = opts.get('keep', False)
-        revs = between(repo, root, topmost, keep)
-        if not revs:
-            raise util.Abort(_('%s is not an ancestor of working directory') %
-                             node.short(root))
-
-        ctxs = [repo[r] for r in revs]
-        if not rules:
-            rules = '\n'.join([makedesc(c) for c in ctxs])
-            rules += '\n\n'
-            rules += editcomment % (node.short(root), 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:
-            if rules == '-':
-                f = sys.stdin
-            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[root].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'))
-
+        if os.path.exists(repo.sjoin('undo')):
+            os.unlink(repo.sjoin('undo'))
+    finally:
+        lockmod.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
@@ -97,6 +97,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