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

[Matt Mackall]

On Wed, 2015-02-04 at 16:07 -0800, Gregory Szorc wrote:
> On Wed, Feb 4, 2015 at 3:52 PM, Matt Mackall <mpm at 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 at selenic.com> wrote:
> > >
> > > > On Wed, 2015-02-04 at 14:17 -0800, Gregory Szorc wrote:
> > > > > # HG changeset patch
> > > > > # User Gregory Szorc <gregory.szorc at 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.

-- 
Mathematics is the supreme nostalgia of our time.