[v5,04/15] git-p4: change the expansion test from basestring to list

Commit Message

Linus Arver via GitGitGadget Dec. 7, 2019, 5:47 p.m. UTC

From: Ben Keene <[e-mail]>

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.

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 invoked when calling subprocess
methods.

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

Signed-off-by: Ben Keene <[e-mail]>
---
 git-p4.py | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

Comments

Junio C Hamano Dec. 9, 2019, 8:25 p.m. UTC | #1

"Ben Keene via GitGitGadget" <[e-mail]> writes:

> 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

s/literal/a &/, probably, but I do not thin this is about "literal"
at all.  Perhaps "if a list or a string was passed ..." is what you
meant, as the code seems to have two ways to represent a command
line in the program, one as a single string with possibly multiple
tokens on it, separated with IFS and quoted just like you would feed
to shell, and the other as a list of strings, each element being a
single argv[] element for the command to be invoked.

So the issue is that isinstance(X, basestring) used to be how you
are supposed to see if X is a "string", and the logic were built
around "if we have a string, then keep the command in a string when
manipulating, and otherwise what we have must be a command line in a
list".  But because Unicode string is not an instance of basestring,
the logic no longer work, so you'd flip the polarity around to use
"if it is not a list, then we must have a command in a string".

Which makes sense.

> determine if the shell should be invoked when calling subprocess
> methods.

This is mostly true, but the use in p4_build_cmd() and the second
use among the two uses in p4CmdList() are different.

	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.

or something like that?

Ben Keene Dec. 13, 2019, 2:40 p.m. UTC | #2

On 12/9/2019 3:25 PM, Junio C Hamano wrote:
> "Ben Keene via GitGitGadget" <[e-mail]> writes:
>
>> 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
>> This is mostly true, but the use in p4_build_cmd() and the second
...
> use among the two uses in p4CmdList() are different.
>
> 	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.
>
> or something like that?
>
I incorporated this into my suggested submission to Yang's work. Thank you.

Patch

diff --git a/git-p4.py b/git-p4.py
index 65e926758c..3153186df0 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -108,7 +108,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
@@ -174,7 +174,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)
@@ -196,7 +196,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)
@@ -232,7 +232,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()
@@ -275,7 +275,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)
@@ -287,7 +287,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)
@@ -525,7 +525,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]):
@@ -612,7 +612,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:
@@ -629,7 +629,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: