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

login
register
mail settings
Submitter Pierre-Yves David
Date April 13, 2015, 6:18 p.m.
Message ID <6a40db689612929075d3.1428949121@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/8639/
State Accepted
Headers show

Comments

Pierre-Yves David - April 13, 2015, 6:18 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 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.
Ryan McElroy - April 14, 2015, 3:45 a.m.
On 4/13/2015 11:18 AM, Pierre-Yves David 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 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...
Martin von Zweigbergk - April 14, 2015, 3:57 a.m.
On Mon, Apr 13, 2015 at 8:46 PM Ryan McElroy <rm@fb.com> wrote:
>
> 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...
>

I brought this up here:
http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/75747/focus=75752

I haven't taken the time to add that syntax to the test runner yet.
Ryan McElroy - April 14, 2015, 5:44 a.m.
On 4/13/2015 8:57 PM, Martin von Zweigbergk wrote:
>
>
> On Mon, Apr 13, 2015 at 8:46 PM Ryan McElroy <rm@fb.com 
> <mailto:rm@fb.com>> wrote:
>
>     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...
>
>
> I brought this up here:
> http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/75747/focus=75752 
> <https://urldefense.proofpoint.com/v1/url?u=http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/75747/focus%3D75752&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=zxRJZ6melt%2FqLtQ%2Bw2Gaeg%3D%3D%0A&m=B131xN7YhmkKP95EdF%2BNyUjre%2FhGNHb29T2yr6YWix0%3D%0A&s=584ec665a1465e012048c5c3b794529b2a14e2286e8ddac850a92a61c1587ffd>
>
> I haven't taken the time to add that syntax to the test runner yet.
Yeah, we're on the same page. I don't think we even need to mark 
something as BROKEN if it's fixed in the next patch... It's just 
documenting the old and new behavior. I'm not even totally aware of what 
BROKEN means or implies (but I'd like to learn).
Martin von Zweigbergk - April 14, 2015, 5:50 a.m.
On Mon, Apr 13, 2015 at 10:45 PM Ryan McElroy <rm@fb.com> wrote:

> On 4/13/2015 8:57 PM, Martin von Zweigbergk wrote:
>
>
>
> On Mon, Apr 13, 2015 at 8:46 PM Ryan McElroy <rm@fb.com> wrote:
>>
>> 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...
>>
>
>  I brought this up here:
>
> http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/75747/focus=75752
> <https://urldefense.proofpoint.com/v1/url?u=http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/75747/focus%3D75752&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=zxRJZ6melt%2FqLtQ%2Bw2Gaeg%3D%3D%0A&m=B131xN7YhmkKP95EdF%2BNyUjre%2FhGNHb29T2yr6YWix0%3D%0A&s=584ec665a1465e012048c5c3b794529b2a14e2286e8ddac850a92a61c1587ffd>
>
>  I haven't taken the time to add that syntax to the test runner yet.
>
> Yeah, we're on the same page. I don't think we even need to mark something
> as BROKEN if it's fixed in the next patch... It's just documenting the old
> and new behavior. I'm not even totally aware of what BROKEN means or
> implies (but I'd like to learn).
>

I've marked tests of broken functionality BROKEN, whether I have fixed them
in the next patch or not. Some of them are still not fixed. But sure, if
they really are going to be fixed in the next patch, I don't care so much
that they are marked as broken in the first patch, as long as it's clear
that the test is testing broken functionality. In your particular case, it
was clearly not broken to start with; it was just not ideal.
Pierre-Yves David - April 14, 2015, 2:51 p.m.
On 04/13/2015 11:45 PM, Ryan McElroy wrote:
> On 4/13/2015 11:18 AM, Pierre-Yves David 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 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?

This is actually a sneaky unrelated changes, as we are testing 
re-entrance of stuff, I'm also testing re-entrance with the transaction. 
This is not even really worth testing but cheap and simple.

I can drop it if you are too uncomfortable with it.

>> +  >     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...

I had a discussion with Matt about that in 2011, I can with the same 
position you currently have and Matt managed to turn me around as on 
would do with a pancake. Let's try to remember the argument:

- We better have test being correct and passing most of the time
- Adding such commit would basically double with amount of commit with 
mostly noise.
- Behavior changes should be documented in the commit message anyway if 
they are notable.
- Checking a test afterward is rare (like, 2 month later), and we have a 
good enough doing `hg up p1(X); hg revert --rev X tests/`

(I actually do the former while queuing if I've a doubt on test validity.
Matt Mackall - April 16, 2015, 10:58 p.m.
On Mon, 2015-04-13 at 22:44 -0700, Ryan McElroy wrote:
> On 4/13/2015 8:57 PM, Martin von Zweigbergk wrote:
> >
> >
> > On Mon, Apr 13, 2015 at 8:46 PM Ryan McElroy <rm@fb.com 
> > <mailto:rm@fb.com>> wrote:
> >
> >     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...
> >
> >
> > I brought this up here:
> > http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/75747/focus=75752 
> > <https://urldefense.proofpoint.com/v1/url?u=http://thread.gmane.org/gmane.comp.version-control.mercurial.devel/75747/focus%3D75752&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=zxRJZ6melt%2FqLtQ%2Bw2Gaeg%3D%3D%0A&m=B131xN7YhmkKP95EdF%2BNyUjre%2FhGNHb29T2yr6YWix0%3D%0A&s=584ec665a1465e012048c5c3b794529b2a14e2286e8ddac850a92a61c1587ffd>
> >
> > I haven't taken the time to add that syntax to the test runner yet.
> Yeah, we're on the same page. I don't think we even need to mark 
> something as BROKEN if it's fixed in the next patch... It's just 
> documenting the old and new behavior. I'm not even totally aware of what 
> BROKEN means or implies (but I'd like to learn).

It's just the plain English meaning with emphasis.

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 properlocking(ui, repo):
+  >     """check that reentrance is fine"""
+  >     wl = repo.wlock()
+  >     lo = repo.lock()
+  >     tr = repo.transaction('proper')
+  >     tr2 = repo.transaction('proper')
+  >     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 ..