Git version rendered with bytes type representation on System Information table

[Ryan J Ollos]

Looks like this may fix the issue. This change would revised trunk/tracopt/versioncontrol/git/PyGIT.py@17482:17483. I will look at adding test coverage.

tracopt/versioncontrol/git/PyGIT.py

@@ -335 +335 @@
         try:
             g = GitCore(git_bin=git_bin)
             [v] = g.version().splitlines()
-            version = v.strip().split()[2]
+            version = str(v.strip().split()[2], 'utf-8')
             # 'version' has usually at least 3 numeric version
             # components, e.g.::
             #  1.5.4.2
@@ -348 +348 @@
                 except ValueError:
                     return s
 
-            split_version = tuple(map(try_int, version.split(b'.')))
+            split_version = tuple(map(try_int, version.split('.')))
 
             result = {}
             result['v_str'] = version

[Ryan J Ollos]

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

[Jun Omae]

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'

[Ryan J Ollos]

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

[Ryan J Ollos]

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

@@ -332 +332 @@
     @staticmethod
     def git_version(git_bin='git'):
         GIT_VERSION_MIN_REQUIRED = (1, 5, 6)
-        g = GitCore(git_bin=git_bin)
-        # 'version' has usually at least 3 numeric version
-        # components, e.g.::
+        version = str(GitCore(git_bin=git_bin).version(), 'utf-8')
+        # 'version' should have at least 3 numeric version components:
+        #  1.5.6
+        #  1.5.6.windows.1
         #  1.5.6.2
         #  1.5.6.3.230.g2db511
         #  1.5.6.GIT
-        try:
-            v = g.version()
-            m = re.match(b'git version (.*)\n$', v)
-            version = str(m.group(1), 'utf-8')
-            split_version = tuple(as_int(s, s) for s in version.split('.'))
-        except Exception as e:
-            raise GitError("Could not retrieve GIT version (tried to "
-                           "execute/parse '%s --version' but got %s)"
-                           % (git_bin, repr(e)))
+        m = re.match('git version (.*)\n$', version)
+        if not m:
+            raise GitError("Could not retrieve GIT version. "
+                           "'%s --version' returned '%s'" % (git_bin, version))
+        version_str = m.group(1)
+        version_tuple = tuple(as_int(s, s) for s in version_str.split('.'))
         return {
-            'v_str': version,
-            'v_tuple': split_version,
+            'v_str': version_str,
+            'v_tuple': version_tuple,
             'v_min_tuple': GIT_VERSION_MIN_REQUIRED,
             'v_min_str': '.'.join(map(str, GIT_VERSION_MIN_REQUIRED)),
-            'v_compatible': split_version >= GIT_VERSION_MIN_REQUIRED,
+            'v_compatible': version_tuple >= GIT_VERSION_MIN_REQUIRED,
         }
 
     def __init__(self, git_dir, log, git_bin='git', git_fs_encoding=None,

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

[Jun Omae]

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 bytes → str 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).

[Ryan J Ollos]

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

[Ryan J Ollos]

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

tracopt/versioncontrol/git/PyGIT.py

@@ -339 +339 @@
         #  1.5.6.2
         #  1.5.6.3.230.g2db511
         #  1.5.6.GIT
-        m = re.match('git version (.*)\n$', version)
+        m = re.match('git version (.*)$', version)
         if not m:
             raise GitError("Could not retrieve GIT version. "
                            "'%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.