Patchwork [2,of,2] exchange: refactor push so that extensions can wrap pushop

login
register
mail settings
Submitter Sean Farley
Date Oct. 15, 2015, 6:03 a.m.
Message ID <9984305b7184ceea37cb.1444889029@laptop.local>
Download mbox | patch
Permalink /patch/11097/
State Deferred
Delegated to: Pierre-Yves David
Headers show

Comments

Sean Farley - Oct. 15, 2015, 6:03 a.m.
# HG changeset patch
# User Sean Farley <sean@farley.io>
# Date 1444802693 25200
#      Tue Oct 13 23:04:53 2015 -0700
# Node ID 9984305b7184ceea37cb266f64f40e1021bdc10d
# Parent  eee993036c7c1593dbef90ae639cca5c07f10594
exchange: refactor push so that extensions can wrap pushop
Pierre-Yves David - Oct. 15, 2015, 10:28 a.m.
On 10/15/2015 07:03 AM, Sean Farley wrote:
> # HG changeset patch
> # User Sean Farley <sean@farley.io>
> # Date 1444802693 25200
> #      Tue Oct 13 23:04:53 2015 -0700
> # Node ID 9984305b7184ceea37cb266f64f40e1021bdc10d
> # Parent  eee993036c7c1593dbef90ae639cca5c07f10594
> exchange: refactor push so that extensions can wrap pushop

I'm unsure about why you need it and if this is the right way to do that.

What are you trying to achieve?

In all case I think I would prefer a clearer way to do this. Something 
like a 'pushopsetup' function or something.
Gregory Szorc - Oct. 15, 2015, 5:13 p.m.
On Thu, Oct 15, 2015 at 3:28 AM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 10/15/2015 07:03 AM, Sean Farley wrote:
>
>> # HG changeset patch
>> # User Sean Farley <sean@farley.io>
>> # Date 1444802693 25200
>> #      Tue Oct 13 23:04:53 2015 -0700
>> # Node ID 9984305b7184ceea37cb266f64f40e1021bdc10d
>> # Parent  eee993036c7c1593dbef90ae639cca5c07f10594
>> exchange: refactor push so that extensions can wrap pushop
>>
>
> I'm unsure about why you need it and if this is the right way to do that.
>
> What are you trying to achieve?
>
> In all case I think I would prefer a clearer way to do this. Something
> like a 'pushopsetup' function or something.


I've also wanted this before. Use case is extensions may want to change
attributes of pushoperation or pulloperation when they are constructed. It
is sometimes difficult to influence these attributes from just the function
arguments to push() and pull().

I would still prefer we stop refactoring functions to facilitate
monkeypatching and would institute something more formal, like
https://selenic.com/pipermail/mercurial-devel/2014-September/062046.html.
There was lukewarm reception to that idea. It was not accepted because an
in-tree consumer (probably an extension) was needed. Perhaps we can find
something for 3.7...
Pierre-Yves David - Oct. 15, 2015, 5:15 p.m.
On 10/15/2015 06:13 PM, Gregory Szorc wrote:
> On Thu, Oct 15, 2015 at 3:28 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>
>
>     On 10/15/2015 07:03 AM, Sean Farley wrote:
>
>         # HG changeset patch
>         # User Sean Farley <sean@farley.io <mailto:sean@farley.io>>
>         # Date 1444802693 25200
>         #      Tue Oct 13 23:04:53 2015 -0700
>         # Node ID 9984305b7184ceea37cb266f64f40e1021bdc10d
>         # Parent  eee993036c7c1593dbef90ae639cca5c07f10594
>         exchange: refactor push so that extensions can wrap pushop
>
>
>     I'm unsure about why you need it and if this is the right way to do
>     that.
>
>     What are you trying to achieve?
>
>     In all case I think I would prefer a clearer way to do this.
>     Something like a 'pushopsetup' function or something.
>
>
> I've also wanted this before. Use case is extensions may want to change
> attributes of pushoperation or pulloperation when they are constructed.
> It is sometimes difficult to influence these attributes from just the
> function arguments to push() and pull().
>
> I would still prefer we stop refactoring functions to facilitate
> monkeypatching and would institute something more formal, like
> https://selenic.com/pipermail/mercurial-devel/2014-September/062046.html. There
> was lukewarm reception to that idea. It was not accepted because an
> in-tree consumer (probably an extension) was needed. Perhaps we can find
> something for 3.7...

pull have a oparg argument that is passed to the pull operation. We 
should probably go that route if we want to extensibility in push operation.

(still wondering smf usecase)
Sean Farley - Oct. 15, 2015, 5:45 p.m.
Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:

> On 10/15/2015 06:13 PM, Gregory Szorc wrote:
>> On Thu, Oct 15, 2015 at 3:28 AM, Pierre-Yves David
>> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>> wrote:
>>
>>
>>
>>     On 10/15/2015 07:03 AM, Sean Farley wrote:
>>
>>         # HG changeset patch
>>         # User Sean Farley <sean@farley.io <mailto:sean@farley.io>>
>>         # Date 1444802693 25200
>>         #      Tue Oct 13 23:04:53 2015 -0700
>>         # Node ID 9984305b7184ceea37cb266f64f40e1021bdc10d
>>         # Parent  eee993036c7c1593dbef90ae639cca5c07f10594
>>         exchange: refactor push so that extensions can wrap pushop
>>
>>
>>     I'm unsure about why you need it and if this is the right way to do
>>     that.
>>
>>     What are you trying to achieve?
>>
>>     In all case I think I would prefer a clearer way to do this.
>>     Something like a 'pushopsetup' function or something.
>>
>>
>> I've also wanted this before. Use case is extensions may want to change
>> attributes of pushoperation or pulloperation when they are constructed.
>> It is sometimes difficult to influence these attributes from just the
>> function arguments to push() and pull().

It is exactly as Greg said here.

>> I would still prefer we stop refactoring functions to facilitate
>> monkeypatching and would institute something more formal, like
>> https://selenic.com/pipermail/mercurial-devel/2014-September/062046.html. There
>> was lukewarm reception to that idea. It was not accepted because an
>> in-tree consumer (probably an extension) was needed. Perhaps we can find
>> something for 3.7...

Cool, but we want something in before the freeze.

> pull have a oparg argument that is passed to the pull operation. We 
> should probably go that route if we want to extensibility in push operation.
>
> (still wondering smf usecase)

For remote bookmarks, we want to endow the push operation with some new
flags. Since push creates the pushop, we want to skip that and create it
ourselves so we can pass around the new attributes.

The 'opargs' idea for pullop doesn't go far enough. Currently, there is
no way to actually use opargs to set new attributes. If you want, we can
do something like this for pushop / pullop:

def __init__(..., opargs):
  ...
  for key, val in opargs.iteritems():
    settattr(self, key, val)

Thoughts?
Gregory Szorc - Oct. 15, 2015, 5:51 p.m.
On Thu, Oct 15, 2015 at 10:45 AM, Sean Farley <sean@farley.io> wrote:

>
> Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:
>
> > On 10/15/2015 06:13 PM, Gregory Szorc wrote:
> >> On Thu, Oct 15, 2015 at 3:28 AM, Pierre-Yves David
> >> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org
> >>
> >> wrote:
> >>
> >>
> >>
> >>     On 10/15/2015 07:03 AM, Sean Farley wrote:
> >>
> >>         # HG changeset patch
> >>         # User Sean Farley <sean@farley.io <mailto:sean@farley.io>>
> >>         # Date 1444802693 25200
> >>         #      Tue Oct 13 23:04:53 2015 -0700
> >>         # Node ID 9984305b7184ceea37cb266f64f40e1021bdc10d
> >>         # Parent  eee993036c7c1593dbef90ae639cca5c07f10594
> >>         exchange: refactor push so that extensions can wrap pushop
> >>
> >>
> >>     I'm unsure about why you need it and if this is the right way to do
> >>     that.
> >>
> >>     What are you trying to achieve?
> >>
> >>     In all case I think I would prefer a clearer way to do this.
> >>     Something like a 'pushopsetup' function or something.
> >>
> >>
> >> I've also wanted this before. Use case is extensions may want to change
> >> attributes of pushoperation or pulloperation when they are constructed.
> >> It is sometimes difficult to influence these attributes from just the
> >> function arguments to push() and pull().
>
> It is exactly as Greg said here.
>
> >> I would still prefer we stop refactoring functions to facilitate
> >> monkeypatching and would institute something more formal, like
> >>
> https://selenic.com/pipermail/mercurial-devel/2014-September/062046.html.
> There
> >> was lukewarm reception to that idea. It was not accepted because an
> >> in-tree consumer (probably an extension) was needed. Perhaps we can find
> >> something for 3.7...
>
> Cool, but we want something in before the freeze.
>
> > pull have a oparg argument that is passed to the pull operation. We
> > should probably go that route if we want to extensibility in push
> operation.
> >
> > (still wondering smf usecase)
>
> For remote bookmarks, we want to endow the push operation with some new
> flags. Since push creates the pushop, we want to skip that and create it
> ourselves so we can pass around the new attributes.
>
> The 'opargs' idea for pullop doesn't go far enough. Currently, there is
> no way to actually use opargs to set new attributes. If you want, we can
> do something like this for pushop / pullop:
>
> def __init__(..., opargs):
>   ...
>   for key, val in opargs.iteritems():
>     settattr(self, key, val)
>
> Thoughts?
>
>
I'd prefer the ability for extensions to "hook" the {push,pull}operation
instance as close to construction time as possible. I like your patch as is
for a quick fix. There is precedent for doing ugly hacks like this in the
name of 3rd party extensions. e.g.
https://selenic.com/repo/hg/rev/aae14b3d0a9c
Pierre-Yves David - Oct. 15, 2015, 5:53 p.m.
On 10/15/2015 06:45 PM, Sean Farley wrote:
>
> Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:
>
>> On 10/15/2015 06:13 PM, Gregory Szorc wrote:
>>> On Thu, Oct 15, 2015 at 3:28 AM, Pierre-Yves David
>>> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>>> wrote:
>>>
>>>
>>>
>>>      On 10/15/2015 07:03 AM, Sean Farley wrote:
>>>
>>>          # HG changeset patch
>>>          # User Sean Farley <sean@farley.io <mailto:sean@farley.io>>
>>>          # Date 1444802693 25200
>>>          #      Tue Oct 13 23:04:53 2015 -0700
>>>          # Node ID 9984305b7184ceea37cb266f64f40e1021bdc10d
>>>          # Parent  eee993036c7c1593dbef90ae639cca5c07f10594
>>>          exchange: refactor push so that extensions can wrap pushop
>>>
>>>
>>>      I'm unsure about why you need it and if this is the right way to do
>>>      that.
>>>
>>>      What are you trying to achieve?
>>>
>>>      In all case I think I would prefer a clearer way to do this.
>>>      Something like a 'pushopsetup' function or something.
>>>
>>>
>>> I've also wanted this before. Use case is extensions may want to change
>>> attributes of pushoperation or pulloperation when they are constructed.
>>> It is sometimes difficult to influence these attributes from just the
>>> function arguments to push() and pull().
>
> It is exactly as Greg said here.
>
>>> I would still prefer we stop refactoring functions to facilitate
>>> monkeypatching and would institute something more formal, like
>>> https://selenic.com/pipermail/mercurial-devel/2014-September/062046.html. There
>>> was lukewarm reception to that idea. It was not accepted because an
>>> in-tree consumer (probably an extension) was needed. Perhaps we can find
>>> something for 3.7...
>
> Cool, but we want something in before the freeze.
>
>> pull have a oparg argument that is passed to the pull operation. We
>> should probably go that route if we want to extensibility in push operation.
>>
>> (still wondering smf usecase)
>
> For remote bookmarks, we want to endow the push operation with some new
> flags. Since push creates the pushop, we want to skip that and create it
> ourselves so we can pass around the new attributes.
>
> The 'opargs' idea for pullop doesn't go far enough. Currently, there is
> no way to actually use opargs to set new attributes. If you want, we can
> do something like this for pushop / pullop:
>
> def __init__(..., opargs):
>    ...
>    for key, val in opargs.iteritems():
>      settattr(self, key, val)
>
> Thoughts?

if it work for pulloperation is works for pushoperation

You should wrap/subclass pushoperation to make use of this new arguments 
(and voila).
Ryan McElroy - Oct. 15, 2015, 5:54 p.m.
On 10/15/2015 10:45 AM, Sean Farley wrote:
> Pierre-Yves David <pierre-yves.david@ens-lyon.org> writes:
>
>> On 10/15/2015 06:13 PM, Gregory Szorc wrote:
>>> On Thu, Oct 15, 2015 at 3:28 AM, Pierre-Yves David
>>> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
>>> wrote:
>>>
>>>
>>>
>>>      On 10/15/2015 07:03 AM, Sean Farley wrote:
>>>
>>>          # HG changeset patch
>>>          # User Sean Farley <sean@farley.io <mailto:sean@farley.io>>
>>>          # Date 1444802693 25200
>>>          #      Tue Oct 13 23:04:53 2015 -0700
>>>          # Node ID 9984305b7184ceea37cb266f64f40e1021bdc10d
>>>          # Parent  eee993036c7c1593dbef90ae639cca5c07f10594
>>>          exchange: refactor push so that extensions can wrap pushop
>>>
>>>
>>>      I'm unsure about why you need it and if this is the right way to do
>>>      that.
>>>
>>>      What are you trying to achieve?
>>>
>>>      In all case I think I would prefer a clearer way to do this.
>>>      Something like a 'pushopsetup' function or something.

A simple pushopsetup() function that returns a pushop would work for 
this and be pretty clear, I think. Thoughts on this approach, Sean?

>>>
>>>
>>> I've also wanted this before. Use case is extensions may want to change
>>> attributes of pushoperation or pulloperation when they are constructed.
>>> It is sometimes difficult to influence these attributes from just the
>>> function arguments to push() and pull().
> It is exactly as Greg said here.
>
>>> I would still prefer we stop refactoring functions to facilitate
>>> monkeypatching and would institute something more formal, like
>>> https://selenic.com/pipermail/mercurial-devel/2014-September/062046.html. There
>>> was lukewarm reception to that idea. It was not accepted because an
>>> in-tree consumer (probably an extension) was needed. Perhaps we can find
>>> something for 3.7...
> Cool, but we want something in before the freeze.
>
>> pull have a oparg argument that is passed to the pull operation. We
>> should probably go that route if we want to extensibility in push operation.
>>
>> (still wondering smf usecase)
> For remote bookmarks, we want to endow the push operation with some new
> flags. Since push creates the pushop, we want to skip that and create it
> ourselves so we can pass around the new attributes.

Go go into even more detail, we want to be able to set a new attribute 
like 'newbranch' on the pushop. Right now, we're using global state 
inside of remotenames which is ugly and error-prone.

>
> The 'opargs' idea for pullop doesn't go far enough. Currently, there is
> no way to actually use opargs to set new attributes. If you want, we can
> do something like this for pushop / pullop:
>
> def __init__(..., opargs):
>    ...
>    for key, val in opargs.iteritems():
>      settattr(self, key, val)

This sounds more complicated, I like the pushopsetup() idea proposed 
above, but whatever lets us move away from globals would be good.

>
> Thoughts?
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -183,11 +183,13 @@  def push(repo, remote, force=False, revs
       - 0 means HTTP error
       - 1 means we pushed and remote head count is unchanged *or*
         we have outgoing changesets but refused to push
       - other values as described by addchangegroup()
     '''
-    pushop = pushoperation(repo, remote, force, revs, newbranch, bookmarks)
+    return _push(pushoperation(repo, remote, force, revs, newbranch, bookmarks))
+
+def _push(pushop):
     if pushop.remote.local():
         missing = (set(pushop.repo.requirements)
                    - pushop.remote.local().supported)
         if missing:
             msg = _("required features are not"