Patchwork histedit: hold wlock and lock while in progress

login
register
mail settings
Submitter Siddharth Agarwal
Date Nov. 17, 2013, 11:40 p.m.
Message ID <856587a47e937114e45e.1384731656@dev1091.prn1.facebook.com>
Download mbox | patch
Permalink /patch/3052/
State Accepted
Commit 4778f398ec83274fff9db1117f6cc75d6437eeef
Headers show

Comments

Siddharth Agarwal - Nov. 17, 2013, 11:40 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1384729869 28800
#      Sun Nov 17 15:11:09 2013 -0800
# Node ID 856587a47e937114e45e686dbbf172a789c33c64
# Parent  5e301ca90b26c64d2bfaa6f9e50007778c96df0a
histedit: hold wlock and lock while in progress

Currently, histedit acquires and releases lock and wlock several times during
its run. This isn't great because it allows other hg processes to come in and
change state. With this fix, lock and wlock are acquired and released exactly
once.

The change to test-histedit-drop.t is a minor implementation one -- the cache
is still correctly invalidated, but it just happens a little later and only
gets printed out because of the unrelated --debug flag.
Matt Mackall - Nov. 18, 2013, 5:16 p.m.
On Sun, 2013-11-17 at 15:40 -0800, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1384729869 28800
> #      Sun Nov 17 15:11:09 2013 -0800
> # Node ID 856587a47e937114e45e686dbbf172a789c33c64
> # Parent  5e301ca90b26c64d2bfaa6f9e50007778c96df0a
> histedit: hold wlock and lock while in progress

Does this belong on stable?

> Currently, histedit acquires and releases lock and wlock several times during
> its run. This isn't great because it allows other hg processes to come in and
> change state. With this fix, lock and wlock are acquired and released exactly
> once.
> 
> The change to test-histedit-drop.t is a minor implementation one -- the cache
> is still correctly invalidated, but it just happens a little later and only
> gets printed out because of the unrelated --debug flag.
> 
> diff --git a/hgext/histedit.py b/hgext/histedit.py
> --- a/hgext/histedit.py
> +++ b/hgext/histedit.py
> @@ -159,6 +159,7 @@
>  from mercurial import util
>  from mercurial import obsolete
>  from mercurial import merge as mergemod
> +from mercurial.lock import release
>  from mercurial.i18n import _
>  
>  cmdtable = {}
> @@ -476,6 +477,15 @@
>      for intentional "edit" command, but also for resolving unexpected
>      conflicts).
>      """
> +    lock = wlock = None
> +    try:
> +        wlock = repo.wlock()
> +        lock = repo.lock()
> +        _histedit(ui, repo, *freeargs, **opts)
> +    finally:
> +        release(lock, wlock)
> +
> +def _histedit(ui, repo, *freeargs, **opts):
>      # TODO only abort if we try and histedit mq patches, not just
>      # blanket if mq patches are applied somewhere
>      mq = getattr(repo, 'mq', None)
> 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 @@
>  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
Siddharth Agarwal - Nov. 18, 2013, 5:27 p.m.
On 11/18/2013 10:16 AM, Matt Mackall wrote:
> On Sun, 2013-11-17 at 15:40 -0800, Siddharth Agarwal wrote:
>> # HG changeset patch
>> # User Siddharth Agarwal <sid0@fb.com>
>> # Date 1384729869 28800
>> #      Sun Nov 17 15:11:09 2013 -0800
>> # Node ID 856587a47e937114e45e686dbbf172a789c33c64
>> # Parent  5e301ca90b26c64d2bfaa6f9e50007778c96df0a
>> histedit: hold wlock and lock while in progress
> Does this belong on stable?

It's a fix for a real bug, so I'd say so.
Matt Mackall - Nov. 18, 2013, 9:56 p.m.
On Mon, 2013-11-18 at 10:27 -0700, Siddharth Agarwal wrote:
> On 11/18/2013 10:16 AM, Matt Mackall wrote:
> > On Sun, 2013-11-17 at 15:40 -0800, Siddharth Agarwal wrote:
> >> # HG changeset patch
> >> # User Siddharth Agarwal <sid0@fb.com>
> >> # Date 1384729869 28800
> >> #      Sun Nov 17 15:11:09 2013 -0800
> >> # Node ID 856587a47e937114e45e686dbbf172a789c33c64
> >> # Parent  5e301ca90b26c64d2bfaa6f9e50007778c96df0a
> >> histedit: hold wlock and lock while in progress
> > Does this belong on stable?
> 
> It's a fix for a real bug, so I'd say so.

Ok, queued for stable.

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -159,6 +159,7 @@ 
 from mercurial import util
 from mercurial import obsolete
 from mercurial import merge as mergemod
+from mercurial.lock import release
 from mercurial.i18n import _
 
 cmdtable = {}
@@ -476,6 +477,15 @@ 
     for intentional "edit" command, but also for resolving unexpected
     conflicts).
     """
+    lock = wlock = None
+    try:
+        wlock = repo.wlock()
+        lock = repo.lock()
+        _histedit(ui, repo, *freeargs, **opts)
+    finally:
+        release(lock, wlock)
+
+def _histedit(ui, repo, *freeargs, **opts):
     # TODO only abort if we try and histedit mq patches, not just
     # blanket if mq patches are applied somewhere
     mq = getattr(repo, 'mq', None)
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 @@ 
 Check histedit_source
 
   $ hg log --debug --rev f518305ce889
+  invalid branchheads cache (visible): tip differs
   changeset:   4:f518305ce889c07cb5bd05522176d75590ef3324
   tag:         tip
   phase:       draft