[PATCH 5 of 5] changelog: rely on transaction for finalization

Pierre-Yves David pierre-yves.david at ens-lyon.org
Mon Nov 3 13:15:52 CST 2014



On 11/03/2014 05:11 PM, Gregory Szorc wrote:
> On 11/2/14 6:20 AM, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david at fb.com>
>> # Date 1413619781 25200
>> #      Sat Oct 18 01:09:41 2014 -0700
>> # Node ID 3270e2ef0b7d74003e49fcdec588763c258a1038
>> # Parent  d434fffe10dac3cb137b3695426a69a4cef9ef35
>> changelog: rely on transaction for finalization
>>
>> Instead of calling `cl.finalize()` by hand (possibly at a bogus time) we
>> register it to the transaction during `delayupdate` and rely on
>> `tr.close()` to
>> call it at the right time.
>>
>> diff --git a/mercurial/changelog.py b/mercurial/changelog.py
>> --- a/mercurial/changelog.py
>> +++ b/mercurial/changelog.py
>> @@ -3,10 +3,11 @@
>>   # Copyright 2005-2007 Matt Mackall <mpm at selenic.com>
>>   #
>>   # This software may be used and distributed according to the terms
>> of the
>>   # GNU General Public License version 2 or any later version.
>>
>> +import weakref
>>   from node import bin, hex, nullid
>>   from i18n import _
>>   import util, error, revlog, encoding
>>
>>   _defaultextra = {'branch': 'default'}
>> @@ -237,12 +238,14 @@ class changelog(revlog.revlog):
>>                   self._delaybuf = []
>>                   self.opener = _delayopener(self._realopener,
>> self.indexfile,
>>                                              self._delaybuf)
>>           self._delayed = True
>>           tr.addpending('cl-%i' % id(self), self._writepending)
>> +        trp = weakref.proxy(tr)
>> +        tr.addfinalize('cl-%i' % id(self), lambda: self._finalize(trp))
>
> I tend to get suspicious any time I see weakref. Looking into the
> future, I could see other consumers of these callbacks also looking to
> use a transaction reference. Requiring the caller to do the weakref
> magic to avoid reference cycles seems fragile and prone to leaks.

This code have been using a weakref for ever. It just become visible 
here because I'm mving code around.

> Should we nip this in the bud by passing the transaction instance to the
> callback?

It could make sense yes, it would help adding hooks data inside those 
callback.

> As an extension author, I may also want a repo instance in the callback.
> But that's not currently captured as part of transaction instances, so
> that's scope bloat. (The use case I care about is creating a SQL
> transaction that can be committed or rolled back with the lifecycle of
> the hg transaction. This patch series makes it much easier.)

Cheers,

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list