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

[Gregory Szorc]

On 7/8/14, 3:38 AM, Pierre-Yves David wrote:
>
>
> 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.

"extension enabled" does not imply "extension active for a given repo".

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

Sure. But at the same time, the collection of all the APIs on localrepo 
gives extension authors a clear target for extensibility points. As a 
newbie extension author, I would have loved the docs to say "open up 
localrepo.py and look at the methods on localrepo - those are what you 
should consider targeting first." This would have enabled me to write 
extensions easier and faster. Asking extension authors to sift through a 
mountain of modules is comparatively very difficult. Since the Python 
API is not stable, you are asking extension authors to step outside a 
safe zone. Having clear extensibility points [on classes via methods 
such as this patch introduced] is a huge win for extension authors.

I think our disagreement is mostly due to there not being an obvious 
solution to the problem of giving extensions clear extensibility points 
for per-repo behavior. I have a [crazy] idea.

Hooks are a pattern to solve this problem. I think they work well. 
However, the currents hooks system is tailored for out-of-process 
consumers. What if we supplemented that hooks system by adding a 
mechanism for Python-only hooks that is geared towards giving extensions 
a more clear API. Here is an example extension:

def onpushopcreate(repo, pushop):
     # Do stuff with a pushoperation before push

def onpushopfinish(repo, pushop):
     # Do stuff with a pushoperation after push

def reposetup(ui, repo):
     if not repo.local():
         return

     repo.addhook('pushopcreate', onpushopcreate)
     repo.addhook('pushopfinish', onpushopfinish)

We sort of have this today with localrepo.prepushoutgoinghooks. What if 
we made it generic?

It solves the problem the patch is trying to solve, solves it with 
per-repo instance hacking, avoids module level monkeypatching, and gives 
extension authors a clear set of recommended extension points. I think 
there are wins all around.

What do you think? I would happily implement this if given the green light.