[PATCH 4 of 4] devel: also warn about transaction started without a lock

[Mads Kiilerich]

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 at 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