[PATCH v2] pager: migrate heavily-used extension into core

[raf at durin42.com]

(+yuya explicitly for chg thoughts)

On Fri, Feb 03, 2017 at 04:16:09PM -0800, Bryan O'Sullivan wrote:
> # HG changeset patch
> # User Bryan O'Sullivan <bryano at fb.com>
> # Date 1486160890 28800
> #      Fri Feb 03 14:28:10 2017 -0800
> # Node ID 30ee18bf947b97eca3582555f63eb3b2441e9db8
> # Parent  abf029200e198878a4576a87e095bd8d77d9cea9
> pager: migrate heavily-used extension into core
>
> No default behaviours were harmed during the making of this change.
>
> Note: 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 pagedcommands on the ui class.

I've got feedback on this approach, which I'd like to talk out a bit
before we either decide to land this or go on a v3 voyage. See below.

If this is agreeable, I'll just push other things aside and do this,
since it's something that we've got pretty clear consensus on, and I'm
the one proposing a chunk of work to try and gild the lily.

Sorry for how long this is - if you want, jump to the API sketch at
the bottom and start from there, and look at my paragraph of rationale
only if that looks bad?

>
> 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,39 @@ 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.pagedcommands)
> +            ignore = ui.configlist('pager', 'ignore')
> +            cmds, _ = cmdutil.findcmd(cmd, commands.table)

This bums me out as an approach, because it means that a command is
necessarily always paged or always not paged, which is not always
appropriate. The immediate example I can think of is shelve, which is
multi-mode:

`shelve --list --patch` -> definitely wants to be paged, this is
likely a ton of output

`shelve --interactive` -> is *broken* if a pager is in play.

Rather than stick with the brute-force attend list in core, I'd like
to move in the direction of adding a ui.pager() call to the ui object,
which starts the pager. We'd then have a transitional period (a couple
of releases?) where programmatically setting the attend list was
supported in the old pager extension code, and then always support
setting pager.attend to forcibly page commands which don't think they
want pager love. This also plays reasonably well with chg, because it
means that there's (still) a trivial point for chg to be told "start
the pager now".

My reason for wanting to move away from the attend list is the above,
but also that it's hopelessly buggy in certain circumstances, like
aliases, and it doesn't have a good affordance for new commands to
specify default behavior (other than poking themselves in a list, but
if the user has configured the attend list they no longer get the
benefits of the sane defaults people hopefully put thought into).

How does that sound, overall? I'll tackle the refactoring today or
Wednesday to mail an RFC patch if that doesn't sound too much like the
perfect being the enemy of the good.

(This design, btw, was inspired by the way git handles paging, where
it's largely up to the command to decide if it wants to invoke the
pager.)



As a sketch of where this is headed, API-wise:

class ui:
  def pager(self, command, category):
    ""Starts the pager, if desired by the user.

    `command` specifies the current command the user is running,
    something like `log` or `shelve`. It should be the whole original
    command name, not the partial name or alias name.

    `category` specifies a broad category this pager invocation fits
    into. Examples include `diff`, `log`, `status`, `help`. This
    allows users to disable paging of entire types of commands easily.
    """
    # pager starts, self.pageractive=true, etc

@command
def shelve(ui, ...):
  if action == 'list':
    ui.pager('shelve', 'diff' if --patch else 'log')
  ...

@command
def summary(ui, ...):
  ui.pager('summary', 'status')
  ...

> +
> +            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')
> +            if util.safehasattr(signal, "SIGPIPE"):
> +                signal.signal(signal.SIGPIPE, signal.SIG_DFL)
> +            ui._runpager(p)
>          return cmdfunc()
>      except error.SignatureError:
>          raise error.CommandError(cmd, _('invalid arguments'))