[PATCH 1 of 2] localrepo: add afterpush()

Gregory Szorc gregory.szorc at gmail.com
Sat Jul 12 12:42:32 CDT 2014


On 7/9/2014 1:10 PM, Matt Mackall wrote:
> On Tue, 2014-07-08 at 03:03 -0700, Gregory Szorc wrote:
> 
>> FWIW, this reasoning is why I'm a fan of grouping related functions in
>> classes over the traditional Mercurial "style" of module level
>> functions. With classes, I can go in with a scalpel and adjust
>> __class__ on a per instance basis. When that instance is destroyed,
>> the hacks die with it. Adjusting module symbols is juggling chainsaws
>> by comparison. There's too many considerations, especially in
>> persistent processes, such as hgweb and the command server. I'd prefer
>> we limit the surface area for bugs by encouraging instance specific
>> hacking over global/module hacking.
> 
> That line of reasoning is all very good and I'd be inclined to agree
> with you if extensions were first-class citizens. But they're explicitly
> not: we intentionally sacrifice maintainability of modules for
> maintainability of the core. And the complexity of localrepo in
> particular has become a very obvious maintenance problem.
> 
> However, if this is a big deal, perhaps the answer is a helper in
> extensions.py that calls your replacement function if the repo arg is of
> a particular class. This probably makes sense for both function and
> command wrappers.
> 
> Relatedly, lots of our monkeypatch setup work could probably be replaced
> with decorators.
> 
> 
> However, we can perhaps add a magic wrapper to extensions that does some
> sort of class matching on repo args for you.

I don't think this approach will work because I don't believe there's a
good way to get a reference to a dynamically created class to perform
the isinstance() check. e.g.

class Base(object):
    pass

def changeclass(o):
    class Child(o.__class__):
        pass

    o.__class__ = Child

c = Base()
changeclass(c)

print(c)
> <__main__.Child object at 0x7f14836ad450>

print(c.__class__)
> <class '__main__.Child'>

print(dir(changeclass))
['__call__', '__class__', '__closure__', '__code__', '__defaults__',
'__delattr__', '__dict__', '__doc__', '__format__', '__get__',
'__getattribute__', '__globals__', '__hash__', '__init__', '__module__',
'__name__', '__new__', '__reduce__', '__reduce_ex__', '__repr__',
'__setattr__', '__sizeof__', '__str__', '__subclasshook__',
'func_closure', 'func_code', 'func_defaults', 'func_dict', 'func_doc',
'func_globals', 'func_name']

print(sorted(globals()))
['Base', '__builtins__', '__doc__', '__file__', '__name__',
'__package__', 'c', 'changeclass']

print(c.__class__.__module__)
__main__

import sys
print(sorted(dir(sys.modules['__main__'])))
['Base', '__builtins__', '__doc__', '__file__', '__name__',
'__package__', 'c', 'changeclass', 'sys']

Furthermore,

c2 = Base()
changeclass(c2)
c.__class__ == c2.__class__
False
isinstance(c2, c.__class__)
False

Such is the nature of dynamic class creation. Each function call
produces a new class type. The only handle on it is the .__class__ of
the instance it is attached to.

You /could/ look at class names. e.g. c.__class__.__name__. But I'm
going to argue that isn't robust enough.

I'm pretty sure we need to store state on each localrepo instance for
this to work. I'll send a few alternative patches to the list. I hope
one will be liked.

Please disregard this patch series.


More information about the Mercurial-devel mailing list