Patchwork [3,of,3] extensions: add unwrapfunction to undo wrapfunction

login
register
mail settings
Submitter Jun Wu
Date July 1, 2016, 12:09 p.m.
Message ID <7a68b01000d12e1795d1.1467374945@x1c>
Download mbox | patch
Permalink /patch/15700/
State Superseded
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - July 1, 2016, 12:09 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1467374153 -3600
#      Fri Jul 01 12:55:53 2016 +0100
# Node ID 7a68b01000d12e1795d142cdb32fdd62a5ffc8cb
# Parent  de798903374922317eb2cbca9733ba0cea415780
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 7a68b01000d1
extensions: add unwrapfunction to undo wrapfunction

Before this patch, we don't have a safe way to undo a wrapfunction because
other extensions may wrap the same function and calling setattr will undo
them accidentally.

This patch adds an "unwrapfunction" to address the issue. It removes the
wrapper from the wrapper chain, and re-wraps everything, which is not the
most efficient but short and easy to understand. We can revisit the code
if we have perf issues with long chains.

The "undo" feature is useful in cases like wrapping a function just in
a scope. And it allows extensions running with chg to add and remove their
wrappers dynamically in reposetup without starting a new chgserver.
Martijn Pieters - July 1, 2016, 12:41 p.m.
On 1 July 2016 at 13:09, Jun Wu <quark@fb.com> wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1467374153 -3600
> #      Fri Jul 01 12:55:53 2016 +0100
> # Node ID 7a68b01000d12e1795d142cdb32fdd62a5ffc8cb
> # Parent  de798903374922317eb2cbca9733ba0cea415780
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 7a68b01000d1
> extensions: add unwrapfunction to undo wrapfunction
>
> Before this patch, we don't have a safe way to undo a wrapfunction because
> other extensions may wrap the same function and calling setattr will undo
> them accidentally.
>
> This patch adds an "unwrapfunction" to address the issue. It removes the
> wrapper from the wrapper chain, and re-wraps everything, which is not the
> most efficient but short and easy to understand. We can revisit the code
> if we have perf issues with long chains.
>
> The "undo" feature is useful in cases like wrapping a function just in
> a scope. And it allows extensions running with chg to add and remove their
> wrappers dynamically in reposetup without starting a new chgserver.
>
> diff --git a/mercurial/extensions.py b/mercurial/extensions.py
> --- a/mercurial/extensions.py
> +++ b/mercurial/extensions.py
> @@ -303,6 +303,19 @@ def wrapfunction(container, funcname, wr
>      setattr(container, funcname, wrap)
>      return origfn
>
> +def unwrapfunction(container, funcname, wrapper):
> +    '''Undo wrapfunction
> +
> +    Pass the same arguments with wrapfunction to undo a wrap.
> +    Handles stacked wrappers correctly.
> +    '''
> +    chain = getwrapperchain(container, funcname)
> +    origfn = chain.pop()
> +    chain.remove(wrapper)
> +    setattr(container, funcname, origfn)
> +    for wrapper in reversed(chain):
> +        wrapfunction(container, funcname, wrapper)
> +


You could just use a stack of functions to rewrap; this avoids having
to re-do the whole chain:

    chain = getwrapperchain(container, funcname)[::-1]
    rewrap = []
    while chain:
        wrapper = chain.pop()
        if wrapper == origfn:
            break
        rewrap.append(wrapper)
    if chain:
        # we found the original and removed it; rewrap whatever we had to remove
                 # to find it.
        setattr(container, funcname, chain[-1])
        while rewrap:
            wrapfunction(container, funcname, rewrap.pop())

>  def getwrapperchain(container, funcname):
>      '''get a chain of wrappers of a function
>
> diff --git a/tests/test-extensions-wrapfunction.py b/tests/test-extensions-wrapfunction.py
> new file mode 100644
> --- /dev/null
> +++ b/tests/test-extensions-wrapfunction.py
> @@ -0,0 +1,24 @@
> +from __future__ import absolute_import, print_function
> +
> +from mercurial import extensions
> +
> +def genwrap(x):
> +    def f(orig, *args, **kwds):
> +        return [x] + orig(*args, **kwds)
> +    return f
> +
> +wrappers = [genwrap(i) for i in range(5)]
> +
> +class dummyclass(object):
> +    def foo(self):
> +        return ['orig']
> +
> +dummy = dummyclass()
> +
> +for w in wrappers + [wrappers[0]]:
> +    extensions.wrapfunction(dummy, 'foo', w)
> +    print(dummy.foo())
> +
> +for i in [3, 0, 4, 0, 2, 1]:
> +    extensions.unwrapfunction(dummy, 'foo', wrappers[i])
> +    print(dummy.foo())
> diff --git a/tests/test-extensions-wrapfunction.py.out b/tests/test-extensions-wrapfunction.py.out
> new file mode 100644
> --- /dev/null
> +++ b/tests/test-extensions-wrapfunction.py.out
> @@ -0,0 +1,12 @@
> +[0, 'orig']
> +[1, 0, 'orig']
> +[2, 1, 0, 'orig']
> +[3, 2, 1, 0, 'orig']
> +[4, 3, 2, 1, 0, 'orig']
> +[0, 4, 3, 2, 1, 0, 'orig']
> +[0, 4, 2, 1, 0, 'orig']
> +[4, 2, 1, 0, 'orig']
> +[2, 1, 0, 'orig']
> +[2, 1, 'orig']
> +[1, 'orig']
> +['orig']
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Jun Wu - July 1, 2016, 1:27 p.m.
Yes the current code is not optimized for performance (as mentioned in
commit message). The optimized code would be re-inventing getwrapperchain
and I want to avoid duplication. getwrapperchain is exposed as an API
because I plan to have something like "debugwraps" to visualize the wrapped
functions in the future.

> You could just use a stack of functions to rewrap; this avoids having
> to re-do the whole chain:
> 
>     chain = getwrapperchain(container, funcname)[::-1]
>     rewrap = []
>     while chain:
>         wrapper = chain.pop()
>         if wrapper == origfn:
>             break
>         rewrap.append(wrapper)
>     if chain:
>         # we found the original and removed it; rewrap whatever we had to remove
>                  # to find it.
>         setattr(container, funcname, chain[-1])
>         while rewrap:
>             wrapfunction(container, funcname, rewrap.pop())

I don't think this works. There are two different wrappers:

  1. the one you passed to wrapfunction, say it's "f"
  2. f.bind(origfunc), say it's "g"

getwrapperchain returns "f"s to make them comparable and hides "g"s for
simplicity. We need to setattr to a "g".
Martijn Pieters - July 1, 2016, 2:02 p.m.
On 1 July 2016 at 14:27, Jun Wu <quark@fb.com> wrote:
> Yes the current code is not optimized for performance (as mentioned in
> commit message). The optimized code would be re-inventing getwrapperchain
> and I want to avoid duplication. getwrapperchain is exposed as an API
> because I plan to have something like "debugwraps" to visualize the wrapped
> functions in the future.
>
>> You could just use a stack of functions to rewrap; this avoids having
>> to re-do the whole chain:
>>
>>     chain = getwrapperchain(container, funcname)[::-1]
>>     rewrap = []
>>     while chain:
>>         wrapper = chain.pop()
>>         if wrapper == origfn:
>>             break
>>         rewrap.append(wrapper)
>>     if chain:
>>         # we found the original and removed it; rewrap whatever we had to remove
>>                  # to find it.
>>         setattr(container, funcname, chain[-1])
>>         while rewrap:
>>             wrapfunction(container, funcname, rewrap.pop())
>
> I don't think this works. There are two different wrappers:
>
>   1. the one you passed to wrapfunction, say it's "f"
>   2. f.bind(origfunc), say it's "g"
>
> getwrapperchain returns "f"s to make them comparable and hides "g"s for
> simplicity. We need to setattr to a "g".

Ah, yes, you are correct. I had the wrong mental model (decorators),
while the actual attribute on the container is a chain of closures
really.

This also invalidates my __wrapper__ argument on the other patch.
Yuya Nishihara - July 3, 2016, 8:11 a.m.
On Fri, 1 Jul 2016 13:09:05 +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1467374153 -3600
> #      Fri Jul 01 12:55:53 2016 +0100
> # Node ID 7a68b01000d12e1795d142cdb32fdd62a5ffc8cb
> # Parent  de798903374922317eb2cbca9733ba0cea415780
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 7a68b01000d1
> extensions: add unwrapfunction to undo wrapfunction
> 
> Before this patch, we don't have a safe way to undo a wrapfunction because
> other extensions may wrap the same function and calling setattr will undo
> them accidentally.
> 
> This patch adds an "unwrapfunction" to address the issue. It removes the
> wrapper from the wrapper chain, and re-wraps everything, which is not the
> most efficient but short and easy to understand. We can revisit the code
> if we have perf issues with long chains.
> 
> The "undo" feature is useful in cases like wrapping a function just in
> a scope. And it allows extensions running with chg to add and remove their
> wrappers dynamically in reposetup without starting a new chgserver.
> 
> diff --git a/mercurial/extensions.py b/mercurial/extensions.py
> --- a/mercurial/extensions.py
> +++ b/mercurial/extensions.py
> @@ -303,6 +303,19 @@ def wrapfunction(container, funcname, wr
>      setattr(container, funcname, wrap)
>      return origfn
>  
> +def unwrapfunction(container, funcname, wrapper):
> +    '''Undo wrapfunction
> +
> +    Pass the same arguments with wrapfunction to undo a wrap.
> +    Handles stacked wrappers correctly.
> +    '''

"unwrap" sounds different to me from what this function does. I expected it
would do unwrapfunction(container, funcname) -> last_wrapper. Any idea?

> +    chain = getwrapperchain(container, funcname)
> +    origfn = chain.pop()
> +    chain.remove(wrapper)
> +    setattr(container, funcname, origfn)
> +    for wrapper in reversed(chain):
> +        wrapfunction(container, funcname, wrapper)

I noticed we can't apply unwrapfunction() to wrapped commands even though
they have _origfunc and _unboundwrapper attributes. But that should be okay
because wrapcommand() has distinct interface from wrapfunction().
Jun Wu - July 6, 2016, 3:47 p.m.
Sorry for the late reply. This thread gets broken while I was debugging my
email client. I will send a V2.

Excerpts from Yuya Nishihara's message of 2016-07-03 17:11:00 +0900:
> "unwrap" sounds different to me from what this function does. I expected it
> would do unwrapfunction(container, funcname) -> last_wrapper. Any idea?

Good idea. I think it makes sense to let the "wrapper" argument default
to None, and in that case, the last wrapper will be removed.

I will also add a "return".

> I noticed we can't apply unwrapfunction() to wrapped commands even though
> they have _origfunc and _unboundwrapper attributes. But that should be okay
> because wrapcommand() has distinct interface from wrapfunction().

Yes it's because unwrapfunction uses wrapfunction. I think it's fine for
now. If we want to do clean-ups, we can try to share code between
wrapfunciont and wrapcommand, and maybe store extra metadata as attributes
of the wrapped method.

Patch

diff --git a/mercurial/extensions.py b/mercurial/extensions.py
--- a/mercurial/extensions.py
+++ b/mercurial/extensions.py
@@ -303,6 +303,19 @@  def wrapfunction(container, funcname, wr
     setattr(container, funcname, wrap)
     return origfn
 
+def unwrapfunction(container, funcname, wrapper):
+    '''Undo wrapfunction
+
+    Pass the same arguments with wrapfunction to undo a wrap.
+    Handles stacked wrappers correctly.
+    '''
+    chain = getwrapperchain(container, funcname)
+    origfn = chain.pop()
+    chain.remove(wrapper)
+    setattr(container, funcname, origfn)
+    for wrapper in reversed(chain):
+        wrapfunction(container, funcname, wrapper)
+
 def getwrapperchain(container, funcname):
     '''get a chain of wrappers of a function
 
diff --git a/tests/test-extensions-wrapfunction.py b/tests/test-extensions-wrapfunction.py
new file mode 100644
--- /dev/null
+++ b/tests/test-extensions-wrapfunction.py
@@ -0,0 +1,24 @@ 
+from __future__ import absolute_import, print_function
+
+from mercurial import extensions
+
+def genwrap(x):
+    def f(orig, *args, **kwds):
+        return [x] + orig(*args, **kwds)
+    return f
+
+wrappers = [genwrap(i) for i in range(5)]
+
+class dummyclass(object):
+    def foo(self):
+        return ['orig']
+
+dummy = dummyclass()
+
+for w in wrappers + [wrappers[0]]:
+    extensions.wrapfunction(dummy, 'foo', w)
+    print(dummy.foo())
+
+for i in [3, 0, 4, 0, 2, 1]:
+    extensions.unwrapfunction(dummy, 'foo', wrappers[i])
+    print(dummy.foo())
diff --git a/tests/test-extensions-wrapfunction.py.out b/tests/test-extensions-wrapfunction.py.out
new file mode 100644
--- /dev/null
+++ b/tests/test-extensions-wrapfunction.py.out
@@ -0,0 +1,12 @@ 
+[0, 'orig']
+[1, 0, 'orig']
+[2, 1, 0, 'orig']
+[3, 2, 1, 0, 'orig']
+[4, 3, 2, 1, 0, 'orig']
+[0, 4, 3, 2, 1, 0, 'orig']
+[0, 4, 2, 1, 0, 'orig']
+[4, 2, 1, 0, 'orig']
+[2, 1, 0, 'orig']
+[2, 1, 'orig']
+[1, 'orig']
+['orig']