Repository: gitpython-developers/GitPython
Pull request #2048 (2025-06-08 05:23:50)

Don't remove quotes if `\` or `"` are present inside
#### Background

#2035 fixed issue #1923, where the ConfigParser would not remove the quotes around single-line values. As discussed in comments there:

- That improved the common cases where quote removal is all that is needed, in particular where no escape sequences are present.
- When there are escape sequences within a single line quoted value, that was already not handled correctly--since no single-line quoted values (other than a quoted empty string) had been handled correctly before [#2035](https://github.com/gitpython-developers/GitPython/pull/2035), in that quote removal was never performed for them.
- Nonetheless, [#2035](https://github.com/gitpython-developers/GitPython/pull/2035) may have made the situation worse for some applications if they handled the quoted values themselves, or if the quoted values were more readily recognized to mean parsing had not succeeded.

#### Let's take the best of both worlds (so far)

This PR keeps the changes from #2035 in the case that they work because the text contained strictly between the beginning and ending `"` characters contains neither any `\` nor any other `"`. This both:

- Preserves the benefit of [#2035](https://github.com/gitpython-developers/GitPython/pull/2035) for [#1923](https://github.com/gitpython-developers/GitPython/issues/1923). (The only exception is cases where a `\` is meant to be preserved rather than treated as an escape character. This is presumably rare--if it ever happens--since that's not the syntax of double-quoted values in Git config files.)
- Keeps the old, potentially safer behavior of doing no transformations, not even quote removal, in cases where quote removal alone would clearly not produce the correct result.

#### Changes

1. c8e4aa0f30d06ea2437539a5d56eede1ffa11432 refactors to prepare for the other changes.
2. 7bcea08873ca4052ab743f9e2f7cff42e7fe62d8 adds a test for the behavior described above.
3. f2b80410e96a256aed044fba0387eab0440a1525 makes those tests pass.

#### But it can get better than this

This is not intended as a long-term alternative to parsing escape sequences. The idea in https://github.com/gitpython-developers/GitPython/pull/2035#issuecomment-2951918138 of handling them is good, and this is not meant to discourage or interfere with that. The new test fixture and test can be modified accordingly. See the docstring and comments in `test_config_with_quotes_containing_escapes`.

#### For review

It seems to me that the idea here is sound, since it restores the main branch to a state where no changes are expected to produce problems for programs and libraries that use GitPython, if a patch release were to be made.

But even if I am right to think that, there are a few reasons it may be useful to have a review here before merging:

- I'm less familiar with the config parser than most of the rest of the code of GitPython, so there could be design subtleties I am missing.
- There are multiple reasonable ways to make this change, and you may have input, stylistically or otherwise.
- It's easy to introduce bugs when making nontrivial (even if small) changes to parsing logic.

(This follows #2046 and #2047, which followed #2035 and #2036.)


Patch:
diff --git a/git/config.py b/git/config.py
index cdcc32e7..5fc099a2 100644
--- a/git/config.py
+++ b/git/config.py
@@ -496,18 +496,26 @@ class GitConfigParser(cp.RawConfigParser, metaclass=MetaParserBuilder):
                 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 patch:
diff --git a/test/fixtures/git_config_with_quotes_escapes b/test/fixtures/git_config_with_quotes_escapes
new file mode 100644
index 00000000..33332c22
--- /dev/null
+++ b/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
diff --git a/test/test_config.py b/test/test_config.py
index 76b918a5..8e1007d9 100644
--- a/test/test_config.py
+++ b/test/test_config.py
@@ -428,6 +428,26 @@ class TestBase(TestCase):
         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)