GitPlugin: PyGIT._get_branches fails with ValueError on commit messages with line feeds

[Peter Suter]

Created as part of the move of GitPlugin. Tickets originally reported for ​th:GitPlugin: ​th:#8639

Quoting Carsten Klein:

I have a commit here that includes a subject line that includes ^M characters. This will cause get_branches to fail when trying to unpack the malformed lines resulting from splitting on linefeed boundary.

Here is a proposed fast solution that will replace all ^M characters so that retrieving the branch information does not fail

--- /trunk/tracopt/versioncontrol/git/PyGIT.py
+++ /trunk/tracopt/versioncontrol/git/PyGIT.py
    def _get_branches(self):
        "returns list of (local) branches, with active (= HEAD) one being the first item"

        result = []
-        for e in self.repo.branch("-v", "--no-abbrev").splitlines():
+        for e in self.repo.branch("-v", "--no-abbrev").replace('\r', '/CTRL-R').splitlines():
            bname, bsha = e[1:].strip().split()[:2]
            if e.startswith('*'):
                result.insert(0, (bname, bsha))
            else:
                result.append((bname, bsha))

        return result

[Christian Boos]

Link to a git repos demonstrating the issue welcome.

[Jun Omae]

Reproduced with CRs in the center of commit log. In [cac08aede/jomae.git], added the fix and unit tests.

[Jun Omae]

Fixed in [11802].

[Ryan J Ollos]

Would it be a good idea to move rmtree​ to branches/1.0-stable/trac/tests/compat.py@12081​ and also have branches/1.0-stable/trac/tests/functional/compat.py@12106:25-30​ use the same function?

I've prepared the proposed change, but don't want to commit it unless it has sufficient testing on Windows: log:rjollos.git:t10598. I've tested with Python 2.5, 2.6 and 2.7 on Ubuntu 13.04 so far.

[Peter Suter]

Reusing the same function sounds like a good idea.

• This comment​ seems to be specific to the git tests.
• The old comment​ sounded helpful. Also, it suggests "Even on Linux, shutil.rmtree chokes on read-only directories", and it looks like ​function would be os.rmdir in that case, which is not handled here​.
• Could ​stat.S_IWUSR be used instead of 0666? (Or would S_IWOTH|S_IROTH|S_IRGRP|S_IWGRP|S_IRUSR|S_IWUSR be required?)
• Should there be a else: raise after the only handled error condition​?

[Ryan J Ollos]

Replying to psuter:

• This comment​ seems to be specific to the git tests.

I'm not sure about the other questions you raised, but that code comment was my primary reason for having reservations about reusing the function. It seems generally useful to utf-8 encode the path, but I'm concerned about unexpected behavior.

[Jun Omae]

Replying to rjollos:

Would it be a good idea to move rmtree​ to branches/1.0-stable/trac/tests/compat.py@12081​ and also have branches/1.0-stable/trac/tests/functional/compat.py@12106:25-30​ use the same function?

Yes, good.

I've prepared the proposed change, but don't want to commit it unless it has sufficient testing on Windows: log:rjollos.git:t10598. I've tested with Python 2.5, 2.6 and 2.7 on Ubuntu 13.04 so far.

Verified with Python 2.6 on Windows XP.

But I've been wrong. I would like to use sys.getfilesystemencoding() instead 'utf-8' like this.

trac/tests/compat.py

@@ -17 +17 @@
 
 import os
 import shutil
+import sys
 import unittest
 
 
@@ -91 +92 @@
             mode = os.stat(path).st_mode
             os.chmod(path, mode | 0666)
             function(path)
-    if os.name == 'nt':
-        # Git repository for tests has unicode characters
-        # in the path and branch names
-        path = unicode(path, 'utf-8')
+    if os.name == 'nt' and isinstance(path, str):
+        # use unicode characters in order to operate non-ansi characters
+        # on Windows
+        path = unicode(path, sys.getfilesystemencoding())
     shutil.rmtree(path, onerror=onerror)

[Ryan J Ollos]

Replying to psuter:

Reusing the same function sounds like a good idea.

• This comment​ seems to be specific to the git tests.
• The old comment​ sounded helpful. Also, it suggests "Even on Linux, shutil.rmtree chokes on read-only directories", and it looks like ​function would be os.rmdir in that case, which is not handled here​.

I haven't been able to reproduce this issue on Linux with Python 2.5 - 2.7.

• Could ​stat.S_IWUSR be used instead of 0666? (Or would S_IWOTH|S_IROTH|S_IRGRP|S_IWGRP|S_IRUSR|S_IWUSR be required?)
• Should there be a else: raise after the only handled error condition​?

It seems like a good idea to have the else: raise, so I added it to the proposed changes, though I haven't reproduced a condition under which it would be exercised.

Hopefully I'm being a bit conservative by primarily making changes that I can test in some way. I'll wait a few days before pushing the proposed changes in case anyone else wants to add anything else.

I pulled in Jun's change from comment:8 to log:rjollos.git:t10598 as well.

[Ryan J Ollos]

Committed to 1.0-stable in [12247:12249] and merged to trunk in [12250].