| Submitter | Pierre-Yves David |
|---|---|
| Date | March 11, 2015, 4:56 a.m. |
| Message ID | <5cab671ca5e7346e25b7.1426049810@marginatus.alto.octopoid.net> |
| Download | mbox | patch |
| Permalink | /patch/8002/ |
| State | Superseded |
| Commit | 026f8af88e49ae11ef4e5af3acb0c6171c50aaf8 |
| Headers | show |
Comments
On 3/10/2015 9:56 PM, 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 5cab671ca5e7346e25b7d1a98cd9bdb1f4f25894 > # Parent fb5fc276e7112b1d2b849d42d3502103e95a6d6a > 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. > > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py > --- a/mercurial/localrepo.py > +++ b/mercurial/localrepo.py > @@ -896,10 +896,18 @@ class localrepository(object): > if tr and tr.running(): > return tr > return None > > def transaction(self, desc, report=None): > + if self.ui.develflag('check-locks'): > + l = self._lockref and self._lockref() > + if l is None or not l.held: > + msg = 'transaction with no lock\n' No translation because it's a debug statement, even if we print it as an error? Why not move the creation of this down to where it's used? > + if self.ui.tracebackflag: > + util.debugstacktrace(msg, 2) This 2 is a disappointing magic number. I have no idea what this line does. Is this just something mercurial-y or python-y that is obvious to trained eyes? I'd much prefer a comment or something more explicit about what you're pulling out here. > + 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,14 +23,16 @@ > > 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" > $ cd .. > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@selenic.com > http://selenic.com/mailman/listinfo/mercurial-devel
On Tue, Mar 10, 2015 at 09:56:50PM -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 5cab671ca5e7346e25b7d1a98cd9bdb1f4f25894 > # Parent fb5fc276e7112b1d2b849d42d3502103e95a6d6a > devel: also warn about transaction started without a lock I don't disagree with Ryan's comments, but wanted to say that I support this series. I've been meaning to get a buildbot running with the lock checker enabled for a while, and this is probably a better solution. > > 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 > @@ -896,10 +896,18 @@ class localrepository(object): > if tr and tr.running(): > return tr > return None > > def transaction(self, desc, report=None): > + if self.ui.develflag('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, 2) > + 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,14 +23,16 @@ > > 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" > $ cd .. > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@selenic.com > http://selenic.com/mailman/listinfo/mercurial-devel
On 03/11/2015 04:53 PM, Ryan McElroy wrote: > On 3/10/2015 9:56 PM, 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 5cab671ca5e7346e25b7d1a98cd9bdb1f4f25894 >> # Parent fb5fc276e7112b1d2b849d42d3502103e95a6d6a >> 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. >> >> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py >> --- a/mercurial/localrepo.py >> +++ b/mercurial/localrepo.py >> @@ -896,10 +896,18 @@ class localrepository(object): >> if tr and tr.running(): >> return tr >> return None >> def transaction(self, desc, report=None): >> + if self.ui.develflag('check-locks'): >> + l = self._lockref and self._lockref() >> + if l is None or not l.held: >> + msg = 'transaction with no lock\n' > > No translation because it's a debug statement, even if we print it as > an error? Why not move the creation of this down to where it's used? > >> + if self.ui.tracebackflag: >> + util.debugstacktrace(msg, 2) > > This 2 is a disappointing magic number. I have no idea what this line > does. Is this just something mercurial-y or python-y that is obvious > to trained eyes? I'd much prefer a comment or something more explicit > about what you're pulling out here. I think the number 2 totally makes sense. It is not magic, considering what debugstacktrace do according to its docstring. I don't think it is necessary to duplicate the information here. +1 to the series. The lock checker in contrib was not so useful. The other part of the original lock checker was to hook into opener functions and verify that the right locks had been locked first. That is probably still not possible to do without gross hacks. /Mads
On 03/11/2015 08:53 AM, Ryan McElroy wrote: > On 3/10/2015 9:56 PM, 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 5cab671ca5e7346e25b7d1a98cd9bdb1f4f25894 >> # Parent fb5fc276e7112b1d2b849d42d3502103e95a6d6a >> 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. >> >> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py >> --- a/mercurial/localrepo.py >> +++ b/mercurial/localrepo.py >> @@ -896,10 +896,18 @@ class localrepository(object): >> if tr and tr.running(): >> return tr >> return None >> def transaction(self, desc, report=None): >> + if self.ui.develflag('check-locks'): >> + l = self._lockref and self._lockref() >> + if l is None or not l.held: >> + msg = 'transaction with no lock\n' > > No translation because it's a debug statement, even if we print it as an > error? Why not move the creation of this down to where it's used? No translation because the target are Mercurial developers, not users. I expect all people writing mercurial code to be able to read, understand and grep english sentence. (I will not make any assumption their ability to write proper english as this could backfire quickly) The message is used twice, so having a temporary variable right before the if/else seems perfectly sensible to me. >> + if self.ui.tracebackflag: >> + util.debugstacktrace(msg, 2) > > This 2 is a disappointing magic number. I have no idea what this line > does. Is this just something mercurial-y or python-y that is obvious to > trained eyes? I'd much prefer a comment or something more explicit about > what you're pulling out here. This the standard argument of python's traceback method. This tell the number of frame that need to be skipped when reporting the traceback. This is used to make the traceback conclude to the function that did the infamous call. This is standard python' s stuff and I do not think we should document it. However, you comment points out that the --traceback variant is untested and the value is wrong (should be 1 since we have less nesting than in the extension) I'll send a V2 with the right value and proper testing.
Patch
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py --- a/mercurial/localrepo.py +++ b/mercurial/localrepo.py @@ -896,10 +896,18 @@ class localrepository(object): if tr and tr.running(): return tr return None def transaction(self, desc, report=None): + if self.ui.develflag('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, 2) + 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,14 +23,16 @@ > 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" $ cd ..