From b35a808712600c0c9d712a5d09cabddd12c6df2a Mon Sep 17 00:00:00 2001 From: Ryan Farley Date: Sun, 17 Sep 2017 13:24:05 -0500 Subject: [PATCH 1/5] test multiple mutations for config This detects the problem in #2979 --- tests/unit/config/test_configfiles.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unit/config/test_configfiles.py b/tests/unit/config/test_configfiles.py index c10b0c4b4..4be773e82 100644 --- a/tests/unit/config/test_configfiles.py +++ b/tests/unit/config/test_configfiles.py @@ -205,9 +205,11 @@ class TestConfigPy: assert config.instance._values['bindings.commands'] == expected def test_mutating(self, confpy): - confpy.write('c.aliases["foo"] = "message-info foo"') + confpy.write('c.aliases["foo"] = "message-info foo"', + 'c.aliases["bar"] = "message-info bar"') configfiles.read_config_py(confpy.filename) assert config.instance._values['aliases']['foo'] == 'message-info foo' + assert config.instance._values['aliases']['bar'] == 'message-info bar' def test_reading_default_location(self, config_tmpdir): (config_tmpdir / 'config.py').write_text( From c6ea0f837278b9bcf2d56f0e6f11b27088ec9e35 Mon Sep 17 00:00:00 2001 From: Ryan Farley Date: Sun, 17 Sep 2017 16:07:52 -0500 Subject: [PATCH 2/5] Use dictionary for configuration mutable storage Includes a test for persistence of intermediate mutations in a configuration file (i.e. more than one update) and a switch of the _mutable attribute in configurations to a dictionary of (old, new) values rather than (name, old, new). get_obj() now checks for an existing mutable value and returns a reference to that value, only making an initial copy; this preserves changes between update_mutables() --- qutebrowser/config/config.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/qutebrowser/config/config.py b/qutebrowser/config/config.py index 9907eb205..98018bbd1 100644 --- a/qutebrowser/config/config.py +++ b/qutebrowser/config/config.py @@ -379,7 +379,7 @@ class Config(QObject): super().__init__(parent) self.changed.connect(_render_stylesheet.cache_clear) self._values = {} - self._mutables = [] + self._mutables = {} self._yaml = yaml_config def _set_value(self, opt, value): @@ -426,7 +426,10 @@ class Config(QObject): # Then we watch the returned object for changes. if isinstance(obj, (dict, list)): if mutable: - self._mutables.append((name, copy.deepcopy(obj), obj)) + if name not in self._mutables: + self._mutables[name] = (copy.deepcopy(obj), obj) + else: + obj = self._mutables[name][1] else: # Shouldn't be mutable (and thus hashable) assert obj.__hash__ is not None, obj @@ -470,11 +473,13 @@ class Config(QObject): Here, we check all those saved copies for mutations, and if something mutated, we call set_obj again so we save the new value. """ - for name, old_value, new_value in self._mutables: + for name, vals in self._mutables: + old_value = vals[0] + new_value = vals[1] if old_value != new_value: log.config.debug("{} was mutated, updating".format(name)) self.set_obj(name, new_value, save_yaml=save_yaml) - self._mutables = [] + self._mutables = {} def dump_userconfig(self): """Get the part of the config which was changed by the user. From 83473b9c69ce9f01aace95d013556de58274bfb0 Mon Sep 17 00:00:00 2001 From: Ryan Farley Date: Tue, 19 Sep 2017 14:00:44 -0500 Subject: [PATCH 3/5] fix test for new tuples --- tests/unit/config/test_config.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/config/test_config.py b/tests/unit/config/test_config.py index 4e4816e7c..a726b0506 100644 --- a/tests/unit/config/test_config.py +++ b/tests/unit/config/test_config.py @@ -697,7 +697,7 @@ class TestConfig: assert obj == new if mutable: - assert conf._mutables == [(option, old, new)] + assert conf._mutables == ( old, new) if mutable and mutated: # Now let's update From a530b0cc954d9d15db09eaebaae25c234164a2c0 Mon Sep 17 00:00:00 2001 From: Ryan Farley Date: Tue, 19 Sep 2017 14:19:28 -0500 Subject: [PATCH 4/5] fixed iteration --- qutebrowser/config/config.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/qutebrowser/config/config.py b/qutebrowser/config/config.py index 9899d673a..88312facd 100644 --- a/qutebrowser/config/config.py +++ b/qutebrowser/config/config.py @@ -483,9 +483,9 @@ class Config(QObject): Here, we check all those saved copies for mutations, and if something mutated, we call set_obj again so we save the new value. """ - for name, vals in self._mutables: - old_value = vals[0] - new_value = vals[1] + for name in self._mutables: + old_value = self._mutables[name][0] + new_value = self._mutables[name][1] if old_value != new_value: log.config.debug("{} was mutated, updating".format(name)) self.set_obj(name, new_value, save_yaml=save_yaml) From dd4294de0316109c0e66fce4bf2b0fcfef72a6ad Mon Sep 17 00:00:00 2001 From: Ryan Farley Date: Tue, 19 Sep 2017 16:26:02 -0500 Subject: [PATCH 5/5] fix #2979: use dictionary for mutable tracking Using a dictionary instead of a list keeps only one working copy, allowing consistency in between calls of update_mutables() --- qutebrowser/config/config.py | 28 ++++++++++++++++------------ tests/unit/config/test_config.py | 2 +- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/qutebrowser/config/config.py b/qutebrowser/config/config.py index 88312facd..2a3e79b79 100644 --- a/qutebrowser/config/config.py +++ b/qutebrowser/config/config.py @@ -368,7 +368,7 @@ class Config(QObject): Attributes: _values: A dict mapping setting names to their values. - _mutables: A list of mutable objects to be checked for changes. + _mutables: A dictionary of mutable objects to be checked for changes. _yaml: A YamlConfig object or None. Signals: @@ -430,19 +430,23 @@ class Config(QObject): If mutable=True is set, watch the returned object for mutations. """ opt = self.get_opt(name) - # We always return a copy of the value stored internally, so the + obj = None + # If we allow mutation, there is a chance that prior mutations already + # entered the mutable dictionary and thus further copies are unneeded + # until update_mutables() is called + if name in self._mutables and mutable: + obj = self._mutables[name][1] + # Otherwise, we return a copy of the value stored internally, so the # internal value can never be changed by mutating the object returned. - obj = copy.deepcopy(self._values.get(name, opt.default)) - # Then we watch the returned object for changes. - if isinstance(obj, (dict, list)): - if mutable: - if name not in self._mutables: - self._mutables[name] = (copy.deepcopy(obj), obj) - else: - obj = self._mutables[name][1] else: - # Shouldn't be mutable (and thus hashable) - assert obj.__hash__ is not None, obj + obj = copy.deepcopy(self._values.get(name, opt.default)) + # Then we watch the returned object for changes. + if isinstance(obj, (dict, list)): + if mutable: + self._mutables[name] = (copy.deepcopy(obj), obj) + else: + # Shouldn't be mutable (and thus hashable) + assert obj.__hash__ is not None, obj return obj def get_str(self, name): diff --git a/tests/unit/config/test_config.py b/tests/unit/config/test_config.py index ab9e31fa8..b5be51546 100644 --- a/tests/unit/config/test_config.py +++ b/tests/unit/config/test_config.py @@ -706,7 +706,7 @@ class TestConfig: assert obj == new if mutable: - assert conf._mutables == ( old, new) + assert conf._mutables[option] == (old, new) if mutable and mutated: # Now let's update