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

login
register
mail settings
Submitter Bryan O'Sullivan
Date Feb. 4, 2017, 12:16 a.m.
Message ID <30ee18bf947b97eca358.1486167369@bryano-mbp.local>
Download mbox | patch
Permalink /patch/18325/
State Superseded
Headers show

Comments

Bryan O'Sullivan - Feb. 4, 2017, 12:16 a.m.
# HG changeset patch
# User Bryan O'Sullivan <bryano@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.
Sean Farley - Feb. 5, 2017, 12:18 a.m.
Bryan O'Sullivan <bos@serpentine.com> writes:

> # HG changeset patch
> # User Bryan O'Sullivan <bryano@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
Yuya Nishihara - Feb. 5, 2017, 9:44 a.m.
On Fri, 03 Feb 2017 16:16:09 -0800, Bryan O'Sullivan wrote:
> # HG changeset patch
> # User Bryan O'Sullivan <bryano@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.

I like the direction of this patch, but this still involves a behavior
change. If PAGER variable is set but pager extension disabled, pager will
be started wrongly.

>  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'
Bryan O'Sullivan - Feb. 6, 2017, 2:29 a.m.
On Sun, Feb 5, 2017 at 1:44 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> I like the direction of this patch, but this still involves a behavior
> change. If PAGER variable is set but pager extension disabled, pager will
> be started wrongly.
>

I'm inclined to *not* add special code to see if the old pager extension
has been disabled. That's achievable by disabling the pager instead. This
would require a release note, of course (just in case anyone reads them).
Augie Fackler - Feb. 6, 2017, 3:24 a.m.
> On Feb 5, 2017, at 9:29 PM, Bryan O'Sullivan <bos@serpentine.com> wrote:
> 
> 
> On Sun, Feb 5, 2017 at 1:44 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> I like the direction of this patch, but this still involves a behavior
> change. If PAGER variable is set but pager extension disabled, pager will
> be started wrongly.
> 
> I'm inclined to *not* add special code to see if the old pager extension has been disabled. That's achievable by disabling the pager instead. This would require a release note, of course (just in case anyone reads them).

I’m conflicted here: I’ve been chasing my tail on a problem with emacs tramp-mode for months, and finally root-caused it to a missing flag in my pager settings for less, only triggered by emacs running hg over ssh. It was pretty baffling.

On the other hand, it’s clearly the most-right choice to have the pager on as part of the recommended configuration. I’ve been using it (as an experiment) at work for over a year, and I’ve finally gotten used to it and (for the most part) like it.

What do other people think?

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Jun Wu - Feb. 6, 2017, 5:40 p.m.
Excerpts from Augie Fackler's message of 2017-02-05 22:24:39 -0500:
> > On Sun, Feb 5, 2017 at 1:44 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > I like the direction of this patch, but this still involves a behavior
> > change. If PAGER variable is set but pager extension disabled, pager will
> > be started wrongly.
> > 
> > I'm inclined to *not* add special code to see if the old pager extension
> > has been disabled. That's achievable by disabling the pager instead.
> > This would require a release note, of course (just in case anyone reads
> > them).
> I’m conflicted here: I’ve been chasing my tail on a problem with emacs
> tramp-mode for months, and finally root-caused it to a missing flag in my
> pager settings for less, only triggered by emacs running hg over ssh. It
> was pretty baffling.

I guess the TTY check could prevent pager from running incorrectly?

> On the other hand, it’s clearly the most-right choice to have the pager on
> as part of the recommended configuration. I’ve been using it (as an
> experiment) at work for over a year, and I’ve finally gotten used to it
> and (for the most part) like it.
> 
> What do other people think?

I think the point of moving pager to core is to make it more accessible.
If a new user only needs to set PAGER without touching .hgrc to use a pager,
that sounds like a step forward to me.

If BC is a concern, some temporary deprecation warnings might be helpful.

The patch seems to conflict with Simon's stdout change - a rebase is
probably needed. Otherwise it looks good to me, I have especially checked
chg compatibility.
Augie Fackler - Feb. 6, 2017, 5:44 p.m.
> On Feb 6, 2017, at 12:40, Jun Wu <quark@fb.com> wrote:
> 
> Excerpts from Augie Fackler's message of 2017-02-05 22:24:39 -0500:
>>> On Sun, Feb 5, 2017 at 1:44 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>>> I like the direction of this patch, but this still involves a behavior
>>> change. If PAGER variable is set but pager extension disabled, pager will
>>> be started wrongly.
>>> 
>>> I'm inclined to *not* add special code to see if the old pager extension
>>> has been disabled. That's achievable by disabling the pager instead.
>>> This would require a release note, of course (just in case anyone reads
>>> them).
>> I’m conflicted here: I’ve been chasing my tail on a problem with emacs
>> tramp-mode for months, and finally root-caused it to a missing flag in my
>> pager settings for less, only triggered by emacs running hg over ssh. It
>> was pretty baffling.
> 
> I guess the TTY check could prevent pager from running incorrectly?

That's my thought too, but pager already looks for a tty today. I think tramp actually requests a tty on the other end of the connection for some reason. I haven't had a chance to delve deeper.

> On the other hand, it’s clearly the most-right choice to have the pager on
>> as part of the recommended configuration. I’ve been using it (as an
>> experiment) at work for over a year, and I’ve finally gotten used to it
>> and (for the most part) like it.
>> 
>> What do other people think?
> 
> I think the point of moving pager to core is to make it more accessible.
> If a new user only needs to set PAGER without touching .hgrc to use a pager,
> that sounds like a step forward to me.

Yes, we should definitely make pager easier to use. My own informal surveys of users are that even a setting in hgrc would be better than an extension, because there's a perception that an extension is somehow dangerous.

> If BC is a concern, some temporary deprecation warnings might be helpful.
> 
> The patch seems to conflict with Simon's stdout change - a rebase is
> probably needed. Otherwise it looks good to me, I have especially checked
> chg compatibility.

Cool. I'll make a pass on this patch's substance today and try and offer any comments to save time on a v3.

Thanks!
Augie
Augie Fackler - Feb. 6, 2017, 6:30 p.m.
(+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@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'))
Bryan O'Sullivan - Feb. 6, 2017, 7:30 p.m.
On Mon, Feb 6, 2017 at 9:44 AM, Augie Fackler <raf@durin42.com> wrote:

> Yes, we should definitely make pager easier to use. My own informal
> surveys of users are that even a setting in hgrc would be better than an
> extension, because there's a perception that an extension is somehow
> dangerous.
>

Not only that, but extensions are significantly more costly than built-in
code, because they have to be loaded eagerly so that they can commit all
their myriad sins during startup.
Augie Fackler - Feb. 6, 2017, 10:52 p.m.
(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@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'))
Sean Farley - Feb. 6, 2017, 11:04 p.m.
Augie Fackler <raf@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@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.
Augie Fackler - Feb. 6, 2017, 11:13 p.m.
> On Feb 6, 2017, at 18:04, Sean Farley <sean@farley.io> wrote:
> 
> Augie Fackler <raf@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:
[...]

>> (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.

Mostly. pager.attend will probably live on as a config knob for users to force paging of commands that don't request it. I can't see it working without that? Though it'd be the alternative form, that looks like

[pager]
attend-foo = yes

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

I don't know about Windows, but I think we should follow git's lead and default to using *a* pager. On debian, that means sensible-pager, on most other unices that means less, on some unfortunate platforms it means more. In-tree, we should probably default to more, with a recommendation that packagers specify a more reasonable default in the system-wide hgrc.

(I've had a PAGER environment variable for longer than I've had my dotfiles source control, but I have no idea how common this is. For some highly unscientific data, I've posted a poll on twitter: https://twitter.com/durin42/status/828742645145075712 - maybe we'll learn something.)

> The API looks pretty good to me. Nothing off the top of my head that I
> can add / question besides what I asked above.
Sean Farley - Feb. 6, 2017, 11:32 p.m.
Augie Fackler <raf@durin42.com> writes:

>> On Feb 6, 2017, at 18:04, Sean Farley <sean@farley.io> wrote:
>> 
>> Augie Fackler <raf@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:
> [...]
>
>>> (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.
>
> Mostly. pager.attend will probably live on as a config knob for users to force paging of commands that don't request it. I can't see it working without that? Though it'd be the alternative form, that looks like

You mean for third-party commands? My conclusion from your previous
email was that all the internal commands would eventually be patched to
use ui.pager, right?

> [pager]
> attend-foo = yes

Is there anything preventing this form?

>>> 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.
>
> I don't know about Windows, but I think we should follow git's lead and default to using *a* pager. On debian, that means sensible-pager, on most other unices that means less, on some unfortunate platforms it means more. In-tree, we should probably default to more, with a recommendation that packagers specify a more reasonable default in the system-wide hgrc.
>
> (I've had a PAGER environment variable for longer than I've had my dotfiles source control, but I have no idea how common this is. For some highly unscientific data, I've posted a poll on twitter: https://twitter.com/durin42/status/828742645145075712 - maybe we'll learn something.)

Hmmm, I seem to recall talk about vendoring a python pager? Didn't
Anatoly write one?
Augie Fackler - Feb. 6, 2017, 11:56 p.m.
> On Feb 6, 2017, at 18:32, Sean Farley <sean@farley.io> wrote:
> 
> Augie Fackler <raf@durin42.com> writes:
> 
>>> On Feb 6, 2017, at 18:04, Sean Farley <sean@farley.io> wrote:
>>> 
>>> Augie Fackler <raf@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:
>> [...]
>> 
>>>> (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.
>> 
>> Mostly. pager.attend will probably live on as a config knob for users to force paging of commands that don't request it. I can't see it working without that? Though it'd be the alternative form, that looks like
> 
> You mean for third-party commands?

Yep.

> My conclusion from your previous
> email was that all the internal commands would eventually be patched to
> use ui.pager, right?

Most, not all. commit doesn't really want a pager, and summary probably doesn't want it as a default? We'll have to do some fiddling around the margins, but yes, I'm thinking I'll include in my series moving log/export/diff and friends to the new API.

> 
>> [pager]
>> attend-foo = yes
> 
> Is there anything preventing this form?

I believe this already works...

> 
>>>> 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.
>> 
>> I don't know about Windows, but I think we should follow git's lead and default to using *a* pager. On debian, that means sensible-pager, on most other unices that means less, on some unfortunate platforms it means more. In-tree, we should probably default to more, with a recommendation that packagers specify a more reasonable default in the system-wide hgrc.
>> 
>> (I've had a PAGER environment variable for longer than I've had my dotfiles source control, but I have no idea how common this is. For some highly unscientific data, I've posted a poll on twitter: https://twitter.com/durin42/status/828742645145075712 - maybe we'll learn something.)
> 
> Hmmm, I seem to recall talk about vendoring a python pager? Didn't
> Anatoly write one?

I honestly have no idea - someone with a windows clue will probably need to fill in the sensible windows defaults.

(I'm -0 on bundling a pager, but maybe we'll have to do that on windows...)
Sean Farley - Feb. 7, 2017, 12:02 a.m.
Augie Fackler <raf@durin42.com> writes:

>> On Feb 6, 2017, at 18:32, Sean Farley <sean@farley.io> wrote:
>> 
>> Augie Fackler <raf@durin42.com> writes:
>> 
>>>> On Feb 6, 2017, at 18:04, Sean Farley <sean@farley.io> wrote:
>>>> 
>>>> Augie Fackler <raf@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:
>>> [...]
>>> 
>>>>> (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.
>>> 
>>> Mostly. pager.attend will probably live on as a config knob for users to force paging of commands that don't request it. I can't see it working without that? Though it'd be the alternative form, that looks like
>> 
>> You mean for third-party commands?
>
> Yep.
>
>> My conclusion from your previous
>> email was that all the internal commands would eventually be patched to
>> use ui.pager, right?
>
> Most, not all. commit doesn't really want a pager, and summary probably doesn't want it as a default? We'll have to do some fiddling around the margins, but yes, I'm thinking I'll include in my series moving log/export/diff and friends to the new API.

Ok, I see. Sounds good to me.

>>> [pager]
>>> attend-foo = yes
>> 
>> Is there anything preventing this form?
>
> I believe this already works...

Ha, ok, shows how much I dig into pager.attend code.

>>>>> 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.
>>> 
>>> I don't know about Windows, but I think we should follow git's lead and default to using *a* pager. On debian, that means sensible-pager, on most other unices that means less, on some unfortunate platforms it means more. In-tree, we should probably default to more, with a recommendation that packagers specify a more reasonable default in the system-wide hgrc.
>>> 
>>> (I've had a PAGER environment variable for longer than I've had my dotfiles source control, but I have no idea how common this is. For some highly unscientific data, I've posted a poll on twitter: https://twitter.com/durin42/status/828742645145075712 - maybe we'll learn something.)
>> 
>> Hmmm, I seem to recall talk about vendoring a python pager? Didn't
>> Anatoly write one?
>
> I honestly have no idea - someone with a windows clue will probably need to fill in the sensible windows defaults.
>
> (I'm -0 on bundling a pager, but maybe we'll have to do that on windows...)

No worries. It's always something we can add / decide on later. I look
forward to this API and getting pager into core :-)
Jun Wu - Feb. 7, 2017, 12:26 a.m.
Excerpts from Augie Fackler's message of 2017-02-06 17:52:30 -0500:
> (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)

chg would be fine so long as ui.pager() calls ui._runpager().

> [snip]
> 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.)

The API is not new to me, I'm +1 on it, and did the chg ui._runpager
refactoring to make the change easier.

The only concern I have is the amount of work actually needed to finish the
change. It probably has to be a separate series.
 
> As a sketch of where this is headed, API-wise:
> 
> class ui:
>  def pager(self, command, category):

Just to confirm if I understand correctly, command and category are only
used to find out the actual pager command from the config? That looks good.
Maybe swap category and command and make command (or both) optional, to
discourage using commands in configs.
Augie Fackler - Feb. 7, 2017, 5:21 a.m.
> On Feb 6, 2017, at 19:26, Jun Wu <quark@fb.com> wrote:
> 
>> As a sketch of where this is headed, API-wise:
>> 
>> class ui:
>> def pager(self, command, category):
> 
> Just to confirm if I understand correctly, command and category are only
> used to find out the actual pager command from the config? That looks good.
> Maybe swap category and command and make command (or both) optional, to
> discourage using commands in configs.

I couldn't quite wrap my head around categories in the end. The categories I could sort of see are:

cat:
  cat, annotate, grep
status:
  status, summary, resolve
log:
  log, tags, incoming, outgoing
diff:
  export, {log, incoming, outgoing with --patch}, qdiff
???:
  files, manifest, locate, help

For now, the series discards the notion of a category in the 10th patch or so, but we can easily re-add it or make it the preferred option if we can figure out what the ontology is here.

If you want to see the work in progress stack, it's here: https://hg.durin42.com/hg-wip/log?rev=%40%3A%3A7c0170f0420f%20-%20%40&revcount=50

Patch

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)
+
+            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'))
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,119 +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')
-            if util.safehasattr(signal, "SIGPIPE"):
-                signal.signal(signal.SIGPIPE, signal.SIG_DFL)
-            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)
+    pagedcommands = ['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