Patchwork extensions: move wrapfilecache function from fsmonitor

login
register
mail settings
Submitter Augie Fackler
Date June 8, 2017, 2:51 p.m.
Message ID <ff739458783d32b8b0ce.1496933498@augie-macbookpro2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/21245/
State Accepted
Headers show

Comments

Augie Fackler - June 8, 2017, 2:51 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1496933093 14400
#      Thu Jun 08 10:44:53 2017 -0400
# Node ID ff739458783d32b8b0ce97a6771513caf280c09b
# Parent  04c19c8082410049465e2cdc510e24801530c94b
extensions: move wrapfilecache function from fsmonitor

It makes more sense to put this in core, so other extensions can
trivially get access to it without having to rely on importing
fsmonitor.
Yuya Nishihara - June 8, 2017, 3:53 p.m.
On Thu, 08 Jun 2017 10:51:38 -0400, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1496933093 14400
> #      Thu Jun 08 10:44:53 2017 -0400
> # Node ID ff739458783d32b8b0ce97a6771513caf280c09b
> # Parent  04c19c8082410049465e2cdc510e24801530c94b
> extensions: move wrapfilecache function from fsmonitor
> 
> It makes more sense to put this in core, so other extensions can
> trivially get access to it without having to rely on importing
> fsmonitor.
> 
> diff --git a/hgext/fsmonitor/__init__.py b/hgext/fsmonitor/__init__.py
> --- a/hgext/fsmonitor/__init__.py
> +++ b/hgext/fsmonitor/__init__.py
> @@ -561,7 +561,8 @@ def wrapdirstate(orig, self):
>      return ds
>  
>  def extsetup(ui):
> -    wrapfilecache(localrepo.localrepository, 'dirstate', wrapdirstate)
> +    extensions.wrapfilecache(
> +        localrepo.localrepository, 'dirstate', wrapdirstate)
>      if pycompat.sysplatform == 'darwin':
>          # An assist for avoiding the dangling-symlink fsevents bug
>          extensions.wrapfunction(os, 'symlink', wrapsymlink)
> @@ -709,21 +710,3 @@ def reposetup(ui, repo):
>                  return overridestatus(orig, self, *args, **kwargs)
>  
>          repo.__class__ = fsmonitorrepo
> -
> -def wrapfilecache(cls, propname, wrapper):
> -    """Wraps a filecache property. These can't be wrapped using the normal
> -    wrapfunction. This should eventually go into upstream Mercurial.
> -    """
> -    assert callable(wrapper)
> -    for currcls in cls.__mro__:
> -        if propname in currcls.__dict__:
> -            origfn = currcls.__dict__[propname].func
> -            assert callable(origfn)
> -            def wrap(*args, **kwargs):
> -                return wrapper(origfn, *args, **kwargs)
> -            currcls.__dict__[propname].func = wrap
> -            break
> -
> -    if currcls is object:
> -        raise AttributeError(
> -            _("type '%s' has no property '%s'") % (cls, propname))

IIRC, Jun had a bad time with this. Is this a good pattern to reliably
wrap filecache?
Augie Fackler - June 8, 2017, 4:47 p.m.
> On Jun 8, 2017, at 11:53, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Thu, 08 Jun 2017 10:51:38 -0400, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1496933093 14400
>> #      Thu Jun 08 10:44:53 2017 -0400
>> # Node ID ff739458783d32b8b0ce97a6771513caf280c09b
>> # Parent  04c19c8082410049465e2cdc510e24801530c94b
>> extensions: move wrapfilecache function from fsmonitor
>> 
>> It makes more sense to put this in core, so other extensions can
>> trivially get access to it without having to rely on importing
>> fsmonitor.
>> 
>> diff --git a/hgext/fsmonitor/__init__.py b/hgext/fsmonitor/__init__.py
>> --- a/hgext/fsmonitor/__init__.py
>> +++ b/hgext/fsmonitor/__init__.py
>> @@ -561,7 +561,8 @@ def wrapdirstate(orig, self):
>>     return ds
>> 
>> def extsetup(ui):
>> -    wrapfilecache(localrepo.localrepository, 'dirstate', wrapdirstate)
>> +    extensions.wrapfilecache(
>> +        localrepo.localrepository, 'dirstate', wrapdirstate)
>>     if pycompat.sysplatform == 'darwin':
>>         # An assist for avoiding the dangling-symlink fsevents bug
>>         extensions.wrapfunction(os, 'symlink', wrapsymlink)
>> @@ -709,21 +710,3 @@ def reposetup(ui, repo):
>>                 return overridestatus(orig, self, *args, **kwargs)
>> 
>>         repo.__class__ = fsmonitorrepo
>> -
>> -def wrapfilecache(cls, propname, wrapper):
>> -    """Wraps a filecache property. These can't be wrapped using the normal
>> -    wrapfunction. This should eventually go into upstream Mercurial.
>> -    """
>> -    assert callable(wrapper)
>> -    for currcls in cls.__mro__:
>> -        if propname in currcls.__dict__:
>> -            origfn = currcls.__dict__[propname].func
>> -            assert callable(origfn)
>> -            def wrap(*args, **kwargs):
>> -                return wrapper(origfn, *args, **kwargs)
>> -            currcls.__dict__[propname].func = wrap
>> -            break
>> -
>> -    if currcls is object:
>> -        raise AttributeError(
>> -            _("type '%s' has no property '%s'") % (cls, propname))
> 
> IIRC, Jun had a bad time with this. Is this a good pattern to reliably
> wrap filecache?

No idea - we need it for some stuff internal to Google (ugh), and it felt like we should move this in core and then improve it as needed to correct defects.
Jun Wu - June 8, 2017, 10:32 p.m.
Excerpts from Yuya Nishihara's message of 2017-06-09 00:53:19 +0900:
> IIRC, Jun had a bad time with this. Is this a good pattern to reliably
> wrap filecache?

I think it's good to have it in core despite the implementation looks hacky.
This enables journal to work with fsmonitor better.
Pierre-Yves David - June 8, 2017, 10:49 p.m.
On 06/08/2017 05:47 PM, Augie Fackler wrote:
>
>> On Jun 8, 2017, at 11:53, Yuya Nishihara <yuya@tcha.org> wrote:
>>
>> On Thu, 08 Jun 2017 10:51:38 -0400, Augie Fackler wrote:
>>> # HG changeset patch
>>> # User Augie Fackler <augie@google.com>
>>> # Date 1496933093 14400
>>> #      Thu Jun 08 10:44:53 2017 -0400
>>> # Node ID ff739458783d32b8b0ce97a6771513caf280c09b
>>> # Parent  04c19c8082410049465e2cdc510e24801530c94b
>>> extensions: move wrapfilecache function from fsmonitor
>>>
>>> It makes more sense to put this in core, so other extensions can
>>> trivially get access to it without having to rely on importing
>>> fsmonitor.
>>>
>>> diff --git a/hgext/fsmonitor/__init__.py b/hgext/fsmonitor/__init__.py
>>> --- a/hgext/fsmonitor/__init__.py
>>> +++ b/hgext/fsmonitor/__init__.py
>>> @@ -561,7 +561,8 @@ def wrapdirstate(orig, self):
>>>     return ds
>>>
>>> def extsetup(ui):
>>> -    wrapfilecache(localrepo.localrepository, 'dirstate', wrapdirstate)
>>> +    extensions.wrapfilecache(
>>> +        localrepo.localrepository, 'dirstate', wrapdirstate)
>>>     if pycompat.sysplatform == 'darwin':
>>>         # An assist for avoiding the dangling-symlink fsevents bug
>>>         extensions.wrapfunction(os, 'symlink', wrapsymlink)
>>> @@ -709,21 +710,3 @@ def reposetup(ui, repo):
>>>                 return overridestatus(orig, self, *args, **kwargs)
>>>
>>>         repo.__class__ = fsmonitorrepo
>>> -
>>> -def wrapfilecache(cls, propname, wrapper):
>>> -    """Wraps a filecache property. These can't be wrapped using the normal
>>> -    wrapfunction. This should eventually go into upstream Mercurial.
>>> -    """
>>> -    assert callable(wrapper)
>>> -    for currcls in cls.__mro__:
>>> -        if propname in currcls.__dict__:
>>> -            origfn = currcls.__dict__[propname].func
>>> -            assert callable(origfn)
>>> -            def wrap(*args, **kwargs):
>>> -                return wrapper(origfn, *args, **kwargs)
>>> -            currcls.__dict__[propname].func = wrap
>>> -            break
>>> -
>>> -    if currcls is object:
>>> -        raise AttributeError(
>>> -            _("type '%s' has no property '%s'") % (cls, propname))
>>
>> IIRC, Jun had a bad time with this. Is this a good pattern to reliably
>> wrap filecache?
>
> No idea - we need it for some stuff internal to Google (ugh), and it felt like we should move this in core and then improve it as needed to correct defects.

+1

This that is something needed in evolve from time to time too. +1 for 
having something in core.

It seems sensible to move the fsmonitor version in core -as-is- and 
improve it later (in core) if necessary. I assume fsmonitor will still 
be happy if the patterns improves.

Cheers,
Yuya Nishihara - June 9, 2017, 12:54 p.m.
On Thu, 8 Jun 2017 15:32:46 -0700, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2017-06-09 00:53:19 +0900:
> > IIRC, Jun had a bad time with this. Is this a good pattern to reliably
> > wrap filecache?
> 
> I think it's good to have it in core despite the implementation looks hacky.
> This enables journal to work with fsmonitor better.

Okay, queued, thanks.
Yuya Nishihara - June 9, 2017, 1:02 p.m.
On Thu, 8 Jun 2017 23:49:27 +0100, Pierre-Yves David wrote:
> On 06/08/2017 05:47 PM, Augie Fackler wrote:
> >> On Jun 8, 2017, at 11:53, Yuya Nishihara <yuya@tcha.org> wrote:
> >> On Thu, 08 Jun 2017 10:51:38 -0400, Augie Fackler wrote:
> >>> # HG changeset patch
> >>> # User Augie Fackler <augie@google.com>
> >>> # Date 1496933093 14400
> >>> #      Thu Jun 08 10:44:53 2017 -0400
> >>> # Node ID ff739458783d32b8b0ce97a6771513caf280c09b
> >>> # Parent  04c19c8082410049465e2cdc510e24801530c94b
> >>> extensions: move wrapfilecache function from fsmonitor
> >>>
> >>> It makes more sense to put this in core, so other extensions can
> >>> trivially get access to it without having to rely on importing
> >>> fsmonitor.
> >>>
> >>> diff --git a/hgext/fsmonitor/__init__.py b/hgext/fsmonitor/__init__.py
> >>> --- a/hgext/fsmonitor/__init__.py
> >>> +++ b/hgext/fsmonitor/__init__.py
> >>> @@ -561,7 +561,8 @@ def wrapdirstate(orig, self):
> >>>     return ds
> >>>
> >>> def extsetup(ui):
> >>> -    wrapfilecache(localrepo.localrepository, 'dirstate', wrapdirstate)
> >>> +    extensions.wrapfilecache(
> >>> +        localrepo.localrepository, 'dirstate', wrapdirstate)
> >>>     if pycompat.sysplatform == 'darwin':
> >>>         # An assist for avoiding the dangling-symlink fsevents bug
> >>>         extensions.wrapfunction(os, 'symlink', wrapsymlink)
> >>> @@ -709,21 +710,3 @@ def reposetup(ui, repo):
> >>>                 return overridestatus(orig, self, *args, **kwargs)
> >>>
> >>>         repo.__class__ = fsmonitorrepo
> >>> -
> >>> -def wrapfilecache(cls, propname, wrapper):
> >>> -    """Wraps a filecache property. These can't be wrapped using the normal
> >>> -    wrapfunction. This should eventually go into upstream Mercurial.
> >>> -    """
> >>> -    assert callable(wrapper)
> >>> -    for currcls in cls.__mro__:
> >>> -        if propname in currcls.__dict__:
> >>> -            origfn = currcls.__dict__[propname].func
> >>> -            assert callable(origfn)
> >>> -            def wrap(*args, **kwargs):
> >>> -                return wrapper(origfn, *args, **kwargs)
> >>> -            currcls.__dict__[propname].func = wrap
> >>> -            break
> >>> -
> >>> -    if currcls is object:
> >>> -        raise AttributeError(
> >>> -            _("type '%s' has no property '%s'") % (cls, propname))
> >>
> >> IIRC, Jun had a bad time with this. Is this a good pattern to reliably
> >> wrap filecache?
> >
> > No idea - we need it for some stuff internal to Google (ugh), and it felt like we should move this in core and then improve it as needed to correct defects.
> 
> +1
> 
> This that is something needed in evolve from time to time too. +1 for 
> having something in core.
> 
> It seems sensible to move the fsmonitor version in core -as-is- and 
> improve it later (in core) if necessary. I assume fsmonitor will still 
> be happy if the patterns improves.

I think wrapfilecache() inherently conflicts with our class wrapping rule,
but anyway it would be better to not copy-paste wrapfilecache() hack around.

https://www.mercurial-scm.org/wiki/WritingExtensions#Wrapping_methods_on_the_ui_and_repo_classes

Patch

diff --git a/hgext/fsmonitor/__init__.py b/hgext/fsmonitor/__init__.py
--- a/hgext/fsmonitor/__init__.py
+++ b/hgext/fsmonitor/__init__.py
@@ -561,7 +561,8 @@  def wrapdirstate(orig, self):
     return ds
 
 def extsetup(ui):
-    wrapfilecache(localrepo.localrepository, 'dirstate', wrapdirstate)
+    extensions.wrapfilecache(
+        localrepo.localrepository, 'dirstate', wrapdirstate)
     if pycompat.sysplatform == 'darwin':
         # An assist for avoiding the dangling-symlink fsevents bug
         extensions.wrapfunction(os, 'symlink', wrapsymlink)
@@ -709,21 +710,3 @@  def reposetup(ui, repo):
                 return overridestatus(orig, self, *args, **kwargs)
 
         repo.__class__ = fsmonitorrepo
-
-def wrapfilecache(cls, propname, wrapper):
-    """Wraps a filecache property. These can't be wrapped using the normal
-    wrapfunction. This should eventually go into upstream Mercurial.
-    """
-    assert callable(wrapper)
-    for currcls in cls.__mro__:
-        if propname in currcls.__dict__:
-            origfn = currcls.__dict__[propname].func
-            assert callable(origfn)
-            def wrap(*args, **kwargs):
-                return wrapper(origfn, *args, **kwargs)
-            currcls.__dict__[propname].func = wrap
-            break
-
-    if currcls is object:
-        raise AttributeError(
-            _("type '%s' has no property '%s'") % (cls, propname))
diff --git a/mercurial/extensions.py b/mercurial/extensions.py
--- a/mercurial/extensions.py
+++ b/mercurial/extensions.py
@@ -308,6 +308,25 @@  def wrapcommand(table, command, wrapper,
     table[key] = tuple(newentry)
     return entry
 
+def wrapfilecache(cls, propname, wrapper):
+    """Wraps a filecache property.
+
+    These can't be wrapped using the normal wrapfunction.
+    """
+    assert callable(wrapper)
+    for currcls in cls.__mro__:
+        if propname in currcls.__dict__:
+            origfn = currcls.__dict__[propname].func
+            assert callable(origfn)
+            def wrap(*args, **kwargs):
+                return wrapper(origfn, *args, **kwargs)
+            currcls.__dict__[propname].func = wrap
+            break
+
+    if currcls is object:
+        raise AttributeError(
+            _("type '%s' has no property '%s'") % (cls, propname))
+
 def wrapfunction(container, funcname, wrapper):
     '''Wrap the function named funcname in container