Patchwork [4,of,4] chgserver: move wrapchgui to runcommand

login
register
mail settings
Submitter Jun Wu
Date Dec. 16, 2016, 3:01 p.m.
Message ID <8fe60192f17f6ae99fa6.1481900481@x1c>
Download mbox | patch
Permalink /patch/17936/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Jun Wu - Dec. 16, 2016, 3:01 p.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1481900282 0
#      Fri Dec 16 14:58:02 2016 +0000
# Node ID 8fe60192f17f6ae99fa66c6bce1ec306772e31df
# Parent  eb3017f14d56dfdc9870b06a684ef9bcf7a030e6
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 8fe60192f17f
chgserver: move wrapchgui to runcommand

The wrapping logic changes ui.system, which should only affect runcommand.
This makes future refactoring a bit cleaner.
Yuya Nishihara - Dec. 18, 2016, 8:30 a.m.
On Fri, 16 Dec 2016 15:01:21 +0000, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1481900282 0
> #      Fri Dec 16 14:58:02 2016 +0000
> # Node ID 8fe60192f17f6ae99fa66c6bce1ec306772e31df
> # Parent  eb3017f14d56dfdc9870b06a684ef9bcf7a030e6
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 8fe60192f17f
> chgserver: move wrapchgui to runcommand
> 
> The wrapping logic changes ui.system, which should only affect runcommand.
> This makes future refactoring a bit cleaner.
> 
> diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
> --- a/mercurial/chgserver.py
> +++ b/mercurial/chgserver.py
> @@ -330,5 +330,4 @@ class chgcmdserver(commandserver.server)
>      def __init__(self, ui, repo, fin, fout, sock, hashstate, baseaddress):
>          self._csystem = channeledsystem(fin, fout, 'S')
> -        _wrapchgui(ui, self._csystem)
>          super(chgcmdserver, self).__init__(ui, repo, fin, fout)
>          self.clientsock = sock
> @@ -507,4 +506,5 @@ class chgcmdserver(commandserver.server)
>  
>      def runcommand(self):
> +        _wrapchgui(self.ui, self._csystem)
>          return super(chgcmdserver, self).runcommand()

This change doesn't make sense to me. The first patch implies we need a global
ui class having the knowledge about chg. This patch delays the introduction of
the wrapped class, but still it lives in global space. (And you know,
theoretically "runcommand" can be called more than once.)

If we have to narrow the scope of csystem-ed ui without recreating it, we'll
need a flag (or ui._csystem = None|object) to switch the behavior.
Jun Wu - Dec. 18, 2016, 6:24 p.m.
Excerpts from Yuya Nishihara's message of 2016-12-18 17:30:31 +0900:
> On Fri, 16 Dec 2016 15:01:21 +0000, Jun Wu wrote:
> > # HG changeset patch
> > # User Jun Wu <quark@fb.com>
> > # Date 1481900282 0
> > #      Fri Dec 16 14:58:02 2016 +0000
> > # Node ID 8fe60192f17f6ae99fa66c6bce1ec306772e31df
> > # Parent  eb3017f14d56dfdc9870b06a684ef9bcf7a030e6
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 8fe60192f17f
> > chgserver: move wrapchgui to runcommand
> > 
> > The wrapping logic changes ui.system, which should only affect runcommand.
> > This makes future refactoring a bit cleaner.
> > 
> > diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
> > --- a/mercurial/chgserver.py
> > +++ b/mercurial/chgserver.py
> > @@ -330,5 +330,4 @@ class chgcmdserver(commandserver.server)
> >      def __init__(self, ui, repo, fin, fout, sock, hashstate, baseaddress):
> >          self._csystem = channeledsystem(fin, fout, 'S')
> > -        _wrapchgui(ui, self._csystem)
> >          super(chgcmdserver, self).__init__(ui, repo, fin, fout)
> >          self.clientsock = sock
> > @@ -507,4 +506,5 @@ class chgcmdserver(commandserver.server)
> >  
> >      def runcommand(self):
> > +        _wrapchgui(self.ui, self._csystem)
> >          return super(chgcmdserver, self).runcommand()
> 
> This change doesn't make sense to me. The first patch implies we need a global
> ui class having the knowledge about chg. This patch delays the introduction of
> the wrapped class, but still it lives in global space. (And you know,
> theoretically "runcommand" can be called more than once.)
> 
> If we have to narrow the scope of csystem-ed ui without recreating it, we'll
> need a flag (or ui._csystem = None|object) to switch the behavior.

The direction is to eventually lose control on "ui" used in runcommand, and
let dispatch construct a "ui" object on its own. And we then use a special
"uisetup" which calls "wrapui" to modify the ui object created by dispatch.

Maybe this patch should be moved after adding "uisetup" in dispatch.request.
Yuya Nishihara - Dec. 19, 2016, 2:46 p.m.
On Sun, 18 Dec 2016 18:24:45 +0000, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2016-12-18 17:30:31 +0900:
> > On Fri, 16 Dec 2016 15:01:21 +0000, Jun Wu wrote:
> > > # HG changeset patch
> > > # User Jun Wu <quark@fb.com>
> > > # Date 1481900282 0
> > > #      Fri Dec 16 14:58:02 2016 +0000
> > > # Node ID 8fe60192f17f6ae99fa66c6bce1ec306772e31df
> > > # Parent  eb3017f14d56dfdc9870b06a684ef9bcf7a030e6
> > > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > > #              hg pull https://bitbucket.org/quark-zju/hg-draft  -r 8fe60192f17f
> > > chgserver: move wrapchgui to runcommand
> > > 
> > > The wrapping logic changes ui.system, which should only affect runcommand.
> > > This makes future refactoring a bit cleaner.
> > > 
> > > diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
> > > --- a/mercurial/chgserver.py
> > > +++ b/mercurial/chgserver.py
> > > @@ -330,5 +330,4 @@ class chgcmdserver(commandserver.server)
> > >      def __init__(self, ui, repo, fin, fout, sock, hashstate, baseaddress):
> > >          self._csystem = channeledsystem(fin, fout, 'S')
> > > -        _wrapchgui(ui, self._csystem)
> > >          super(chgcmdserver, self).__init__(ui, repo, fin, fout)
> > >          self.clientsock = sock
> > > @@ -507,4 +506,5 @@ class chgcmdserver(commandserver.server)
> > >  
> > >      def runcommand(self):
> > > +        _wrapchgui(self.ui, self._csystem)
> > >          return super(chgcmdserver, self).runcommand()
> > 
> > This change doesn't make sense to me. The first patch implies we need a global
> > ui class having the knowledge about chg. This patch delays the introduction of
> > the wrapped class, but still it lives in global space. (And you know,
> > theoretically "runcommand" can be called more than once.)
> > 
> > If we have to narrow the scope of csystem-ed ui without recreating it, we'll
> > need a flag (or ui._csystem = None|object) to switch the behavior.
> 
> The direction is to eventually lose control on "ui" used in runcommand, and
> let dispatch construct a "ui" object on its own.

I got it.

> And we then use a special
> "uisetup" which calls "wrapui" to modify the ui object created by dispatch.

Who will call this "uisetup"? dispatch or chgserver.runcommand?

I'm slightly afraid of modifying ui class in the middle of the server session
since the ui might be used after runcommand(). That could lead  to a subtle bug.

> Maybe this patch should be moved after adding "uisetup" in dispatch.request.

If it is a temporary code, I don't care much about the cleanliness.
Jun Wu - Dec. 19, 2016, 4:32 p.m.
Excerpts from Yuya Nishihara's message of 2016-12-19 23:46:26 +0900:
> On Sun, 18 Dec 2016 18:24:45 +0000, Jun Wu wrote:
> > The direction is to eventually lose control on "ui" used in runcommand, and
> > let dispatch construct a "ui" object on its own.
> 
> I got it.
> 
> > And we then use a special
> > "uisetup" which calls "wrapui" to modify the ui object created by dispatch.
> 
> Who will call this "uisetup"? dispatch or chgserver.runcommand?

The plan is to add a "uisetup" argument to dispatch.request:

  diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
  --- a/mercurial/dispatch.py
  +++ b/mercurial/dispatch.py
  @@ -49,5 +49,5 @@ from . import (
   class request(object):
       def __init__(self, args, ui=None, repo=None, fin=None, fout=None,
  -                 ferr=None):
  +                 ferr=None, uisetup=None):
           self.args = args
           self.ui = ui
  @@ -59,4 +59,7 @@ class request(object):
           self.ferr = ferr
   
  +        # an extra uisetup unrelated to extensions, used by chg
  +        self.uisetup = uisetup
  +
   def run():
       "run the command in sys.argv"
  @@ -660,4 +663,9 @@ def _dispatch(req):
       extensions.loadall(lui)
       exts = [ext for ext in extensions.extensions() if ext[0] not in _loaded]
  +
  +    # An extra uisetup of the request, currently used by chg
  +    if req.uisetup is not None:
  +        req.uisetup(lui)
  +
       # Propagate any changes to lui.__class__ by extensions
       ui.__class__ = lui.__class__

> I'm slightly afraid of modifying ui class in the middle of the server session
> since the ui might be used after runcommand(). That could lead  to a subtle bug.

I agree. So patch 4 should be deferred until the uisetup work is done. Since
that depends on a lot of other things. Patch 4 should be dropped now.

> > Maybe this patch should be moved after adding "uisetup" in dispatch.request.
> 
> If it is a temporary code, I don't care much about the cleanliness.
Yuya Nishihara - Dec. 20, 2016, 12:47 p.m.
On Mon, 19 Dec 2016 16:32:16 +0000, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2016-12-19 23:46:26 +0900:
> > On Sun, 18 Dec 2016 18:24:45 +0000, Jun Wu wrote:
> > > The direction is to eventually lose control on "ui" used in runcommand, and
> > > let dispatch construct a "ui" object on its own.
> > 
> > I got it.
> > 
> > > And we then use a special
> > > "uisetup" which calls "wrapui" to modify the ui object created by dispatch.
> > 
> > Who will call this "uisetup"? dispatch or chgserver.runcommand?
> 
> The plan is to add a "uisetup" argument to dispatch.request:
> 
>   diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
>   --- a/mercurial/dispatch.py
>   +++ b/mercurial/dispatch.py
>   @@ -49,5 +49,5 @@ from . import (
>    class request(object):
>        def __init__(self, args, ui=None, repo=None, fin=None, fout=None,
>   -                 ferr=None):
>   +                 ferr=None, uisetup=None):
>            self.args = args
>            self.ui = ui
>   @@ -59,4 +59,7 @@ class request(object):
>            self.ferr = ferr
>    
>   +        # an extra uisetup unrelated to extensions, used by chg
>   +        self.uisetup = uisetup
>   +
>    def run():
>        "run the command in sys.argv"
>   @@ -660,4 +663,9 @@ def _dispatch(req):
>        extensions.loadall(lui)
>        exts = [ext for ext in extensions.extensions() if ext[0] not in _loaded]
>   +
>   +    # An extra uisetup of the request, currently used by chg
>   +    if req.uisetup is not None:
>   +        req.uisetup(lui)
>   +
>        # Propagate any changes to lui.__class__ by extensions
>        ui.__class__ = lui.__class__

Ah, that seems okay.

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())

> > I'm slightly afraid of modifying ui class in the middle of the server session
> > since the ui might be used after runcommand(). That could lead  to a subtle bug.
> 
> I agree. So patch 4 should be deferred until the uisetup work is done. Since
> that depends on a lot of other things. Patch 4 should be dropped now.
Jun Wu - Dec. 20, 2016, 2:03 p.m.
Excerpts from Yuya Nishihara's message of 2016-12-20 21:47:23 +0900:
> On Mon, 19 Dec 2016 16:32:16 +0000, Jun Wu wrote:
> > Excerpts from Yuya Nishihara's message of 2016-12-19 23:46:26 +0900:
> > > On Sun, 18 Dec 2016 18:24:45 +0000, Jun Wu wrote:
> > > > The direction is to eventually lose control on "ui" used in runcommand, and
> > > > let dispatch construct a "ui" object on its own.
> > > 
> > > I got it.
> > > 
> > > > And we then use a special
> > > > "uisetup" which calls "wrapui" to modify the ui object created by dispatch.
> > > 
> > > Who will call this "uisetup"? dispatch or chgserver.runcommand?
> > 
> > The plan is to add a "uisetup" argument to dispatch.request:
> > 
> >   diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> >   --- a/mercurial/dispatch.py
> >   +++ b/mercurial/dispatch.py
> >   @@ -49,5 +49,5 @@ from . import (
> >    class request(object):
> >        def __init__(self, args, ui=None, repo=None, fin=None, fout=None,
> >   -                 ferr=None):
> >   +                 ferr=None, uisetup=None):
> >            self.args = args
> >            self.ui = ui
> >   @@ -59,4 +59,7 @@ class request(object):
> >            self.ferr = ferr
> >    
> >   +        # an extra uisetup unrelated to extensions, used by chg
> >   +        self.uisetup = uisetup
> >   +
> >    def run():
> >        "run the command in sys.argv"
> >   @@ -660,4 +663,9 @@ def _dispatch(req):
> >        extensions.loadall(lui)
> >        exts = [ext for ext in extensions.extensions() if ext[0] not in _loaded]
> >   +
> >   +    # An extra uisetup of the request, currently used by chg
> >   +    if req.uisetup is not None:
> >   +        req.uisetup(lui)
> >   +
> >        # Propagate any changes to lui.__class__ by extensions
> >        ui.__class__ = lui.__class__
> 
> Ah, that seems okay.
> 
> 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.

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.

> > > I'm slightly afraid of modifying ui class in the middle of the server session
> > > since the ui might be used after runcommand(). That could lead  to a subtle bug.
> > 
> > I agree. So patch 4 should be deferred until the uisetup work is done. Since
> > that depends on a lot of other things. Patch 4 should be dropped now.
Yuya Nishihara - Dec. 20, 2016, 2:29 p.m.
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.

> 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.
Jun Wu - Dec. 20, 2016, 4:46 p.m.
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.
Yuya Nishihara - Dec. 21, 2016, 1:20 p.m.
On Tue, 20 Dec 2016 16:46:38 +0000, Jun Wu wrote:
> 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.

Sounds reasonable to me. I had something similar in mind, which would only
implement the low-level API:

 1. add ui._runpager() -> _runpager()
 2. s/_runpager()/ui._runpager()/
 3. override ui._runpager() by chgui
Yuya Nishihara - Dec. 21, 2016, 2:15 p.m.
On Tue, 20 Dec 2016 14:03:04 +0000, Jun Wu wrote:
> 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.

I've queued 1 and 3, thanks.
Jun Wu - Dec. 21, 2016, 3:39 p.m.
Excerpts from Yuya Nishihara's message of 2016-12-21 23:15:28 +0900:
> On Tue, 20 Dec 2016 14:03:04 +0000, Jun Wu wrote:
> > 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.
> 
> I've queued 1 and 3, thanks.

Actually, patch 1 is unnecessary if we go with the "ui._runpager" approach.
Maybe someone can drop it without adding too many markers.
Yuya Nishihara - Dec. 21, 2016, 3:48 p.m.
On Wed, 21 Dec 2016 15:39:05 +0000, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2016-12-21 23:15:28 +0900:
> > On Tue, 20 Dec 2016 14:03:04 +0000, Jun Wu wrote:
> > > 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.
> > 
> > I've queued 1 and 3, thanks.
> 
> Actually, patch 1 is unnecessary if we go with the "ui._runpager" approach.
> Maybe someone can drop it without adding too many markers.

That's probably me. I'll drop the patch: dfb19aed409e "chgserver: store csystem
separately."
Augie Fackler - Dec. 21, 2016, 5:39 p.m.
> On Dec 20, 2016, at 11:46, Jun Wu <quark@fb.com> wrote:
> 
> 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.

This matches my understanding of where we probably have to go for the pager work.

I'm not sure if we should plan to make the pager exit-able, but that depends on the architecture we use for paging, which I haven't given much thought lately.

Patch

diff --git a/mercurial/chgserver.py b/mercurial/chgserver.py
--- a/mercurial/chgserver.py
+++ b/mercurial/chgserver.py
@@ -330,5 +330,4 @@  class chgcmdserver(commandserver.server)
     def __init__(self, ui, repo, fin, fout, sock, hashstate, baseaddress):
         self._csystem = channeledsystem(fin, fout, 'S')
-        _wrapchgui(ui, self._csystem)
         super(chgcmdserver, self).__init__(ui, repo, fin, fout)
         self.clientsock = sock
@@ -507,4 +506,5 @@  class chgcmdserver(commandserver.server)
 
     def runcommand(self):
+        _wrapchgui(self.ui, self._csystem)
         return super(chgcmdserver, self).runcommand()