Edgewall Software
Modify

Opened 3 years ago

Closed 3 years ago

Last modified 15 months ago

#13405 closed defect (fixed)

Git version rendered with bytes type representation on System Information table

Reported by: Ryan J Ollos Owned by: Ryan J Ollos
Priority: normal Milestone: 1.5.4
Component: general Version:
Severity: normal Keywords:
Cc: Branch:
Release Notes:

Fixed incorrect rendering of Git version in System Information table.

API Changes:
Internal Changes:

Description (last modified by Ryan J Ollos)

Attachments (1)

Screen Shot 2021-05-27 at 00.18.14.jpg (34.8 KB ) - added by Ryan J Ollos 3 years ago.

Download all attachments as: .zip

Change History (11)

by Ryan J Ollos, 3 years ago

comment:1 by Ryan J Ollos, 3 years ago

Description: modified (diff)

comment:2 by Ryan J Ollos, 3 years ago

Looks like this may fix the issue. I will look at adding test coverage.

  • tracopt/versioncontrol/git/PyGIT.py

    diff --git a/tracopt/versioncontrol/git/PyGIT.py b/tracopt/versioncontrol/git/PyGIT.py
    index 534869e92c..8d4431ee7a 100644
    a b class Storage(object):  
    335335        try:
    336336            g = GitCore(git_bin=git_bin)
    337337            [v] = g.version().splitlines()
    338             version = v.strip().split()[2]
     338            version = str(v.strip().split()[2], 'utf-8')
    339339            # 'version' has usually at least 3 numeric version
    340340            # components, e.g.::
    341341            #  1.5.4.2
    class Storage(object):  
    348348                except ValueError:
    349349                    return s
    350350
    351             split_version = tuple(map(try_int, version.split(b'.')))
     351            split_version = tuple(map(try_int, version.split('.')))
    352352
    353353            result = {}
    354354            result['v_str'] = version
Version 0, edited 3 years ago by Ryan J Ollos (next)

comment:3 by Ryan J Ollos, 3 years ago

Summary: Git displayed as bytes string on System Information tableGit version rendered with bytes type representation on System Information table

comment:4 by Ryan J Ollos, 3 years ago

Owner: set to Ryan J Ollos
Status: newassigned

I refactored the function and added test coverage: [9224cac767/rjollos.git].

comment:5 by Jun Omae, 3 years ago

Looks good to me (unit-test passes on ubuntu, macos and windows with git 2.31.1).

I thought git --version writes the version with CRLF on Windows, however LF is actually used instead of.

Latest git for Windows (2.31.1)

>>> from tracopt.versioncontrol.git.PyGIT import Storage, GitCore
>>> Storage.git_version()
{'v_str': '2.31.1.windows.1', 'v_tuple': (2, 31, 1, 'windows', 1), 'v_min_tuple': (1, 5, 6), 'v_min_str': '1.5.6', 'v_compatible': True}
>>> GitCore().version()
b'git version 2.31.1.windows.1\n'

Oldest git for Windows (2.5.0)

>>> from tracopt.versioncontrol.git.PyGIT import Storage, GitCore
>>> Storage.git_version()
{'v_str': '2.5.0.windows.1', 'v_tuple': (2, 5, 0, 'windows', 1), 'v_min_tuple': (1, 5, 6), 'v_min_str': '1.5.6', 'v_compatible': True}
>>> GitCore().version()
b'git version 2.5.0.windows.1\n'

comment:6 by Ryan J Ollos, 3 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Thanks for testing, and checking the line ending on Windows. Committed in r17549.

comment:7 by Ryan J Ollos, 3 years ago

I've been thinking it probably doesn't make sense to keep the exception handler when matching the regex. Proposed refactoring:

  • tracopt/versioncontrol/git/PyGIT.py

    diff --git a/tracopt/versioncontrol/git/PyGIT.py b/tracopt/versioncontrol/git/PyGIT.py
    index ea3a15f563..995f130a48 100644
    a b class Storage(object):  
    332332    @staticmethod
    333333    def git_version(git_bin='git'):
    334334        GIT_VERSION_MIN_REQUIRED = (1, 5, 6)
    335         g = GitCore(git_bin=git_bin)
    336         # 'version' has usually at least 3 numeric version
    337         # components, e.g.::
     335        version = str(GitCore(git_bin=git_bin).version(), 'utf-8')
     336        # 'version' should have at least 3 numeric version components:
     337        #  1.5.6
     338        #  1.5.6.windows.1
    338339        #  1.5.6.2
    339340        #  1.5.6.3.230.g2db511
    340341        #  1.5.6.GIT
    341         try:
    342             v = g.version()
    343             m = re.match(b'git version (.*)\n$', v)
    344             version = str(m.group(1), 'utf-8')
    345             split_version = tuple(as_int(s, s) for s in version.split('.'))
    346         except Exception as e:
    347             raise GitError("Could not retrieve GIT version (tried to "
    348                            "execute/parse '%s --version' but got %s)"
    349                            % (git_bin, repr(e)))
     342        m = re.match('git version (.*)\n$', version)
     343        if not m:
     344            raise GitError("Could not retrieve GIT version. "
     345                           "'%s --version' returned '%s'" % (git_bin, version))
     346        version_str = m.group(1)
     347        version_tuple = tuple(as_int(s, s) for s in version_str.split('.'))
    350348        return {
    351             'v_str': version,
    352             'v_tuple': split_version,
     349            'v_str': version_str,
     350            'v_tuple': version_tuple,
    353351            'v_min_tuple': GIT_VERSION_MIN_REQUIRED,
    354352            'v_min_str': '.'.join(map(str, GIT_VERSION_MIN_REQUIRED)),
    355             'v_compatible': split_version >= GIT_VERSION_MIN_REQUIRED,
     353            'v_compatible': version_tuple >= GIT_VERSION_MIN_REQUIRED,
    356354        }
    357355
    358356    def __init__(self, git_dir, log, git_bin='git', git_fs_encoding=None,

Also I wonder if GitCore should do bytesstr conversion, but I don't know the design well enough to decide that.

in reply to:  7 comment:8 by Jun Omae, 3 years ago

Replying to Ryan J Ollos:

I've been thinking it probably doesn't make sense to keep the exception handler when matching the regex. Proposed refactoring:

#!diff...

Seems to be good rather than the patch in comment:4.

Also I wonder if GitCore should do bytesstr conversion, but I don't know the design well enough to decide that.

GitCore must handle bytes data for input/output because the encoding is not always utf-8 and configureable (e.g. i18n.commitEncoding, filesystem encoding, binary contents).

comment:9 by Ryan J Ollos, 3 years ago

I made a few minor revisions and committed the refactoring in r17550.

comment:10 by Ryan J Ollos, 3 years ago

Would it be safer to omit the newline in the regex?

  • tracopt/versioncontrol/git/PyGIT.py

    diff --git a/tracopt/versioncontrol/git/PyGIT.py b/tracopt/versioncontrol/git/PyGIT.py
    index 51a30666fe..8426091060 100644
    a b class Storage(object):  
    339339        #  1.5.6.2
    340340        #  1.5.6.3.230.g2db511
    341341        #  1.5.6.GIT
    342         m = re.match('git version (.*)\n$', version)
     342        m = re.match('git version (.*)$', version)
    343343        if not m:
    344344            raise GitError("Could not retrieve GIT version. "
    345345                           "'%s --version' returned %s"

The regex documentation notes:

$ Matches the end of the string or just before the newline at the end of the string.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Ryan J Ollos 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.