Patchwork D6712: config: fix defaultvalue template keyword

login
register
mail settings
Submitter phabricator
Date Aug. 3, 2019, 6:52 a.m.
Message ID <differential-rev-PHID-DREV-yulrewino3hzrl4l4iam-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/41137/
State Superseded
Headers show

Comments

phabricator - Aug. 3, 2019, 6:52 a.m.
navaneeth.suresh created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This is a follow-up patch to 51a2e3102db2 <https://phab.mercurial-scm.org/rHG51a2e3102db2c24fffe6ee5594cca9189b6b0e4b>. This does three things.
  
  - Shows a warning on `configitems.dynamicdefault`.
  - Removes `pycompat.bytestr` to preserve `None`.
  - Moves `pycompat.bytestr` out of the if loop.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6712

AFFECTED FILES
  mercurial/commands.py
  mercurial/ui.py
  tests/test-config.t

CHANGE DETAILS




To: navaneeth.suresh, #hg-reviewers
Cc: mercurial-devel
Pierre-Yves David - Aug. 3, 2019, 11:38 a.m.
On 8/3/19 8:52 AM, navaneeth.suresh (Navaneeth Suresh) wrote:
> navaneeth.suresh created this revision.
> Herald added a subscriber: mercurial-devel.
> Herald added a reviewer: hg-reviewers.
> 
> REVISION SUMMARY
>    This is a follow-up patch to 51a2e3102db2 <https://phab.mercurial-scm.org/rHG51a2e3102db2c24fffe6ee5594cca9189b6b0e4b>. This does three things.

If this does three thing, it should probably be three patch.

>    - Shows a warning on `configitems.dynamicdefault`.

Why are we adding a warning ? The API usage seems valid.

>    - Removes `pycompat.bytestr` to preserve `None`.
>    - Moves `pycompat.bytestr` out of the if loop.
> 
> REPOSITORY
>    rHG Mercurial
> 
> REVISION DETAIL
>    https://phab.mercurial-scm.org/D6712
> 
> AFFECTED FILES
>    mercurial/commands.py
>    mercurial/ui.py
>    tests/test-config.t
> 
> CHANGE DETAILS
> 
> diff --git a/tests/test-config.t b/tests/test-config.t
> --- a/tests/test-config.t
> +++ b/tests/test-config.t
> @@ -57,11 +57,13 @@
>     $ hg showconfig Section -Tjson
>     [
>      {
> +    "defaultvalue": null,
>       "name": "Section.KeY",
>       "source": "*.hgrc:*", (glob)
>       "value": "Case Sensitive"
>      },
>      {
> +    "defaultvalue": null,
>       "name": "Section.key",
>       "source": "*.hgrc:*", (glob)
>       "value": "lower case"
> @@ -70,15 +72,15 @@
>     $ hg showconfig Section.KeY -Tjson
>     [
>      {
> -    "defaultvalue": "None",
> +    "defaultvalue": null,
>       "name": "Section.KeY",
>       "source": "*.hgrc:*", (glob)
>       "value": "Case Sensitive"
>      }
>     ]
>     $ hg showconfig -Tjson | tail -7
> -   },
>      {
> +    "defaultvalue": null,
>       "name": "*", (glob)
>       "source": "*", (glob)
>       "value": "*" (glob)
> @@ -103,7 +105,7 @@
>     $ hg config empty.source -Tjson
>     [
>      {
> -    "defaultvalue": "None",
> +    "defaultvalue": null,
>       "name": "empty.source",
>       "source": "",
>       "value": "value"
> @@ -170,15 +172,19 @@
>   config affected by environment variables
>   
>     $ EDITOR=e1 VISUAL=e2 hg config --debug | grep 'ui\.editor'
> +  config item requires an explicit default value: 'ui.editor'
>     $VISUAL: ui.editor=e2
>   
>     $ VISUAL=e2 hg config --debug --config ui.editor=e3 | grep 'ui\.editor'
> +  config item requires an explicit default value: 'ui.editor'
>     --config: ui.editor=e3
>   
>     $ PAGER=p1 hg config --debug | grep 'pager\.pager'
> +  config item requires an explicit default value: 'pager.pager'
>     $PAGER: pager.pager=p1
>   
>     $ PAGER=p1 hg config --debug --config pager.pager=p2 | grep 'pager\.pager'
> +  config item requires an explicit default value: 'pager.pager'
>     --config: pager.pager=p2
>   
>   verify that aliases are evaluated as well
> diff --git a/mercurial/ui.py b/mercurial/ui.py
> --- a/mercurial/ui.py
> +++ b/mercurial/ui.py
> @@ -792,7 +792,10 @@
>                   itemdefault = item.default()
>               else:
>                   itemdefault = item.default
> -        return itemdefault
> +            if item.default is configitems.dynamicdefault:
> +                self.warn(_("config item requires an explicit default value: "
> +                            "'%s.%s'\n" % (section, name)))
> +            return itemdefault
>   
>       def hasconfig(self, section, name, untrusted=False):
>           return self._data(untrusted).hasitem(section, name)
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -1872,7 +1872,7 @@
>       for section, name, value in ui.walkconfig(untrusted=untrusted):
>           source = ui.configsource(section, name, untrusted)
>           value = pycompat.bytestr(value)
> -        defaultvalue = pycompat.bytestr(ui.configdefault(section, name))
> +        defaultvalue = ui.configdefault(section, name)
>           if fm.isplain():
>               source = source or 'none'
>               value = value.replace('\n', '\\n')
> @@ -1881,8 +1881,8 @@
>               continue
>           fm.startitem()
>           fm.condwrite(ui.debugflag, 'source', '%s: ', source)
> +        fm.data(name=entryname, defaultvalue=defaultvalue)
>           if uniquesel:
> -            fm.data(name=entryname, defaultvalue=defaultvalue)
>               fm.write('value', '%s\n', value)
>           else:
>               fm.write('name value', '%s=%s\n', entryname, value)
> 
> 
> 
> To: navaneeth.suresh, #hg-reviewers
> Cc: mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
phabricator - Aug. 7, 2019, 8:20 a.m.
marmoute added a comment.


  For some reasons, my previous comment seems to have never made it to phabricator:
  
  You description says the changesets does three things. So it should be three different changesets. Can you split them ?
  
  I don't understand why we are adding a warning here. The API usage seems valid.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6712/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6712

To: navaneeth.suresh, #hg-reviewers
Cc: marmoute, mercurial-devel
phabricator - Aug. 7, 2019, 12:09 p.m.
durin42 added a comment.
durin42 resigned from this revision.


  I agree with marmoute on all points.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6712/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6712

To: navaneeth.suresh, #hg-reviewers
Cc: durin42, marmoute, mercurial-devel
phabricator - Aug. 7, 2019, 2:19 p.m.
pulkit added a comment.


  The `pycompat.bytestr()` was there for py3 compatibility, however removing it should be fine in this case. Can you make sure `test-config.t` passes with this patch on Python 3?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6712/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6712

To: navaneeth.suresh, #hg-reviewers
Cc: pulkit, durin42, marmoute, mercurial-devel
phabricator - Aug. 7, 2019, 2:24 p.m.
pulkit added a comment.


  This patch needs a better description.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6712/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6712

To: navaneeth.suresh, #hg-reviewers
Cc: pulkit, durin42, marmoute, mercurial-devel
phabricator - Aug. 7, 2019, 3:20 p.m.
navaneeth.suresh added a comment.


  In D6712#98424 <https://phab.mercurial-scm.org/D6712#98424>, @pulkit wrote:
  
  > The `pycompat.bytestr()` was there for py3 compatibility, however removing it should be fine in this case. Can you make sure `test-config.t` passes with this patch on Python 3?
  
  Yes, `test-config.t` passes with this patch on py3.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6712/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6712

To: navaneeth.suresh, #hg-reviewers
Cc: pulkit, durin42, marmoute, mercurial-devel
Yuya Nishihara - Aug. 7, 2019, 11:45 p.m.
> The `pycompat.bytestr()` was there for py3 compatibility,

Not really for the "value" variable which may be unprintable object, but
I think it's okay to assume defaultvalues are None/bool/int/float/bytes type.
phabricator - Aug. 7, 2019, 11:46 p.m.
yuja added a comment.


  > The `pycompat.bytestr()` was there for py3 compatibility,
  
  Not really for the "value" variable which may be unprintable object, but
  I think it's okay to assume defaultvalues are None/bool/int/float/bytes type.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6712/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6712

To: navaneeth.suresh, #hg-reviewers, pulkit
Cc: yuja, pulkit, durin42, marmoute, mercurial-devel

Patch

diff --git a/tests/test-config.t b/tests/test-config.t
--- a/tests/test-config.t
+++ b/tests/test-config.t
@@ -57,11 +57,13 @@ 
   $ hg showconfig Section -Tjson
   [
    {
+    "defaultvalue": null,
     "name": "Section.KeY",
     "source": "*.hgrc:*", (glob)
     "value": "Case Sensitive"
    },
    {
+    "defaultvalue": null,
     "name": "Section.key",
     "source": "*.hgrc:*", (glob)
     "value": "lower case"
@@ -70,15 +72,15 @@ 
   $ hg showconfig Section.KeY -Tjson
   [
    {
-    "defaultvalue": "None",
+    "defaultvalue": null,
     "name": "Section.KeY",
     "source": "*.hgrc:*", (glob)
     "value": "Case Sensitive"
    }
   ]
   $ hg showconfig -Tjson | tail -7
-   },
    {
+    "defaultvalue": null,
     "name": "*", (glob)
     "source": "*", (glob)
     "value": "*" (glob)
@@ -103,7 +105,7 @@ 
   $ hg config empty.source -Tjson
   [
    {
-    "defaultvalue": "None",
+    "defaultvalue": null,
     "name": "empty.source",
     "source": "",
     "value": "value"
@@ -170,15 +172,19 @@ 
 config affected by environment variables
 
   $ EDITOR=e1 VISUAL=e2 hg config --debug | grep 'ui\.editor'
+  config item requires an explicit default value: 'ui.editor'
   $VISUAL: ui.editor=e2
 
   $ VISUAL=e2 hg config --debug --config ui.editor=e3 | grep 'ui\.editor'
+  config item requires an explicit default value: 'ui.editor'
   --config: ui.editor=e3
 
   $ PAGER=p1 hg config --debug | grep 'pager\.pager'
+  config item requires an explicit default value: 'pager.pager'
   $PAGER: pager.pager=p1
 
   $ PAGER=p1 hg config --debug --config pager.pager=p2 | grep 'pager\.pager'
+  config item requires an explicit default value: 'pager.pager'
   --config: pager.pager=p2
 
 verify that aliases are evaluated as well
diff --git a/mercurial/ui.py b/mercurial/ui.py
--- a/mercurial/ui.py
+++ b/mercurial/ui.py
@@ -792,7 +792,10 @@ 
                 itemdefault = item.default()
             else:
                 itemdefault = item.default
-        return itemdefault
+            if item.default is configitems.dynamicdefault:
+                self.warn(_("config item requires an explicit default value: "
+                            "'%s.%s'\n" % (section, name)))
+            return itemdefault
 
     def hasconfig(self, section, name, untrusted=False):
         return self._data(untrusted).hasitem(section, name)
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -1872,7 +1872,7 @@ 
     for section, name, value in ui.walkconfig(untrusted=untrusted):
         source = ui.configsource(section, name, untrusted)
         value = pycompat.bytestr(value)
-        defaultvalue = pycompat.bytestr(ui.configdefault(section, name))
+        defaultvalue = ui.configdefault(section, name)
         if fm.isplain():
             source = source or 'none'
             value = value.replace('\n', '\\n')
@@ -1881,8 +1881,8 @@ 
             continue
         fm.startitem()
         fm.condwrite(ui.debugflag, 'source', '%s: ', source)
+        fm.data(name=entryname, defaultvalue=defaultvalue)
         if uniquesel:
-            fm.data(name=entryname, defaultvalue=defaultvalue)
             fm.write('value', '%s\n', value)
         else:
             fm.write('name value', '%s=%s\n', entryname, value)