[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