Patchwork [1,of,9,pager,v2] pager: move pager-initiating code into core

login
register
mail settings
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

Augie Fackler - Feb. 16, 2017, 4:59 p.m.
# 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.
Matt Harbison - Feb. 26, 2017, 4:22 a.m.
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
Augie Fackler - Feb. 26, 2017, 4:32 a.m.
> 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
Matt Harbison - Feb. 26, 2017, 5:25 a.m.
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
Simon King - Feb. 27, 2017, 10:55 a.m.
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?