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
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?
> 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.
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.
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,
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.
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