On Tue, Dec 27, 2016 at 09:07:58PM +0900, Yuya Nishihara wrote: > On Mon, 26 Dec 2016 14:10:05 +0000, Jun Wu wrote: > > The goal is to move "pager" logic inside "runcommand" (both client and > > server side) and remove the "getpager" API. And I'd like to discuss some > > implementation details before sending patches. > > > > Problems: > > > > 1. Which channel to send "pagercmd" to the client? > > 2. After sending "pagercmd" to client by chgui._runpager, how to deal with > > "attachio"? > > 3. How to arrange the signal handling / pager code client side? > > > > Proposed options: > > > > For 1, > > > > a) Reuse S channel. > > The format to pass cwd, environ is reusable. > > The format to read exitcode is not reusable. > > An extra "command type" parameter is needed, which may be a minor BC. > > (preferred if we don't care about such minor BC) > > > > b) Use a separate channel. > > Will not BC old clients on the S channel format. > > +1 for (a). I don't want to introduce new channel per s2c request. > > Regarding BC (IPC incompatibility), I'm thinking of adding a capability header > to make sure the chg client is the version supported by the server, something > like 'capabilities: chg-S-channel-2'.
A thought: it's (notionally) optional for clients of the cmdserver to ignore the pager operation, right? That is, it's totally valid to drop the "start a pager now" message on the floor and act like no pager is in play. Maybe it's worth having some mechanism for that. > > > For 2, > > > > a) Pass "attachio" or "chgcmdserver" to "_newchgui" so chgui can use it. > > This is the most direct way. > > (preferred) > > > > b) Move "attachio" related logic to chgui, and delegate chgcmdserver's > > channels to ui. This seems unnecessarily complex and it does not seem > > better than a. > > (a) seems okay as a start, but I don't think (b) is that complex. Since we're > going to override the "runcommand" function, we will no longer need to replace > server's cin, cout, and cerr channels. Which would mean the core logic of > attachio() can be moved to ui (or a free function that takes ui.) > > > For 3, > > > > "pagerpid" is used in sighandler. Assuming we don't want new global > > variables in all cases, I think the options are roughly: > > > > | pager | sighandler | new API | annotate history > > -------------------------------------------------------------------- > > now | chg.c | chg.c | - | no > > a) | chg.c | chg.c | hgc_setrunpager | yes > > | | | callback | > > b) | hgclient.c | chg.c | hgc_getpagerpid | no > > c) | util.c | util.c | - | no > > d) | pager.c | sigutil.c | get/setpagerpid | yes > > e) | procutil.c | procutil.c | - | yes > > > > I slightly prefer e) if we can find a good file name to contain both > > pager and sighandler logic. > > (e) sounds nice since pager/sighandler stuffs are getting bigger. > > Thanks for the good summary about the pager refactoring. +1. If you want to talk more about pager stuff let me know - I'm not at work again until the 9th of January, but may be able to find time here or there to chat before then if you need. > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@mercurial-scm.org > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
