Patchwork [2,of,2,v2] hgweb: forward arguments to ui.config

login
register
mail settings
Submitter David Demelier
Date June 28, 2017, 11:42 a.m.
Message ID <ff76472b66ad8b748876.1498650134@fedy>
Download mbox | patch
Permalink /patch/21796/
State Superseded
Headers show

Comments

David Demelier - June 28, 2017, 11:42 a.m.
# HG changeset patch
# User David Demelier <demelier.david@gmail.com>
# Date 1498646251 -7200
#      Wed Jun 28 12:37:31 2017 +0200
# Node ID ff76472b66ad8b74887674887654bdffabe41b78
# Parent  9a80ff1ee41ce0c87cf1f7f5219b6337f3c04941
hgweb: forward arguments to ui.config
David Demelier - June 28, 2017, 11:43 a.m.
This one should be applied first, it prevents warnings in the earlier one.
Jun Wu - June 30, 2017, 4:42 a.m.
I might miss something, but why is this change necessary? I slightly prefer
the old code where arguments are more explicit, and people can override
untrusted to False.

Excerpts from 's message of 2017-06-28 13:42:14 +0200:
> # HG changeset patch
> # User David Demelier <demelier.david at gmail.com>
> # Date 1498646251 -7200
> #      Wed Jun 28 12:37:31 2017 +0200
> # Node ID ff76472b66ad8b74887674887654bdffabe41b78
> # Parent  9a80ff1ee41ce0c87cf1f7f5219b6337f3c04941
> hgweb: forward arguments to ui.config
> 
> diff -r 9a80ff1ee41c -r ff76472b66ad mercurial/hgweb/hgweb_mod.py
> --- a/mercurial/hgweb/hgweb_mod.py    Wed Jun 28 12:44:36 2017 +0200
> +++ b/mercurial/hgweb/hgweb_mod.py    Wed Jun 28 12:37:31 2017 +0200
> @@ -119,21 +119,21 @@
>          self.csp, self.nonce = cspvalues(self.repo.ui)
>  
>      # Trust the settings from the .hg/hgrc files by default.
> -    def config(self, section, name, default=None, untrusted=True):
> -        return self.repo.ui.config(section, name, default,
> -                                   untrusted=untrusted)
> +    def config(self, *args, **kwargs):
> +        kwargs['untrusted'] = True
> +        return self.repo.ui.config(*args, **kwargs)
>  
> -    def configbool(self, section, name, default=False, untrusted=True):
> -        return self.repo.ui.configbool(section, name, default,
> -                                       untrusted=untrusted)
> +    def configbool(self, *args, **kwargs):
> +        kwargs['untrusted'] = True
> +        return self.repo.ui.configbool(*args, **kwargs)
>  
> -    def configint(self, section, name, default=None, untrusted=True):
> -        return self.repo.ui.configint(section, name, default,
> -                                      untrusted=untrusted)
> +    def configint(self, *args, **kwargs):
> +        kwargs['untrusted'] = True
> +        return self.repo.ui.configint(*args, **kwargs)
>  
> -    def configlist(self, section, name, default=None, untrusted=True):
> -        return self.repo.ui.configlist(section, name, default,
> -                                       untrusted=untrusted)
> +    def configlist(self, *args, **kwargs):
> +        kwargs['untrusted'] = True
> +        return self.repo.ui.configlist(*args, **kwargs)
>  
>      def archivelist(self, nodeid):
>          allowed = self.configlist('web', 'allow_archive')
> diff -r 9a80ff1ee41c -r ff76472b66ad mercurial/hgweb/hgwebdir_mod.py
> --- a/mercurial/hgweb/hgwebdir_mod.py    Wed Jun 28 12:44:36 2017 +0200
> +++ b/mercurial/hgweb/hgwebdir_mod.py    Wed Jun 28 12:37:31 2017 +0200
> @@ -404,9 +404,9 @@
>                  except Exception as e:
>                      u.warn(_('error reading %s/.hg/hgrc: %s\n') % (path, e))
>                      continue
> -                def get(section, name, default=None):
> -                    return u.config(section, name, default, untrusted=True)
> -
> +                def get(*args, **kwargs):
> +                    kwargs['untrusted'] = True
> +                    return u.config(*args, **kwargs)
>                  if u.configbool("web", "hidden", untrusted=True):
>                      continue
>
Augie Fackler - June 30, 2017, 1:55 p.m.
On Wed, Jun 28, 2017 at 01:42:14PM +0200, David Demelier wrote:
> # HG changeset patch
> # User David Demelier <demelier.david@gmail.com>
> # Date 1498646251 -7200
> #      Wed Jun 28 12:37:31 2017 +0200
> # Node ID ff76472b66ad8b74887674887654bdffabe41b78
> # Parent  9a80ff1ee41ce0c87cf1f7f5219b6337f3c04941
> hgweb: forward arguments to ui.config
>
> diff -r 9a80ff1ee41c -r ff76472b66ad mercurial/hgweb/hgweb_mod.py
> --- a/mercurial/hgweb/hgweb_mod.py	Wed Jun 28 12:44:36 2017 +0200
> +++ b/mercurial/hgweb/hgweb_mod.py	Wed Jun 28 12:37:31 2017 +0200
> @@ -119,21 +119,21 @@
>          self.csp, self.nonce = cspvalues(self.repo.ui)
>
>      # Trust the settings from the .hg/hgrc files by default.
> -    def config(self, section, name, default=None, untrusted=True):
> -        return self.repo.ui.config(section, name, default,
> -                                   untrusted=untrusted)
> +    def config(self, *args, **kwargs):
> +        kwargs['untrusted'] = True
> +        return self.repo.ui.config(*args, **kwargs)

Is it intentional that the new code prevents untrusted=False?

>
> -    def configbool(self, section, name, default=False, untrusted=True):
> -        return self.repo.ui.configbool(section, name, default,
> -                                       untrusted=untrusted)
> +    def configbool(self, *args, **kwargs):
> +        kwargs['untrusted'] = True
> +        return self.repo.ui.configbool(*args, **kwargs)
>
> -    def configint(self, section, name, default=None, untrusted=True):
> -        return self.repo.ui.configint(section, name, default,
> -                                      untrusted=untrusted)
> +    def configint(self, *args, **kwargs):
> +        kwargs['untrusted'] = True
> +        return self.repo.ui.configint(*args, **kwargs)
>
> -    def configlist(self, section, name, default=None, untrusted=True):
> -        return self.repo.ui.configlist(section, name, default,
> -                                       untrusted=untrusted)
> +    def configlist(self, *args, **kwargs):
> +        kwargs['untrusted'] = True
> +        return self.repo.ui.configlist(*args, **kwargs)
>
>      def archivelist(self, nodeid):
>          allowed = self.configlist('web', 'allow_archive')
> diff -r 9a80ff1ee41c -r ff76472b66ad mercurial/hgweb/hgwebdir_mod.py
> --- a/mercurial/hgweb/hgwebdir_mod.py	Wed Jun 28 12:44:36 2017 +0200
> +++ b/mercurial/hgweb/hgwebdir_mod.py	Wed Jun 28 12:37:31 2017 +0200
> @@ -404,9 +404,9 @@
>                  except Exception as e:
>                      u.warn(_('error reading %s/.hg/hgrc: %s\n') % (path, e))
>                      continue
> -                def get(section, name, default=None):
> -                    return u.config(section, name, default, untrusted=True)
> -
> +                def get(*args, **kwargs):
> +                    kwargs['untrusted'] = True
> +                    return u.config(*args, **kwargs)
>                  if u.configbool("web", "hidden", untrusted=True):
>                      continue
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
David Demelier - July 1, 2017, 4:03 a.m.
On Thu, 2017-06-29 at 21:42 -0700, Jun Wu wrote:
> I might miss something, but why is this change necessary? I slightly
> prefer
> the old code where arguments are more explicit, and people can
> override
> untrusted to False.

The problem is that def.config uses a local _unset variable for the
default argument.

The hgweb_mod.py uses None so you get a warning while running the tests
because of this code:

 463             msg = ("specifying a default value for a registered "
 464                    "config item: '%s.%s' '%s'")

An other solution woudl be to rename _unset to unset and export it so
that hgweb_mod.py can pass unset instead of None by default.

What do you think?
Pierre-Yves David - July 1, 2017, 3:13 p.m.
On 07/01/2017 06:03 AM, demelier.david@gmail.com wrote:
> On Thu, 2017-06-29 at 21:42 -0700, Jun Wu wrote:
>> I might miss something, but why is this change necessary? I slightly
>> prefer
>> the old code where arguments are more explicit, and people can
>> override
>> untrusted to False.
> 
> The problem is that def.config uses a local _unset variable for the
> default argument.
> 
> The hgweb_mod.py uses None so you get a warning while running the tests
> because of this code:
> 
>   463             msg = ("specifying a default value for a registered "
>   464                    "config item: '%s.%s' '%s'")
> 
> An other solution woudl be to rename _unset to unset and export it so
> that hgweb_mod.py can pass unset instead of None by default.
> 
> What do you think?

The issue with your patch is that is change the behavior from

  * untrusted default to False but another value can be passed

to

  * untrusted is always True.

You can easily fixes it by avoiding overwriting the untrusted value if 
it is passed.

The extended version of the code would be:

   if 'untrusted' not in kwargs:
       kwargs['untrusted'] = True

A short version would be:

   kwargs.setdefault('untrusted', True)

Cheers,
Yuya Nishihara - July 2, 2017, 3:13 a.m.
On Sat, 1 Jul 2017 17:13:22 +0200, Pierre-Yves David wrote:
> On 07/01/2017 06:03 AM, demelier.david@gmail.com wrote:
> > On Thu, 2017-06-29 at 21:42 -0700, Jun Wu wrote:
> >> I might miss something, but why is this change necessary? I slightly
> >> prefer
> >> the old code where arguments are more explicit, and people can
> >> override
> >> untrusted to False.
> > 
> > The problem is that def.config uses a local _unset variable for the
> > default argument.
> > 
> > The hgweb_mod.py uses None so you get a warning while running the tests
> > because of this code:
> > 
> >   463             msg = ("specifying a default value for a registered "
> >   464                    "config item: '%s.%s' '%s'")
> > 
> > An other solution woudl be to rename _unset to unset and export it so
> > that hgweb_mod.py can pass unset instead of None by default.
> > 
> > What do you think?
> 
> The issue with your patch is that is change the behavior from
> 
>   * untrusted default to False but another value can be passed
> 
> to
> 
>   * untrusted is always True.
> 
> You can easily fixes it by avoiding overwriting the untrusted value if 
> it is passed.
> 
> The extended version of the code would be:
> 
>    if 'untrusted' not in kwargs:
>        kwargs['untrusted'] = True
> 
> A short version would be:
> 
>    kwargs.setdefault('untrusted', True)

Perhaps we can just use uimod._unset until default= arguments are removed
from these functions. I generally prefer explicit list of arguments than
*args, **kwargs.

Patch

diff -r 9a80ff1ee41c -r ff76472b66ad mercurial/hgweb/hgweb_mod.py
--- a/mercurial/hgweb/hgweb_mod.py	Wed Jun 28 12:44:36 2017 +0200
+++ b/mercurial/hgweb/hgweb_mod.py	Wed Jun 28 12:37:31 2017 +0200
@@ -119,21 +119,21 @@ 
         self.csp, self.nonce = cspvalues(self.repo.ui)
 
     # Trust the settings from the .hg/hgrc files by default.
-    def config(self, section, name, default=None, untrusted=True):
-        return self.repo.ui.config(section, name, default,
-                                   untrusted=untrusted)
+    def config(self, *args, **kwargs):
+        kwargs['untrusted'] = True
+        return self.repo.ui.config(*args, **kwargs)
 
-    def configbool(self, section, name, default=False, untrusted=True):
-        return self.repo.ui.configbool(section, name, default,
-                                       untrusted=untrusted)
+    def configbool(self, *args, **kwargs):
+        kwargs['untrusted'] = True
+        return self.repo.ui.configbool(*args, **kwargs)
 
-    def configint(self, section, name, default=None, untrusted=True):
-        return self.repo.ui.configint(section, name, default,
-                                      untrusted=untrusted)
+    def configint(self, *args, **kwargs):
+        kwargs['untrusted'] = True
+        return self.repo.ui.configint(*args, **kwargs)
 
-    def configlist(self, section, name, default=None, untrusted=True):
-        return self.repo.ui.configlist(section, name, default,
-                                       untrusted=untrusted)
+    def configlist(self, *args, **kwargs):
+        kwargs['untrusted'] = True
+        return self.repo.ui.configlist(*args, **kwargs)
 
     def archivelist(self, nodeid):
         allowed = self.configlist('web', 'allow_archive')
diff -r 9a80ff1ee41c -r ff76472b66ad mercurial/hgweb/hgwebdir_mod.py
--- a/mercurial/hgweb/hgwebdir_mod.py	Wed Jun 28 12:44:36 2017 +0200
+++ b/mercurial/hgweb/hgwebdir_mod.py	Wed Jun 28 12:37:31 2017 +0200
@@ -404,9 +404,9 @@ 
                 except Exception as e:
                     u.warn(_('error reading %s/.hg/hgrc: %s\n') % (path, e))
                     continue
-                def get(section, name, default=None):
-                    return u.config(section, name, default, untrusted=True)
-
+                def get(*args, **kwargs):
+                    kwargs['untrusted'] = True
+                    return u.config(*args, **kwargs)
                 if u.configbool("web", "hidden", untrusted=True):
                     continue