[v4,02/11] git-p4: change the expansion test from basestring to list
diff mbox series

Message ID 0ef2f56b04803cad2e60bf881e86d8bdd69463a6.1575498577.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • git-p4.py: Cast byte strings to unicode strings in python3
Related show

Commit Message

sunlin via GitGitGadget Dec. 4, 2019, 10:29 p.m. UTC
From: Ben Keene <seraphire@gmail.com>

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.7+ support. The current code fails basic tests under python 3.7.

Change references to basestring in the isinstance tests to use list instead. This prepares the code to remove all references to basestring.

The original code used basestring in a test to determine if a list or literal string was passed into 9 different functions.  This is used to determine if the shell should be evoked when calling subprocess methods.

Signed-off-by: Ben Keene <seraphire@gmail.com>
(cherry picked from commit 5b1b1c145479b5d5fd242122737a3134890409e6)
---
 git-p4.py | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Denton Liu Dec. 5, 2019, 10:27 a.m. UTC | #1
Hi Ben,

On Wed, Dec 04, 2019 at 10:29:28PM +0000, Ben Keene via GitGitGadget wrote:
> From: Ben Keene <seraphire@gmail.com>
> 
> Python 3+ handles strings differently than Python 2.7.

Do you mean Python 3?

> Since Python 2 is reaching it's end of life, a series of changes are being submitted to enable python 3.7+ support. The current code fails basic tests under python 3.7.

Python 3.5 doesn't reach EOL until Q4 2020[1]. We should be testing
these changes under 3.5 to ensure that we're not accidentally
introducing stuff that's not backwards compatible.

> 
> Change references to basestring in the isinstance tests to use list instead. This prepares the code to remove all references to basestring.
> 
> The original code used basestring in a test to determine if a list or literal string was passed into 9 different functions.  This is used to determine if the shell should be evoked when calling subprocess methods.

Once again, I'd swap the above two paragraphs. Problem then solution.

Also, did you mean "invoked" instead of "evoked"?

> 
> Signed-off-by: Ben Keene <seraphire@gmail.com>
> (cherry picked from commit 5b1b1c145479b5d5fd242122737a3134890409e6)
> ---
>  git-p4.py | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

The patch itself looks good, though.

[1]: https://devguide.python.org/#branchstatus
Ben Keene Dec. 5, 2019, 5:05 p.m. UTC | #2
On 12/5/2019 5:27 AM, Denton Liu wrote:
> Hi Ben,
>
> On Wed, Dec 04, 2019 at 10:29:28PM +0000, Ben Keene via GitGitGadget wrote:
>> From: Ben Keene <seraphire@gmail.com>
>>
>> Python 3+ handles strings differently than Python 2.7.
> Do you mean Python 3?
>
>> Since Python 2 is reaching it's end of life, a series of changes are being submitted to enable python 3.7+ support. The current code fails basic tests under python 3.7.
> Python 3.5 doesn't reach EOL until Q4 2020[1]. We should be testing
> these changes under 3.5 to ensure that we're not accidentally
> introducing stuff that's not backwards compatible.


I changed my commit text to say support for version 3.5 (which is 
actually the version I am running the test with).


>> Change references to basestring in the isinstance tests to use list instead. This prepares the code to remove all references to basestring.
>>
>> The original code used basestring in a test to determine if a list or literal string was passed into 9 different functions.  This is used to determine if the shell should be evoked when calling subprocess methods.
> Once again, I'd swap the above two paragraphs. Problem then solution.
>
> Also, did you mean "invoked" instead of "evoked"?


Changed.  And yes, I meant 'invoked'. I wasn't trying to make my code 
feel anything!


>> Signed-off-by: Ben Keene <seraphire@gmail.com>
>> (cherry picked from commit 5b1b1c145479b5d5fd242122737a3134890409e6)
>> ---
>>   git-p4.py | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
> The patch itself looks good, though.
>
> [1]: https://devguide.python.org/#branchstatus

Patch
diff mbox series

diff --git a/git-p4.py b/git-p4.py
index b2ffbc057b..0f27996393 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -109,7 +109,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
@@ -175,7 +175,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)
@@ -197,7 +197,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, err)
@@ -233,7 +233,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 = pipe.readlines()
@@ -276,7 +276,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)
@@ -288,7 +288,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)
@@ -526,7 +526,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]):
@@ -613,7 +613,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:
@@ -630,7 +630,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: