[PATCH 4 of 4] chgserver: move wrapchgui to runcommand

Jun Wu quark at fb.com
Tue Dec 20 11:46:38 EST 2016


Excerpts from Yuya Nishihara's message of 2016-12-20 23:29:17 +0900:
> On Tue, 20 Dec 2016 14:03:04 +0000, Jun Wu wrote:
> > Excerpts from Yuya Nishihara's message of 2016-12-20 21:47:23 +0900:
> > > BTW, is there any reason we have to delay the uisetup() call? I think we can
> > > just set req.ui in place of req.uisetup:
> > > 
> > >   class chgui(uimod.ui):
> > >       ...
> > >   req = dispatch.request(ui=chgui.load())
> > 
> > It's useful if runcommand needs to wrap on top of the side effects of other
> > extensions (ex. pager). In my WIP patch, chgcmdserver.uisetup looks like:
> > 
> >     def _uisetup(self, ui):
> >         _wrapui(ui, self._csystem)
> >         try:
> >             pager = extensions.find('pager')
> >         except KeyError:
> >             pass
> >         else:
> >             if util.safehasattr(pager, '_runpager'):
> >                 extensions.wrapfunction(pager, '_runpager', self._runpager)
> > 
> >     def _runpager(self, orig, ui, pagercmd):
> >         self._csystem.write(pagercmd, type='pager')
> >         while True:
> >             cmd = self.client.readline()[:-1]
> >             _log('pager subcommand: %s' % cmd)
> >             if cmd == 'attachio':
> >                 self.attachio(ui)
> >             elif cmd == '':
> >                 break
> >             else:
> >                 raise error.Abort(_('unexpected command %s') % cmd)
> > 
> > _runpager is coupled with chgcmdserver.
> 
> Could it be implemented without req.uisetup() if we had ui.pager() function?
> 
> https://www.mercurial-scm.org/wiki/PagerInCorePlan 
> 
> I believe we'll need to refactor the pager handling to fix a couple of pager
> issues (e.g. issue5377.) So I'm not enthusiastic about this _runpager() change.

I was aware of the pager refactoring. If we can figure out the final APIs,
and decouple the complex pager refactoring so the part needed by chg is
small, I can do that.

The pager API has 2 levels:

  - high-level (pagecmd): decide the command of the pager, call low-level
  - low-level (_runpager): accept a command and run it unconditionally

I think ui.pager() should be high-level, and chg only wants to replace the
low-level one.

Therefore a possible API is:

  - ui.pager() as the new high-level API which parses config and calls
    low-level method.
  - ui._runpager(pagercmd) as the low-level API which will be implemented
    differently by chg.

A possible approach is:

  1. Move pager._runpager to uimod.ui._runpager
  2. Override ui._runpager in chgui
  3. Move part of pagecmd to uimod.ui.pager (or startpager if we plan to
     have an endpager in the future)
  4. Revisit when to call ui.pager (complex)

1 and 2 are easy and related to chg. 3 does not block chg refactoring and is
simple enough so I could help by the way. 4 is a complex core part of the
pager plan but I'd like to avoid as it has nothing to do with chg.

+durin42 because of pager.

> > So uisetup is still necessary to wrap _runpager. The _wrapui change is
> > unnecessary if we use your proposal. In that case, both Patch 2 and 4 can be
> > dropped and Patch 1 and 3 are useful.
> 
> Yep, that's why I haven't pushed these patches yet.


More information about the Mercurial-devel mailing list