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

[Pierre-Yves David]

On 07/08/2014 12:03 PM, Gregory Szorc wrote:
> On Jul 8, 2014, at 2:31, Pierre-Yves David <pierre-yves.david at ens-lyon.org> wrote:
>>
>>
>>
>>> On 07/08/2014 05:43 AM, Gregory Szorc wrote:
>>> # HG changeset patch
>>> # User Gregory Szorc <gregory.szorc at gmail.com>
>>> # Date 1404785357 25200
>>> #      Mon Jul 07 19:09:17 2014 -0700
>>> # Node ID 30333e516874716b8d9b41450282d1f22b87eba1
>>> # Parent  7537e57f5dbd0805aad3c9e5e4f7dd8bbf6bbde1
>>> localrepo: add afterpush()
>>
>> The trend is to remove method from localrepo, not adding new ones.
>>
>> (`localrepo.push(…)` was not thrown in fire as a courtesy to all the extension that may rely on it.)
>>
>> The hook in itself is a good idea, but please add it in the `mercurial.exchange` module.
>
> An issue with moving things out of localrepo is that changes to module level symbols are global and apply to all repos. With localrepo, the pattern is to dynamically declare a new repo class during reposetup and only hack up the repo instances you want. I find this approach far more maintainable than peppering isinstance/hasattribute checks in monkeypatched module functions.

This is not a massive issue as the module level function should be able 
to easily check if the repo has the extension enabled or not.

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

The issue here is that all of the Mercurial logic ultimatly ends up on a 
single class, localrepo. And all the possible state that someone persist 
ends up on a single class, localrepo. This has proven to be a massive 
pain an we have been actively working on removing code from localrepo.

Push is a simple, (usually) one shot call. It has nothing to do on the 
localrepo class.


-- 
Pierre-Yves David