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

[raf]

(+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 
> # 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, ...):
  

Re: Backwards compatibility before all else? [was Re: [PATCH v2] pager: migrate heavily-used extension into core]

[Simon Farnsworth]

On 06/02/2017 20:03, Augie Fackler wrote:



On Feb 6, 2017, at 15:00, Bryan O'Sullivan  wrote:

On Sun, Feb 5, 2017 at 7:24 PM, Augie Fackler  wrote:


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?

Well, this is an interesting case of a bigger pattern.

To be honest, I think that the long-time insistence on (and acquiescence to) 
backwards compatibility for every little aspect of behaviour for all eternity 
has been extremely stifling. The fact that git users have thrived despite a 
decade of occasional incompatible changes (including enabling pager use by 
default) suggests that the compatibility-before-all perspective wasn't ever 
really prioritised correctly in the first place.

I think that the community would do well to slightly loosen its standards for 
breaking changes. Yes, such changes will cause occasional problems for a small 
number of people, but the alternative of stagnation isn't super appealing.


As a general principle, I agree. I think we'll have to take it on a 
case-by-case basis though: I'd like to avoid things that would cause widespread 
carnage in scripts that have been built over the years using sh scripts, 
chewing gum, and bailing wire.

(My bias on the pager thing, btw, is to go with it, and try and encourage some 
widespread testing early in the cycle to try and sniff out any problems. It's 
rough in this case because our biggest captive tester audiences already default 
the pager to enabled.)


I'd hope that part of that case-by-case basis should be "how many BC 
breaks have we taken this cycle", with an objective of ensuring that 
someone who tries to keep up with our releases doesn't get burnt badly 
by a release that happens to break everything they depend upon.


With suitable release notes (another discussion ongoing right now) 
calling out the BC breaks, giving people who care a chance to object and 
have the BC break made configurable or undone for the next release, we 
should be able to keep our users happy while still improving.


--
Simon Farnsworth
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Augie Fackler]

> On Feb 6, 2017, at 19:26, Jun Wu  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=50
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Jun Wu]

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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Sean Farley]

Augie Fackler  writes:

>> On Feb 6, 2017, at 18:32, Sean Farley  wrote:
>> 
>> Augie Fackler  writes:
>> 
 On Feb 6, 2017, at 18:04, Sean Farley  wrote:
 
 Augie Fackler  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 :-)
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Augie Fackler]

> On Feb 6, 2017, at 18:32, Sean Farley  wrote:
> 
> Augie Fackler  writes:
> 
>>> On Feb 6, 2017, at 18:04, Sean Farley  wrote:
>>> 
>>> Augie Fackler  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...)

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Sean Farley]

Augie Fackler  writes:

>> On Feb 6, 2017, at 18:04, Sean Farley  wrote:
>> 
>> Augie Fackler  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?
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Augie Fackler]

> On Feb 6, 2017, at 18:04, Sean Farley  wrote:
> 
> Augie Fackler  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.

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Sean Farley]

Augie Fackler  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 
>> # 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 

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

[Augie Fackler]

(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 
> # 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, 

Re: Backwards compatibility before all else? [was Re: [PATCH v2] pager: migrate heavily-used extension into core]

[Sean Farley]

Bryan O'Sullivan  writes:

> On Sun, Feb 5, 2017 at 7:24 PM, Augie Fackler  wrote:
>
>>
>> > 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?
>>
>
> Well, this is an interesting case of a bigger pattern.
>
> To be honest, I think that the long-time insistence on (and acquiescence
> to) backwards compatibility for every little aspect of behaviour for all
> eternity has been extremely stifling. The fact that git users have thrived
> despite a decade of occasional incompatible changes (including enabling
> pager use by default) suggests that the compatibility-before-all
> perspective wasn't ever really prioritised correctly in the first place.
>
> I think that the community would do well to slightly loosen its standards
> for breaking changes. Yes, such changes will cause occasional problems for
> a small number of people, but the alternative of stagnation isn't super
> appealing.

This resonates so very much with me.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Backwards compatibility before all else? [was Re: [PATCH v2] pager: migrate heavily-used extension into core]

[Kevin Bullock]

> On Feb 6, 2017, at 14:00, Bryan O'Sullivan  wrote:
> 
> Well, this is an interesting case of a bigger pattern.
> 
> To be honest, I think that the long-time insistence on (and acquiescence to) 
> backwards compatibility for every little aspect of behaviour for all eternity 
> has been extremely stifling. The fact that git users have thrived despite a 
> decade of occasional incompatible changes (including enabling pager use by 
> default) suggests that the compatibility-before-all perspective wasn't ever 
> really prioritised correctly in the first place.
> 
> I think that the community would do well to slightly loosen its standards for 
> breaking changes. Yes, such changes will cause occasional problems for a 
> small number of people, but the alternative of stagnation isn't super 
> appealing.

I for one still generally agree with the strong emphasis we've always placed on 
backward compatibility. I think there's a pretty well-established, if subtle, 
set of distinctions that we've made about different kinds of backward 
compatibility, though. Some of them have been more strongly guarded than 
others, so we should be clear on what we're discussing if we want to change our 
approach.

First, there's compatibility of on-disk repositories and the wire protocol: new 
versions should always be able to interact with old repos. This is one of the 
strongest guarantees we maintain.

Second, there's stability of the command-line output. The degree to which this 
is guarded depends on the command, namely on how likely it is to be used in 
everyone's janky scripts.

And so on, for a bunch more categories that are listed on 
 and more that probably 
aren't.

Anyway, these things often ultimately come down to a case-by-case judgement 
call, and I suspect that the current reviewers will tend to fall a bit less 
often on the side of keeping things static than Matt did. But I think the case 
still needs to be made clearly as to the benefit vs. potential for breakage of 
any particular change.

pacem in terris / мир / शान्ति / ‎‫سَلاَم‬ / 平和
Kevin R. Bullock

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Backwards compatibility before all else? [was Re: [PATCH v2] pager: migrate heavily-used extension into core]

[Augie Fackler]

> On Feb 6, 2017, at 15:00, Bryan O'Sullivan  wrote:
> 
> On Sun, Feb 5, 2017 at 7:24 PM, Augie Fackler  wrote:
> 
> > 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?
> 
> Well, this is an interesting case of a bigger pattern.
> 
> To be honest, I think that the long-time insistence on (and acquiescence to) 
> backwards compatibility for every little aspect of behaviour for all eternity 
> has been extremely stifling. The fact that git users have thrived despite a 
> decade of occasional incompatible changes (including enabling pager use by 
> default) suggests that the compatibility-before-all perspective wasn't ever 
> really prioritised correctly in the first place.
> 
> I think that the community would do well to slightly loosen its standards for 
> breaking changes. Yes, such changes will cause occasional problems for a 
> small number of people, but the alternative of stagnation isn't super 
> appealing.

As a general principle, I agree. I think we'll have to take it on a 
case-by-case basis though: I'd like to avoid things that would cause widespread 
carnage in scripts that have been built over the years using sh scripts, 
chewing gum, and bailing wire.

(My bias on the pager thing, btw, is to go with it, and try and encourage some 
widespread testing early in the cycle to try and sniff out any problems. It's 
rough in this case because our biggest captive tester audiences already default 
the pager to enabled.)
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Backwards compatibility before all else? [was Re: [PATCH v2] pager: migrate heavily-used extension into core]

[Bryan O'Sullivan]

On Sun, Feb 5, 2017 at 7:24 PM, Augie Fackler  wrote:

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

Well, this is an interesting case of a bigger pattern.

To be honest, I think that the long-time insistence on (and acquiescence
to) backwards compatibility for every little aspect of behaviour for all
eternity has been extremely stifling. The fact that git users have thrived
despite a decade of occasional incompatible changes (including enabling
pager use by default) suggests that the compatibility-before-all
perspective wasn't ever really prioritised correctly in the first place.

I think that the community would do well to slightly loosen its standards
for breaking changes. Yes, such changes will cause occasional problems for
a small number of people, but the alternative of stagnation isn't super
appealing.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Bryan O'Sullivan]

On Mon, Feb 6, 2017 at 9:44 AM, Augie Fackler  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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Jun Wu]

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  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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Augie Fackler]

> On Feb 5, 2017, at 9:29 PM, Bryan O'Sullivan  wrote:
> 
> 
> On Sun, Feb 5, 2017 at 1:44 AM, Yuya Nishihara  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

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Bryan O'Sullivan]

On Sun, Feb 5, 2017 at 1:44 AM, Yuya Nishihara  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).
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

[Bryan O'Sullivan]

# HG changeset patch
# User Bryan O'Sullivan 
# 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.

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 
-#
-# 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=::
   - 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,