| Submitter | Augie Fackler |
|---|---|
| Date | Feb. 16, 2017, 4:59 p.m. |
| Message ID | <cd19a77477bcd18d1f7a.1487264350@arthedain.pit.corp.google.com> |
| Download | mbox | patch |
| Permalink | /patch/18548/ |
| State | Accepted |
| Headers | show |
Comments
On Thu, 16 Feb 2017 11:59:10 -0500, Augie Fackler <raf@durin42.com> wrote: > # HG changeset patch > # User Augie Fackler <augie@google.com> > # Date 1487198871 18000 > # Wed Feb 15 17:47:51 2017 -0500 > # Node ID cd19a77477bcd18d1f7ab4fa73ee0dbf3c7a4e46 > # Parent 791b4e846a7b9a0783440b9504585438777fe2d2 > pager: move pager-initiating code into core > > No functionality change. > > A previous version of this API had a category argument on > ui.pager(). As I migrated the commands in core, I couldn't come up > with good enough consistency in any categorization scheme so I just > scrapped the whole idea. It may be worth revisiting in the future. > > diff --git a/hgext/pager.py b/hgext/pager.py > --- a/hgext/pager.py > +++ b/hgext/pager.py > + # TODO: add a "system defaults" config section so this default > + # of more(1) can be easily replaced with a global > + # configuration file. For example, on OS X the sane default is > + # less(1), not more(1), and on debian it's > + # sensible-pager(1). We should probably also give the system > + # default editor command similar treatment. > + envpager = encoding.environ.get('PAGER', 'more') > + pagercmd = self.config('pager', 'pager', envpager) Any thoughts on what this looks like? It seems like it might be distantly(?) related to something I tried a couple years ago [1], and the discussions around it. Does it seem reasonable to move the above to a method, similar to ui.editor()? I'd like to detect MSYS and default to `less`. IDK if we can pull that off with just a global config file. (The only way I see to detect MSYS is to look for 'MSYSTEM' in env.) In theory, the `less` command in GnuWin32 can be installed and made visible to cmd.exe too. I wonder if (on some platforms anyway), there should be a list of default choices, and the first one found in $PATH used. ui.editor() doesn't make sure the editor is valid, so I figured keeping that kind of check out of ui was purposeful. A bad editor will produce an error that tries to indicate the problem (though I missed it the first time in the OS error output): $ HGEDITOR=xzy ../hg ci 'xzy' is not recognized as an internal or external command, operable program or batch file. abort: edit failed: xzy exited with status 1 But not so much for the pager: hg>set PAGER=less hg>hg help filesets 'less' is not recognized as an internal or external command, operable program or batch file. I'm not sure if the right thing here is to error out, or to treat the pager as disabled. Especially if it is on by default, with an env knob that isn't as obvious as the example above. So it seems like we might need some sort of check, whether or not we try a list of defaults. [1] https://www.mercurial-scm.org/pipermail/mercurial-devel/2015-March/067563.html
> On Feb 25, 2017, at 11:22 PM, Matt Harbison <mharbison72@gmail.com> wrote: > > On Thu, 16 Feb 2017 11:59:10 -0500, Augie Fackler <raf@durin42.com> wrote: > >> # HG changeset patch >> # User Augie Fackler <augie@google.com> >> # Date 1487198871 18000 >> # Wed Feb 15 17:47:51 2017 -0500 >> # Node ID cd19a77477bcd18d1f7ab4fa73ee0dbf3c7a4e46 >> # Parent 791b4e846a7b9a0783440b9504585438777fe2d2 >> pager: move pager-initiating code into core >> >> No functionality change. >> >> A previous version of this API had a category argument on >> ui.pager(). As I migrated the commands in core, I couldn't come up >> with good enough consistency in any categorization scheme so I just >> scrapped the whole idea. It may be worth revisiting in the future. >> >> diff --git a/hgext/pager.py b/hgext/pager.py >> --- a/hgext/pager.py >> +++ b/hgext/pager.py >> + # TODO: add a "system defaults" config section so this default >> + # of more(1) can be easily replaced with a global >> + # configuration file. For example, on OS X the sane default is >> + # less(1), not more(1), and on debian it's >> + # sensible-pager(1). We should probably also give the system >> + # default editor command similar treatment. >> + envpager = encoding.environ.get('PAGER', 'more') >> + pagercmd = self.config('pager', 'pager', envpager) > > Any thoughts on what this looks like? It seems like it might be distantly(?) related to something I tried a couple years ago [1], and the discussions around it. It’s pretty different from the previous round in my mind: it’d be a config section, only advertised for sysadmins and packagers, specifically intended for setting more-reasonable defaults, e.g.: (debian) [systemdefaults] editor = sensible-editor pager = sensible-pager (macOS) [systemdefaults] editor = nano pager = less (FreeBSD) [systemdefaults] editor = vi pager = less etc. > Does it seem reasonable to move the above to a method, similar to ui.editor()? I'd like to detect MSYS and default to `less`. IDK if we can pull that off with just a global config file. (The only way I see to detect MSYS is to look for 'MSYSTEM' in env.) In line with the above, I’d expect an mays-backed package to set systemdefaults.pager to less (maybe that’s not a thing? I dunno.) I’m extremely hesitant to add any kind of conditional logic in hgrc. Do you know of any other systems that use this kind of configuration approach but don’t have a scripting language as their config language? Note that systemdefaults (which is a name I came up with in the process of writing this mail, and probably isn’t what we should keep) would *not* overwrite environment variables, so if the user has EDITOR or PAGER set, that takes precedence over systemdefaults - this new config section is explicitly about helping packagers get the best possible “out of the box” experience without having to patch hg (which the debian package currently has to for sensible-editor). > In theory, the `less` command in GnuWin32 can be installed and made visible to cmd.exe too. I wonder if (on some platforms anyway), there should be a list of default choices, and the first one found in $PATH used. ui.editor() doesn't make sure the editor is valid, so I figured keeping that kind of check out of ui was purposeful. A bad editor will produce an error that tries to indicate the problem (though I missed it the first time in the OS error output): > > $ HGEDITOR=xzy ../hg ci > 'xzy' is not recognized as an internal or external command, > operable program or batch file. > abort: edit failed: xzy exited with status 1 > > But not so much for the pager: > > hg>set PAGER=less > hg>hg help filesets > 'less' is not recognized as an internal or external command, > operable program or batch file. Could you please file a bug for this? It reproduces on OS X too, with equally mystifying results (I’m on a slow connection right now, so email is tolerable but https is not). > I'm not sure if the right thing here is to error out, or to treat the pager as disabled. Especially if it is on by default, with an env knob that isn't as obvious as the example above. So it seems like we might need some sort of check, whether or not we try a list of defaults. git disables the pager, with a warning that it couldn’t exec the pager. We probably should do something similar, I can’t think of anything else reasonable to do. > [1] https://www.mercurial-scm.org/pipermail/mercurial-devel/2015-March/067563.html
On Sat, 25 Feb 2017 23:32:12 -0500, Augie Fackler <raf@durin42.com> wrote: > >> On Feb 25, 2017, at 11:22 PM, Matt Harbison <mharbison72@gmail.com> >> wrote: >> >> On Thu, 16 Feb 2017 11:59:10 -0500, Augie Fackler <raf@durin42.com> >> wrote: >> >>> # HG changeset patch >>> # User Augie Fackler <augie@google.com> >>> # Date 1487198871 18000 >>> # Wed Feb 15 17:47:51 2017 -0500 >>> # Node ID cd19a77477bcd18d1f7ab4fa73ee0dbf3c7a4e46 >>> # Parent 791b4e846a7b9a0783440b9504585438777fe2d2 >>> pager: move pager-initiating code into core >>> >>> No functionality change. >>> >>> A previous version of this API had a category argument on >>> ui.pager(). As I migrated the commands in core, I couldn't come up >>> with good enough consistency in any categorization scheme so I just >>> scrapped the whole idea. It may be worth revisiting in the future. >>> >>> diff --git a/hgext/pager.py b/hgext/pager.py >>> --- a/hgext/pager.py >>> +++ b/hgext/pager.py >>> + # TODO: add a "system defaults" config section so this default >>> + # of more(1) can be easily replaced with a global >>> + # configuration file. For example, on OS X the sane default is >>> + # less(1), not more(1), and on debian it's >>> + # sensible-pager(1). We should probably also give the system >>> + # default editor command similar treatment. >>> + envpager = encoding.environ.get('PAGER', 'more') >>> + pagercmd = self.config('pager', 'pager', envpager) >> >> Any thoughts on what this looks like? It seems like it might be >> distantly(?) related to something I tried a couple years ago [1], and >> the discussions around it. > > It’s pretty different from the previous round in my mind: it’d be a > config section, only advertised for sysadmins and packagers, > specifically intended for setting more-reasonable defaults, e.g.: > > (debian) > [systemdefaults] > editor = sensible-editor > pager = sensible-pager > > (macOS) > [systemdefaults] > editor = nano > pager = less > > (FreeBSD) > [systemdefaults] > editor = vi > pager = less > > etc. > >> Does it seem reasonable to move the above to a method, similar to >> ui.editor()? I'd like to detect MSYS and default to `less`. IDK if we >> can pull that off with just a global config file. (The only way I see >> to detect MSYS is to look for 'MSYSTEM' in env.) > > In line with the above, I’d expect an mays-backed package to set > systemdefaults.pager to less (maybe that’s not a thing? I dunno.) I'm not thinking of an msys package, rather my personal usage. If I'm doing anything of any substance on the command line, I'll use msys. But TortoiseHg launches cmd.exe with a simple Ctrl + Shift + T, and sometimes that's good enough. Therefore, I can't just set a pager value in %HOME%/mercurial.ini. The hg.exe built from source and the one bundled with TortoiseHg are each visible to msys and cmd.exe, depending on what directory I'm in. So even if TortoiseHg picked one (and we ignore hg built from the dev repo), it may or may not use the best pager available without a little help. > I’m extremely hesitant to add any kind of conditional logic in hgrc. Agreed. The conditional logic I'm thinking of is in python, similar to how plan9 sets its last-ditch editor to 'E' in ui.geteditor(). Based on the vision you've described, I don't see a list as useful. And because I can't think of any other OS with such a bad pager (and a better one so tantalizingly close), maybe a one-off python conditional is OK? > Do you know of any other systems that use this kind of configuration > approach but don’t have a scripting language as their config language? IDK what this means, so I'm gonna say no. :-) > Note that systemdefaults (which is a name I came up with in the process > of writing this mail, and probably isn’t what we should keep) would > *not* overwrite environment variables, so if the user has EDITOR or > PAGER set, that takes precedence over systemdefaults - this new config > section is explicitly about helping packagers get the best possible “out > of the box” experience without having to patch hg (which the debian > package currently has to for sensible-editor). Makes sense. This would get rid of the special case editor for plan9, for packages anyway. It wouldn't for hg built from source, but that could presumably be set in ~/.hgrc. >> In theory, the `less` command in GnuWin32 can be installed and made >> visible to cmd.exe too. I wonder if (on some platforms anyway), there >> should be a list of default choices, and the first one found in $PATH >> used. ui.editor() doesn't make sure the editor is valid, so I figured >> keeping that kind of check out of ui was purposeful. A bad editor will >> produce an error that tries to indicate the problem (though I missed it >> the first time in the OS error output): >> >> $ HGEDITOR=xzy ../hg ci >> 'xzy' is not recognized as an internal or external command, >> operable program or batch file. >> abort: edit failed: xzy exited with status 1 >> >> But not so much for the pager: >> >> hg>set PAGER=less >> hg>hg help filesets >> 'less' is not recognized as an internal or external command, >> operable program or batch file. > > Could you please file a bug for this? It reproduces on OS X too, with > equally mystifying results (I’m on a slow connection right now, so email > is tolerable but https is not). Done. >> I'm not sure if the right thing here is to error out, or to treat the >> pager as disabled. Especially if it is on by default, with an env knob >> that isn't as obvious as the example above. So it seems like we might >> need some sort of check, whether or not we try a list of defaults. > > git disables the pager, with a warning that it couldn’t exec the pager. > We probably should do something similar, I can’t think of anything else > reasonable to do. Seems reasonable. Not sure if the warning should come before or after the wall of text, but it would be an improvement either way. >> [1] >> https://www.mercurial-scm.org/pipermail/mercurial-devel/2015-March/067563.html
On Sun, Feb 26, 2017 at 5:25 AM, Matt Harbison <mharbison72@gmail.com> wrote: > On Sat, 25 Feb 2017 23:32:12 -0500, Augie Fackler <raf@durin42.com> wrote: [snip] >>> Does it seem reasonable to move the above to a method, similar to >>> ui.editor()? I'd like to detect MSYS and default to `less`. IDK if we can >>> pull that off with just a global config file. (The only way I see to detect >>> MSYS is to look for 'MSYSTEM' in env.) >> >> >> In line with the above, I’d expect an mays-backed package to set >> systemdefaults.pager to less (maybe that’s not a thing? I dunno.) > > > I'm not thinking of an msys package, rather my personal usage. If I'm doing > anything of any substance on the command line, I'll use msys. But > TortoiseHg launches cmd.exe with a simple Ctrl + Shift + T, and sometimes > that's good enough. Therefore, I can't just set a pager value in > %HOME%/mercurial.ini. The hg.exe built from source and the one bundled with > TortoiseHg are each visible to msys and cmd.exe, depending on what directory > I'm in. So even if TortoiseHg picked one (and we ignore hg built from the > dev repo), it may or may not use the best pager available without a little > help. > >> I’m extremely hesitant to add any kind of conditional logic in hgrc. > You could get what you want by using "%include" with an environment variable in your %HOME%/mercurial.ini, something along the lines of: %include mercurial.%MSYSTEM%.ini It's not particularly user-friendly, but I think it will work. Simon
Patch
diff --git a/hgext/pager.py b/hgext/pager.py --- a/hgext/pager.py +++ b/hgext/pager.py @@ -60,19 +60,11 @@ you can use --pager=<value>:: ''' from __future__ import absolute_import -import atexit -import os -import signal -import subprocess -import sys - from mercurial.i18n import _ from mercurial import ( cmdutil, commands, dispatch, - encoding, - error, extensions, util, ) @@ -83,48 +75,14 @@ from mercurial import ( # leave the attribute unspecified. testedwith = 'ships-with-hg-core' -def _runpager(ui, p): - pager = subprocess.Popen(p, shell=True, bufsize=-1, - close_fds=util.closefds, stdin=subprocess.PIPE, - stdout=util.stdout, stderr=util.stderr) - - # back up original file descriptors - stdoutfd = os.dup(util.stdout.fileno()) - stderrfd = os.dup(util.stderr.fileno()) - - os.dup2(pager.stdin.fileno(), util.stdout.fileno()) - if ui._isatty(util.stderr): - os.dup2(pager.stdin.fileno(), util.stderr.fileno()) - - @atexit.register - def killpager(): - if util.safehasattr(signal, "SIGINT"): - signal.signal(signal.SIGINT, signal.SIG_IGN) - # restore original fds, closing pager.stdin copies in the process - os.dup2(stdoutfd, util.stdout.fileno()) - os.dup2(stderrfd, util.stderr.fileno()) - pager.stdin.close() - pager.wait() - -def catchterm(*args): - raise error.SignalInterrupt - def uisetup(ui): - class pagerui(ui.__class__): - def _runpager(self, pagercmd): - _runpager(self, pagercmd) - - ui.__class__ = pagerui def pagecmd(orig, ui, options, cmd, cmdfunc): - p = ui.config("pager", "pager", encoding.environ.get("PAGER")) usepager = False always = util.parsebool(options['pager']) auto = options['pager'] == 'auto' - if not p or '--debugger' in sys.argv or not ui.formatted(): - pass - elif always: + if always: usepager = True elif not auto: usepager = False @@ -143,14 +101,8 @@ def uisetup(ui): usepager = True break - setattr(ui, 'pageractive', usepager) - if usepager: - ui.setconfig('ui', 'formatted', ui.formatted(), 'pager') - ui.setconfig('ui', 'interactive', False, 'pager') - if util.safehasattr(signal, "SIGPIPE"): - signal.signal(signal.SIGPIPE, catchterm) - ui._runpager(p) + ui.pager('extension-via-attend-' + cmd) return orig(ui, options, cmd, cmdfunc) # Wrap dispatch._runcommand after color is loaded so color can see diff --git a/mercurial/ui.py b/mercurial/ui.py --- a/mercurial/ui.py +++ b/mercurial/ui.py @@ -7,6 +7,7 @@ from __future__ import absolute_import +import atexit import collections import contextlib import errno @@ -14,7 +15,9 @@ import getpass import inspect import os import re +import signal import socket +import subprocess import sys import tempfile import traceback @@ -120,6 +123,8 @@ class httppasswordmgrdbproxy(object): def find_user_password(self, *args, **kwargs): return self._get_mgr().find_user_password(*args, **kwargs) +def _catchterm(*args): + raise error.SignalInterrupt class ui(object): def __init__(self, src=None): @@ -154,6 +159,7 @@ class ui(object): self.fout = src.fout self.ferr = src.ferr self.fin = src.fin + self.pageractive = src.pageractive self._tcfg = src._tcfg.copy() self._ucfg = src._ucfg.copy() @@ -171,6 +177,7 @@ class ui(object): self.fout = util.stdout self.ferr = util.stderr self.fin = util.stdin + self.pageractive = False # shared read-only environment self.environ = encoding.environ @@ -829,6 +836,77 @@ class ui(object): return False return util.isatty(fh) + def pager(self, command): + """Start a pager for subsequent command output. + + Commands which produce a long stream of output should call + this function to activate the user's preferred pagination + mechanism (which may be no pager). Calling this function + precludes any future use of interactive functionality, such as + prompting the user or activating curses. + + Args: + command: The full, non-aliased name of the command. That is, "log" + not "history, "summary" not "summ", etc. + """ + if (self.pageractive + # TODO: if we want to allow HGPLAINEXCEPT=pager, + # formatted() will need some adjustment. + or not self.formatted() + or self.plain() + # TODO: expose debugger-enabled on the UI object + or '--debugger' in sys.argv): + # We only want to paginate if the ui appears to be + # interactive, the user didn't say HGPLAIN or + # HGPLAINEXCEPT=pager, and the user didn't specify --debug. + return + + # TODO: add a "system defaults" config section so this default + # of more(1) can be easily replaced with a global + # configuration file. For example, on OS X the sane default is + # less(1), not more(1), and on debian it's + # sensible-pager(1). We should probably also give the system + # default editor command similar treatment. + envpager = encoding.environ.get('PAGER', 'more') + pagercmd = self.config('pager', 'pager', envpager) + self.pageractive = True + # Preserve the formatted-ness of the UI. This is important + # because we mess with stdout, which might confuse + # auto-detection of things being formatted. + self.setconfig('ui', 'formatted', self.formatted(), 'pager') + self.setconfig('ui', 'interactive', False, 'pager') + if util.safehasattr(signal, "SIGPIPE"): + signal.signal(signal.SIGPIPE, _catchterm) + self._runpager(pagercmd) + + def _runpager(self, command): + """Actually start the pager and set up file descriptors. + + This is separate in part so that extensions (like chg) can + override how a pager is invoked. + """ + pager = subprocess.Popen(command, shell=True, bufsize=-1, + close_fds=util.closefds, stdin=subprocess.PIPE, + stdout=util.stdout, stderr=util.stderr) + + # back up original file descriptors + stdoutfd = os.dup(util.stdout.fileno()) + stderrfd = os.dup(util.stderr.fileno()) + + os.dup2(pager.stdin.fileno(), util.stdout.fileno()) + if self._isatty(util.stderr): + os.dup2(pager.stdin.fileno(), util.stderr.fileno()) + + @atexit.register + def killpager(): + if util.safehasattr(signal, "SIGINT"): + signal.signal(signal.SIGINT, signal.SIG_IGN) + # restore original fds, closing pager.stdin copies in the process + os.dup2(stdoutfd, util.stdout.fileno()) + os.dup2(stderrfd, util.stderr.fileno()) + pager.stdin.close() + pager.wait() + def interface(self, feature): """what interface to use for interactive console features?