Patchwork [v3,rebased] pager: migrate heavily-used extension into core

# HG changeset patch
# User Bryan O'Sullivan <bryano@fb.com>
# Date 1486160890 28800
#      Fri Feb 03 14:28:10 2017 -0800
# Node ID ae22925dafd4a270cb80a7bb54c9d70bce49a633
# Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
pager: migrate heavily-used extension into core

No default behaviours were harmed during the making of this change.

Notes:

* This patch will break out-of-tree extensions that rely on the
  location of the old pager module's attend variable.  It is now a
  static variable named pagercommands on the ui class.

* It used to be possible to disable the pager via config by disabling
  the loading of the extension.  With the extension gone, that
  method no longer works.  Instead, set pager.attend to a command
  that does not exist, e.g. "--config pager.attend=nothing".

> On Feb 6, 2017, at 14:47, Bryan O'Sullivan <bos@serpentine.com> wrote:
> 
> # HG changeset patch
> # User Bryan O'Sullivan <bryano@fb.com>
> # Date 1486160890 28800
> #      Fri Feb 03 14:28:10 2017 -0800
> # Node ID ae22925dafd4a270cb80a7bb54c9d70bce49a633
> # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
> pager: migrate heavily-used extension into core

Here's my counterproposal, not quite done enough to mail (mostly on account of it being late in my timezone):

https://hg.durin42.com/hg-wip/log?rev=%40%3A%3A7c0170f0420f%20-%20%40&revcount=50

It's 32 patches (but most of them are one line, turning on paging of a command), and probably breaks some hairy edges of the pager extension in the name of moving the behavior to core behind a more usable API. I only touched commands in core or that were mentioned in pager.attend, so there's another round of changes to make in hgext to get things fully on the pager bandwagon. That said: it should work! Tests pass modulo small things you'd sort of expect: lots of output churn from the new global variable, and one `help -k` test fails because I marked the pager extension as deprecated.

I'll try and coordinate with someone on IRC tomorrow to go over this with me and get it mailed, since it sounded like everyone on the other thread was largely enthusiastic about this direction.

> No default behaviours were harmed during the making of this change.
> 
> Notes:
> 
> * This patch will break out-of-tree extensions that rely on the
>  location of the old pager module's attend variable.  It is now a
>  static variable named pagercommands on the ui class.
> 
> * It used to be possible to disable the pager via config by disabling
>  the loading of the extension.  With the extension gone, that
>  method no longer works.  Instead, set pager.attend to a command
>  that does not exist, e.g. "--config pager.attend=nothing".
> 
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -107,6 +107,8 @@ globalopts = [
>     ('', 'version', None, _('output version information and exit')),
>     ('h', 'help', None, _('display help and exit')),
>     ('', 'hidden', False, _('consider hidden changesets')),
> +    ('', 'pager', 'auto',
> +     _("when to paginate (boolean, always, auto, or never)"), _('TYPE')),
> ]
> 
> dryrunopts = [('n', 'dry-run', None,
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -816,6 +816,37 @@ def _dispatch(req):
> def _runcommand(ui, options, cmd, cmdfunc):
>     """Run a command function, possibly with profiling enabled."""
>     try:
> +        p = ui.config("pager", "pager", encoding.environ.get("PAGER"))
> +        usepager = ui.pageractive
> +        always = util.parsebool(options['pager'])
> +        auto = options['pager'] == 'auto'
> +
> +        if not p or '--debugger' in sys.argv or not ui.formatted():
> +            pass
> +        elif always:
> +            usepager = True
> +        elif not auto:
> +            usepager = False
> +        else:
> +            attend = ui.configlist('pager', 'attend', ui.pagercommands)
> +            ignore = ui.configlist('pager', 'ignore')
> +            cmds, _ = cmdutil.findcmd(cmd, commands.table)
> +
> +            for cmd in cmds:
> +                var = 'attend-%s' % cmd
> +                if ui.config('pager', var):
> +                    usepager = ui.configbool('pager', var)
> +                    break
> +                if cmd in attend or (cmd not in ignore and not attend):
> +                    usepager = True
> +                    break
> +
> +        ui.pageractive = usepager
> +
> +        if usepager:
> +            ui.setconfig('ui', 'formatted', ui.formatted(), 'pager')
> +            ui.setconfig('ui', 'interactive', False, 'pager')
> +            ui._runpager(p)
>         return cmdfunc()
>     except error.SignatureError:
>         raise error.CommandError(cmd, _('invalid arguments'))
> diff --git a/mercurial/extensions.py b/mercurial/extensions.py
> --- a/mercurial/extensions.py
> +++ b/mercurial/extensions.py
> @@ -27,7 +27,7 @@ from . import (
> _aftercallbacks = {}
> _order = []
> _builtin = set(['hbisect', 'bookmarks', 'parentrevspec', 'progress', 'interhg',
> -                'inotify', 'hgcia'])
> +                'inotify', 'hgcia', 'pager'])
> 
> def extensions(ui=None):
>     if ui:
> diff --git a/mercurial/help.py b/mercurial/help.py
> --- a/mercurial/help.py
> +++ b/mercurial/help.py
> @@ -230,6 +230,7 @@ helptable = sorted([
>      loaddoc('scripting')),
>     (['internals'], _("Technical implementation topics"),
>      internalshelp),
> +    (["pager"], _("Using an External Pager"), loaddoc('pager')),
> ])
> 
> # Maps topics with sub-topics to a list of their sub-topics.
> diff --git a/hgext/pager.py b/mercurial/help/pager.txt
> rename from hgext/pager.py
> rename to mercurial/help/pager.txt
> --- a/hgext/pager.py
> +++ b/mercurial/help/pager.txt
> @@ -1,19 +1,3 @@
> -# pager.py - display output using a pager
> -#
> -# Copyright 2008 David Soria Parra <dsp@php.net>
> -#
> -# This software may be used and distributed according to the terms of the
> -# GNU General Public License version 2 or any later version.
> -#
> -# To load the extension, add it to your configuration file:
> -#
> -#   [extension]
> -#   pager =
> -#
> -# Run 'hg help pager' to get info on configuration.
> -
> -'''browse command output with an external pager
> -
> To set the pager that should be used, set the application variable::
> 
>   [pager]
> @@ -56,117 +40,3 @@ you can use --pager=<value>::
>   - require the pager: `yes` or `on`.
>   - suppress the pager: `no` or `off` (any unrecognized value
>   will also work).
> -
> -'''
> -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,
> -    extensions,
> -    util,
> -    )
> -
> -# Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for
> -# extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
> -# be specifying the version(s) of Mercurial they are tested with, or
> -# 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 objects and descriptors
> -    olduifout = ui.fout
> -    oldstdout = util.stdout
> -    stdoutfd = os.dup(util.stdout.fileno())
> -    stderrfd = os.dup(util.stderr.fileno())
> -
> -    # create new line-buffered stdout so that output can show up immediately
> -    ui.fout = util.stdout = newstdout = os.fdopen(util.stdout.fileno(), 'wb', 1)
> -    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)
> -        pager.stdin.close()
> -        ui.fout = olduifout
> -        util.stdout = oldstdout
> -        # close new stdout while it's associated with pager; otherwise stdout
> -        # fd would be closed when newstdout is deleted
> -        newstdout.close()
> -        # restore original fds: stdout is open again
> -        os.dup2(stdoutfd, util.stdout.fileno())
> -        os.dup2(stderrfd, util.stderr.fileno())
> -        pager.wait()
> -
> -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:
> -            usepager = True
> -        elif not auto:
> -            usepager = False
> -        else:
> -            attend = ui.configlist('pager', 'attend', attended)
> -            ignore = ui.configlist('pager', 'ignore')
> -            cmds, _ = cmdutil.findcmd(cmd, commands.table)
> -
> -            for cmd in cmds:
> -                var = 'attend-%s' % cmd
> -                if ui.config('pager', var):
> -                    usepager = ui.configbool('pager', var)
> -                    break
> -                if (cmd in attend or
> -                     (cmd not in ignore and not attend)):
> -                    usepager = True
> -                    break
> -
> -        setattr(ui, 'pageractive', usepager)
> -
> -        if usepager:
> -            ui.setconfig('ui', 'formatted', ui.formatted(), 'pager')
> -            ui.setconfig('ui', 'interactive', False, 'pager')
> -            ui._runpager(p)
> -        return orig(ui, options, cmd, cmdfunc)
> -
> -    # Wrap dispatch._runcommand after color is loaded so color can see
> -    # ui.pageractive. Otherwise, if we loaded first, color's wrapped
> -    # dispatch._runcommand would run without having access to ui.pageractive.
> -    def afterloaded(loaded):
> -        extensions.wrapfunction(dispatch, '_runcommand', pagecmd)
> -    extensions.afterloaded('color', afterloaded)
> -
> -def extsetup(ui):
> -    commands.globalopts.append(
> -        ('', 'pager', 'auto',
> -         _("when to paginate (boolean, always, auto, or never)"),
> -         _('TYPE')))
> -
> -attended = ['annotate', 'cat', 'diff', 'export', 'glog', 'log', 'qdiff']
> diff --git a/mercurial/statprof.py b/mercurial/statprof.py
> --- a/mercurial/statprof.py
> +++ b/mercurial/statprof.py
> @@ -130,7 +130,7 @@ skips = set(["util.py:check", "extension
>              "color.py:colorcmd", "dispatch.py:checkargs",
>              "dispatch.py:<lambda>", "dispatch.py:_runcatch",
>              "dispatch.py:_dispatch", "dispatch.py:_runcommand",
> -             "pager.py:pagecmd", "dispatch.py:run",
> +             "dispatch.py:run",
>              "dispatch.py:dispatch", "dispatch.py:runcommand",
>              "hg.py:<module>", "evolve.py:warnobserrors",
>          ])
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -7,13 +7,16 @@
> 
> from __future__ import absolute_import
> 
> +import atexit
> import contextlib
> import errno
> import getpass
> import inspect
> import os
> import re
> +import signal
> import socket
> +import subprocess
> import sys
> import tempfile
> import traceback
> @@ -95,6 +98,11 @@ default = %s
> }
> 
> class ui(object):
> +    # commands whose output may be run through the pager
> +    # (this is a class-level variable in case extensions need to add to it)
> +    pagercommands = ['annotate', 'cat', 'diff', 'export', 'glog', 'log',
> +                     'qdiff']
> +
>     def __init__(self, src=None):
>         """Create a fresh new ui object if no src given
> 
> @@ -102,6 +110,7 @@ class ui(object):
>         In most cases, you should use ui.copy() to create a copy of an existing
>         ui object.
>         """
> +        self.pageractive = False
>         # _buffers: used for temporary capture of output
>         self._buffers = []
>         # 3-tuple describing how each buffer in the stack behaves.
> @@ -1251,6 +1260,39 @@ class ui(object):
>             if ('ui', 'quiet') in overrides:
>                 self.fixconfig(section='ui')
> 
> +    def _runpager(self, 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 objects and descriptors
> +        olduifout = self.fout
> +        oldstdout = util.stdout
> +        stdoutfd = os.dup(util.stdout.fileno())
> +        stderrfd = os.dup(util.stderr.fileno())
> +
> +        # create new line-buffered stdout so that output can show up immediately
> +        self.fout = util.stdout = newstdout = os.fdopen(util.stdout.fileno(),
> +                                                        'wb', 1)
> +        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)
> +            pager.stdin.close()
> +            self.fout = olduifout
> +            util.stdout = oldstdout
> +            # close new stdout while it's associated with pager; otherwise
> +            # stdout fd would be closed when newstdout is deleted
> +            newstdout.close()
> +            # restore original fds: stdout is open again
> +            os.dup2(stdoutfd, util.stdout.fileno())
> +            os.dup2(stderrfd, util.stderr.fileno())
> +            pager.wait()
> +
> class paths(dict):
>     """Represents a collection of paths and their configs.
> 
> diff --git a/tests/test-chg.t b/tests/test-chg.t
> --- a/tests/test-chg.t
> +++ b/tests/test-chg.t
> @@ -41,13 +41,11 @@ pager
>>    sys.stdout.write('paged! %r\n' % line)
>> EOF
> 
> -enable pager extension globally, but spawns the master server with no tty:
> +enable pager globally, but spawn the master server with no tty:
> 
>   $ chg init pager
>   $ cd pager
>   $ cat >> $HGRCPATH <<EOF
> -  > [extensions]
> -  > pager =
>> [pager]
>> pager = python $TESTTMP/fakepager.py
>> EOF
> diff --git a/tests/test-completion.t b/tests/test-completion.t
> --- a/tests/test-completion.t
> +++ b/tests/test-completion.t
> @@ -138,6 +138,7 @@ Show the global options
>   --help
>   --hidden
>   --noninteractive
> +  --pager
>   --profile
>   --quiet
>   --repository
> @@ -171,6 +172,7 @@ Show the options for the "serve" command
>   --ipv6
>   --name
>   --noninteractive
> +  --pager
>   --pid-file
>   --port
>   --prefix
> diff --git a/tests/test-extension.t b/tests/test-extension.t
> --- a/tests/test-extension.t
> +++ b/tests/test-extension.t
> @@ -543,6 +543,8 @@ hide outer repo
>       --version           output version information and exit
>    -h --help              display help and exit
>       --hidden            consider hidden changesets
> +      --pager TYPE        when to paginate (boolean, always, auto, or never)
> +                          (default: auto)
> 
> 
> 
> @@ -578,6 +580,8 @@ hide outer repo
>       --version           output version information and exit
>    -h --help              display help and exit
>       --hidden            consider hidden changesets
> +      --pager TYPE        when to paginate (boolean, always, auto, or never)
> +                          (default: auto)
> 
> 
> 
> @@ -856,6 +860,8 @@ extension help itself
>       --version           output version information and exit
>    -h --help              display help and exit
>       --hidden            consider hidden changesets
> +      --pager TYPE        when to paginate (boolean, always, auto, or never)
> +                          (default: auto)
> 
> Make sure that single '-v' option shows help and built-ins only for 'dodo' command
>   $ hg help -v dodo
> @@ -889,6 +895,8 @@ Make sure that single '-v' option shows 
>       --version           output version information and exit
>    -h --help              display help and exit
>       --hidden            consider hidden changesets
> +      --pager TYPE        when to paginate (boolean, always, auto, or never)
> +                          (default: auto)
> 
> In case when extension name doesn't match any of its commands,
> help message should ask for '-v' to get list of built-in aliases
> @@ -960,6 +968,8 @@ help options '-v' and '-v -e' should be 
>       --version           output version information and exit
>    -h --help              display help and exit
>       --hidden            consider hidden changesets
> +      --pager TYPE        when to paginate (boolean, always, auto, or never)
> +                          (default: auto)
> 
>   $ hg help -v -e dudu
>   dudu extension -
> @@ -992,6 +1002,8 @@ help options '-v' and '-v -e' should be 
>       --version           output version information and exit
>    -h --help              display help and exit
>       --hidden            consider hidden changesets
> +      --pager TYPE        when to paginate (boolean, always, auto, or never)
> +                          (default: auto)
> 
> Disabled extension commands:
> 
> diff --git a/tests/test-globalopts.t b/tests/test-globalopts.t
> --- a/tests/test-globalopts.t
> +++ b/tests/test-globalopts.t
> @@ -351,6 +351,7 @@ Testing -h/--help:
>    hgweb         Configuring hgweb
>    internals     Technical implementation topics
>    merge-tools   Merge Tools
> +   pager         Using an External Pager
>    patterns      File Name Patterns
>    phases        Working with Phases
>    revisions     Specifying Revisions
> @@ -432,6 +433,7 @@ Testing -h/--help:
>    hgweb         Configuring hgweb
>    internals     Technical implementation topics
>    merge-tools   Merge Tools
> +   pager         Using an External Pager
>    patterns      File Name Patterns
>    phases        Working with Phases
>    revisions     Specifying Revisions
> diff --git a/tests/test-help.t b/tests/test-help.t
> --- a/tests/test-help.t
> +++ b/tests/test-help.t
> @@ -113,6 +113,7 @@ Short help:
>    hgweb         Configuring hgweb
>    internals     Technical implementation topics
>    merge-tools   Merge Tools
> +   pager         Using an External Pager
>    patterns      File Name Patterns
>    phases        Working with Phases
>    revisions     Specifying Revisions
> @@ -188,6 +189,7 @@ Short help:
>    hgweb         Configuring hgweb
>    internals     Technical implementation topics
>    merge-tools   Merge Tools
> +   pager         Using an External Pager
>    patterns      File Name Patterns
>    phases        Working with Phases
>    revisions     Specifying Revisions
> @@ -262,7 +264,6 @@ Test extension help:
>        largefiles    track large binary files
>        mq            manage a stack of patches
>        notify        hooks for sending email push notifications
> -       pager         browse command output with an external pager
>        patchbomb     command to send changesets as (a series of) patch emails
>        purge         command to delete untracked files from the working
>                      directory
> @@ -326,6 +327,8 @@ Test short command list with verbose opt
>       --version           output version information and exit
>    -h --help              display help and exit
>       --hidden            consider hidden changesets
> +      --pager TYPE        when to paginate (boolean, always, auto, or never)
> +                          (default: auto)
> 
>   (use 'hg help' for the full list of commands)
> 
> @@ -422,6 +425,8 @@ Verbose help for add
>       --version           output version information and exit
>    -h --help              display help and exit
>       --hidden            consider hidden changesets
> +      --pager TYPE        when to paginate (boolean, always, auto, or never)
> +                          (default: auto)
> 
> Test the textwidth config option
> 
> @@ -827,6 +832,7 @@ Test that default list of commands omits
>    hgweb         Configuring hgweb
>    internals     Technical implementation topics
>    merge-tools   Merge Tools
> +   pager         Using an External Pager
>    patterns      File Name Patterns
>    phases        Working with Phases
>    revisions     Specifying Revisions
> @@ -1914,6 +1920,13 @@ Dish up an empty repo; serve it cold.
>   Merge Tools
>   </td></tr>
>   <tr><td>
> +  <a href="/help/pager">
> +  pager
> +  </a>
> +  </td><td>
> +  Using an External Pager
> +  </td></tr>
> +  <tr><td>
>   <a href="/help/patterns">
>   patterns
>   </a>
> @@ -2523,6 +2536,9 @@ Dish up an empty repo; serve it cold.
>   <tr><td></td>
>   <td>--hidden</td>
>   <td>consider hidden changesets</td></tr>
> +  <tr><td></td>
> +  <td>--pager TYPE</td>
> +  <td>when to paginate (boolean, always, auto, or never) (default: auto)</td></tr>
>   </table>
> 
>   </div>
> @@ -2718,6 +2734,9 @@ Dish up an empty repo; serve it cold.
>   <tr><td></td>
>   <td>--hidden</td>
>   <td>consider hidden changesets</td></tr>
> +  <tr><td></td>
> +  <td>--pager TYPE</td>
> +  <td>when to paginate (boolean, always, auto, or never) (default: auto)</td></tr>
>   </table>
> 
>   </div>
> diff --git a/tests/test-hgweb-json.t b/tests/test-hgweb-json.t
> --- a/tests/test-hgweb-json.t
> +++ b/tests/test-hgweb-json.t
> @@ -1593,6 +1593,10 @@ help/ shows help topics
>         "topic": "merge-tools"
>       },
>       {
> +        "summary": "Using an External Pager",
> +        "topic": "pager"
> +      },
> +      {
>         "summary": "File Name Patterns",
>         "topic": "patterns"
>       },
> diff --git a/tests/test-install.t b/tests/test-install.t
> --- a/tests/test-install.t
> +++ b/tests/test-install.t
> @@ -166,6 +166,7 @@ path variables are expanded (~ is the sa
>     help/hg.1.txt
>     help/hgignore.5.txt
>     help/hgrc.5.txt
> +    help/pager.txt
>   Not tracked:
> 
>   $ python wixxml.py templates
> diff --git a/tests/test-pager.t b/tests/test-pager.t
> --- a/tests/test-pager.t
> +++ b/tests/test-pager.t
> @@ -10,8 +10,6 @@ pager was running.
>   $ cat >> $HGRCPATH <<EOF
>> [ui]
>> formatted = yes
> -  > [extensions]
> -  > pager=
>> [pager]
>> pager = python $TESTTMP/fakepager.py
>> EOF
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Augie Fackler <raf@durin42.com> writes:

>> On Feb 6, 2017, at 14:47, Bryan O'Sullivan <bos@serpentine.com> wrote:
>> 
>> # HG changeset patch
>> # User Bryan O'Sullivan <bryano@fb.com>
>> # Date 1486160890 28800
>> #      Fri Feb 03 14:28:10 2017 -0800
>> # Node ID ae22925dafd4a270cb80a7bb54c9d70bce49a633
>> # Parent  1f51b4658f21bbb797e922d155c1046eddccf91d
>> pager: migrate heavily-used extension into core
>
> Here's my counterproposal, not quite done enough to mail (mostly on account of it being late in my timezone):
>
> https://hg.durin42.com/hg-wip/log?rev=%40%3A%3A7c0170f0420f%20-%20%40&revcount=50
>
> It's 32 patches (but most of them are one line, turning on paging of a command), and probably breaks some hairy edges of the pager extension in the name of moving the behavior to core behind a more usable API. I only touched commands in core or that were mentioned in pager.attend, so there's another round of changes to make in hgext to get things fully on the pager bandwagon. That said: it should work! Tests pass modulo small things you'd sort of expect: lots of output churn from the new global variable, and one `help -k` test fails because I marked the pager extension as deprecated.
>
> I'll try and coordinate with someone on IRC tomorrow to go over this with me and get it mailed, since it sounded like everyone on the other thread was largely enthusiastic about this direction.

Wow, impressive!