Patchwork [2,of,2] transaction: turn lack of locking into a hard failure (API)

login
register
mail settings
Submitter Pierre-Yves David
Date May 13, 2016, 3:07 p.m.
Message ID <59c815c01aee4777f88c.1463152042@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/15104/
State Accepted
Delegated to: Yuya Nishihara
Headers show

Comments

Pierre-Yves David - May 13, 2016, 3:07 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1462458053 -7200
#      Thu May 05 16:20:53 2016 +0200
# Node ID 59c815c01aee4777f88c6edc52fb959685813bec
# Parent  d0c4dd6d292cc13c1ec2046d296380ba37c88033
# EXP-Topic consistencycleanup
transaction: turn lack of locking into a hard failure (API)

We have been warning about transactions without lock for about a year (and
three releases), third party extensions had a fair grace period to fix their
code, we are moving lack of locking to a hard failure in order to protect users
against repository corruptions.
timeless - May 15, 2016, 3:06 p.m.
> We have been warning about transactions without lock

locks

> to protect users
against repository corruptions.

corruption

--
The actual changes in both commits lgtm.
Katsunori FUJIWARA - May 16, 2016, 6:47 a.m.
At Fri, 13 May 2016 17:07:22 +0200,
Pierre-Yves David wrote:
> 
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1462458053 -7200
> #      Thu May 05 16:20:53 2016 +0200
> # Node ID 59c815c01aee4777f88c6edc52fb959685813bec
> # Parent  d0c4dd6d292cc13c1ec2046d296380ba37c88033
> # EXP-Topic consistencycleanup
> transaction: turn lack of locking into a hard failure (API)
> 
> We have been warning about transactions without lock for about a year (and
> three releases), third party extensions had a fair grace period to fix their
> code, we are moving lack of locking to a hard failure in order to protect users
> against repository corruptions.

Isn't "deprecwarn() period" needed before actual making transaction
without locking raise exception, like dirstate.write() without tr
argument ?


> diff -r d0c4dd6d292c -r 59c815c01aee mercurial/localrepo.py
> --- a/mercurial/localrepo.py	Thu May 05 16:13:22 2016 +0200
> +++ b/mercurial/localrepo.py	Thu May 05 16:20:53 2016 +0200
> @@ -1000,7 +1000,8 @@ class localrepository(object):
>                  or self.ui.configbool('devel', 'check-locks')):
>              l = self._lockref and self._lockref()
>              if l is None or not l.held:
> -                self.ui.develwarn('transaction with no lock')
> +                raise RuntimeError('programming error: transaction requires '
> +                                   'locking')
>          tr = self.currenttransaction()
>          if tr is not None:
>              return tr.nest()
> diff -r d0c4dd6d292c -r 59c815c01aee tests/test-devel-warnings.t
> --- a/tests/test-devel-warnings.t	Thu May 05 16:13:22 2016 +0200
> +++ b/tests/test-devel-warnings.t	Thu May 05 16:20:53 2016 +0200
> @@ -159,7 +159,15 @@
>  
>  Test programming error failure:
>  
> -  $ hg buggytransaction
> -  devel-warn: transaction with no lock at: $TESTTMP/buggylocking.py:* (buggylocking) (glob)
> +  $ hg buggytransaction 2>&1 | egrep -v '^  '
> +  ** Unknown exception encountered with possibly-broken third-party extension buggylocking
> +  ** which supports versions unknown of Mercurial.
> +  ** Please disable buggylocking and try your action again.
> +  ** If that fixes the bug please report it to the extension author.
> +  ** Python * (*)[*] (glob)
> +  ** Mercurial Distributed SCM (*) (glob)
> +  ** Extensions loaded: * (glob)
> +  Traceback (most recent call last):
> +  RuntimeError: programming error: transaction requires locking
>  
>    $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Yuya Nishihara - May 18, 2016, 12:43 p.m.
On Fri, 13 May 2016 17:07:22 +0200, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1462458053 -7200
> #      Thu May 05 16:20:53 2016 +0200
> # Node ID 59c815c01aee4777f88c6edc52fb959685813bec
> # Parent  d0c4dd6d292cc13c1ec2046d296380ba37c88033
> # EXP-Topic consistencycleanup
> transaction: turn lack of locking into a hard failure (API)

LGTM, pushed to the committed repo, thanks.

> We have been warning about transactions without lock for about a year (and
> three releases), third party extensions had a fair grace period to fix their
> code, we are moving lack of locking to a hard failure in order to protect users
> against repository corruptions.
> 
> diff -r d0c4dd6d292c -r 59c815c01aee mercurial/localrepo.py
> --- a/mercurial/localrepo.py	Thu May 05 16:13:22 2016 +0200
> +++ b/mercurial/localrepo.py	Thu May 05 16:20:53 2016 +0200
> @@ -1000,7 +1000,8 @@ class localrepository(object):
>                  or self.ui.configbool('devel', 'check-locks')):
>              l = self._lockref and self._lockref()
>              if l is None or not l.held:
> -                self.ui.develwarn('transaction with no lock')
> +                raise RuntimeError('programming error: transaction requires '
> +                                   'locking')

If we want to protect users, I think this needs to be enforced no matter if
develwarn is enabled or not. But that isn't big deal, queued this version.

And I got a test failure. I've made the glob pattern wider.

@@ -164,7 +164,7 @@
   ** which supports versions unknown of Mercurial.
   ** Please disable buggylocking and try your action again.
   ** If that fixes the bug please report it to the extension author.
-  ** Python * (*)[*] (glob)
+  ** Python 2.7.11+ (default, May  9 2016, 15:54:33) [GCC 5.3.1 20160429]
   ** Mercurial Distributed SCM (*) (glob)
   ** Extensions loaded: * (glob)
   Traceback (most recent call last):
Augie Fackler - May 23, 2016, 7:52 p.m.
On Fri, May 13, 2016 at 05:07:22PM +0200, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1462458053 -7200
> #      Thu May 05 16:20:53 2016 +0200
> # Node ID 59c815c01aee4777f88c6edc52fb959685813bec
> # Parent  d0c4dd6d292cc13c1ec2046d296380ba37c88033
> # EXP-Topic consistencycleanup
> transaction: turn lack of locking into a hard failure (API)

I'm a fan, but this bitrotted too much while I was sick and unable to
catch up, can you rebase and resend?

Thanks!

>
> We have been warning about transactions without lock for about a year (and
> three releases), third party extensions had a fair grace period to fix their
> code, we are moving lack of locking to a hard failure in order to protect users
> against repository corruptions.
>
> diff -r d0c4dd6d292c -r 59c815c01aee mercurial/localrepo.py
> --- a/mercurial/localrepo.py	Thu May 05 16:13:22 2016 +0200
> +++ b/mercurial/localrepo.py	Thu May 05 16:20:53 2016 +0200
> @@ -1000,7 +1000,8 @@ class localrepository(object):
>                  or self.ui.configbool('devel', 'check-locks')):
>              l = self._lockref and self._lockref()
>              if l is None or not l.held:
> -                self.ui.develwarn('transaction with no lock')
> +                raise RuntimeError('programming error: transaction requires '
> +                                   'locking')
>          tr = self.currenttransaction()
>          if tr is not None:
>              return tr.nest()
> diff -r d0c4dd6d292c -r 59c815c01aee tests/test-devel-warnings.t
> --- a/tests/test-devel-warnings.t	Thu May 05 16:13:22 2016 +0200
> +++ b/tests/test-devel-warnings.t	Thu May 05 16:20:53 2016 +0200
> @@ -159,7 +159,15 @@
>
>  Test programming error failure:
>
> -  $ hg buggytransaction
> -  devel-warn: transaction with no lock at: $TESTTMP/buggylocking.py:* (buggylocking) (glob)
> +  $ hg buggytransaction 2>&1 | egrep -v '^  '
> +  ** Unknown exception encountered with possibly-broken third-party extension buggylocking
> +  ** which supports versions unknown of Mercurial.
> +  ** Please disable buggylocking and try your action again.
> +  ** If that fixes the bug please report it to the extension author.
> +  ** Python * (*)[*] (glob)
> +  ** Mercurial Distributed SCM (*) (glob)
> +  ** Extensions loaded: * (glob)
> +  Traceback (most recent call last):
> +  RuntimeError: programming error: transaction requires locking
>
>    $ cd ..
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Pierre-Yves David - May 23, 2016, 10:15 p.m.
On 05/23/2016 09:52 PM, Augie Fackler wrote:
> On Fri, May 13, 2016 at 05:07:22PM +0200, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1462458053 -7200
>> #      Thu May 05 16:20:53 2016 +0200
>> # Node ID 59c815c01aee4777f88c6edc52fb959685813bec
>> # Parent  d0c4dd6d292cc13c1ec2046d296380ba37c88033
>> # EXP-Topic consistencycleanup
>> transaction: turn lack of locking into a hard failure (API)
> 
> I'm a fan, but this bitrotted too much while I was sick and unable to
> catch up, can you rebase and resend?
> 
> Thanks!

It's not bitrotted, it's already in. Yuya pushed it about 10 days ago.

Cheers,

Patch

diff -r d0c4dd6d292c -r 59c815c01aee mercurial/localrepo.py
--- a/mercurial/localrepo.py	Thu May 05 16:13:22 2016 +0200
+++ b/mercurial/localrepo.py	Thu May 05 16:20:53 2016 +0200
@@ -1000,7 +1000,8 @@  class localrepository(object):
                 or self.ui.configbool('devel', 'check-locks')):
             l = self._lockref and self._lockref()
             if l is None or not l.held:
-                self.ui.develwarn('transaction with no lock')
+                raise RuntimeError('programming error: transaction requires '
+                                   'locking')
         tr = self.currenttransaction()
         if tr is not None:
             return tr.nest()
diff -r d0c4dd6d292c -r 59c815c01aee tests/test-devel-warnings.t
--- a/tests/test-devel-warnings.t	Thu May 05 16:13:22 2016 +0200
+++ b/tests/test-devel-warnings.t	Thu May 05 16:20:53 2016 +0200
@@ -159,7 +159,15 @@ 
 
 Test programming error failure:
 
-  $ hg buggytransaction
-  devel-warn: transaction with no lock at: $TESTTMP/buggylocking.py:* (buggylocking) (glob)
+  $ hg buggytransaction 2>&1 | egrep -v '^  '
+  ** Unknown exception encountered with possibly-broken third-party extension buggylocking
+  ** which supports versions unknown of Mercurial.
+  ** Please disable buggylocking and try your action again.
+  ** If that fixes the bug please report it to the extension author.
+  ** Python * (*)[*] (glob)
+  ** Mercurial Distributed SCM (*) (glob)
+  ** Extensions loaded: * (glob)
+  Traceback (most recent call last):
+  RuntimeError: programming error: transaction requires locking
 
   $ cd ..