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

@@ -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

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:

tests/end2end/features/test_editor_bdd.py

@@ -18,6 +18,7 @@
 # along with qutebrowser.  If not, see <http://www.gnu.org/licenses/>.
 
 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)

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):
+    @pytest.mark.parametrize('option, old_value, inp, new_value', [
-        """Run ':set [-t] url.auto_search dns'.
+        ('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):

tests/unit/config/test_configtypes.py

@@ -1699,10 +1699,10 @@ class TestShellCommand:
         return configtypes.ShellCommand
 
     @pytest.mark.parametrize('kwargs, val, expected', [
-        ({}, 'foobar', ['foobar']),
+        ({}, '[foobar]', ['foobar']),
-        ({'placeholder': '{}'}, 'foo {} bar', ['foo', '{}', 'bar']),
+        ({'placeholder': '{}'}, '[foo, "{}", bar]', ['foo', '{}', 'bar']),
-        ({'placeholder': '{}'}, 'foo{}bar', ['foo{}bar']),
+        ({'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]'),
-        ({'placeholder': '{}'}, 'foo { } bar'),
+        ({'placeholder': '{}'}, '[foo, "{", "}", bar'),
-        ({}, 'foo"'),  # not splittable with shlex
     ])
     def test_from_str_invalid(self, klass, kwargs, val):
         with pytest.raises(configexc.ValidationError):