[PATCH 00/13] git-p4: python3 compatibility

This patchset adds python3 compatibility to git-p4.

While some clean-up refactoring would have been nice, I specifically avoided
making any major changes to the internal API, aiming to have passing tests
with as few changes as possible.

CI results can be seen from this GitHub PR: https://github.com/git/git/pull/673

(As of writing, the CI pipelines are intermittently failing due to reasons
that appear unrelated to code. I do have python3 tests passing locally on
a Gentoo host.)

Yang Zhao (13):
  ci: also run linux-gcc pipeline with python-3.7 environment
  git-p4: make python-2.7 the oldest supported version
  git-p4: simplify python version detection
  git-p4: decode response from p4 to str for python3
  git-p4: properly encode/decode communication with git for python 3
  git-p4: convert path to unicode before processing them
  git-p4: open .gitp4-usercache.txt in text mode
  git-p4: use marshal format version 2 when sending to p4
  git-p4: fix freezing while waiting for fast-import progress
  git-p4: use functools.reduce instead of reduce
  git-p4: use dict.items() iteration for python3 compatibility
  git-p4: simplify regex pattern generation for parsing diff-tree
  git-p4: use python3's input() everywhere

 azure-pipelines.yml |  11 +++
 git-p4.py           | 195 ++++++++++++++++++++++++++++----------------
 2 files changed, 136 insertions(+), 70 deletions(-)

-- 
2.21.0.windows.1

git-p4.py includes support for python-3, but this was not previously
validated in CI. Lets actually do that.

There is no tangible benefit to repeating python-3 tests for all
environments, so only limit it to linux-gcc for now.

Signed-off-by: Yang Zhao <redacted>
---

I assert that we don't need to run python3 tests on more platforms,
but is this actually reasonable?

 azure-pipelines.yml | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/azure-pipelines.yml b/azure-pipelines.yml
index 37ed7e06c6..d5f9413248 100644
--- a/azure-pipelines.yml
+++ b/azure-pipelines.yml

@@ -331,7 +331,18 @@ jobs:
   displayName: linux-gcc
   condition: succeeded()
   pool: Hosted Ubuntu 1604
+  strategy:
+    matrix:
+      python27:
+        python.version: '2.7'
+      python37:
+        python.version: '3.7'
   steps:
+  - task: UsePythonVersion@0
+    inputs:
+      versionSpec: '$(python.version)'
+  - bash: |
+      echo "##vso[task.setvariable variable=python_path]$(which python)"
   - bash: |
        test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1
 

-- 
2.21.0.windows.1

Python-2.6 and earlier have been end-of-life'd for many years now, and
we actually already use 2.7-only features in the code. Make the version
check reflect current realities.
---
 git-p4.py | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 60c73b6a37..d8f88884db 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -8,9 +8,8 @@
 # License: MIT <http://www.opensource.org/licenses/mit-license.php>
 #
 import sys
-if sys.hexversion < 0x02040000:
-    # The limiter is the subprocess module
-    sys.stderr.write("git-p4: requires Python 2.4 or later.\n")
+if sys.version_info.major < 3 and sys.version_info.minor < 7:
+    sys.stderr.write("git-p4: requires Python 2.7 or later.\n")
     sys.exit(1)
 import os
 import optparse

-- 
2.21.0.windows.1

Instead of type shenanigans, just check the version object.

Signed-off-by: Yang Zhao <redacted>
---
 git-p4.py | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index d8f88884db..ebeef35a92 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -27,18 +27,9 @@
 import errno
 
 # support basestring in python3
-try:
-    unicode = unicode
-except NameError:
-    # 'unicode' is undefined, must be Python 3
-    str = str
-    unicode = str
-    bytes = bytes
+if sys.version_info.major >= 3:
     basestring = (str,bytes)
 else:
-    # 'unicode' exists, must be Python 2
-    str = str
-    unicode = unicode
     bytes = str
     basestring = basestring
 

-- 
2.21.0.windows.1

The marshalled dict in the response given on STDOUT by p4 uses `str` for
keys and string values. When run using python3, these values are
deserialized as `bytes`, leading to a whole host of problems as the rest
of the code assumes `str` is used throughout.

This patch changes the deserialization behaviour such that, as much as
possible, text output from p4 is decoded to native unicode strings.
Exceptions are made for the field `data` as it is usually arbitrary
binary data. `depotFile[0-9]*`, `path`, and `clientFile` are also exempt
as they contain path information which may not be UTF-8 encoding
compatible, and must survive round-trip back to p4.

Signed-off-by: Yang Zhao <redacted>

SQUASH: use unicode string internally throughout
---
 git-p4.py | 61 ++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 16 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index ebeef35a92..6720c7b24a 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -157,6 +157,19 @@ def die(msg):
         sys.stderr.write(msg + "\n")
         sys.exit(1)
 
+# We need different encoding/decoding strategies for text data being passed
+# around in pipes depending on python version
+if sys.version_info.major >= 3:
+    def decode_text_stream(s):
+        return s.decode() if isinstance(s, bytes) else s
+    def encode_text_stream(s):
+        return s.encode() if isinstance(s, str) else s
+else:
+    def decode_text_stream(s):
+        return s
+    def encode_text_stream(s):
+        return s.encode('utf_8') if isinstance(s, unicode) else s
+
 def write_pipe(c, stdin):
     if verbose:
         sys.stderr.write('Writing pipe: %s\n' % str(c))

@@ -186,7 +199,7 @@ def read_pipe_full(c):
     expand = isinstance(c,basestring)
     p = subprocess.Popen(c, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=expand)
     (out, err) = p.communicate()
-    return (p.returncode, out, err)
+    return (p.returncode, out, decode_text_stream(err))
 
 def read_pipe(c, ignore_error=False):
     """ Read output from  command. Returns the output text on

@@ -209,11 +222,11 @@ def read_pipe_text(c):
     if retcode != 0:
         return None
     else:
-        return out.rstrip()
+        return decode_text_stream(out).rstrip()
 
-def p4_read_pipe(c, ignore_error=False):
+def p4_read_pipe(c, ignore_error=False, raw=False):
     real_cmd = p4_build_cmd(c)
-    return read_pipe(real_cmd, ignore_error)
+    return read_pipe(real_cmd, ignore_error, raw=raw)
 
 def read_pipe_lines(c):
     if verbose:

@@ -222,7 +235,7 @@ def read_pipe_lines(c):
     expand = isinstance(c, basestring)
     p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
     pipe = p.stdout
-    val = pipe.readlines()
+    val = [decode_text_stream(line) for line in pipe.readlines()]
     if pipe.close() or p.wait():
         die('Command failed: %s' % str(c))
 

@@ -253,6 +266,7 @@ def p4_has_move_command():
     cmd = p4_build_cmd(["move", "-k", "@from", "@to"])
     p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
     (out, err) = p.communicate()
+    err = decode_text_stream(err)
     # return code will be 1 in either case
     if err.find("Invalid option") >= 0:
         return False

@@ -633,6 +647,20 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
     try:
         while True:
             entry = marshal.load(p4.stdout)
+            if bytes is not str:
+                # Decode unmarshalled dict to use str keys and values, except for:
+                #   - `data` which may contain arbitrary binary data
+                #   - `depotFile[0-9]*`, `path`, or `clientFile` which may contain non-UTF8 encoded text
+                decoded_entry = {}
+                for key, value in entry.items():
+                    key = key.decode()
+                    if isinstance(value, bytes) and not (key in ('data', 'path', 'clientFile') or key.startswith('depotFile')):
+                        value = value.decode()
+                    decoded_entry[key] = value
+                # Parse out data if it's an error response
+                if decoded_entry.get('code') == 'error' and 'data' in decoded_entry:
+                    decoded_entry['data'] = decoded_entry['data'].decode()
+                entry = decoded_entry
             if skip_info:
                 if 'code' in entry and entry['code'] == 'info':
                     continue

@@ -850,6 +878,7 @@ def branch_exists(branch):
     cmd = [ "git", "rev-parse", "--symbolic", "--verify", branch ]
     p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
     out, _ = p.communicate()
+    out = decode_text_stream(out)
     if p.returncode:
         return False
     # expect exactly one line of output: the branch name

@@ -1993,7 +2022,7 @@ def applyCommit(self, id):
         tmpFile = os.fdopen(handle, "w+b")
         if self.isWindows:
             submitTemplate = submitTemplate.replace("\n", "\r\n")
-        tmpFile.write(submitTemplate)
+        tmpFile.write(encode_text_stream(submitTemplate))
         tmpFile.close()
 
         if self.prepare_p4_only:

@@ -2040,11 +2069,11 @@ def applyCommit(self, id):
             if self.edit_template(fileName):
                 # read the edited message and submit
                 tmpFile = open(fileName, "rb")
-                message = tmpFile.read()
+                message = decode_text_stream(tmpFile.read())
                 tmpFile.close()
                 if self.isWindows:
                     message = message.replace("\r\n", "\n")
-                submitTemplate = message[:message.index(separatorLine)]
+                submitTemplate = encode_text_stream(message[:message.index(separatorLine)])
 
                 if update_shelve:
                     p4_write_pipe(['shelve', '-r', '-i'], submitTemplate)

@@ -2145,7 +2174,7 @@ def exportGitTags(self, gitTags):
                 print("Not creating p4 label %s for tag due to option" \
                       " --prepare-p4-only" % name)
             else:
-                p4_write_pipe(["label", "-i"], labelTemplate)
+                p4_write_pipe(["label", "-i"], encode_text_stream(labelTemplate))
 
                 # Use the label
                 p4_system(["tag", "-l", name] +

@@ -2469,7 +2498,7 @@ def append(self, view_line):
 
     def convert_client_path(self, clientFile):
         # chop off //client/ part to make it relative
-        if not clientFile.startswith(self.client_prefix):
+        if not decode_path(clientFile).startswith(self.client_prefix):
             die("No prefix '%s' on clientFile '%s'" %
                 (self.client_prefix, clientFile))
         return clientFile[len(self.client_prefix):]

@@ -2729,7 +2758,7 @@ def splitFilesIntoBranches(self, commit):
         return branches
 
     def writeToGitStream(self, gitMode, relPath, contents):
-        self.gitStream.write('M %s inline %s\n' % (gitMode, relPath))
+        self.gitStream.write(encode_text_stream(u'M {} inline {}\n'.format(gitMode, relPath)))
         self.gitStream.write('data %d\n' % sum(len(d) for d in contents))
         for d in contents:
             self.gitStream.write(d)

@@ -2770,7 +2799,7 @@ def streamOneP4File(self, file, contents):
             git_mode = "120000"
             # p4 print on a symlink sometimes contains "target\n";
             # if it does, remove the newline
-            data = ''.join(contents)
+            data = ''.join(decode_text_stream(c) for c in contents)
             if not data:
                 # Some version of p4 allowed creating a symlink that pointed
                 # to nothing.  This causes p4 errors when checking out such

@@ -2824,7 +2853,7 @@ def streamOneP4File(self, file, contents):
         pattern = p4_keywords_regexp_for_type(type_base, type_mods)
         if pattern:
             regexp = re.compile(pattern, re.VERBOSE)
-            text = ''.join(contents)
+            text = ''.join(decode_text_stream(c) for c in contents)
             text = regexp.sub(r'$\1$', text)
             contents = [ text ]
 

@@ -2839,7 +2868,7 @@ def streamOneP4Deletion(self, file):
         if verbose:
             sys.stdout.write("delete %s\n" % relPath)
             sys.stdout.flush()
-        self.gitStream.write("D %s\n" % relPath)
+        self.gitStream.write(encode_text_stream(u'D {}\n'.format(relPath)))
 
         if self.largeFileSystem and self.largeFileSystem.isLargeFile(relPath):
             self.largeFileSystem.removeLargeFile(relPath)

@@ -2939,9 +2968,9 @@ def streamP4FilesCbSelf(entry):
                 if 'shelved_cl' in f:
                     # Handle shelved CLs using the "p4 print file@=N" syntax to print
                     # the contents
-                    fileArg = '%s@=%d' % (f['path'], f['shelved_cl'])
+                    fileArg = f['path'] + encode_text_stream('@={}'.format(f['shelved_cl']))
                 else:
-                    fileArg = '%s#%s' % (f['path'], f['rev'])
+                    fileArg = f['path'] + encode_text_stream('#{}'.format(f['rev']))
 
                 fileArgs.append(fileArg)
 

-- 
2.21.0.windows.1

Under python3, calls to write() on the stream to `git fast-import` must
be encoded.  This patch wraps the IO object such that this encoding is
done transparently.

Conversely, any text data read from subprocesses must also be decoded
before running through the rest of the pipeline.

Signed-off-by: Yang Zhao <redacted>
---
 git-p4.py | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 6720c7b24a..fefa716b17 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -201,10 +201,12 @@ def read_pipe_full(c):
     (out, err) = p.communicate()
     return (p.returncode, out, decode_text_stream(err))
 
-def read_pipe(c, ignore_error=False):
+def read_pipe(c, ignore_error=False, raw=False):
     """ Read output from  command. Returns the output text on
         success. On failure, terminates execution, unless
         ignore_error is True, when it returns an empty string.
+
+        If raw is True, do not attempt to decode output text.
     """
     (retcode, out, err) = read_pipe_full(c)
     if retcode != 0:

@@ -212,6 +214,8 @@ def read_pipe(c, ignore_error=False):
             out = ""
         else:
             die('Command failed: %s\nError: %s' % (str(c), err))
+    if not raw:
+        out = decode_text_stream(out)
     return out
 
 def read_pipe_text(c):

@@ -238,7 +242,6 @@ def read_pipe_lines(c):
     val = [decode_text_stream(line) for line in pipe.readlines()]
     if pipe.close() or p.wait():
         die('Command failed: %s' % str(c))
-
     return val
 
 def p4_read_pipe_lines(c):

@@ -634,7 +637,8 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
             stdin_file.write(stdin)
         else:
             for i in stdin:
-                stdin_file.write(i + '\n')
+                stdin_file.write(encode_text_stream(i))
+                stdin_file.write(b'\n')
         stdin_file.flush()
         stdin_file.seek(0)
 

@@ -3556,6 +3560,15 @@ def openStreams(self):
         self.gitStream = self.importProcess.stdin
         self.gitError = self.importProcess.stderr
 
+        if bytes is not str:
+            # Wrap gitStream.write() so that it can be called using `str` arguments
+            def make_encoded_write(write):
+                def encoded_write(s):
+                    return write(s.encode() if isinstance(s, str) else s)
+                return encoded_write
+
+            self.gitStream.write = make_encoded_write(self.gitStream.write)
+
     def closeStreams(self):
         self.gitStream.close()
         if self.importProcess.wait() != 0:

-- 
2.21.0.windows.1

Opening .gitp4-usercache.txt in text mode makes python 3 happy without
explicitly adding encoding and decoding.

Signed-off-by: Yang Zhao <redacted>
---
 git-p4.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index c7d543b18e..bd3118e0e8 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -1402,14 +1402,14 @@ def getUserMapFromPerforceServer(self):
         for (key, val) in self.users.items():
             s += "%s\t%s\n" % (key.expandtabs(1), val.expandtabs(1))
 
-        open(self.getUserCacheFilename(), "wb").write(s)
+        open(self.getUserCacheFilename(), 'w').write(s)
         self.userMapFromPerforceServer = True
 
     def loadUserMapFromCache(self):
         self.users = {}
         self.userMapFromPerforceServer = False
         try:
-            cache = open(self.getUserCacheFilename(), "rb")
+            cache = open(self.getUserCacheFilename(), 'r')
             lines = cache.readlines()
             cache.close()
             for line in lines:

-- 
2.21.0.windows.1

P4 allows essentially arbitrary encoding for path data while we would
perfer to be dealing only with unicode strings.  Since path data need to
survive round-trip back to p4, this patch implements the general policy
that we store path data as-is, but decode them to unicode before doing
any non-trivial processing.

A new `decode_path()` method is provided that generally does the correct
conversion, taking into account `git-p4.pathEncoding` configuration.

For python2.7, path strings will be left as-is if it only contains ASCII
characters.

For python3, decoding is always done so that we have str objects.

Signed-off-by: Yang Zhao <redacted>
---
 git-p4.py | 67 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 24 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index fefa716b17..088924fbe1 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -170,6 +170,21 @@ def decode_text_stream(s):
     def encode_text_stream(s):
         return s.encode('utf_8') if isinstance(s, unicode) else s
 
+def decode_path(path):
+    """Decode a given string (bytes or otherwise) using configured path encoding options
+    """
+    encoding = gitConfig('git-p4.pathEncoding') or 'utf_8'
+    if bytes is not str:
+        return path.decode(encoding, errors='replace') if isinstance(path, bytes) else path
+    else:
+        try:
+            path.decode('ascii')
+        except:
+            path = path.decode(encoding, errors='replace')
+            if verbose:
+                print('Path with non-ASCII characters detected. Used {} to decode: {}'.format(encoding, path))
+        return path
+
 def write_pipe(c, stdin):
     if verbose:
         sys.stderr.write('Writing pipe: %s\n' % str(c))

@@ -715,7 +730,8 @@ def p4Where(depotPath):
         if "depotFile" in entry:
             # Search for the base client side depot path, as long as it starts with the branch's P4 path.
             # The base path always ends with "/...".
-            if entry["depotFile"].find(depotPath) == 0 and entry["depotFile"][-4:] == "/...":
+            entry_path = decode_path(entry['depotFile'])
+            if entry_path.find(depotPath) == 0 and entry_path[-4:] == "/...":
                 output = entry
                 break
         elif "data" in entry:

@@ -730,11 +746,11 @@ def p4Where(depotPath):
         return ""
     clientPath = ""
     if "path" in output:
-        clientPath = output.get("path")
+        clientPath = decode_path(output['path'])
     elif "data" in output:
         data = output.get("data")
-        lastSpace = data.rfind(" ")
-        clientPath = data[lastSpace + 1:]
+        lastSpace = data.rfind(b" ")
+        clientPath = decode_path(data[lastSpace + 1:])
 
     if clientPath.endswith("..."):
         clientPath = clientPath[:-3]

@@ -2511,7 +2527,7 @@ def update_client_spec_path_cache(self, files):
         """ Caching file paths by "p4 where" batch query """
 
         # List depot file paths exclude that already cached
-        fileArgs = [f['path'] for f in files if f['path'] not in self.client_spec_path_cache]
+        fileArgs = [f['path'] for f in files if decode_path(f['path']) not in self.client_spec_path_cache]
 
         if len(fileArgs) == 0:
             return  # All files in cache

@@ -2526,16 +2542,18 @@ def update_client_spec_path_cache(self, files):
             if "unmap" in res:
                 # it will list all of them, but only one not unmap-ped
                 continue
+            depot_path = decode_path(res['depotFile'])
             if gitConfigBool("core.ignorecase"):
-                res['depotFile'] = res['depotFile'].lower()
-            self.client_spec_path_cache[res['depotFile']] = self.convert_client_path(res["clientFile"])
+                depot_path = depot_path.lower()
+            self.client_spec_path_cache[depot_path] = self.convert_client_path(res["clientFile"])
 
         # not found files or unmap files set to ""
         for depotFile in fileArgs:
+            depotFile = decode_path(depotFile)
             if gitConfigBool("core.ignorecase"):
                 depotFile = depotFile.lower()
             if depotFile not in self.client_spec_path_cache:
-                self.client_spec_path_cache[depotFile] = ""
+                self.client_spec_path_cache[depotFile] = b''
 
     def map_in_client(self, depot_path):
         """Return the relative location in the client where this

@@ -2653,7 +2671,7 @@ def isPathWanted(self, path):
             elif path.lower() == p.lower():
                 return False
         for p in self.depotPaths:
-            if p4PathStartsWith(path, p):
+            if p4PathStartsWith(path, decode_path(p)):
                 return True
         return False
 

@@ -2662,7 +2680,7 @@ def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0):
         fnum = 0
         while "depotFile%s" % fnum in commit:
             path =  commit["depotFile%s" % fnum]
-            found = self.isPathWanted(path)
+            found = self.isPathWanted(decode_path(path))
             if not found:
                 fnum = fnum + 1
                 continue

@@ -2696,7 +2714,7 @@ def stripRepoPath(self, path, prefixes):
         if self.useClientSpec:
             # branch detection moves files up a level (the branch name)
             # from what client spec interpretation gives
-            path = self.clientSpecDirs.map_in_client(path)
+            path = decode_path(self.clientSpecDirs.map_in_client(path))
             if self.detectBranches:
                 for b in self.knownBranches:
                     if p4PathStartsWith(path, b + "/"):

@@ -2730,14 +2748,15 @@ def splitFilesIntoBranches(self, commit):
         branches = {}
         fnum = 0
         while "depotFile%s" % fnum in commit:
-            path =  commit["depotFile%s" % fnum]
+            raw_path = commit["depotFile%s" % fnum]
+            path = decode_path(raw_path)
             found = self.isPathWanted(path)
             if not found:
                 fnum = fnum + 1
                 continue
 
             file = {}
-            file["path"] = path
+            file["path"] = raw_path
             file["rev"] = commit["rev%s" % fnum]
             file["action"] = commit["action%s" % fnum]
             file["type"] = commit["type%s" % fnum]

@@ -2746,7 +2765,7 @@ def splitFilesIntoBranches(self, commit):
             # start with the full relative path where this file would
             # go in a p4 client
             if self.useClientSpec:
-                relPath = self.clientSpecDirs.map_in_client(path)
+                relPath = decode_path(self.clientSpecDirs.map_in_client(path))
             else:
                 relPath = self.stripRepoPath(path, self.depotPaths)
 

@@ -2784,14 +2803,15 @@ def encodeWithUTF8(self, path):
     # - helper for streamP4Files
 
     def streamOneP4File(self, file, contents):
-        relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
-        relPath = self.encodeWithUTF8(relPath)
+        file_path = file['depotFile']
+        relPath = self.stripRepoPath(decode_path(file_path), self.branchPrefixes)
+
         if verbose:
             if 'fileSize' in self.stream_file:
                 size = int(self.stream_file['fileSize'])
             else:
                 size = 0 # deleted files don't get a fileSize apparently
-            sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], relPath, size/1024/1024))
+            sys.stdout.write('\r%s --> %s (%i MB)\n' % (file_path, relPath, size/1024/1024))
             sys.stdout.flush()
 
         (type_base, type_mods) = split_p4_type(file["type"])

@@ -2809,7 +2829,7 @@ def streamOneP4File(self, file, contents):
                 # to nothing.  This causes p4 errors when checking out such
                 # a change, and errors here too.  Work around it by ignoring
                 # the bad symlink; hopefully a future change fixes it.
-                print("\nIgnoring empty symlink in %s" % file['depotFile'])
+                print("\nIgnoring empty symlink in %s" % file_path)
                 return
             elif data[-1] == '\n':
                 contents = [data[:-1]]

@@ -2828,7 +2848,7 @@ def streamOneP4File(self, file, contents):
             # just the native "NT" type.
             #
             try:
-                text = p4_read_pipe(['print', '-q', '-o', '-', '%s@%s' % (file['depotFile'], file['change'])])
+                text = p4_read_pipe(['print', '-q', '-o', '-', '%s@%s' % (decode_path(file['depotFile']), file['change'])], raw=True)
             except Exception as e:
                 if 'Translation of file content failed' in str(e):
                     type_base = 'binary'

@@ -2836,7 +2856,7 @@ def streamOneP4File(self, file, contents):
                     raise e
             else:
                 if p4_version_string().find('/NT') >= 0:
-                    text = text.replace('\r\n', '\n')
+                    text = text.replace(b'\r\n', b'\n')
                 contents = [ text ]
 
         if type_base == "apple":

@@ -2867,8 +2887,7 @@ def streamOneP4File(self, file, contents):
         self.writeToGitStream(git_mode, relPath, contents)
 
     def streamOneP4Deletion(self, file):
-        relPath = self.stripRepoPath(file['path'], self.branchPrefixes)
-        relPath = self.encodeWithUTF8(relPath)
+        relPath = self.stripRepoPath(decode_path(file['path']), self.branchPrefixes)
         if verbose:
             sys.stdout.write("delete %s\n" % relPath)
             sys.stdout.flush()

@@ -3055,8 +3074,8 @@ def commit(self, details, files, branch, parent = "", allow_empty=False):
         if self.clientSpecDirs:
             self.clientSpecDirs.update_client_spec_path_cache(files)
 
-        files = [f for f in files
-            if self.inClientSpec(f['path']) and self.hasBranchPrefix(f['path'])]
+        files = [f for (f, path) in ((f, decode_path(f['path'])) for f in files)
+            if self.inClientSpec(path) and self.hasBranchPrefix(path)]
 
         if gitConfigBool('git-p4.keepEmptyCommits'):
             allow_empty = True

-- 
2.21.0.windows.1

P4 allows essentially arbitrary encoding for path data while we would
perfer to be dealing only with unicode strings.  Since path data need to
survive round-trip back to p4, this patch implements the general policy
that we store path data as-is, but decode them to unicode before doing
any non-trivial processing.

A new `decode_path()` method is provided that generally does the correct
conversion, taking into account `git-p4.pathEncoding` configuration.

For python2.7, path strings will be left as-is if it only contains ASCII
characters.

For python3, decoding is always done so that we have str objects.

Signed-off-by: Yang Zhao <redacted>
---
 git-p4.py | 67 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 43 insertions(+), 24 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index bd3118e0e8..650c11eb62 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -170,6 +170,21 @@ def decode_text_stream(s):
     def encode_text_stream(s):
         return s.encode('utf_8') if isinstance(s, unicode) else s
 
+def decode_path(path):
+    """Decode a given string (bytes or otherwise) using configured path encoding options
+    """
+    encoding = gitConfig('git-p4.pathEncoding') or 'utf_8'
+    if bytes is not str:
+        return path.decode(encoding, errors='replace') if isinstance(path, bytes) else path
+    else:
+        try:
+            path.decode('ascii')
+        except:
+            path = path.decode(encoding, errors='replace')
+            if verbose:
+                print('Path with non-ASCII characters detected. Used {} to decode: {}'.format(encoding, path))
+        return path
+
 def write_pipe(c, stdin):
     if verbose:
         sys.stderr.write('Writing pipe: %s\n' % str(c))

@@ -720,7 +735,8 @@ def p4Where(depotPath):
         if "depotFile" in entry:
             # Search for the base client side depot path, as long as it starts with the branch's P4 path.
             # The base path always ends with "/...".
-            if entry["depotFile"].find(depotPath) == 0 and entry["depotFile"][-4:] == "/...":
+            entry_path = decode_path(entry['depotFile'])
+            if entry_path.find(depotPath) == 0 and entry_path[-4:] == "/...":
                 output = entry
                 break
         elif "data" in entry:

@@ -735,11 +751,11 @@ def p4Where(depotPath):
         return ""
     clientPath = ""
     if "path" in output:
-        clientPath = output.get("path")
+        clientPath = decode_path(output['path'])
     elif "data" in output:
         data = output.get("data")
-        lastSpace = data.rfind(" ")
-        clientPath = data[lastSpace + 1:]
+        lastSpace = data.rfind(b" ")
+        clientPath = decode_path(data[lastSpace + 1:])
 
     if clientPath.endswith("..."):
         clientPath = clientPath[:-3]

@@ -2516,7 +2532,7 @@ def update_client_spec_path_cache(self, files):
         """ Caching file paths by "p4 where" batch query """
 
         # List depot file paths exclude that already cached
-        fileArgs = [f['path'] for f in files if f['path'] not in self.client_spec_path_cache]
+        fileArgs = [f['path'] for f in files if decode_path(f['path']) not in self.client_spec_path_cache]
 
         if len(fileArgs) == 0:
             return  # All files in cache

@@ -2531,16 +2547,18 @@ def update_client_spec_path_cache(self, files):
             if "unmap" in res:
                 # it will list all of them, but only one not unmap-ped
                 continue
+            depot_path = decode_path(res['depotFile'])
             if gitConfigBool("core.ignorecase"):
-                res['depotFile'] = res['depotFile'].lower()
-            self.client_spec_path_cache[res['depotFile']] = self.convert_client_path(res["clientFile"])
+                depot_path = depot_path.lower()
+            self.client_spec_path_cache[depot_path] = self.convert_client_path(res["clientFile"])
 
         # not found files or unmap files set to ""
         for depotFile in fileArgs:
+            depotFile = decode_path(depotFile)
             if gitConfigBool("core.ignorecase"):
                 depotFile = depotFile.lower()
             if depotFile not in self.client_spec_path_cache:
-                self.client_spec_path_cache[depotFile] = ""
+                self.client_spec_path_cache[depotFile] = b''
 
     def map_in_client(self, depot_path):
         """Return the relative location in the client where this

@@ -2658,7 +2676,7 @@ def isPathWanted(self, path):
             elif path.lower() == p.lower():
                 return False
         for p in self.depotPaths:
-            if p4PathStartsWith(path, p):
+            if p4PathStartsWith(path, decode_path(p)):
                 return True
         return False
 

@@ -2667,7 +2685,7 @@ def extractFilesFromCommit(self, commit, shelved=False, shelved_cl = 0):
         fnum = 0
         while "depotFile%s" % fnum in commit:
             path =  commit["depotFile%s" % fnum]
-            found = self.isPathWanted(path)
+            found = self.isPathWanted(decode_path(path))
             if not found:
                 fnum = fnum + 1
                 continue

@@ -2701,7 +2719,7 @@ def stripRepoPath(self, path, prefixes):
         if self.useClientSpec:
             # branch detection moves files up a level (the branch name)
             # from what client spec interpretation gives
-            path = self.clientSpecDirs.map_in_client(path)
+            path = decode_path(self.clientSpecDirs.map_in_client(path))
             if self.detectBranches:
                 for b in self.knownBranches:
                     if p4PathStartsWith(path, b + "/"):

@@ -2735,14 +2753,15 @@ def splitFilesIntoBranches(self, commit):
         branches = {}
         fnum = 0
         while "depotFile%s" % fnum in commit:
-            path =  commit["depotFile%s" % fnum]
+            raw_path = commit["depotFile%s" % fnum]
+            path = decode_path(raw_path)
             found = self.isPathWanted(path)
             if not found:
                 fnum = fnum + 1
                 continue
 
             file = {}
-            file["path"] = path
+            file["path"] = raw_path
             file["rev"] = commit["rev%s" % fnum]
             file["action"] = commit["action%s" % fnum]
             file["type"] = commit["type%s" % fnum]

@@ -2751,7 +2770,7 @@ def splitFilesIntoBranches(self, commit):
             # start with the full relative path where this file would
             # go in a p4 client
             if self.useClientSpec:
-                relPath = self.clientSpecDirs.map_in_client(path)
+                relPath = decode_path(self.clientSpecDirs.map_in_client(path))
             else:
                 relPath = self.stripRepoPath(path, self.depotPaths)
 

@@ -2789,14 +2808,15 @@ def encodeWithUTF8(self, path):
     # - helper for streamP4Files
 
     def streamOneP4File(self, file, contents):
-        relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
-        relPath = self.encodeWithUTF8(relPath)
+        file_path = file['depotFile']
+        relPath = self.stripRepoPath(decode_path(file_path), self.branchPrefixes)
+
         if verbose:
             if 'fileSize' in self.stream_file:
                 size = int(self.stream_file['fileSize'])
             else:
                 size = 0 # deleted files don't get a fileSize apparently
-            sys.stdout.write('\r%s --> %s (%i MB)\n' % (file['depotFile'], relPath, size/1024/1024))
+            sys.stdout.write('\r%s --> %s (%i MB)\n' % (file_path, relPath, size/1024/1024))
             sys.stdout.flush()
 
         (type_base, type_mods) = split_p4_type(file["type"])

@@ -2814,7 +2834,7 @@ def streamOneP4File(self, file, contents):
                 # to nothing.  This causes p4 errors when checking out such
                 # a change, and errors here too.  Work around it by ignoring
                 # the bad symlink; hopefully a future change fixes it.
-                print("\nIgnoring empty symlink in %s" % file['depotFile'])
+                print("\nIgnoring empty symlink in %s" % file_path)
                 return
             elif data[-1] == '\n':
                 contents = [data[:-1]]

@@ -2833,7 +2853,7 @@ def streamOneP4File(self, file, contents):
             # just the native "NT" type.
             #
             try:
-                text = p4_read_pipe(['print', '-q', '-o', '-', '%s@%s' % (file['depotFile'], file['change'])])
+                text = p4_read_pipe(['print', '-q', '-o', '-', '%s@%s' % (decode_path(file['depotFile']), file['change'])], raw=True)
             except Exception as e:
                 if 'Translation of file content failed' in str(e):
                     type_base = 'binary'

@@ -2841,7 +2861,7 @@ def streamOneP4File(self, file, contents):
                     raise e
             else:
                 if p4_version_string().find('/NT') >= 0:
-                    text = text.replace('\r\n', '\n')
+                    text = text.replace(b'\r\n', b'\n')
                 contents = [ text ]
 
         if type_base == "apple":

@@ -2872,8 +2892,7 @@ def streamOneP4File(self, file, contents):
         self.writeToGitStream(git_mode, relPath, contents)
 
     def streamOneP4Deletion(self, file):
-        relPath = self.stripRepoPath(file['path'], self.branchPrefixes)
-        relPath = self.encodeWithUTF8(relPath)
+        relPath = self.stripRepoPath(decode_path(file['path']), self.branchPrefixes)
         if verbose:
             sys.stdout.write("delete %s\n" % relPath)
             sys.stdout.flush()

@@ -3060,8 +3079,8 @@ def commit(self, details, files, branch, parent = "", allow_empty=False):
         if self.clientSpecDirs:
             self.clientSpecDirs.update_client_spec_path_cache(files)
 
-        files = [f for f in files
-            if self.inClientSpec(f['path']) and self.hasBranchPrefix(f['path'])]
+        files = [f for (f, path) in ((f, decode_path(f['path'])) for f in files)
+            if self.inClientSpec(path) and self.hasBranchPrefix(path)]
 
         if gitConfigBool('git-p4.keepEmptyCommits'):
             allow_empty = True

-- 
2.21.0.windows.1

Opening .gitp4-usercache.txt in text mode makes python 3 happy without
explicitly adding encoding and decoding.

Signed-off-by: Yang Zhao <redacted>
---
 git-p4.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 088924fbe1..af563cf23d 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -1413,14 +1413,14 @@ def getUserMapFromPerforceServer(self):
         for (key, val) in self.users.items():
             s += "%s\t%s\n" % (key.expandtabs(1), val.expandtabs(1))
 
-        open(self.getUserCacheFilename(), "wb").write(s)
+        open(self.getUserCacheFilename(), 'w').write(s)
         self.userMapFromPerforceServer = True
 
     def loadUserMapFromCache(self):
         self.users = {}
         self.userMapFromPerforceServer = False
         try:
-            cache = open(self.getUserCacheFilename(), "rb")
+            cache = open(self.getUserCacheFilename(), 'r')
             lines = cache.readlines()
             cache.close()
             for line in lines:

-- 
2.21.0.windows.1

As part of its importing process, git-p4 sends a `checkpoint` followed
immediately by `progress` to fast-import in to force synchronization.
Due to buffering, it is possible for the `progress` command to not be
flushed before git-p4 proceeds to wait for the corresponding response.
This causes the script to freeze completely, and is consistently
observable at least on python-3.6.9.

Make sure this command sequence is completely flushed before waiting.

Signed-off-by: Yang Zhao <redacted>
---
 git-p4.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-p4.py b/git-p4.py
index c2a3de59e7..1007b936c8 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -2659,6 +2659,7 @@ def __init__(self):
     def checkpoint(self):
         self.gitStream.write("checkpoint\n\n")
         self.gitStream.write("progress checkpoint\n\n")
+        self.gitStream.flush()
         out = self.gitOutput.readline()
         if self.verbose:
             print("checkpoint finished: " + out)

-- 
2.21.0.windows.1

p4 does not appear to understand marshal format version 3 and above.
Version 2 was the latest supported by python-2.7.

Signed-off-by: Yang Zhao <redacted>
---
 git-p4.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index af563cf23d..c2a3de59e7 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -1697,7 +1697,8 @@ def modifyChangelistUser(self, changelist, newUser):
         c = changes[0]
         if c['User'] == newUser: return   # nothing to do
         c['User'] = newUser
-        input = marshal.dumps(c)
+        # p4 does not understand format version 3 and above
+        input = marshal.dumps(c, 2)
 
         result = p4CmdList("change -f -i", stdin=input)
         for r in result:

-- 
2.21.0.windows.1

For python3, reduce() has been moved to functools.reduce().  This is
also available in python2.7.

Signed-off-by: Yang Zhao <redacted>
---
 git-p4.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index 1007b936c8..c888e4825a 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -13,6 +13,7 @@
     sys.exit(1)
 import os
 import optparse
+import functools
 import marshal
 import subprocess
 import tempfile

@@ -1176,7 +1177,7 @@ def pushFile(self, localLargeFile):
         assert False, "Method 'pushFile' required in " + self.__class__.__name__
 
     def hasLargeFileExtension(self, relPath):
-        return reduce(
+        return functools.reduce(
             lambda a, b: a or b,
             [relPath.endswith('.' + e) for e in gitConfigList('git-p4.largeFileExtensions')],
             False

-- 
2.21.0.windows.1

Python3 uses dict.items() instead of .iteritems() to provide
iteratoration over dict.  Although items() is technically less efficient
for python2.7 (allocates a new list instead of simply iterating), the
amount of data involved is very small and the penalty negligible.

Signed-off-by: Yang Zhao <redacted>
---
 git-p4.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-p4.py b/git-p4.py
index c888e4825a..867a8d42ef 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -1763,7 +1763,7 @@ def prepareSubmitTemplate(self, changelist=None):
                 break
         if not change_entry:
             die('Failed to decode output of p4 change -o')
-        for key, value in change_entry.iteritems():
+        for key, value in change_entry.items():
             if key.startswith('File'):
                 if 'depot-paths' in settings:
                     if not [p for p in settings['depot-paths']

-- 
2.21.0.windows.1

Python3 deprecates raw_input() from 2.7 and replaced it with input().
Since we do not need 2.7's input() semantics, `raw_input()` is aliased
to `input()` for easy forward compatability.

Signed-off-by: Yang Zhao <redacted>
---
 git-p4.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index f975f197a5..97a9def657 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -33,6 +33,8 @@
 else:
     bytes = str
     basestring = basestring
+    # We want python3's input() semantics
+    input = raw_input
 
 try:
     from subprocess import CalledProcessError

@@ -1819,7 +1821,7 @@ def edit_template(self, template_file):
             return True
 
         while True:
-            response = raw_input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
+            response = input("Submit template unchanged. Submit anyway? [y]es, [n]o (skip this patch) ")
             if response == 'y':
                 return True
             if response == 'n':

@@ -2390,7 +2392,7 @@ def run(self, args):
                         # prompt for what to do, or use the option/variable
                         if self.conflict_behavior == "ask":
                             print("What do you want to do?")
-                            response = raw_input("[s]kip this commit but apply"
+                            response = input("[s]kip this commit but apply"
                                                  " the rest, or [q]uit? ")
                             if not response:
                                 continue

-- 
2.21.0.windows.1

It is not clear why a generator was used to create the regex used to
parse git-diff-tree output; I assume an early implementation required
it, but is not part of the mainline change.

Simply use a lazily initialized global instead.

Signed-off-by: Yang Zhao <redacted>
---
 git-p4.py | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 867a8d42ef..f975f197a5 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -562,12 +562,7 @@ def getGitTags():
         gitTags.add(tag)
     return gitTags
 
-def diffTreePattern():
-    # This is a simple generator for the diff tree regex pattern. This could be
-    # a class variable if this and parseDiffTreeEntry were a part of a class.
-    pattern = re.compile(':(\d+) (\d+) (\w+) (\w+) ([A-Z])(\d+)?\t(.*?)((\t(.*))|$)')
-    while True:
-        yield pattern
+_diff_tree_pattern = None
 
 def parseDiffTreeEntry(entry):
     """Parses a single diff tree entry into its component elements.

@@ -588,7 +583,11 @@ def parseDiffTreeEntry(entry):
 
     If the pattern is not matched, None is returned."""
 
-    match = diffTreePattern().next().match(entry)
+    global _diff_tree_pattern
+    if not _diff_tree_pattern:
+        _diff_tree_pattern = re.compile(':(\d+) (\d+) (\w+) (\w+) ([A-Z])(\d+)?\t(.*?)((\t(.*))|$)')
+
+    match = _diff_tree_pattern.match(entry)
     if match:
         return {
             'src_mode': match.group(1),

-- 
2.21.0.windows.1

Hi Yang,

On Fri, Dec 06, 2019 at 04:33:18PM -0800, Yang Zhao wrote:

This patchset adds python3 compatibility to git-p4.

While some clean-up refactoring would have been nice, I specifically avoided
making any major changes to the internal API, aiming to have passing tests
with as few changes as possible.

CI results can be seen from this GitHub PR: https://github.com/git/git/pull/673

(As of writing, the CI pipelines are intermittently failing due to reasons
that appear unrelated to code. I do have python3 tests passing locally on
a Gentoo host.)

Currently, there's a competing effort to do the same thing[1] by Ben
Keene (CC'd). Like the last time[2] two competing topics arose at the
same time, I'm going to make the same suggestion.

Would it be possible for both of you to join forces?

Thanks,

Denton

[1]: https://lore.kernel.org/git/pull.463.v4.git.1575498577.gitgitgadget@gmail.com/
[2]: https://lore.kernel.org/git/xmqq5zs7oexn.fsf@gitster-ct.c.googlers.com/

On Fri, Dec 6, 2019 at 5:09 PM Denton Liu [off-list ref] wrote:

On Fri, Dec 06, 2019 at 04:33:18PM -0800, Yang Zhao wrote:

quoted

This patchset adds python3 compatibility to git-p4.
...

Currently, there's a competing effort to do the same thing[1] by Ben
Keene (CC'd). Like the last time[2] two competing topics arose at the
same time, I'm going to make the same suggestion.

Would it be possible for both of you to join forces?

Yes, I do believe we are aware of each other's efforts. I had submitted
an RFC patch set around the time Ben was preparing his own patchset.

I have not reviewed Ben's first patchset as I did not feel that I understood
the systems well enough at the time. I've briefly skimmed through Ben's latest
iteration and it would appear the general approach is very similar, but there's
more added abstractions and just general code change in his version.

Regardless, I'm open to working together.

Ideally, I would prefer we land something minimal and working in mainline soon,
then further collaborate on changes that clean up code and enable more features.

My end-game is to have P4 Streams working in git-p4, and maybe LFS-like support
that uses p4 as the backend. It would be great to not be the only one
spending effort
in that direction.

Yang

On Fri, Dec 6, 2019 at 4:33 PM Yang Zhao [off-list ref] wrote:

This patchset adds python3 compatibility to git-p4.

While some clean-up refactoring would have been nice, I specifically avoided
making any major changes to the internal API, aiming to have passing tests
with as few changes as possible.

CI results can be seen from this GitHub PR: https://github.com/git/git/pull/673

Looks like p4 LFS tests are failing for python3.  Looks like it's just
more bytes vs str.

Will have to enable LFS tests in my own environment.

-- 
Yang Zhao

On 12/7/2019 2:29 AM, Yang Zhao wrote:

On Fri, Dec 6, 2019 at 5:09 PM Denton Liu [off-list ref] wrote:

quoted

On Fri, Dec 06, 2019 at 04:33:18PM -0800, Yang Zhao wrote:

quoted

This patchset adds python3 compatibility to git-p4.
...

Currently, there's a competing effort to do the same thing[1] by Ben
Keene (CC'd). Like the last time[2] two competing topics arose at the
same time, I'm going to make the same suggestion.

Would it be possible for both of you to join forces?

Yes, I do believe we are aware of each other's efforts. I had submitted
an RFC patch set around the time Ben was preparing his own patchset.
I have not reviewed Ben's first patchset as I did not feel that I understood
the systems well enough at the time. I've briefly skimmed through Ben's latest
iteration and it would appear the general approach is very similar, but there's
more added abstractions and just general code change in his version.

Regardless, I'm open to working together.

I am also open to working together, and could really use the help, as I'm
not a python developer.

I have taken all the suggestions from my first patch set and have reworked
my code and commits and will submit them now for review.  With the smaller
patches and cleaner commit messages I hope that it will make it easier
to see what I've done so far and what is still open work.

Ideally, I would prefer we land something minimal and working in mainline soon,
then further collaborate on changes that clean up code and enable more features.

My end-game is to have P4 Streams working in git-p4, and maybe LFS-like support
that uses p4 as the backend. It would be great to not be the only one
spending effort
in that direction.

Yang

I have similar goals.  I would love to get the smallest set of non-breaking
changes in that allows the program to basically work with Python 3.5+.

My rush has been because I need to use git-p4 for work and have been 
working
on the project at the office.  Once I reach a point where I am able to
generally work (when t9800 is complete) I'll really not be free to spend 
too
much work time on the project, but I am eager to see this through!

As far as status, the last time I ran tests, python 2.7 passed all the tests
and Python 3.5 passed some of the tests.  I know it is not passing t9801
at this time and I'm trying to find out why.

So, Yang, I am very interested in working together.


Kindest regards,

Ben Keene

On Sat, Dec 7, 2019 at 8:21 AM Ben Keene [off-list ref] wrote:

On 12/7/2019 2:29 AM, Yang Zhao wrote:

quoted

Ideally, I would prefer we land something minimal and working in mainline soon,
then further collaborate on changes that clean up code and enable more features.

My end-game is to have P4 Streams working in git-p4, and maybe LFS-like support
that uses p4 as the backend. It would be great to not be the only one
spending effort
in that direction.

I have similar goals.  I would love to get the smallest set of non-breaking
changes in that allows the program to basically work with Python 3.5+.

My rush has been because I need to use git-p4 for work and have been
working
on the project at the office.  Once I reach a point where I am able to
generally work (when t9800 is complete) I'll really not be free to spend
too
much work time on the project, but I am eager to see this through!

I'm in a similar situation, but we use p4 Streams and so I actually need further
development before being able to make a full switch. I am given more liberty
in terms of how much work time I can dedicate to this, though.

Given the situation, can you give my patch set a try in your work environment?
It is currently passing everything except t9824-git-p4-git-lfs.

If you're OK with it, I would prefer that we work from my version as a base and
add some of your quality-of-life enhancements on top. I can do the merges myself
if you are pressed for time.

Thanks,
Yang

On 12/7/2019 2:59 PM, Yang Zhao wrote:

On Sat, Dec 7, 2019 at 8:21 AM Ben Keene [off-list ref] wrote:

quoted

On 12/7/2019 2:29 AM, Yang Zhao wrote:

quoted

Ideally, I would prefer we land something minimal and working in mainline soon,
then further collaborate on changes that clean up code and enable more features.

My end-game is to have P4 Streams working in git-p4, and maybe LFS-like support
that uses p4 as the backend. It would be great to not be the only one
spending effort
in that direction.

I have similar goals.  I would love to get the smallest set of non-breaking
changes in that allows the program to basically work with Python 3.5+.

My rush has been because I need to use git-p4 for work and have been
working
on the project at the office.  Once I reach a point where I am able to
generally work (when t9800 is complete) I'll really not be free to spend
too
much work time on the project, but I am eager to see this through!

I'm in a similar situation, but we use p4 Streams and so I actually need further
development before being able to make a full switch. I am given more liberty
in terms of how much work time I can dedicate to this, though.

Given the situation, can you give my patch set a try in your work environment?
It is currently passing everything except t9824-git-p4-git-lfs.

I downloaded your code and it looks like it works for Python 2.7.  I'm 
seeing errors with the following tests:

* 9816.5

     Traceback (most recent call last):
     File "/home/bkeene/git/git-p4", line 4227, in <module>
         main()
     File "/home/bkeene/git/git-p4", line 4221, in main
         if not cmd.run(args):
     File "/home/bkeene/git/git-p4", line 2381, in run
         ok = self.applyCommit(commit)
     File "/home/bkeene/git/git-p4", line 2106, in applyCommit
         p4_write_pipe(['submit', '-i'], submitTemplate)
     File "/home/bkeene/git/git-p4", line 207, in p4_write_pipe
         return write_pipe(real_cmd, stdin)
     File "/home/bkeene/git/git-p4", line 201, in write_pipe
         die('Command failed: %s' % str(c))
     File "/home/bkeene/git/git-p4", line 158, in die
         raise Exception(msg)
     Exception: Command failed: ['p4', '-r', '3', 'submit', '-i']

* 9816.6

     Traceback (most recent call last):
     File "/home/bkeene/git/git-p4", line 4227, in <module>
         main()
     File "/home/bkeene/git/git-p4", line 4221, in main
         if not cmd.run(args):
     File "/home/bkeene/git/git-p4", line 2381, in run
         ok = self.applyCommit(commit)
     File "/home/bkeene/git/git-p4", line 2106, in applyCommit
         p4_write_pipe(['submit', '-i'], submitTemplate)
     File "/home/bkeene/git/git-p4", line 207, in p4_write_pipe
         return write_pipe(real_cmd, stdin)
     File "/home/bkeene/git/git-p4", line 201, in write_pipe
         die('Command failed: %s' % str(c))
     File "/home/bkeene/git/git-p4", line 158, in die
         raise Exception(msg)
     Exception: Command failed: ['p4', '-r', '3', 'submit', '-i']

* 9816.7

  Traceback (most recent call last):
    File "/home/bkeene/git/git-p4", line 4227, in <module>
      main()
    File "/home/bkeene/git/git-p4", line 4221, in main
      if not cmd.run(args):
    File "/home/bkeene/git/git-p4", line 2381, in run
      ok = self.applyCommit(commit)
    File "/home/bkeene/git/git-p4", line 2106, in applyCommit
      p4_write_pipe(['submit', '-i'], submitTemplate)
    File "/home/bkeene/git/git-p4", line 207, in p4_write_pipe
      return write_pipe(real_cmd, stdin)
    File "/home/bkeene/git/git-p4", line 201, in write_pipe
      die('Command failed: %s' % str(c))
    File "/home/bkeene/git/git-p4", line 158, in die
      raise Exception(msg)
  Exception: Command failed: ['p4', '-r', '3', 'submit', '-i']

* 9816.9

     Traceback (most recent call last):
     File "/home/bkeene/git/git-p4", line 4227, in <module>
         main()
     File "/home/bkeene/git/git-p4", line 4221, in main
         if not cmd.run(args):
     File "/home/bkeene/git/git-p4", line 2381, in run
         ok = self.applyCommit(commit)
     File "/home/bkeene/git/git-p4", line 2106, in applyCommit
         p4_write_pipe(['submit', '-i'], submitTemplate)
     File "/home/bkeene/git/git-p4", line 207, in p4_write_pipe
         return write_pipe(real_cmd, stdin)
     File "/home/bkeene/git/git-p4", line 201, in write_pipe
         die('Command failed: %s' % str(c))
     File "/home/bkeene/git/git-p4", line 158, in die
         raise Exception(msg)
     Exception: Command failed: ['p4', '-r', '3', 'submit', '-i']

* 9810.16

     Traceback (most recent call last):
     File "/home/bkeene/git/git-p4", line 4227, in <module>
         main()
     File "/home/bkeene/git/git-p4", line 4221, in main
         if not cmd.run(args):
     File "/home/bkeene/git/git-p4", line 2436, in run
         rebase.rebase()
     File "/home/bkeene/git/git-p4", line 3913, in rebase
         system("git rebase %s" % upstream)
     File "/home/bkeene/git/git-p4", line 305, in system
         raise CalledProcessError(retcode, cmd)
     subprocess.CalledProcessError: Command 'git rebase 
remotes/p4/master' returned non-zero exit status 1

The last test was a breaking test that stopped the test make.

If you're OK with it, I would prefer that we work from my version as a base and
add some of your quality-of-life enhancements on top. I can do the merges myself
if you are pressed for time.

Thanks,
Yang

I am not a Python developer and my code is further behind than yours, so
it makes complete sense to use yours as the base.

On 12/9/2019 10:03 AM, Ben Keene wrote:

On 12/7/2019 2:59 PM, Yang Zhao wrote:

quoted

On Sat, Dec 7, 2019 at 8:21 AM Ben Keene [off-list ref] wrote:

quoted

On 12/7/2019 2:29 AM, Yang Zhao wrote:

quoted

Ideally, I would prefer we land something minimal and working in 
mainline soon,
then further collaborate on changes that clean up code and enable 
more features.

My end-game is to have P4 Streams working in git-p4, and maybe 
LFS-like support
that uses p4 as the backend. It would be great to not be the only one
spending effort
in that direction.

I have similar goals.  I would love to get the smallest set of 
non-breaking
changes in that allows the program to basically work with Python 3.5+.

My rush has been because I need to use git-p4 for work and have been
working
on the project at the office.  Once I reach a point where I am able to
generally work (when t9800 is complete) I'll really not be free to 
spend
too
much work time on the project, but I am eager to see this through!

I'm in a similar situation, but we use p4 Streams and so I actually 
need further
development before being able to make a full switch. I am given more 
liberty
in terms of how much work time I can dedicate to this, though.

Given the situation, can you give my patch set a try in your work 
environment?
It is currently passing everything except t9824-git-p4-git-lfs.

I downloaded your code and it looks like it works for Python 2.7. I'm 
seeing errors with the following tests:

* 9816.5

    Traceback (most recent call last):
    File "/home/bkeene/git/git-p4", line 4227, in <module>
        main()
    File "/home/bkeene/git/git-p4", line 4221, in main
        if not cmd.run(args):
    File "/home/bkeene/git/git-p4", line 2381, in run
        ok = self.applyCommit(commit)
    File "/home/bkeene/git/git-p4", line 2106, in applyCommit
        p4_write_pipe(['submit', '-i'], submitTemplate)
    File "/home/bkeene/git/git-p4", line 207, in p4_write_pipe
        return write_pipe(real_cmd, stdin)
    File "/home/bkeene/git/git-p4", line 201, in write_pipe
        die('Command failed: %s' % str(c))
    File "/home/bkeene/git/git-p4", line 158, in die
        raise Exception(msg)
    Exception: Command failed: ['p4', '-r', '3', 'submit', '-i']

* 9816.6

    Traceback (most recent call last):
    File "/home/bkeene/git/git-p4", line 4227, in <module>
        main()
    File "/home/bkeene/git/git-p4", line 4221, in main
        if not cmd.run(args):
    File "/home/bkeene/git/git-p4", line 2381, in run
        ok = self.applyCommit(commit)
    File "/home/bkeene/git/git-p4", line 2106, in applyCommit
        p4_write_pipe(['submit', '-i'], submitTemplate)
    File "/home/bkeene/git/git-p4", line 207, in p4_write_pipe
        return write_pipe(real_cmd, stdin)
    File "/home/bkeene/git/git-p4", line 201, in write_pipe
        die('Command failed: %s' % str(c))
    File "/home/bkeene/git/git-p4", line 158, in die
        raise Exception(msg)
    Exception: Command failed: ['p4', '-r', '3', 'submit', '-i']

* 9816.7

 Traceback (most recent call last):
   File "/home/bkeene/git/git-p4", line 4227, in <module>
     main()
   File "/home/bkeene/git/git-p4", line 4221, in main
     if not cmd.run(args):
   File "/home/bkeene/git/git-p4", line 2381, in run
     ok = self.applyCommit(commit)
   File "/home/bkeene/git/git-p4", line 2106, in applyCommit
     p4_write_pipe(['submit', '-i'], submitTemplate)
   File "/home/bkeene/git/git-p4", line 207, in p4_write_pipe
     return write_pipe(real_cmd, stdin)
   File "/home/bkeene/git/git-p4", line 201, in write_pipe
     die('Command failed: %s' % str(c))
   File "/home/bkeene/git/git-p4", line 158, in die
     raise Exception(msg)
 Exception: Command failed: ['p4', '-r', '3', 'submit', '-i']

* 9816.9

    Traceback (most recent call last):
    File "/home/bkeene/git/git-p4", line 4227, in <module>
        main()
    File "/home/bkeene/git/git-p4", line 4221, in main
        if not cmd.run(args):
    File "/home/bkeene/git/git-p4", line 2381, in run
        ok = self.applyCommit(commit)
    File "/home/bkeene/git/git-p4", line 2106, in applyCommit
        p4_write_pipe(['submit', '-i'], submitTemplate)
    File "/home/bkeene/git/git-p4", line 207, in p4_write_pipe
        return write_pipe(real_cmd, stdin)
    File "/home/bkeene/git/git-p4", line 201, in write_pipe
        die('Command failed: %s' % str(c))
    File "/home/bkeene/git/git-p4", line 158, in die
        raise Exception(msg)
    Exception: Command failed: ['p4', '-r', '3', 'submit', '-i']

* 9810.16

    Traceback (most recent call last):
    File "/home/bkeene/git/git-p4", line 4227, in <module>
        main()
    File "/home/bkeene/git/git-p4", line 4221, in main
        if not cmd.run(args):
    File "/home/bkeene/git/git-p4", line 2436, in run
        rebase.rebase()
    File "/home/bkeene/git/git-p4", line 3913, in rebase
        system("git rebase %s" % upstream)
    File "/home/bkeene/git/git-p4", line 305, in system
        raise CalledProcessError(retcode, cmd)
    subprocess.CalledProcessError: Command 'git rebase 
remotes/p4/master' returned non-zero exit status 1

The last test was a breaking test that stopped the test make.

quoted

If you're OK with it, I would prefer that we work from my version as 
a base and
add some of your quality-of-life enhancements on top. I can do the 
merges myself
if you are pressed for time.

Thanks,
Yang

I am not a Python developer and my code is further behind than yours, so
it makes complete sense to use yours as the base.

So, I just attempted to run a base case on windows: git p4 clone //depot 
and I'm getting an error:

Depot paths must start with "//": /depot

Hi Ben,

On Mon, 9 Dec 2019, Ben Keene wrote:

So, I just attempted to run a base case on windows: git p4 clone //depot and
I'm getting an error:

Depot paths must start with "//": /depot

You started this in a Bash, right?

The Git Bash has the very specific problem that many of Git's shell
scripts assume that forward slashes are directory separators, not
backslashes, and that absolute paths start with a single forward slash. In
other words, they expect Unix paths.

But we're on Windows! So the MSYS2 runtime (which is the POSIX emulation
layer derived from Cygwin which allows us to build and run Bash on
Windows) "translates" between the paths. For example, if you pass `/depot`
as a parameter to a Git command, the MSYS2 runtime notices that `git.exe`
is not an MSYS2 program (i.e. it does not understand pseudo-Unix paths),
and translates the path to `C:/Program Files/Git/depot`.

However, your call has _two_ slashes, right? That is unfortunately MSYS2's
trick to say "oh BTW keep the slash, this is not a Unix path".

To avoid this, just set `MSYS_NO_PATHCONV`, like so:

	MSYS_NO_PATHCONV=1 git p4 clone //depot

This behavior is documented in our release notes, by the way:
https://github.com/git-for-windows/build-extra/blob/master/ReleaseNotes.md#known-issues

Ciao,
Johannes

On Mon, Dec 9, 2019 at 7:03 AM Ben Keene [off-list ref] wrote:

I downloaded your code and it looks like it works for Python 2.7.  I'm
seeing errors with the following tests:

* 9816.5
...

I'm not sure why those would fail for your local environment but not in CI.

I've just pushed an updated PR to GitHub which is now passing all
tests on python-3.5. Give that a go.

If tests are still failing for you, it'd be good to get the verbose
output from the specific test scripts. They don't tell us much without
it.

e.g.:
  ~/git.git/t $ : ./t9816-git-p4-locked.sh --verbose

Thanks,
Yang

On Fri, Dec 06, 2019 at 04:33:19PM -0800, Yang Zhao wrote:

git-p4.py includes support for python-3, but this was not previously
validated in CI. Lets actually do that.

There is no tangible benefit to repeating python-3 tests for all
environments, so only limit it to linux-gcc for now.

In the subject line and the commit message body you speak about CI in
general, without sinling out a particular CI system ...

 azure-pipelines.yml | 11 +++++++++++
 1 file changed, 11 insertions(+)

... but the patch only modifies 'azure-pipelines.yml', and not
'.travis.yml'.

I don't speak 'azure-pipelines.yml', so question: will this build Git
and run the whole test suite twice, once with Python 2.7 and once with
3.7?  I'm asking because 'git-p4' is the one and only Python script we
have, with no plans for more, so running the whole test suite with a
different Python version for a second time instead of running only the
'git-p4'-specific tests (t98*) seems to be quite wasteful.

Furthermore, this is the first patch of the series, with all the
Python3 fixes in subsequent commits, so the Azure Pipelines build with
Python 3.7 would fail with only this patch, wouldn't it?  I think this
patch should be the last in the series, after all the Python 2 vs 3
issues are sorted out.

   - bash: |
        test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1
 
-- 
2.21.0.windows.1

On 12/9/2019 2:48 PM, Johannes Schindelin wrote:

Hi Ben,

On Mon, 9 Dec 2019, Ben Keene wrote:

quoted

So, I just attempted to run a base case on windows: git p4 clone //depot and
I'm getting an error:

Depot paths must start with "//": /depot

You started this in a Bash, right?

No, I started it from a windows command cmd.exe prompt.  (I almost never 
use the bash prompt)

The Git Bash has the very specific problem that many of Git's shell
scripts assume that forward slashes are directory separators, not
backslashes, and that absolute paths start with a single forward slash. In
other words, they expect Unix paths.

But we're on Windows! So the MSYS2 runtime (which is the POSIX emulation
layer derived from Cygwin which allows us to build and run Bash on
Windows) "translates" between the paths. For example, if you pass `/depot`
as a parameter to a Git command, the MSYS2 runtime notices that `git.exe`
is not an MSYS2 program (i.e. it does not understand pseudo-Unix paths),
and translates the path to `C:/Program Files/Git/depot`.

That is good to know!

However, your call has _two_ slashes, right? That is unfortunately MSYS2's
trick to say "oh BTW keep the slash, this is not a Unix path".

To avoid this, just set `MSYS_NO_PATHCONV`, like so:

When I first installed git, I didn't read the release notes. (Shame on 
me!) and I installed
python for windows and added an alias for git-p4.py against the windows 
version of
python, so when I run git, it's not performing that conversion.

	MSYS_NO_PATHCONV=1 git p4 clone //depot

This behavior is documented in our release notes, by the way:
https://github.com/git-for-windows/build-extra/blob/master/ReleaseNotes.md#known-issues

Ciao,
Johannes

I'm starting to run out of time at work, so I'll be slow to try and 
repro this.

Thanks for the info!

- Ben

On Tue, Dec 10, 2019 at 2:30 AM SZEDER Gábor [off-list ref] wrote:

On Fri, Dec 06, 2019 at 04:33:19PM -0800, Yang Zhao wrote:

quoted

diff --git a/azure-pipelines.yml b/azure-pipelines.yml
index 37ed7e06c6..d5f9413248 100644
--- a/azure-pipelines.yml
+++ b/azure-pipelines.yml

@@ -331,7 +331,18 @@ jobs:
   displayName: linux-gcc
   condition: succeeded()
   pool: Hosted Ubuntu 1604
+  strategy:
+    matrix:
+      python27:
+        python.version: '2.7'
+      python37:
+        python.version: '3.7'
   steps:
+  - task: UsePythonVersion@0
+    inputs:
+      versionSpec: '$(python.version)'
+  - bash: |
+      echo "##vso[task.setvariable variable=python_path]$(which python)"

I don't speak 'azure-pipelines.yml', so question: will this build Git
and run the whole test suite twice, once with Python 2.7 and once with
3.7?  I'm asking because 'git-p4' is the one and only Python script we
have, with no plans for more, so running the whole test suite with a
different Python version for a second time instead of running only the
'git-p4'-specific tests (t98*) seems to be quite wasteful.

The CI scripts as it is currently does not separate compiling and testing for
non-Windows builds. I don't see a good way to only run a specific set of tests
given a particular environment without re-architecturing the CI pipeline.

Furthermore, there's a step in the build that hard-codes the
environment's python
path into the installed version of the script. This complicates being
able to even create
a `git-p4` that runs under different python environments in Azure
Pipelines due to how
`UsePythonVersion@0` pulls python into version-specific directories.
I haven't dug into
why this hardcoding is done in the first place.

So, the question is if it's worth doing this work now when the desire
seems to be dropping
python-2.7 completely in the (near?) future.

-- 
Yang

On Tue, Dec 10, 2019 at 11:11:09AM -0800, Yang Zhao wrote:

On Tue, Dec 10, 2019 at 2:30 AM SZEDER Gábor [off-list ref] wrote:

quoted

On Fri, Dec 06, 2019 at 04:33:19PM -0800, Yang Zhao wrote:

quoted

diff --git a/azure-pipelines.yml b/azure-pipelines.yml
index 37ed7e06c6..d5f9413248 100644
--- a/azure-pipelines.yml
+++ b/azure-pipelines.yml

@@ -331,7 +331,18 @@ jobs:
   displayName: linux-gcc
   condition: succeeded()
   pool: Hosted Ubuntu 1604
+  strategy:
+    matrix:
+      python27:
+        python.version: '2.7'
+      python37:
+        python.version: '3.7'
   steps:
+  - task: UsePythonVersion@0
+    inputs:
+      versionSpec: '$(python.version)'
+  - bash: |
+      echo "##vso[task.setvariable variable=python_path]$(which python)"

I don't speak 'azure-pipelines.yml', so question: will this build Git
and run the whole test suite twice, once with Python 2.7 and once with
3.7?  I'm asking because 'git-p4' is the one and only Python script we
have, with no plans for more, so running the whole test suite with a
different Python version for a second time instead of running only the
'git-p4'-specific tests (t98*) seems to be quite wasteful.

The CI scripts as it is currently does not separate compiling and testing for
non-Windows builds. I don't see a good way to only run a specific set of tests
given a particular environment without re-architecturing the CI pipeline.

Building git and running the test suite is encapsulated in the
'ci/run-build-and-tests.sh' script, while installing dependencies is
encapsulated in 'ci/install-dependencies.sh', just in case Azure
Pipelines Linux images don't contain both Python 2 and 3 (Travis CI
images contain 2.7 and 3.5)  So I don't think it's necessary to touch
'azure-pipelines.yml' or '.travis.yml' at all.

Furthermore, there's a step in the build that hard-codes the
environment's python
path into the installed version of the script. This complicates being
able to even create
a `git-p4` that runs under different python environments in Azure
Pipelines due to how
`UsePythonVersion@0` pulls python into version-specific directories.

The PYTHON_PATH that we build 'git p4' with can be a symbolink link,
and then choosing which Python version to use is only a matter of
pointing that symbolic link to the python binary of the desired
version.

In fact our default PYTHON_PATH is '/usr/bin/python', which is a
symbolic link pointing to 'python2.7' on Ubuntu 16.04, including the
Travis CI's images that we use.

I haven't dug into
why this hardcoding is done in the first place.

So, the question is if it's worth doing this work now when the desire
seems to be dropping
python-2.7 completely in the (near?) future.

-- 
Yang

On Thu, Dec 12, 2019 at 6:13 AM SZEDER Gábor [off-list ref] wrote:

quoted

The CI scripts as it is currently does not separate compiling and testing for
non-Windows builds. I don't see a good way to only run a specific set of tests
given a particular environment without re-architecturing the CI pipeline.

Building git and running the test suite is encapsulated in the
'ci/run-build-and-tests.sh' script, while installing dependencies is
encapsulated in 'ci/install-dependencies.sh', just in case Azure
Pipelines Linux images don't contain both Python 2 and 3 (Travis CI
images contain 2.7 and 3.5)  So I don't think it's necessary to touch
'azure-pipelines.yml' or '.travis.yml' at all.

Yes, and this is implemented as a single step as far as the CI
pipeline is concerned. It does not produce a build artifact that can
then be loaded into multiple environments for running tests.

Unless there's a very good reason to _not_ use Azure Pipeline's
built-in Python version selection support, I believe it's more
desirable in the long-run to leverage the feature rather than maintain
some custom solution.

-- 
Yang

On Thu, Dec 12, 2019 at 09:04:24AM -0800, Yang Zhao wrote:

On Thu, Dec 12, 2019 at 6:13 AM SZEDER Gábor [off-list ref] wrote:

quoted

quoted

The CI scripts as it is currently does not separate compiling and testing for
non-Windows builds. I don't see a good way to only run a specific set of tests
given a particular environment without re-architecturing the CI pipeline.

Building git and running the test suite is encapsulated in the
'ci/run-build-and-tests.sh' script, while installing dependencies is
encapsulated in 'ci/install-dependencies.sh', just in case Azure
Pipelines Linux images don't contain both Python 2 and 3 (Travis CI
images contain 2.7 and 3.5)  So I don't think it's necessary to touch
'azure-pipelines.yml' or '.travis.yml' at all.

Yes, and this is implemented as a single step as far as the CI
pipeline is concerned. It does not produce a build artifact that can
then be loaded into multiple environments for running tests.

I don't understand what artifact should be loaded into what
environments...

Unless there's a very good reason to _not_ use Azure Pipeline's
built-in Python version selection support, I believe it's more
desirable in the long-run to leverage the feature rather than maintain
some custom solution.

Azure Pipelines's built-in Python version selection support only works
on Azure Pipelines, therefore it's more desirable to have a general
solution.

On Thu, Dec 12, 2019 at 9:15 AM SZEDER Gábor [off-list ref] wrote:

On Thu, Dec 12, 2019 at 09:04:24AM -0800, Yang Zhao wrote:

quoted

Unless there's a very good reason to _not_ use Azure Pipeline's
built-in Python version selection support, I believe it's more
desirable in the long-run to leverage the feature rather than maintain
some custom solution.

Azure Pipelines's built-in Python version selection support only works
on Azure Pipelines, therefore it's more desirable to have a general
solution.

That's fair. However, if we actually want to have something unified
that works for Linux and macOS (t90** isn't run on Windows afaict)
then I won't have the bandwidth for it in the near term. I'd be more
inclined to drop the CI changes from the series if we don't want a
stop-gap in the meantime.

Here's a patch I have on my tree that I would offer -
it removes references to basestring and should be a drop in patch.


 From 1cc3c0f8570adb1ef2bacc0009aac979a3263d70 Mon Sep 17 00:00:00 2001
From: Ben Keene <redacted>
Date: Tue, 3 Dec 2019 16:36:26 -0500
Subject: [PATCH] git-p4: change the expansion test from basestring to list

Python 3 handles strings differently than Python 2.7. Since Python 2
is reaching it's end of life, a series of changes are being submitted to
enable python 3.5 and following support. The current code fails basic
tests under python 3.5.

Some codepaths can represent a command line the program
internally prepares to execute either as a single string
(i.e. each token properly quoted, concatenated with $IFS) or
as a list of argv[] elements, and there are 9 places where
we say "if X is isinstance(_, basestring), then do this
thing to handle X as a command line in a single string; if
not, X is a command line in a list form".

This does not work well with Python 3, as there is no
basestring (everything is Unicode now), and even with Python
2, it was not an ideal way to tell the two cases apart,
because an internally formed command line could have been in
a single Unicode string.

Flip the check to say "if X is not a list, then handle X as
a command line in a single string; otherwise treat it as a
command line in a list form".

This will get rid of references to 'basestring', to migrate
the code ready for Python 3.

Thanks-to: Junio C Hamano [off-list ref]
Signed-off-by: Ben Keene <redacted>
---
  git-p4.py | 18 +++++++++---------
  1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 25d8012e23..d322ae20ef 100755
--- a/git-p4.py
+++ b/git-p4.py

@@ -98,7 +98,7 @@ def p4_build_cmd(cmd):
          # Provide a way to not pass this option by setting 

git-p4.retries to 0
          real_cmd += ["-r", str(retries)]

-    if isinstance(cmd,basestring):
+    if not isinstance(cmd, list):
          real_cmd = ' '.join(real_cmd) + ' ' + cmd
      else:
          real_cmd += cmd

@@ -192,7 +192,7 @@ def write_pipe(c, stdin):
      if verbose:
          sys.stderr.write('Writing pipe: %s\n' % str(c))

-    expand = isinstance(c,basestring)
+    expand = not isinstance(c, list)
      p = subprocess.Popen(c, stdin=subprocess.PIPE, shell=expand)
      pipe = p.stdin
      val = pipe.write(stdin)

@@ -214,7 +214,7 @@ def read_pipe_full(c):
      if verbose:
          sys.stderr.write('Reading pipe: %s\n' % str(c))

-    expand = isinstance(c,basestring)
+    expand = not isinstance(c, list)
      p = subprocess.Popen(c, stdout=subprocess.PIPE, 

stderr=subprocess.PIPE, shell=expand)
      (out, err) = p.communicate()
      return (p.returncode, out, decode_text_stream(err))

@@ -254,7 +254,7 @@ def read_pipe_lines(c):
      if verbose:
          sys.stderr.write('Reading pipe: %s\n' % str(c))

-    expand = isinstance(c, basestring)
+    expand = not isinstance(c, list)
      p = subprocess.Popen(c, stdout=subprocess.PIPE, shell=expand)
      pipe = p.stdout
      val = [decode_text_stream(line) for line in pipe.readlines()]

@@ -297,7 +297,7 @@ def p4_has_move_command():
      return True

  def system(cmd, ignore_error=False):
-    expand = isinstance(cmd,basestring)
+    expand = not isinstance(cmd, list)
      if verbose:
          sys.stderr.write("executing %s\n" % str(cmd))
      retcode = subprocess.call(cmd, shell=expand)

@@ -309,7 +309,7 @@ def system(cmd, ignore_error=False):
  def p4_system(cmd):
      """Specifically invoke p4 as the system command. """
      real_cmd = p4_build_cmd(cmd)
-    expand = isinstance(real_cmd, basestring)
+    expand = not isinstance(real_cmd, list)
      retcode = subprocess.call(real_cmd, shell=expand)
      if retcode:
          raise CalledProcessError(retcode, real_cmd)

@@ -547,7 +547,7 @@ def getP4OpenedType(file):
  # Return the set of all p4 labels
  def getP4Labels(depotPaths):
      labels = set()
-    if isinstance(depotPaths,basestring):
+    if not isinstance(depotPaths, list):
          depotPaths = [depotPaths]

      for l in p4CmdList(["labels"] + ["%s..." % p for p in depotPaths]):

@@ -633,7 +633,7 @@ def isModeExecChanged(src_mode, dst_mode):
  def p4CmdList(cmd, stdin=None, stdin_mode='w+b', cb=None, skip_info=False,
          errors_as_exceptions=False):

-    if isinstance(cmd,basestring):
+    if not isinstance(cmd, list):
          cmd = "-G " + cmd
          expand = True
      else:

@@ -650,7 +650,7 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b', 

cb=None, skip_info=False,
      stdin_file = None
      if stdin is not None:
          stdin_file = tempfile.TemporaryFile(prefix='p4-stdin', 
mode=stdin_mode)
-        if isinstance(stdin,basestring):
+        if not isinstance(stdin, list):
              stdin_file.write(stdin)
          else:
              for i in stdin:
-- 
2.24.1.windows.2