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

[Sean Farley]

Augie Fackler <raf at durin42.com> writes:

> (sending again because this seems to have gotten stuck somewhere...)
> (+yuya explicitly for chg thoughts, +smf so someone else can forward to the list if it's stuck again)
>
> 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.)

If I'm understanding you correctly, this will get rid of the
pager.attend variable? If that's true, then I fully support that.

> 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')
>  ...

Would this get rid of the need to set pager.pager=less? I think last
time the pager was brought up (I believe the Munich sprint), there was a
consensus on not relying on the existence of less / more / windows
weirdness.

The API looks pretty good to me. Nothing off the top of my head that I
can add / question besides what I asked above.