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

Ryan McElroy rm at fb.com
Tue Apr 14 03:45:38 UTC 2015


On 4/13/2015 11:18 AM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david at fb.com>
> # Date 1428847308 14400
> #      Sun Apr 12 10:01:48 2015 -0400
> # Node ID 6a40db689612929075d307e9fa4ccd5702ef5922
> # Parent  c229ba58b956c7872af7ed220ad5099ea4a6ecf7
> 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 properlocking(ui, repo):
> +  >     """check that reentrance is fine"""
> +  >     wl = repo.wlock()
> +  >     lo = repo.lock()
> +  >     tr = repo.transaction('proper')
> +  >     tr2 = repo.transaction('proper')
Why do you need these transactions to demonstrate the problem? Shouldn't 
just this do it?:

l1 = repo.wlock()
l2 = repo.lock()
l3 = repo.wlock()

If that's not sufficient, can you explain to me why not?
> +  >     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 ..
>

This change lgtm.

A meta-point/question: I often find it would be useful to see how a test 
behaved *before* a fix. What does the community think about a patch that 
introduces a test that shows a failure, followed by a patch that fixes 
the code and shows how the behavior is now fixed and better than before? 
I might start doing this in my patches...



More information about the Mercurial-devel mailing list