Patchwork [1,of,2] wlock: only issue devel warning when actually acquiring the lock

login
register
mail settings
Submitter Pierre-Yves David
Date April 12, 2015, 5:33 p.m.
Message ID <66b327c773f63f0b9f2b.1428860033@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/8622/
State Superseded
Commit bedefc611f2564aa2421b99dabb2a9bd7c4c01cc
Headers show

Comments

Pierre-Yves David - April 12, 2015, 5:33 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1428847308 14400
#      Sun Apr 12 10:01:48 2015 -0400
# Node ID 66b327c773f63f0b9f2b00b2126adbf113f1ca36
# Parent  52ff737c63d2b2cb41185549aa9c35bc47317032
wlock: only issue devel warning when actually acquiring the lock

Before this change, any call to 'wlock' after we acquired a 'lock' was issuing a
warning. This is wrong is the 'wlock' have been properly acquired before the
'lock' in a previous call.

We move the warning code to only issue such warnings when we are acquiring the
'wlock' -after- acquiring 'lock'. This is the expected behavior of this warning
from the start.
Martin von Zweigbergk - April 12, 2015, 8:29 p.m.
On Sun, Apr 12, 2015 at 10:34 AM Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1428847308 14400
> #      Sun Apr 12 10:01:48 2015 -0400
> # Node ID 66b327c773f63f0b9f2b00b2126adbf113f1ca36
> # Parent  52ff737c63d2b2cb41185549aa9c35bc47317032
> wlock: only issue devel warning when actually acquiring the lock
>
> Before this change, any call to 'wlock' after we acquired a 'lock' was
> issuing a
> warning. This is wrong is the 'wlock' have been properly acquired before
> the
> 'lock' in a previous call.
>
> We move the warning code to only issue such warnings when we are acquiring
> the
> 'wlock' -after- acquiring 'lock'. This is the expected behavior of this
> warning
> from the start.
>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1199,23 +1199,24 @@ class localrepository(object):
>
>      def wlock(self, wait=True):
>          '''Lock the non-store parts of the repository (everything under
>          .hg except .hg/store) and return a weak reference to the lock.
>          Use this before modifying files in .hg.'''
> +        l = self._wlockref and self._wlockref()
> +        if l is not None and l.held:
> +            l.lock()
> +            return l
> +
>          if (self.ui.configbool('devel', 'all')
>                  or self.ui.configbool('devel', 'check-locks')):
>              l = self._lockref and self._lockref()
>              if l is not None and l.held:
>                  msg = '"lock" taken before "wlock"\n'
>                  if self.ui.tracebackflag:
>                      util.debugstacktrace(msg, 1)
>                  else:
>                      self.ui.write_err(msg)
> -        l = self._wlockref and self._wlockref()
> -        if l is not None and l.held:
> -            l.lock()
> -            return l
>
>          def unlock():
>              if self.dirstate.pendingparentchange():
>                  self.dirstate.invalidate()
>              else:
> diff --git a/tests/test-devel-warnings.t b/tests/test-devel-warnings.t
> --- a/tests/test-devel-warnings.t
> +++ b/tests/test-devel-warnings.t
> @@ -13,10 +13,26 @@
>    >     tr = repo.transaction('buggy')
>    >     lo = repo.lock()
>    >     wl = repo.wlock()
>    >     wl.release()
>    >     lo.release()
> +  >
> +  > @command('properlocking', [], '')
> +  > def buggylocking(ui, repo):
>

Is the name a copy&paste error?


> +  >     """check that reentrance is fine"""
> +  >     wl = repo.wlock()
> +  >     lo = repo.lock()
> +  >     tr = repo.transaction('buggy')
> +  >     tr2 = repo.transaction('buggy')
>

Here too? We're *not* testing buggy locking, are we?


> +  >     lo2 = repo.lock()
> +  >     wl2 = repo.wlock()
> +  >     wl2.release()
> +  >     lo2.release()
> +  >     tr2.close()
> +  >     tr.close()
> +  >     lo.release()
> +  >     wl.release()
>    > EOF
>
>    $ cat << EOF >> $HGRCPATH
>    > [extensions]
>    > buggylocking=$TESTTMP/buggylocking.py
> @@ -62,6 +78,7 @@
>     */mercurial/dispatch.py:* in _runcommand (glob)
>     */mercurial/dispatch.py:* in checkargs (glob)
>     */mercurial/dispatch.py:* in <lambda> (glob)
>     */mercurial/util.py:* in check (glob)
>     $TESTTMP/buggylocking.py:* in buggylocking (glob)
> +  $ hg properlocking
>    $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1199,23 +1199,24 @@  class localrepository(object):
 
     def wlock(self, wait=True):
         '''Lock the non-store parts of the repository (everything under
         .hg except .hg/store) and return a weak reference to the lock.
         Use this before modifying files in .hg.'''
+        l = self._wlockref and self._wlockref()
+        if l is not None and l.held:
+            l.lock()
+            return l
+
         if (self.ui.configbool('devel', 'all')
                 or self.ui.configbool('devel', 'check-locks')):
             l = self._lockref and self._lockref()
             if l is not None and l.held:
                 msg = '"lock" taken before "wlock"\n'
                 if self.ui.tracebackflag:
                     util.debugstacktrace(msg, 1)
                 else:
                     self.ui.write_err(msg)
-        l = self._wlockref and self._wlockref()
-        if l is not None and l.held:
-            l.lock()
-            return l
 
         def unlock():
             if self.dirstate.pendingparentchange():
                 self.dirstate.invalidate()
             else:
diff --git a/tests/test-devel-warnings.t b/tests/test-devel-warnings.t
--- a/tests/test-devel-warnings.t
+++ b/tests/test-devel-warnings.t
@@ -13,10 +13,26 @@ 
   >     tr = repo.transaction('buggy')
   >     lo = repo.lock()
   >     wl = repo.wlock()
   >     wl.release()
   >     lo.release()
+  > 
+  > @command('properlocking', [], '')
+  > def buggylocking(ui, repo):
+  >     """check that reentrance is fine"""
+  >     wl = repo.wlock()
+  >     lo = repo.lock()
+  >     tr = repo.transaction('buggy')
+  >     tr2 = repo.transaction('buggy')
+  >     lo2 = repo.lock()
+  >     wl2 = repo.wlock()
+  >     wl2.release()
+  >     lo2.release()
+  >     tr2.close()
+  >     tr.close()
+  >     lo.release()
+  >     wl.release()
   > EOF
 
   $ cat << EOF >> $HGRCPATH
   > [extensions]
   > buggylocking=$TESTTMP/buggylocking.py
@@ -62,6 +78,7 @@ 
    */mercurial/dispatch.py:* in _runcommand (glob)
    */mercurial/dispatch.py:* in checkargs (glob)
    */mercurial/dispatch.py:* in <lambda> (glob)
    */mercurial/util.py:* in check (glob)
    $TESTTMP/buggylocking.py:* in buggylocking (glob)
+  $ hg properlocking
   $ cd ..