[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