[v4,05/11] git-p4: Add new functions in preparation of usage

Commit Message

Linus Arver via GitGitGadget Dec. 4, 2019, 10:29 p.m. UTC

From: Ben Keene <[e-mail]>

This changelist is an intermediate submission for migrating the P4 support from Python2 to Python3. The code needs access to the encodeWithUTF8() for support of non-UTF8 filenames in the clone class as well as the sync class.

Move the function encodeWithUTF8() from the P4Sync class to a stand-alone function.  This will allow other classes to use this function without instanciating the P4Sync class. Change the self.verbose reference to an optional method parameter. Update the existing references to this function to pass the self.verbose since it is no longer available on "self" since the function is no longer contained on the P4Sync class.

Modify the functions write_pipe() and p4_write_pipe() to remove the return value.  The return value for both functions is the number of bytes, but the meaning is lost under python3 since the count does not match the number of characters that may have been encoded.  Additionally, the return value was never used, so this is removed to avoid future ambiguity.

Add a new method gitConfigSet(). This method will set a value in the git configuration cache list.

Signed-off-by: Ben Keene <[e-mail]>
(cherry picked from commit affe888f432bb6833df78962e8671fccdf76c47a)
---
 git-p4.py | 60 ++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 16 deletions(-)

Comments

Denton Liu Dec. 5, 2019, 10:50 a.m. UTC | #1

> Subject: git-p4: Add new functions in preparation of usage

Nit: as a convention, you should lowercase the letter after the colon in
the subject. As in "git-p4: add new functions..."

This applies for other patches as well.

On Wed, Dec 04, 2019 at 10:29:31PM +0000, Ben Keene via GitGitGadget wrote:
> From: Ben Keene <[e-mail]>
> 
> This changelist is an intermediate submission for migrating the P4 support from Python2 to Python3. The code needs access to the encodeWithUTF8() for support of non-UTF8 filenames in the clone class as well as the sync class.
> 
> Move the function encodeWithUTF8() from the P4Sync class to a stand-alone function.  This will allow other classes to use this function without instanciating the P4Sync class. Change the self.verbose reference to an optional method parameter. Update the existing references to this function to pass the self.verbose since it is no longer available on "self" since the function is no longer contained on the P4Sync class.

Hmmm, so does the patch before this not actually work since
encodeWithUTF8() isn't defined yet? When you reroll this series, you
should swap the order of the patches since the previous patch depends on
this one, not the other way around.

> 
> Modify the functions write_pipe() and p4_write_pipe() to remove the return value.  The return value for both functions is the number of bytes, but the meaning is lost under python3 since the count does not match the number of characters that may have been encoded.  Additionally, the return value was never used, so this is removed to avoid future ambiguity.
> 
> Add a new method gitConfigSet(). This method will set a value in the git configuration cache list.
> 
> Signed-off-by: Ben Keene <[e-mail]>
> (cherry picked from commit affe888f432bb6833df78962e8671fccdf76c47a)
> ---
>  git-p4.py | 60 ++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 44 insertions(+), 16 deletions(-)
> 
> diff --git a/git-p4.py b/git-p4.py
> index b283ef1029..2659531c2e 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -237,6 +237,8 @@ def die(msg):
>          sys.exit(1)
>  
>  def write_pipe(c, stdin):
> +    """ Executes the command 'c', passing 'stdin' on the standard input
> +    """
>      if verbose:
>          sys.stderr.write('Writing pipe: %s\n' % str(c))
>  
> @@ -248,11 +250,12 @@ def write_pipe(c, stdin):
>      if p.wait():
>          die('Command failed: %s' % str(c))
>  
> -    return val
>  
>  def p4_write_pipe(c, stdin):
> +    """ Runs a P4 command 'c', passing 'stdin' data to P4
> +    """
>      real_cmd = p4_build_cmd(c)
> -    return write_pipe(real_cmd, stdin)
> +    write_pipe(real_cmd, stdin)
>  
>  def read_pipe_full(c):
>      """ Read output from  command. Returns a tuple
> @@ -653,6 +656,38 @@ def isModeExec(mode):
>      # otherwise False.
>      return mode[-3:] == "755"
>  
> +def encodeWithUTF8(path, verbose = False):

Nit: no spaces surrounding `=` in default args.

> +    """ Ensure that the path is encoded as a UTF-8 string
> +
> +        Returns bytes(P3)/str(P2)
> +    """
> +   

Trailing whitespace.

> +    if isunicode:
> +        try:
> +            if isinstance(path, unicode):
> +                # It is already unicode, cast it as a bytes
> +                # that is encoded as utf-8.
> +                return path.encode('utf-8', 'strict')
> +            path.decode('ascii', 'strict')
> +        except:
> +            encoding = 'utf8'
> +            if gitConfig('git-p4.pathEncoding'):
> +                encoding = gitConfig('git-p4.pathEncoding')
> +            path = path.decode(encoding, 'replace').encode('utf8', 'replace')
> +            if verbose:
> +                print('\nNOTE:Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, to_unicode(path)))
> +    else:    

Trailing whitespace.

> +        try:
> +            path.decode('ascii')
> +        except:
> +            encoding = 'utf8'
> +            if gitConfig('git-p4.pathEncoding'):
> +                encoding = gitConfig('git-p4.pathEncoding')
> +            path = path.decode(encoding, 'replace').encode('utf8', 'replace')
> +            if verbose:
> +                print('Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, path))
> +    return path
> +
>  class P4Exception(Exception):
>      """ Base class for exceptions from the p4 client """
>      def __init__(self, exit_code):
> @@ -891,6 +926,11 @@ def gitConfigList(key):
>              _gitConfig[key] = []
>      return _gitConfig[key]
>  
> +def gitConfigSet(key, value):
> +    """ Set the git configuration key 'key' to 'value' for this session
> +    """
> +    _gitConfig[key] = value
> +
>  def p4BranchesInGit(branchesAreInRemotes=True):
>      """Find all the branches whose names start with "p4/", looking
>         in remotes or heads as specified by the argument.  Return
> @@ -2814,24 +2854,12 @@ def writeToGitStream(self, gitMode, relPath, contents):
>              self.gitStream.write(d)
>          self.gitStream.write('\n')
>  
> -    def encodeWithUTF8(self, path):
> -        try:
> -            path.decode('ascii')
> -        except:
> -            encoding = 'utf8'
> -            if gitConfig('git-p4.pathEncoding'):
> -                encoding = gitConfig('git-p4.pathEncoding')
> -            path = path.decode(encoding, 'replace').encode('utf8', 'replace')
> -            if self.verbose:
> -                print('Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, path))
> -        return path
> -
>      # output one file from the P4 stream
>      # - helper for streamP4Files
>  
>      def streamOneP4File(self, file, contents):
>          relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
> -        relPath = self.encodeWithUTF8(relPath)
> +        relPath = encodeWithUTF8(relPath, self.verbose)
>          if verbose:
>              if 'fileSize' in self.stream_file:
>                  size = int(self.stream_file['fileSize'])
> @@ -2914,7 +2942,7 @@ def streamOneP4File(self, file, contents):
>  
>      def streamOneP4Deletion(self, file):
>          relPath = self.stripRepoPath(file['path'], self.branchPrefixes)
> -        relPath = self.encodeWithUTF8(relPath)
> +        relPath = encodeWithUTF8(relPath, self.verbose)
>          if verbose:
>              sys.stdout.write("delete %s\n" % relPath)
>              sys.stdout.flush()
> -- 
> gitgitgadget
>

Ben Keene Dec. 5, 2019, 7:23 p.m. UTC | #2

On 12/5/2019 5:50 AM, Denton Liu wrote:
>> Subject: git-p4: Add new functions in preparation of usage
> Nit: as a convention, you should lowercase the letter after the colon in
> the subject. As in "git-p4: add new functions..."
>
> This applies for other patches as well.


Got it.  Changing all leading characters to lower case.


>
> On Wed, Dec 04, 2019 at 10:29:31PM +0000, Ben Keene via GitGitGadget wrote:
>> From: Ben Keene <[e-mail]>
>>
>> This changelist is an intermediate submission for migrating the P4 support from Python2 to Python3. The code needs access to the encodeWithUTF8() for support of non-UTF8 filenames in the clone class as well as the sync class.
>>
>> Move the function encodeWithUTF8() from the P4Sync class to a stand-alone function.  This will allow other classes to use this function without instanciating the P4Sync class. Change the self.verbose reference to an optional method parameter. Update the existing references to this function to pass the self.verbose since it is no longer available on "self" since the function is no longer contained on the P4Sync class.
> Hmmm, so does the patch before this not actually work since
> encodeWithUTF8() isn't defined yet? When you reroll this series, you
> should swap the order of the patches since the previous patch depends on
> this one, not the other way around.

Good catch.  That's correct, the encodeWithUTF8() should be first.  I 
moved that commit earlier in the chain and actually split it up from the 
changes to write_pipe and gitConfigSet() so the text will be easier to see.


>> Modify the functions write_pipe() and p4_write_pipe() to remove the return value.  The return value for both functions is the number of bytes, but the meaning is lost under python3 since the count does not match the number of characters that may have been encoded.  Additionally, the return value was never used, so this is removed to avoid future ambiguity.
>>
>> Add a new method gitConfigSet(). This method will set a value in the git configuration cache list.
>>
>> Signed-off-by: Ben Keene <[e-mail]>
>> (cherry picked from commit affe888f432bb6833df78962e8671fccdf76c47a)
>> ---
>>   git-p4.py | 60 ++++++++++++++++++++++++++++++++++++++++---------------
>>   1 file changed, 44 insertions(+), 16 deletions(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index b283ef1029..2659531c2e 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -237,6 +237,8 @@ def die(msg):
>>           sys.exit(1)
>>   
>>   def write_pipe(c, stdin):
>> +    """ Executes the command 'c', passing 'stdin' on the standard input
>> +    """
>>       if verbose:
>>           sys.stderr.write('Writing pipe: %s\n' % str(c))
>>   
>> @@ -248,11 +250,12 @@ def write_pipe(c, stdin):
>>       if p.wait():
>>           die('Command failed: %s' % str(c))
>>   
>> -    return val
>>   
>>   def p4_write_pipe(c, stdin):
>> +    """ Runs a P4 command 'c', passing 'stdin' data to P4
>> +    """
>>       real_cmd = p4_build_cmd(c)
>> -    return write_pipe(real_cmd, stdin)
>> +    write_pipe(real_cmd, stdin)
>>   
>>   def read_pipe_full(c):
>>       """ Read output from  command. Returns a tuple
>> @@ -653,6 +656,38 @@ def isModeExec(mode):
>>       # otherwise False.
>>       return mode[-3:] == "755"
>>   
>> +def encodeWithUTF8(path, verbose = False):
> Nit: no spaces surrounding `=` in default args.


Fixed


>> +    """ Ensure that the path is encoded as a UTF-8 string
>> +
>> +        Returns bytes(P3)/str(P2)
>> +    """
>> +
> Trailing whitespace.
>
>> +    if isunicode:
>> +        try:
>> +            if isinstance(path, unicode):
>> +                # It is already unicode, cast it as a bytes
>> +                # that is encoded as utf-8.
>> +                return path.encode('utf-8', 'strict')
>> +            path.decode('ascii', 'strict')
>> +        except:
>> +            encoding = 'utf8'
>> +            if gitConfig('git-p4.pathEncoding'):
>> +                encoding = gitConfig('git-p4.pathEncoding')
>> +            path = path.decode(encoding, 'replace').encode('utf8', 'replace')
>> +            if verbose:
>> +                print('\nNOTE:Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, to_unicode(path)))
>> +    else:
> Trailing whitespace.
>
>> +        try:
>> +            path.decode('ascii')
>> +        except:
>> +            encoding = 'utf8'
>> +            if gitConfig('git-p4.pathEncoding'):
>> +                encoding = gitConfig('git-p4.pathEncoding')
>> +            path = path.decode(encoding, 'replace').encode('utf8', 'replace')
>> +            if verbose:
>> +                print('Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, path))
>> +    return path
>> +
>>   class P4Exception(Exception):
>>       """ Base class for exceptions from the p4 client """
>>       def __init__(self, exit_code):
>> @@ -891,6 +926,11 @@ def gitConfigList(key):
>>               _gitConfig[key] = []
>>       return _gitConfig[key]
>>   
>> +def gitConfigSet(key, value):
>> +    """ Set the git configuration key 'key' to 'value' for this session
>> +    """
>> +    _gitConfig[key] = value
>> +
>>   def p4BranchesInGit(branchesAreInRemotes=True):
>>       """Find all the branches whose names start with "p4/", looking
>>          in remotes or heads as specified by the argument.  Return
>> @@ -2814,24 +2854,12 @@ def writeToGitStream(self, gitMode, relPath, contents):
>>               self.gitStream.write(d)
>>           self.gitStream.write('\n')
>>   
>> -    def encodeWithUTF8(self, path):
>> -        try:
>> -            path.decode('ascii')
>> -        except:
>> -            encoding = 'utf8'
>> -            if gitConfig('git-p4.pathEncoding'):
>> -                encoding = gitConfig('git-p4.pathEncoding')
>> -            path = path.decode(encoding, 'replace').encode('utf8', 'replace')
>> -            if self.verbose:
>> -                print('Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, path))
>> -        return path
>> -
>>       # output one file from the P4 stream
>>       # - helper for streamP4Files
>>   
>>       def streamOneP4File(self, file, contents):
>>           relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
>> -        relPath = self.encodeWithUTF8(relPath)
>> +        relPath = encodeWithUTF8(relPath, self.verbose)
>>           if verbose:
>>               if 'fileSize' in self.stream_file:
>>                   size = int(self.stream_file['fileSize'])
>> @@ -2914,7 +2942,7 @@ def streamOneP4File(self, file, contents):
>>   
>>       def streamOneP4Deletion(self, file):
>>           relPath = self.stripRepoPath(file['path'], self.branchPrefixes)
>> -        relPath = self.encodeWithUTF8(relPath)
>> +        relPath = encodeWithUTF8(relPath, self.verbose)
>>           if verbose:
>>               sys.stdout.write("delete %s\n" % relPath)
>>               sys.stdout.flush()
>> -- 
>> gitgitgadget
>>

Patch

diff --git a/git-p4.py b/git-p4.py
index b283ef1029..2659531c2e 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -237,6 +237,8 @@  def die(msg):
         sys.exit(1)
 
 def write_pipe(c, stdin):
+    """ Executes the command 'c', passing 'stdin' on the standard input
+    """
     if verbose:
         sys.stderr.write('Writing pipe: %s\n' % str(c))
 
@@ -248,11 +250,12 @@  def write_pipe(c, stdin):
     if p.wait():
         die('Command failed: %s' % str(c))
 
-    return val
 
 def p4_write_pipe(c, stdin):
+    """ Runs a P4 command 'c', passing 'stdin' data to P4
+    """
     real_cmd = p4_build_cmd(c)
-    return write_pipe(real_cmd, stdin)
+    write_pipe(real_cmd, stdin)
 
 def read_pipe_full(c):
     """ Read output from  command. Returns a tuple
@@ -653,6 +656,38 @@  def isModeExec(mode):
     # otherwise False.
     return mode[-3:] == "755"
 
+def encodeWithUTF8(path, verbose = False):
+    """ Ensure that the path is encoded as a UTF-8 string
+
+        Returns bytes(P3)/str(P2)
+    """
+   
+    if isunicode:
+        try:
+            if isinstance(path, unicode):
+                # It is already unicode, cast it as a bytes
+                # that is encoded as utf-8.
+                return path.encode('utf-8', 'strict')
+            path.decode('ascii', 'strict')
+        except:
+            encoding = 'utf8'
+            if gitConfig('git-p4.pathEncoding'):
+                encoding = gitConfig('git-p4.pathEncoding')
+            path = path.decode(encoding, 'replace').encode('utf8', 'replace')
+            if verbose:
+                print('\nNOTE:Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, to_unicode(path)))
+    else:    
+        try:
+            path.decode('ascii')
+        except:
+            encoding = 'utf8'
+            if gitConfig('git-p4.pathEncoding'):
+                encoding = gitConfig('git-p4.pathEncoding')
+            path = path.decode(encoding, 'replace').encode('utf8', 'replace')
+            if verbose:
+                print('Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, path))
+    return path
+
 class P4Exception(Exception):
     """ Base class for exceptions from the p4 client """
     def __init__(self, exit_code):
@@ -891,6 +926,11 @@  def gitConfigList(key):
             _gitConfig[key] = []
     return _gitConfig[key]
 
+def gitConfigSet(key, value):
+    """ Set the git configuration key 'key' to 'value' for this session
+    """
+    _gitConfig[key] = value
+
 def p4BranchesInGit(branchesAreInRemotes=True):
     """Find all the branches whose names start with "p4/", looking
        in remotes or heads as specified by the argument.  Return
@@ -2814,24 +2854,12 @@  def writeToGitStream(self, gitMode, relPath, contents):
             self.gitStream.write(d)
         self.gitStream.write('\n')
 
-    def encodeWithUTF8(self, path):
-        try:
-            path.decode('ascii')
-        except:
-            encoding = 'utf8'
-            if gitConfig('git-p4.pathEncoding'):
-                encoding = gitConfig('git-p4.pathEncoding')
-            path = path.decode(encoding, 'replace').encode('utf8', 'replace')
-            if self.verbose:
-                print('Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, path))
-        return path
-
     # output one file from the P4 stream
     # - helper for streamP4Files
 
     def streamOneP4File(self, file, contents):
         relPath = self.stripRepoPath(file['depotFile'], self.branchPrefixes)
-        relPath = self.encodeWithUTF8(relPath)
+        relPath = encodeWithUTF8(relPath, self.verbose)
         if verbose:
             if 'fileSize' in self.stream_file:
                 size = int(self.stream_file['fileSize'])
@@ -2914,7 +2942,7 @@  def streamOneP4File(self, file, contents):
 
     def streamOneP4Deletion(self, file):
         relPath = self.stripRepoPath(file['path'], self.branchPrefixes)
-        relPath = self.encodeWithUTF8(relPath)
+        relPath = encodeWithUTF8(relPath, self.verbose)
         if verbose:
             sys.stdout.write("delete %s\n" % relPath)
             sys.stdout.flush()