| 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
> We have been warning about transactions without lock locks > to protect users against repository corruptions. corruption -- The actual changes in both commits lgtm.
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
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):
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
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 ..