Patchwork [2,of,4] devel: move the lock-checking code into core

login
register
mail settings
Submitter Pierre-Yves David
Date March 11, 2015, 4:56 a.m.
Message ID <831c69fcda436381cc27.1426049808@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/8000/
State Superseded
Commit d6ac30f4edefcf205bf27a3ddf37626922fa8018
Headers show

Comments

Pierre-Yves David - March 11, 2015, 4:56 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1421405470 28800
#      Fri Jan 16 02:51:10 2015 -0800
# Node ID 831c69fcda436381cc27a0679c05b71d69184376
# Parent  e9e42adc21a5c0a9bd68079bdd209b7a086ad046
devel: move the lock-checking code into core

If the developer warning are enabled, bad locking order will be reported without
the need for the contrib extensions.
Ryan McElroy - March 11, 2015, 3:56 p.m.
On 3/10/2015 9:56 PM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1421405470 28800
> #      Fri Jan 16 02:51:10 2015 -0800
> # Node ID 831c69fcda436381cc27a0679c05b71d69184376
> # Parent  e9e42adc21a5c0a9bd68079bdd209b7a086ad046
> devel: move the lock-checking code into core
>
> If the developer warning are enabled, bad locking order will be reported without
> the need for the contrib extensions.
>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -1144,10 +1144,18 @@ 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.'''
> +        if self.ui.develflag('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, 2)
> +                else:
> +                    self.ui.write_err(msg)

Same feedback as on #4: 2 is a magic number, msg is constructed before 
it's needed and not always used.

>           l = self._wlockref and self._wlockref()
>           if l is not None and l.held:
>               l.lock()
>               return l
>   
> diff --git a/tests/test-devel-warnings.t b/tests/test-devel-warnings.t
> new file mode 100644
> --- /dev/null
> +++ b/tests/test-devel-warnings.t
> @@ -0,0 +1,35 @@
> +
> +  $ cat << EOF > buggylocking.py
> +  > """A small extension to acquires lock in the wrong order
> +  > """
> +  >
> +  > from mercurial import cmdutil
> +  >
> +  > cmdtable = {}
> +  > command = cmdutil.command(cmdtable)
> +  >
> +  > @command('buggylocking', [], '')
> +  > def buggylocking(ui, repo):
> +  >     lo = repo.lock()
> +  >     wl = repo.wlock()
> +  > EOF
> +
> +  $ cat << EOF >> $HGRCPATH
> +  > [extensions]
> +  > buggylocking=$TESTTMP/buggylocking.py
> +  > [devel]
> +  > all=1
> +  > EOF
> +
> +  $ hg init lock-checker
> +  $ cd lock-checker
> +  $ hg buggylocking
> +  "lock" taken before "wlock"
> +  $ cat << EOF >> $HGRCPATH
> +  > [devel]
> +  > all=0
> +  > check-locks=1
> +  > EOF
> +  $ hg buggylocking
> +  "lock" taken before "wlock"
> +  $ cd ..

Overall, I like this patchset; prefer it with Yuya's and my suggestions.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1144,10 +1144,18 @@  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.'''
+        if self.ui.develflag('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, 2)
+                else:
+                    self.ui.write_err(msg)
         l = self._wlockref and self._wlockref()
         if l is not None and l.held:
             l.lock()
             return l
 
diff --git a/tests/test-devel-warnings.t b/tests/test-devel-warnings.t
new file mode 100644
--- /dev/null
+++ b/tests/test-devel-warnings.t
@@ -0,0 +1,35 @@ 
+
+  $ cat << EOF > buggylocking.py
+  > """A small extension to acquires lock in the wrong order
+  > """
+  > 
+  > from mercurial import cmdutil
+  > 
+  > cmdtable = {}
+  > command = cmdutil.command(cmdtable)
+  > 
+  > @command('buggylocking', [], '')
+  > def buggylocking(ui, repo):
+  >     lo = repo.lock()
+  >     wl = repo.wlock()
+  > EOF
+
+  $ cat << EOF >> $HGRCPATH
+  > [extensions]
+  > buggylocking=$TESTTMP/buggylocking.py
+  > [devel]
+  > all=1
+  > EOF
+
+  $ hg init lock-checker
+  $ cd lock-checker
+  $ hg buggylocking
+  "lock" taken before "wlock"
+  $ cat << EOF >> $HGRCPATH
+  > [devel]
+  > all=0
+  > check-locks=1
+  > EOF
+  $ hg buggylocking
+  "lock" taken before "wlock"
+  $ cd ..