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

login
register
mail settings
Submitter Gregory Szorc
Date March 13, 2017, 4:57 a.m.
Message ID <265102f455de6451e493.1489381053@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/19268/
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 1489380642 25200
#      Sun Mar 12 21:50:42 2017 -0700
# Node ID 265102f455de6451e493024fb8a9f24d816ce1c2
# Parent  1c48a8278b2f015fca607dfc652823560a5ac580
context: don't use mutable default argument value

Mutable default argument values are a Python gotcha and can
represent subtle, hard-to-find bugs. Lets rid our code base
of them.
Gregory Szorc - March 13, 2017, 5:02 a.m.
On Sun, Mar 12, 2017 at 9:57 PM, Gregory Szorc <gregory.szorc@gmail.com>
wrote:

> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1489380642 25200
> #      Sun Mar 12 21:50:42 2017 -0700
> # Node ID 265102f455de6451e493024fb8a9f24d816ce1c2
> # Parent  1c48a8278b2f015fca607dfc652823560a5ac580
> context: don't use mutable default argument value
>
> Mutable default argument values are a Python gotcha and can
> represent subtle, hard-to-find bugs. Lets rid our code base
> of them.
>

There was bikeshedding the last time I sent this series. Most of it was
over the code to enforce this with an automatic checker. Since the existing
code represents bugs, I've decided to drop that patch and just fix the code.

The previous send also involved some concerns around how to do this and
whether "foo or []" is the right pattern (versus say a dummy object you
could "is" compare against). Martijn uses the "foo or []" pattern in
d30fb3de4b40 and he knows more about Python than practically anyone. So
I'll use that to justify my opinion that "foo or []" is acceptable.


>
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -298,10 +298,10 @@ class basectx(object):
>          '''
>          return subrepo.subrepo(self, path, allowwdir=True)
>
> -    def match(self, pats=[], include=None, exclude=None, default='glob',
> +    def match(self, pats=None, include=None, exclude=None, default='glob',
>                listsubrepos=False, badfn=None):
>          r = self._repo
> -        return matchmod.match(r.root, r.getcwd(), pats,
> +        return matchmod.match(r.root, r.getcwd(), pats or [],
>                                include, exclude, default,
>                                auditor=r.nofsauditor, ctx=self,
>                                listsubrepos=listsubrepos, badfn=badfn)
> @@ -1515,16 +1515,16 @@ class workingctx(committablectx):
>                      self._repo.dirstate.normallookup(dest)
>                  self._repo.dirstate.copy(source, dest)
>
> -    def match(self, pats=[], include=None, exclude=None, default='glob',
> +    def match(self, pats=None, include=None, exclude=None, default='glob',
>                listsubrepos=False, badfn=None):
>          r = self._repo
>
>          # Only a case insensitive filesystem needs magic to translate
> user input
>          # to actual case in the filesystem.
>          if not util.fscasesensitive(r.root):
> -            return matchmod.icasefsmatcher(r.root, r.getcwd(), pats,
> include,
> -                                           exclude, default, r.auditor,
> self,
> -                                           listsubrepos=listsubrepos,
> +            return matchmod.icasefsmatcher(r.root, r.getcwd(), pats or
> [],
> +                                           include, exclude, default,
> r.auditor,
> +                                           self,
> listsubrepos=listsubrepos,
>                                             badfn=badfn)
>          return matchmod.match(r.root, r.getcwd(), pats,
>                                include, exclude, default,
>
Yuya Nishihara - March 14, 2017, 2:34 a.m.
On Sun, 12 Mar 2017 21:57:33 -0700, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1489380642 25200
> #      Sun Mar 12 21:50:42 2017 -0700
> # Node ID 265102f455de6451e493024fb8a9f24d816ce1c2
> # Parent  1c48a8278b2f015fca607dfc652823560a5ac580
> context: don't use mutable default argument value
> 
> Mutable default argument values are a Python gotcha and can
> represent subtle, hard-to-find bugs. Lets rid our code base
> of them.
> 
> diff --git a/mercurial/context.py b/mercurial/context.py
> --- a/mercurial/context.py
> +++ b/mercurial/context.py
> @@ -298,10 +298,10 @@ class basectx(object):
>          '''
>          return subrepo.subrepo(self, path, allowwdir=True)
>  
> -    def match(self, pats=[], include=None, exclude=None, default='glob',
> +    def match(self, pats=None, include=None, exclude=None, default='glob',
>                listsubrepos=False, badfn=None):
>          r = self._repo
> -        return matchmod.match(r.root, r.getcwd(), pats,
> +        return matchmod.match(r.root, r.getcwd(), pats or [],
>                                include, exclude, default,
>                                auditor=r.nofsauditor, ctx=self,
>                                listsubrepos=listsubrepos, badfn=badfn)
> @@ -1515,16 +1515,16 @@ class workingctx(committablectx):
>                      self._repo.dirstate.normallookup(dest)
>                  self._repo.dirstate.copy(source, dest)
>  
> -    def match(self, pats=[], include=None, exclude=None, default='glob',
> +    def match(self, pats=None, include=None, exclude=None, default='glob',
>                listsubrepos=False, badfn=None):
>          r = self._repo
>  
>          # Only a case insensitive filesystem needs magic to translate user input
>          # to actual case in the filesystem.
>          if not util.fscasesensitive(r.root):
> -            return matchmod.icasefsmatcher(r.root, r.getcwd(), pats, include,
> -                                           exclude, default, r.auditor, self,
> -                                           listsubrepos=listsubrepos,
> +            return matchmod.icasefsmatcher(r.root, r.getcwd(), pats or [],
> +                                           include, exclude, default, r.auditor,
> +                                           self, listsubrepos=listsubrepos,
>                                             badfn=badfn)
>          return matchmod.match(r.root, r.getcwd(), pats,
                                                     ^^^^

Missed 'pats or []'. Fixed in-flight and queued the series, thanks.

Patch

diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -298,10 +298,10 @@  class basectx(object):
         '''
         return subrepo.subrepo(self, path, allowwdir=True)
 
-    def match(self, pats=[], include=None, exclude=None, default='glob',
+    def match(self, pats=None, include=None, exclude=None, default='glob',
               listsubrepos=False, badfn=None):
         r = self._repo
-        return matchmod.match(r.root, r.getcwd(), pats,
+        return matchmod.match(r.root, r.getcwd(), pats or [],
                               include, exclude, default,
                               auditor=r.nofsauditor, ctx=self,
                               listsubrepos=listsubrepos, badfn=badfn)
@@ -1515,16 +1515,16 @@  class workingctx(committablectx):
                     self._repo.dirstate.normallookup(dest)
                 self._repo.dirstate.copy(source, dest)
 
-    def match(self, pats=[], include=None, exclude=None, default='glob',
+    def match(self, pats=None, include=None, exclude=None, default='glob',
               listsubrepos=False, badfn=None):
         r = self._repo
 
         # Only a case insensitive filesystem needs magic to translate user input
         # to actual case in the filesystem.
         if not util.fscasesensitive(r.root):
-            return matchmod.icasefsmatcher(r.root, r.getcwd(), pats, include,
-                                           exclude, default, r.auditor, self,
-                                           listsubrepos=listsubrepos,
+            return matchmod.icasefsmatcher(r.root, r.getcwd(), pats or [],
+                                           include, exclude, default, r.auditor,
+                                           self, listsubrepos=listsubrepos,
                                            badfn=badfn)
         return matchmod.match(r.root, r.getcwd(), pats,
                               include, exclude, default,