Repository: gitpython-developers/GitPython
Pull request #467 (2016-06-07 08:06:37)

Don't choke on (legitimately) invalidly encoded Unicode paths
We've come across path names that contain bytes that are invalid in UTF-8 encoded strings, even though they're very rare. My assumption here is these commits have been created by an old (buggy?) version of Git, and now live in the tree objects with this data. Since we return only unicode strings for the `a_path` and `b_path` properties, we're not able to decode this string and thus choke when asking for the diff.

This PR fixes that by using "replace" semantics when decoding. This will effectively replace the illegal bytes `\200` (or `\x80`) by \ufffd (= �).


### Follow-up discussion

However, this also means that if you would want to git-blame this file, there's no good way of referencing this path, since it's inherently a bytes path. Normally, when we pass unicode paths to git-blame via GitPython's blame API, the paths get converted to UTF-8 right before issuing the external command. But there's no way of getting the original bytes back after the "replace" operation happened.

Example:

- The input path is `b'illegal-\x80.txt'` (containing illegal byte `\x80`)
- When decoded to UTF-8 with characters replaced, we get the unicode string `u'illegal-\ufffd.txt'` (= "illegal-�.txt")
- When encoding that in UTF-8, we find `b'illegal-\xef\xbf\xbd.txt'`

When we next pass `illegal-\xef\xbf\xbd.txt` to git-blame, it will not be able to find this path. Perhaps it would be a good idea to not only return the decoded path strings, but also provide access to the raw bytes found, i.e. by exposing `a_rawpath` and `b_rawpath`, which would always be bytes? That way, you could still have the friendly "unicode paths" for most use cases, but use bytes if you need to speak the language of Git more accurately.


Patch:
diff --git a/git/diff.py b/git/diff.py
index 9073767e..aeaa67d5 100644
--- a/git/diff.py
+++ b/git/diff.py
@@ -404,15 +404,15 @@ class Diff(object):
             a_mode = old_mode or deleted_file_mode or (a_path and (b_mode or new_mode or new_file_mode))
             b_mode = b_mode or new_mode or new_file_mode or (b_path and a_mode)
             index.append(Diff(repo,
-                              a_path and a_path.decode(defenc),
-                              b_path and b_path.decode(defenc),
+                              a_path and a_path.decode(defenc, 'replace'),
+                              b_path and b_path.decode(defenc, 'replace'),
                               a_blob_id and a_blob_id.decode(defenc),
                               b_blob_id and b_blob_id.decode(defenc),
                               a_mode and a_mode.decode(defenc),
                               b_mode and b_mode.decode(defenc),
                               new_file, deleted_file,
-                              rename_from and rename_from.decode(defenc),
-                              rename_to and rename_to.decode(defenc),
+                              rename_from and rename_from.decode(defenc, 'replace'),
+                              rename_to and rename_to.decode(defenc, 'replace'),
                               None))
 
             previous_header = header


Test patch:
diff --git a/git/test/fixtures/diff_patch_unsafe_paths b/git/test/fixtures/diff_patch_unsafe_paths
index 9ee6b834..1aad6754 100644
--- a/git/test/fixtures/diff_patch_unsafe_paths
+++ b/git/test/fixtures/diff_patch_unsafe_paths
@@ -68,6 +68,13 @@ index 0000000000000000000000000000000000000000..eaf5f7510320b6a327fb308379de2f94
 +++ "b/path/\360\237\222\251.txt"
 @@ -0,0 +1 @@
 +dummy content
+diff --git "a/path/\200-invalid-unicode-path.txt" "b/path/\200-invalid-unicode-path.txt"
+new file mode 100644
+index 0000000000000000000000000000000000000000..eaf5f7510320b6a327fb308379de2f94d8859a54
+--- /dev/null
++++ "b/path/\200-invalid-unicode-path.txt"
+@@ -0,0 +1 @@
++dummy content
 diff --git a/a/with spaces b/b/with some spaces
 similarity index 100%
 rename from a/with spaces
diff --git a/git/test/test_diff.py b/git/test/test_diff.py
index 8966351a..8d189b12 100644
--- a/git/test/test_diff.py
+++ b/git/test/test_diff.py
@@ -162,16 +162,17 @@ class TestDiff(TestBase):
         self.assertEqual(res[7].b_path, u'path/with-question-mark?')
         self.assertEqual(res[8].b_path, u'path/¯\\_(ツ)_|¯')
         self.assertEqual(res[9].b_path, u'path/💩.txt')
+        self.assertEqual(res[10].b_path, u'path/�-invalid-unicode-path.txt')
 
         # The "Moves"
         # NOTE: The path prefixes a/ and b/ here are legit!  We're actually
         # verifying that it's not "a/a/" that shows up, see the fixture data.
-        self.assertEqual(res[10].a_path, u'a/with spaces')       # NOTE: path a/ here legit!
-        self.assertEqual(res[10].b_path, u'b/with some spaces')  # NOTE: path b/ here legit!
-        self.assertEqual(res[11].a_path, u'a/ending in a space ')
-        self.assertEqual(res[11].b_path, u'b/ending with space ')
-        self.assertEqual(res[12].a_path, u'a/"with-quotes"')
-        self.assertEqual(res[12].b_path, u'b/"with even more quotes"')
+        self.assertEqual(res[11].a_path, u'a/with spaces')       # NOTE: path a/ here legit!
+        self.assertEqual(res[11].b_path, u'b/with some spaces')  # NOTE: path b/ here legit!
+        self.assertEqual(res[12].a_path, u'a/ending in a space ')
+        self.assertEqual(res[12].b_path, u'b/ending with space ')
+        self.assertEqual(res[13].a_path, u'a/"with-quotes"')
+        self.assertEqual(res[13].b_path, u'b/"with even more quotes"')
 
     def test_diff_patch_format(self):
         # test all of the 'old' format diffs for completness - it should at least