[v4,06/11] git-p4: Fix assumed path separators to be more Windows friendly
diff mbox series

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

Commit Message

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

When a computer is configured to use Git for windows and Python for windows, and not a Unix subsystem like cygwin or WSL, the directory separator changes and causes git-p4 to fail to properly determine paths.

Fix 3 path separator errors:

1. getUserCacheFilename should not use string concatenation. Change this code to use os.path.join to build an OS tolerant path.
2. defaultDestiantion used the OS.path.split to split depot paths.  This is incorrect on windows. Change the code to split on a forward slash(/) instead since depot paths use this character regardless  of the operating system.
3. The call to isvalidGitDir() in the main code also used a literal forward slash. Change the cose to use os.path.join to correctly format the path for the operating system.

These three changes allow the suggested windows configuration to properly locate files while retaining the existing behavior on non-windows operating systems.

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

Comments

Junio C Hamano Dec. 5, 2019, 1:38 p.m. UTC | #1
"Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ben Keene <seraphire@gmail.com>
>
> When a computer is configured to use Git for windows and Python for windows, and not a Unix subsystem like cygwin or WSL, the directory separator changes and causes git-p4 to fail to properly determine paths.
>
> Fix 3 path separator errors:
>
> 1. getUserCacheFilename should not use string concatenation. Change this code to use os.path.join to build an OS tolerant path.
> 2. defaultDestiantion used the OS.path.split to split depot paths.  This is incorrect on windows. Change the code to split on a forward slash(/) instead since depot paths use this character regardless  of the operating system.
> 3. The call to isvalidGitDir() in the main code also used a literal forward slash. Change the cose to use os.path.join to correctly format the path for the operating system.

s/isvalid/isValid/;
s/cose/code/; 

Also please wrap your lines at around 72 columns (that will let
reviewers quote what you write (which adds "> " prefix and consumes
2 more columns), and would allow us a handful of exchanges (each
round adding ">" prefix to consume 1 more column) before bumping
into the right edge of the terminal at 80 columns.

> These three changes allow the suggested windows configuration to properly locate files while retaining the existing behavior on non-windows operating systems.
>
> Signed-off-by: Ben Keene <seraphire@gmail.com>
> (cherry picked from commit a5b45c12c3861638a933b05a1ffee0c83978dcb2)

As Denton mentioned, general public do not care if you "cherry
picked" it from your earlier unpublished work.  Remove it.

Aside from these small nits, the proposed log message for this step
is quite cleanly done and easily readable.  All the decisions are
clearly written and agreeable.  Nicely done.

> ---
>  git-p4.py | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 2659531c2e..7ac8cb42ef 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1454,8 +1454,10 @@ def p4UserIsMe(self, p4User):
>              return True
>  
>      def getUserCacheFilename(self):
> +        """ Returns the filename of the username cache 
> +	    """

Inconsistent use of spaces and a tab I see on these two lines.
Intended?

>          home = os.environ.get("HOME", os.environ.get("USERPROFILE"))
> -        return home + "/.gitp4-usercache.txt"
> +        return os.path.join(home, ".gitp4-usercache.txt")
>  
>      def getUserMapFromPerforceServer(self):
>          if self.userMapFromPerforceServer:
> @@ -3973,13 +3975,16 @@ def __init__(self):
>          self.cloneBare = False
>  
>      def defaultDestination(self, args):
> +        """ Returns the last path component as the default git 
> +            repository directory name
> +        """
>          ## TODO: use common prefix of args?
>          depotPath = args[0]
>          depotDir = re.sub("(@[^@]*)$", "", depotPath)
>          depotDir = re.sub("(#[^#]*)$", "", depotDir)
>          depotDir = re.sub(r"\.\.\.$", "", depotDir)
>          depotDir = re.sub(r"/$", "", depotDir)
> -        return os.path.split(depotDir)[1]
> +        return depotDir.split('/')[-1]
>  
>      def run(self, args):
>          if len(args) < 1:
> @@ -4252,8 +4257,8 @@ def main():
>                          chdir(cdup);
>  
>          if not isValidGitDir(cmd.gitdir):
> -            if isValidGitDir(cmd.gitdir + "/.git"):
> -                cmd.gitdir += "/.git"
> +            if isValidGitDir(os.path.join(cmd.gitdir, ".git")):
> +                cmd.gitdir = os.path.join(cmd.gitdir, ".git")
>              else:
>                  die("fatal: cannot locate git repository at %s" % cmd.gitdir)
Ben Keene Dec. 5, 2019, 7:37 p.m. UTC | #2
On 12/5/2019 8:38 AM, Junio C Hamano wrote:
> "Ben Keene via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Ben Keene <seraphire@gmail.com>
>>
>> When a computer is configured to use Git for windows and Python for windows, and not a Unix subsystem like cygwin or WSL, the directory separator changes and causes git-p4 to fail to properly determine paths.
>>
>> Fix 3 path separator errors:
>>
>> 1. getUserCacheFilename should not use string concatenation. Change this code to use os.path.join to build an OS tolerant path.
>> 2. defaultDestiantion used the OS.path.split to split depot paths.  This is incorrect on windows. Change the code to split on a forward slash(/) instead since depot paths use this character regardless  of the operating system.
>> 3. The call to isvalidGitDir() in the main code also used a literal forward slash. Change the cose to use os.path.join to correctly format the path for the operating system.
> s/isvalid/isValid/;
> s/cose/code/;
>
> Also please wrap your lines at around 72 columns (that will let
> reviewers quote what you write (which adds "> " prefix and consumes
> 2 more columns), and would allow us a handful of exchanges (each
> round adding ">" prefix to consume 1 more column) before bumping
> into the right edge of the terminal at 80 columns.
>
>> These three changes allow the suggested windows configuration to properly locate files while retaining the existing behavior on non-windows operating systems.
>>
>> Signed-off-by: Ben Keene <seraphire@gmail.com>
>> (cherry picked from commit a5b45c12c3861638a933b05a1ffee0c83978dcb2)
> As Denton mentioned, general public do not care if you "cherry
> picked" it from your earlier unpublished work.  Remove it.
>
> Aside from these small nits, the proposed log message for this step
> is quite cleanly done and easily readable.  All the decisions are
> clearly written and agreeable.  Nicely done.


Thank you. I've been working through all the commits and updating them.


>> ---
>>   git-p4.py | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 2659531c2e..7ac8cb42ef 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -1454,8 +1454,10 @@ def p4UserIsMe(self, p4User):
>>               return True
>>   
>>       def getUserCacheFilename(self):
>> +        """ Returns the filename of the username cache
>> +	    """
> Inconsistent use of spaces and a tab I see on these two lines.
> Intended?

Good catch! It should have been spaces.  Corrected.


>
>>           home = os.environ.get("HOME", os.environ.get("USERPROFILE"))
>> -        return home + "/.gitp4-usercache.txt"
>> +        return os.path.join(home, ".gitp4-usercache.txt")
>>   
>>       def getUserMapFromPerforceServer(self):
>>           if self.userMapFromPerforceServer:
>> @@ -3973,13 +3975,16 @@ def __init__(self):
>>           self.cloneBare = False
>>   
>>       def defaultDestination(self, args):
>> +        """ Returns the last path component as the default git
>> +            repository directory name
>> +        """
>>           ## TODO: use common prefix of args?
>>           depotPath = args[0]
>>           depotDir = re.sub("(@[^@]*)$", "", depotPath)
>>           depotDir = re.sub("(#[^#]*)$", "", depotDir)
>>           depotDir = re.sub(r"\.\.\.$", "", depotDir)
>>           depotDir = re.sub(r"/$", "", depotDir)
>> -        return os.path.split(depotDir)[1]
>> +        return depotDir.split('/')[-1]
>>   
>>       def run(self, args):
>>           if len(args) < 1:
>> @@ -4252,8 +4257,8 @@ def main():
>>                           chdir(cdup);
>>   
>>           if not isValidGitDir(cmd.gitdir):
>> -            if isValidGitDir(cmd.gitdir + "/.git"):
>> -                cmd.gitdir += "/.git"
>> +            if isValidGitDir(os.path.join(cmd.gitdir, ".git")):
>> +                cmd.gitdir = os.path.join(cmd.gitdir, ".git")
>>               else:
>>                   die("fatal: cannot locate git repository at %s" % cmd.gitdir)

Patch
diff mbox series

diff --git a/git-p4.py b/git-p4.py
index 2659531c2e..7ac8cb42ef 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1454,8 +1454,10 @@  def p4UserIsMe(self, p4User):
             return True
 
     def getUserCacheFilename(self):
+        """ Returns the filename of the username cache 
+	    """
         home = os.environ.get("HOME", os.environ.get("USERPROFILE"))
-        return home + "/.gitp4-usercache.txt"
+        return os.path.join(home, ".gitp4-usercache.txt")
 
     def getUserMapFromPerforceServer(self):
         if self.userMapFromPerforceServer:
@@ -3973,13 +3975,16 @@  def __init__(self):
         self.cloneBare = False
 
     def defaultDestination(self, args):
+        """ Returns the last path component as the default git 
+            repository directory name
+        """
         ## TODO: use common prefix of args?
         depotPath = args[0]
         depotDir = re.sub("(@[^@]*)$", "", depotPath)
         depotDir = re.sub("(#[^#]*)$", "", depotDir)
         depotDir = re.sub(r"\.\.\.$", "", depotDir)
         depotDir = re.sub(r"/$", "", depotDir)
-        return os.path.split(depotDir)[1]
+        return depotDir.split('/')[-1]
 
     def run(self, args):
         if len(args) < 1:
@@ -4252,8 +4257,8 @@  def main():
                         chdir(cdup);
 
         if not isValidGitDir(cmd.gitdir):
-            if isValidGitDir(cmd.gitdir + "/.git"):
-                cmd.gitdir += "/.git"
+            if isValidGitDir(os.path.join(cmd.gitdir, ".git")):
+                cmd.gitdir = os.path.join(cmd.gitdir, ".git")
             else:
                 die("fatal: cannot locate git repository at %s" % cmd.gitdir)