[PATCH 2 of 2] keyword: eliminate potential reference cycles from kwrepo

[Christian Ebert]

* Martin Geisler on Tuesday, July 07, 2009 at 22:50:37 +0200
> Matt Mackall <mpm at selenic.com> writes:
> 
>> How do we work around this? Good question. In the case of kwrepo,
>> where we're defining a new class already, the easiest thing to do is
>> simply to superclass:
>> 
>> class baz(foo):
>>  def bar(self):
>>    # add our baz-specific behavior
>>    ...
>>    # fall through to base behavior where appropriate
>>    super(baz, self).bar()
>> 
>> Then no reassignment is necessary and we never need to deal with bound
>> methods.
>
> Right, that makes good sense. And so I would say that the part of the
> patch that adds wrappers like this is unnecessary (since we don't add
> any kwrepo-specific behavior to sopener):
> 
>    class kwrepo(repo.__class__):
>        def sopener(self, *args):
>            return super(kwrepo, self).sopener(*args)

In the code should it be:

    class kwrepo(repo.__class__):
        def file(self, f):
            if f[0] == '/':
                f = f[1:]
            return kwfilelog(self.sopener, kwt, f)

or:

    class kwrepo(repo.__class__):
        def file(self, f):
            if f[0] == '/':
                f = f[1:]
            return kwfilelog(repo.sopener, kwt, f)
                             ^^^^
or doesn't it matter at all?

>> The second issue is repo.foo vs self.foo. If you have something that
>> looks like:
>> 
>> def wrapobject(repo):
>>  class superrepo(repo.__class__):
>>    def foo(self):
>>      repo.val += 1
>>  repo.__class__ == superrepo
>> 
>> ..that repo.val pins a reference to repo from the surrounding scope
>> inside the class definition. When we slap it onto the repo object,
>> repo now contains a reference loop again. Changing that repo.val to
>> self.val cures that.
> 
> With code like this:
> 
>        def commit(self, text='', user=None, date=None, match=None,
>                   force=False, editor=None, extra={}):
>            # use custom commitctx for user commands
>            # other extensions can still wrap repo.commitctx directly
>            repo.commitctx = self.kwcommitctx
>            return super(kwrepo, self).commit(text, user, date, match, force,
>                         editor, extra)
> 
>        def kwcommitctx(self, ctx, error=False):
>            # ...
> 
> where the patch changes repo.commitctx to self.commitctx, would it not
> make more sense to simply override commitctx in the kwrepo subclass?

That's what I did first. But then the "double wrapper" was
proposed for the reasons I try to describe in the comment.

Practical example discussed were external convert extensions.

> If needed, commit could set a flag to signal to commitctx when
> to do something and when to just pass control to the
> superclass.

But what about extensions where you don't know their load order?
 
c
-- 
_B A U S T E L L E N_ lesen! --->> <http://www.blacktrash.org/baustellen/>