From 6618c3a6e8158f5208d7bca32eafec2039b662bd Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Thu, 14 Sep 2017 14:15:06 +0200 Subject: [PATCH] Don't use shlex for configtypes.ShellCommand We accidentally did show the command as a list in to_str(). However, after correcting that to use shlex.escape, we got ugly qutebrowser command lines when tabbing to the default value, because of how shlex handles double-escaping: >>> print(shlex.quote("gvim -f '{}'")) 'gvim -f '"'"'{}'"'"'' While in this case, outputting "gvim -f '{}'" would be much more appropriate, it doesn't look like we can teach shlex.quote to do that. Instead, we now only accept a list as input for ShellCommand, at the price that the user needs to do :set editor.command '["gvim", "-f", "{}"]' instead of :set editor.command 'gvim -f {}' Fixes #2962. --- qutebrowser/config/config.py | 5 +++-- qutebrowser/config/configtypes.py | 16 +--------------- tests/end2end/features/test_editor_bdd.py | 5 +++-- tests/unit/config/test_config.py | 21 ++++++++++++++------- tests/unit/config/test_configtypes.py | 13 ++++++------- 5 files changed, 27 insertions(+), 33 deletions(-) diff --git a/qutebrowser/config/config.py b/qutebrowser/config/config.py index 6c92bedc1..67e44fa8a 100644 --- a/qutebrowser/config/config.py +++ b/qutebrowser/config/config.py @@ -240,8 +240,6 @@ class ConfigCommands: If the option name ends with '!' and it is a boolean value, toggle it. - // - Args: option: The name of the option. values: The value to set, or the values to cycle through. @@ -457,6 +455,9 @@ class Config(QObject): """ opt = self.get_opt(name) converted = opt.typ.from_str(value) + log.config.debug("Setting {} (type {}) to {!r} (converted from {!r})" + .format(name, opt.typ.__class__.__name__, converted, + value)) self._set_value(opt, converted) if save_yaml: self._yaml.values[name] = converted diff --git a/qutebrowser/config/configtypes.py b/qutebrowser/config/configtypes.py index 86514e4f0..115051466 100644 --- a/qutebrowser/config/configtypes.py +++ b/qutebrowser/config/configtypes.py @@ -46,7 +46,6 @@ Config types can do different conversations: import re import html -import shlex import codecs import os.path import itertools @@ -1239,7 +1238,7 @@ class FormatString(BaseType): class ShellCommand(List): - """A shellcommand which is split via shlex. + """A shell command as a list. Attributes: placeholder: If there should be a placeholder. @@ -1249,19 +1248,6 @@ class ShellCommand(List): super().__init__(valtype=String(), none_ok=none_ok) self.placeholder = placeholder - def from_str(self, value): - self._basic_str_validation(value) - if not value: - return None - - try: - split_val = shlex.split(value) - except ValueError as e: - raise configexc.ValidationError(value, str(e)) - - self.to_py(split_val) - return split_val - def to_py(self, value): value = super().to_py(value) if not value: diff --git a/tests/end2end/features/test_editor_bdd.py b/tests/end2end/features/test_editor_bdd.py index 00e959f4d..8e0537e69 100644 --- a/tests/end2end/features/test_editor_bdd.py +++ b/tests/end2end/features/test_editor_bdd.py @@ -18,6 +18,7 @@ # along with qutebrowser. If not, see . import sys +import json import textwrap import pytest_bdd as bdd @@ -41,7 +42,7 @@ def set_up_editor_replacement(quteproc, httpbin, tmpdir, text, replacement): with open(sys.argv[1], 'w', encoding='utf-8') as f: f.write(data) """.format(text=text, replacement=replacement))) - editor = '"{}" "{}" {{}}'.format(sys.executable, script) + editor = json.dumps([sys.executable, str(script), '{}']) quteproc.set_setting('editor.command', editor) @@ -55,5 +56,5 @@ def set_up_editor(quteproc, httpbin, tmpdir, text): with open(sys.argv[1], 'w', encoding='utf-8') as f: f.write({text!r}) """.format(text=text))) - editor = '"{}" "{}" {{}}'.format(sys.executable, script) + editor = json.dumps([sys.executable, str(script), '{}']) quteproc.set_setting('editor.command', editor) diff --git a/tests/unit/config/test_config.py b/tests/unit/config/test_config.py index 572a6c6f8..5a67651da 100644 --- a/tests/unit/config/test_config.py +++ b/tests/unit/config/test_config.py @@ -293,22 +293,29 @@ class TestSetConfigCommand: assert msg.text == 'url.auto_search = never' @pytest.mark.parametrize('temp', [True, False]) - def test_set_simple(self, monkeypatch, commands, config_stub, temp): - """Run ':set [-t] url.auto_search dns'. + @pytest.mark.parametrize('option, old_value, inp, new_value', [ + ('url.auto_search', 'naive', 'dns', 'dns'), + # https://github.com/qutebrowser/qutebrowser/issues/2962 + ('editor.command', ['gvim', '-f', '{}'], '[emacs, "{}"]', + ['emacs', '{}']), + ]) + def test_set_simple(self, monkeypatch, commands, config_stub, + temp, option, old_value, inp, new_value): + """Run ':set [-t] option value'. Should set the setting accordingly. """ monkeypatch.setattr(objects, 'backend', usertypes.Backend.QtWebKit) - assert config_stub.val.url.auto_search == 'naive' + assert config_stub.get(option) == old_value - commands.set(0, 'url.auto_search', 'dns', temp=temp) + commands.set(0, option, inp, temp=temp) - assert config_stub.val.url.auto_search == 'dns' + assert config_stub.get(option) == new_value if temp: - assert 'url.auto_search' not in config_stub._yaml.values + assert option not in config_stub._yaml.values else: - assert config_stub._yaml.values['url.auto_search'] == 'dns' + assert config_stub._yaml.values[option] == new_value @pytest.mark.parametrize('temp', [True, False]) def test_set_temp_override(self, commands, config_stub, temp): diff --git a/tests/unit/config/test_configtypes.py b/tests/unit/config/test_configtypes.py index dcf18a675..5daf63294 100644 --- a/tests/unit/config/test_configtypes.py +++ b/tests/unit/config/test_configtypes.py @@ -1699,10 +1699,10 @@ class TestShellCommand: return configtypes.ShellCommand @pytest.mark.parametrize('kwargs, val, expected', [ - ({}, 'foobar', ['foobar']), - ({'placeholder': '{}'}, 'foo {} bar', ['foo', '{}', 'bar']), - ({'placeholder': '{}'}, 'foo{}bar', ['foo{}bar']), - ({'placeholder': '{}'}, 'foo "bar {}"', ['foo', 'bar {}']), + ({}, '[foobar]', ['foobar']), + ({'placeholder': '{}'}, '[foo, "{}", bar]', ['foo', '{}', 'bar']), + ({'placeholder': '{}'}, '["foo{}bar"]', ['foo{}bar']), + ({'placeholder': '{}'}, '[foo, "bar {}"]', ['foo', 'bar {}']), ]) def test_valid(self, klass, kwargs, val, expected): cmd = klass(**kwargs) @@ -1710,9 +1710,8 @@ class TestShellCommand: assert cmd.to_py(expected) == expected @pytest.mark.parametrize('kwargs, val', [ - ({'placeholder': '{}'}, 'foo bar'), - ({'placeholder': '{}'}, 'foo { } bar'), - ({}, 'foo"'), # not splittable with shlex + ({'placeholder': '{}'}, '[foo, bar]'), + ({'placeholder': '{}'}, '[foo, "{", "}", bar'), ]) def test_from_str_invalid(self, klass, kwargs, val): with pytest.raises(configexc.ValidationError):