Patchwork [4,of,8,STABLE] pager: rename 'pager.enable' to 'ui.pager'

login
register
mail settings
Submitter Pierre-Yves David
Date May 1, 2017, 4:12 p.m.
Message ID <5f53e796056e717a208d.1493655166@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/20337/
State Accepted
Headers show

Comments

Pierre-Yves David - May 1, 2017, 4:12 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1493649410 -7200
#      Mon May 01 16:36:50 2017 +0200
# Branch stable
# Node ID 5f53e796056e717a208d389b845c0a6d22c405bf
# Parent  ea4296acb04e7d3f7a1e163023c737ef6edf8bee
# EXP-Topic pager
# Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
#              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 5f53e796056e
pager: rename 'pager.enable' to 'ui.pager'

This aligns with what we do for color (see 7fec37746417). Pager is a central
enough notion that having the master config in the [ui] section makes senses. It
will helps with consistency, discoverability. It will also help having a simple
and clear example hgrc mentioning pager.

The previous form of the option had never been released so the rename seems fine.
Pierre-Yves David - May 1, 2017, 10:11 p.m.
On 05/01/2017 10:23 PM, Kevin Bullock wrote:
>> On May 1, 2017, at 11:12, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1493649410 -7200
>> #      Mon May 01 16:36:50 2017 +0200
>> # Branch stable
>> # Node ID 5f53e796056e717a208d389b845c0a6d22c405bf
>> # Parent  ea4296acb04e7d3f7a1e163023c737ef6edf8bee
>> # EXP-Topic pager
>> # Available At https://www.mercurial-scm.org/repo/users/marmoute/mercurial/
>> #              hg pull https://www.mercurial-scm.org/repo/users/marmoute/mercurial/ -r 5f53e796056e
>> pager: rename 'pager.enable' to 'ui.pager'
>>
>> This aligns with what we do for color (see 7fec37746417). Pager is a central
>> enough notion that having the master config in the [ui] section makes senses. It
>> will helps with consistency, discoverability. It will also help having a simple
>> and clear example hgrc mentioning pager.
>>
>> The previous form of the option had never been released so the rename seems fine.
>
> This turns out to be a bit of a can of worms that Augie, Pierre-Yves, Jun, Sean, I, and others have been discussing via VC and IRC today. To summarize:
>
> * pager.enable is inconsistent with ui.color=yes/no/auto/always
>
>   * ...but ui.pager is confusing in comparison to ui.editor and pager.pager
>
> * We've already released pager.enable in 4.2-rc, and while that isn't subject to the normal BC guarantees, it would suck to change the name of the config setting again for our most active testers (i.e. those who bothered to install the RC)

To summarize my main concerns:

* 4.2 introduces a large behavior change by adding a pager by default. 
So their will be a reasonable share of users who will seek to disable 
(back) that pager in the coming months.

   So we should ensure this 4.2 release is smooth on that regard and 
getting the config option in its definitive spot is part of this 
smoothing process.

* Even after the migration, this will be an important config knob that 
people will reach out to.

   This is my main motivation to have it in the main '[ui]' config next 
to the similar 'ui.color' knob. (the exact form is not too important).

* `hg config --edit` and its default content have been a great tool to 
help users. Since pager is a critical part of the user experience, we 
should make sure the global page config knob is mentioned there as soon 
as possible (whatever the form of the config we end up on).

> * Setting ui.pager=yes would mean that even commands that don't invoke the pager would be paged. This seems like a pretty clear bug.
>
> * Additionally, ui.color=yes currently means the same as --color=always, which (Augie and I think) goes a bit too far. Case in point: if a global config sets ui.color=False, a user wanting to turn it back on is likely to simplistically set ui.color=True. They would then get colorized output even if stdout is not a TTY, probably breaking their scripts.
>
>   * This has a pretty obvious workaround (set ui.color=auto instead), so could be fixed in 4.2.1.

I'm not too concerned about these cases. My personal opinion is that I 
don't think they are going to be common enough. I would rather see if we 
receive actual bug report about this before adding the extra complexity 
in the config definition (leading to a more complex UI).

However, I do not have a too strong opinion on this, so if their is a 
consensus on updating it, I agree with Kevin that we can smoothly 
updating it in 4.2.1 without worrisome impact on our users.

Cheers,
Augie Fackler - May 2, 2017, 1:18 a.m.
On Tue, May 02, 2017 at 12:11:28AM +0200, Pierre-Yves David wrote:
> On 05/01/2017 10:23 PM, Kevin Bullock wrote:
> > > On May 1, 2017, at 11:12, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> > >
> > This turns out to be a bit of a can of worms that Augie, Pierre-Yves, Jun, Sean, I, and others have been discussing via VC and IRC today. To summarize:
> >
> > * pager.enable is inconsistent with ui.color=yes/no/auto/always
> >
> >   * ...but ui.pager is confusing in comparison to ui.editor and pager.pager
> >
> > * We've already released pager.enable in 4.2-rc, and while that isn't subject to the normal BC guarantees, it would suck to change the name of the config setting again for our most active testers (i.e. those who bothered to install the RC)
>
> To summarize my main concerns:
>
> * 4.2 introduces a large behavior change by adding a pager by default. So
> their will be a reasonable share of users who will seek to disable (back)
> that pager in the coming months.
>
>   So we should ensure this 4.2 release is smooth on that regard and getting
> the config option in its definitive spot is part of this smoothing process.
>
> * Even after the migration, this will be an important config knob that
> people will reach out to.
>
>   This is my main motivation to have it in the main '[ui]' config next to
> the similar 'ui.color' knob. (the exact form is not too important).

I agree that on the one hand it'd be nice to have ui.color and
ui.pager next to each other, but I feel it's too confusing to have
ui.editor and ui.pager be wildly different (one is a program to
invoke, the other a boolean).

I'd have suggested moving ui.color to color.enabled but the free-form
nature of the existing [color] namespace means that's probably
unwise. It's my opinion, after reflecting on this for most of today,
that the existing names are probably the path of least confusion overall.

Patch

diff --git a/mercurial/help/pager.txt b/mercurial/help/pager.txt
--- a/mercurial/help/pager.txt
+++ b/mercurial/help/pager.txt
@@ -29,7 +29,7 @@  you can use --pager=<value>:
 
 To globally turn off all attempts to use a pager, set::
 
-  [pager]
-  enable = false
+  [ui]
+  pager = false
 
 which will prevent the pager from running.
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -846,7 +846,7 @@  class ui(object):
         if (self._disablepager
             or self.pageractive
             or command in self.configlist('pager', 'ignore')
-            or not self.configbool('pager', 'enable', True)
+            or not self.configbool('ui', 'pager', True)
             or not self.configbool('pager', 'attend-' + command, True)
             # TODO: if we want to allow HGPLAINEXCEPT=pager,
             # formatted() will need some adjustment.
diff --git a/tests/test-pager.t b/tests/test-pager.t
--- a/tests/test-pager.t
+++ b/tests/test-pager.t
@@ -54,21 +54,21 @@  By default diff and log are paged, but i
 
 We can control the pager from the config
 
-  $ hg log --limit 1 --config 'pager.enable=False'
+  $ hg log --limit 1 --config 'ui.pager=False'
   changeset:   10:46106edeeb38
   tag:         tip
   user:        test
   date:        Thu Jan 01 00:00:00 1970 +0000
   summary:     modify a 10
   
-  $ hg log --limit 1 --config 'pager.enable=0'
+  $ hg log --limit 1 --config 'ui.pager=0'
   changeset:   10:46106edeeb38
   tag:         tip
   user:        test
   date:        Thu Jan 01 00:00:00 1970 +0000
   summary:     modify a 10
   
-  $ hg log --limit 1 --config 'pager.enable=1'
+  $ hg log --limit 1 --config 'ui.pager=1'
   paged! 'changeset:   10:46106edeeb38\n'
   paged! 'tag:         tip\n'
   paged! 'user:        test\n'