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

Pierre-Yves David pierre-yves.david at ens-lyon.org
Wed Mar 11 17:16:15 CDT 2015



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

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.

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list