Patchwork [v2] pager: migrate to core

# HG changeset patch
# User Bryan O'Sullivan <bos@serpentine.com>
# Date 1448403972 28800
#      Tue Nov 24 14:26:12 2015 -0800
# Node ID a27d1306d305a0012508441e6262f4c65b9fdf86
# Parent  f668eef04abc3de94f41b527daa0bb7a0cf76f56
pager: migrate to core

On Tue, Nov 24, 2015 at 02:26:44PM -0800, Bryan O'Sullivan wrote:
> # HG changeset patch
> # User Bryan O'Sullivan <bos@serpentine.com>
> # Date 1448403972 28800
> #      Tue Nov 24 14:26:12 2015 -0800
> # Node ID a27d1306d305a0012508441e6262f4c65b9fdf86
> # Parent  f668eef04abc3de94f41b527daa0bb7a0cf76f56
> pager: migrate to core

Looks sensible here, though I'm not sure if there's anything other
than "the work needed to be done" that was on the list for this
project. CC'ing mpm so maybe I can have saved him some time.

>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -74,6 +74,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
> @@ -33,6 +33,7 @@ from . import (
>      fancyopts,
>      hg,
>      hook,
> +    pager,
>      ui as uimod,
>      util,
>  )
> @@ -779,6 +780,7 @@ def _dispatch(req):
>      if shellaliasfn:
>          return shellaliasfn()
>
> +    pager.uisetup(lui)
>      # Configure extensions in phases: uisetup, extsetup, cmdtable, and
>      # reposetup. Programs like TortoiseHg will call _dispatch several
>      # times so we keep track of configured extensions in _loaded.
> diff --git a/mercurial/extensions.py b/mercurial/extensions.py
> --- a/mercurial/extensions.py
> +++ b/mercurial/extensions.py
> @@ -24,7 +24,8 @@ from . import (
>  _extensions = {}
>  _aftercallbacks = {}
>  _order = []
> -_ignore = ['hbisect', 'bookmarks', 'parentrevspec', 'interhg', 'inotify']
> +_ignore = set(['hbisect', 'bookmarks', 'parentrevspec', 'pager', 'interhg',
> +               'inotify'])
>
>  def extensions(ui=None):
>      if ui:
> diff --git a/mercurial/help.py b/mercurial/help.py
> --- a/mercurial/help.py
> +++ b/mercurial/help.py
> @@ -172,6 +172,7 @@ helptable = sorted([
>      (["glossary"], _("Glossary"), loaddoc('glossary')),
>      (["hgignore", "ignore"], _("Syntax for Mercurial Ignore Files"),
>       loaddoc('hgignore')),
> +    (["pager"], _("Using an External Pager"), loaddoc('pager')),
>      (["phases"], _("Working with Phases"), loaddoc('phases')),
>      (['scripting'], _('Using Mercurial from scripts and automation'),
>       loaddoc('scripting')),
> 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]
> @@ -52,103 +36,3 @@ to specify them in your user configurati
>  The --pager=... option can also be used to control when the pager is
>  used. Use a boolean value like yes, no, on, off, or use auto for
>  normal behavior.
> -
> -'''
> -
> -import atexit, sys, os, signal, subprocess
> -from mercurial import commands, dispatch, util, extensions, cmdutil
> -from mercurial.i18n import _
> -
> -# Note for extension authors: ONLY specify testedwith = 'internal' 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 = 'internal'
> -
> -def _runpager(ui, p):
> -    pager = subprocess.Popen(p, shell=True, bufsize=-1,
> -                             close_fds=util.closefds, stdin=subprocess.PIPE,
> -                             stdout=sys.stdout, stderr=sys.stderr)
> -
> -    # back up original file objects and descriptors
> -    olduifout = ui.fout
> -    oldstdout = sys.stdout
> -    stdoutfd = os.dup(sys.stdout.fileno())
> -    stderrfd = os.dup(sys.stderr.fileno())
> -
> -    # create new line-buffered stdout so that output can show up immediately
> -    ui.fout = sys.stdout = newstdout = os.fdopen(sys.stdout.fileno(), 'wb', 1)
> -    os.dup2(pager.stdin.fileno(), sys.stdout.fileno())
> -    if ui._isatty(sys.stderr):
> -        os.dup2(pager.stdin.fileno(), sys.stderr.fileno())
> -
> -    @atexit.register
> -    def killpager():
> -        if util.safehasattr(signal, "SIGINT"):
> -            signal.signal(signal.SIGINT, signal.SIG_IGN)
> -        pager.stdin.close()
> -        ui.fout = olduifout
> -        sys.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, sys.stdout.fileno())
> -        os.dup2(stderrfd, sys.stderr.fileno())
> -        pager.wait()
> -
> -def uisetup(ui):
> -    if '--debugger' in sys.argv or not ui.formatted():
> -        return
> -
> -    def pagecmd(orig, ui, options, cmd, cmdfunc):
> -        p = ui.config("pager", "pager", os.environ.get("PAGER"))
> -        usepager = False
> -        always = util.parsebool(options['pager'])
> -        auto = options['pager'] == 'auto'
> -
> -        if not p:
> -            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')
> -            if util.safehasattr(signal, "SIGPIPE"):
> -                signal.signal(signal.SIGPIPE, signal.SIG_DFL)
> -            _runpager(ui, 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/hgext/pager.py b/mercurial/pager.py
> copy from hgext/pager.py
> copy to mercurial/pager.py
> --- a/hgext/pager.py
> +++ b/mercurial/pager.py
> @@ -4,66 +4,10 @@
>  #
>  # 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]
> -  pager = less -FRX
> -
> -If no pager is set, the pager extensions uses the environment variable
> -$PAGER. If neither pager.pager, nor $PAGER is set, no pager is used.
> -
> -You can disable the pager for certain commands by adding them to the
> -pager.ignore list::
> -
> -  [pager]
> -  ignore = version, help, update
> -
> -You can also enable the pager only for certain commands using
> -pager.attend. Below is the default list of commands to be paged::
> -
> -  [pager]
> -  attend = annotate, cat, diff, export, glog, log, qdiff
> -
> -Setting pager.attend to an empty value will cause all commands to be
> -paged.
> -
> -If pager.attend is present, pager.ignore will be ignored.
> -
> -Lastly, you can enable and disable paging for individual commands with
> -the attend-<command> option. This setting takes precedence over
> -existing attend and ignore options and defaults::
> -
> -  [pager]
> -  attend-cat = false
> -
> -To ignore global commands like :hg:`version` or :hg:`help`, you have
> -to specify them in your user configuration file.
> -
> -The --pager=... option can also be used to control when the pager is
> -used. Use a boolean value like yes, no, on, off, or use auto for
> -normal behavior.
> -
> -'''
>
>  import atexit, sys, os, signal, subprocess
> -from mercurial import commands, dispatch, util, extensions, cmdutil
> -from mercurial.i18n import _
> -
> -# Note for extension authors: ONLY specify testedwith = 'internal' 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 = 'internal'
> +from . import commands, dispatch, util, extensions, cmdutil
> +from .i18n import _
>
>  def _runpager(ui, p):
>      pager = subprocess.Popen(p, shell=True, bufsize=-1,
> @@ -145,10 +89,4 @@ def uisetup(ui):
>          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/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
>     merge-tools   Merge Tools
>     multirevs     Specifying Multiple Revisions
> +   pager         Using an External Pager
>     patterns      File Name Patterns
>     phases        Working with Phases
>     revisions     Specifying Single Revisions
> @@ -189,6 +190,7 @@ Short help:
>     hgweb         Configuring hgweb
>     merge-tools   Merge Tools
>     multirevs     Specifying Multiple Revisions
> +   pager         Using an External Pager
>     patterns      File Name Patterns
>     phases        Working with Phases
>     revisions     Specifying Single Revisions
> @@ -266,7 +268,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
> @@ -332,6 +333,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)
>
> @@ -413,6 +416,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 help option with version option
>
> @@ -762,6 +767,7 @@ Test that default list of commands omits
>     hgweb         Configuring hgweb
>     merge-tools   Merge Tools
>     multirevs     Specifying Multiple Revisions
> +   pager         Using an External Pager
>     patterns      File Name Patterns
>     phases        Working with Phases
>     revisions     Specifying Single Revisions
> @@ -1497,6 +1503,13 @@ Dish up an empty repo; serve it cold.
>    Specifying Multiple Revisions
>    </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>
> @@ -2080,6 +2093,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>
> @@ -2273,6 +2289,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>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel

On 11/24/2015 02:26 PM, Bryan O'Sullivan wrote:
> # HG changeset patch
> # User Bryan O'Sullivan <bos@serpentine.com>
> # Date 1448403972 28800
> #      Tue Nov 24 14:26:12 2015 -0800
> # Node ID a27d1306d305a0012508441e6262f4c65b9fdf86
> # Parent  f668eef04abc3de94f41b527daa0bb7a0cf76f56
> pager: migrate to core

+1 for having the code into core

-1 for changing the default behavior in the same patch.

This requires more discussion. Street rumor also says we have a couple 
of bug with the pager config and behavior that we should probably take 
care of first.

>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -74,6 +74,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')),
>   ]

We should import this with a "never" default to not alter current core 
behavior. Moving the code in is a long awaited event but discussing 
behavior change and implementing them should happen separately.

In the same spirit of adding global flag. I think having a global -P 
flag to do '--pager yes' easily on any command would make sense (this 
should not be done in this patch, just throwing the idea in the air)

>   dryrunopts = [('n', 'dry-run', None,
> diff --git a/mercurial/dispatch.py b/mercurial/dispatch.py
> --- a/mercurial/dispatch.py
> +++ b/mercurial/dispatch.py
> @@ -33,6 +33,7 @@ from . import (
>       fancyopts,
>       hg,
>       hook,
> +    pager,
>       ui as uimod,
>       util,
>   )
> @@ -779,6 +780,7 @@ def _dispatch(req):
>       if shellaliasfn:
>           return shellaliasfn()
>
> +    pager.uisetup(lui)

urg, do you plan to extract the uisetup part to merge it with the actual 
code? Monkey patching Core Mercurial from Core Mercurial is something we 
probably want to avoid.


>       # Configure extensions in phases: uisetup, extsetup, cmdtable, and
>       # reposetup. Programs like TortoiseHg will call _dispatch several
>       # times so we keep track of configured extensions in _loaded.
> diff --git a/mercurial/extensions.py b/mercurial/extensions.py
> --- a/mercurial/extensions.py
> +++ b/mercurial/extensions.py
> @@ -24,7 +24,8 @@ from . import (
>   _extensions = {}
>   _aftercallbacks = {}
>   _order = []
> -_ignore = ['hbisect', 'bookmarks', 'parentrevspec', 'interhg', 'inotify']
> +_ignore = set(['hbisect', 'bookmarks', 'parentrevspec', 'pager', 'interhg',
> +               'inotify'])

Delaying the behavior change means we still need the extensions to turn 
the default configuration to "auto".

On Tue, Nov 24, 2015 at 7:24 PM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:
>
> -1 for changing the default behavior in the same patch.

That's a good point.

> urg, do you plan to extract the uisetup part to merge it with the actual
code? Monkey patching Core Mercurial from Core Mercurial is something we
probably want to avoid.

It's really not monkeypatching anything from my reading. Can you be more
specific about what you object to?

> Street rumor also says we have a couple of bug with the pager config and
behavior that we should probably take care of first.

This idea that everything has to be fixed (even vague unknown things)
before any progress can be made is not obviously productive.

On 11/24/2015 04:33 PM, Bryan O'Sullivan wrote:
> On Tue, Nov 24, 2015 at 7:24 PM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>  >
>  > -1 for changing the default behavior in the same patch.
>
> That's a good point.
>
>  > urg, do you plan to extract the uisetup part to merge it with the
> actual code? Monkey patching Core Mercurial from Core Mercurial is
> something we probably want to avoid.
>
> It's really not monkeypatching anything from my reading. Can you be more
> specific about what you object to?

the current uisetup of pager is calling extensions.wrapfunction, I 
either miss your removal of it or it smell like monkey patching ;-)

>  > Street rumor also says we have a couple of bug with the pager config
> and behavior that we should probably take care of first.
>
> This idea that everything has to be fixed (even vague unknown things)
> before any progress can be made is not obviously productive.

This is not what I meant here. I'm all for moving the code in core. I 
would like to be sure we do not have pager related regression before 
turning it one by default (if we do turn on by default at some point). 
And I'm at least aware of one such bug.

rephrased: Moving the pager code from an extension to the core code base 
without increasing the number of current bugs affecting pager is a great 
step forward and should be pursued. Thanks for making it happen.

On Tue, Nov 24, 2015 at 7:41 PM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
> It's really not monkeypatching anything from my reading. Can you be more
>> specific about what you object to?
>>
>
> the current uisetup of pager is calling extensions.wrapfunction, I either
> miss your removal of it or it smell like monkey patching ;-)


That only happens if the color extension is used, so it doesn't count :-)
Not only that, but it even seems like a reasonable implementation given
that constraint.


> This is not what I meant here. I'm all for moving the code in core. I
> would like to be sure we do not have pager related regression before
> turning it one by default (if we do turn on by default at some point). And
> I'm at least aware of one such bug.
>

Can you please point me at it? I may be able to fix it, but a search of the
bug database for "pager" reveals nothing. Can't fix something without any
hint of its existence :-)