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

login
register
mail settings
Submitter Katsunori FUJIWARA
Date March 5, 2014, 12:12 p.m.
Message ID <202d835ca3a173f80826.1394021527@juju>
Download mbox | patch
Permalink /patch/3851/
State Superseded
Headers show

Comments

Katsunori FUJIWARA - March 5, 2014, 12:12 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@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
Mads Kiilerich - March 6, 2014, 2:48 p.m.
On 03/05/2014 01:12 PM, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@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?

In general, largefiles and subrepos have very similar needs. I think we 
could make both much more pretty if subrepos could get better "extension 
points" and if largefiles started using the subrepo hooks.

/Mads

> +        """Extensions can override this function if additional checks have
> +        to be performed on outgoing revisions before pushing, or call it
> +        if they override push command.
> +
> +        'outgoing' argument is the result of 'discovery.findcommonoutgoing()'.
> +        """
> +        pass
> +
>       def push(self, remote, force=False, revs=None, newbranch=False):
>           return exchange.push(self, remote, force, revs, newbranch)
>   
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Katsunori FUJIWARA - March 7, 2014, 3:59 p.m.
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@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 ?


> In general, largefiles and subrepos have very similar needs. I think we 
> could make both much more pretty if subrepos could get better "extension 
> points" and if largefiles started using the subrepo hooks.
> 
> /Mads
> 
> > +        """Extensions can override this function if additional checks have
> > +        to be performed on outgoing revisions before pushing, or call it
> > +        if they override push command.
> > +
> > +        'outgoing' argument is the result of 'discovery.findcommonoutgoing()'.
> > +        """
> > +        pass
> > +
> >       def push(self, remote, force=False, revs=None, newbranch=False):
> >           return exchange.push(self, remote, force, revs, newbranch)
> >   
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@selenic.com
> > http://selenic.com/mailman/listinfo/mercurial-devel
> 
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Mads Kiilerich - March 7, 2014, 6:22 p.m.
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@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

Patch

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):
+        """Extensions can override this function if additional checks have
+        to be performed on outgoing revisions before pushing, or call it
+        if they override push command.
+
+        'outgoing' argument is the result of 'discovery.findcommonoutgoing()'.
+        """
+        pass
+
     def push(self, remote, force=False, revs=None, newbranch=False):
         return exchange.push(self, remote, force, revs, newbranch)