Fix SimpleConfig

SimpleConfig claims to handle configuration options in priority
command line, user config, system config, which makes sense.
In fact it appears it used priority command line, system config,
user config.
Also, from the priority ordering, it would seem correct that a
value should be unmodifiable if and only if it's set on the command
line.  Previously anything in the system config would be unmodifiable.

This patch fixes these and cleans the code up a bit.  I noticed this
whilst attempting to unify the 'auto_cycle' setting.

Fixup tests accordingly.

lib/simple_config.py

@@ -3,6 +3,7 @@ import json
 import threading
 import os
 
+from copy import deepcopy
 from util import user_dir, print_error, print_msg
 
 SYSTEM_CONFIG_PATH = "/etc/electrum.conf"
@@ -32,19 +33,12 @@ class SimpleConfig(object):
     They are taken in order (1. overrides config options set in 2., that
     override config set in 3.)
     """
-    def __init__(self, options=None, read_system_config_function=None,
+    def __init__(self, options={}, read_system_config_function=None,
                  read_user_config_function=None, read_user_dir_function=None):
 
-        # This is the holder of actual options for the current user.
-        self.read_only_options = {}
         # This lock needs to be acquired for updating and reading the config in
         # a thread-safe way.
         self.lock = threading.RLock()
-        # The path for the config directory. This is set later by init_path()
-        self.path = None
-
-        if options is None:
-            options = {}  # Having a mutable as a default value is a bad idea.
 
         # The following two functions are there for dependency injection when
         # testing.
@@ -57,45 +51,39 @@ class SimpleConfig(object):
         else:
             self.user_dir = read_user_dir_function
 
-        # Save the command-line keys to make sure we don't override them.
-        self.command_line_keys = options.keys()
-        # Save the system config keys to make sure we don't override them.
-        self.system_config_keys = []
+        # The command line options
+        self.cmdline_options = deepcopy(options)
 
-        if options.get('portable') is not True:
-            # system conf
-            system_config = read_system_config_function()
-            self.system_config_keys = system_config.keys()
-            self.read_only_options.update(system_config)
+        # Portable wallets don't use a system config
+        if self.cmdline_options.get('portable', False):
+            self.system_config = read_system_config_function()
+        else:
+            self.system_config = {}
 
-        # update the current options with the command line options last (to
-        # override both others).
-        self.read_only_options.update(options)
-        # init path
-        self.init_path()
-        # user config.
+        # Set self.path and read the user config
+        self.user_config = {}  # for self.get in electrum_path()
+        self.path = self.electrum_path()
         self.user_config = read_user_config_function(self.path)
         # Make a singleton instance of 'self'
         set_config(self)
 
-    def init_path(self):
-        # Read electrum path in the command line configuration
-        self.path = self.read_only_options.get('electrum_path')
-
-        # If not set, use the user's default data directory.
-        if self.path is None:
-            self.path = self.user_dir()
+    def electrum_path(self):
+        # Read electrum_path from command line / system configuration
+        # Otherwise use the user's default data directory.
+        path = self.get('electrum_path')
+        if path is None:
+            path = self.user_dir()
 
         # Make directory if it does not yet exist.
-        if not os.path.exists(self.path):
-            os.mkdir(self.path)
+        if not os.path.exists(path):
+            os.mkdir(path)
 
-        print_error( "electrum directory", self.path)
+        print_error("electrum directory", path)
+        return path
 
     def set_key(self, key, value, save = True):
         if not self.is_modifiable(key):
-            print "Warning: not changing key '%s' because it is not modifiable" \
-                  " (passed as command line option or defined in /etc/electrum.conf)"%key
+            print_error("Warning: not changing config key '%s' set on the command line" % key)
             return
 
         with self.lock:
@@ -105,19 +93,16 @@ class SimpleConfig(object):
         return
 
     def get(self, key, default=None):
-        out = None
         with self.lock:
-            out = self.read_only_options.get(key)
+            out = self.cmdline_options.get(key)
             if out is None:
-                out = self.user_config.get(key, default)
+                out = self.user_config.get(key)
+                if out is None:
+                    out = self.system_config.get(key, default)
         return out
 
     def is_modifiable(self, key):
-        if key in self.command_line_keys:
-            return False
-        if key in self.system_config_keys:
-            return False
-        return True
+        return not key in self.cmdline_options
 
     def save_user_config(self):
         if not self.path:

lib/tests/test_simple_config.py

@@ -48,17 +48,16 @@ class Test_SimpleConfig(unittest.TestCase):
         self.assertEqual(self.options.get("electrum_path"),
                          config.get("electrum_path"))
 
-    def test_simple_config_system_config_overrides_user_config(self):
-        """Options passed in system config override user config."""
+    def test_simple_config_user_config_overrides_system_config(self):
+        """Options passed in user config override system config."""
         fake_read_system = lambda : {"electrum_path": self.electrum_dir}
         fake_read_user = lambda _: {"electrum_path": "b"}
         read_user_dir = lambda : self.user_dir
-        config = SimpleConfig(options=None,
+        config = SimpleConfig(options={},
                               read_system_config_function=fake_read_system,
                               read_user_config_function=fake_read_user,
                               read_user_dir_function=read_user_dir)
-        self.assertEqual(self.options.get("electrum_path"),
-                         config.get("electrum_path"))
+        self.assertEqual("b", config.get("electrum_path"))
 
     def test_simple_config_system_config_ignored_if_portable(self):
         """If electrum is started with the "portable" flag, system
@@ -79,7 +78,7 @@ class Test_SimpleConfig(unittest.TestCase):
         fake_read_system = lambda : {}
         fake_read_user = lambda _: {"electrum_path": self.electrum_dir}
         read_user_dir = lambda : self.user_dir
-        config = SimpleConfig(options=None,
+        config = SimpleConfig(options={},
                               read_system_config_function=fake_read_system,
                               read_user_config_function=fake_read_user,
                               read_user_dir_function=read_user_dir)
@@ -98,7 +97,7 @@ class Test_SimpleConfig(unittest.TestCase):
         self.assertEqual(self.options.get("electrum_path"),
                          config.get("electrum_path"))
 
-    def test_cannot_set_options_from_system_config(self):
+    def test_can_set_options_from_system_config(self):
         fake_read_system = lambda : {"electrum_path": self.electrum_dir}
         fake_read_user = lambda _: {}
         read_user_dir = lambda : self.user_dir
@@ -107,8 +106,7 @@ class Test_SimpleConfig(unittest.TestCase):
                               read_user_config_function=fake_read_user,
                               read_user_dir_function=read_user_dir)
         config.set_key("electrum_path", "c")
-        self.assertEqual(self.options.get("electrum_path"),
-                         config.get("electrum_path"))
+        self.assertEqual("c", config.get("electrum_path"))
 
     def test_can_set_options_set_in_user_config(self):
         another_path = tempfile.mkdtemp()