Edgewall Software
Modify

Opened 8 years ago

Closed 7 years ago

Last modified 6 years ago

#10598 closed defect (fixed)

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

Reported by: Peter Suter Owned by: Jun Omae
Priority: lowest Milestone: 1.0.2
Component: plugin/git Version:
Severity: normal Keywords:
Cc: Ryan J Ollos Branch:
Release Notes:

fix ValueError in Storage._get_branches() if a branch head has carriage returns in the commit log

API Changes:

Extended exception handling in trac.tests.util:rmtree.

Description

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

Attachments (0)

Change History (10)

comment:1 by Christian Boos, 7 years ago

Milestone: next-stable-1.0.x
Priority: normallowest

Link to a git repos demonstrating the issue welcome.

comment:2 by Jun Omae, 7 years ago

Owner: set to Jun Omae
Status: newassigned

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

comment:3 by Jun Omae, 7 years ago

Milestone: next-stable-1.0.x1.0.2
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Fixed in [11802].

comment:4 by Ryan J Ollos, 6 years ago

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.

comment:5 by Ryan J Ollos, 6 years ago

Cc: Ryan J Ollos added

comment:6 by Peter Suter, 6 years ago

Reusing the same function sounds like a good idea.

in reply to:  6 comment:7 by Ryan J Ollos, 6 years ago

Replying to psuter:

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.

in reply to:  4 comment:8 by Jun Omae, 6 years ago

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

    diff --git a/trac/tests/compat.py b/trac/tests/compat.py
    index 08c3f5e..29af1fc 100644
    a b with previous versions of Python from 2.5 onward.  
    1717
    1818import os
    1919import shutil
     20import sys
    2021import unittest
    2122
    2223
    def rmtree(path):  
    9192            mode = os.stat(path).st_mode
    9293            os.chmod(path, mode | 0666)
    9394            function(path)
    94     if os.name == 'nt':
    95         # Git repository for tests has unicode characters
    96         # in the path and branch names
    97         path = unicode(path, 'utf-8')
     95    if os.name == 'nt' and isinstance(path, str):
     96        # use unicode characters in order to operate non-ansi characters
     97        # on Windows
     98        path = unicode(path, sys.getfilesystemencoding())
    9899    shutil.rmtree(path, onerror=onerror)

in reply to:  6 comment:9 by Ryan J Ollos, 6 years ago

Replying to psuter:

Reusing the same function sounds like a good idea.

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

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.

comment:10 by Ryan J Ollos, 6 years ago

API Changes: modified (diff)

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Jun Omae to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.