Merge pull request #2297 from JingHao-Leon/fix/config-unquoted-escape-parsing

fix(config): decode escapes in unquoted config values

Author: Byron

AGENTS.md

@@ -5,6 +5,10 @@ including the [Prevent agent impersonation](CONTRIBUTING.md#prevent-agent-impers
 section governing agent identification and separation of unaltered user statements
 when communicating through a person's account.
 
+[Prefer pull requests](CONTRIBUTING.md#prefer-pull-requests): a PR can serve as an
+issue with a fix attached. Do not create an issue solely to follow it immediately
+with a PR that fixes it; describe the problem in the PR instead.
+
 # Commit messages
 
 Follow Conventional Commits for every commit. Every commit must have a

CONTRIBUTING.md

@@ -9,6 +9,16 @@ The following is a short step-by-step rundown of what one typically would do to
 - Feel free to add yourself to AUTHORS file.
 - Create a pull request.
 
+## Prefer pull requests
+
+Pull requests are preferred when you have a fix or improvement to contribute: a PR
+can be treated as an issue with a fix attached. Describe the problem and proposed
+solution in the PR itself; a separate issue is not required.
+
+Please avoid opening an issue solely to follow it immediately with a PR that fixes
+it. Open the PR directly instead. Issues are still welcome for problems without a
+fix or for changes that need discussion before implementation.
+
 ## Quality expectations
 
 Contributions must be made with care and meet the quality bar of the surrounding code.

README.md

@@ -201,8 +201,12 @@ Please have a look at the [contributions file][contributing].
 - [Questions and Answers](http://stackexchange.com/filters/167317/gitpython)
 - Please post on Stack Overflow and use the `gitpython` tag
 - [Issue Tracker](https://github.com/gitpython-developers/GitPython/issues)
-  - Post reproducible bugs and feature requests as a new issue.
-    Please be sure to provide the following information if posting bugs:
+  - Post reproducible bugs and feature requests as a new issue when you do not have
+    a fix or need discussion before implementation. If you have a fix or improvement,
+    [open a pull request directly](CONTRIBUTING.md#prefer-pull-requests) and describe
+    the problem there; you do not need to create an issue first.
+    Please be sure to provide the following information when reporting bugs in an
+    issue or PR:
     - GitPython version (e.g. `import git; git.__version__`)
     - Python version (e.g. `python --version`)
     - The encountered stack-trace, if applicable

git/config.py

@@ -69,6 +69,13 @@
 VALID_CONFIG_OPTION_NAME_RE = re.compile(r"^[A-Za-z0-9_.-]+$")
 """Pattern for option names that can be written without changing config syntax."""
 
+UNQUOTED_ESCAPES = {"b": "\b", "n": "\n", "t": "\t", '"': '"', "\\": "\\"}
+"""Escape sequences git recognizes in an unquoted value.
+
+Any other backslash escape is an error for git, which is why a value holding one is
+left undecoded rather than rejected.
+"""
+
 
 class MetaParserBuilder(abc.ABCMeta):  # noqa: B024
     """Utility class wrapping base-class methods into decorators that assure read-only
@@ -530,10 +537,9 @@ def parse_value(value: str) -> str:
             parsed: List[str] = []
             whitespace: List[str] = []
             quoted = escaped = False
-            escapes = {"b": "\b", "n": "\n", "t": "\t", '"': '"', "\\": "\\"}
             for char in value:
                 if escaped:
-                    parsed.append(escapes.get(char, "\\" + char))
+                    parsed.append(UNQUOTED_ESCAPES.get(char, "\\" + char))
                     escaped = False
                     continue
                 if char.isspace() and not quoted:
@@ -552,6 +558,26 @@ def parse_value(value: str) -> str:
                     parsed.append(char)
             return "".join(parsed)
 
+        def escapes_are_known(value: str) -> bool:
+            """Whether every backslash in `value` introduces an escape git recognises.
+
+            Git rejects an unquoted value carrying an unknown escape outright. GitPython
+            tolerates such values, because that is what lets a Windows path such as
+            ``C:\\Users\\me`` stay literal, so a value is decoded only when every one
+            of its backslashes is part of a known escape. Decoding part of a value would
+            otherwise turn the ``\\t`` of ``\\Temp\\test`` into a tab while leaving the
+            ``\\U`` of ``\\Users`` alone.
+            """
+            index = value.find("\\")
+            while index >= 0:
+                # A trailing backslash has nothing to escape and git drops it, so
+                # only a backslash followed by an unknown escape disqualifies the
+                # value from being decoded.
+                if index + 1 < len(value) and value[index + 1] not in UNQUOTED_ESCAPES:
+                    return False
+                index = value.find("\\", index + 2)
+            return True
+
         while True:
             # We assume to read binary!
             raw_line = fp.readline()
@@ -606,23 +632,30 @@ def parse_value(value: str) -> str:
                         # next line is appended before the complete value is
                         # parsed. An even number means the last backslash is
                         # escaped and the value ends there.
-                        continued = False
+                        eof_continuation = False
                         while True:
                             if not is_line_continuation(optval):
                                 break
                             continuation = fp.readline()
                             if not continuation:
-                                # Backslash at end of file: git drops it.
-                                optval = optval[:-1]
+                                # Keep the backslash until parsing so preceding
+                                # whitespace is significant even though git drops it.
+                                eof_continuation = True
                                 break
                             lineno = lineno + 1
                             joined = continuation.decode(defenc)
                             while joined.endswith("\n") or joined.endswith("\r"):
                                 joined = joined[:-1]
                             optval = optval[:-1] + joined
-                            continued = True
-                        if continued:
+                        # Git decodes the standard escape sequences in unquoted
+                        # values, but only a value in which every backslash is part
+                        # of a known escape is decoded, so that a Windows path keeps
+                        # its backslashes instead of losing them one escape at a time.
+                        optval, _ = strip_inline_comment(optval)
+                        if escapes_are_known(optval):
                             optval = parse_value(optval)
+                        elif eof_continuation:
+                            optval = optval[:-1]
                     elif quote_open:
                         # Opens quoting and does not close: appears to start multi-line quoting.
                         is_multi_line = True

test/fixtures/git_config_unquoted_escapes

@@ -0,0 +1,12 @@
+[custom]
+  hasnewline = first\nsecond
+  hastab = first\tsecond
+  hasbackslash = foo\\bar
+  hasquote = ab\"cd
+  hastrailingbackslash = word\\
+  ordinary = hello world
+  withcolon = host:port
+  withequals = key=value
+  withhash = value#notcomment
+  hasunknownescape = foo\qbar
+  haswindowspath = C:\Users\RUNNER~1\AppData\Local\Temp\test_foo

test/test_config.py

@@ -365,7 +365,7 @@ def test_backslash_line_continuation(self):
             (b"[a]\n\tk = one\\\n two ; ignored\n", "one two"),
             (b'[a]\n\tk = one\\\n "two"\n', "one two"),
             (b"[a]\n\tk = one\\\n two\\tthree\n", "one two\tthree"),
-            (b"[a]\n\tk = val\\\\\n next\n", "val\\\\"),
+            (b"[a]\n\tk = val\\\\\n next\n", "val\\"),
             (b"[a]\n\tk = end\\\n", "end"),
             (b"[alias]\n\tco = checkout \\\n\t\t-v\n", "checkout \t\t-v"),
         ]
@@ -378,6 +378,33 @@ def test_backslash_line_continuation(self):
             key = "co" if section == "alias" else "k"
             self.assertEqual(config.get_value(section, key), expected)
 
+    @with_rw_directory
+    def test_continued_values_preserve_comments_and_eof_whitespace(self, rw_dir):
+        cases = [
+            (b"one\\\n two # C:\\Users\n", "one two"),
+            (b"one\\\n two ; C:\\Users\n", "one two"),
+            (b"one\\\n two\\tthree # C:\\Users\n", "one two\tthree"),
+            (b'one\\\n "two # three" # C:\\Users\n', "one two # three"),
+            (b"value \\", "value "),
+            (b"value \\\n", "value "),
+            (b"value \t\\\n", "value \t"),
+            (b"one\\\n two \\\n", "one two "),
+            (b"C:\\Users \\\n", "C:\\Users "),
+        ]
+        config_path = osp.join(rw_dir, "config")
+        for value, expected in cases:
+            with self.subTest(value=value):
+                with open(config_path, "wb") as config_file:
+                    config_file.write(b"[a]\n\tk = " + value)
+
+                with GitConfigParser(config_path, read_only=False) as config:
+                    self.assertEqual(config.get_value("a", "k"), expected)
+                    config.set_value("other", "value", "updated")
+
+                with GitConfigParser(config_path) as config:
+                    self.assertEqual(config.get_value("a", "k"), expected)
+                    self.assertEqual(config.get_value("other", "value"), "updated")
+
     def test_get_value_reads_git_boolean_spellings(self):
         # git accepts yes/no and on/off as well as true/false, and getboolean on
         # this class already did. get_value returned them as strings, so "no" and
@@ -1209,6 +1236,30 @@ def test_config_with_quotes_containing_escapes(self):
         # Cases without quotes should still parse correctly even after those, too.
         self.assertEqual(cr.get("custom", "unquoted"), "good evening")
 
+    def test_config_with_unquoted_escapes(self):
+        """Decode git's escape sequences in unquoted values, but only when every
+        backslash in the value is part of a known escape."""
+        cr = GitConfigParser(fixture_path("git_config_unquoted_escapes"), read_only=True)
+
+        self.assertEqual(cr.get("custom", "hasnewline"), "first\nsecond")
+        self.assertEqual(cr.get("custom", "hastab"), "first\tsecond")
+        self.assertEqual(cr.get("custom", "hasbackslash"), R"foo\bar")
+        self.assertEqual(cr.get("custom", "hasquote"), 'ab"cd')
+        self.assertEqual(cr.get("custom", "hastrailingbackslash"), "word\\")
+
+        # Git rejects an unknown escape rather than decoding it. GitPython keeps
+        # such a value literal instead of rejecting it, which is what keeps a
+        # Windows path usable, so no backslash in one may be decoded: doing so
+        # would turn the \t of \Temp\test into a tab while leaving \U alone.
+        self.assertEqual(cr.get("custom", "hasunknownescape"), R"foo\qbar")
+        self.assertEqual(cr.get("custom", "haswindowspath"), R"C:\Users\RUNNER~1\AppData\Local\Temp\test_foo")
+
+        # Values without escapes keep parsing as before.
+        self.assertEqual(cr.get("custom", "ordinary"), "hello world")
+        self.assertEqual(cr.get("custom", "withcolon"), "host:port")
+        self.assertEqual(cr.get("custom", "withequals"), "key=value")
+        self.assertEqual(cr.get("custom", "withhash"), "value")
+
     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)