Patchwork [6,of,8,V2] util: don't use mutable default argument value

login
register
mail settings
Submitter Gregory Szorc
Date March 13, 2017, 4:57 a.m.
Message ID <e379f89d119b7b1cd40c.1489381058@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/19273/
State Accepted
Headers show

Comments

Gregory Szorc - March 13, 2017, 4:57 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1489380872 25200
#      Sun Mar 12 21:54:32 2017 -0700
# Node ID e379f89d119b7b1cd40c313693912b5fdc4a3360
# Parent  f72bef9154d773c05fa8b2ae30d7db55861fa639
util: don't use mutable default argument value

I don't think this is any tight loops and we'd need to worry about
PyObject creation overhead. Also, I'm pretty sure strptime()
will be much slower than PyObject creation (date parsing is
surprisingly slow).
Yuya Nishihara - March 14, 2017, 2:36 a.m.
On Sun, 12 Mar 2017 21:57:38 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1489380872 25200
> #      Sun Mar 12 21:54:32 2017 -0700
> # Node ID e379f89d119b7b1cd40c313693912b5fdc4a3360
> # Parent  f72bef9154d773c05fa8b2ae30d7db55861fa639
> util: don't use mutable default argument value
> 
> I don't think this is any tight loops and we'd need to worry about
> PyObject creation overhead. Also, I'm pretty sure strptime()
> will be much slower than PyObject creation (date parsing is
> surprisingly slow).
> 
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -1827,9 +1827,11 @@ def parsetimezone(s):
>  
>      return None, s
>  
> -def strdate(string, format, defaults=[]):
> +def strdate(string, format, defaults=None):
>      """parse a localized time string and return a (unixtime, offset) tuple.
>      if the string cannot be parsed, ValueError is raised."""
> +    defaults = defaults or []

Actually the default appears to be a dict type. It's keyed by str.
Can you fix it as follow up?
Gregory Szorc - March 14, 2017, 3:44 p.m.
On Mon, Mar 13, 2017 at 7:36 PM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Sun, 12 Mar 2017 21:57:38 -0700, Gregory Szorc wrote:
> > # HG changeset patch
> > # User Gregory Szorc <gregory.szorc@gmail.com>
> > # Date 1489380872 25200
> > #      Sun Mar 12 21:54:32 2017 -0700
> > # Node ID e379f89d119b7b1cd40c313693912b5fdc4a3360
> > # Parent  f72bef9154d773c05fa8b2ae30d7db55861fa639
> > util: don't use mutable default argument value
> >
> > I don't think this is any tight loops and we'd need to worry about
> > PyObject creation overhead. Also, I'm pretty sure strptime()
> > will be much slower than PyObject creation (date parsing is
> > surprisingly slow).
> >
> > diff --git a/mercurial/util.py b/mercurial/util.py
> > --- a/mercurial/util.py
> > +++ b/mercurial/util.py
> > @@ -1827,9 +1827,11 @@ def parsetimezone(s):
> >
> >      return None, s
> >
> > -def strdate(string, format, defaults=[]):
> > +def strdate(string, format, defaults=None):
> >      """parse a localized time string and return a (unixtime, offset)
> tuple.
> >      if the string cannot be parsed, ValueError is raised."""
> > +    defaults = defaults or []
>
> Actually the default appears to be a dict type. It's keyed by str.
> Can you fix it as follow up?
>

Will patchbomb shortly.

On further inspection, aspects of `hg convert` may have been broken due to
this incorrect type *before my change*. I'm not sure if there is explicit
test coverage for this code path. And I don't feel like installing darcs,
monotone, or gnuarch to find out. Either way, the upcoming patch would fix
any problems.

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -1827,9 +1827,11 @@  def parsetimezone(s):
 
     return None, s
 
-def strdate(string, format, defaults=[]):
+def strdate(string, format, defaults=None):
     """parse a localized time string and return a (unixtime, offset) tuple.
     if the string cannot be parsed, ValueError is raised."""
+    defaults = defaults or []
+
     # NOTE: unixtime = localunixtime + offset
     offset, date = parsetimezone(string)