Merge pull request #2048 from EliahKagan/conservative-quote-removal

Don't remove quotes if `\` or `"` are present inside

Author: Byron

git/config.py

@@ -496,18 +496,26 @@ def string_decode(v: str) -> str:
                 if mo:
                     # We might just have handled the last line, which could contain a quotation we want to remove.
                     optname, vi, optval = mo.group("option", "vi", "value")
+                    optname = self.optionxform(optname.rstrip())
+
                     if vi in ("=", ":") and ";" in optval and not optval.strip().startswith('"'):
                         pos = optval.find(";")
                         if pos != -1 and optval[pos - 1].isspace():
                             optval = optval[:pos]
                     optval = optval.strip()
-                    optname = self.optionxform(optname.rstrip())
-                    if len(optval) > 1 and optval[0] == '"' and optval[-1] != '"':
+
+                    if len(optval) < 2 or optval[0] != '"':
+                        # Does not open quoting.
+                        pass
+                    elif optval[-1] != '"':
+                        # Opens quoting and does not close: appears to start multi-line quoting.
                         is_multi_line = True
                         optval = string_decode(optval[1:])
-                    elif len(optval) > 1 and optval[0] == '"' and optval[-1] == '"':
+                    elif optval.find("\\", 1, -1) == -1 and optval.find('"', 1, -1) == -1:
+                        # Opens and closes quoting. Single line, and all we need is quote removal.
                         optval = optval[1:-1]
-                    # END handle multi-line
+                    # TODO: Handle other quoted content, especially well-formed backslash escapes.
+
                     # Preserves multiple values for duplicate optnames.
                     cursect.add(optname, optval)
                 else:

test/fixtures/git_config_with_quotes_escapes

@@ -0,0 +1,9 @@
+[custom]
+  hasnewline = "first\nsecond"
+  hasbackslash = "foo\\bar"
+  hasquote = "ab\"cd"
+  hastrailingbackslash = "word\\"
+  hasunrecognized = "p\qrs"
+  hasunescapedquotes = "ab"cd"e"
+  ordinary = "hello world"
+  unquoted = good evening

test/test_config.py

@@ -428,6 +428,26 @@ def test_config_with_quotes_with_whitespace_outside_value(self):
         cr = GitConfigParser(fixture_path("git_config_with_quotes_whitespace_outside"), read_only=True)
         self.assertEqual(cr.get("init", "defaultBranch"), "trunk")
 
+    def test_config_with_quotes_containing_escapes(self):
+        """For now just suppress quote removal. But it would be good to interpret most of these."""
+        cr = GitConfigParser(fixture_path("git_config_with_quotes_escapes"), read_only=True)
+
+        # These can eventually be supported by substituting the represented character.
+        self.assertEqual(cr.get("custom", "hasnewline"), R'"first\nsecond"')
+        self.assertEqual(cr.get("custom", "hasbackslash"), R'"foo\\bar"')
+        self.assertEqual(cr.get("custom", "hasquote"), R'"ab\"cd"')
+        self.assertEqual(cr.get("custom", "hastrailingbackslash"), R'"word\\"')
+        self.assertEqual(cr.get("custom", "hasunrecognized"), R'"p\qrs"')
+
+        # It is less obvious whether and what to eventually do with this.
+        self.assertEqual(cr.get("custom", "hasunescapedquotes"), '"ab"cd"e"')
+
+        # Cases where quote removal is clearly safe should happen even after those.
+        self.assertEqual(cr.get("custom", "ordinary"), "hello world")
+
+        # Cases without quotes should still parse correctly even after those, too.
+        self.assertEqual(cr.get("custom", "unquoted"), "good evening")
+
     def test_get_values_works_without_requiring_any_other_calls_first(self):
         file_obj = self._to_memcache(fixture_path("git_config_multiple"))
         cr = GitConfigParser(file_obj, read_only=True)