[PATCH 02 of 10] localrepo: introduce "checkoutgoing()" to extend outgoing revision check easily

Mads Kiilerich mads at kiilerich.com
Fri Mar 7 12:22:49 CST 2014


On 03/07/2014 04:59 PM, FUJIWARA Katsunori wrote:
> At Thu, 06 Mar 2014 15:48:31 +0100,
> Mads Kiilerich wrote:
>> On 03/05/2014 01:12 PM, FUJIWARA Katsunori wrote:
>>> # HG changeset patch
>>> # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
>>> # Date 1394019743 -32400
>>> #      Wed Mar 05 20:42:23 2014 +0900
>>> # Node ID 202d835ca3a173f808262677fccc796257945268
>>> # Parent  655cacfc3311c174c995e4bab13755b498cb43dc
>>> localrepo: introduce "checkoutgoing()" to extend outgoing revision check easily
>>>
>>> diff --git a/mercurial/exchange.py b/mercurial/exchange.py
>>> --- a/mercurial/exchange.py
>>> +++ b/mercurial/exchange.py
>>> @@ -103,6 +103,7 @@
>>>            try:
>>>                _pushdiscovery(pushop)
>>>                if _pushcheckoutgoing(pushop):
>>> +                pushop.repo.checkoutgoing(pushop.remote, pushop.outgoing)
>>>                    _pushchangeset(pushop)
>>>                _pushcomputecommonheads(pushop)
>>>                _pushsyncphase(pushop)
>>> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
>>> --- a/mercurial/localrepo.py
>>> +++ b/mercurial/localrepo.py
>>> @@ -1677,6 +1677,15 @@
>>>            """
>>>            pass
>>>    
>>> +    def checkoutgoing(self, remote, outgoing):
>> Yes, pretty please, let's get some nice hooks so we can get rid of some
>> of the ugly hacks largefiles had to use when it was a standalone extension.
>>
>> The 'check' name indicates to me that it can return a check value ...
>> which it can't.
>>
>> To me it sounds more like a 'hook'. Could we call it something like
>> 'prepushchangesethook'? I don't know if it could be some kind of 'real'
>> hooks like the other ones we have. If not, could we not just
>> override _pushchangeset or _pushcheckoutgoing instead?
> I just followed the style of existing hook point "checkpush()" in
> localrepository class.
>
>      def checkpush(self, force, revs):
>          """Extensions can override this function if additional checks have
>          to be performed before pushing, or call it if they override push
>          command.
>          """
>          pass
>
> 'prepushchangesethook' seems to be good, at least for me.
>
> IMHO, overriding "_pushchangeset()" or "_pushcheckoutgoing()" needs
> checking whether source repo is lfilesrepo or not, and looks worse
> than adding hook point(s) to localrepository class.
>
>
> BTW, hook function overriding requires overrider classes to invoke
> "super(overrider, self).hookfunc(...)" correctly to follow chain of
> overridings. But it can be broken easily and silently by careless
> extensions.
>
> Should I add new variable instantiated from "util.hooks" (like
> "cmdutil.summaryhooks") instead of adding new hook method to be
> overridden ?

It sounds like you have good reasons for doing it the way you do. I was 
not aware of checkpush. Introducing checkoutgoing do not increase the 
conceptual complexity - doing it the way I suggest would.

(But it seems to me like it perhaps would reduce the overall conceptual 
complexity if checkpush and checkoutgoing in another refactoring was 
renamed to something that made it clear that it just is an extension 
hook that only could have side effects.)

/Mads


More information about the Mercurial-devel mailing list