Patchwork [2,of,4] dispatch: add an additional extensibility point during dispatch

On Wed, 2015-02-04 at 14:17 -0800, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1423087102 28800
> #      Wed Feb 04 13:58:22 2015 -0800
> # Node ID f6866444b15b1bb1c5e24ea31660e1f8deb78a83
> # Parent  c1b76e9330b1f39342899d8d53c3ddab388c3cb5
> dispatch: add an additional extensibility point during dispatch
> 
> An upcoming patch will introduce an ordering dependency between the
> color and pager extensions. Specifically, one's uisetup function must
> come before the other's.
> 
> There is no easy way to accomplish guaranteed ordering of extension
> functions. The Mercurial project has survived many years without an
> explicit dependency system between extensions. Instead of inventing
> one to solve this problem, we instead create a new, separate monkeypatch
> point for the color extension to use to ensure its code runs after
> pager's. It is ugly. But it is simple.

This is pretty simple too:

Is it sufficient for your purposes? There are a few other places that
want this sort of thing.

On Wed, Feb 4, 2015 at 2:43 PM, Matt Mackall <mpm@selenic.com> wrote:

> On Wed, 2015-02-04 at 14:17 -0800, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1423087102 28800
> > #      Wed Feb 04 13:58:22 2015 -0800
> > # Node ID f6866444b15b1bb1c5e24ea31660e1f8deb78a83
> > # Parent  c1b76e9330b1f39342899d8d53c3ddab388c3cb5
> > dispatch: add an additional extensibility point during dispatch
> >
> > An upcoming patch will introduce an ordering dependency between the
> > color and pager extensions. Specifically, one's uisetup function must
> > come before the other's.
> >
> > There is no easy way to accomplish guaranteed ordering of extension
> > functions. The Mercurial project has survived many years without an
> > explicit dependency system between extensions. Instead of inventing
> > one to solve this problem, we instead create a new, separate monkeypatch
> > point for the color extension to use to ensure its code runs after
> > pager's. It is ugly. But it is simple.
>
> This is pretty simple too:
>
> diff -r 7f375d2de945 mercurial/extensions.py
> --- a/mercurial/extensions.py   Mon Jan 26 14:30:12 2015 -0500
> +++ b/mercurial/extensions.py   Wed Feb 04 16:42:12 2015 -0600
> @@ -10,6 +10,7 @@
>  from i18n import _, gettext
>
>  _extensions = {}
> +_aftercallbacks = {}
>  _order = []
>  _ignore = ['hbisect', 'bookmarks', 'parentrevspec', 'interhg', 'inotify']
>
> @@ -87,6 +88,8 @@
>              mod = importh(name)
>      _extensions[shortname] = mod
>      _order.append(shortname)
> +    if shortname in _aftercallbacks:
> +        _aftercallbacks[shortname]()
>      return mod
>
>  def loadall(ui):
> @@ -123,6 +126,15 @@
>                      raise
>                  extsetup() # old extsetup with no ui argument
>
> +def aftersetup(extension, callback):
> +    """Run the specified function after the named extension is set up, or
> +    immediately"""
> +
> +    if extension in _extensions:
> +        callback()
> +    else:
> +        _aftercallbacks[extension] = callback
> +
>  def wrapcommand(table, command, wrapper):
>      '''Wrap the command named `command' in table
>
> Is it sufficient for your purposes? There are a few other places that
> want this sort of thing.


I think I can make this work. Although, the problem is still that pager and
color both do their magic at command dispatch time, not uisetup() time.

But there is one part that's a bit wonky. If color does an
aftersetup('pager') and pager is never loaded, the callback never fires. So
I guess we'd just double execute the color mode selection code in the
scenario where pager is loaded after color?

I started writing a patch that mucked about extensions._order. You could
even have extensions declare "after" or "before" dependencies via module
variables. I thought this was too much scope bloat.

Is there precedent for core code to call into bundled extensions like
color? It's very tempting to throw that into dispatch._runcommand...

On Wed, 2015-02-04 at 15:42 -0800, Gregory Szorc wrote:
> On Wed, Feb 4, 2015 at 2:43 PM, Matt Mackall <mpm@selenic.com> wrote:
> 
> > On Wed, 2015-02-04 at 14:17 -0800, Gregory Szorc wrote:
> > > # HG changeset patch
> > > # User Gregory Szorc <gregory.szorc@gmail.com>
> > > # Date 1423087102 28800
> > > #      Wed Feb 04 13:58:22 2015 -0800
> > > # Node ID f6866444b15b1bb1c5e24ea31660e1f8deb78a83
> > > # Parent  c1b76e9330b1f39342899d8d53c3ddab388c3cb5
> > > dispatch: add an additional extensibility point during dispatch
> > >
> > > An upcoming patch will introduce an ordering dependency between the
> > > color and pager extensions. Specifically, one's uisetup function must
> > > come before the other's.
> > >
> > > There is no easy way to accomplish guaranteed ordering of extension
> > > functions. The Mercurial project has survived many years without an
> > > explicit dependency system between extensions. Instead of inventing
> > > one to solve this problem, we instead create a new, separate monkeypatch
> > > point for the color extension to use to ensure its code runs after
> > > pager's. It is ugly. But it is simple.
> >
> > This is pretty simple too:
> >
> > diff -r 7f375d2de945 mercurial/extensions.py
> > --- a/mercurial/extensions.py   Mon Jan 26 14:30:12 2015 -0500
> > +++ b/mercurial/extensions.py   Wed Feb 04 16:42:12 2015 -0600
> > @@ -10,6 +10,7 @@
> >  from i18n import _, gettext
> >
> >  _extensions = {}
> > +_aftercallbacks = {}
> >  _order = []
> >  _ignore = ['hbisect', 'bookmarks', 'parentrevspec', 'interhg', 'inotify']
> >
> > @@ -87,6 +88,8 @@
> >              mod = importh(name)
> >      _extensions[shortname] = mod
> >      _order.append(shortname)
> > +    if shortname in _aftercallbacks:
> > +        _aftercallbacks[shortname]()
> >      return mod
> >
> >  def loadall(ui):
> > @@ -123,6 +126,15 @@
> >                      raise
> >                  extsetup() # old extsetup with no ui argument
> >
> > +def aftersetup(extension, callback):
> > +    """Run the specified function after the named extension is set up, or
> > +    immediately"""
> > +
> > +    if extension in _extensions:
> > +        callback()
> > +    else:
> > +        _aftercallbacks[extension] = callback
> > +
> >  def wrapcommand(table, command, wrapper):
> >      '''Wrap the command named `command' in table
> >
> > Is it sufficient for your purposes? There are a few other places that
> > want this sort of thing.
> 
> 
> I think I can make this work. Although, the problem is still that pager and
> color both do their magic at command dispatch time, not uisetup() time.
> 
> But there is one part that's a bit wonky. If color does an
> aftersetup('pager') and pager is never loaded, the callback never fires.

Yeah, I figured you'd just put the pager-specific logic in there?

> I started writing a patch that mucked about extensions._order. You could
> even have extensions declare "after" or "before" dependencies via module
> variables. I thought this was too much scope bloat.

I think the "usual" usage is going to be something like extension X
wants to call wrapcommand on on Y.Z... iff Y is loaded. The after
callback is well-suited to that.

It also gives us a straightforward way to handle mutual dependencies: if
X wants to wrap something in Y and vice versa. That's harder to do with
just the monolithic setup functions.

> Is there precedent for core code to call into bundled extensions like
> color? It's very tempting to throw that into dispatch._runcommand...

Nope.

On Wed, Feb 4, 2015 at 3:52 PM, Matt Mackall <mpm@selenic.com> wrote:

> On Wed, 2015-02-04 at 15:42 -0800, Gregory Szorc wrote:
> > On Wed, Feb 4, 2015 at 2:43 PM, Matt Mackall <mpm@selenic.com> wrote:
> >
> > > On Wed, 2015-02-04 at 14:17 -0800, Gregory Szorc wrote:
> > > > # HG changeset patch
> > > > # User Gregory Szorc <gregory.szorc@gmail.com>
> > > > # Date 1423087102 28800
> > > > #      Wed Feb 04 13:58:22 2015 -0800
> > > > # Node ID f6866444b15b1bb1c5e24ea31660e1f8deb78a83
> > > > # Parent  c1b76e9330b1f39342899d8d53c3ddab388c3cb5
> > > > dispatch: add an additional extensibility point during dispatch
> > > >
> > > > An upcoming patch will introduce an ordering dependency between the
> > > > color and pager extensions. Specifically, one's uisetup function must
> > > > come before the other's.
> > > >
> > > > There is no easy way to accomplish guaranteed ordering of extension
> > > > functions. The Mercurial project has survived many years without an
> > > > explicit dependency system between extensions. Instead of inventing
> > > > one to solve this problem, we instead create a new, separate
> monkeypatch
> > > > point for the color extension to use to ensure its code runs after
> > > > pager's. It is ugly. But it is simple.
> > >
> > > This is pretty simple too:
> > >
> > > diff -r 7f375d2de945 mercurial/extensions.py
> > > --- a/mercurial/extensions.py   Mon Jan 26 14:30:12 2015 -0500
> > > +++ b/mercurial/extensions.py   Wed Feb 04 16:42:12 2015 -0600
> > > @@ -10,6 +10,7 @@
> > >  from i18n import _, gettext
> > >
> > >  _extensions = {}
> > > +_aftercallbacks = {}
> > >  _order = []
> > >  _ignore = ['hbisect', 'bookmarks', 'parentrevspec', 'interhg',
> 'inotify']
> > >
> > > @@ -87,6 +88,8 @@
> > >              mod = importh(name)
> > >      _extensions[shortname] = mod
> > >      _order.append(shortname)
> > > +    if shortname in _aftercallbacks:
> > > +        _aftercallbacks[shortname]()
> > >      return mod
> > >
> > >  def loadall(ui):
> > > @@ -123,6 +126,15 @@
> > >                      raise
> > >                  extsetup() # old extsetup with no ui argument
> > >
> > > +def aftersetup(extension, callback):
> > > +    """Run the specified function after the named extension is set
> up, or
> > > +    immediately"""
> > > +
> > > +    if extension in _extensions:
> > > +        callback()
> > > +    else:
> > > +        _aftercallbacks[extension] = callback
> > > +
> > >  def wrapcommand(table, command, wrapper):
> > >      '''Wrap the command named `command' in table
> > >
> > > Is it sufficient for your purposes? There are a few other places that
> > > want this sort of thing.
> >
> >
> > I think I can make this work. Although, the problem is still that pager
> and
> > color both do their magic at command dispatch time, not uisetup() time.
> >
> > But there is one part that's a bit wonky. If color does an
> > aftersetup('pager') and pager is never loaded, the callback never fires.
>
> Yeah, I figured you'd just put the pager-specific logic in there?
>

Well, the code path is /slightly/ more complicated because we have to
resolve "auto". I kinda like how part 4 is just 2 additional lines to color.


> > I started writing a patch that mucked about extensions._order. You could
> > even have extensions declare "after" or "before" dependencies via module
> > variables. I thought this was too much scope bloat.
>
> I think the "usual" usage is going to be something like extension X
> wants to call wrapcommand on on Y.Z... iff Y is loaded. The after
> callback is well-suited to that.
>

Just to make sure I am grokking you, you are advocating wrapping the
wrapper function from another extension. e.g.

def afterpagerdispatch(orig, innerorig, ui, *args):
   if getattr(ui, 'pageractive'):
       ...

   return orig(innerorig, ui, *args)

def aftersetup():
    pager = extensions.find('pager')
    extensions.wrapcommand(pager, '_wrapcommand', afterpagerdispatch)

Won't the addition of the "orig" function to the arguments list for every
nested wrapping lead to problems (you don't know what level your wrapper is
installed at so argument positions won't be constant)?

On Wed, 2015-02-04 at 16:07 -0800, Gregory Szorc wrote:
> On Wed, Feb 4, 2015 at 3:52 PM, Matt Mackall <mpm@selenic.com> wrote:
> 
> > On Wed, 2015-02-04 at 15:42 -0800, Gregory Szorc wrote:
> > > On Wed, Feb 4, 2015 at 2:43 PM, Matt Mackall <mpm@selenic.com> wrote:
> > >
> > > > On Wed, 2015-02-04 at 14:17 -0800, Gregory Szorc wrote:
> > > > > # HG changeset patch
> > > > > # User Gregory Szorc <gregory.szorc@gmail.com>
> > > > > # Date 1423087102 28800
> > > > > #      Wed Feb 04 13:58:22 2015 -0800
> > > > > # Node ID f6866444b15b1bb1c5e24ea31660e1f8deb78a83
> > > > > # Parent  c1b76e9330b1f39342899d8d53c3ddab388c3cb5
> > > > > dispatch: add an additional extensibility point during dispatch
> > > > >
> > > > > An upcoming patch will introduce an ordering dependency between the
> > > > > color and pager extensions. Specifically, one's uisetup function must
> > > > > come before the other's.
> > > > >
> > > > > There is no easy way to accomplish guaranteed ordering of extension
> > > > > functions. The Mercurial project has survived many years without an
> > > > > explicit dependency system between extensions. Instead of inventing
> > > > > one to solve this problem, we instead create a new, separate
> > monkeypatch
> > > > > point for the color extension to use to ensure its code runs after
> > > > > pager's. It is ugly. But it is simple.
> > > >
> > > > This is pretty simple too:
> > > >
> > > > diff -r 7f375d2de945 mercurial/extensions.py
> > > > --- a/mercurial/extensions.py   Mon Jan 26 14:30:12 2015 -0500
> > > > +++ b/mercurial/extensions.py   Wed Feb 04 16:42:12 2015 -0600
> > > > @@ -10,6 +10,7 @@
> > > >  from i18n import _, gettext
> > > >
> > > >  _extensions = {}
> > > > +_aftercallbacks = {}
> > > >  _order = []
> > > >  _ignore = ['hbisect', 'bookmarks', 'parentrevspec', 'interhg',
> > 'inotify']
> > > >
> > > > @@ -87,6 +88,8 @@
> > > >              mod = importh(name)
> > > >      _extensions[shortname] = mod
> > > >      _order.append(shortname)
> > > > +    if shortname in _aftercallbacks:
> > > > +        _aftercallbacks[shortname]()
> > > >      return mod
> > > >
> > > >  def loadall(ui):
> > > > @@ -123,6 +126,15 @@
> > > >                      raise
> > > >                  extsetup() # old extsetup with no ui argument
> > > >
> > > > +def aftersetup(extension, callback):
> > > > +    """Run the specified function after the named extension is set
> > up, or
> > > > +    immediately"""
> > > > +
> > > > +    if extension in _extensions:
> > > > +        callback()
> > > > +    else:
> > > > +        _aftercallbacks[extension] = callback
> > > > +
> > > >  def wrapcommand(table, command, wrapper):
> > > >      '''Wrap the command named `command' in table
> > > >
> > > > Is it sufficient for your purposes? There are a few other places that
> > > > want this sort of thing.
> > >
> > >
> > > I think I can make this work. Although, the problem is still that pager
> > and
> > > color both do their magic at command dispatch time, not uisetup() time.
> > >
> > > But there is one part that's a bit wonky. If color does an
> > > aftersetup('pager') and pager is never loaded, the callback never fires.
> >
> > Yeah, I figured you'd just put the pager-specific logic in there?
> >
> 
> Well, the code path is /slightly/ more complicated because we have to
> resolve "auto". I kinda like how part 4 is just 2 additional lines to color.
> 
> 
> > > I started writing a patch that mucked about extensions._order. You could
> > > even have extensions declare "after" or "before" dependencies via module
> > > variables. I thought this was too much scope bloat.
> >
> > I think the "usual" usage is going to be something like extension X
> > wants to call wrapcommand on on Y.Z... iff Y is loaded. The after
> > callback is well-suited to that.
> >
> 
> Just to make sure I am grokking you, you are advocating wrapping the
> wrapper function from another extension. e.g.
> 
> def afterpagerdispatch(orig, innerorig, ui, *args):
>    if getattr(ui, 'pageractive'):
>        ...
> 
>    return orig(innerorig, ui, *args)
> 
> def aftersetup():
>     pager = extensions.find('pager')
>     extensions.wrapcommand(pager, '_wrapcommand', afterpagerdispatch)

I'm referring to the much more boring case of, say, largefiles wanting
to wrap record's record command if record is loaded. That becomes easy.
I'm not proposing a specific approach for the color vs pager problem,
just a way to have a well-ordered callback in color.

> Won't the addition of the "orig" function to the arguments list for every
> nested wrapping lead to problems (you don't know what level your wrapper is
> installed at so argument positions won't be constant)?

Normally the closures hide all that, but I'm actually not sure what
you're trying to do above.