Patchwork [RFC] ui: add support for a tweakdefaults knob

login
register
mail settings
Submitter Augie Fackler
Date June 15, 2017, 1:37 a.m.
Message ID <0e5ea7a86a8021d02218.1497490641@imladris.local>
Download mbox | patch
Permalink /patch/21382/
State Accepted
Headers show

Comments

Augie Fackler - June 15, 2017, 1:37 a.m.
# HG changeset patch
# User Augie Fackler <augie@google.com>
# Date 1497488194 14400
#      Wed Jun 14 20:56:34 2017 -0400
# Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
# Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
ui: add support for a tweakdefaults knob

We've been talking for years about a one-stop config knob to opt in to
better behavior. There have been a lot of ideas thrown around, but
they all seem to be too complicated to get anyone to actually do the
work.. As such, this patch is the stupidest thing that can possibly
work in the name of getting a good feature to users.

Right now it's just three config settings that I think are generally
uncontroversial, but I expect to add more soon. That will likely
include adding new config knobs for the express purpose of adding them
to tweakdefaults.
Sean Farley - June 15, 2017, 1:45 a.m.
Augie Fackler <raf@durin42.com> writes:

> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1497488194 14400
> #      Wed Jun 14 20:56:34 2017 -0400
> # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
> # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
> ui: add support for a tweakdefaults knob
>
> We've been talking for years about a one-stop config knob to opt in to
> better behavior. There have been a lot of ideas thrown around, but
> they all seem to be too complicated to get anyone to actually do the
> work.. As such, this patch is the stupidest thing that can possibly
> work in the name of getting a good feature to users.
>
> Right now it's just three config settings that I think are generally
> uncontroversial, but I expect to add more soon. That will likely
> include adding new config knobs for the express purpose of adding them
> to tweakdefaults.

https://media.giphy.com/media/Mes24baGbS5Og/giphy.gif
Sean Farley - June 15, 2017, 1:51 a.m.
Sean Farley <sean@farley.io> writes:

> Augie Fackler <raf@durin42.com> writes:
>
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1497488194 14400
>> #      Wed Jun 14 20:56:34 2017 -0400
>> # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
>> # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
>> ui: add support for a tweakdefaults knob
>>
>> We've been talking for years about a one-stop config knob to opt in to
>> better behavior. There have been a lot of ideas thrown around, but
>> they all seem to be too complicated to get anyone to actually do the
>> work.. As such, this patch is the stupidest thing that can possibly
>> work in the name of getting a good feature to users.
>>
>> Right now it's just three config settings that I think are generally
>> uncontroversial, but I expect to add more soon. That will likely
>> include adding new config knobs for the express purpose of adding them
>> to tweakdefaults.
>
> https://media.giphy.com/media/Mes24baGbS5Og/giphy.gif

In case it wasn't clear, I am excited about this patch. The name is fine
to me since it makes it very clear on what it does.
Siddharth Agarwal - June 15, 2017, 2:26 a.m.
On 6/14/17 6:37 PM, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1497488194 14400
> #      Wed Jun 14 20:56:34 2017 -0400
> # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
> # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
> ui: add support for a tweakdefaults knob

+1 -- this gives us a great path forward.

>
> We've been talking for years about a one-stop config knob to opt in to
> better behavior. There have been a lot of ideas thrown around, but
> they all seem to be too complicated to get anyone to actually do the
> work.. As such, this patch is the stupidest thing that can possibly
> work in the name of getting a good feature to users.
>
> Right now it's just three config settings that I think are generally
> uncontroversial, but I expect to add more soon. That will likely
> include adding new config knobs for the express purpose of adding them
> to tweakdefaults.
>
> diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
> --- a/mercurial/help/config.txt
> +++ b/mercurial/help/config.txt
> @@ -2071,6 +2071,15 @@ User interface controls.
>       on all exceptions, even those recognized by Mercurial (such as
>       IOError or MemoryError). (default: False)
>   
> +``tweakdefaults``
> +
> +    By default Mercurial's behavior changes very little from release
> +    to release, but over time the recommended config settings
> +    shift. Enable this config to opt in to get automatic tweaks to
> +    Mercurial's behavior over time. This config setting will have no
> +    effet if ``HGPLAIN` is set or ``HGPLAINEXCEPT`` is set and does
> +    not include ``tweakdefaults``. (default: False)
> +
>   ``username``
>       The committer of a changeset created when running "commit".
>       Typically a person's name and email address, e.g. ``Fred Widget
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -43,6 +43,20 @@ urlreq = util.urlreq
>   _keepalnum = ''.join(c for c in map(pycompat.bytechr, range(256))
>                        if not c.isalnum())
>   
> +# The config knobs that will be altered (if unset) by ui.tweakdefaults.
> +tweakrc = """
> +[ui]
> +# The rollback command is dangerous. As a rule, don't use it.
> +rollback = False
> +
> +[commands]
> +# Make `hg status` emit cwd-relative paths by default.
> +status.relative = yes
> +
> +[diff]
> +git = 1
> +"""
> +
>   samplehgrcs = {
>       'user':
>   """# example user config (see 'hg help config' for more info)
> @@ -182,6 +196,7 @@ class ui(object):
>               self.fin = src.fin
>               self.pageractive = src.pageractive
>               self._disablepager = src._disablepager
> +            self._tweaked = src._tweaked
>   
>               self._tcfg = src._tcfg.copy()
>               self._ucfg = src._ucfg.copy()
> @@ -205,6 +220,7 @@ class ui(object):
>               self.fin = util.stdin
>               self.pageractive = False
>               self._disablepager = False
> +            self._tweaked = False
>   
>               # shared read-only environment
>               self.environ = encoding.environ
> @@ -241,8 +257,29 @@ class ui(object):
>                       u.fixconfig(section=section)
>               else:
>                   raise error.ProgrammingError('unknown rctype: %s' % t)
> +        u._maybetweakdefaults()
>           return u
>   
> +    def _maybetweakdefaults(self):
> +        if not self.configbool('ui', 'tweakdefaults'):
> +            return
> +        if self._tweaked or self.plain('tweakdefaults'):
> +            return
> +
> +        # Note: it is SUPER IMPORTANT that you set self._tweaked to
> +        # True *before* any calls to setconfig(), otherwise you'll get
> +        # infinite recursion between setconfig and this method.
> +        #
> +        # TODO: We should extract an inner method in setconfig() to
> +        # avoid this weirdness.
> +        self._tweaked = True
> +        tmpcfg = config.config()
> +        tmpcfg.parse('<tweakdefaults>', tweakrc)
> +        for section in tmpcfg:
> +            for name, value in tmpcfg.items(section):
> +                if not self.hasconfig(section, name):
> +                    self.setconfig(section, name, value, "<tweakdefaults>")
> +
>       def copy(self):
>           return self.__class__(self)
>   
> @@ -387,6 +424,7 @@ class ui(object):
>           for cfg in (self._ocfg, self._tcfg, self._ucfg):
>               cfg.set(section, name, value, source)
>           self.fixconfig(section=section)
> +        self._maybetweakdefaults()
>   
>       def _data(self, untrusted):
>           return untrusted and self._ucfg or self._tcfg
> diff --git a/tests/test-status.t b/tests/test-status.t
> --- a/tests/test-status.t
> +++ b/tests/test-status.t
> @@ -107,6 +107,29 @@ combining patterns with root and pattern
>     ? a/in_a
>     ? b/in_b
>   
> +tweaking defaults works
> +  $ hg status --cwd a --config ui.tweakdefaults=yes
> +  ? 1/in_a_1
> +  ? in_a
> +  ? ../b/1/in_b_1
> +  ? ../b/2/in_b_2
> +  ? ../b/in_b
> +  ? ../in_root
> +  $ HGPLAIN=1 hg status --cwd a --config ui.tweakdefaults=yes
> +  ? a/1/in_a_1 (glob)
> +  ? a/in_a (glob)
> +  ? b/1/in_b_1 (glob)
> +  ? b/2/in_b_2 (glob)
> +  ? b/in_b (glob)
> +  ? in_root
> +  $ HGPLAINEXCEPT=tweakdefaults hg status --cwd a --config ui.tweakdefaults=yes
> +  ? 1/in_a_1
> +  ? in_a
> +  ? ../b/1/in_b_1
> +  ? ../b/2/in_b_2
> +  ? ../b/in_b
> +  ? ../in_root
> +
>   relative paths can be requested
>   
>     $ cat >> $HGRCPATH <<EOF
> @@ -128,6 +151,19 @@ relative paths can be requested
>     ? b/in_b (glob)
>     ? in_root
>   
> +if relative paths are explicitly off, tweakdefaults doesn't change it
> +  $ cat >> $HGRCPATH <<EOF
> +  > [commands]
> +  > status.relative = False
> +  > EOF
> +  $ hg status --cwd a --config ui.tweakdefaults=yes
> +  ? a/1/in_a_1 (glob)
> +  ? a/in_a (glob)
> +  ? b/1/in_b_1 (glob)
> +  ? b/2/in_b_2 (glob)
> +  ? b/in_b (glob)
> +  ? in_root
> +
>     $ cd ..
>   
>     $ hg init repo2
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - June 15, 2017, 5:14 a.m.
On Wed, Jun 14, 2017 at 6:37 PM, Augie Fackler <raf@durin42.com> wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1497488194 14400
> #      Wed Jun 14 20:56:34 2017 -0400
> # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
> # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
> ui: add support for a tweakdefaults knob
>
> We've been talking for years about a one-stop config knob to opt in to
> better behavior. There have been a lot of ideas thrown around, but
> they all seem to be too complicated to get anyone to actually do the
> work.. As such, this patch is the stupidest thing that can possibly
> work in the name of getting a good feature to users.
>
> Right now it's just three config settings that I think are generally
> uncontroversial, but I expect to add more soon. That will likely
> include adding new config knobs for the express purpose of adding them
> to tweakdefaults.
>
> diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
> --- a/mercurial/help/config.txt
> +++ b/mercurial/help/config.txt
> @@ -2071,6 +2071,15 @@ User interface controls.
>      on all exceptions, even those recognized by Mercurial (such as
>      IOError or MemoryError). (default: False)
>
> +``tweakdefaults``
> +
> +    By default Mercurial's behavior changes very little from release
> +    to release, but over time the recommended config settings
> +    shift. Enable this config to opt in to get automatic tweaks to
> +    Mercurial's behavior over time. This config setting will have no
> +    effet if ``HGPLAIN` is set or ``HGPLAINEXCEPT`` is set and does
> +    not include ``tweakdefaults``. (default: False)
> +
>  ``username``
>      The committer of a changeset created when running "commit".
>      Typically a person's name and email address, e.g. ``Fred Widget
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -43,6 +43,20 @@ urlreq = util.urlreq
>  _keepalnum = ''.join(c for c in map(pycompat.bytechr, range(256))
>                       if not c.isalnum())
>
> +# The config knobs that will be altered (if unset) by ui.tweakdefaults.
> +tweakrc = """
> +[ui]
> +# The rollback command is dangerous. As a rule, don't use it.
> +rollback = False
> +
> +[commands]
> +# Make `hg status` emit cwd-relative paths by default.
> +status.relative = yes
> +
> +[diff]
> +git = 1
> +"""
> +
>  samplehgrcs = {
>      'user':
>  """# example user config (see 'hg help config' for more info)
> @@ -182,6 +196,7 @@ class ui(object):
>              self.fin = src.fin
>              self.pageractive = src.pageractive
>              self._disablepager = src._disablepager
> +            self._tweaked = src._tweaked
>
>              self._tcfg = src._tcfg.copy()
>              self._ucfg = src._ucfg.copy()
> @@ -205,6 +220,7 @@ class ui(object):
>              self.fin = util.stdin
>              self.pageractive = False
>              self._disablepager = False
> +            self._tweaked = False
>
>              # shared read-only environment
>              self.environ = encoding.environ
> @@ -241,8 +257,29 @@ class ui(object):
>                      u.fixconfig(section=section)
>              else:
>                  raise error.ProgrammingError('unknown rctype: %s' % t)
> +        u._maybetweakdefaults()
>          return u
>
> +    def _maybetweakdefaults(self):
> +        if not self.configbool('ui', 'tweakdefaults'):
> +            return
> +        if self._tweaked or self.plain('tweakdefaults'):
> +            return
> +
> +        # Note: it is SUPER IMPORTANT that you set self._tweaked to
> +        # True *before* any calls to setconfig(), otherwise you'll get
> +        # infinite recursion between setconfig and this method.
> +        #
> +        # TODO: We should extract an inner method in setconfig() to
> +        # avoid this weirdness.
> +        self._tweaked = True
> +        tmpcfg = config.config()
> +        tmpcfg.parse('<tweakdefaults>', tweakrc)
> +        for section in tmpcfg:
> +            for name, value in tmpcfg.items(section):
> +                if not self.hasconfig(section, name):
> +                    self.setconfig(section, name, value, "<tweakdefaults>")
> +
>      def copy(self):
>          return self.__class__(self)
>
> @@ -387,6 +424,7 @@ class ui(object):
>          for cfg in (self._ocfg, self._tcfg, self._ucfg):
>              cfg.set(section, name, value, source)
>          self.fixconfig(section=section)
> +        self._maybetweakdefaults()
>
>      def _data(self, untrusted):
>          return untrusted and self._ucfg or self._tcfg
> diff --git a/tests/test-status.t b/tests/test-status.t
> --- a/tests/test-status.t
> +++ b/tests/test-status.t
> @@ -107,6 +107,29 @@ combining patterns with root and pattern
>    ? a/in_a
>    ? b/in_b
>
> +tweaking defaults works
> +  $ hg status --cwd a --config ui.tweakdefaults=yes
> +  ? 1/in_a_1
> +  ? in_a
> +  ? ../b/1/in_b_1
> +  ? ../b/2/in_b_2
> +  ? ../b/in_b
> +  ? ../in_root
> +  $ HGPLAIN=1 hg status --cwd a --config ui.tweakdefaults=yes
> +  ? a/1/in_a_1 (glob)
> +  ? a/in_a (glob)
> +  ? b/1/in_b_1 (glob)
> +  ? b/2/in_b_2 (glob)
> +  ? b/in_b (glob)
> +  ? in_root
> +  $ HGPLAINEXCEPT=tweakdefaults hg status --cwd a --config ui.tweakdefaults=yes
> +  ? 1/in_a_1
> +  ? in_a
> +  ? ../b/1/in_b_1
> +  ? ../b/2/in_b_2
> +  ? ../b/in_b
> +  ? ../in_root
> +
>  relative paths can be requested
>
>    $ cat >> $HGRCPATH <<EOF
> @@ -128,6 +151,19 @@ relative paths can be requested
>    ? b/in_b (glob)
>    ? in_root
>
> +if relative paths are explicitly off, tweakdefaults doesn't change it

At what config level is the tweaking done? I didn't understand the
code well enough to tell. Concretely, let's say I have
ui.tweakdefaults=yes in my system setting and
command.status.relative=no in my local settings would I get relative
status or not? I would assume not. How about the inverse
(command.status.relative=no in my system settings and
ui.tweakdefaults=yes in my local settings)? I'm not sure what I would
expect in this case, but probably to not get relative status. (It's a
weird example because there's no reason to set
command.status.relative=no in the system settings, but there are
probably more reasonable examples for non-boolean options).

> +  $ cat >> $HGRCPATH <<EOF
> +  > [commands]
> +  > status.relative = False
> +  > EOF
> +  $ hg status --cwd a --config ui.tweakdefaults=yes
> +  ? a/1/in_a_1 (glob)
> +  ? a/in_a (glob)
> +  ? b/1/in_b_1 (glob)
> +  ? b/2/in_b_2 (glob)
> +  ? b/in_b (glob)
> +  ? in_root
> +
>    $ cd ..
>
>    $ hg init repo2
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - June 15, 2017, 2:09 p.m.
> On Jun 15, 2017, at 01:14, Martin von Zweigbergk <martinvonz@google.com> wrote:
> 
> At what config level is the tweaking done?

It's done really late, because it's plausible that the user-level or repo-level hgrc would contain ui.tweakdefaults=True. It checks for all the config entries it wants to set, and if anything is already set to a value it doesn't make any changes.

> I didn't understand the
> code well enough to tell.


> Concretely, let's say I have
> ui.tweakdefaults=yes in my system setting and
> command.status.relative=no in my local settings would I get relative
> status or not? I would assume not.

You would not get relative status.

> How about the inverse
> (command.status.relative=no in my system settings and
> ui.tweakdefaults=yes in my local settings)?

You would also not get relative status.

> I'm not sure what I would
> expect in this case, but probably to not get relative status. (It's a
> weird example because there's no reason to set
> command.status.relative=no in the system settings, but there are
> probably more reasonable examples for non-boolean options).

Yeah, it's definitely rough around the edges, but I think it's better than not having the feature.
Yuya Nishihara - June 15, 2017, 3:19 p.m.
On Wed, 14 Jun 2017 21:37:21 -0400, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1497488194 14400
> #      Wed Jun 14 20:56:34 2017 -0400
> # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
> # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
> ui: add support for a tweakdefaults knob

+1

> +    def _maybetweakdefaults(self):
> +        if not self.configbool('ui', 'tweakdefaults'):
> +            return
> +        if self._tweaked or self.plain('tweakdefaults'):
> +            return
> +
> +        # Note: it is SUPER IMPORTANT that you set self._tweaked to
> +        # True *before* any calls to setconfig(), otherwise you'll get
> +        # infinite recursion between setconfig and this method.
> +        #
> +        # TODO: We should extract an inner method in setconfig() to
> +        # avoid this weirdness.
> +        self._tweaked = True
> +        tmpcfg = config.config()
> +        tmpcfg.parse('<tweakdefaults>', tweakrc)
> +        for section in tmpcfg:
> +            for name, value in tmpcfg.items(section):
> +                if not self.hasconfig(section, name):
> +                    self.setconfig(section, name, value, "<tweakdefaults>")

Maybe we want tmpcfg -> {ucfg, tcfg} -> ocfg layers, but it wouldn't be
doable right now since tmpcfg should be inserted *after* [uto]cfg are
loaded.
Augie Fackler - June 15, 2017, 3:21 p.m.
> On Jun 15, 2017, at 11:19, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Wed, 14 Jun 2017 21:37:21 -0400, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1497488194 14400
>> #      Wed Jun 14 20:56:34 2017 -0400
>> # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
>> # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
>> ui: add support for a tweakdefaults knob
> 
> +1
> 
>> +    def _maybetweakdefaults(self):
>> +        if not self.configbool('ui', 'tweakdefaults'):
>> +            return
>> +        if self._tweaked or self.plain('tweakdefaults'):
>> +            return
>> +
>> +        # Note: it is SUPER IMPORTANT that you set self._tweaked to
>> +        # True *before* any calls to setconfig(), otherwise you'll get
>> +        # infinite recursion between setconfig and this method.
>> +        #
>> +        # TODO: We should extract an inner method in setconfig() to
>> +        # avoid this weirdness.
>> +        self._tweaked = True
>> +        tmpcfg = config.config()
>> +        tmpcfg.parse('<tweakdefaults>', tweakrc)
>> +        for section in tmpcfg:
>> +            for name, value in tmpcfg.items(section):
>> +                if not self.hasconfig(section, name):
>> +                    self.setconfig(section, name, value, "<tweakdefaults>")
> 
> Maybe we want tmpcfg -> {ucfg, tcfg} -> ocfg layers, but it wouldn't be
> doable right now since tmpcfg should be inserted *after* [uto]cfg are
> loaded.

Yeah. Nobody seems motivated enough to do that work, which I totally get, so I did "done" instead of "clean". If it ever gets to be enough of a pain we can refactor, and we can maybe fix it up if we ever redo configuration to be a stack of immutable objects.
Yuya Nishihara - June 15, 2017, 3:48 p.m.
On Thu, 15 Jun 2017 11:21:33 -0400, Augie Fackler wrote:
> > On Jun 15, 2017, at 11:19, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Wed, 14 Jun 2017 21:37:21 -0400, Augie Fackler wrote:
> >> # HG changeset patch
> >> # User Augie Fackler <augie@google.com>
> >> # Date 1497488194 14400
> >> #      Wed Jun 14 20:56:34 2017 -0400
> >> # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
> >> # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
> >> ui: add support for a tweakdefaults knob
> > 
> > +1
> > 
> >> +    def _maybetweakdefaults(self):
> >> +        if not self.configbool('ui', 'tweakdefaults'):
> >> +            return
> >> +        if self._tweaked or self.plain('tweakdefaults'):
> >> +            return
> >> +
> >> +        # Note: it is SUPER IMPORTANT that you set self._tweaked to
> >> +        # True *before* any calls to setconfig(), otherwise you'll get
> >> +        # infinite recursion between setconfig and this method.
> >> +        #
> >> +        # TODO: We should extract an inner method in setconfig() to
> >> +        # avoid this weirdness.
> >> +        self._tweaked = True
> >> +        tmpcfg = config.config()
> >> +        tmpcfg.parse('<tweakdefaults>', tweakrc)
> >> +        for section in tmpcfg:
> >> +            for name, value in tmpcfg.items(section):
> >> +                if not self.hasconfig(section, name):
> >> +                    self.setconfig(section, name, value, "<tweakdefaults>")
> > 
> > Maybe we want tmpcfg -> {ucfg, tcfg} -> ocfg layers, but it wouldn't be
> > doable right now since tmpcfg should be inserted *after* [uto]cfg are
> > loaded.
> 
> Yeah. Nobody seems motivated enough to do that work, which I totally get, so I did "done" instead of "clean". If it ever gets to be enough of a pain we can refactor, and we can maybe fix it up if we ever redo configuration to be a stack of immutable objects.

Nobody except for Jun. I'll review this patch more carefully tomorrow and
probably queue it.
via Mercurial-devel - June 15, 2017, 3:53 p.m.
On Wed, Jun 14, 2017 at 6:37 PM, Augie Fackler <raf@durin42.com> wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1497488194 14400
> #      Wed Jun 14 20:56:34 2017 -0400
> # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
> # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
> ui: add support for a tweakdefaults knob
>
> We've been talking for years about a one-stop config knob to opt in to
> better behavior. There have been a lot of ideas thrown around, but
> they all seem to be too complicated to get anyone to actually do the
> work.. As such, this patch is the stupidest thing that can possibly
> work in the name of getting a good feature to users.
>
> Right now it's just three config settings that I think are generally
> uncontroversial, but I expect to add more soon. That will likely
> include adding new config knobs for the express purpose of adding them
> to tweakdefaults.
>
> diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
> --- a/mercurial/help/config.txt
> +++ b/mercurial/help/config.txt
> @@ -2071,6 +2071,15 @@ User interface controls.
>      on all exceptions, even those recognized by Mercurial (such as
>      IOError or MemoryError). (default: False)
>
> +``tweakdefaults``
> +
> +    By default Mercurial's behavior changes very little from release
> +    to release, but over time the recommended config settings
> +    shift. Enable this config to opt in to get automatic tweaks to
> +    Mercurial's behavior over time. This config setting will have no
> +    effet if ``HGPLAIN` is set or ``HGPLAINEXCEPT`` is set and does
> +    not include ``tweakdefaults``. (default: False)
> +
>  ``username``
>      The committer of a changeset created when running "commit".
>      Typically a person's name and email address, e.g. ``Fred Widget
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -43,6 +43,20 @@ urlreq = util.urlreq
>  _keepalnum = ''.join(c for c in map(pycompat.bytechr, range(256))
>                       if not c.isalnum())
>
> +# The config knobs that will be altered (if unset) by ui.tweakdefaults.
> +tweakrc = """
> +[ui]
> +# The rollback command is dangerous. As a rule, don't use it.
> +rollback = False
> +
> +[commands]
> +# Make `hg status` emit cwd-relative paths by default.
> +status.relative = yes
> +
> +[diff]
> +git = 1
> +"""
> +
>  samplehgrcs = {
>      'user':
>  """# example user config (see 'hg help config' for more info)
> @@ -182,6 +196,7 @@ class ui(object):
>              self.fin = src.fin
>              self.pageractive = src.pageractive
>              self._disablepager = src._disablepager
> +            self._tweaked = src._tweaked
>
>              self._tcfg = src._tcfg.copy()
>              self._ucfg = src._ucfg.copy()
> @@ -205,6 +220,7 @@ class ui(object):
>              self.fin = util.stdin
>              self.pageractive = False
>              self._disablepager = False
> +            self._tweaked = False
>
>              # shared read-only environment
>              self.environ = encoding.environ
> @@ -241,8 +257,29 @@ class ui(object):
>                      u.fixconfig(section=section)
>              else:
>                  raise error.ProgrammingError('unknown rctype: %s' % t)
> +        u._maybetweakdefaults()
>          return u
>
> +    def _maybetweakdefaults(self):
> +        if not self.configbool('ui', 'tweakdefaults'):
> +            return
> +        if self._tweaked or self.plain('tweakdefaults'):
> +            return
> +
> +        # Note: it is SUPER IMPORTANT that you set self._tweaked to
> +        # True *before* any calls to setconfig(), otherwise you'll get
> +        # infinite recursion between setconfig and this method.
> +        #
> +        # TODO: We should extract an inner method in setconfig() to
> +        # avoid this weirdness.
> +        self._tweaked = True
> +        tmpcfg = config.config()
> +        tmpcfg.parse('<tweakdefaults>', tweakrc)
> +        for section in tmpcfg:
> +            for name, value in tmpcfg.items(section):
> +                if not self.hasconfig(section, name):
> +                    self.setconfig(section, name, value, "<tweakdefaults>")
> +
>      def copy(self):
>          return self.__class__(self)
>
> @@ -387,6 +424,7 @@ class ui(object):
>          for cfg in (self._ocfg, self._tcfg, self._ucfg):
>              cfg.set(section, name, value, source)
>          self.fixconfig(section=section)
> +        self._maybetweakdefaults()
>
>      def _data(self, untrusted):
>          return untrusted and self._ucfg or self._tcfg
> diff --git a/tests/test-status.t b/tests/test-status.t
> --- a/tests/test-status.t
> +++ b/tests/test-status.t

As I said on IRC, I'd consider testing this in test-config.t and only
checking the output of "hg (show)config". Since it will be completely
transparent to the users (such as the status command), I think that's
probably enough. Or at least it probably means we can test less in the
non-config tests. It would also be a natural place for testing setting
of the config at different levels (global/user/repo), even if we don't
care about perfecting the behavior in that regard (which I agree about
leaving for later).

> @@ -107,6 +107,29 @@ combining patterns with root and pattern
>    ? a/in_a
>    ? b/in_b
>
> +tweaking defaults works
> +  $ hg status --cwd a --config ui.tweakdefaults=yes
> +  ? 1/in_a_1
> +  ? in_a
> +  ? ../b/1/in_b_1
> +  ? ../b/2/in_b_2
> +  ? ../b/in_b
> +  ? ../in_root
> +  $ HGPLAIN=1 hg status --cwd a --config ui.tweakdefaults=yes
> +  ? a/1/in_a_1 (glob)
> +  ? a/in_a (glob)
> +  ? b/1/in_b_1 (glob)
> +  ? b/2/in_b_2 (glob)
> +  ? b/in_b (glob)
> +  ? in_root
> +  $ HGPLAINEXCEPT=tweakdefaults hg status --cwd a --config ui.tweakdefaults=yes
> +  ? 1/in_a_1
> +  ? in_a
> +  ? ../b/1/in_b_1
> +  ? ../b/2/in_b_2
> +  ? ../b/in_b
> +  ? ../in_root
> +
>  relative paths can be requested
>
>    $ cat >> $HGRCPATH <<EOF
> @@ -128,6 +151,19 @@ relative paths can be requested
>    ? b/in_b (glob)
>    ? in_root
>
> +if relative paths are explicitly off, tweakdefaults doesn't change it
> +  $ cat >> $HGRCPATH <<EOF
> +  > [commands]
> +  > status.relative = False
> +  > EOF
> +  $ hg status --cwd a --config ui.tweakdefaults=yes
> +  ? a/1/in_a_1 (glob)
> +  ? a/in_a (glob)
> +  ? b/1/in_b_1 (glob)
> +  ? b/2/in_b_2 (glob)
> +  ? b/in_b (glob)
> +  ? in_root
> +
>    $ cd ..
>
>    $ hg init repo2
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
via Mercurial-devel - June 15, 2017, 3:57 p.m.
On Thu, Jun 15, 2017 at 8:19 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Wed, 14 Jun 2017 21:37:21 -0400, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1497488194 14400
>> #      Wed Jun 14 20:56:34 2017 -0400
>> # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
>> # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
>> ui: add support for a tweakdefaults knob
>
> +1

I forgot to say, but definitely +1 from me too. Thanks!

>
>> +    def _maybetweakdefaults(self):
>> +        if not self.configbool('ui', 'tweakdefaults'):
>> +            return
>> +        if self._tweaked or self.plain('tweakdefaults'):
>> +            return
>> +
>> +        # Note: it is SUPER IMPORTANT that you set self._tweaked to
>> +        # True *before* any calls to setconfig(), otherwise you'll get
>> +        # infinite recursion between setconfig and this method.
>> +        #
>> +        # TODO: We should extract an inner method in setconfig() to
>> +        # avoid this weirdness.
>> +        self._tweaked = True
>> +        tmpcfg = config.config()
>> +        tmpcfg.parse('<tweakdefaults>', tweakrc)
>> +        for section in tmpcfg:
>> +            for name, value in tmpcfg.items(section):
>> +                if not self.hasconfig(section, name):
>> +                    self.setconfig(section, name, value, "<tweakdefaults>")
>
> Maybe we want tmpcfg -> {ucfg, tcfg} -> ocfg layers, but it wouldn't be
> doable right now since tmpcfg should be inserted *after* [uto]cfg are
> loaded.

I don't think I followed that, but another possible way it could work
is for tweakdefaults to behave as if the settings were updated inline,
so tweakdefaults=yes in system config would behave as if all the
tweaks happened in the system config, etc. Anyway, let's leave that
polish (whatever form it takes) for a later. I think we can live with
that little BC breakage (like we did with the environment variable
overrides by Jun).

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Augie Fackler - June 15, 2017, 4:32 p.m.
> On Jun 15, 2017, at 11:53, Martin von Zweigbergk <martinvonz@google.com> wrote:
> 
> As I said on IRC, I'd consider testing this in test-config.t and only
> checking the output of "hg (show)config". Since it will be completely
> transparent to the users (such as the status command), I think that's
> probably enough. Or at least it probably means we can test less in the
> non-config tests. It would also be a natural place for testing setting
> of the config at different levels (global/user/repo), even if we don't
> care about perfecting the behavior in that regard (which I agree about
> leaving for later).

I could see adding that test - but I like having a case that actually demonstrates that this works "in the wild" too. Should I do a v2 with a test-config.t test added?
via Mercurial-devel - June 15, 2017, 4:34 p.m.
On Thu, Jun 15, 2017 at 9:32 AM, Augie Fackler <raf@durin42.com> wrote:
>
>> On Jun 15, 2017, at 11:53, Martin von Zweigbergk <martinvonz@google.com> wrote:
>>
>> As I said on IRC, I'd consider testing this in test-config.t and only
>> checking the output of "hg (show)config". Since it will be completely
>> transparent to the users (such as the status command), I think that's
>> probably enough. Or at least it probably means we can test less in the
>> non-config tests. It would also be a natural place for testing setting
>> of the config at different levels (global/user/repo), even if we don't
>> care about perfecting the behavior in that regard (which I agree about
>> leaving for later).
>
> I could see adding that test - but I like having a case that actually demonstrates that this works "in the wild" too. Should I do a v2 with a test-config.t test added?

Yes, I'd appreciate that. I still won't queue it, but only because it
sounded like Yuya wanted to review it in more detail tomorrow. I'm
sure I'll accept it quickly thereafter; I'm looking forward to getting
this in and seeing more of FB's tweakdefaults extension move into
core.
Yuya Nishihara - June 16, 2017, 3:14 p.m.
On Wed, 14 Jun 2017 21:37:21 -0400, Augie Fackler wrote:
> # HG changeset patch
> # User Augie Fackler <augie@google.com>
> # Date 1497488194 14400
> #      Wed Jun 14 20:56:34 2017 -0400
> # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
> # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
> ui: add support for a tweakdefaults knob

Queued this, thanks.

> @@ -241,8 +257,29 @@ class ui(object):
>                      u.fixconfig(section=section)
>              else:
>                  raise error.ProgrammingError('unknown rctype: %s' % t)
> +        u._maybetweakdefaults()
>          return u
>  
> +    def _maybetweakdefaults(self):
> +        if not self.configbool('ui', 'tweakdefaults'):
> +            return
> +        if self._tweaked or self.plain('tweakdefaults'):
> +            return

tweakdefaults can't be disabled by --config if it's enabled by rc files.
Maybe this could be fixed by moving ui._maybetweakdefaults() to dispatch.py
where color.setup() is called.

> +        # Note: it is SUPER IMPORTANT that you set self._tweaked to
> +        # True *before* any calls to setconfig(), otherwise you'll get
> +        # infinite recursion between setconfig and this method.
> +        #
> +        # TODO: We should extract an inner method in setconfig() to
> +        # avoid this weirdness.
> +        self._tweaked = True
> +        tmpcfg = config.config()
> +        tmpcfg.parse('<tweakdefaults>', tweakrc)
> +        for section in tmpcfg:
> +            for name, value in tmpcfg.items(section):
> +                if not self.hasconfig(section, name):
> +                    self.setconfig(section, name, value, "<tweakdefaults>")

I have no idea how we should process values which are only set in untrusted
config. Using hasconfig(untrusted=True) might be a bit safer, but there would
still be inconsistency.

Another problem is self._ocfg can shadow repo config which isn't loaded yet
when _maybetweakdefaults() is called.
Augie Fackler - June 16, 2017, 11:26 p.m.
> On Jun 16, 2017, at 11:14, Yuya Nishihara <yuya@tcha.org> wrote:
> 
> On Wed, 14 Jun 2017 21:37:21 -0400, Augie Fackler wrote:
>> # HG changeset patch
>> # User Augie Fackler <augie@google.com>
>> # Date 1497488194 14400
>> #      Wed Jun 14 20:56:34 2017 -0400
>> # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
>> # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
>> ui: add support for a tweakdefaults knob
> 
> Queued this, thanks.
> 
>> @@ -241,8 +257,29 @@ class ui(object):
>>                     u.fixconfig(section=section)
>>             else:
>>                 raise error.ProgrammingError('unknown rctype: %s' % t)
>> +        u._maybetweakdefaults()
>>         return u
>> 
>> +    def _maybetweakdefaults(self):
>> +        if not self.configbool('ui', 'tweakdefaults'):
>> +            return
>> +        if self._tweaked or self.plain('tweakdefaults'):
>> +            return
> 
> tweakdefaults can't be disabled by --config if it's enabled by rc files.
> Maybe this could be fixed by moving ui._maybetweakdefaults() to dispatch.py
> where color.setup() is called.

Good idea! do you want to send a followup, or should I?

> 
>> +        # Note: it is SUPER IMPORTANT that you set self._tweaked to
>> +        # True *before* any calls to setconfig(), otherwise you'll get
>> +        # infinite recursion between setconfig and this method.
>> +        #
>> +        # TODO: We should extract an inner method in setconfig() to
>> +        # avoid this weirdness.
>> +        self._tweaked = True
>> +        tmpcfg = config.config()
>> +        tmpcfg.parse('<tweakdefaults>', tweakrc)
>> +        for section in tmpcfg:
>> +            for name, value in tmpcfg.items(section):
>> +                if not self.hasconfig(section, name):
>> +                    self.setconfig(section, name, value, "<tweakdefaults>")
> 
> I have no idea how we should process values which are only set in untrusted
> config. Using hasconfig(untrusted=True) might be a bit safer, but there would
> still be inconsistency.

I thought untrusted was the default? Or do you just want it explicit?

> 
> Another problem is self._ocfg can shadow repo config which isn't loaded yet
> when _maybetweakdefaults() is called.
Yuya Nishihara - June 17, 2017, 5:01 a.m.
On Fri, 16 Jun 2017 19:26:00 -0400, Augie Fackler wrote:
> > On Jun 16, 2017, at 11:14, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Wed, 14 Jun 2017 21:37:21 -0400, Augie Fackler wrote:
> >> # HG changeset patch
> >> # User Augie Fackler <augie@google.com>
> >> # Date 1497488194 14400
> >> #      Wed Jun 14 20:56:34 2017 -0400
> >> # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
> >> # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
> >> ui: add support for a tweakdefaults knob
> > 
> > Queued this, thanks.
> > 
> >> @@ -241,8 +257,29 @@ class ui(object):
> >>                     u.fixconfig(section=section)
> >>             else:
> >>                 raise error.ProgrammingError('unknown rctype: %s' % t)
> >> +        u._maybetweakdefaults()
> >>         return u
> >> 
> >> +    def _maybetweakdefaults(self):
> >> +        if not self.configbool('ui', 'tweakdefaults'):
> >> +            return
> >> +        if self._tweaked or self.plain('tweakdefaults'):
> >> +            return
> > 
> > tweakdefaults can't be disabled by --config if it's enabled by rc files.
> > Maybe this could be fixed by moving ui._maybetweakdefaults() to dispatch.py
> > where color.setup() is called.
> 
> Good idea! do you want to send a followup, or should I?

I haven't started that yet. Perhaps we should add some test-config.t tests
before doing further tweaks.

> >> +        # Note: it is SUPER IMPORTANT that you set self._tweaked to
> >> +        # True *before* any calls to setconfig(), otherwise you'll get
> >> +        # infinite recursion between setconfig and this method.
> >> +        #
> >> +        # TODO: We should extract an inner method in setconfig() to
> >> +        # avoid this weirdness.
> >> +        self._tweaked = True
> >> +        tmpcfg = config.config()
> >> +        tmpcfg.parse('<tweakdefaults>', tweakrc)
> >> +        for section in tmpcfg:
> >> +            for name, value in tmpcfg.items(section):
> >> +                if not self.hasconfig(section, name):
> >> +                    self.setconfig(section, name, value, "<tweakdefaults>")
> > 
> > I have no idea how we should process values which are only set in untrusted
> > config. Using hasconfig(untrusted=True) might be a bit safer, but there would
> > still be inconsistency.
> 
> I thought untrusted was the default? Or do you just want it explicit?

untrusted is False by default. I think this and the repo config problems can
be mitigated by not setting tweaked values to _ocfg.

   if not tcfg.hasitem(section, name):
       tcfg.set(section, name, value, "<tweakdefaults>")
   if not ucfg.hasitem(section, name):
       ucfg.set(section, name, value, "<tweakdefaults>")
   fixconfig()
Pierre-Yves David - June 18, 2017, 7:01 p.m.
On 06/15/2017 05:48 PM, Yuya Nishihara wrote:
> On Thu, 15 Jun 2017 11:21:33 -0400, Augie Fackler wrote:
>>> On Jun 15, 2017, at 11:19, Yuya Nishihara <yuya@tcha.org> wrote:
>>> On Wed, 14 Jun 2017 21:37:21 -0400, Augie Fackler wrote:
>>>> # HG changeset patch
>>>> # User Augie Fackler <augie@google.com>
>>>> # Date 1497488194 14400
>>>> #      Wed Jun 14 20:56:34 2017 -0400
>>>> # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
>>>> # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
>>>> ui: add support for a tweakdefaults knob
>>>
>>> +1
>>>
>>>> +    def _maybetweakdefaults(self):
>>>> +        if not self.configbool('ui', 'tweakdefaults'):
>>>> +            return
>>>> +        if self._tweaked or self.plain('tweakdefaults'):
>>>> +            return
>>>> +
>>>> +        # Note: it is SUPER IMPORTANT that you set self._tweaked to
>>>> +        # True *before* any calls to setconfig(), otherwise you'll get
>>>> +        # infinite recursion between setconfig and this method.
>>>> +        #
>>>> +        # TODO: We should extract an inner method in setconfig() to
>>>> +        # avoid this weirdness.
>>>> +        self._tweaked = True
>>>> +        tmpcfg = config.config()
>>>> +        tmpcfg.parse('<tweakdefaults>', tweakrc)
>>>> +        for section in tmpcfg:
>>>> +            for name, value in tmpcfg.items(section):
>>>> +                if not self.hasconfig(section, name):
>>>> +                    self.setconfig(section, name, value, "<tweakdefaults>")
>>>
>>> Maybe we want tmpcfg -> {ucfg, tcfg} -> ocfg layers, but it wouldn't be
>>> doable right now since tmpcfg should be inserted *after* [uto]cfg are
>>> loaded.
>>
>> Yeah. Nobody seems motivated enough to do that work, which I totally get, so I did "done" instead of "clean". If it ever gets to be enough of a pain we can refactor, and we can maybe fix it up if we ever redo configuration to be a stack of immutable objects.
>
> Nobody except for Jun. I'll review this patch more carefully tomorrow and
> probably queue it.

A registry of config option including default value would also fit that 
need. the tweak default knob could update these default in (a copy of) 
the config registry. That would skip the explicit update of the config 
content (and associated concerns).

There have been talk about such registry of option for quite some time 
(including at the 4.2 central sprint). David Demelier ping me about 
this[1] again 10 days ago and found some time to poke at it yesterday.

It turned out I ended up with something that work, I've started 
patchbombing it[2]. Once this is in, we can update the tweakdefault flag 
to use this.

[1] as part of his quest for 
https://www.mercurial-scm.org/wiki/ConfigConsolidationPlan
[2] 
https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-June/099866.html

Cheers,
Augie Fackler - June 19, 2017, 1:59 a.m.
> On Jun 17, 2017, at 01:01, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>>> I have no idea how we should process values which are only set in untrusted
>>> config. Using hasconfig(untrusted=True) might be a bit safer, but there would
>>> still be inconsistency.
>> 
>> I thought untrusted was the default? Or do you just want it explicit?
> 
> untrusted is False by default. I think this and the repo config problems can
> be mitigated by not setting tweaked values to _ocfg.
> 
>   if not tcfg.hasitem(section, name):
>       tcfg.set(section, name, value, "<tweakdefaults>")
>   if not ucfg.hasitem(section, name):
>       ucfg.set(section, name, value, "<tweakdefaults>")
>   fixconfig()

Oh, I see the problem now. I'm not sure how to address that. It was intentional that tweakdefaults is only respected if it's a trusted config entry, so all its items can be treated as trusted.

I think your fix sounds reasonable for the setting of the config items. Should I roll a v3 that moves the tweakdefaults() call to dispatch and make it work this way instead of on (ab)using ui.setconfig?

Thanks!
Augie
Augie Fackler - June 19, 2017, 2:29 a.m.
> On Jun 18, 2017, at 21:59, Augie Fackler <raf@durin42.com> wrote:
> 
>> 
>> On Jun 17, 2017, at 01:01, Yuya Nishihara <yuya@tcha.org> wrote:
>> 
>>>> I have no idea how we should process values which are only set in untrusted
>>>> config. Using hasconfig(untrusted=True) might be a bit safer, but there would
>>>> still be inconsistency.
>>> 
>>> I thought untrusted was the default? Or do you just want it explicit?
>> 
>> untrusted is False by default. I think this and the repo config problems can
>> be mitigated by not setting tweaked values to _ocfg.
>> 
>>  if not tcfg.hasitem(section, name):
>>      tcfg.set(section, name, value, "<tweakdefaults>")
>>  if not ucfg.hasitem(section, name):
>>      ucfg.set(section, name, value, "<tweakdefaults>")
>>  fixconfig()
> 
> Oh, I see the problem now. I'm not sure how to address that. It was intentional that tweakdefaults is only respected if it's a trusted config entry, so all its items can be treated as trusted.
> 
> I think your fix sounds reasonable for the setting of the config items. Should I roll a v3 that moves the tweakdefaults() call to dispatch and make it work this way instead of on (ab)using ui.setconfig?

I just noticed the tweakdefaults patch is pretty far back in the stack at this point. I can do a followup if the items I've mentioned here sound good (tomorrow, since it's time for me to sleep now...)

> 
> Thanks!
> Augie
David Soria Parra - June 19, 2017, 8:06 a.m.
On Sat, Jun 17, 2017 at 12:14:22AM +0900, Yuya Nishihara wrote:
> On Wed, 14 Jun 2017 21:37:21 -0400, Augie Fackler wrote:
> > # HG changeset patch
> > # User Augie Fackler <augie@google.com>
> > # Date 1497488194 14400
> > #      Wed Jun 14 20:56:34 2017 -0400
> > # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
> > # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
> > ui: add support for a tweakdefaults knob
> 
> Queued this, thanks.
> 
I briefly glanced over this. I like the direciton, but prefer our initial
proposal of using ui.compat= with multiple compat knobs. I can revive my patch
for this, but I was initially holding back to wait for Jun's immutable config
work. ui.compat would allow us to allow to add multiple configuration points,
e.g. ui.compat=hg4.0, ui.compat=latest. I feel that the name `tweakdefaults` is
not perfectly suitable if this becomes a one-stop-shop for changing behavior
rather than just setting defaults.

Besides these minors things, I am hugely in favor of this.
Yuya Nishihara - June 19, 2017, 1:31 p.m.
On Sun, 18 Jun 2017 21:59:28 -0400, Augie Fackler wrote:
> 
> > On Jun 17, 2017, at 01:01, Yuya Nishihara <yuya@tcha.org> wrote:
> > 
> >>> I have no idea how we should process values which are only set in untrusted
> >>> config. Using hasconfig(untrusted=True) might be a bit safer, but there would
> >>> still be inconsistency.
> >> 
> >> I thought untrusted was the default? Or do you just want it explicit?
> > 
> > untrusted is False by default. I think this and the repo config problems can
> > be mitigated by not setting tweaked values to _ocfg.
> > 
> >   if not tcfg.hasitem(section, name):
> >       tcfg.set(section, name, value, "<tweakdefaults>")
> >   if not ucfg.hasitem(section, name):
> >       ucfg.set(section, name, value, "<tweakdefaults>")
> >   fixconfig()
> 
> Oh, I see the problem now. I'm not sure how to address that. It was intentional that tweakdefaults is only respected if it's a trusted config entry, so all its items can be treated as trusted.

I agree it's probably okay to ignore an untrusted tweakdefaults option.

> I think your fix sounds reasonable for the setting of the config items. Should I roll a v3 that moves the tweakdefaults() call to dispatch and make it work this way instead of on (ab)using ui.setconfig?

Not using setconfig() sounds nice, too.
Yuya Nishihara - June 19, 2017, 1:54 p.m.
On Sun, 18 Jun 2017 21:01:03 +0200, Pierre-Yves David wrote:
> 
> 
> On 06/15/2017 05:48 PM, Yuya Nishihara wrote:
> > On Thu, 15 Jun 2017 11:21:33 -0400, Augie Fackler wrote:
> >>> On Jun 15, 2017, at 11:19, Yuya Nishihara <yuya@tcha.org> wrote:
> >>> On Wed, 14 Jun 2017 21:37:21 -0400, Augie Fackler wrote:
> >>>> # HG changeset patch
> >>>> # User Augie Fackler <augie@google.com>
> >>>> # Date 1497488194 14400
> >>>> #      Wed Jun 14 20:56:34 2017 -0400
> >>>> # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
> >>>> # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
> >>>> ui: add support for a tweakdefaults knob
> >>>
> >>> +1
> >>>
> >>>> +    def _maybetweakdefaults(self):
> >>>> +        if not self.configbool('ui', 'tweakdefaults'):
> >>>> +            return
> >>>> +        if self._tweaked or self.plain('tweakdefaults'):
> >>>> +            return
> >>>> +
> >>>> +        # Note: it is SUPER IMPORTANT that you set self._tweaked to
> >>>> +        # True *before* any calls to setconfig(), otherwise you'll get
> >>>> +        # infinite recursion between setconfig and this method.
> >>>> +        #
> >>>> +        # TODO: We should extract an inner method in setconfig() to
> >>>> +        # avoid this weirdness.
> >>>> +        self._tweaked = True
> >>>> +        tmpcfg = config.config()
> >>>> +        tmpcfg.parse('<tweakdefaults>', tweakrc)
> >>>> +        for section in tmpcfg:
> >>>> +            for name, value in tmpcfg.items(section):
> >>>> +                if not self.hasconfig(section, name):
> >>>> +                    self.setconfig(section, name, value, "<tweakdefaults>")
> >>>
> >>> Maybe we want tmpcfg -> {ucfg, tcfg} -> ocfg layers, but it wouldn't be
> >>> doable right now since tmpcfg should be inserted *after* [uto]cfg are
> >>> loaded.
> >>
> >> Yeah. Nobody seems motivated enough to do that work, which I totally get, so I did "done" instead of "clean". If it ever gets to be enough of a pain we can refactor, and we can maybe fix it up if we ever redo configuration to be a stack of immutable objects.
> >
> > Nobody except for Jun. I'll review this patch more carefully tomorrow and
> > probably queue it.
> 
> A registry of config option including default value would also fit that 
> need. the tweak default knob could update these default in (a copy of) 
> the config registry. That would skip the explicit update of the config 
> content (and associated concerns).

So it's a kind of config layer like defaults -> u/tcfg -> ocfg? Perhaps that
will solve the issue of inserting tweaked defaults without overriding.

> There have been talk about such registry of option for quite some time 
> (including at the 4.2 central sprint). David Demelier ping me about 
> this[1] again 10 days ago and found some time to poke at it yesterday.
> 
> It turned out I ended up with something that work, I've started 
> patchbombing it[2]. Once this is in, we can update the tweakdefault flag 
> to use this.
> 
> [1] as part of his quest for 
> https://www.mercurial-scm.org/wiki/ConfigConsolidationPlan
> [2] 
> https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-June/099866.html

These patches look a good move, though I think the config registry idea is
bloated a bit. Suppose we have tons of config items, I don't think it's good
idea to register them per item.
Pierre-Yves David - June 19, 2017, 2:04 p.m.
On 06/19/2017 03:54 PM, Yuya Nishihara wrote:
> On Sun, 18 Jun 2017 21:01:03 +0200, Pierre-Yves David wrote:
>>
>>
>> On 06/15/2017 05:48 PM, Yuya Nishihara wrote:
>>> On Thu, 15 Jun 2017 11:21:33 -0400, Augie Fackler wrote:
>>>>> On Jun 15, 2017, at 11:19, Yuya Nishihara <yuya@tcha.org> wrote:
>>>>> On Wed, 14 Jun 2017 21:37:21 -0400, Augie Fackler wrote:
>>>>>> # HG changeset patch
>>>>>> # User Augie Fackler <augie@google.com>
>>>>>> # Date 1497488194 14400
>>>>>> #      Wed Jun 14 20:56:34 2017 -0400
>>>>>> # Node ID 0e5ea7a86a8021d02218c35b07366ac6081ab3fb
>>>>>> # Parent  3abba5bc34546951b11b1bd3f5e5c77b90d950d1
>>>>>> ui: add support for a tweakdefaults knob
>>>>>
>>>>> +1
>>>>>
>>>>>> +    def _maybetweakdefaults(self):
>>>>>> +        if not self.configbool('ui', 'tweakdefaults'):
>>>>>> +            return
>>>>>> +        if self._tweaked or self.plain('tweakdefaults'):
>>>>>> +            return
>>>>>> +
>>>>>> +        # Note: it is SUPER IMPORTANT that you set self._tweaked to
>>>>>> +        # True *before* any calls to setconfig(), otherwise you'll get
>>>>>> +        # infinite recursion between setconfig and this method.
>>>>>> +        #
>>>>>> +        # TODO: We should extract an inner method in setconfig() to
>>>>>> +        # avoid this weirdness.
>>>>>> +        self._tweaked = True
>>>>>> +        tmpcfg = config.config()
>>>>>> +        tmpcfg.parse('<tweakdefaults>', tweakrc)
>>>>>> +        for section in tmpcfg:
>>>>>> +            for name, value in tmpcfg.items(section):
>>>>>> +                if not self.hasconfig(section, name):
>>>>>> +                    self.setconfig(section, name, value, "<tweakdefaults>")
>>>>>
>>>>> Maybe we want tmpcfg -> {ucfg, tcfg} -> ocfg layers, but it wouldn't be
>>>>> doable right now since tmpcfg should be inserted *after* [uto]cfg are
>>>>> loaded.
>>>>
>>>> Yeah. Nobody seems motivated enough to do that work, which I totally get, so I did "done" instead of "clean". If it ever gets to be enough of a pain we can refactor, and we can maybe fix it up if we ever redo configuration to be a stack of immutable objects.
>>>
>>> Nobody except for Jun. I'll review this patch more carefully tomorrow and
>>> probably queue it.
>>
>> A registry of config option including default value would also fit that
>> need. the tweak default knob could update these default in (a copy of)
>> the config registry. That would skip the explicit update of the config
>> content (and associated concerns).
>
> So it's a kind of config layer like defaults -> u/tcfg -> ocfg? Perhaps that
> will solve the issue of inserting tweaked defaults without overriding.

Yes, still want the config layer approach to handle further overwrite 
but this handles the lower, default layers that needs to be special anyway.

(adding David in CC as mentioned he needed something similar to make 
progress on his ui.compat idea)

>> There have been talk about such registry of option for quite some time
>> (including at the 4.2 central sprint). David Demelier ping me about
>> this[1] again 10 days ago and found some time to poke at it yesterday.
>>
>> It turned out I ended up with something that work, I've started
>> patchbombing it[2]. Once this is in, we can update the tweakdefault flag
>> to use this.
>>
>> [1] as part of his quest for
>> https://www.mercurial-scm.org/wiki/ConfigConsolidationPlan
>> [2]
>> https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-June/099866.html
>
> These patches look a good move, though I think the config registry idea is
> bloated a bit. Suppose we have tons of config items, I don't think it's good
> idea to register them per item.

I'm not sure what is your concerns here can you elaborate? We have about 
200 known config option in core. That seems a manageable number of object.

As centralized config growth feature it will needs one item for each 
config option anyway (alias, default, documentation, etc...).
Do you have an alternative idea in mind?

Cheers,
Yuya Nishihara - June 19, 2017, 2:58 p.m.
On Mon, 19 Jun 2017 16:04:41 +0200, Pierre-Yves David wrote:
> >> There have been talk about such registry of option for quite some time
> >> (including at the 4.2 central sprint). David Demelier ping me about
> >> this[1] again 10 days ago and found some time to poke at it yesterday.
> >>
> >> It turned out I ended up with something that work, I've started
> >> patchbombing it[2]. Once this is in, we can update the tweakdefault flag
> >> to use this.
> >>
> >> [1] as part of his quest for
> >> https://www.mercurial-scm.org/wiki/ConfigConsolidationPlan
> >> [2]
> >> https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-June/099866.html
> >
> > These patches look a good move, though I think the config registry idea is
> > bloated a bit. Suppose we have tons of config items, I don't think it's good
> > idea to register them per item.
> 
> I'm not sure what is your concerns here can you elaborate? We have about 
> 200 known config option in core. That seems a manageable number of object.

Sorry, I just felt it's overengineered. I don't have no particular concern
about speed.

> As centralized config growth feature it will needs one item for each 
> config option anyway (alias, default, documentation, etc...).
> Do you have an alternative idea in mind?

Hmm. If we want these features, perhaps configitem() function would be the
best.
Pierre-Yves David - June 19, 2017, 3:32 p.m.
On 06/19/2017 04:58 PM, Yuya Nishihara wrote:
> On Mon, 19 Jun 2017 16:04:41 +0200, Pierre-Yves David wrote:
>>>> There have been talk about such registry of option for quite some time
>>>> (including at the 4.2 central sprint). David Demelier ping me about
>>>> this[1] again 10 days ago and found some time to poke at it yesterday.
>>>>
>>>> It turned out I ended up with something that work, I've started
>>>> patchbombing it[2]. Once this is in, we can update the tweakdefault flag
>>>> to use this.
>>>>
>>>> [1] as part of his quest for
>>>> https://www.mercurial-scm.org/wiki/ConfigConsolidationPlan
>>>> [2]
>>>> https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-June/099866.html
>>>
>>> These patches look a good move, though I think the config registry idea is
>>> bloated a bit. Suppose we have tons of config items, I don't think it's good
>>> idea to register them per item.
>>
>> I'm not sure what is your concerns here can you elaborate? We have about
>> 200 known config option in core. That seems a manageable number of object.
>
> Sorry, I just felt it's overengineered. I don't have no particular concern
> about speed.
>
>> As centralized config growth feature it will needs one item for each
>> config option anyway (alias, default, documentation, etc...).
>> Do you have an alternative idea in mind?
>
> Hmm. If we want these features, perhaps configitem() function would be the
> best.

I think we want these features. David Demelier (+ at least I) want the 
alias feature for ConfigConsolidationPlan. Gregory Szorc (and others 
including me) wants the documentation features. If someone has concerns 
about these feature we should probably resolves them before pushing 
further forward.

I agree that without the future (more complex) feature, that would be a 
bit heavy (a simple list of 3 tuples would be fine) :-)

Cheers,
Augie Fackler - June 20, 2017, 1:53 a.m.
> On Jun 19, 2017, at 11:32 AM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
>>> 
>>> I'm not sure what is your concerns here can you elaborate? We have about
>>> 200 known config option in core. That seems a manageable number of object.
>> 
>> Sorry, I just felt it's overengineered. I don't have no particular concern
>> about speed.
>> 
>>> As centralized config growth feature it will needs one item for each
>>> config option anyway (alias, default, documentation, etc...).
>>> Do you have an alternative idea in mind?
>> 
>> Hmm. If we want these features, perhaps configitem() function would be the
>> best.
> 
> I think we want these features. David Demelier (+ at least I) want the alias feature for ConfigConsolidationPlan. Gregory Szorc (and others including me) wants the documentation features. If someone has concerns about these feature we should probably resolves them before pushing further forward.
> 
> I agree that without the future (more complex) feature, that would be a bit heavy (a simple list of 3 tuples would be fine) :-)

I’ve got some nits about the API as expressed in that series, but I see overall merit if it gives us a centralized way to manage config options (and more importantly documenting them in a consistent way).

I think I’d like to leave actually registering a config knob as optional for a cycle to see if it gets much adoption though - feels like a lot of churn to put people through all at once for what is (for now) unclear benefit, especially when you consider how disruptive 4.3 has already been...
Pierre-Yves David - June 20, 2017, 9:48 a.m.
On 06/20/2017 03:53 AM, Augie Fackler wrote:
>
>> On Jun 19, 2017, at 11:32 AM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>>>>
>>>> I'm not sure what is your concerns here can you elaborate? We have about
>>>> 200 known config option in core. That seems a manageable number of object.
>>>
>>> Sorry, I just felt it's overengineered. I don't have no particular concern
>>> about speed.
>>>
>>>> As centralized config growth feature it will needs one item for each
>>>> config option anyway (alias, default, documentation, etc...).
>>>> Do you have an alternative idea in mind?
>>>
>>> Hmm. If we want these features, perhaps configitem() function would be the
>>> best.
>>
>> I think we want these features. David Demelier (+ at least I) want the alias feature for ConfigConsolidationPlan. Gregory Szorc (and others including me) wants the documentation features. If someone has concerns about these feature we should probably resolves them before pushing further forward.
>>
>> I agree that without the future (more complex) feature, that would be a bit heavy (a simple list of 3 tuples would be fine) :-)
>
> I’ve got some nits about the API as expressed in that series, but I see overall merit if it gives us a centralized way to manage config options (and more importantly documenting them in a consistent way).
>
> I think I’d like to leave actually registering a config knob as optional for a cycle to see if it gets much adoption though - feels like a lot of churn to put people through all at once for what is (for now) unclear benefit, especially when you consider how disruptive 4.3 has already been...

I agree here, I'm not planning to make registration mandatory in 4.3. In 
all cases we'll start with at least one cycle of devel-warning about 
unregistered option and we might want to keep unregistered option 
supported for a long while (with a devel warning).

A key feature to push code toward config registration will be the 
documentation management.

Cheers,

Patch

diff --git a/mercurial/help/config.txt b/mercurial/help/config.txt
--- a/mercurial/help/config.txt
+++ b/mercurial/help/config.txt
@@ -2071,6 +2071,15 @@  User interface controls.
     on all exceptions, even those recognized by Mercurial (such as
     IOError or MemoryError). (default: False)
 
+``tweakdefaults``
+
+    By default Mercurial's behavior changes very little from release
+    to release, but over time the recommended config settings
+    shift. Enable this config to opt in to get automatic tweaks to
+    Mercurial's behavior over time. This config setting will have no
+    effet if ``HGPLAIN` is set or ``HGPLAINEXCEPT`` is set and does
+    not include ``tweakdefaults``. (default: False)
+
 ``username``
     The committer of a changeset created when running "commit".
     Typically a person's name and email address, e.g. ``Fred Widget
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -43,6 +43,20 @@  urlreq = util.urlreq
 _keepalnum = ''.join(c for c in map(pycompat.bytechr, range(256))
                      if not c.isalnum())
 
+# The config knobs that will be altered (if unset) by ui.tweakdefaults.
+tweakrc = """
+[ui]
+# The rollback command is dangerous. As a rule, don't use it.
+rollback = False
+
+[commands]
+# Make `hg status` emit cwd-relative paths by default.
+status.relative = yes
+
+[diff]
+git = 1
+"""
+
 samplehgrcs = {
     'user':
 """# example user config (see 'hg help config' for more info)
@@ -182,6 +196,7 @@  class ui(object):
             self.fin = src.fin
             self.pageractive = src.pageractive
             self._disablepager = src._disablepager
+            self._tweaked = src._tweaked
 
             self._tcfg = src._tcfg.copy()
             self._ucfg = src._ucfg.copy()
@@ -205,6 +220,7 @@  class ui(object):
             self.fin = util.stdin
             self.pageractive = False
             self._disablepager = False
+            self._tweaked = False
 
             # shared read-only environment
             self.environ = encoding.environ
@@ -241,8 +257,29 @@  class ui(object):
                     u.fixconfig(section=section)
             else:
                 raise error.ProgrammingError('unknown rctype: %s' % t)
+        u._maybetweakdefaults()
         return u
 
+    def _maybetweakdefaults(self):
+        if not self.configbool('ui', 'tweakdefaults'):
+            return
+        if self._tweaked or self.plain('tweakdefaults'):
+            return
+
+        # Note: it is SUPER IMPORTANT that you set self._tweaked to
+        # True *before* any calls to setconfig(), otherwise you'll get
+        # infinite recursion between setconfig and this method.
+        #
+        # TODO: We should extract an inner method in setconfig() to
+        # avoid this weirdness.
+        self._tweaked = True
+        tmpcfg = config.config()
+        tmpcfg.parse('<tweakdefaults>', tweakrc)
+        for section in tmpcfg:
+            for name, value in tmpcfg.items(section):
+                if not self.hasconfig(section, name):
+                    self.setconfig(section, name, value, "<tweakdefaults>")
+
     def copy(self):
         return self.__class__(self)
 
@@ -387,6 +424,7 @@  class ui(object):
         for cfg in (self._ocfg, self._tcfg, self._ucfg):
             cfg.set(section, name, value, source)
         self.fixconfig(section=section)
+        self._maybetweakdefaults()
 
     def _data(self, untrusted):
         return untrusted and self._ucfg or self._tcfg
diff --git a/tests/test-status.t b/tests/test-status.t
--- a/tests/test-status.t
+++ b/tests/test-status.t
@@ -107,6 +107,29 @@  combining patterns with root and pattern
   ? a/in_a
   ? b/in_b
 
+tweaking defaults works
+  $ hg status --cwd a --config ui.tweakdefaults=yes
+  ? 1/in_a_1
+  ? in_a
+  ? ../b/1/in_b_1
+  ? ../b/2/in_b_2
+  ? ../b/in_b
+  ? ../in_root
+  $ HGPLAIN=1 hg status --cwd a --config ui.tweakdefaults=yes
+  ? a/1/in_a_1 (glob)
+  ? a/in_a (glob)
+  ? b/1/in_b_1 (glob)
+  ? b/2/in_b_2 (glob)
+  ? b/in_b (glob)
+  ? in_root
+  $ HGPLAINEXCEPT=tweakdefaults hg status --cwd a --config ui.tweakdefaults=yes
+  ? 1/in_a_1
+  ? in_a
+  ? ../b/1/in_b_1
+  ? ../b/2/in_b_2
+  ? ../b/in_b
+  ? ../in_root
+
 relative paths can be requested
 
   $ cat >> $HGRCPATH <<EOF
@@ -128,6 +151,19 @@  relative paths can be requested
   ? b/in_b (glob)
   ? in_root
 
+if relative paths are explicitly off, tweakdefaults doesn't change it
+  $ cat >> $HGRCPATH <<EOF
+  > [commands]
+  > status.relative = False
+  > EOF
+  $ hg status --cwd a --config ui.tweakdefaults=yes
+  ? a/1/in_a_1 (glob)
+  ? a/in_a (glob)
+  ? b/1/in_b_1 (glob)
+  ? b/2/in_b_2 (glob)
+  ? b/in_b (glob)
+  ? in_root
+
   $ cd ..
 
   $ hg init repo2