Patchwork [3,of,3,V2] devel: also warn about transaction started without a lock

login
register
mail settings
Submitter Pierre-Yves David
Date March 18, 2015, 11:47 p.m.
Message ID <89890be5e4d784cb2645.1426722477@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/8157/
State Accepted
Commit 026f8af88e49ae11ef4e5af3acb0c6171c50aaf8
Headers show

Comments

Pierre-Yves David - March 18, 2015, 11:47 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1426046625 25200
#      Tue Mar 10 21:03:45 2015 -0700
# Node ID 89890be5e4d784cb264583b5366d852a5a50561f
# Parent  81e2aeedbed4a8b18dc7d8d3f9d6f8d7eb6d0d24
devel: also warn about transaction started without a lock

Nobody should even start a transaction on an unlocked repository. If developer
warning are enabled this will be reported. This use the same config as the bad
locking order as this is closely related.
Augie Fackler - March 19, 2015, 1:39 p.m.
On Wed, Mar 18, 2015 at 04:47:57PM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1426046625 25200
> #      Tue Mar 10 21:03:45 2015 -0700
> # Node ID 89890be5e4d784cb264583b5366d852a5a50561f
> # Parent  81e2aeedbed4a8b18dc7d8d3f9d6f8d7eb6d0d24
> devel: also warn about transaction started without a lock

Looks good to me, queued. Note that you should probably coordinate
with Brendan to get at least one builder running with this set, or
maybe it should just be on for the testsuite.

>
> Nobody should even start a transaction on an unlocked repository. If developer
> warning are enabled this will be reported. This use the same config as the bad
> locking order as this is closely related.
>
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -917,10 +917,19 @@ class localrepository(object):
>          if tr and tr.running():
>              return tr
>          return None
>
>      def transaction(self, desc, report=None):
> +        if (self.ui.configbool('devel', 'all')
> +                or self.ui.configbool('devel', 'check-locks')):
> +            l = self._lockref and self._lockref()
> +            if l is None or not l.held:
> +                msg = 'transaction with no lock\n'
> +                if self.ui.tracebackflag:
> +                    util.debugstacktrace(msg, 1)
> +                else:
> +                    self.ui.write_err(msg)
>          tr = self.currenttransaction()
>          if tr is not None:
>              return tr.nest()
>
>          # abort here if the journal already exists
> 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
> @@ -8,10 +8,11 @@
>    > cmdtable = {}
>    > command = cmdutil.command(cmdtable)
>    >
>    > @command('buggylocking', [], '')
>    > def buggylocking(ui, repo):
> +  >     tr = repo.transaction('buggy')
>    >     lo = repo.lock()
>    >     wl = repo.wlock()
>    > EOF
>
>    $ cat << EOF >> $HGRCPATH
> @@ -22,19 +23,34 @@
>    > EOF
>
>    $ hg init lock-checker
>    $ cd lock-checker
>    $ hg buggylocking
> +  transaction with no lock
>    "lock" taken before "wlock"
>    $ cat << EOF >> $HGRCPATH
>    > [devel]
>    > all=0
>    > check-locks=1
>    > EOF
>    $ hg buggylocking
> +  transaction with no lock
>    "lock" taken before "wlock"
>    $ hg buggylocking --traceback
> +  transaction with no lock
> +   at:
> +   */hg:* in <module> (glob)
> +   */mercurial/dispatch.py:* in run (glob)
> +   */mercurial/dispatch.py:* in dispatch (glob)
> +   */mercurial/dispatch.py:* in _runcatch (glob)
> +   */mercurial/dispatch.py:* in _dispatch (glob)
> +   */mercurial/dispatch.py:* in runcommand (glob)
> +   */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)
>    "lock" taken before "wlock"
>     at:
>     */hg:* in <module> (glob)
>     */mercurial/dispatch.py:* in run (glob)
>     */mercurial/dispatch.py:* in dispatch (glob)
> _______________________________________________
> 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
@@ -917,10 +917,19 @@  class localrepository(object):
         if tr and tr.running():
             return tr
         return None
 
     def transaction(self, desc, report=None):
+        if (self.ui.configbool('devel', 'all')
+                or self.ui.configbool('devel', 'check-locks')):
+            l = self._lockref and self._lockref()
+            if l is None or not l.held:
+                msg = 'transaction with no lock\n'
+                if self.ui.tracebackflag:
+                    util.debugstacktrace(msg, 1)
+                else:
+                    self.ui.write_err(msg)
         tr = self.currenttransaction()
         if tr is not None:
             return tr.nest()
 
         # abort here if the journal already exists
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
@@ -8,10 +8,11 @@ 
   > cmdtable = {}
   > command = cmdutil.command(cmdtable)
   > 
   > @command('buggylocking', [], '')
   > def buggylocking(ui, repo):
+  >     tr = repo.transaction('buggy')
   >     lo = repo.lock()
   >     wl = repo.wlock()
   > EOF
 
   $ cat << EOF >> $HGRCPATH
@@ -22,19 +23,34 @@ 
   > EOF
 
   $ hg init lock-checker
   $ cd lock-checker
   $ hg buggylocking
+  transaction with no lock
   "lock" taken before "wlock"
   $ cat << EOF >> $HGRCPATH
   > [devel]
   > all=0
   > check-locks=1
   > EOF
   $ hg buggylocking
+  transaction with no lock
   "lock" taken before "wlock"
   $ hg buggylocking --traceback
+  transaction with no lock
+   at:
+   */hg:* in <module> (glob)
+   */mercurial/dispatch.py:* in run (glob)
+   */mercurial/dispatch.py:* in dispatch (glob)
+   */mercurial/dispatch.py:* in _runcatch (glob)
+   */mercurial/dispatch.py:* in _dispatch (glob)
+   */mercurial/dispatch.py:* in runcommand (glob)
+   */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)
   "lock" taken before "wlock"
    at:
    */hg:* in <module> (glob)
    */mercurial/dispatch.py:* in run (glob)
    */mercurial/dispatch.py:* in dispatch (glob)