Patchwork [RFC] ui: introduce sysdefault section for pager and editor configuration

login
register
mail settings
Submitter Augie Fackler
Date March 8, 2017, 11:48 p.m.
Message ID <71fc64c48cfff1b7a212.1489016935@augie-macbookair2.roam.corp.google.com>
Download mbox | patch
Permalink /patch/19041/
State Changes Requested
Headers show

Comments

Augie Fackler - March 8, 2017, 11:48 p.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1489016567 18000
#      Wed Mar 08 18:42:47 2017 -0500
# Node ID 71fc64c48cfff1b7a2120c60e2b958da3263c0dc
# Parent  92f7d6585c185e85763b3bad81b1304b8cdb5937
ui: introduce sysdefault section for pager and editor configuration

The debian package currently has to patch Mercurial to move the
default editor from `vi` to `sensible-editor`. Now that we're growing
another suboptimal-on-most-platforms default program (`more` as the
pager), let's do packagers a small favor and give them a place where
they can specify the default program for their platform, rather than
having to rely on patching code during the build process. I'd expect
the configuration on OS X to be something like:

[sysdefault]
editor = nano
pager = LESS=FRX less

and on debian to be:

[sysdefault]
editor = sensible-editor
pager = sensible-pager
Jun Wu - March 9, 2017, 12:06 a.m.
A "sysdefault" section sounds a bit weird. Usually files under "/etc" are
defining "system defaults". A new section also makes it harder to see what
configs they are overriding.

How about appending ":sysdefault" to normal configs? The idea was also
mentioned by Yuya at [1] and we use them in "[paths]":

  [ui]
  pager:sysdefault = sensible-pager
  editor:sysdefault = sensible-editor

This makes it easier to see what configs they are overriding, and makes the
section less crowded if we want the same thing for other sections.

[1]: https://patchwork.mercurial-scm.org/patch/16825/

Excerpts from Augie Fackler's message of 2017-03-08 18:48:55 -0500:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1489016567 18000
> #      Wed Mar 08 18:42:47 2017 -0500
> # Node ID 71fc64c48cfff1b7a2120c60e2b958da3263c0dc
> # Parent  92f7d6585c185e85763b3bad81b1304b8cdb5937
> ui: introduce sysdefault section for pager and editor configuration
> 
> The debian package currently has to patch Mercurial to move the
> default editor from `vi` to `sensible-editor`. Now that we're growing
> another suboptimal-on-most-platforms default program (`more` as the
> pager), let's do packagers a small favor and give them a place where
> they can specify the default program for their platform, rather than
> having to rely on patching code during the build process. I'd expect
> the configuration on OS X to be something like:
> 
> [sysdefault]
> editor = nano
> pager = LESS=FRX less
> 
> and on debian to be:
> 
> [sysdefault]
> editor = sensible-editor
> pager = sensible-pager
> 
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -907,13 +907,11 @@ class ui(object):
>              # HGPLAINEXCEPT=pager, and the user didn't specify --debug.
>              return
>  
> -        # TODO: add a "system defaults" config section so this default
> -        # of more(1) can be easily replaced with a global
> -        # configuration file. For example, on OS X the sane default is
> -        # less(1), not more(1), and on debian it's
> -        # sensible-pager(1). We should probably also give the system
> -        # default editor command similar treatment.
> -        envpager = encoding.environ.get('PAGER', 'more')
> +        # sysdefault.pager is available for packagers or system
> +        # administrators to specify a saner default pager for their
> +        # environment.
> +        defaultpager = self.config('sysdefault', 'pager', default='more')
> +        envpager = encoding.environ.get('PAGER', defaultpager)
>          pagercmd = self.config('pager', 'pager', envpager)
>          if not pagercmd:
>              return
> @@ -1348,6 +1346,10 @@ class ui(object):
>              editor = 'E'
>          else:
>              editor = 'vi'
> +        # sysdefault.editor is available for packagers or system
> +        # administrators to specify a saner default editor for their
> +        # environment.
> +        editor = self.config("sysdefault", "editor", default=editor)
>          return (encoding.environ.get("HGEDITOR") or
>                  self.config("ui", "editor") or
>                  encoding.environ.get("VISUAL") or
Yuya Nishihara - March 10, 2017, 12:26 a.m.
On Wed, 8 Mar 2017 16:06:12 -0800, Jun Wu wrote:
> A "sysdefault" section sounds a bit weird. Usually files under "/etc" are
> defining "system defaults". A new section also makes it harder to see what
> configs they are overriding.
> 
> How about appending ":sysdefault" to normal configs? The idea was also
> mentioned by Yuya at [1] and we use them in "[paths]":
> 
>   [ui]
>   pager:sysdefault = sensible-pager
>   editor:sysdefault = sensible-editor
> 
> This makes it easier to see what configs they are overriding, and makes the
> section less crowded if we want the same thing for other sections.
> 
> [1]: https://patchwork.mercurial-scm.org/patch/16825/

A separate section sounds good to me. It would serve a similar role to
[merge-tools], which normal users wouldn't touch. I think :attr syntax
is more open to users.
Sean Farley - March 10, 2017, 2:49 a.m.
Kevin Bullock <kbullock+mercurial@ringworld.org> writes:

>> On Mar 8, 2017, at 18:06, Jun Wu <quark@fb.com> wrote:
>> 
>> A "sysdefault" section sounds a bit weird. Usually files under "/etc" are
>> defining "system defaults". A new section also makes it harder to see what
>> configs they are overriding.
>> 
>> How about appending ":sysdefault" to normal configs? The idea was also
>> mentioned by Yuya at [1] and we use them in "[paths]":
>> 
>>  [ui]
>>  pager:sysdefault = sensible-pager
>>  editor:sysdefault = sensible-editor
>> 
>> This makes it easier to see what configs they are overriding, and makes the
>> section less crowded if we want the same thing for other sections.
>
> I like that idea.

I guess I don't follow since I think of things in /etc as the system
defaults to begin with. Are we having a namespace clash with users
having 'foo=BAR' set so that setting 'foo' in /etc won't override that?
Jun Wu - March 10, 2017, 2:52 a.m.
Excerpts from Sean Farley's message of 2017-03-09 18:49:18 -0800:
> Kevin Bullock <kbullock+mercurial@ringworld.org> writes:
> 
> >> On Mar 8, 2017, at 18:06, Jun Wu <quark@fb.com> wrote:
> >> 
> >> A "sysdefault" section sounds a bit weird. Usually files under "/etc" are
> >> defining "system defaults". A new section also makes it harder to see what
> >> configs they are overriding.
> >> 
> >> How about appending ":sysdefault" to normal configs? The idea was also
> >> mentioned by Yuya at [1] and we use them in "[paths]":
> >> 
> >>  [ui]
> >>  pager:sysdefault = sensible-pager
> >>  editor:sysdefault = sensible-editor
> >> 
> >> This makes it easier to see what configs they are overriding, and makes the
> >> section less crowded if we want the same thing for other sections.
> >
> > I like that idea.
> 
> I guess I don't follow since I think of things in /etc as the system
> defaults to begin with. Are we having a namespace clash with users
> having 'foo=BAR' set so that setting 'foo' in /etc won't override that?

The point is to define configs that cannot be overrided by environment
variables. If we check "source", and treat things in "/etc" differently, we
may get rid of this. But that may make the config system more complicated.
Jun Wu - March 10, 2017, 2:58 a.m.
Excerpts from Jun Wu's message of 2017-03-09 18:52:34 -0800:
> Excerpts from Sean Farley's message of 2017-03-09 18:49:18 -0800:
> > Kevin Bullock <kbullock+mercurial@ringworld.org> writes:
> > 
> > >> On Mar 8, 2017, at 18:06, Jun Wu <quark@fb.com> wrote:
> > >> 
> > >> A "sysdefault" section sounds a bit weird. Usually files under "/etc" are
> > >> defining "system defaults". A new section also makes it harder to see what
> > >> configs they are overriding.
> > >> 
> > >> How about appending ":sysdefault" to normal configs? The idea was also
> > >> mentioned by Yuya at [1] and we use them in "[paths]":
> > >> 
> > >>  [ui]
> > >>  pager:sysdefault = sensible-pager
> > >>  editor:sysdefault = sensible-editor
> > >> 
> > >> This makes it easier to see what configs they are overriding, and makes the
> > >> section less crowded if we want the same thing for other sections.
> > >
> > > I like that idea.
> > 
> > I guess I don't follow since I think of things in /etc as the system
> > defaults to begin with. Are we having a namespace clash with users
> > having 'foo=BAR' set so that setting 'foo' in /etc won't override that?
> 
> The point is to define configs that cannot be overrided by environment
> variables. If we check "source", and treat things in "/etc" differently, we
> may get rid of this. But that may make the config system more complicated.

I forgot to mention I have a half-complete series that changes "config" to
layered, immutable config objects that are chainned together. So we can
figure out layers of configs easily. However, that also introduces some
overhead, especially when we want to maintain correct order of config items.

If we decide to go the modern way of having layered immutable configs, at
the cost of scarifying perf or memory usage, I can polish and send that
series.
Sean Farley - March 10, 2017, 3:01 a.m.
Jun Wu <quark@fb.com> writes:

> Excerpts from Sean Farley's message of 2017-03-09 18:49:18 -0800:
>> Kevin Bullock <kbullock+mercurial@ringworld.org> writes:
>> 
>> >> On Mar 8, 2017, at 18:06, Jun Wu <quark@fb.com> wrote:
>> >> 
>> >> A "sysdefault" section sounds a bit weird. Usually files under "/etc" are
>> >> defining "system defaults". A new section also makes it harder to see what
>> >> configs they are overriding.
>> >> 
>> >> How about appending ":sysdefault" to normal configs? The idea was also
>> >> mentioned by Yuya at [1] and we use them in "[paths]":
>> >> 
>> >>  [ui]
>> >>  pager:sysdefault = sensible-pager
>> >>  editor:sysdefault = sensible-editor
>> >> 
>> >> This makes it easier to see what configs they are overriding, and makes the
>> >> section less crowded if we want the same thing for other sections.
>> >
>> > I like that idea.
>> 
>> I guess I don't follow since I think of things in /etc as the system
>> defaults to begin with. Are we having a namespace clash with users
>> having 'foo=BAR' set so that setting 'foo' in /etc won't override that?
>
> The point is to define configs that cannot be overrided by environment
> variables. If we check "source", and treat things in "/etc" differently, we
> may get rid of this. But that may make the config system more complicated.

Ok, I see.
Jun Wu - March 12, 2017, 7:36 p.m.
What if instead of reading environments directly, introduce a new config
layer which is converted from environment variables? Like:

  - (Top)    layer: command line config flags
  -          layer: user configs
  -          layer: config converted from environment variables
                    like, convert "HGEDITOR" to "ui.editor"
  - (Bottom) layer: system configs

In this way, we don't need a [sysdefault] and can just put things in /etc.
And that will solve other problems like:

  <spectral> ryanmce's complaint to me yesterday was HGEDITOR=vim hg
    --config ui.editor=true foo   would run vim, even though editor is
    'specified on command line'...

Excerpts from Augie Fackler's message of 2017-03-08 18:48:55 -0500:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1489016567 18000
> #      Wed Mar 08 18:42:47 2017 -0500
> # Node ID 71fc64c48cfff1b7a2120c60e2b958da3263c0dc
> # Parent  92f7d6585c185e85763b3bad81b1304b8cdb5937
> ui: introduce sysdefault section for pager and editor configuration
> 
> The debian package currently has to patch Mercurial to move the
> default editor from `vi` to `sensible-editor`. Now that we're growing
> another suboptimal-on-most-platforms default program (`more` as the
> pager), let's do packagers a small favor and give them a place where
> they can specify the default program for their platform, rather than
> having to rely on patching code during the build process. I'd expect
> the configuration on OS X to be something like:
> 
> [sysdefault]
> editor = nano
> pager = LESS=FRX less
> 
> and on debian to be:
> 
> [sysdefault]
> editor = sensible-editor
> pager = sensible-pager
> 
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -907,13 +907,11 @@ class ui(object):
>              # HGPLAINEXCEPT=pager, and the user didn't specify --debug.
>              return
>  
> -        # TODO: add a "system defaults" config section so this default
> -        # of more(1) can be easily replaced with a global
> -        # configuration file. For example, on OS X the sane default is
> -        # less(1), not more(1), and on debian it's
> -        # sensible-pager(1). We should probably also give the system
> -        # default editor command similar treatment.
> -        envpager = encoding.environ.get('PAGER', 'more')
> +        # sysdefault.pager is available for packagers or system
> +        # administrators to specify a saner default pager for their
> +        # environment.
> +        defaultpager = self.config('sysdefault', 'pager', default='more')
> +        envpager = encoding.environ.get('PAGER', defaultpager)
>          pagercmd = self.config('pager', 'pager', envpager)
>          if not pagercmd:
>              return
> @@ -1348,6 +1346,10 @@ class ui(object):
>              editor = 'E'
>          else:
>              editor = 'vi'
> +        # sysdefault.editor is available for packagers or system
> +        # administrators to specify a saner default editor for their
> +        # environment.
> +        editor = self.config("sysdefault", "editor", default=editor)
>          return (encoding.environ.get("HGEDITOR") or
>                  self.config("ui", "editor") or
>                  encoding.environ.get("VISUAL") or

Patch

diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -907,13 +907,11 @@  class ui(object):
             # HGPLAINEXCEPT=pager, and the user didn't specify --debug.
             return
 
-        # TODO: add a "system defaults" config section so this default
-        # of more(1) can be easily replaced with a global
-        # configuration file. For example, on OS X the sane default is
-        # less(1), not more(1), and on debian it's
-        # sensible-pager(1). We should probably also give the system
-        # default editor command similar treatment.
-        envpager = encoding.environ.get('PAGER', 'more')
+        # sysdefault.pager is available for packagers or system
+        # administrators to specify a saner default pager for their
+        # environment.
+        defaultpager = self.config('sysdefault', 'pager', default='more')
+        envpager = encoding.environ.get('PAGER', defaultpager)
         pagercmd = self.config('pager', 'pager', envpager)
         if not pagercmd:
             return
@@ -1348,6 +1346,10 @@  class ui(object):
             editor = 'E'
         else:
             editor = 'vi'
+        # sysdefault.editor is available for packagers or system
+        # administrators to specify a saner default editor for their
+        # environment.
+        editor = self.config("sysdefault", "editor", default=editor)
         return (encoding.environ.get("HGEDITOR") or
                 self.config("ui", "editor") or
                 encoding.environ.get("VISUAL") or